Skip to content

MGMT-23045: toggle metal3 integration#9933

Open
rccrdpccl wants to merge 1 commit intoopenshift:masterfrom
rccrdpccl:MGMT-23045-toggle-metal3-integration
Open

MGMT-23045: toggle metal3 integration#9933
rccrdpccl wants to merge 1 commit intoopenshift:masterfrom
rccrdpccl:MGMT-23045-toggle-metal3-integration

Conversation

@rccrdpccl
Copy link
Contributor

@rccrdpccl rccrdpccl commented Feb 24, 2026

Cluster API Provider Openshift Assisted requires assisted to work even without metal3: in some cases, we want to use other infrastructure providers (openstack, kubevirt, etc) and not be forced to have metal3 crds installed.

In case in the future we want them to coexist (i.e. we want to use separately Assisted Installer ZTP standalone - not through CAPI), we'd just install the metal3 crds and enable the integration on the assisted side.

List all the issues related to this PR

  • New Feature
  • Enhancement
  • Bug fix
  • Tests
  • Documentation
  • CI/CD

What environments does this code impact?

  • Automation (CI, tools, etc)
  • Cloud
  • Operator Managed Deployments
  • None

How was this code tested?

  • assisted-test-infra environment
  • dev-scripts environment
  • Reviewer's test appreciated
  • Waiting for CI to do a full test run
  • Manual (Elaborate on how it was tested)
  • No tests needed

Checklist

  • Title and description added to both, commit and PR.
  • Relevant issues have been associated (see CONTRIBUTING guide)
  • This change does not require a documentation update (docstring, docs, README, etc)
  • Does this change include unit-tests (note that code changes require unit-tests)

Reviewers Checklist

  • Are the title and description (in both PR and commit) meaningful and clear?
  • Is there a bug required (and linked) for this change?
  • Should this PR be backported?

Summary by CodeRabbit

Release Notes

  • New Features

    • Added EnableMetal3 configuration flag to optionally enable Metal3 integration.
    • Metal3-related controllers now conditionally activate based on configuration settings.
  • Tests

    • Expanded test coverage for Metal3-enabled and Metal3-disabled configurations.

Signed-off-by: Riccardo Piccoli <rpiccoli@redhat.com>
@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Feb 24, 2026
@openshift-ci-robot
Copy link

openshift-ci-robot commented Feb 24, 2026

@rccrdpccl: This pull request references MGMT-23045 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 "4.22.0" version, but no target version was set.

Details

In response to this:

Cluster API Provider Openshift Assisted requires assisted to work even without metal3: in some cases, we want to use other infrastructure providers (openstack, kubevirt, etc) and not be forced to have metal3 crds installed.

In case in the future we want them to coexist (i.e. we want to use separately Assisted Installer ZTP standalone - not through CAPI), we'd just install the metal3 crds and enable the integration on the assisted side.

List all the issues related to this PR

  • New Feature
  • Enhancement
  • Bug fix
  • Tests
  • Documentation
  • CI/CD

What environments does this code impact?

  • Automation (CI, tools, etc)
  • Cloud
  • Operator Managed Deployments
  • None

How was this code tested?

  • assisted-test-infra environment
  • dev-scripts environment
  • Reviewer's test appreciated
  • Waiting for CI to do a full test run
  • Manual (Elaborate on how it was tested)
  • No tests needed

Checklist

  • Title and description added to both, commit and PR.
  • Relevant issues have been associated (see CONTRIBUTING guide)
  • This change does not require a documentation update (docstring, docs, README, etc)
  • Does this change include unit-tests (note that code changes require unit-tests)

Reviewers Checklist

  • Are the title and description (in both PR and commit) meaningful and clear?
  • Is there a bug required (and linked) for this change?
  • Should this PR be backported?

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.

@rccrdpccl
Copy link
Contributor Author

/hold

@coderabbitai
Copy link

coderabbitai bot commented Feb 24, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

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

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to data retention organization setting

📥 Commits

Reviewing files that changed from the base of the PR and between 10cee3a and 360f37d.

📒 Files selected for processing (3)
  • cmd/main.go
  • internal/controller/controllers/agent_controller.go
  • internal/controller/controllers/agent_controller_test.go

Walkthrough

This change introduces a Metal3 feature flag (EnableMetal3) throughout the codebase to conditionally enable or disable Metal3 and BareMetalHost (BMH) handling. The flag is added to the global configuration, propagated during controller initialization, and used to gate Metal3-related controllers and logic paths. Test coverage is added to verify Metal3-disabled behavior.

Changes

Cohort / File(s) Summary
Configuration & Initialization
cmd/main.go
Adds EnableMetal3 field to Options config, creates getBMOUtils helper function, and conditionally initializes PreprovisioningImage and BMH controllers and caching based on the flag. Propagates flag through controller startup.
Feature-Gated Reconciler Logic
internal/controller/controllers/agent_controller.go
Adds EnableMetal3 field to AgentReconciler struct. Implements early-return guards in getBMH and bmhExists methods that skip Metal3/BMH logic when the flag is disabled.
Test Coverage
internal/controller/controllers/agent_controller_test.go
Adds test cases for Metal3-disabled scenarios, verifies unbinding/reclaim behavior works correctly regardless of Metal3 state, enables Metal3 path in test initializations, and tests helper method behavior under both Metal3-enabled and disabled conditions.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes


Important

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

❌ Failed checks (2 warnings, 2 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Test Structure And Quality ⚠️ Warning Tests in agent_controller_test.go use default Gomega expectations without descriptive failure messages, violating assertion context requirements. Update Expect calls with clear failure messages and ensure It blocks focus on single behaviors with appropriate timeouts.
Description check ❓ Inconclusive The description covers the motivation and context for the change, but several template sections remain unchecked and incomplete, including issue categorization, testing details, and the checklist items. Complete the template by checking appropriate issue type (appears to be Enhancement), providing more details on testing verification, and addressing unfilled checklist items regarding title/description in commits and documentation updates.
Stable And Deterministic Test Names ❓ Inconclusive Cannot verify Ginkgo test titles without access to the agent_controller_test.go file contents. File inspection required. Provide the contents of internal/controller/controllers/agent_controller_test.go for analysis of dynamic values in test titles.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: introducing a toggle for Metal3 integration, which is the primary objective of the changeset.
✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Comment @coderabbitai help to get the list of available commands and usage tips.

@openshift-ci openshift-ci bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 24, 2026
@openshift-ci
Copy link

openshift-ci bot commented Feb 24, 2026

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rccrdpccl

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:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 24, 2026
@openshift-ci-robot
Copy link

openshift-ci-robot commented Feb 24, 2026

@rccrdpccl: This pull request references MGMT-23045 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 "4.22.0" version, but no target version was set.

Details

In response to this:

Cluster API Provider Openshift Assisted requires assisted to work even without metal3: in some cases, we want to use other infrastructure providers (openstack, kubevirt, etc) and not be forced to have metal3 crds installed.

In case in the future we want them to coexist (i.e. we want to use separately Assisted Installer ZTP standalone - not through CAPI), we'd just install the metal3 crds and enable the integration on the assisted side.

List all the issues related to this PR

  • New Feature
  • Enhancement
  • Bug fix
  • Tests
  • Documentation
  • CI/CD

What environments does this code impact?

  • Automation (CI, tools, etc)
  • Cloud
  • Operator Managed Deployments
  • None

How was this code tested?

  • assisted-test-infra environment
  • dev-scripts environment
  • Reviewer's test appreciated
  • Waiting for CI to do a full test run
  • Manual (Elaborate on how it was tested)
  • No tests needed

Checklist

  • Title and description added to both, commit and PR.
  • Relevant issues have been associated (see CONTRIBUTING guide)
  • This change does not require a documentation update (docstring, docs, README, etc)
  • Does this change include unit-tests (note that code changes require unit-tests)

Reviewers Checklist

  • Are the title and description (in both PR and commit) meaningful and clear?
  • Is there a bug required (and linked) for this change?
  • Should this PR be backported?

Summary by CodeRabbit

Release Notes

  • New Features

  • Added EnableMetal3 configuration flag to optionally enable Metal3 integration.

  • Metal3-related controllers now conditionally activate based on configuration settings.

  • Tests

  • Expanded test coverage for Metal3-enabled and Metal3-disabled configurations.

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.

@codecov
Copy link

codecov bot commented Feb 24, 2026

Codecov Report

❌ Patch coverage is 10.25641% with 35 lines in your changes missing coverage. Please review.
✅ Project coverage is 44.06%. Comparing base (10cee3a) to head (360f37d).
⚠️ Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
cmd/main.go 0.00% 35 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #9933   +/-   ##
=======================================
  Coverage   44.06%   44.06%           
=======================================
  Files         414      414           
  Lines       72172    72180    +8     
=======================================
+ Hits        31803    31807    +4     
- Misses      37494    37498    +4     
  Partials     2875     2875           
Files with missing lines Coverage Δ
...nternal/controller/controllers/agent_controller.go 76.66% <100.00%> (+0.06%) ⬆️
cmd/main.go 0.00% <0.00%> (ø)

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@openshift-ci
Copy link

openshift-ci bot commented Feb 24, 2026

@rccrdpccl: The following test 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/edge-e2e-metal-assisted-4-21 360f37d link true /test edge-e2e-metal-assisted-4-21

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants