METAL-1756: Baremetal platform improvements#30868
METAL-1756: Baremetal platform improvements#30868honza wants to merge 9 commits intoopenshift:mainfrom
Conversation
The empty string check for provisioningNetwork was unreachable because an empty string already satisfies the `!= "Disabled"` condition in the preceding branch. This caused missing Provisioning CRs to skip with the misleading "Unsupported config" message instead of reporting the actual problem. Check for empty string first. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This assertion was disabled over 3 years ago waiting for a BMO fix to prevent HostFirmwareSettings with 0 entries. That fix has long since shipped. Reenable the check. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplace unstructured access with typed Metal3 conversions; add firmware/provisioning helpers; update tests to use metal3v1alpha1 types and new assertions (provisioning, pods, deployment, firmware settings); update OWNERS and bump metal3 apis module version. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/extended/baremetal/hosts.go (1)
181-187: Consider checking init container statuses as well.The test only verifies
ContainerStatusesbut notInitContainerStatuses. While init containers should have completed for running pods, checking their restart counts could catch initialization instability issues.♻️ Optional enhancement to include init containers
for _, pod := range pods.Items { g.By(fmt.Sprintf("checking containers in pod %s", pod.Name)) + for _, cs := range pod.Status.InitContainerStatuses { + o.Expect(cs.RestartCount).To(o.BeNumerically("<", 5), fmt.Sprintf("init container %s in pod %s has restarted %d times", cs.Name, pod.Name, cs.RestartCount)) + } for _, cs := range pod.Status.ContainerStatuses { o.Expect(cs.Ready).To(o.BeTrue(), fmt.Sprintf("container %s in pod %s is not ready", cs.Name, pod.Name)) o.Expect(cs.RestartCount).To(o.BeNumerically("<", 5), fmt.Sprintf("container %s in pod %s has restarted %d times", cs.Name, pod.Name, cs.RestartCount)) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/baremetal/hosts.go` around lines 181 - 187, The test currently iterates pods.Items and only inspects pod.Status.ContainerStatuses; update the check to also iterate pod.Status.InitContainerStatuses (e.g., inside the same pod loop) and apply the same expectations (o.Expect(... Ready? only applies to normal containers; for init containers assert RestartCount < 5 and that the state indicates terminated/succeeded if appropriate) — specifically reference the pod variable and the ContainerStatuses and InitContainerStatuses slices and ensure you call o.Expect on each init cs.RestartCount to be < 5 and include the same failure message format mentioning cs.Name and pod.Name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/extended/baremetal/hosts.go`:
- Around line 181-187: The test currently iterates pods.Items and only inspects
pod.Status.ContainerStatuses; update the check to also iterate
pod.Status.InitContainerStatuses (e.g., inside the same pod loop) and apply the
same expectations (o.Expect(... Ready? only applies to normal containers; for
init containers assert RestartCount < 5 and that the state indicates
terminated/succeeded if appropriate) — specifically reference the pod variable
and the ContainerStatuses and InitContainerStatuses slices and ensure you call
o.Expect on each init cs.RestartCount to be < 5 and include the same failure
message format mentioning cs.Name and pod.Name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b3a19b3c-4794-4366-ab1b-6764d24e47ea
📒 Files selected for processing (2)
test/extended/baremetal/common.gotest/extended/baremetal/hosts.go
honza
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@honza: This pull request references METAL-1756 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 story to target the "4.22.0" version, but no target version was set. 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. |
|
Scheduling required tests: Scheduling tests matching the |
|
@honza: This pull request references METAL-1756 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 story to target the "4.22.0" version, but no target version was set. 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. |
openshift-ci
bot
added
the
do-not-merge/invalid-owners-file
|
Scheduling required tests: Scheduling tests matching the |
|
@honza: This pull request references METAL-1756 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 story to target the "4.22.0" version, but no target version was set. 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: honza The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/extended/baremetal/hosts.go (1)
114-120: Consider verifying the condition exists.The loop checks that if a
FirmwareSettingsValidcondition exists, it must beTrue. However, if the condition is missing entirely, the test passes silently. Consider asserting that the condition is found.🔧 Optional: Ensure the condition exists
g.By("check that hostfirmwaresettings conditions show resource is valid") o.Expect(hfs.Status.Conditions).ToNot(o.BeEmpty()) + foundValidCondition := false for _, cond := range hfs.Status.Conditions { if cond.Type == string(metal3v1alpha1.FirmwareSettingsValid) { o.Expect(cond.Status).To(o.Equal(metav1.ConditionTrue)) + foundValidCondition = true } } + o.Expect(foundValidCondition).To(o.BeTrue(), "FirmwareSettingsValid condition not found for host %s", bmh.Name)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/baremetal/hosts.go` around lines 114 - 120, The test iterates hfs.Status.Conditions and only asserts the status if a FirmwareSettingsValid condition is present, so it silently passes when the condition is missing; update the check in the test (in hosts.go) to assert the condition is present and true by scanning hfs.Status.Conditions for metal3v1alpha1.FirmwareSettingsValid (e.g., set a found flag while looping over hfs.Status.Conditions), then o.Expect(found).To(o.BeTrue()) and o.Expect(cond.Status).To(o.Equal(metav1.ConditionTrue)) for the matched condition (or otherwise fail the test if not found).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/extended/baremetal/hosts.go`:
- Around line 114-120: The test iterates hfs.Status.Conditions and only asserts
the status if a FirmwareSettingsValid condition is present, so it silently
passes when the condition is missing; update the check in the test (in hosts.go)
to assert the condition is present and true by scanning hfs.Status.Conditions
for metal3v1alpha1.FirmwareSettingsValid (e.g., set a found flag while looping
over hfs.Status.Conditions), then o.Expect(found).To(o.BeTrue()) and
o.Expect(cond.Status).To(o.Equal(metav1.ConditionTrue)) for the matched
condition (or otherwise fail the test if not found).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5fb2651c-b953-45f1-bc77-416db20e5136
⛔ Files ignored due to path filters (4)
go.sumis excluded by!**/*.sumvendor/github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1/baremetalhost_types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1/hostfirmwarecomponents_types.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (4)
go.modtest/extended/baremetal/common.gotest/extended/baremetal/high_availability.gotest/extended/baremetal/hosts.go
|
@honza: This pull request references METAL-1756 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 story to target the "4.22.0" version, but no target version was set. 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. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/extended/baremetal/hosts.go (1)
114-120: Consider asserting that the FirmwareSettingsValid condition is found.The loop searches for the
FirmwareSettingsValidcondition but doesn't fail if the condition is absent. If this condition is expected to always be present, consider adding an assertion.♻️ Optional: Add assertion that condition was found
g.By("check that hostfirmwaresettings conditions show resource is valid") o.Expect(hfs.Status.Conditions).ToNot(o.BeEmpty()) + foundValid := false for _, cond := range hfs.Status.Conditions { if cond.Type == string(metal3v1alpha1.FirmwareSettingsValid) { o.Expect(cond.Status).To(o.Equal(metav1.ConditionTrue)) + foundValid = true } } + o.Expect(foundValid).To(o.BeTrue(), "FirmwareSettingsValid condition not found for host %s", bmh.Name)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/baremetal/hosts.go` around lines 114 - 120, The test iterates hfs.Status.Conditions looking for the FirmwareSettingsValid condition but never asserts that it was found; update the check in the test (around hfs.Status.Conditions loop) to set a boolean (e.g., foundFirmwareSettingsValid) when cond.Type == string(metal3v1alpha1.FirmwareSettingsValid) and cond.Status == metav1.ConditionTrue, then after the loop assert that foundFirmwareSettingsValid is true (using the same o.Expect helper) so the test fails if the condition is missing or not True.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/extended/baremetal/hosts.go`:
- Around line 114-120: The test iterates hfs.Status.Conditions looking for the
FirmwareSettingsValid condition but never asserts that it was found; update the
check in the test (around hfs.Status.Conditions loop) to set a boolean (e.g.,
foundFirmwareSettingsValid) when cond.Type ==
string(metal3v1alpha1.FirmwareSettingsValid) and cond.Status ==
metav1.ConditionTrue, then after the loop assert that foundFirmwareSettingsValid
is true (using the same o.Expect helper) so the test fails if the condition is
missing or not True.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1e597aa8-7257-41d5-9fc6-dd1cf416c0bf
⛔ Files ignored due to path filters (4)
go.sumis excluded by!**/*.sumvendor/github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1/baremetalhost_types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1/hostfirmwarecomponents_types.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (5)
go.modtest/extended/baremetal/OWNERStest/extended/baremetal/common.gotest/extended/baremetal/high_availability.gotest/extended/baremetal/hosts.go
openshift-ci
bot
removed
the
do-not-merge/invalid-owners-file
|
@honza: This pull request references METAL-1756 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 story to target the "4.22.0" version, but no target version was set. 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. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/extended/baremetal/common.go (1)
104-106: Consider reusingprovisioningGVR()ingetProvisioningNetwork.The same GVR is defined inline at line 67 in
getProvisioningNetwork. Consider refactoring to use this helper for consistency.♻️ Suggested refactor
func getProvisioningNetwork(dc dynamic.Interface) string { - provisioningGVR := schema.GroupVersionResource{Group: "metal3.io", Resource: "provisionings", Version: "v1alpha1"} - provisioningClient := dc.Resource(provisioningGVR) + provisioningClient := dc.Resource(provisioningGVR()) provisioningConfig, err := provisioningClient.Get(context.Background(), "provisioning-configuration", metav1.GetOptions{})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/baremetal/common.go` around lines 104 - 106, The GroupVersionResource for provisionings is duplicated; replace the inline GVR in getProvisioningNetwork with a call to the existing provisioningGVR() helper to avoid duplication. In getProvisioningNetwork, remove the inline schema.GroupVersionResource{Group: "metal3.io", Resource: "provisionings", Version: "v1alpha1"} and use provisioningGVR() when building the dynamic client request and any comparisons so all code references the single provisioningGVR() function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/extended/baremetal/hosts.go`:
- Around line 115-120: The test currently iterates hfs.Status.Conditions and
only asserts when it finds a FirmwareSettingsValid entry, which lets the test
pass if that condition is absent; modify the check to first assert that a
condition with Type == string(metal3v1alpha1.FirmwareSettingsValid) exists
(e.g., set a bool found flag while iterating) and then assert its Status equals
metav1.ConditionTrue using o.Expect; update the block around
hfs.Status.Conditions and the loop that inspects cond.Type/cond.Status so the
test fails when the FirmwareSettingsValid condition is missing or not
ConditionTrue.
---
Nitpick comments:
In `@test/extended/baremetal/common.go`:
- Around line 104-106: The GroupVersionResource for provisionings is duplicated;
replace the inline GVR in getProvisioningNetwork with a call to the existing
provisioningGVR() helper to avoid duplication. In getProvisioningNetwork, remove
the inline schema.GroupVersionResource{Group: "metal3.io", Resource:
"provisionings", Version: "v1alpha1"} and use provisioningGVR() when building
the dynamic client request and any comparisons so all code references the single
provisioningGVR() function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d1314e5c-923b-4e1e-adbe-1cdc886573b3
⛔ Files ignored due to path filters (4)
go.sumis excluded by!**/*.sumvendor/github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1/baremetalhost_types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1/hostfirmwarecomponents_types.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (5)
go.modtest/extended/baremetal/OWNERStest/extended/baremetal/common.gotest/extended/baremetal/high_availability.gotest/extended/baremetal/hosts.go
🚧 Files skipped from review as they are similar to previous changes (2)
- go.mod
- test/extended/baremetal/high_availability.go
honza
force-pushed
the
baremetal
branch
2 times, most recently
from
51674be to
2f2b7aa
Compare
|
Scheduling required tests: Scheduling tests matching the |
|
@honza: This pull request references METAL-1756 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 story to target the "4.22.0" version, but no target version was set. 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. |
|
/test e2e-metal-ipi-ovn-ipv6 |
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: 2f2b7aa
New tests seen in this PR at sha: 2f2b7aa
|
| e2eskipper.Skipf("Unable to read ProvisioningNetwork from Provisioning CR") | ||
| } else { | ||
| return | ||
| } else if provisioningNetwork != "Disabled" { |
There was a problem hiding this comment.
While we've never announced, we actually support provisioning network on "none" since 4.20 (I think)
There was a problem hiding this comment.
(it does not have to be fixed here, to be clear)
|
Looks good, but the new and updated tests fail in the v6 job. |
The Provisioning CR (provisioning-configuration) is the key resource managed by cluster-baremetal-operator, yet it had zero test coverage. Add a test that verifies it exists and has a provisioningNetwork spec field. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> fix
The existing test only checks that the metal3 deployment has 1 available replica. Add a test that verifies every container in the metal3 pod is Ready and hasn't been crash-looping (< 5 restarts). This catches issues with individual containers like ironic that the deployment-level check misses. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The image-customization-controller is deployed by CBO on baremetal platform as the metal3-image-customization deployment but had zero test coverage. Add a test that verifies the deployment exists, has 1 available replica, and carries the expected CBO ownership annotation and label. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
All baremetal test suites use metal3.io API resources but were missing the [apigroup:metal3.io] annotation needed for proper CI filtering. Also add [apigroup:config.openshift.io] to the baremetal-only suite which queries the Infrastructure resource. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace manual unstructured field access (expectStringField, getStringField, expectBoolField, etc.) with typed metal3.io/v1alpha1 structs using runtime.DefaultUnstructuredConverter. This catches API drift at compile time and makes the tests much more readable. The dynamic client is still used for API calls since there is no generated clientset for metal3.io, but results are now converted to BareMetalHost and HostFirmwareSettings typed structs for assertions. The Provisioning CR and BootMacAddress update tests retain unstructured access where needed (Provisioning has no vendored type, and the update test requires manipulating raw unstructured objects). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Update from v0.11.0-alpha.0 to the v0.11.0 release. No struct field changes affect our tests — the only API change is GetChecksum() returning error instead of bool. No transitive dependency changes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Scheduling required tests: Scheduling tests matching the |
|
@honza: 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. |
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: 72e7cd4
New tests seen in this PR at sha: 72e7cd4
|
4 participants
Summary by CodeRabbit