MGMT-24419: Delete DataImage after cluster is installed and remove detached annotation#844
MGMT-24419: Delete DataImage after cluster is installed and remove detached annotation#844giladravid16 wants to merge 1 commit into
Conversation
…tached annotation Having the detached annotation prevents users from performing hardware lifecycle operations, and keeping the DataImage causes the BMO to remount it after firmware updates. So we want to remove both of them.
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
@giladravid16: This pull request references MGMT-24419 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target either version "5.0." or "openshift-5.0.", but it targets "ACM 5.0" instead. 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. |
WalkthroughA new ChangesBMH/DataImage helper extraction and monitor install-completion gate
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: giladravid16 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
|
/test e2e-ibio |
![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
🧹 Nitpick comments (1)
controllers/imageclusterinstall_monitor_test.go (1)
200-205: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert detached annotation removal too.
This updated assertion verifies rebooting, but the fixture BMH never has
detachedAnnotation, so the test would miss a regression where the annotation remains and blocks lifecycle operations. Seed it before reconcile and assert it is absent here.🤖 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 `@controllers/imageclusterinstall_monitor_test.go` around lines 200 - 205, The test in imageclusterinstall_monitor_test.go only checks rebootAnnotation on the BMH after DataImage removal, so it can miss a regression where detachedAnnotation is still present. Update the test setup around the bmh fixture and reconcile path to seed detachedAnnotation before the operation, then extend the existing BMH assertions to verify that detachedAnnotation has been removed alongside the reboot behavior. Use the same bmh object and the existing annotation checks in this test to keep the assertion aligned with the DataImage removal flow.
🤖 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 `@controllers/common.go`:
- Around line 60-74: In removeBMHDataImage, detached annotation cleanup is
skipped when deleteDataImage returns nil, so the BareMetalHost is never fetched
in the already-missing DataImage case. Update the flow so the BareMetalHost
lookup and detachedAnnotation removal always happen after deleteDataImage, and
only call attachAndRebootBMH when a DataImage deletion was actually initiated
and dataImage is non-nil. Use removeBMHDataImage, deleteDataImage, and
attachAndRebootBMH to locate the logic.
---
Nitpick comments:
In `@controllers/imageclusterinstall_monitor_test.go`:
- Around line 200-205: The test in imageclusterinstall_monitor_test.go only
checks rebootAnnotation on the BMH after DataImage removal, so it can miss a
regression where detachedAnnotation is still present. Update the test setup
around the bmh fixture and reconcile path to seed detachedAnnotation before the
operation, then extend the existing BMH assertions to verify that
detachedAnnotation has been removed alongside the reboot behavior. Use the same
bmh object and the existing annotation checks in this test to keep the assertion
aligned with the DataImage removal flow.
🪄 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: 909251f3-c3a2-405e-aa52-143d0e250f69
📒 Files selected for processing (4)
controllers/common.gocontrollers/imageclusterinstall_controller.gocontrollers/imageclusterinstall_monitor.gocontrollers/imageclusterinstall_monitor_test.go
| func removeBMHDataImage(ctx context.Context, c client.Client, log logrus.FieldLogger, bmhRef types.NamespacedName) (*bmh_v1alpha1.DataImage, error) { | ||
| dataImage, err := deleteDataImage(ctx, c, log, bmhRef) | ||
| if err != nil || dataImage == nil { | ||
| return dataImage, err | ||
| } | ||
|
|
||
| bmh := &bmh_v1alpha1.BareMetalHost{} | ||
| if err := c.Get(ctx, bmhRef, bmh); err != nil { | ||
| if k8sapierrors.IsNotFound(err) { | ||
| log.Warnf("Referenced BareMetalHost %s/%s does not exist, not waiting for dataImage deletion", bmhRef.Namespace, bmhRef.Name) | ||
| return nil, nil | ||
| } | ||
| return dataImage, fmt.Errorf("failed to get BareMetalHost %s/%s: %w", bmhRef.Namespace, bmhRef.Name, err) | ||
| } | ||
| return dataImage, attachAndRebootBMH(ctx, c, log, bmh) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Decouple detached-annotation cleanup from DataImage existence.
When deleteDataImage returns nil for an already-missing DataImage, Line 62 returns before fetching the BMH, so detachedAnnotation is never removed. That leaves the lifecycle-blocking annotation in exactly the already-cleaned-up/DataImage-missing case this PR should handle.
Possible fix direction
dataImage, err := deleteDataImage(ctx, c, log, bmhRef)
- if err != nil || dataImage == nil {
+ if err != nil {
return dataImage, err
}
bmh := &bmh_v1alpha1.BareMetalHost{}Then patch the BMH so detached removal always runs, while adding the reboot annotation only when a DataImage deletion was actually requested.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func removeBMHDataImage(ctx context.Context, c client.Client, log logrus.FieldLogger, bmhRef types.NamespacedName) (*bmh_v1alpha1.DataImage, error) { | |
| dataImage, err := deleteDataImage(ctx, c, log, bmhRef) | |
| if err != nil || dataImage == nil { | |
| return dataImage, err | |
| } | |
| bmh := &bmh_v1alpha1.BareMetalHost{} | |
| if err := c.Get(ctx, bmhRef, bmh); err != nil { | |
| if k8sapierrors.IsNotFound(err) { | |
| log.Warnf("Referenced BareMetalHost %s/%s does not exist, not waiting for dataImage deletion", bmhRef.Namespace, bmhRef.Name) | |
| return nil, nil | |
| } | |
| return dataImage, fmt.Errorf("failed to get BareMetalHost %s/%s: %w", bmhRef.Namespace, bmhRef.Name, err) | |
| } | |
| return dataImage, attachAndRebootBMH(ctx, c, log, bmh) | |
| func removeBMHDataImage(ctx context.Context, c client.Client, log logrus.FieldLogger, bmhRef types.NamespacedName) (*bmh_v1alpha1.DataImage, error) { | |
| dataImage, err := deleteDataImage(ctx, c, log, bmhRef) | |
| if err != nil { | |
| return dataImage, err | |
| } | |
| bmh := &bmh_v1alpha1.BareMetalHost{} | |
| if err := c.Get(ctx, bmhRef, bmh); err != nil { | |
| if k8sapierrors.IsNotFound(err) { | |
| log.Warnf("Referenced BareMetalHost %s/%s does not exist, not waiting for dataImage deletion", bmhRef.Namespace, bmhRef.Name) | |
| return nil, nil | |
| } | |
| return dataImage, fmt.Errorf("failed to get BareMetalHost %s/%s: %w", bmhRef.Namespace, bmhRef.Name, err) | |
| } | |
| return dataImage, attachAndRebootBMH(ctx, c, log, bmh) | |
| } |
🤖 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 `@controllers/common.go` around lines 60 - 74, In removeBMHDataImage, detached
annotation cleanup is skipped when deleteDataImage returns nil, so the
BareMetalHost is never fetched in the already-missing DataImage case. Update the
flow so the BareMetalHost lookup and detachedAnnotation removal always happen
after deleteDataImage, and only call attachAndRebootBMH when a DataImage
deletion was actually initiated and dataImage is non-nil. Use
removeBMHDataImage, deleteDataImage, and attachAndRebootBMH to locate the logic.
|
@giladravid16: 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. |
|
/testwith openshift/image-based-install-operator/main/e2e-ibio #844 openshift/release#80991 |
2 participants
Having the detached annotation prevents users from performing hardware lifecycle operations, and keeping the DataImage causes the BMO to remount it after firmware updates. So we want to remove both of them.
Summary by CodeRabbit
New Features
Bug Fixes