Repository navigation
OSAC-3548: Fix IsRestartComplete to wait for host to power on - #83
Conversation
|
@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. 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. |
|
Warning Review limit reached
Next review available in: 45 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository: osac-project/coderabbit/.coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Walkthrough
ChangesRestart completion validation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![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/baremetalinstance_controller.go`:
- Around line 409-413: The RunStrategyUnspecified test coverage lacks the
transitioning-power-state case. Extend the relevant controller test to set
PowerStatus.IsTransitioning true and verify the default management recheck
interval, BareMetalInstancePhaseProgressing phase, no SetPowerState call, and
PowerSynced=False with Progressing reason.
🪄 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: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3d0c0baf-d4b0-4671-8413-3306976d2ce8
📒 Files selected for processing (1)
internal/controller/baremetalinstance_controller.go
| if powerStatus != nil && powerStatus.IsTransitioning { | ||
| bareMetalInstance.Status.Phase = v1alpha1.BareMetalInstancePhaseProgressing | ||
| return ctrl.Result{RequeueAfter: r.ManagementRecheckIntervalDuration}, nil | ||
| } | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 'RunStrategyUnspecified|IsTransitioning|RequeueAfter' \
internal/controller/baremetalinstance_controller_test.goRepository: osac-project/bare-metal-fulfillment-operator
Length of output: 14851
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the controller segment around the newly added fallback requeue path.
sed -n '360,430p' internal/controller/baremetalinstance_controller.go
echo '--- locate reconcileManagement outline and runStrategy blocks ---'
rg -n -C 4 'RunStrategyUnspecified|RunStrategyHalted|RunStrategyAlways|isTransitioning|RequeueAfter: r.ManagementRecheckIntervalDuration' internal/controller/baremetalinstance_controller.go internal/controller/baremetalinstance_controller_test.goRepository: osac-project/bare-metal-fulfillment-operator
Length of output: 42167
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- relevant test contexts around unspecified RunStrategy ---\n'
sed -n '460,492p' internal/controller/baremetalinstance_controller_test.go
printf '\n--- definitions of reconcileManagement result expectations ---\n'
python3 - <<'PY'
from pathlib import Path
text = Path('internal/controller/baremetalinstance_controller_test.go').read_text()
# Print test headers inside reconcileManagement and syncBareMetalInstanceStatus specs for RunStrategyUnspecified.
lines = text.splitlines()
for start, end in [(300, 571), (840, 925)]:
for i in range(start, end+1):
if i <= len(lines) and ('It(' in lines[i-1] or 'BeforeEach' in lines[i-1] or 'Context(' in lines[i-1] or 'Expect(' in lines[i-1]):
print(f'{i}: {lines[i-1]}')
PYRepository: osac-project/bare-metal-fulfillment-operator
Length of output: 273
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- relevant test contexts around unspecified RunStrategy ---'
sed -n '460,492p' internal/controller/baremetalinstance_controller_test.go || true
echo
echo '--- RunStrategyUnspecified tests ---'
rg -n -C 6 'RunStrategyUnspecified' internal/controller/baremetalinstance_controller_test.go || trueRepository: osac-project/bare-metal-fulfillment-operator
Length of output: 2642
Add regression coverage for an unspecified RunStrategy with a transitioning power state.
RunStrategyUnspecified currently covers only the stable offline case that reaches Ready. Add a case with PowerStatus.IsTransitioning == true that asserts DefaultManagementRecheckIntervalDuration, Phase == BareMetalInstancePhaseProgressing, SetPowerState was not called, and PowerSynced == False/Progressing.
🤖 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/baremetalinstance_controller.go` around lines 409 - 413,
The RunStrategyUnspecified test coverage lacks the transitioning-power-state
case. Extend the relevant controller test to set PowerStatus.IsTransitioning
true and verify the default management recheck interval,
BareMetalInstancePhaseProgressing phase, no SetPowerState call, and
PowerSynced=False with Progressing reason.
8bc0459 to
81db67c
Compare
mennyaboush
changed the title
The Metal3 IsRestartComplete only checked whether the reboot annotation was removed from the BMH. The BMH controller removes the annotation as soon as it sends the reboot command to Ironic, before the host finishes rebooting. This caused the operator to declare restart complete while the host was still mid-reboot, setting PowerSynced=True then immediately overwriting it with PowerSynced=False in syncBareMetalInstanceStatus. Add a poweredOn check: restart is complete only when the annotation is gone AND the host is back online (bmh.Status.PoweredOn=true). This keeps the operator in the reconcileRestartTrigger requeue loop until the host actually finishes rebooting. Fixes: https://redhat.atlassian.net/browse/OSAC-3548 Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: MENNY ABOUSH <maboush@maboush-thinkpadt14gen5.raanaii.csb>
81db67c to
953177c
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ajamias, mennyaboush 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 |
3 participants
Summary
Fix BMI restart getting permanently stuck in Starting state. The Metal3
IsRestartCompleteonly 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.Root Cause
The operator declares restart complete while the host is still mid-reboot. In the same reconcile pass,
syncBareMetalInstanceStatusseespoweredOn=falseand overwritesPowerSyncedback toFalse/Progressing. WhenRunStrategyis unset (the default), the convergence check is skipped and the operator stops reconciling — leavingPowerSyncedstuck.Fix
Add a
poweredOncheck toIsRestartComplete: restart is complete only when the annotation is gone AND the host is back online (bmh.Status.PoweredOn=true). This keeps the operator in thereconcileRestartTriggerrequeue loop until the host actually finishes rebooting.Testing
Related
Assisted-by: Claude Code noreply@anthropic.com