OCPBUGS-84842: Bootloader update attempt should be conditional#5907
OCPBUGS-84842: Bootloader update attempt should be conditional#5907djoshy wants to merge 2 commits intoopenshift:mainfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
jira/valid-reference
|
@djoshy: This pull request references Jira Issue OCPBUGS-84842, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (3)
WalkthroughThis change introduces Secure Boot shim safety detection for bootloader updates and standardizes architecture constant usage across the codebase. It adds shim RPM version comparison logic to verify minimum versions before running bootloader updates, and centralizes Go architecture identifiers into shared constants. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Review rate limit: 9/10 reviews remaining, refill in 6 minutes. Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: djoshy The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
@djoshy: This pull request references Jira Issue OCPBUGS-84842, which is valid. 3 validation(s) were run on this bug
DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
| // compareNumericSegments extracts all runs of digits from each string and | ||
| // compares them pairwise as integers. Non-digit characters act as separators. | ||
| // "15.8" → [15,8], "3.el9_2" → [3,9,2]. | ||
| func compareNumericSegments(a, b string) int { |
There was a problem hiding this comment.
I asked claude to simplify this - the original comparison it generated was far too complex. Would really like an RHCOS engineer to let me know if I'm missing something though 😄
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/daemon/bootupd.go (1)
243-251: 💤 Low valueMinor: Consider using null-delimited find output for robustness.
The
find | xargs sha256sumpattern could fail with filenames containing spaces, though EFI files typically have standard names. Since this is logging-only and doesn't affect the update, it's acceptable.If you want more robustness for logging:
- beforeSums, _ := runGetOut("sh", "-c", "find /boot/efi -type f | xargs sha256sum") + beforeSums, _ := runGetOut("sh", "-c", "find /boot/efi -type f -print0 | xargs -0 sha256sum")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/daemon/bootupd.go` around lines 243 - 251, The logging of SHA256 sums uses runGetOut to execute a shell pipeline ("find /boot/efi -type f | xargs sha256sum") which can break on filenames with whitespace; update the commands that produce beforeSums and afterSums (the runGetOut invocations) to use a null-delimited find output combined with a null-aware xargs invocation so filenames with spaces are handled robustly, leaving runCmdSync and logSystem calls unchanged; adjust both the beforeSums and afterSums invocations to the null-delimited variant to keep them symmetric and robust.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/daemon/bootupd.go`:
- Around line 243-251: The logging of SHA256 sums uses runGetOut to execute a
shell pipeline ("find /boot/efi -type f | xargs sha256sum") which can break on
filenames with whitespace; update the commands that produce beforeSums and
afterSums (the runGetOut invocations) to use a null-delimited find output
combined with a null-aware xargs invocation so filenames with spaces are handled
robustly, leaving runCmdSync and logSystem calls unchanged; adjust both the
beforeSums and afterSums invocations to the null-delimited variant to keep them
symmetric and robust.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 250c0400-01f2-4bb9-900f-ff5500552a89
📒 Files selected for processing (4)
.snykpkg/daemon/bootupd.gopkg/daemon/bootupd_test.gopkg/daemon/update.go
djoshy
force-pushed
the
bootloader-update-cond
branch
from
c8e84a8 to
63717bc
Compare
|
@djoshy: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
- What I did
Follow-up fix to #5868; this adds a simple shim version check so the bootloader update routine isn't invoked on every node update.
- How to verify it
Bootloader updates should continue to work, but they should not be attempted for shims >
15.8-3.el9_2, i.e. any cluster installed past4.15.23or4.16.0. All bootimages used after those releases have the minimum version that contains the Secure Boot signing keys. This should be verified onamd64andaarch64clusters.Summary by CodeRabbit
Release Notes
New Features
Bug Fixes