MGMT-20398: Force status on bmh after install - #10606
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
@pastequo: This pull request references MGMT-20398 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.0.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. |
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
size/XXL
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pastequo 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
|
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:
WalkthroughThe change adds configurable, phased stale-provisioning remediation for BareMetalHosts. It also updates dependencies, admission decoder types, controller-runtime wiring, RBAC, mocks, utility behavior, CRD schemas, and compatibility tests. ChangesStale provisioning remediation
Platform compatibility updates
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant BMACReconciler
participant BareMetalHost
participant BMO
BMACReconciler->>BareMetalHost: Set stale remediation marker and force detach
BMO->>BareMetalHost: Report operational detachment
BMACReconciler->>BareMetalHost: Patch provisioning state to StateProvisioned
BMACReconciler->>BareMetalHost: Remove forced-detach annotation
BMO->>BareMetalHost: Clear operational detachment
BMACReconciler->>BareMetalHost: Remove stale remediation marker
Suggested labels: Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 3 warnings)
✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Trivy (0.72.0)Trivy execution failed: 2026-08-07T13:53:33Z FATAL Fatal error run error: fs scan error: scan error: scan failed: failed analysis: post analysis error: post analysis error: ansible scan error: scan config error: find projects: walk dir: range error: stat .coderabbit-opengrep-fallback.399fe6a3-728d-469e-8312-4175b136b22c.yml: no such file or directory Comment |
5125872 to
3c35750
Compare
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
|
/retest |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #10606 +/- ##
==========================================
+ Coverage 44.47% 44.66% +0.18%
==========================================
Files 423 426 +3
Lines 73355 73854 +499
==========================================
+ Hits 32626 32984 +358
- Misses 37810 37915 +105
- Partials 2919 2955 +36
🚀 New features to boost your workflow:
|
3c35750 to
fb02b31
Compare
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@go.mod`:
- Line 41: Update the go.mod dependency pins for github.com/jackc/pgx/v5,
github.com/moby/moby, and github.com/containerd/containerd to releases that
contain fixes for their known advisories; if any vulnerable version must remain,
document why its affected code paths are unreachable.
In `@internal/controller/controllers/bmh_agent_controller_test.go`:
- Around line 4509-5038: Update the test fixtures in the
reconcileBMHStaleProvisioning specs to stop constructing
DetachedAnnotationArguments with the unverified Force field. Use only fields
supported by the pinned bmh_v1alpha1 type while preserving the serialized
force-detach annotation expected by the phase and cleanup tests.
In `@internal/controller/controllers/bmh_agent_controller.go`:
- Around line 2139-2241: Update reconcileBMHStaleProvisioning to bound the Phase
2, Phase 4, and Phase 5 wait states with a persistent timeout or circuit
breaker, so remediation cannot remain blocked indefinitely while BMO fails to
detach or correct status. Track elapsed waiting time across reconciles using the
existing remediation state, and when the threshold is exceeded, stop waiting,
clean up or transition the annotations consistently, and emit an
operator-visible metric, event, or alert identifying the stalled host and phase.
- Around line 2223-2237: The Phase 3 correction in the force-detach handling
must update the live BareMetalHost status rather than only writing
BMH_STATUS_ANNOTATION. Modify the flow around the correctedStatus assignment to
persist the corrected StateProvisioned status through the controller’s status
update mechanism, while retaining the annotation cleanup and stale-provisioning
marker behavior.
In `@internal/controller/controllers/bmo_utils.go`:
- Around line 97-100: Update the error branch in the Ironic IP validation flow
to emit the constructed error, not just attach it to the logger. In the block
checking len(ironicIPs) and ironicIPs[0], make the r.log.WithError(err) chain
invoke the appropriate terminal logging method before returning, while
preserving the existing error and return values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
fb02b31 to
fbacfb9
Compare
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
go.mod (1)
41-47: 🔒 Security & Privacy | 🔴 CriticalPreviously reported vulnerable dependency pins remain.
github.com/jackc/pgx/v5 v5.6.0andgithub.com/moby/moby v27.2.1+incompatiblestill match the advisories reported in the previous review. Upgrade to fixed releases before merging.#!/usr/bin/env bash set -euo pipefail osv-scanner scan source -r . go mod verify go list -m -json github.com/jackc/pgx/v5 github.com/moby/moby🤖 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 `@go.mod` around lines 41 - 47, Update the go.mod dependency pins for github.com/jackc/pgx/v5 and github.com/moby/moby to releases that contain fixes for their reported advisories, then synchronize go.sum and verify the resolved module versions.Sources: Path instructions, Linters/SAST tools
🤖 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 `@go.mod`:
- Around line 241-246: Update the module dependency for google.golang.org/grpc
to version v1.82.1 or newer, ensuring the resolved module graph no longer
retains v1.79.3 while preserving compatible indirect OpenTelemetry dependencies.
---
Duplicate comments:
In `@go.mod`:
- Around line 41-47: Update the go.mod dependency pins for
github.com/jackc/pgx/v5 and github.com/moby/moby to releases that contain fixes
for their reported advisories, then synchronize go.sum and verify the resolved
module versions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
fbacfb9 to
b6c2c51
Compare
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
b6c2c51 to
a87aaff
Compare
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
internal/controller/controllers/bmh_agent_controller_test.go (4)
4839-4846: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the shared
ClusterDeploymentfixture.The same
test-cdClusterDeploymentplusagent.Spec.ClusterDeploymentNameassignment appears in four places: theprechecks,Phase 2,Phase 3, andpipeline orderingBeforeEachblocks, and in bothsetupCDclosures. Move it into one helper at the suite level.🤖 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/controllers/bmh_agent_controller_test.go` around lines 4839 - 4846, Extract the repeated test-cd ClusterDeployment creation and agent.Spec.ClusterDeploymentName assignment into a single suite-level helper. Update the prechecks, Phase 2, Phase 3, pipeline ordering BeforeEach blocks, and both setupCD closures to call that helper while preserving the existing test setup behavior.
4999-5013: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the expected
Reconcileerror instead of discarding both return values.Line 5007 discards the
ctrl.Resultand the error. The test then cannot distinguish the acknowledged MCS-certificate failure from a new failure earlier in the pipeline. IfReconcilebegins to fail beforehandlePauseAndDetachBMHAnnotations, the annotation assertion fails without indicating the cause.Capture the error and assert what the test expects.
♻️ Suggested change
- // The Reconcile may error in later stages (e.g. MCS cert lookup), but - // the deferred BMH patch ensures annotations are persisted regardless. - bmhr.Reconcile(ctx, newBMHRequest(host)) //nolint:errcheck + // Reconcile fails in a later stage (MCS cert lookup), but the deferred + // BMH patch persists the annotations regardless. + _, err := bmhr.Reconcile(ctx, newBMHRequest(host)) + Expect(err).To(HaveOccurred())🤖 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/controllers/bmh_agent_controller_test.go` around lines 4999 - 5013, Update the Reconcile invocation in the “normal flow resumes after remediation completes” test to capture both the ctrl.Result and error, then assert the expected MCS-certificate lookup error before checking the persisted annotation. Do not discard either return value, so failures occurring before handlePauseAndDetachBMHAnnotations are reported directly.
4795-4830: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the
Status.Provisioning.CustomDeploycopy in Phase 3.Phase 3 also copies
bmh.Spec.CustomDeployintoStatus.Provisioning.CustomDeploy. This fixture leavesSpec.CustomDeploynil, so that copy is never exercised. SetSpec.CustomDeployin the fixture and assert the persisted status field.♻️ Suggested addition
host.Status.Provisioning.State = bmh_v1alpha1.StateProvisioning host.Status.OperationalStatus = bmh_v1alpha1.OperationalStatusDetached + host.Spec.CustomDeploy = &bmh_v1alpha1.CustomDeploy{Method: ASSISTED_DEPLOY_METHOD}Expect(persisted.Status.Provisioning.State).To(Equal(bmh_v1alpha1.StateProvisioned)) + Expect(persisted.Status.Provisioning.CustomDeploy).To(Equal(host.Spec.CustomDeploy))🤖 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/controllers/bmh_agent_controller_test.go` around lines 4795 - 4830, Update the Phase 3 fixture in the BeforeEach setup to populate host.Spec.CustomDeploy with a representative value, then extend the “patches live status to provisioned directly” test to assert the persisted Status.Provisioning.CustomDeploy matches it. Use the existing host, persisted, and reconcileBMHStaleProvisioning symbols without changing unrelated assertions.
4960-4967: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueRemove the redundant
BootMACAddressmutation.
newBMHalready sets the same value, andnewBMHRequestuses only the object key. Remove the assignment afterCreate.🤖 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/controllers/bmh_agent_controller_test.go` around lines 4960 - 4967, Remove the redundant host.Spec.BootMACAddress assignment to macStr after c.Create in this test; rely on newBMH’s existing value while preserving the object creation and subsequent test 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 `@go.mod`:
- Around line 263-274: Restore the root go.mod replacement for
github.com/metal3-io/baremetal-operator, or update it to a resolvable compatible
version required by cluster-baremetal-operator. Then regenerate the vendor
directory and rerun the module checks to ensure dependency resolution succeeds.
In `@internal/controller/controllers/clusterdeployments_controller_test.go`:
- Line 3429: The test assertion only checks result.Requeue but ignores the
RequeueAfter field, which can also schedule reconciliation. Replace the
Expect(result.Requeue).To(BeFalse()) assertion with
Expect(result).To(Equal(ctrl.Result{})) to validate that the complete
ctrl.Result object matches an empty result and properly stops reconciliation.
---
Nitpick comments:
In `@internal/controller/controllers/bmh_agent_controller_test.go`:
- Around line 4839-4846: Extract the repeated test-cd ClusterDeployment creation
and agent.Spec.ClusterDeploymentName assignment into a single suite-level
helper. Update the prechecks, Phase 2, Phase 3, pipeline ordering BeforeEach
blocks, and both setupCD closures to call that helper while preserving the
existing test setup behavior.
- Around line 4999-5013: Update the Reconcile invocation in the “normal flow
resumes after remediation completes” test to capture both the ctrl.Result and
error, then assert the expected MCS-certificate lookup error before checking the
persisted annotation. Do not discard either return value, so failures occurring
before handlePauseAndDetachBMHAnnotations are reported directly.
- Around line 4795-4830: Update the Phase 3 fixture in the BeforeEach setup to
populate host.Spec.CustomDeploy with a representative value, then extend the
“patches live status to provisioned directly” test to assert the persisted
Status.Provisioning.CustomDeploy matches it. Use the existing host, persisted,
and reconcileBMHStaleProvisioning symbols without changing unrelated assertions.
- Around line 4960-4967: Remove the redundant host.Spec.BootMACAddress
assignment to macStr after c.Create in this test; rely on newBMH’s existing
value while preserving the object creation and subsequent test 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
|
/retest |
Assisted-by: OpenCode
a87aaff to
4c01cfe
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Caution CodeRabbit couldn't update its existing comment. The review summary may be out of date. Error details |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@go.mod`:
- Around line 241-246: Update the dependency selecting google.golang.org/grpc so
the module graph resolves to a non-vulnerable fixed version instead of v1.79.3,
then regenerate go.mod/go.sum and vendor metadata to reflect the resolved
version. Preserve the existing OpenTelemetry dependency updates.
- Line 41: Upgrade the github.com/jackc/pgx/v5 dependency from v5.6.0 to a
fixed, non-vulnerable release, then regenerate the related dependency metadata
so go.mod and go.sum remain consistent.
- Line 47: Update the github.com/moby/moby dependency from v27.2.1+incompatible
to a non-vulnerable compatible release when it is used by production or
image-management code; otherwise isolate it from the production dependency graph
as test-only and document that boundary.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| github.com/hashicorp/go-version v1.7.0 | ||
| github.com/iancoleman/strcase v0.2.0 | ||
| github.com/itchyny/gojq v0.12.8 | ||
| github.com/jackc/pgx/v5 v5.6.0 |
There was a problem hiding this comment.
Upgrade github.com/jackc/pgx/v5 before merge.
The v5.6.0 pin at Line 41 remains vulnerable. OSV Scanner reports critical SQL-injection and memory-safety advisories. pkg/db/db.go imports this module, so the dependency is used on a production path. Upgrade to a fixed release and regenerate dependency metadata. This repeats the existing review finding.
🧰 Tools
🪛 OSV Scanner (2.4.0)
[CRITICAL] 41-41: github.com/jackc/pgx/v5 5.6.0: CVE-2026-33815 in github.com/jackc/pgx
(GO-2026-4771)
[CRITICAL] 41-41: github.com/jackc/pgx/v5 5.6.0: CVE-2026-33816 in github.com/jackc/pgx
(GO-2026-4772)
[CRITICAL] 41-41: github.com/jackc/pgx/v5 5.6.0: SQL Injection via placeholder confusion with dollar quoted string literals in github.com/jackc/pgx
(GO-2026-5004)
[CRITICAL] 41-41: github.com/jackc/pgx/v5 5.6.0: Memory-safety vulnerability in github.com/jackc/pgx/v5.
[CRITICAL] 41-41: github.com/jackc/pgx/v5 5.6.0: pgx: SQL Injection via placeholder confusion with dollar quoted string literals
[CRITICAL] 41-41: github.com/jackc/pgx/v5 5.6.0: pgx contains memory-safety vulnerability
🤖 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 `@go.mod` at line 41, Upgrade the github.com/jackc/pgx/v5 dependency from
v5.6.0 to a fixed, non-vulnerable release, then regenerate the related
dependency metadata so go.mod and go.sum remain consistent.
Source: Linters/SAST tools
| github.com/krishicks/yaml-patch v0.0.10 | ||
| github.com/metal3-io/baremetal-operator/apis v0.2.0 | ||
| github.com/metal3-io/baremetal-operator/apis v0.13.1 | ||
| github.com/moby/moby v27.2.1+incompatible |
There was a problem hiding this comment.
Upgrade or isolate github.com/moby/moby.
The v27.2.1+incompatible pin at Line 47 remains affected by high-severity Docker/Moby advisories. Upgrade it if production or image-management code loads the affected packages. If this dependency is test-only, keep it outside the production dependency graph and document that boundary. This repeats the existing review finding.
🧰 Tools
🪛 OSV Scanner (2.4.0)
[HIGH] 47-47: github.com/moby/moby 27.2.1+incompatible: Moby has an Off-by-one error in its plugin privilege validation in github.com/docker/docker
(GO-2026-4883)
[HIGH] 47-47: github.com/moby/moby 27.2.1+incompatible: Moby has AuthZ plugin bypass when provided oversized request bodies in github.com/docker/docker
(GO-2026-4887)
[HIGH] 47-47: github.com/moby/moby 27.2.1+incompatible: Race condition in 'docker cp' in github.com/docker/docker allows bind mount redirection
(GO-2026-5617)
[HIGH] 47-47: github.com/moby/moby 27.2.1+incompatible: Docker: Race condition in docker cp allows creation of arbitrary empty files on the host via symlink swap in github.com/docker/docker
(GO-2026-5668)
[HIGH] 47-47: github.com/moby/moby 27.2.1+incompatible: Docker: 'PUT /containers/{id}/archive' executes container binary on the host in github.com/docker/docker
(GO-2026-5746)
[HIGH] 47-47: github.com/moby/moby 27.2.1+incompatible: Moby has an Off-by-one error in its plugin privilege validation
[HIGH] 47-47: github.com/moby/moby 27.2.1+incompatible: Docker: Race condition in docker cp allows bind mount redirection to host path
[HIGH] 47-47: github.com/moby/moby 27.2.1+incompatible: Docker: Race condition in docker cp allows creation of arbitrary empty files on the host via symlink swap
[HIGH] 47-47: github.com/moby/moby 27.2.1+incompatible: Moby has AuthZ plugin bypass when provided oversized request bodies
[HIGH] 47-47: github.com/moby/moby 27.2.1+incompatible: Docker: PUT /containers/{id}/archive executes container binary on the host
🤖 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 `@go.mod` at line 47, Update the github.com/moby/moby dependency from
v27.2.1+incompatible to a non-vulnerable compatible release when it is used by
production or image-management code; otherwise isolate it from the production
dependency graph as test-only and document that boundary.
Source: Linters/SAST tools
| go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc v0.60.0 // indirect | ||
| go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp v0.61.0 // indirect | ||
| go.opentelemetry.io/otel v1.43.0 // indirect | ||
| go.opentelemetry.io/otel/metric v1.43.0 // indirect | ||
| go.opentelemetry.io/otel/sdk v1.43.0 // indirect | ||
| go.opentelemetry.io/otel/trace v1.43.0 // indirect |
There was a problem hiding this comment.
Resolve google.golang.org/grpc to a fixed version.
The dependency updates at Lines 241-246 still leave google.golang.org/grpc v1.79.3 in the module graph. OSV Scanner reports vulnerabilities in the xDS RBAC and HTTP/2 implementations. Upgrade the dependency that selects gRPC, then regenerate vendor metadata. This repeats the existing review finding.
🧰 Tools
🪛 OSV Scanner (2.4.0)
[HIGH] 241-241: google.golang.org/grpc 1.79.3: Vulnerabilities in the xDS RBAC authorization engine and the HTTP/2 transport server implementation in google.golang.org/grpc
(GO-2026-6061)
[HIGH] 241-241: google.golang.org/grpc 1.79.3: gRPC-Go: xDS RBAC and HTTP/2 Vulnerabilities
🤖 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 `@go.mod` around lines 241 - 246, Update the dependency selecting
google.golang.org/grpc so the module graph resolves to a non-vulnerable fixed
version instead of v1.79.3, then regenerate go.mod/go.sum and vendor metadata to
reflect the resolved version. Preserve the existing OpenTelemetry dependency
updates.
Source: Linters/SAST tools
|
@pastequo: 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
Note: I updated the BMO go mod dependency to have the new flag, which in turns required a new go-client. So I had to remove the go replace directives, which leads to extra large vendoring changes
If upgrading all that raise trouble, I should still be able to implement the fix without upgrading bmh mod, by hardcoding the annotation (probably)
List all the issues related to this PR
What environments does this code impact?
How was this code tested?
Checklist
docs, README, etc)Reviewers Checklist
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Chores