Conversation
Signed-off-by: Andrew Dodds <andrew.dodds@sap.com>
atd9876
force-pushed
the
857-bug-conditionbmcresetpostsettingapply-never-set-for-hpe-causes-infinite-re-apply-loop
branch
from
bca6a23 to
a3de350
Compare
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughRefactors BMCSettings controller to a phased apply→(optional reset)→verify sequence gated by Conditions; adds exported ChangesBMCSettings Apply-Verify-Reset Flow Refactor
Sequence DiagramsequenceDiagram
participant Ctrl as BMCSettings Controller
participant Cond as Conditions (status)
participant BMC as BMC Device
participant Verify as Verification/Recompute
Ctrl->>Cond: Read ConditionBMCSettingsChangesIssued / ObservedGeneration
alt ChangesIssued outdated or not issued
Ctrl->>BMC: Issue/apply settings
BMC-->>Ctrl: Apply result
Ctrl->>Ctrl: persist apply-cycle conditions (ChangesVerified=pending, ResetPostSettingApply)
alt ResetIssued
Ctrl->>BMC: handleBMCReset()
BMC-->>Ctrl: Reset complete
else ResetRequired
Ctrl->>Cond: Set ResetPostSettingApply = ReasonResetRequired
end
end
Ctrl->>Verify: Recompute effective settings / compute diff
alt Diff empty
Verify-->>Ctrl: Verified
Ctrl->>Cond: Set ChangesVerified = True
else Diff non-empty
Ctrl->>Cond: Set ChangesVerified = Pending (ReasonBMCSettingsVerificationPending)
Ctrl-->>Ctrl: Requeue
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/controller/bmcsettings_controller.go (1)
427-554:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake the
ChangesIssuedgate generation-aware.Once this condition has
ReasonBMCSettingsChangesIssued, Phase 1 is skipped on every later reconcile. If theBMCSettingsspec changes while the object is stillInProgress, the controller keeps requeueing in verification against the new diff but never re-issues the updated settings. Compare the gate againstsettings.Generation/Status.ObservedGeneration, or clear it whenever the desired settings change.As per coding guidelines, "Implement idempotent reconciliation logic - safe to run the same reconciliation multiple times without side effects."
🤖 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/bmcsettings_controller.go` around lines 427 - 554, The ChangesIssued gate currently only checks changesIssued.Reason and skips Phase 1 forever once ReasonBMCSettingsChangesIssued is set; update the gating in the reconcile block that reads ConditionBMCSettingsChangesIssued (around the GetCondition call and the if changesIssued.Reason != ReasonBMCSettingsChangesIssued branch) to be generation-aware: compare settings.Generation to settings.Status.ObservedGeneration (or clear/reset the ConditionBMCSettingsChangesIssued condition) and only treat changes as issued if the observed generation matches the current spec generation; if the spec has changed (generation differs) reset the condition (or set its Status/Reason to indicate pending re-issue) so handleBMCPowerState + SetBMCAttributesImmediately will run again for the new diff, and persist that update via r.updateBMCSettingsStatus/ r.Conditions.Update so the controller re-enters Phase 1 for new spec generations.
🤖 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/bmcsettings_controller.go`:
- Around line 569-583: When drift is detected you must also reset later-phase
conditions so a subsequent reconcile can re-run verification; in the
drift-handling block (where you call GetCondition for
ConditionBMCSettingsChangesIssued and update it) also GetCondition for
ConditionBMCSettingsChangesVerified (using the same helpers) and call
r.Conditions.Update to set its status to False (or Unknown), set a DriftDetected
reason and a message like "Settings drift detected, re-verify needed", then
include that updated condition in the call to r.updateBMCSettingsStatus so the
status no longer shows Verified=True while re-apply is required.
---
Outside diff comments:
In `@internal/controller/bmcsettings_controller.go`:
- Around line 427-554: The ChangesIssued gate currently only checks
changesIssued.Reason and skips Phase 1 forever once
ReasonBMCSettingsChangesIssued is set; update the gating in the reconcile block
that reads ConditionBMCSettingsChangesIssued (around the GetCondition call and
the if changesIssued.Reason != ReasonBMCSettingsChangesIssued branch) to be
generation-aware: compare settings.Generation to
settings.Status.ObservedGeneration (or clear/reset the
ConditionBMCSettingsChangesIssued condition) and only treat changes as issued if
the observed generation matches the current spec generation; if the spec has
changed (generation differs) reset the condition (or set its Status/Reason to
indicate pending re-issue) so handleBMCPowerState + SetBMCAttributesImmediately
will run again for the new diff, and persist that update via
r.updateBMCSettingsStatus/ r.Conditions.Update so the controller re-enters Phase
1 for new spec generations.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a4f46937-feb3-4cf2-aac5-08368f0c46f6
📒 Files selected for processing (2)
internal/controller/bmcsettings_controller.gointernal/controller/conditions.go
Co-authored-by: Copilot <copilot@github.com> Signed-off-by: Andrew Dodds <andrew.dodds@sap.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Co-authored-by: Copilot <copilot@github.com> Signed-off-by: Andrew Dodds <andrew.dodds@sap.com>
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/bmcsettings_controller.go (1)
433-505:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset later-phase conditions when a new apply cycle starts.
This branch re-issues settings for a newer generation, but it only updates
ConditionBMCSettingsChangesIssued. If the previous generation leftConditionBMCResetPostSettingApplyatReasonResetIssued, the next reconcile for a generation whereresetBMCReq == falsewill still enterhandleBMCReset()and wait on or act on that stale reset state.ConditionBMCSettingsChangesVerified=Truecan also leak across generations and report verification success before the new generation is actually verified.Please overwrite/reset the phase-2 and phase-3 conditions whenever Phase 1 starts a fresh apply, including explicitly recording the “no reset required” case for
ConditionBMCResetPostSettingApply.As per coding guidelines,
internal/controller/**/*_controller.gomust "Implement idempotent reconciliation logic - safe to run the same reconciliation multiple times without side effects".🤖 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/bmcsettings_controller.go` around lines 433 - 505, When re-applying settings (the branch where changesIssued is stale and you call handleBMCPowerState and then issue settings), reset/overwrite phase-2 and phase-3 conditions so stale state doesn’t leak across generations: explicitly update ConditionBMCResetPostSettingApply and ConditionBMCSettingsChangesVerified to the appropriate defaults (e.g., ResetPostSettingApply = False with ReasonResetNotRequired when resetBMCReq is false, and SettingsChangesVerified = False) before calling r.updateBMCSettingsStatus; use the same r.Conditions.Update flow (as used for changesIssued) and ensure the updates reference changesIssued/resetCond and persist via r.updateBMCSettingsStatus so each new apply cycle is idempotent.
🤖 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/bmcsettings_controller.go`:
- Around line 433-505: When re-applying settings (the branch where changesIssued
is stale and you call handleBMCPowerState and then issue settings),
reset/overwrite phase-2 and phase-3 conditions so stale state doesn’t leak
across generations: explicitly update ConditionBMCResetPostSettingApply and
ConditionBMCSettingsChangesVerified to the appropriate defaults (e.g.,
ResetPostSettingApply = False with ReasonResetNotRequired when resetBMCReq is
false, and SettingsChangesVerified = False) before calling
r.updateBMCSettingsStatus; use the same r.Conditions.Update flow (as used for
changesIssued) and ensure the updates reference changesIssued/resetCond and
persist via r.updateBMCSettingsStatus so each new apply cycle is idempotent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ac7057c9-4c6e-484a-a696-cbf2abedc3b2
📒 Files selected for processing (2)
internal/controller/bmcsettings_controller.gointernal/controller/conditions.go
✅ Files skipped from review due to trivial changes (1)
- internal/controller/conditions.go
Co-authored-by: Copilot <copilot@github.com> Signed-off-by: Andrew Dodds <andrew.dodds@sap.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/controller/bmcsettings_controller.go (1)
655-658:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not short-circuit the fresh
ConditionResetIssuedpath here.This guard now treats an unset
ConditionResetIssuedas "nothing to do", so the stabilization reset at Line 418 never runs on a fresh reconcile. That regresses the generic pre-apply reset flow; this no-op branch should only apply toConditionBMCResetPostSettingApplyor an explicit "no reset needed" state.Suggested minimal fix
- // No reset requested — nothing to do - if resetBMC.Reason != ReasonResetRequired && resetBMC.Reason != ReasonResetIssued { + // Only the post-apply reset condition may legitimately mean "no reset needed". + if conditionType == ConditionBMCResetPostSettingApply && + resetBMC.Reason != ReasonResetRequired && resetBMC.Reason != ReasonResetIssued { return true, nil }As per coding guidelines,
internal/controller/**/*_controller.go: "Implement idempotent reconciliation logic - safe to run the same reconciliation multiple times without side effects".🤖 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/bmcsettings_controller.go` around lines 655 - 658, The current guard in the "No reset requested — nothing to do" branch incorrectly treats an unset ConditionResetIssued as a no-op; update the logic so we only short-circuit when there is an explicit "no reset needed" state (e.g. a Reason value that denotes no-reset) or when the BMC is already in the post-setting-apply state (ConditionBMCResetPostSettingApply), and allow the fresh ConditionResetIssued path to proceed to stabilization/reset handling; locate the branch checking resetBMC.Reason (the two-way check against ReasonResetRequired and ReasonResetIssued) and change it to return early only for the explicit no-reset reason or for ConditionBMCResetPostSettingApply, otherwise continue so the stabilization reset run (the path that relies on ConditionResetIssued) executes.
🤖 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/bmcsettings_controller.go`:
- Around line 494-527: The verifiedCond (ChangesVerified) update is never
persisted because updateBMCSettingsStatus is only called for changesIssued and
resetCond; call updateBMCSettingsStatus to persist verifiedCond (the condition
object you set earlier) before persisting changesIssued/resetCond so
ChangesVerified=True/False is written out before leaving Phase 1; update the
sequence around verifiedCond, changesIssued and resetCond to ensure
updateBMCSettingsStatus(ctx, settings, settings.Status.State, verifiedCond) is
invoked (or otherwise include verifiedCond in the single persistence call) so
the verified condition cannot leak into the next apply cycle.
---
Duplicate comments:
In `@internal/controller/bmcsettings_controller.go`:
- Around line 655-658: The current guard in the "No reset requested — nothing to
do" branch incorrectly treats an unset ConditionResetIssued as a no-op; update
the logic so we only short-circuit when there is an explicit "no reset needed"
state (e.g. a Reason value that denotes no-reset) or when the BMC is already in
the post-setting-apply state (ConditionBMCResetPostSettingApply), and allow the
fresh ConditionResetIssued path to proceed to stabilization/reset handling;
locate the branch checking resetBMC.Reason (the two-way check against
ReasonResetRequired and ReasonResetIssued) and change it to return early only
for the explicit no-reset reason or for ConditionBMCResetPostSettingApply,
otherwise continue so the stabilization reset run (the path that relies on
ConditionResetIssued) executes.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0c4861d4-f8cd-4df2-be20-eb493e910617
📒 Files selected for processing (1)
internal/controller/bmcsettings_controller.go
Co-authored-by: Copilot <copilot@github.com> Signed-off-by: Andrew Dodds <andrew.dodds@sap.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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/bmcsettings_controller.go`:
- Around line 555-570: persistApplyCycleConditions() currently uses
ReasonResetIssued for the no-reset branch causing logic/guard mismatches; add a
new constant ReasonNoResetRequired and use it in the "no reset" update call
(replace ReasonResetIssued in the else block that updates resetCond and the
similar update around lines ~664-667), and ensure handleBMCReset()'s
fast-path/guard is keyed off ConditionBMCResetPostSettingApply (and/or checks
ReasonResetIssued only when the reset was actually issued) so that pre-apply
calls remain effective; update any references to the old misuse to the new
ReasonNoResetRequired to make the semantics explicit.
- Around line 575-581: Currently the code calls r.updateBMCSettingsStatus three
times in a row which can persist ChangesIssued before the other conditions are
updated; instead prepare the three condition updates in-memory (use the existing
changesIssued, verifiedCond, resetCond and update settings.Status.Conditions or
the condition slice) and then call r.updateBMCSettingsStatus once to persist the
combined condition slice for the settings object; keep using the same settings
and settings.Status.State values and the existing updateBMCSettingsStatus helper
so the single patch is atomic from the controller's perspective.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 22189ec9-f714-4c8f-b37b-dc61bc85e605
📒 Files selected for processing (1)
internal/controller/bmcsettings_controller.go
Co-authored-by: Copilot <copilot@github.com> Signed-off-by: Andrew Dodds <andrew.dodds@sap.com>
4 participants
Proposed Changes
Fixes #857
Summary by CodeRabbit
Bug Fixes
Improvements