[mennyaboush]

Summary

Fix BMI restart getting permanently stuck in Starting state. Cherry-picked from bare-metal-fulfillment-operator PR #83 (already merged in standalone repo).

Root Cause

The Metal3 IsRestartComplete only checked if the reboot annotation was removed, but the BMH controller removes it as soon as it sends the reboot command — before the host finishes rebooting. The operator declared restart complete while the host was still mid-reboot, leaving PowerSynced=False/Progressing stuck.

Fix

Restart is complete only when the annotation is gone AND the host is back online (bmh.Status.PoweredOn=true).

Testing

• Unit tests pass (including new test for annotation-gone-but-not-powered-on scenario)
• Validated end-to-end on remote dev cluster — restart completes in ~30 seconds

Jira

OSAC-3548

Related

• E2E restart test: osac-test-infra PR #302
• Original standalone PR: bare-metal-fulfillment-operator PR #83 (merged)

Assisted-by: Claude Code noreply@anthropic.com

Summary by CodeRabbit

• Bug Fixes
  • Improved restart status reporting by confirming that the host is powered on before marking a restart as complete.
  • Prevented older restart requests from being reported as finished prematurely.
  • Ensured restart status accurately reflects whether the current restart request has completed.

[openshift-ci-robot]

@mennyaboush: This pull request references OSAC-3548 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.

Details

In response to this:

Summary

Fix BMI restart getting permanently stuck in Starting state. Cherry-picked from bare-metal-fulfillment-operator PR #83 (already merged in standalone repo).

Root Cause

The Metal3 IsRestartComplete only checked if the reboot annotation was removed, but the BMH controller removes it as soon as it sends the reboot command — before the host finishes rebooting. The operator declared restart complete while the host was still mid-reboot, leaving PowerSynced=False/Progressing stuck.

Fix

Restart is complete only when the annotation is gone AND the host is back online (bmh.Status.PoweredOn=true).

Testing

• Unit tests pass (including new test for annotation-gone-but-not-powered-on scenario)
• Validated end-to-end on remote dev cluster — restart completes in ~30 seconds

Jira

OSAC-3548

Related

• E2E restart test: osac-test-infra PR #302
• Original standalone PR: bare-metal-fulfillment-operator PR #83 (merged)

Assisted-by: Claude Code noreply@anthropic.com

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]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

IsRestartComplete now requires the host to be powered on after reboot annotation removal. The reconciler completes a restart only when the spec and status restart triggers match. Tests cover completed and incomplete restart states.

Changes

Restart completion synchronization

Layer / File(s)	Summary
Require powered-on status bare-metal-fulfillment-operator/internal/management/metal3.go, bare-metal-fulfillment-operator/internal/management/metal3_test.go	IsRestartComplete checks bmh.Status.PoweredOn after annotation removal. A powered-off host remains incomplete without an error.
Synchronize restart triggers fulfillment-service/internal/controllers/baremetalinstance/baremetalinstance_reconciler_function.go, fulfillment-service/internal/controllers/baremetalinstance/baremetalinstance_reconciler_function_test.go	The reconciler records the restart and clears restart conditions only when the spec and status triggers match. A mismatched trigger keeps the restart in progress and clears restart failure.

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

Suggested reviewers: carbonin, tzumainn

Sequence Diagram(s)

sequenceDiagram
  participant BareMetalInstance
  participant Reconciler
  participant Metal3Operator
  BareMetalInstance->>Reconciler: Provide spec and status restart triggers
  Reconciler->>Metal3Operator: Check restart completion
  Metal3Operator-->>Reconciler: Return powered-on and annotation state
  Reconciler-->>BareMetalInstance: Finalize matching restart or keep it in progress

Loading

🚥 Pre-merge checks | ✅ 11

✅ Passed checks (11 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 main change: requiring IsRestartComplete to wait for the host to power on.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.
No-Hardcoded-Secrets	✅ Passed	The four-file patch adds only restart logic, comments, and numeric test fixtures; it contains no API keys, passwords, tokens, private keys, credential URLs, or secret-shaped blobs.
No-Weak-Crypto	✅ Passed	The PR diff adds restart-state logic and tests only. Changed production imports contain no crypto APIs, and scans found no MD5, SHA1, DES, RC4, Blowfish, ECB, or secret comparisons.
No-Injection-Vectors	✅ Passed	The PR adds no SQL, shell, eval/exec, pickle, unsafe YAML, os.system, or HTML sink; changed code only checks Kubernetes state and updates conditions.
Container-Privileges	✅ Passed	The PR changes only Go source and test files. The added diff contains no privileged:true, hostPID, hostNetwork, hostIPC, SYS_ADMIN, root, or allowPrivilegeEscalation settings.
No-Sensitive-Data-In-Logs	✅ Passed	The commit adds no logging calls or log fields. The changed lines contain restart logic, comments, and tests only; existing debug logs are unchanged and expose no secret values.
Ai-Attribution	✅ Passed	AI use is disclosed with an Assisted-by: Claude Code trailer in the HEAD commit and PR description; no AI Co-Authored-By trailer is present.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

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.

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adriengentil, mennyaboush

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:

• bare-metal-fulfillment-operator/OWNERS [adriengentil]
• fulfillment-service/OWNERS [adriengentil]

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