OSAC-3548: Fix premature restart_trigger echo in syncStatus

[mennyaboush]

Summary

Fix a race condition where the fulfillment controller prematurely echoes status.restart_trigger before the operator has processed the restart, leaving the BMI permanently stuck in Starting state.

Root Cause

The fulfillment controller is event-driven via pg_notify and reconciles within milliseconds of a restart_trigger update. It reads stale PowerSynced=True from the K8s CR (the operator hasn't acted yet) and interprets it as "restart completed", echoing status.restart_trigger = spec.restart_trigger. This makes restartPending=false permanently, and the if !restartPending { continue } guard in syncStatus() skips all PowerSynced processing forever.

Fix

Guard the trigger echo on the operator having also echoed Status.RestartTrigger on the K8s CR (object.Spec.RestartTrigger == object.Status.RestartTrigger). The operator only writes this after the restart truly completes (line 638 of baremetalinstance_controller.go), so a stale PowerSynced=True from before the restart is correctly skipped.

Testing

• Updated existing test to set CR Status.RestartTrigger matching (happy path)
• Added new test: "should not echo restart trigger when operator has not completed restart" (the race condition scenario)
• All 70 baremetalinstance controller tests pass

Evidence

Bug reproduced on CI (4 consecutive runs on osac-test-infra PR #277) and on remote dev cluster (osac-sno). Detailed analysis in OSAC-3548.

Jira

OSAC-3548

Related

• E2E restart test: osac-test-infra PR #302 (depends on this fix to pass)

Assisted-by: Claude Code noreply@anthropic.com

Summary by CodeRabbit

• Bug Fixes
  • Improved restart synchronization so completion is only reported after the expected restart trigger is observed.
  • Prevented restart conditions from being cleared prematurely during incomplete restarts.
  • Ensured completed restarts accurately reflect their fulfillment status.

[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 a race condition where the fulfillment controller prematurely echoes status.restart_trigger before the operator has processed the restart, leaving the BMI permanently stuck in Starting state.

Root Cause

The fulfillment controller is event-driven via pg_notify and reconciles within milliseconds of a restart_trigger update. It reads stale PowerSynced=True from the K8s CR (the operator hasn't acted yet) and interprets it as "restart completed", echoing status.restart_trigger = spec.restart_trigger. This makes restartPending=false permanently, and the if !restartPending { continue } guard in syncStatus() skips all PowerSynced processing forever.

Fix

Guard the trigger echo on the operator having also echoed Status.RestartTrigger on the K8s CR (object.Spec.RestartTrigger == object.Status.RestartTrigger). The operator only writes this after the restart truly completes (line 638 of baremetalinstance_controller.go), so a stale PowerSynced=True from before the restart is correctly skipped.

Testing

• Updated existing test to set CR Status.RestartTrigger matching (happy path)
• Added new test: "should not echo restart trigger when operator has not completed restart" (the race condition scenario)
• All 70 baremetalinstance controller tests pass

Evidence

Bug reproduced on CI (4 consecutive runs on osac-test-infra PR #277) and on remote dev cluster (osac-sno). Detailed analysis in OSAC-3548.

Jira

OSAC-3548

Related

• E2E restart test: osac-test-infra PR #302 (depends on this fix to pass)

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]

Warning

Review limit reached

@mennyaboush, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 40 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 71eb137c-56de-4993-a6aa-f7963f7a5853

📥 Commits

Reviewing files that changed from the base of the PR and between 1d9191f and ce4d788.

📒 Files selected for processing (2)

• internal/controllers/baremetalinstance/baremetalinstance_reconciler_function.go
• internal/controllers/baremetalinstance/baremetalinstance_reconciler_function_test.go

Walkthrough

The reconciler now requires matching operator specification and status restart triggers before it clears restart conditions and updates fulfillment status. Tests cover completed and incomplete restart synchronization.

Changes

Restart synchronization

Layer / File(s)	Summary
Gate restart completion on the observed trigger internal/controllers/baremetalinstance/baremetalinstance_reconciler_function.go, internal/controllers/baremetalinstance/baremetalinstance_reconciler_function_test.go	The reconciler waits for the operator’s Status.RestartTrigger to match Spec.RestartTrigger. Tests cover completed and incomplete restart states.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

• osac-project/fulfillment-service#884: Both changes modify RestartTrigger handling in the BareMetalInstance reconciler.
• osac-project/fulfillment-service#885: Both changes modify restart-trigger synchronization through the operator’s Spec and Status fields.

Suggested reviewers: carbonin

🚥 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 identifies the fix for premature restart_trigger echoing in syncStatus.
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 two-file diff adds only restart-trigger logic and test values 42/0; added-line scans found no credential assignments, embedded credentials, private-key material, or long base64/hex secrets.
No-Weak-Crypto	✅ Passed	The patch adds only an int64 RestartTrigger equality guard and tests; it introduces no MD5, SHA1, DES, RC4, Blowfish, ECB, custom crypto, or secret comparison.
No-Injection-Vectors	✅ Passed	The patch adds only a RestartTrigger comparison and test data; scans found no SQL concatenation, shell execution, unsafe deserialization, eval/exec, os.system, or innerHTML usage.
Container-Privileges	✅ Passed	The PR changes only Go source and tests. No container or Kubernetes manifest adds prohibited privileges; existing deployments use runAsNonRoot and allowPrivilegeEscalation: false.
No-Sensitive-Data-In-Logs	✅ Passed	The patch adds only a RestartTrigger equality guard and tests; it adds no logging and does not log trigger, secret content, credentials, or customer data.
Ai-Attribution	✅ Passed	The sole PR commit has an Assisted-by: Claude Code trailer, and no AI Co-Authored-By marker 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.}

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ajamias, mennyaboush
Once this PR has been reviewed and has the lgtm label, please assign vladikr for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

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