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
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. |
WalkthroughThe change centralizes BareMetalHost and DataImage helpers, adds post-cleanup annotation handling, and gates installation completion on DataImage deletion. The monitor requeues while deletion is pending and updates installation conditions. ChangesDataImage cleanup lifecycle
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 11❌ Failed checks (1 warning, 10 inconclusive)
✅ Passed checks (4 passed)
✨ 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.
|
/testwith openshift/image-based-install-operator/main/e2e-ibio #844 openshift/release#80991 |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
The CI failure seem to be from the seed image. |
|
/hold Putting a hold here until we understand how this affects backup/restore. |
openshift-ci
Bot
added
the
do-not-merge/hold
…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. To avoid a race condition that can occur when restoring an ICI that isn't complete, we also annotate new ICIs with a post-cleanup annotation and make sure their DataImages were deleted after they are installed. The race condition can occur when the main reconciler recreates the DataImage after the Monitor reconciler deletes it.
7e1d6a7 to
13f216f
Compare
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 `@controllers/imageclusterinstall_controller_test.go`:
- Around line 2333-2348: Add meaningful failure messages to the lifecycle
assertions in controllers/imageclusterinstall_controller_test.go:2333-2348
identifying the expected post-cleanup annotation; 2350-2366 identifying
restore-path annotation exclusion; 2368-2410 identifying required DataImage
deletion; and 2412-2453 identifying required DataImage retention and annotation
absence. Update the existing assertions in these ranges without changing their
behavior.
🪄 Autofix
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: 14952c02-e16f-40cc-8f93-42e361ec3c80
📒 Files selected for processing (5)
controllers/common.gocontrollers/imageclusterinstall_controller.gocontrollers/imageclusterinstall_controller_test.gocontrollers/imageclusterinstall_monitor.gocontrollers/imageclusterinstall_monitor_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- controllers/imageclusterinstall_monitor_test.go
- controllers/imageclusterinstall_monitor.go
| It("adds DataImage cleanup annotation to new ImageClusterInstalls", func() { | ||
| Expect(c.Create(ctx, clusterInstall)).To(Succeed()) | ||
| Expect(c.Create(ctx, clusterDeployment)).To(Succeed()) | ||
|
|
||
| key := types.NamespacedName{ | ||
| Namespace: clusterInstallNamespace, | ||
| Name: clusterInstallName, | ||
| } | ||
| installerSuccess() | ||
| res, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: key}) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| Expect(res).To(Equal(ctrl.Result{})) | ||
|
|
||
| Expect(c.Get(ctx, key, clusterInstall)).To(Succeed()) | ||
| Expect(clusterInstall.Annotations).To(HaveKeyWithValue(postCleanupAnnotation, postCleanupAnnotationValue)) | ||
| }) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add failure messages to the new lifecycle assertions.
controllers/imageclusterinstall_controller_test.go#L2333-L2348: add messages that identify the expected post-cleanup annotation.controllers/imageclusterinstall_controller_test.go#L2350-L2366: add messages that identify restore-path annotation exclusion.controllers/imageclusterinstall_controller_test.go#L2368-L2410: add messages that identify required DataImage deletion.controllers/imageclusterinstall_controller_test.go#L2412-L2453: add messages that identify required DataImage retention and annotation absence.
As per coding guidelines, “Assertion messages - include meaningful failure messages; flag assertions without messages.”
📍 Affects 1 file
controllers/imageclusterinstall_controller_test.go#L2333-L2348(this comment)controllers/imageclusterinstall_controller_test.go#L2350-L2366controllers/imageclusterinstall_controller_test.go#L2368-L2410controllers/imageclusterinstall_controller_test.go#L2412-L2453
🤖 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_controller_test.go` around lines 2333 - 2348,
Add meaningful failure messages to the lifecycle assertions in
controllers/imageclusterinstall_controller_test.go:2333-2348 identifying the
expected post-cleanup annotation; 2350-2366 identifying restore-path annotation
exclusion; 2368-2410 identifying required DataImage deletion; and 2412-2453
identifying required DataImage retention and annotation absence. Update the
existing assertions in these ranges without changing their behavior.
Source: Coding guidelines
|
@giladravid16: The following tests 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. |
3 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.
To avoid a race condition that can occur when restoring an ICI that isn't complete, we also annotate new ICIs with a post-cleanup annotation and make sure their DataImages were deleted after they are installed.
The race condition can occur when the main reconciler recreates the DataImage after the Monitor reconciler deletes it.
Summary by CodeRabbit
New Features
Bug Fixes