ACM-44197: Disable DataImage deletion by default

[giladravid16]

For performance reasons, deleting the DataImage after the cluster is installed will now only happen as a post-installation step if the ICI has the post-cleanup annotation, which will not be added by our controllers.
Because of this we will also go back to detaching the BMH at the end of the installation, and if post-cleanup is done then it will remove the detached annotation as well.

Summary by CodeRabbit

• New Features

  • Data images referenced by installations are now labeled for backup, including newly created images.
  • Data images are retained after successful installation for improved backup and restore workflows.
  • Completed installations now mark the associated host as detached.

• Bug Fixes

  • Cleanup runs only when explicitly enabled.
  • Host annotations are updated consistently, avoiding unnecessary updates and unintended reboot actions.

[openshift-ci-robot]

@giladravid16: This pull request references ACM-44197 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 bug to target the "5.1.0" version, but no target version was set.

Details

In response to this:

For performance reasons, deleting the DataImage after the cluster is installed will now only happen as a post-installation step if the ICI has the post-cleanup annotation, which will not be added by our controllers.
Because of this we will also go back to detaching the BMH at the end of the installation, and if post-cleanup is done then it will remove the detached annotation as well.

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.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: b25f9dbc-6bdb-489d-936b-619476fde38b

📥 Commits

Reviewing files that changed from the base of the PR and between ea04be0 and 901cf16.

📒 Files selected for processing (5)

• controllers/common.go
• controllers/imageclusterinstall_controller.go
• controllers/imageclusterinstall_controller_test.go
• controllers/imageclusterinstall_monitor.go
• controllers/imageclusterinstall_monitor_test.go

Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.

---

Walkthrough

The installation controllers now retain DataImages, apply backup labels, detach BareMetalHosts after installation, and use explicit post-cleanup annotation values. Reconciliation no longer adds the post-cleanup annotation automatically.

Changes

Installation lifecycle changes

Layer / File(s)	Summary
Annotation and cleanup behavior controllers/common.go, controllers/imageclusterinstall_controller.go, controllers/imageclusterinstall_controller_test.go	Cleanup now requires the post-cleanup annotation to equal "true". BareMetalHost annotation updates remove detached state before reboot and patch only when needed. Reconciliation no longer adds the post-cleanup annotation.
DataImage backup labeling controllers/imageclusterinstall_controller.go, controllers/imageclusterinstall_controller_test.go	Referenced and newly created DataImages receive the backup label. Tests cover existing and newly created DataImages.
Post-installation monitoring controllers/imageclusterinstall_monitor.go, controllers/imageclusterinstall_monitor_test.go	The monitor no longer deletes or waits for DataImages. After installation, it adds the detached annotation to the BareMetalHost and completes without reboot-triggered cleanup.

Priority: ⬇️ Low — Defer this installation-controller change because it only alters DataImage retention, backup labeling, and BareMetalHost detachment without supplied evidence of elevated product or external urgency.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 901cf

This change retains DataImages by default, labels them for backup, and detaches BareMetalHosts after installation while limiting cleanup to explicit opt-in. No current merge-blocking risk remains.

Sequence Diagram(s)

sequenceDiagram
  participant ImageClusterInstallReconciler
  participant DataImage
  participant ImageClusterInstallMonitor
  participant BareMetalHost
  ImageClusterInstallReconciler->>DataImage: apply backup label
  ImageClusterInstallMonitor->>BareMetalHost: add detached annotation
  ImageClusterInstallMonitor-->>DataImage: retain DataImage

Loading

🚥 Pre-merge checks | ✅ 13 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 5 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Test Structure And Quality	⚠️ Warning	The changed monitor tests add several assertions without meaningful failure messages. For example, controllers/imageclusterinstall_monitor_test.go:201, :205-206, :241, and :245-246 assert Data…	Add a diagnostic message to each new or changed monitor assertion. State the expected operation and resource, for example "DataImage should remain after installation", "BMH should receive the detached annotation", and `"BMH should not r…

✅ Passed checks (13 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the primary change: DataImage deletion is disabled by default, with deletion limited to post-installation cleanup when explicitly enabled.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names	✅ Passed	PASS. The pull request changes six Ginkgo test titles. Each title is a static string, such as "labels newly created DataImage for backup" and "marks cluster installed and keeps DataImage when post-cle…
Microshift Test Compatibility	✅ Passed	No new Ginkgo e2e tests were added. The pull request only modifies existing tests in the controllers package, which use controller-runtime fake clients and RunSpecs; the diff changes existing It…
Single Node Openshift (Sno) Test Compatibility	✅ Passed	PASS: The pull request adds no new Ginkgo e2e tests. It only updates existing Ginkgo tests in the controllers package, which use a fake Kubernetes client and test controller resources. The changed t…
Topology-Aware Scheduling Compatibility	✅ Passed	The pull request changes only controller logic and tests. The diff adds annotation handling, DataImage backup labels, DataImage cleanup behavior, and BareMetalHost patching. It adds no anti-affinity, …
Ote Binary Stdout Contract	✅ Passed	PASS. The pull request changes only controller reconciliation code and Ginkgo test cases. Added output calls use the injected logrus.FieldLogger inside reconciliation helpers, not main(), init()…
Ipv6 And Disconnected Network Test Compatibility	✅ Passed	PASS — The PR changes only controller unit tests under controllers/; it adds no e2e test. The changed Ginkgo declarations add assertions for labels, annotations, and fake-client reconciliation. No a…
No-Weak-Crypto	✅ Passed	PASS: The pull-request diff adds no MD5, SHA1, DES, RC4, 3DES, Blowfish, or ECB usage. The existing crypto/sha256 and crypto/sha512 imports are unchanged. The added comparisons only check annotation v…
Container-Privileges	✅ Passed	PASS. The pull request changes only five Go source/test files. It adds no container or Kubernetes manifest changes and adds no privilege-related settings. The existing manager manifest sets hostIPC, h…
No-Sensitive-Data-In-Logs	✅ Passed	PASS. The pull request adds logs for Kubernetes resource names and namespaces when it detaches a BareMetalHost or labels a DataImage. It does not log passwords, tokens, API keys, PII, session IDs, URL…

Full details: Test Structure And Quality

Explanation

The changed monitor tests add several assertions without meaningful failure messages. For example, controllers/imageclusterinstall_monitor_test.go:201, :205-206, :241, and :245-246 assert DataImage retention and BMH annotations without diagnostic messages. This violates the assertion-message requirement. The tests use isolated fake clients and an AfterEach filesystem cleanup, and the changed sections add no indefinite Eventually or Consistently calls.

Resolution

Add a diagnostic message to each new or changed monitor assertion. State the expected operation and resource, for example "DataImage should remain after installation", "BMH should receive the detached annotation", and "BMH should not receive the reboot annotation". Apply the same rule to the result, condition, and resource-fetch assertions in the modified test cases.

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

Warning

Some tools did not complete. Review the errors below.

🔧 golangci-lint (2.13.2)

level=error msg="Running error: context loading failed: failed to load packages: failed to load packages: failed to load with go/packages: context deadline exceeded"
level=error msg="Timeout exceeded: try increasing it by passing --timeout option"

---

_{Comment @coderabbitai help to get the list of available commands.}

[openshift-ci]

[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

Details

Needs approval from an approver in each of these files:

• OWNERS [giladravid16]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[giladravid16]

/test e2e-ibio

[shay23bra]

/lgtm

[giladravid16]

/override ci/prow/e2e-ibio-v4v6

[giladravid16]

/cherry-pick backplane-5.0

[openshift-cherrypick-robot]

@giladravid16: once the present PR merges, I will cherry-pick it on top of backplane-5.0 in a new PR and assign it to you.

Details

In response to this:

/cherry-pick backplane-5.0

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 kubernetes-sigs/prow repository.

[openshift-merge-bot]

/retest-required

Remaining retests: 0 against base HEAD ea04be0 and 2 for PR HEAD 901cf16 in total

[openshift-ci]

@giladravid16: Overrode contexts on behalf of giladravid16: ci/prow/e2e-ibio-v4v6

Details

In response to this:

/override ci/prow/e2e-ibio-v4v6

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 kubernetes-sigs/prow repository.

[openshift-ci]

@giladravid16: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-ibio-v4v6	901cf16	link	true	/test e2e-ibio-v4v6
ci/prow/ibio-reinstall-v4v6	901cf16	link	false	/test ibio-reinstall-v4v6

Full PR test history. Your PR dashboard.

Details

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 kubernetes-sigs/prow repository. I understand the commands that are listed here.

[openshift-cherrypick-robot]

@giladravid16: new pull request created: #866

Details

In response to this:

/cherry-pick backplane-5.0

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 kubernetes-sigs/prow repository.