METAL-1918: Add servicing annotation downstream - #512
Conversation
|
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:
WalkthroughAdds a service annotation prefix, integrates service annotations into BareMetalHost servicing eligibility and cleanup, and adds reconciliation tests for base, suffixed, provisioned, non-provisioned, and missing-policy cases. ChangesService annotation servicing
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant BareMetalHost
participant BareMetalHostController
participant HostUpdatePolicy
BareMetalHost->>BareMetalHostController: Reconcile service annotation
BareMetalHostController->>HostUpdatePolicy: Apply servicing policy
HostUpdatePolicy-->>BareMetalHostController: Servicing completes
BareMetalHostController->>BareMetalHost: Clear base annotation and update
🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
cf58d4c to
db02011
Compare
![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
🤖 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 `@internal/controller/metal3.io/baremetalhost_controller.go`:
- Around line 1585-1588: The firmware policy selection must distinguish
service-annotation triggers from normal reboot/power-on triggers. In
internal/controller/metal3.io/baremetalhost_controller.go:1585-1588, allow
onService only when a service annotation initiated the flow and keep onReboot
restricted to the powered-off/reboot path; at 1756-1757, pass or preserve that
trigger context when selecting the allowed policy. Update
internal/controller/metal3.io/baremetalhost_controller_test.go:1171-1214 to
replace the live-onReboot expectation and verify onService does not run during
an annotation-free power-on.
🪄 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: Pro Plus
Run ID: 0f26c176-7b3c-4fb6-9c73-a86bd3cbf5bb
⛔ Files ignored due to path filters (2)
vendor/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/hostupdatepolicy_types.gois excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (6)
apis/metal3.io/v1alpha1/baremetalhost_types.goapis/metal3.io/v1alpha1/hostupdatepolicy_types.goconfig/base/crds/bases/metal3.io_hostupdatepolicies.yamlconfig/render/capm3.yamlinternal/controller/metal3.io/baremetalhost_controller.gointernal/controller/metal3.io/baremetalhost_controller_test.go
db02011 to
daf4585
Compare
|
/test e2e-metal-ipi-ovn-ipv6 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@internal/controller/metal3.io/baremetalhost_controller_test.go`:
- Line 1129: Update the image fixtures in
internal/controller/metal3.io/baremetalhost_controller_test.go at lines 1129,
1180, 1220, 1259, and 1313 to explicitly set a non-MD5 ChecksumType and replace
each matching test checksum with a value valid for that type, avoiding the
implicit MD5 default and any “MD5 ... usage” warning.
- Around line 1202-1207: Update each tryReconcile predicate in
internal/controller/metal3.io/baremetalhost_controller_test.go at lines
1202-1207, 1241-1245, 1269-1274, and 1293-1297 to explicitly reject
OperationalStatusServicing before accepting its respective final state:
annotation removal, power-on, annotation removal, and available-host steady
state. Preserve the existing success conditions while ensuring every
intermediate servicing status causes the predicate to fail.
🪄 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: Pro Plus
Run ID: 5d6f4124-a7f9-4d05-82ff-706962118704
⛔ Files ignored due to path filters (4)
test/vendor/github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1/baremetalhost_types.gois excluded by!**/vendor/**test/vendor/github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1/hostupdatepolicy_types.gois excluded by!**/vendor/**vendor/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/hostupdatepolicy_types.gois excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (6)
apis/metal3.io/v1alpha1/baremetalhost_types.goapis/metal3.io/v1alpha1/hostupdatepolicy_types.goconfig/base/crds/bases/metal3.io_hostupdatepolicies.yamlconfig/render/capm3.yamlinternal/controller/metal3.io/baremetalhost_controller.gointernal/controller/metal3.io/baremetalhost_controller_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- config/base/crds/bases/metal3.io_hostupdatepolicies.yaml
- config/render/capm3.yaml
- apis/metal3.io/v1alpha1/baremetalhost_types.go
- apis/metal3.io/v1alpha1/hostupdatepolicy_types.go
- internal/controller/metal3.io/baremetalhost_controller.go
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/controller/metal3.io/baremetalhost_controller.go (1)
1760-1784: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep offline hosts out of the live
onServicepath.When a service annotation is present while
Status.PoweredOnis false,servicingAllowedbecomes true anddoServiceIfNeeded(..., hasService)treats the annotation as a live request. That can startservicingservicing on an offline host and clear the annotation; keep the base annotation pending, power on first, then service, or narrowservicingAllowedtohasService && PoweredOn.🤖 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 `@internal/controller/metal3.io/baremetalhost_controller.go` around lines 1760 - 1784, Update the servicingAllowed/doServiceIfNeeded flow so a host with a service annotation and Status.PoweredOn false is not treated as a live servicing request. Keep the annotation pending while offline, and only invoke doServiceIfNeeded with the live service request after the host is powered on, preserving normal servicing behavior for powered-on hosts.
🤖 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.
Outside diff comments:
In `@internal/controller/metal3.io/baremetalhost_controller.go`:
- Around line 1760-1784: Update the servicingAllowed/doServiceIfNeeded flow so a
host with a service annotation and Status.PoweredOn false is not treated as a
live servicing request. Keep the annotation pending while offline, and only
invoke doServiceIfNeeded with the live service request after the host is powered
on, preserving normal servicing behavior for powered-on hosts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 32638e30-5751-45fb-a01d-942a25e96d55
⛔ Files ignored due to path filters (4)
test/vendor/github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1/baremetalhost_types.gois excluded by!**/vendor/**test/vendor/github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1/hostupdatepolicy_types.gois excluded by!**/vendor/**vendor/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/hostupdatepolicy_types.gois excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (6)
apis/metal3.io/v1alpha1/baremetalhost_types.goapis/metal3.io/v1alpha1/hostupdatepolicy_types.goconfig/base/crds/bases/metal3.io_hostupdatepolicies.yamlconfig/render/capm3.yamlinternal/controller/metal3.io/baremetalhost_controller.gointernal/controller/metal3.io/baremetalhost_controller_test.go
|
/retest |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: elfosardo, jacob-anders 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
jacob-anders
changed the title
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
79d2271 to
522f9c2
Compare
jacob-anders
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
/test e2e-metal-ipi-serial-ipv4 |
|
@jacob-anders three requests please:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apis/metal3.io/v1alpha1/baremetalhost_types.go (1)
58-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the downstream-only constant in a dedicated OpenShift file.
ServiceAnnotationPrefixis explicitly OpenShift-specific but is added to the shared upstream API file. Move this declaration to a dedicated*_openshift.gofile in the same package to keep downstream additions isolated and reduce rebase/upstream maintenance cost.🤖 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 `@apis/metal3.io/v1alpha1/baremetalhost_types.go` around lines 58 - 60, Move the OpenShift-specific ServiceAnnotationPrefix declaration out of the shared baremetalhost_types.go file into a dedicated *_openshift.go file in the same package. Preserve its exported name, value, and documentation so downstream consumers retain the existing API while upstream definitions remain isolated.
🤖 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 `@internal/controller/metal3.io/service_annotation_openshift_test.go`:
- Around line 58-77: Update the servicing test around the reconciliation calls
to inspect the fixture or provisioner action history, rather than relying only
on the host.Status.PoweredOn snapshots. Assert that no power-off or reboot
action was requested throughout servicing, while preserving the existing
completion and annotation-clearing assertions.
---
Nitpick comments:
In `@apis/metal3.io/v1alpha1/baremetalhost_types.go`:
- Around line 58-60: Move the OpenShift-specific ServiceAnnotationPrefix
declaration out of the shared baremetalhost_types.go file into a dedicated
*_openshift.go file in the same package. Preserve its exported name, value, and
documentation so downstream consumers retain the existing API while upstream
definitions remain isolated.
🪄 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: da1be154-1e98-413b-ad6e-f8391ec48418
📒 Files selected for processing (4)
apis/metal3.io/v1alpha1/baremetalhost_types.gointernal/controller/metal3.io/baremetalhost_controller.gointernal/controller/metal3.io/service_annotation_openshift.gointernal/controller/metal3.io/service_annotation_openshift_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/controller/metal3.io/baremetalhost_controller.go
| // Servicing should start without powering off | ||
| tryReconcile(t, r, host, | ||
| func(host *metal3api.BareMetalHost, result reconcile.Result) bool { | ||
| return host.Status.OperationalStatus == metal3api.OperationalStatusServicing | ||
| }, | ||
| ) | ||
|
|
||
| // Host should remain powered on during servicing | ||
| assert.True(t, host.Status.PoweredOn) | ||
|
|
||
| // Servicing completes, annotation is cleared | ||
| tryReconcile(t, r, host, | ||
| func(host *metal3api.BareMetalHost, result reconcile.Result) bool { | ||
| _, exists := host.Annotations[metal3api.ServiceAnnotationPrefix] | ||
| return host.Status.OperationalStatus == metal3api.OperationalStatusOK && !exists | ||
| }, | ||
| ) | ||
|
|
||
| // Host should still be powered on | ||
| assert.True(t, host.Status.PoweredOn) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that no power-off action was requested.
The two PoweredOn snapshots do not prove a reboot never occurred between reconciliations. Instrument the fixture or provisioner call history and assert that servicing completed without a power-off/reboot request.
🤖 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 `@internal/controller/metal3.io/service_annotation_openshift_test.go` around
lines 58 - 77, Update the servicing test around the reconciliation calls to
inspect the fixture or provisioner action history, rather than relying only on
the host.Status.PoweredOn snapshots. Assert that no power-off or reboot action
was requested throughout servicing, while preserving the existing completion and
annotation-clearing assertions.
openshift-ci-robot
added
the
jira/valid-reference
|
@jacob-anders: This pull request references METAL-1918 which is a valid jira issue. 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. |
All good points. Should be addressed now, will have one last look once CI is finished and should be good for final review/premerge. |
|
/test e2e-metal-ipi-ovn-ipv6 |
…thout reboot Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ac22ef7 to
418c386
Compare
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
418c386 to
529abec
Compare
When an older Ironic that does not support the allow_grouping_reboots step argument is used with batched firmware updates, the servicing request fails asynchronously with "unexpected arguments: allow_grouping_reboots". Detect this specific error and replace it with a clear Metal3 error message advising the user to upgrade Ironic or update components individually. Do not retry, since the same request will fail the same way. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@jacob-anders: 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. |
4 participants
Summary by CodeRabbit
New Features
Bug Fixes