DNM - for testing OCPBUGS-65645#6016
Conversation
Resolves: OCPBUGS-65645 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds an EL-only verification step during first-boot "Done" transition: the daemon checks that MachineConfig.Spec.Extensions packages exist in the RPM DB and aborts the Done transition if verification fails; non-EL systems skip the check. ChangesExtension package verification + Done transition gate
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: isabella-janssen 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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/daemon/update.go`:
- Around line 1876-1891: The MCO_TEST_EXTENSION_FAILURE injection is only
checked inside the non-ExitCode==1 error branch so it never triggers on
successful rpm queries; after running exec.Command("rpm","-q",
pkg).CombinedOutput() (i.e., after obtaining out and err) move the
os.Getenv("MCO_TEST_EXTENSION_FAILURE") == "true" check out of the error-only
branch so it runs for the healthy path as well—if set, append "test-failing" to
missingPackages (and optionally klog.Warningf) before continuing; keep the
existing exitErr/ExitCode()==1 handling and the error return for other failures
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 576b5f79-08d6-484b-acd5-b5ba4389ef55
📒 Files selected for processing (2)
pkg/daemon/daemon.gopkg/daemon/update.go
| out, err := exec.Command("rpm", "-q", pkg).CombinedOutput() | ||
| if err == nil { | ||
| continue | ||
| } | ||
| // Check if this is exit code 1 (package not installed) vs other errors | ||
| if errors.As(err, &exitErr) && exitErr.ExitCode() == 1 { | ||
| missingPackages = append(missingPackages, pkg) | ||
| klog.Warningf("Extension package %s not found in RPM database", pkg) | ||
| continue | ||
| } | ||
| if os.Getenv("MCO_TEST_EXTENSION_FAILURE") == "true" { | ||
| missingPackages = append(missingPackages, "test-failing") | ||
| } | ||
|
|
||
| // Other errors (execution failure, permission issues, etc.) should fail immediately | ||
| return fmt.Errorf("failed to query RPM database for package %q: %v: %s", pkg, err, strings.TrimSpace(string(out))) |
There was a problem hiding this comment.
Test failure injection never triggers in the healthy path.
MCO_TEST_EXTENSION_FAILURE is only checked after rpm -q has already failed with a non-ExitCode()==1 error, so a normal successful verification can never be force-failed. If this hook is meant to exercise the new post-reboot guard, move the env-var check outside this error branch.
Suggested fix
func (dn *CoreOSDaemon) verifyExtensionPackages(config *mcfgv1.MachineConfig) error {
// Only verify on RHCOS/SCOS nodes
if !dn.os.IsEL() {
return nil
}
+
+ if os.Getenv("MCO_TEST_EXTENSION_FAILURE") == "true" {
+ return fmt.Errorf("injected extension verification failure")
+ }
// Get the list of extensions from the config
extensions := config.Spec.Extensions
@@
- if os.Getenv("MCO_TEST_EXTENSION_FAILURE") == "true" {
- missingPackages = append(missingPackages, "test-failing")
- }
-
// Other errors (execution failure, permission issues, etc.) should fail immediately
return fmt.Errorf("failed to query RPM database for package %q: %v: %s", pkg, err, strings.TrimSpace(string(out)))
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/daemon/update.go` around lines 1876 - 1891, The
MCO_TEST_EXTENSION_FAILURE injection is only checked inside the non-ExitCode==1
error branch so it never triggers on successful rpm queries; after running
exec.Command("rpm","-q", pkg).CombinedOutput() (i.e., after obtaining out and
err) move the os.Getenv("MCO_TEST_EXTENSION_FAILURE") == "true" check out of the
error-only branch so it runs for the healthy path as well—if set, append
"test-failing" to missingPackages (and optionally klog.Warningf) before
continuing; keep the existing exitErr/ExitCode()==1 handling and the error
return for other failures unchanged.
1 participant
- What I did
- How to verify it
- Description for the changelog
Summary by CodeRabbit