Drop BMC Pending state in favor of ConditionReset - #1063
Conversation
github-actions
Bot
added
api-change
documentation
|
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:
📝 WalkthroughWalkthroughThe BMC state API now describes Redfish-reported values without enum validation or a Pending default. Reset reconciliation uses conditions for reset state, requeues while resets are active, and uses renamed reconciler configuration fields. ChangesBMC state and reset handling
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant BMCReconciler
participant ConditionState
participant RedfishBMC
BMCReconciler->>ConditionState: Check ConditionReset
BMCReconciler->>RedfishBMC: Request graceful reset
RedfishBMC-->>BMCReconciler: Return reset result
BMCReconciler->>ConditionState: Clear or retain reset condition
BMCReconciler-->>BMCReconciler: Requeue during active reset
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
🧹 Nitpick comments (1)
internal/controller/bmc_controller.go (1)
480-480: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCapitalize the propagated controller error.
The new error message starts with lowercase
could; controller error messages should start with a capital letter and identify the object type.🤖 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/bmc_controller.go` at line 480, Update the error returned in the BMC reset flow to begin with a capitalized message and explicitly identify the BMC object type, while preserving the existing object name and no-client-connection context.Source: Coding guidelines
🤖 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/bmc_controller.go`:
- Around line 480-490: Update resetBMC so ConditionReset is cleared before
returning when no client connection is available or a permanent ResetManager
error occurs. Preserve ConditionReset only after reset initiation is confirmed
or when the operation is intentionally retryable, so shouldResetBMC can schedule
future attempts for pre-initiation failures.
---
Nitpick comments:
In `@internal/controller/bmc_controller.go`:
- Line 480: Update the error returned in the BMC reset flow to begin with a
capitalized message and explicitly identify the BMC object type, while
preserving the existing object name and no-client-connection context.
🪄 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 Plus
Run ID: d3288ccd-d8c9-49b1-b326-8f6ce056d3f4
⛔ Files ignored due to path filters (2)
dist/chart/templates/crd/bmcs.metal.ironcore.dev.yamlis excluded by!**/dist/**dist/install.yamlis excluded by!**/dist/**
📒 Files selected for processing (6)
api/v1alpha1/applyconfiguration/api/v1alpha1/bmcstatus.goapi/v1alpha1/applyconfiguration/internal/internal.goapi/v1alpha1/bmc_types.goconfig/crd/bases/metal.ironcore.dev_bmcs.yamldocs/api-reference/api.mdinternal/controller/bmc_controller.go
💤 Files with no reviewable changes (1)
- api/v1alpha1/applyconfiguration/internal/internal.go
58397ba to
7ad92b4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/controller/bmc_controller.go (1)
128-141: 🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy liftAuto-reset path always passes a nil client to
resetBMC.
bmcutils.GetBMCClientFromBMCreturns(nil, err)on every failure path, so insideif err != nilthebmcClienthanded toresetBMCat Line 132 is always nil.resetBMCthen takes its nil-client branch: it setsConditionReset=True, immediately clears it, and returns an error — the RedfishResetManagercall is never reached. The automatic reset feature (FailureResetDelay) therefore can never actually reset a BMC, and reconcile returns a hard error instead of the intended requeue at Lines 136-138.A separate client (constructed without the connectivity gate, or via a dedicated reset connection) is needed before attempting
ResetManager.🤖 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/bmc_controller.go` around lines 128 - 141, The auto-reset branch in the reconcile flow must not pass the failed nil bmcClient from GetBMCClientFromBMC into resetBMC. Before calling resetBMC, obtain a separate BMC client without the connectivity-check gate (or use the dedicated reset connection) and pass that client so resetBMC can reach Redfish ResetManager; preserve the existing reset logging and requeue behavior on success.
🧹 Nitpick comments (3)
internal/controller/suite_test.go (1)
180-196: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
FailureResetDelayis left at zero, so the automatic-reset path is never exercised.
shouldResetBMCshort-circuits whenFailureResetDelay == 0, meaning no test in this suite covers the auto-reset branch (see the nil-client issue flagged ininternal/controller/bmc_controller.go). Setting a short delay here would give coverage for that path.🤖 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/suite_test.go` around lines 180 - 196, Set a short non-zero FailureResetDelay in the BMCReconciler initialization used by the suite setup, alongside ResetWaitTime and ClientRetryInterval, so shouldResetBMC exercises the automatic-reset branch while preserving the existing test timing behavior.internal/controller/bmc_controller.go (2)
474-494: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the
if err == nil { ... } else { ... }flow.The
elsebranch after areturnis flagged by revive/golint (indent-error-flow) and the trailing fallthrough to Line 493-494 is hard to follow. Inverting the check reads better and makes the "retryable 5xx" fallthrough explicit.♻️ Proposed refactor
- if err := bmcClient.ResetManager(ctx, bmcObj.Spec.BMCUUID, schemas.GracefulRestartResetType); err == nil { - log.Info("Successfully reset BMC via Redfish", "BMC", bmcObj.Name) - return nil - } else { - if httpErr, ok := errors.AsType[*schemas.Error](err); ok { - // only retryable on 5xx; anything else is a permanent failure for this attempt - if httpErr.HTTPReturnedStatusCode < 500 || httpErr.HTTPReturnedStatusCode >= 600 { - return errors.Join( - r.clearResetCondition(ctx, bmcObj), - fmt.Errorf("could not reset BMC: %w", err), - ) - } - } else { - return errors.Join( - r.clearResetCondition(ctx, bmcObj), - fmt.Errorf("could not reset BMC, unknown error: %w", err), - ) - } - } + err := bmcClient.ResetManager(ctx, bmcObj.Spec.BMCUUID, schemas.GracefulRestartResetType) + if err == nil { + log.Info("Successfully reset BMC via Redfish", "BMC", bmcObj.Name) + return nil + } + httpErr, ok := errors.AsType[*schemas.Error](err) + if !ok { + return errors.Join( + r.clearResetCondition(ctx, bmcObj), + fmt.Errorf("could not reset BMC, unknown error: %w", err), + ) + } + // only retryable on 5xx; anything else is a permanent failure for this attempt + if httpErr.HTTPReturnedStatusCode < 500 || httpErr.HTTPReturnedStatusCode >= 600 { + return errors.Join( + r.clearResetCondition(ctx, bmcObj), + fmt.Errorf("could not reset BMC: %w", err), + ) + }As per coding guidelines, run
make lint-fixandmake testafter editing Go source files.🤖 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/bmc_controller.go` around lines 474 - 494, Refactor the reset handling around bmcClient.ResetManager to invert the err == nil check: return immediately on success, then process non-nil errors without an else branch. Preserve the existing permanent-failure handling, clearResetCondition calls, and retryable 5xx fallthrough to the final log and nil return. Run make lint-fix and make test after editing.Source: Coding guidelines
395-411: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDon’t return the original failure error if you want
ClientRetryIntervalto apply.At
reconcile()line 140, this path returns bothResult{RequeueAfter: r.ClientRetryInterval}and a non-nil error, so controller-runtime ignores the fixed retry delay and uses its default exponential backoff instead. If the intended behavior is a fixed retry cadence for BMC connection failures, set ready condition/status without returning the original error here.🤖 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/bmc_controller.go` around lines 395 - 411, Update updateReadyConditionOnBMCFailure to return nil after successfully patching the BMC Ready condition, rather than propagating the original err; preserve returning the patch error so reconcile can retain the configured ClientRetryInterval for BMC connection failures.
🤖 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 `@cmd/main.go`:
- Around line 432-434: The help text for --bmc-reset-resync-interval must
reflect that ClientRetryInterval controls retries for any BMC connection
failure, not only polling during an in-progress reset. Update the flag
description near its declaration to mention general BMC connection retry
behavior while preserving the existing 2-minute default.
In `@internal/controller/bmc_controller.go`:
- Around line 123-126: The BMC reset condition remains true after an
unsuccessful reset wait, preventing future automatic resets. Update the
reconciliation flow around waitForBMCReset and the failure path after the wait
window expires to clear ConditionReset when the BMC has not reconnected, while
preserving the existing successful-reset and retry behavior.
---
Outside diff comments:
In `@internal/controller/bmc_controller.go`:
- Around line 128-141: The auto-reset branch in the reconcile flow must not pass
the failed nil bmcClient from GetBMCClientFromBMC into resetBMC. Before calling
resetBMC, obtain a separate BMC client without the connectivity-check gate (or
use the dedicated reset connection) and pass that client so resetBMC can reach
Redfish ResetManager; preserve the existing reset logging and requeue behavior
on success.
---
Nitpick comments:
In `@internal/controller/bmc_controller.go`:
- Around line 474-494: Refactor the reset handling around bmcClient.ResetManager
to invert the err == nil check: return immediately on success, then process
non-nil errors without an else branch. Preserve the existing permanent-failure
handling, clearResetCondition calls, and retryable 5xx fallthrough to the final
log and nil return. Run make lint-fix and make test after editing.
- Around line 395-411: Update updateReadyConditionOnBMCFailure to return nil
after successfully patching the BMC Ready condition, rather than propagating the
original err; preserve returning the patch error so reconcile can retain the
configured ClientRetryInterval for BMC connection failures.
In `@internal/controller/suite_test.go`:
- Around line 180-196: Set a short non-zero FailureResetDelay in the
BMCReconciler initialization used by the suite setup, alongside ResetWaitTime
and ClientRetryInterval, so shouldResetBMC exercises the automatic-reset branch
while preserving the existing test timing behavior.
🪄 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 Plus
Run ID: 77b267d4-0ce3-4e2d-9121-8ed0c806ecbe
⛔ Files ignored due to path filters (2)
dist/chart/templates/crd/bmcs.metal.ironcore.dev.yamlis excluded by!**/dist/**dist/install.yamlis excluded by!**/dist/**
📒 Files selected for processing (10)
api/v1alpha1/applyconfiguration/api/v1alpha1/bmcstatus.goapi/v1alpha1/applyconfiguration/internal/internal.goapi/v1alpha1/bmc_types.gocmd/main.goconfig/crd/bases/metal.ironcore.dev_bmcs.yamldocs/api-reference/api.mdinternal/controller/bmc_controller.gointernal/controller/bmc_controller_test.gointernal/controller/conditions.gointernal/controller/suite_test.go
💤 Files with no reviewable changes (1)
- api/v1alpha1/applyconfiguration/internal/internal.go
🚧 Files skipped from review as they are similar to previous changes (4)
- api/v1alpha1/applyconfiguration/api/v1alpha1/bmcstatus.go
- api/v1alpha1/bmc_types.go
- docs/api-reference/api.md
- config/crd/bases/metal.ironcore.dev_bmcs.yaml
b34427f to
da69358
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
| log.V(1).Info("Skipped BMC reconciliation while waiting for BMC reset to complete") | ||
| if err := r.patchBMCStatePending(ctx, bmcObj); err != nil { | ||
| return ctrl.Result{RequeueAfter: r.ClientRetryInterval}, nil | ||
| case resetWaitExpired: |
There was a problem hiding this comment.
maybe resetWaitExpired should use a dedicated ReasonResetExpired/ReasonResetTimeout reason, not ReasonResetFailed, to correctly distinguish a timeout from an actual failure.
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/bmc_controller.go (1)
167-176: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve timeout and failure reasons when reconnecting.
This checks only whether
ConditionResetexists, not whether it isTrue. After the timeout or failure paths set the condition toFalse, the next successful connectivity check rewrites its reason toReasonResetComplete, falsely reporting a successful reset and erasing the new timeout/failure distinction. Only mark the reset complete when the existing condition hasStatus == metav1.ConditionTrue; also propagate lookup errors instead of ignoring them.Proposed fix
- if found, _ := r.Conditions.FindSlice(bmcObj.Status.Conditions, ConditionReset, &metav1.Condition{}); found { + resetCondition := &metav1.Condition{} + found, err := r.Conditions.FindSlice(bmcObj.Status.Conditions, ConditionReset, resetCondition) + if err != nil { + return ctrl.Result{}, fmt.Errorf("failed to find BMC reset condition: %w", err) + } + if found && resetCondition.Status == metav1.ConditionTrue {🤖 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/bmc_controller.go` around lines 167 - 176, Update the reset-condition handling around Conditions.FindSlice and ConditionReset so completion is recorded only when the existing condition has Status == metav1.ConditionTrue. Capture and propagate any lookup error instead of discarding it, and leave False conditions from timeout or failure paths unchanged so their reason and message are preserved.
🤖 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/bmc_controller.go`:
- Around line 167-176: Update the reset-condition handling around
Conditions.FindSlice and ConditionReset so completion is recorded only when the
existing condition has Status == metav1.ConditionTrue. Capture and propagate any
lookup error instead of discarding it, and leave False conditions from timeout
or failure paths unchanged so their reason and message are preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ca9d6603-1e1e-4935-92f2-cc9f7c2e4687
📒 Files selected for processing (2)
internal/controller/bmc_controller.gointernal/controller/conditions.go
3eccc83 to
9961a97
Compare
|
@coderabbitai review |
|
Signed-off-by: Andreas Fritzler <andreas.fritzler@sap.com>
Signed-off-by: Andreas Fritzler <andreas.fritzler@sap.com>
9961a97 to
7a9b728
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
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. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/controller/bmc_controller.go (1)
137-149: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAuto-reset cannot issue a BMC reset on the failed-connection path.
Line 140 runs only after
GetBMCClientFromBMCfails and returns no client.resetBMCreceives that nil client, clearsConditionResetthroughfailReset, and returns an error without callingResetManager. The auto-reset path therefore cannot trigger a Redfish reset as configured; either prevent auto-reset until a BMC client is available, or issue the reset through a separate client path that can work without the same connectivity gate.🤖 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/bmc_controller.go` around lines 137 - 149, Update the auto-reset branch in the BMC reconciliation error path around shouldResetBMC and resetBMC so it does not pass the failed GetBMCClientFromBMC result when bmcClient is unavailable. Only initiate resetBMC after obtaining a usable client, or route the reset through a separate client-acquisition path that can perform ResetManager without the failed-connection client; preserve the existing reconnect and readiness-condition behavior when no reset-capable client is available.
🧹 Nitpick comments (1)
internal/controller/bmc_controller.go (1)
167-177: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePropagate the
FindSliceerror instead of discarding it.Line 167 discards the error and treats a decode failure as "condition absent". The reset completion is then skipped without any signal. Lines 431 and 496 handle the same call's error explicitly. Keep the handling uniform.
♻️ Proposed change
- if found, _ := r.Conditions.FindSlice(bmcObj.Status.Conditions, ConditionReset, &metav1.Condition{}); found { + found, err := r.Conditions.FindSlice(bmcObj.Status.Conditions, ConditionReset, &metav1.Condition{}) + if err != nil { + return ctrl.Result{}, fmt.Errorf("failed to find condition %s: %w", ConditionReset, err) + } + if found {Note that
erris already declared at Line 136, so this uses assignment rather than a new declaration.🤖 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/bmc_controller.go` around lines 167 - 177, Update the reset-condition lookup in the BMC reconciliation flow to capture and propagate the existing FindSlice error instead of discarding it. Reuse the err variable already declared in the surrounding scope, return a contextual error on lookup failure, and preserve the current condition-update behavior when the lookup succeeds.
🤖 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/bmc_controller.go`:
- Around line 145-148: Update the error return in the Reconcile flow around
updateReadyConditionOnBMCFailure to stop setting RequeueAfter when returning the
connection error; return the zero-value ctrl.Result with the error so
controller-runtime applies its backoff. Preserve ReconnectInterval for
successful or non-error requeue paths.
- Around line 402-418: Update updateReadyConditionOnBMCFailure to classify
non-HTTP transport errors, including connection timeouts and refusals from
gofish.ConnectContext, as ReasonConnectionFailed before patchCondition is
called. Preserve the existing HTTP status mappings and unknown-error fallback
for other errors.
---
Outside diff comments:
In `@internal/controller/bmc_controller.go`:
- Around line 137-149: Update the auto-reset branch in the BMC reconciliation
error path around shouldResetBMC and resetBMC so it does not pass the failed
GetBMCClientFromBMC result when bmcClient is unavailable. Only initiate resetBMC
after obtaining a usable client, or route the reset through a separate
client-acquisition path that can perform ResetManager without the
failed-connection client; preserve the existing reconnect and
readiness-condition behavior when no reset-capable client is available.
---
Nitpick comments:
In `@internal/controller/bmc_controller.go`:
- Around line 167-177: Update the reset-condition lookup in the BMC
reconciliation flow to capture and propagate the existing FindSlice error
instead of discarding it. Reuse the err variable already declared in the
surrounding scope, return a contextual error on lookup failure, and preserve the
current condition-update behavior when the lookup succeeds.
🪄 Autofix
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 Plus
Run ID: 90fb883d-614d-48ea-b036-13dde8714160
⛔ Files ignored due to path filters (2)
dist/chart/templates/crd/bmcs.metal.ironcore.dev.yamlis excluded by!**/dist/**dist/install.yamlis excluded by!**/dist/**
📒 Files selected for processing (10)
api/v1alpha1/applyconfiguration/api/v1alpha1/bmcstatus.goapi/v1alpha1/applyconfiguration/internal/internal.goapi/v1alpha1/bmc_types.gocmd/main.goconfig/crd/bases/metal.ironcore.dev_bmcs.yamldocs/api-reference/api.mdinternal/controller/bmc_controller.gointernal/controller/bmc_controller_test.gointernal/controller/conditions.gointernal/controller/suite_test.go
💤 Files with no reviewable changes (1)
- api/v1alpha1/applyconfiguration/internal/internal.go
🚧 Files skipped from review as they are similar to previous changes (8)
- internal/controller/bmc_controller_test.go
- api/v1alpha1/applyconfiguration/api/v1alpha1/bmcstatus.go
- docs/api-reference/api.md
- internal/controller/suite_test.go
- api/v1alpha1/bmc_types.go
- internal/controller/conditions.go
- config/crd/bases/metal.ironcore.dev_bmcs.yaml
- cmd/main.go
| RequeueAfter: r.ReconnectInterval, | ||
| }, nil | ||
| } | ||
| return ctrl.Result{RequeueAfter: r.BMCClientRetryInterval}, r.updateReadyConditionOnBMCFailure(ctx, bmcObj, err) | ||
| return ctrl.Result{RequeueAfter: r.ReconnectInterval}, r.updateReadyConditionOnBMCFailure(ctx, bmcObj, err) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does controller-runtime ignore Result.RequeueAfter when Reconcile returns a non-nil error?
💡 Result:
Yes, controller-runtime ignores the Result (including RequeueAfter) when the Reconcile function returns a non-nil error [1][2]. When a non-nil error is returned, the controller-runtime framework will automatically requeue the request using the controller's configured exponential backoff rate limiter [1][3][2]. The only exception to this behavior is if the returned error is a TerminalError, in which case the request is not requeued [1][2]. Because the Result is discarded whenever an error occurs, returning a non-zero Result (such as setting a specific RequeueAfter duration) alongside an error is ineffective and can be misleading [4][5][6]. The framework may also log a warning if both a non-nil error and a non-zero Result are returned simultaneously [4][5][6].
Citations:
- 1: https://github.com/kubernetes-sigs/controller-runtime/blob/main/pkg/reconcile/reconcile.go
- 2: https://github.com/kubernetes-sigs/controller-runtime/blob/v0.23.3/pkg/reconcile/reconcile.go
- 3: Document ways to trigger/not trigger exponential backoff kubernetes-sigs/controller-runtime#808
- 4: 🌱 log warning if error and non-zero Result returned kubernetes-sigs/controller-runtime#2451
- 5: fix: clear Result when Reconcile returns an error fluxcd/image-reflector-controller#880
- 6: kubernetes-sigs/karpenter@da39546
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate controller file =="
fd -a 'bmc_controller\.go$' . | sed 's#^\./##'
echo "== relevant outline =="
ast-grep outline internal/controller/bmc_controller.go --view expanded 2>/dev/null | sed -n '1,220p' || true
echo "== relevant lines =="
nl -ba internal/controller/bmc_controller.go | sed -n '1,220p'
echo "== search condition helper definitions/usages =="
rg -n "updateReadyConditionOnBMCFailure|ReconnectInterval|RequeueAfter|Reset" internal/controller/bmc_controller.go internal/controller -g '*.go'Repository: ironcore-dev/metal-operator
Length of output: 3664
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate controller file =="
fd -a 'bmc_controller\.go$' . | sed 's#^\./##'
echo "== relevant lines =="
nl -ba internal/controller/bmc_controller.go | sed -n '120,160p'
echo "== helper definition search and contents =="
rg -n "func .*updateReadyConditionOnBMCFailure|updateReadyConditionOnBMCFailure|ReconnectInterval|RequeueAfter|Revert|Reset" internal/controller -g '*.go'
echo "== structural check for methods/functions in bmc_controller.go with concise Python parse =="
python3 - <<'PY'
from pathlib import Path
import re
p = Path('internal/controller/bmc_controller.go')
s = p.read_text()
for m in re.finditer(r'\b(func|const|var)\b', s):
start = m.start()
line = s.count('\n', 0, start) + 1
snippet = s[start:start+80].replace('\n', ' ')
if re.search(r'(updateReadyConditionOnBMCFailure|ReconnectInterval|RequeueAfter|Result\{|Reconcile\b)', snippet):
print(f"{p}:{line}: {snippet[:90]}...")
PYRepository: ironcore-dev/metal-operator
Length of output: 312
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== relevant controller lines =="
awk 'NR>=125 && NR<=160 {printf "%5d\t%s\n", NR, $0}' internal/controller/bmc_controller.go
echo "== condition helper implementation =="
awk 'NR>=402 && NR<=420 {printf "%5d\t%s\n", NR, $0}' internal/controller/bmc_controller.go
echo "== retry/error return sites and helper names =="
python3 - <<'PY'
from pathlib import Path
p = Path('internal/controller/bmc_controller.go')
for line in [136,141,148]:
txt = p.read_text().splitlines()
if line <= len(txt):
print(f"{line}: {txt[line-1]}")
PY
sed -n '136,152p;402,420p' internal/controller/bmc_controller.goRepository: ironcore-dev/metal-operator
Length of output: 5062
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== find go.mod/controller-runtime version references =="
sed -n '1,120p' go.mod 2>/dev/null || true
rg -n "sigs\.k8s\.io/controller-runtime|controller-runtime|RequeueAfter|Result" go.mod go.sum pkg reconcile 2>/dev/null | head -80 || true
echo "== check local controller-runtime reconcile file if vendored/module cache present =="
for d in vendor/sigs.k8s.io/controller-runtime/pkg/reconcile reconcile; do
[ -f "$d/reconcile.go" ] && { echo "--- $d/reconcile.go ---"; sed -n '1,240p' "$d/reconcile.go"; }
doneRepository: ironcore-dev/metal-operator
Length of output: 6253
Replace ReconnectInterval with controller-runtime backoff.
controller-runtime ignores Result.RequeueAfter when Reconcile returns an error. Line 148 returns r.ReconnectInterval plus the connection error from updateReadyConditionOnBMCFailure, so retries use the reconciler backoff instead of the intended interval.
🤖 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/bmc_controller.go` around lines 145 - 148, Update the
error return in the Reconcile flow around updateReadyConditionOnBMCFailure to
stop setting RequeueAfter when returning the connection error; return the
zero-value ctrl.Result with the error so controller-runtime applies its backoff.
Preserve ReconnectInterval for successful or non-error requeue paths.
| func (r *BMCReconciler) updateReadyConditionOnBMCFailure(ctx context.Context, bmcObj *metalv1alpha1.BMC, err error) error { | ||
| httpErr := &schemas.Error{} | ||
| if errors.As(err, &httpErr) { | ||
| // only handle 5xx errors | ||
| reason, message := ReasonUnknownError, fmt.Sprintf("BMC connection error: %v", err) | ||
| if httpErr, ok := errors.AsType[*schemas.Error](err); ok { | ||
| switch httpErr.HTTPReturnedStatusCode { | ||
| case 401: | ||
| // Unauthorized error, likely due to bad credentials | ||
| if err := r.updateConditions(ctx, bmcObj, true, ConditionReady, corev1.ConditionFalse, ReasonAuthenticationFailed, "BMC credentials are invalid"); err != nil { | ||
| return fmt.Errorf("failed to set BMC unauthorized condition: %w", err) | ||
| } | ||
|
|
||
| reason, message = ReasonAuthenticationFailed, "BMC credentials are invalid" | ||
| case 500: | ||
| // Internal Server Error, might be transient | ||
| if err := r.updateConditions(ctx, bmcObj, true, ConditionReady, corev1.ConditionFalse, ReasonInternalError, "BMC internal server error"); err != nil { | ||
| return fmt.Errorf("failed to set BMC internal server error condition: %w", err) | ||
| } | ||
| reason, message = ReasonInternalError, "BMC internal server error" | ||
| case 503: | ||
| // Service Unavailable, might be transient | ||
| if err := r.updateConditions(ctx, bmcObj, true, ConditionReady, corev1.ConditionFalse, ReasonConnectionFailed, "BMC service unavailable"); err != nil { | ||
| return fmt.Errorf("failed to set BMC service unavailable condition: %w", err) | ||
| } | ||
| default: | ||
| if err := r.updateConditions(ctx, bmcObj, true, ConditionReady, corev1.ConditionFalse, ReasonUnknownError, fmt.Sprintf("BMC connection error: %v", err)); err != nil { | ||
| return fmt.Errorf("failed to set BMC error condition: %w", err) | ||
| } | ||
| } | ||
| } else { | ||
| if err := r.updateConditions(ctx, bmcObj, true, ConditionReady, corev1.ConditionFalse, ReasonUnknownError, fmt.Sprintf("BMC connection error: %v", err)); err != nil { | ||
| return fmt.Errorf("failed to set BMC error condition: %w", err) | ||
| reason, message = ReasonConnectionFailed, "BMC service unavailable" | ||
| } | ||
| } | ||
| if err := r.patchCondition(ctx, bmcObj, ConditionReady, corev1.ConditionFalse, reason, message); err != nil { | ||
| return fmt.Errorf("failed to set BMC ready condition: %w", err) | ||
| } | ||
| return err | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check how connection-level errors surface from the BMC client and which reasons are reachable.
set -euo pipefail
echo "=== Reason constants ==="
fd -t f 'conditions.go' -p 'internal/controller' | xargs rg -n 'Reason[A-Za-z]+\s*='
echo "=== Producers of schemas.Error vs raw transport errors ==="
rg -n -C4 'HTTPReturnedStatusCode' --glob '*.go'
echo "=== Client creation error paths ==="
fd -t f 'bmcutils.go' | xargs rg -n -C4 'func CreateBMCClient'Repository: ironcore-dev/metal-operator
Length of output: 1609
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== relevant file list ==="
git ls-files | rg '(^|/)internal/controller/(bmc_controller|.*conditions).*\.go$|(^|/)schemas/.*\.go$|bmcutils\.go$'
echo "=== bmc_controller outline section ==="
if [ -f internal/controller/bmc_controller.go ]; then
wc -l internal/controller/bmc_controller.go
sed -n '120,170p;400,490p' internal/controller/bmc_controller.go
fi
echo "=== conditions file section ==="
conditions="$(git ls-files | rg '(^|/)conditions\.go$' | head -n 1)"
if [ -n "${conditions:-}" ]; then
sed -n '60,110p' "$conditions"
fi
echo "=== schema error type references ==="
rg -n -C3 'type Error|HTTPReturnedStatusCode|Reason[A-Za-z]+' --glob '*.go' | sed -n '1,220p'Repository: ironcore-dev/metal-operator
Length of output: 8577
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== bmcutils.go header and GetBMCClientFromBMC ==="
wc -l pkg/bmcutils/bmcutils.go
rg -n -C8 'func GetBMCClientFromBMC|BMCConnectivityCheckOption|New.*BMC|HTTPReturnedStatusCode|schemas\.Error' pkg/bmcutils/bmcutils.go
echo "=== package declarations of schemas files ==="
git ls-files | rg '/schemas/' | xargs -r sh -c 'echo "--- $1"; sed -n "1,40p" "$1"' sh {}Repository: ironcore-dev/metal-operator
Length of output: 2895
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== bmcutils error constructors ==="
sed -n '1,80p;185,235p' pkg/bmcutils/bmcutils.go
echo "=== bmc package files ==="
git ls-files | rg '(^|/)bmc/'
for f in $(git ls-files | rg '(^|/)bmc/.*\.go$' | head -n 20); do
echo "--- $f"
wc -l "$f"
rg -n -C4 'NewRedfishBMCClient|NewRedfishLocalBMCClient|HTTPReturnedStatusCode|schemas\.Error|type Error|connection|timeout|dial|http' "$f" | sed -n '1,160p'
done
echo "=== schemas package files ==="
git ls-files 'internal/**/*schema*' 'internal/**/schemas*' 'pkg/**/*schema*' 'pkg/**/schemas*'Repository: ironcore-dev/metal-operator
Length of output: 24281
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== bmc/bmc.go imports and error/HTTP types ==="
sed -n '1,80p' bmc/bmc.go
rg -n -C4 'type .*Error|HTTPReturnedStatusCode|func .*Error|HTTPStatus|StatusCode|Wrap|Errorf|errors\.' bmc/bmc.go
echo "=== redfish implementation error wrapping ==="
rg -n -C5 'NewRedfishBMCClient|Do\(|Client\.Do|GET|POST|HTTPReturnedStatusCode|SchemaErrors|errors\.Wrap|wrap|err :=' bmc/redfish.go bmc/redfish_local.go | sed -n '1,260p'
echo "=== tests related to errors and reset reasons ==="
rg -n -C3 'ReasonUnknownError|ReasonConnectionFailed|ReasonInternalError|shouldResetBMC|FailureResetDelay|HTTPReturnedStatusCode|connection refused|timeout|503|500|401' internal/controller/bmc_controller_test.go pkg/bmcutils/bmcutils_test.go bmc/*_test.go internal/controller/bmc_controller.go | sed -n '1,260p'Repository: ironcore-dev/metal-operator
Length of output: 24541
🌐 Web query:
github.com/stmcginnis/gofish schemas Error HTTPReturnedStatusCode
💡 Result:
The HTTPReturnedStatusCode is a field within the common.Error struct in the stmcginnis/gofish Go library, which is used to capture error responses when interacting with Redfish or Swordfish services [1][2]. When the gofish client performs an HTTP request, it checks the response status code [3]. If the status code is not one of the success codes (typically 200, 201, 202, or 204), the library reads the response body and uses the schemas.ConstructError function to return a structured error [3]. This error object contains the HTTPReturnedStatusCode field, which holds the actual HTTP status code returned by the server [1][2]. Developers using the library often type-assert the error returned by client calls to the *common.Error type to inspect this field [4][5]. This is commonly used to handle specific conditions, such as ignoring or logging specific errors like 404 Not Found when polling for optional metrics [4][5][6]. Example of checking the status code: if err!= nil { if commonErr, ok:= err.(*common.Error); ok { if commonErr.HTTPReturnedStatusCode == http.StatusNotFound { // Handle 404 specifically } } } [4][5] Note that if the server returns a non-success status code but does not provide a standard Redfish error body, the library will still capture the status code in this field, allowing for generic error handling [2][7].
Citations:
- 1: https://pkg.go.dev/github.com/stmcginnis/gofish/common
- 2: https://github.com/stmcginnis/gofish/blob/main/client_test.go
- 3: https://github.com/stmcginnis/gofish/blob/v0.21.5/client.go
- 4: https://git.clustercockpit.org/ClusterCockpit/cc-metric-collector/commit/06ca37e705695188047d67293aaa01e442682700
- 5: https://git.clustercockpit.org/ClusterCockpit/cc-metric-collector/raw/commit/888db31dbf13244d6c53f70cba666b69a62c801f/receivers/redfishReceiver.go
- 6: https://git.clustercockpit.org/ClusterCockpit/cc-metric-collector/commit/a8beec29cc880ddeea5e8b483b370367bd9c9a4b
- 7: https://github.com/stmcginnis/gofish/blob/v0.21.5/client_test.go
Map transport-level BMC failures to ReasonConnectionFailed.
updateReadyConditionOnBMCFailure maps only HTTP 401, 500, and 503. A connection timeout or connection refused error from gofish.ConnectContext does not produce *schemas.Error, so it remains ReasonUnknownError.
shouldResetBMC only resets on ReasonInternalError or ReasonConnectionFailed. Network-level BMC failures therefore never trigger automatic reset. Treat transport errors as connection failures before calling this helper.
🤖 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/bmc_controller.go` around lines 402 - 418, Update
updateReadyConditionOnBMCFailure to classify non-HTTP transport errors,
including connection timeouts and refusals from gofish.ConnectContext, as
ReasonConnectionFailed before patchCondition is called. Preserve the existing
HTTP status mappings and unknown-error fallback for other errors.
2 participants
Proposed Changes
Drop BMC Pending state in favor of ConditionReset.
Summary by CodeRabbit
Pendingdefault and fixed allowed-value constraints forstatus.state.