Aggregate HTTPBoot and iPXE Conditions to Compute ServerBootConfiguration Readiness#292
Aggregate HTTPBoot and iPXE Conditions to Compute ServerBootConfiguration Readiness#292
Conversation
WalkthroughThis PR introduces a new Changes
Sequence Diagram(s)sequenceDiagram
actor K8s as Kubernetes API
participant HC as HTTP Controller
participant PC as PXE Controller
participant SBC as ServerBootConfiguration<br/>Resource
participant RC as Readiness Controller
K8s->>HC: Reconcile HTTPBootConfig
HC->>HC: Validate UKI URL
HC->>SBC: Patch HTTPBootReady condition<br/>(True/False/Unknown)
K8s->>PC: Reconcile IPXEBootConfig
PC->>PC: Get image details
PC->>SBC: Patch IPXEBootReady condition<br/>(True/False/Unknown)
K8s->>RC: Reconcile ServerBootConfiguration<br/>(condition change detected)
RC->>SBC: Read current state +<br/>conditions
RC->>RC: Compute desired State from<br/>HTTPBootReady + IPXEBootReady
RC->>SBC: Patch Status.State if changed<br/>(Ready/Error/Pending)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/main.go`:
- Line 112: The readiness controller (serverBootConfigReadiness /
ServerBootConfigurationReadinessReconciler) must not be independently
disable-able because it's the sole writer of
ServerBootConfiguration.Status.State; update cmd/main.go so that when either
mode controller (HTTP or PXE reconcilers) is enabled the readiness controller is
automatically enabled, or else validate the --controllers allow-list at startup
and exit with an error if a mode controller is enabled while
serverBootConfigReadiness is disabled; modify the controller initialization or
add a startup check that references serverBootConfigReadiness and the mode
controller flags and either force-enable readiness or fail fast on the invalid
combination.
In `@internal/controller/serverbootconfig_helpers.go`:
- Around line 251-269: PatchServerBootConfigCondition currently does a single
MergeFrom status patch which can silently overwrite concurrent condition
updates; change it to an optimistic retry-on-conflict loop: fetch the latest
ServerBootConfiguration, set condition.ObservedGeneration if zero, call
apimeta.SetStatusCondition on the fetched object's Status.Conditions, attempt
c.Status().Patch(ctx, &obj, client.MergeFrom(base)), and if you get a conflict
(apierrors.IsConflict) re-fetch and retry a few times before failing. Apply the
same conflict-retry pattern to the two open-coded condition updates in
patchHTTPBootReadyConditionFromHTTPState and
patchIPXEBootReadyConditionFromIPXEState so each updater merges the latest
conditions and retries on conflict instead of silently overwriting.
In `@internal/controller/serverbootconfiguration_http_controller.go`:
- Around line 144-147: The code currently uses HTTPBootConfig.Status.State
directly when patching the parent condition, which can be stale if the child
HTTPBootConfig hasn't reconciled the new generation; update the logic so that
when calling or inside patchHTTPBootReadyConditionFromHTTPState you compare
httpBootConfig.ObjectMeta.Generation (or httpBootConfig.Generation) with the
parent ServerBootConfiguration.Generation (config.Generation) and treat the
child's state as Unknown when they differ. Concretely: modify
patchHTTPBootReadyConditionFromHTTPState (or its call) to detect generation
mismatch and map any non-current-child state to metav1.ConditionUnknown (or the
equivalent Unknown/Unknown constant) before creating/updating the
HTTPBootReadyCondition (HTTPBootReadyConditionType), ensuring the parent
condition is set to Unknown until the child reconciles.
In `@internal/controller/serverbootconfiguration_pxe_controller.go`:
- Around line 163-166: The code patches IPXEBootReadyCondition from the child
IPXEBootConfig immediately after Apply, but may read a stale
IPXEBootConfig.Status.State and incorrectly stamp ObservedGeneration for the new
ServerBootConfiguration; update patchIPXEBootReadyConditionFromIPXEState (and
its call site) to avoid promoting Ready/Error from a stale child status by
either (A) adding an ObservedGeneration field to IPXEBootConfig.Status and only
applying the child's Ready/Error when IPXEBootConfig.Status.ObservedGeneration
== ipxe.Spec.Generation (or otherwise indicates the child has observed the new
spec), or (B) skip setting ObservedGeneration/Ready/Error in
ServerBootConfiguration until a subsequent reconcile when the child status shows
it has processed the spec; reference the functions/methods
patchIPXEBootReadyConditionFromIPXEState, IPXEBootConfig.Status and the
ServerBootConfiguration reconciliation path where Apply is called to locate and
implement the change.
In `@internal/controller/serverbootconfiguration_readiness_controller.go`:
- Around line 55-76: When evaluating HTTPBootReadyConditionType and
IPXEBootReadyConditionType (in the RequireHTTPBoot / RequireIPXEBoot blocks)
treat conditions whose ObservedGeneration != cfg.Generation as if they were
missing: after calling apimeta.FindStatusCondition(cfg.Status.Conditions, ...),
consider the condition nil if c == nil OR c.ObservedGeneration !=
cfg.Generation, and then set allReady = false for that case; keep the existing
handling for c.Status == metav1.ConditionFalse (set hasError = true) and
c.Status != metav1.ConditionTrue (set allReady = false). This ensures stale
conditions are treated as pending until the mode controllers update them.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 26a68016-47ce-46c2-ac59-401c729d2609
📒 Files selected for processing (6)
cmd/main.gointernal/controller/serverbootconfig_helpers.gointernal/controller/serverbootconfiguration_http_controller.gointernal/controller/serverbootconfiguration_pxe_controller.gointernal/controller/serverbootconfiguration_readiness_controller.gointernal/controller/suite_test.go
cmd/main.go
Outdated
| serverBootConfigControllerPxe, | ||
| serverBootConfigControllerHttp, | ||
| httpBootConfigController, | ||
| serverBootConfigReadiness, |
There was a problem hiding this comment.
Don't make readiness independently disable-able.
ServerBootConfigurationReadinessReconciler is now the only writer of ServerBootConfiguration.Status.State. Any deployment that passes an explicit --controllers= allow-list and doesn't include this new switch will keep running the HTTP/PXE reconcilers, but Status.State will stop changing entirely. Please auto-enable readiness whenever either mode controller is enabled, or fail fast on that invalid combination.
Also applies to: 289-299
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/main.go` at line 112, The readiness controller (serverBootConfigReadiness
/ ServerBootConfigurationReadinessReconciler) must not be independently
disable-able because it's the sole writer of
ServerBootConfiguration.Status.State; update cmd/main.go so that when either
mode controller (HTTP or PXE reconcilers) is enabled the readiness controller is
automatically enabled, or else validate the --controllers allow-list at startup
and exit with an error if a mode controller is enabled while
serverBootConfigReadiness is disabled; modify the controller initialization or
add a startup check that references serverBootConfigReadiness and the mode
controller flags and either force-enable readiness or fail fast on the invalid
combination.
| if err := r.patchHTTPBootReadyConditionFromHTTPState(ctx, httpBootConfig, config); err != nil { | ||
| return ctrl.Result{}, fmt.Errorf("failed to patch %s condition from HTTPBootConfig state %s: %w", HTTPBootReadyConditionType, httpBootConfig.Status.State, err) | ||
| } | ||
| log.V(1).Info("Patched server boot config state") | ||
| log.V(1).Info("Patched server boot config condition", "condition", HTTPBootReadyConditionType) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the serverbootconfiguration_http_controller.go file
fd -t f "serverbootconfiguration_http_controller.go" | head -5Repository: ironcore-dev/boot-operator
Length of output: 132
🏁 Script executed:
# Get the full file to understand the context
cat -n internal/controller/serverbootconfiguration_http_controller.go | head -200Repository: ironcore-dev/boot-operator
Length of output: 9707
🏁 Script executed:
# Focus on the critical sections mentioned in the review
sed -n '85,105p' internal/controller/serverbootconfiguration_http_controller.goRepository: ironcore-dev/boot-operator
Length of output: 1014
🏁 Script executed:
# Check lines around 135-181 (child apply and status handling)
sed -n '130,185p' internal/controller/serverbootconfiguration_http_controller.goRepository: ironcore-dev/boot-operator
Length of output: 2617
🏁 Script executed:
# Find the HTTPBootConfig API definition
fd -t f ".*http.*boot.*" | grep -i "api\|types" | head -10Repository: ironcore-dev/boot-operator
Length of output: 106
🏁 Script executed:
# Search for HTTPBootConfig definition
rg "type HTTPBootConfig" -A 20Repository: ironcore-dev/boot-operator
Length of output: 6726
🏁 Script executed:
# Look for the status structure
rg "HTTPBootConfigStatus" -A 15Repository: ironcore-dev/boot-operator
Length of output: 10881
🏁 Script executed:
# Examine the HTTPBootConfig controller reconciliation
cat -n internal/controller/httpbootconfig_controller.go | head -100Repository: ironcore-dev/boot-operator
Length of output: 4489
🏁 Script executed:
# Look for where the HTTPBootConfig controller sets status
rg "Status\.State\|SetStatusCondition\|ObservedGeneration" internal/controller/httpbootconfig_controller.go -B 2 -A 2Repository: ironcore-dev/boot-operator
Length of output: 52
🏁 Script executed:
# Continue reading the patchStatus method
sed -n '97,120p' internal/controller/httpbootconfig_controller.goRepository: ironcore-dev/boot-operator
Length of output: 885
🏁 Script executed:
# Check if there's any ObservedGeneration tracking at all in the HTTPBootConfig controller
rg "Generation" internal/controller/httpbootconfig_controller.goRepository: ironcore-dev/boot-operator
Length of output: 52
🏁 Script executed:
# Look at the full patchStatus implementation
sed -n '97,115p' internal/controller/httpbootconfig_controller.goRepository: ironcore-dev/boot-operator
Length of output: 728
Don't reuse old HTTPBootConfig status for the current SBC generation.
This path executes right after applying the child spec, so HTTPBootConfig.Status.State may still reflect the previous generation's status. The parent condition is then stamped with the current ServerBootConfiguration.Generation, making stale Ready/Error data appear current after a spec change. Until the HTTPBootConfig controller has reconciled the new spec, write HTTPBootReady=Unknown instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/controller/serverbootconfiguration_http_controller.go` around lines
144 - 147, The code currently uses HTTPBootConfig.Status.State directly when
patching the parent condition, which can be stale if the child HTTPBootConfig
hasn't reconciled the new generation; update the logic so that when calling or
inside patchHTTPBootReadyConditionFromHTTPState you compare
httpBootConfig.ObjectMeta.Generation (or httpBootConfig.Generation) with the
parent ServerBootConfiguration.Generation (config.Generation) and treat the
child's state as Unknown when they differ. Concretely: modify
patchHTTPBootReadyConditionFromHTTPState (or its call) to detect generation
mismatch and map any non-current-child state to metav1.ConditionUnknown (or the
equivalent Unknown/Unknown constant) before creating/updating the
HTTPBootReadyCondition (HTTPBootReadyConditionType), ensuring the parent
condition is set to Unknown until the child reconciles.
| if r.RequireHTTPBoot { | ||
| c := apimeta.FindStatusCondition(cfg.Status.Conditions, HTTPBootReadyConditionType) | ||
| switch { | ||
| case c == nil: | ||
| allReady = false | ||
| case c.Status == metav1.ConditionFalse: | ||
| hasError = true | ||
| case c.Status != metav1.ConditionTrue: | ||
| allReady = false | ||
| } | ||
| } | ||
|
|
||
| if r.RequireIPXEBoot { | ||
| c := apimeta.FindStatusCondition(cfg.Status.Conditions, IPXEBootReadyConditionType) | ||
| switch { | ||
| case c == nil: | ||
| allReady = false | ||
| case c.Status == metav1.ConditionFalse: | ||
| hasError = true | ||
| case c.Status != metav1.ConditionTrue: | ||
| allReady = false | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n internal/controller/serverbootconfiguration_readiness_controller.goRepository: ironcore-dev/boot-operator
Length of output: 3972
🏁 Script executed:
# Find the ServerBootConfiguration type definition
find . -name "*.go" -type f | xargs grep -l "type ServerBootConfiguration struct" | head -5Repository: ironcore-dev/boot-operator
Length of output: 52
🏁 Script executed:
# Search for HTTPBootReadyConditionType and IPXEBootReadyConditionType usage
rg "HTTPBootReadyConditionType|IPXEBootReadyConditionType" -A 3 -B 3Repository: ironcore-dev/boot-operator
Length of output: 8224
🏁 Script executed:
# Search for other uses of FindStatusCondition to see if there's a pattern
rg "FindStatusCondition" -B 2 -A 5Repository: ironcore-dev/boot-operator
Length of output: 2501
🏁 Script executed:
# Search for .Generation usage in the codebase
rg "\.Generation" -B 2 -A 2 | head -50Repository: ironcore-dev/boot-operator
Length of output: 3489
Treat stale mode conditions as pending.
The aggregator ignores Condition.ObservedGeneration. After a ServerBootConfiguration spec change, old HTTPBootReady / IPXEBootReady values can keep Status.State at Ready or Error until the mode controllers rewrite them. ObservedGeneration != cfg.Generation should be handled the same as a missing condition.
Suggested guard
if r.RequireHTTPBoot {
c := apimeta.FindStatusCondition(cfg.Status.Conditions, HTTPBootReadyConditionType)
switch {
- case c == nil:
+ case c == nil || c.ObservedGeneration != cfg.Generation:
allReady = false
case c.Status == metav1.ConditionFalse:
hasError = true
case c.Status != metav1.ConditionTrue:
allReady = false
}
}
if r.RequireIPXEBoot {
c := apimeta.FindStatusCondition(cfg.Status.Conditions, IPXEBootReadyConditionType)
switch {
- case c == nil:
+ case c == nil || c.ObservedGeneration != cfg.Generation:
allReady = false
case c.Status == metav1.ConditionFalse:
hasError = true
case c.Status != metav1.ConditionTrue:
allReady = false
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if r.RequireHTTPBoot { | |
| c := apimeta.FindStatusCondition(cfg.Status.Conditions, HTTPBootReadyConditionType) | |
| switch { | |
| case c == nil: | |
| allReady = false | |
| case c.Status == metav1.ConditionFalse: | |
| hasError = true | |
| case c.Status != metav1.ConditionTrue: | |
| allReady = false | |
| } | |
| } | |
| if r.RequireIPXEBoot { | |
| c := apimeta.FindStatusCondition(cfg.Status.Conditions, IPXEBootReadyConditionType) | |
| switch { | |
| case c == nil: | |
| allReady = false | |
| case c.Status == metav1.ConditionFalse: | |
| hasError = true | |
| case c.Status != metav1.ConditionTrue: | |
| allReady = false | |
| } | |
| if r.RequireHTTPBoot { | |
| c := apimeta.FindStatusCondition(cfg.Status.Conditions, HTTPBootReadyConditionType) | |
| switch { | |
| case c == nil || c.ObservedGeneration != cfg.Generation: | |
| allReady = false | |
| case c.Status == metav1.ConditionFalse: | |
| hasError = true | |
| case c.Status != metav1.ConditionTrue: | |
| allReady = false | |
| } | |
| } | |
| if r.RequireIPXEBoot { | |
| c := apimeta.FindStatusCondition(cfg.Status.Conditions, IPXEBootReadyConditionType) | |
| switch { | |
| case c == nil || c.ObservedGeneration != cfg.Generation: | |
| allReady = false | |
| case c.Status == metav1.ConditionFalse: | |
| hasError = true | |
| case c.Status != metav1.ConditionTrue: | |
| allReady = false | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/controller/serverbootconfiguration_readiness_controller.go` around
lines 55 - 76, When evaluating HTTPBootReadyConditionType and
IPXEBootReadyConditionType (in the RequireHTTPBoot / RequireIPXEBoot blocks)
treat conditions whose ObservedGeneration != cfg.Generation as if they were
missing: after calling apimeta.FindStatusCondition(cfg.Status.Conditions, ...),
consider the condition nil if c == nil OR c.ObservedGeneration !=
cfg.Generation, and then set allReady = false for that case; keep the existing
handling for c.Status == metav1.ConditionFalse (set hasError = true) and
c.Status != metav1.ConditionTrue (set allReady = false). This ensures stale
conditions are treated as pending until the mode controllers update them.
hardikdr
force-pushed
the
enh/dual-mode-httpboot-ipxe
branch
from
2e13bdc to
97f67dd
Compare
|
Wouldn't the implementation of ironcore-dev/metal-operator#654, and then producing either an httpBootConfig or an ipxeBootConfig according to the BootMethod, eliminate the race condition without the need for another controller? |
hardikdr
force-pushed
the
enh/dual-mode-httpboot-ipxe
branch
from
97f67dd to
72da76e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
internal/controller/serverbootconfig_helpers.go (1)
149-170:⚠️ Potential issue | 🟠 Major
RetryOnConflictstill isn't conflict-safe with plainMergeFrom.
client.MergeFrom(base)builds a merge patch that replaces modified list fields, and controller-runtime only addsmetadata.resourceVersionpreconditions when you opt intoMergeFromWithOptimisticLock. With the current code, concurrent HTTP/PXE writers can still silently clobber each other'sstatus.conditions, so this helper does not actually close the dual-mode lost-update race. Please switch this helper toclient.MergeFromWithOptions(base, client.MergeFromWithOptimisticLock{})and have the inline HTTP/PXE patchers delegate here so the same fix applies everywhere. (pkg.go.dev)🔧 Suggested direction
- return c.Status().Patch(ctx, &cur, client.MergeFrom(base)) + return c.Status().Patch( + ctx, + &cur, + client.MergeFromWithOptions(base, client.MergeFromWithOptimisticLock{}), + )Does controller-runtime's client.MergeFrom include conflict detection by default, and what does MergeFromWithOptimisticLock add?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/serverbootconfig_helpers.go` around lines 149 - 170, PatchServerBootConfigCondition currently uses client.MergeFrom(base) which creates a merge patch without optimistic-lock preconditions, so concurrent status.conditions updates can be lost; change the final Patch call to use client.MergeFromWithOptions(base, client.MergeFromWithOptimisticLock{}) so controller-runtime will include metadata.resourceVersion preconditions, and update any inline HTTP/PXE patchers to delegate to this helper (or call the same MergeFromWithOptions) to ensure both writers use optimistic locking with RetryOnConflict.cmd/main.go (1)
109-116:⚠️ Potential issue | 🟠 MajorDon't let mode controllers run without readiness.
ServerBootConfigurationReadinessReconcileris now the only writer ofServerBootConfiguration.Status.State, but--controllerscan still enableserverbootconfighttpand/orserverbootconfigpxewhile omittingserverbootconfigreadiness. In that deployment shape, per-mode conditions keep changing andStatus.Statefreezes. Please either auto-enable readiness whenever a mode controller is enabled, or fail fast on that invalid combination.🚫 Example fail-fast guard
flag.Parse() + + if (controllers.Enabled(serverBootConfigControllerHttp) || controllers.Enabled(serverBootConfigControllerPxe)) && + !controllers.Enabled(serverBootConfigReadiness) { + setupLog.Error(nil, "invalid --controllers selection: serverbootconfigreadiness is required when enabling serverbootconfighttp or serverbootconfigpxe") + os.Exit(1) + }Also applies to: 299-309
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/main.go` around lines 109 - 116, When constructing the controllers via switches.New (the controllers variable) ensure that serverbootconfigreadiness is present whenever serverbootconfigpxe (serverBootConfigControllerPxe) or serverbootconfighttp (serverBootConfigControllerHttp) is being enabled by --controllers: either automatically add serverBootConfigReadiness into the controllers slice before calling switches.New, or validate the parsed controller set and fail fast (log.Fatal / return error) if PXE or HTTP mode is enabled without serverBootConfigReadiness; update the construction logic that currently lists ipxeBootConfigController, serverBootConfigControllerPxe, serverBootConfigControllerHttp, httpBootConfigController, serverBootConfigReadiness to perform this check and enforce the invariant.internal/controller/serverbootconfiguration_pxe_controller.go (1)
165-205:⚠️ Potential issue | 🟠 MajorDon't publish current-generation
IPXEBootReadyfrom a just-applied child.The post-apply read feeding this method can still return the previous
IPXEBootConfig.Status.State, but Line 185 stamps that result onto the currentServerBootConfigurationgeneration. A staleReady/Errorhere makes the parent look current before the child has actually processed the new spec. Please only promoteTrue/Falseonce the child can prove it has observed the latest spec (for example via a childstatus.observedGeneration); otherwise keepIPXEBootReadyasUnknown.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/serverbootconfiguration_pxe_controller.go` around lines 165 - 205, The current patchIPXEBootReadyConditionFromIPXEState function uses config.Status.State directly to stamp True/False on the parent even if the child hasn't observed the latest spec; change the logic to only promote ConditionTrue/ConditionFalse when the child has proven it observed the config generation (e.g., compare config.Status.ObservedGeneration >= config.Generation), otherwise set the IPXEBootReady condition to Unknown/BootConfigPending; update the switch in patchIPXEBootReadyConditionFromIPXEState to check config.Status.ObservedGeneration first (and only set metav1.ConditionTrue/False for v1alpha1.IPXEBootConfigStateReady/Error when the observedGeneration is up-to-date), leaving the default/awaiting case as Unknown and keep Reason/Message accordingly.internal/controller/serverbootconfiguration_http_controller.go (1)
146-185:⚠️ Potential issue | 🟠 MajorDon't promote stale
HTTPBootConfigstatus to the current parent generation.This path reads
httpBootConfig.Status.Stateimmediately after applying the child, so it can still be carrying the previous child reconcile result, while Line 166 marks it as observed for the currentServerBootConfigurationgeneration. That can makeHTTPBootReadyflip toTrue/Falsetoo early. Please gateTrue/Falseon proof that the child has observed the latest spec (for example a childstatus.observedGeneration); otherwise keep the parent conditionUnknown.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/serverbootconfiguration_http_controller.go` around lines 146 - 185, In patchHTTPBootReadyConditionFromHTTPState, don't set cond.Status to True/False based solely on httpBootConfig.Status.State; first verify the child has observed the parent generation (check httpBootConfig.Status.ObservedGeneration or similar) against the parent generation used for cond.ObservedGeneration (cur.Generation or cfg.Generation). If the child's observedGeneration is less than the parent's current generation, set cond.Status = metav1.ConditionUnknown with a Pending reason/message; only when observedGeneration >= parent generation map HTTPBootConfigStateReady -> ConditionTrue and HTTPBootConfigStateError -> ConditionFalse; update the logic around httpBootConfig.Status.State and the metav1.Condition construction in patchHTTPBootReadyConditionFromHTTPState (referencing HTTPBootReadyConditionType, cond, httpBootConfig.Status.State, and cond.ObservedGeneration).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/controller/serverbootconfiguration_readiness_controller_test.go`:
- Around line 1-2: Update the SPDX header in
internal/controller/serverbootconfiguration_readiness_controller_test.go to the
repo-standard exact two-line header: replace the current year/token with the
canonical lines "// SPDX-FileCopyrightText: 2024 SAP SE or an SAP affiliate
company and IronCore contributors" and "// SPDX-License-Identifier: Apache-2.0"
at the top of the file so the test file matches other Go files in the
repository.
---
Duplicate comments:
In `@cmd/main.go`:
- Around line 109-116: When constructing the controllers via switches.New (the
controllers variable) ensure that serverbootconfigreadiness is present whenever
serverbootconfigpxe (serverBootConfigControllerPxe) or serverbootconfighttp
(serverBootConfigControllerHttp) is being enabled by --controllers: either
automatically add serverBootConfigReadiness into the controllers slice before
calling switches.New, or validate the parsed controller set and fail fast
(log.Fatal / return error) if PXE or HTTP mode is enabled without
serverBootConfigReadiness; update the construction logic that currently lists
ipxeBootConfigController, serverBootConfigControllerPxe,
serverBootConfigControllerHttp, httpBootConfigController,
serverBootConfigReadiness to perform this check and enforce the invariant.
In `@internal/controller/serverbootconfig_helpers.go`:
- Around line 149-170: PatchServerBootConfigCondition currently uses
client.MergeFrom(base) which creates a merge patch without optimistic-lock
preconditions, so concurrent status.conditions updates can be lost; change the
final Patch call to use client.MergeFromWithOptions(base,
client.MergeFromWithOptimisticLock{}) so controller-runtime will include
metadata.resourceVersion preconditions, and update any inline HTTP/PXE patchers
to delegate to this helper (or call the same MergeFromWithOptions) to ensure
both writers use optimistic locking with RetryOnConflict.
In `@internal/controller/serverbootconfiguration_http_controller.go`:
- Around line 146-185: In patchHTTPBootReadyConditionFromHTTPState, don't set
cond.Status to True/False based solely on httpBootConfig.Status.State; first
verify the child has observed the parent generation (check
httpBootConfig.Status.ObservedGeneration or similar) against the parent
generation used for cond.ObservedGeneration (cur.Generation or cfg.Generation).
If the child's observedGeneration is less than the parent's current generation,
set cond.Status = metav1.ConditionUnknown with a Pending reason/message; only
when observedGeneration >= parent generation map HTTPBootConfigStateReady ->
ConditionTrue and HTTPBootConfigStateError -> ConditionFalse; update the logic
around httpBootConfig.Status.State and the metav1.Condition construction in
patchHTTPBootReadyConditionFromHTTPState (referencing
HTTPBootReadyConditionType, cond, httpBootConfig.Status.State, and
cond.ObservedGeneration).
In `@internal/controller/serverbootconfiguration_pxe_controller.go`:
- Around line 165-205: The current patchIPXEBootReadyConditionFromIPXEState
function uses config.Status.State directly to stamp True/False on the parent
even if the child hasn't observed the latest spec; change the logic to only
promote ConditionTrue/ConditionFalse when the child has proven it observed the
config generation (e.g., compare config.Status.ObservedGeneration >=
config.Generation), otherwise set the IPXEBootReady condition to
Unknown/BootConfigPending; update the switch in
patchIPXEBootReadyConditionFromIPXEState to check
config.Status.ObservedGeneration first (and only set metav1.ConditionTrue/False
for v1alpha1.IPXEBootConfigStateReady/Error when the observedGeneration is
up-to-date), leaving the default/awaiting case as Unknown and keep
Reason/Message accordingly.
🪄 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: c39714dd-e103-4cae-9eb4-0250113cdeb4
📒 Files selected for processing (7)
cmd/main.gointernal/controller/serverbootconfig_helpers.gointernal/controller/serverbootconfiguration_http_controller.gointernal/controller/serverbootconfiguration_pxe_controller.gointernal/controller/serverbootconfiguration_readiness_controller.gointernal/controller/serverbootconfiguration_readiness_controller_test.gointernal/controller/suite_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/controller/suite_test.go
- internal/controller/serverbootconfiguration_readiness_controller.go
| // SPDX-FileCopyrightText: 2026 SAP SE or an SAP affiliate company and IronCore contributors | ||
| // SPDX-License-Identifier: Apache-2.0 |
There was a problem hiding this comment.
Use the repo-standard SPDX header.
Line 1 uses 2026, but this repository requires the exact standard header text for Go files. Please align this new test file with the existing SPDX template.
As per coding guidelines, **/*.go: All Go files require an SPDX header: // SPDX-FileCopyrightText: 2024 SAP SE or an SAP affiliate company and IronCore contributors and // SPDX-License-Identifier: Apache-2.0
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/controller/serverbootconfiguration_readiness_controller_test.go`
around lines 1 - 2, Update the SPDX header in
internal/controller/serverbootconfiguration_readiness_controller_test.go to the
repo-standard exact two-line header: replace the current year/token with the
canonical lines "// SPDX-FileCopyrightText: 2024 SAP SE or an SAP affiliate
company and IronCore contributors" and "// SPDX-License-Identifier: Apache-2.0"
at the top of the file so the test file matches other Go files in the
repository.
72da76e to
a6baf8d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
internal/controller/serverbootconfiguration_readiness_controller.go (1)
86-107:⚠️ Potential issue | 🟠 MajorIgnore stale per-mode conditions when aggregating readiness.
computeDesiredStateonly checks whether the condition exists and what its status is. After aServerBootConfigurationspec change, an oldHTTPBootReady/IPXEBootReadycondition can keepStatus.StateatReadyorErroruntil the mode controller rewrites it. TreatObservedGeneration != cfg.Generationthe same as a missing condition so stale data remains pending.🛠️ Proposed fix
if requireHTTP { c := apimeta.FindStatusCondition(cfg.Status.Conditions, HTTPBootReadyConditionType) switch { - case c == nil: + case c == nil || c.ObservedGeneration != cfg.Generation: allReady = false case c.Status == metav1.ConditionFalse: hasError = true case c.Status != metav1.ConditionTrue: allReady = false } } if requireIPXE { c := apimeta.FindStatusCondition(cfg.Status.Conditions, IPXEBootReadyConditionType) switch { - case c == nil: + case c == nil || c.ObservedGeneration != cfg.Generation: allReady = false case c.Status == metav1.ConditionFalse: hasError = true case c.Status != metav1.ConditionTrue: allReady = false } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/serverbootconfiguration_readiness_controller.go` around lines 86 - 107, The readiness aggregation should ignore stale per-mode conditions: in computeDesiredState when inspecting HTTPBootReadyConditionType and IPXEBootReadyConditionType (via apimeta.FindStatusCondition on cfg.Status.Conditions), treat a condition whose ObservedGeneration != cfg.Generation the same as a missing condition (i.e., set allReady = false) so stale Ready/Error values do not affect aggregation; keep the existing behavior that a present condition with Status == metav1.ConditionFalse still sets hasError = true, but ensure the ObservedGeneration check is applied before trusting Status for both HTTPBootReadyConditionType and IPXEBootReadyConditionType.internal/controller/serverbootconfiguration_http_controller.go (1)
146-147:⚠️ Potential issue | 🟠 MajorDon't publish the child's old state as current readiness.
This path reads
httpBootConfig.Status.Stateimmediately after applying the child spec. At that point the status can still belong to the previousHTTPBootConfiggeneration, but the parent condition is stamped withObservedGeneration=cur.Generation, so stale Ready/Error can be treated as current. Please keepHTTPBootReadyatUnknownuntil the child has reconciled the new spec, or gate the Ready/Error mapping on a child freshness signal.Also applies to: 155-185
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/serverbootconfiguration_http_controller.go` around lines 146 - 147, The code is publishing the child's previous Status.State as current readiness; instead gate mapping on child freshness: in r.patchHTTPBootReadyConditionFromHTTPState (and the other similar call sites 155-185) detect if httpBootConfig.Status.ObservedGeneration < httpBootConfig.Generation (or otherwise indicates the child hasn't reconciled the new spec) and in that case set HTTPBootReadyConditionType to Unknown (do not map Ready/Error from httpBootConfig.Status.State); only map Ready/Error when ObservedGeneration is up-to-date. Also avoid reading httpBootConfig.Status.State for error messages when the child is stale—use a clear "stale/unknown" message or include the freshness check result instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/controller/serverbootconfiguration_readiness_controller.go`:
- Around line 1-2: The SPDX header in the new file uses year 2026 instead of the
repo-standard 2024; update the top two lines so they match the repository
requirement by changing "// SPDX-FileCopyrightText: 2026 SAP SE or an SAP
affiliate company and IronCore contributors" to use 2024 and keep the existing
"// SPDX-License-Identifier: Apache-2.0" line unchanged so the file header
matches the repository standard.
---
Duplicate comments:
In `@internal/controller/serverbootconfiguration_http_controller.go`:
- Around line 146-147: The code is publishing the child's previous Status.State
as current readiness; instead gate mapping on child freshness: in
r.patchHTTPBootReadyConditionFromHTTPState (and the other similar call sites
155-185) detect if httpBootConfig.Status.ObservedGeneration <
httpBootConfig.Generation (or otherwise indicates the child hasn't reconciled
the new spec) and in that case set HTTPBootReadyConditionType to Unknown (do not
map Ready/Error from httpBootConfig.Status.State); only map Ready/Error when
ObservedGeneration is up-to-date. Also avoid reading httpBootConfig.Status.State
for error messages when the child is stale—use a clear "stale/unknown" message
or include the freshness check result instead.
In `@internal/controller/serverbootconfiguration_readiness_controller.go`:
- Around line 86-107: The readiness aggregation should ignore stale per-mode
conditions: in computeDesiredState when inspecting HTTPBootReadyConditionType
and IPXEBootReadyConditionType (via apimeta.FindStatusCondition on
cfg.Status.Conditions), treat a condition whose ObservedGeneration !=
cfg.Generation the same as a missing condition (i.e., set allReady = false) so
stale Ready/Error values do not affect aggregation; keep the existing behavior
that a present condition with Status == metav1.ConditionFalse still sets
hasError = true, but ensure the ObservedGeneration check is applied before
trusting Status for both HTTPBootReadyConditionType and
IPXEBootReadyConditionType.
🪄 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: 365b6f38-da3a-483d-bbb8-ec248ce5a591
📒 Files selected for processing (6)
api/v1alpha1/zz_generated.deepcopy.gointernal/controller/serverbootconfig_helpers.gointernal/controller/serverbootconfiguration_http_controller.gointernal/controller/serverbootconfiguration_pxe_controller.gointernal/controller/serverbootconfiguration_readiness_controller.gointernal/controller/serverbootconfiguration_readiness_controller_test.go
✅ Files skipped from review due to trivial changes (2)
- api/v1alpha1/zz_generated.deepcopy.go
- internal/controller/serverbootconfiguration_readiness_controller_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/controller/serverbootconfig_helpers.go
- internal/controller/serverbootconfiguration_pxe_controller.go
| // SPDX-FileCopyrightText: 2026 SAP SE or an SAP affiliate company and IronCore contributors | ||
| // SPDX-License-Identifier: Apache-2.0 |
There was a problem hiding this comment.
Use the repository-standard SPDX header text.
This new file uses 2026 in the copyright line, but the repository standard here is 2024.
🪪 Proposed fix
-// SPDX-FileCopyrightText: 2026 SAP SE or an SAP affiliate company and IronCore contributors
+// SPDX-FileCopyrightText: 2024 SAP SE or an SAP affiliate company and IronCore contributors
// SPDX-License-Identifier: Apache-2.0As per coding guidelines, **/*.go: All Go files require an SPDX header: // SPDX-FileCopyrightText: 2024 SAP SE or an SAP affiliate company and IronCore contributors and // SPDX-License-Identifier: Apache-2.0
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // SPDX-FileCopyrightText: 2026 SAP SE or an SAP affiliate company and IronCore contributors | |
| // SPDX-License-Identifier: Apache-2.0 | |
| // SPDX-FileCopyrightText: 2024 SAP SE or an SAP affiliate company and IronCore contributors | |
| // SPDX-License-Identifier: Apache-2.0 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/controller/serverbootconfiguration_readiness_controller.go` around
lines 1 - 2, The SPDX header in the new file uses year 2026 instead of the
repo-standard 2024; update the top two lines so they match the repository
requirement by changing "// SPDX-FileCopyrightText: 2026 SAP SE or an SAP
affiliate company and IronCore contributors" to use 2024 and keep the existing
"// SPDX-License-Identifier: Apache-2.0" line unchanged so the file header
matches the repository standard.
a6baf8d to
0edbc58
Compare
hardikdr
force-pushed
the
enh/dual-mode-httpboot-ipxe
branch
from
0edbc58 to
6f510fd
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (4)
internal/controller/serverbootconfiguration_readiness_controller_test.go (1)
1-2:⚠️ Potential issue | 🟡 MinorUse the repository SPDX header verbatim.
Line 1 uses
2026, but this repo requires the canonical2024header text on Go files.As per coding guidelines,
**/*.go: All Go files require an SPDX header:// SPDX-FileCopyrightText: 2024 SAP SE or an SAP affiliate company and IronCore contributorsand// SPDX-License-Identifier: Apache-2.0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/serverbootconfiguration_readiness_controller_test.go` around lines 1 - 2, Replace the incorrect SPDX year header: update the two top-of-file SPDX comment lines in serverbootconfiguration_readiness_controller_test.go so they exactly match the repository canonical header text, i.e. change the year from 2026 to 2024 and ensure both lines are present as "// SPDX-FileCopyrightText: 2024 SAP SE or an SAP affiliate company and IronCore contributors" and "// SPDX-License-Identifier: Apache-2.0".internal/controller/serverbootconfiguration_pxe_controller.go (1)
165-204:⚠️ Potential issue | 🟠 MajorDon't mark the new SBC generation from a stale
IPXEBootConfigstatus.
config.Status.Stateis read immediately after the Apply, before theIPXEBootConfigcontroller has necessarily observed the new spec. This helper then stampsObservedGeneration: cur.Generation, so an old childReady/Errorcan be promoted as current for the newServerBootConfigurationgeneration. Please keepIPXEBootReadyinUnknownuntil the child can prove it processed the latest spec, or add generation tracking toIPXEBootConfig.Status.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/serverbootconfiguration_pxe_controller.go` around lines 165 - 204, The helper patchIPXEBootReadyConditionFromIPXEState must not promote a stale child status: instead of using config.Status.State immediately, set IPXEBootReady condition to Unknown unless the IPXEBootConfig has actually observed the new spec; change the logic in patchIPXEBootReadyConditionFromIPXEState to check generation tracking (e.g. require config.Status.ObservedGeneration >= config.Generation or a dedicated Status.ObservedSpecGeneration field) before mapping config.Status.State to True/False, and default to Unknown (Reason "BootConfigPending") when the child's observed generation is older; update any references to config.Status.State accordingly and add generation tracking to IPXEBootConfig.Status if it doesn't exist.internal/controller/serverbootconfiguration_readiness_controller.go (2)
1-2:⚠️ Potential issue | 🟡 MinorUse the repository SPDX header verbatim.
Line 1 uses
2026, but this repo requires the canonical2024header text on Go files.As per coding guidelines,
**/*.go: All Go files require an SPDX header:// SPDX-FileCopyrightText: 2024 SAP SE or an SAP affiliate company and IronCore contributorsand// SPDX-License-Identifier: Apache-2.0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/serverbootconfiguration_readiness_controller.go` around lines 1 - 2, Replace the incorrect SPDX header year in the file's top comment: change the two header lines that currently contain "2026" to the canonical repository header "2024" so they read "// SPDX-FileCopyrightText: 2024 SAP SE or an SAP affiliate company and IronCore contributors" and "// SPDX-License-Identifier: Apache-2.0"; update the header in serverbootconfiguration_readiness_controller.go wherever the SPDX lines appear.
86-108:⚠️ Potential issue | 🟠 MajorTreat stale mode conditions as pending.
After a
ServerBootConfigurationspec change, the previousHTTPBootReady/IPXEBootReadyconditions remain on the object until the mode controllers rewrite them. BecausecomputeDesiredStateignoresObservedGeneration, those stale values can keepStatus.StateatReadyorErrorfor the new generation instead of falling back toPending.🛠️ Minimal guard
if requireHTTP { c := apimeta.FindStatusCondition(cfg.Status.Conditions, HTTPBootReadyConditionType) switch { - case c == nil: + case c == nil || c.ObservedGeneration != cfg.Generation: allReady = false case c.Status == metav1.ConditionFalse: hasError = true case c.Status != metav1.ConditionTrue: allReady = false } } if requireIPXE { c := apimeta.FindStatusCondition(cfg.Status.Conditions, IPXEBootReadyConditionType) switch { - case c == nil: + case c == nil || c.ObservedGeneration != cfg.Generation: allReady = false case c.Status == metav1.ConditionFalse: hasError = true case c.Status != metav1.ConditionTrue: allReady = false } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/serverbootconfiguration_readiness_controller.go` around lines 86 - 108, The mode-ready checks in computeDesiredState treat stale HTTPBootReady/IPXEBootReady conditions as current; change the logic that reads apimeta.FindStatusCondition(cfg.Status.Conditions, HTTPBootReadyConditionType) and IPXEBootReadyConditionType to treat a condition whose ObservedGeneration != cfg.Generation as "pending" (i.e., act like c == nil -> set allReady = false) so old-generation False/True values don't keep Status.State at Ready/Error; only set hasError = true when c is present, c.ObservedGeneration == cfg.Generation, and c.Status == metav1.ConditionFalse, otherwise set allReady = false for nil/stale/non-True conditions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@internal/controller/serverbootconfiguration_pxe_controller.go`:
- Around line 165-204: The helper patchIPXEBootReadyConditionFromIPXEState must
not promote a stale child status: instead of using config.Status.State
immediately, set IPXEBootReady condition to Unknown unless the IPXEBootConfig
has actually observed the new spec; change the logic in
patchIPXEBootReadyConditionFromIPXEState to check generation tracking (e.g.
require config.Status.ObservedGeneration >= config.Generation or a dedicated
Status.ObservedSpecGeneration field) before mapping config.Status.State to
True/False, and default to Unknown (Reason "BootConfigPending") when the child's
observed generation is older; update any references to config.Status.State
accordingly and add generation tracking to IPXEBootConfig.Status if it doesn't
exist.
In `@internal/controller/serverbootconfiguration_readiness_controller_test.go`:
- Around line 1-2: Replace the incorrect SPDX year header: update the two
top-of-file SPDX comment lines in
serverbootconfiguration_readiness_controller_test.go so they exactly match the
repository canonical header text, i.e. change the year from 2026 to 2024 and
ensure both lines are present as "// SPDX-FileCopyrightText: 2024 SAP SE or an
SAP affiliate company and IronCore contributors" and "//
SPDX-License-Identifier: Apache-2.0".
In `@internal/controller/serverbootconfiguration_readiness_controller.go`:
- Around line 1-2: Replace the incorrect SPDX header year in the file's top
comment: change the two header lines that currently contain "2026" to the
canonical repository header "2024" so they read "// SPDX-FileCopyrightText: 2024
SAP SE or an SAP affiliate company and IronCore contributors" and "//
SPDX-License-Identifier: Apache-2.0"; update the header in
serverbootconfiguration_readiness_controller.go wherever the SPDX lines appear.
- Around line 86-108: The mode-ready checks in computeDesiredState treat stale
HTTPBootReady/IPXEBootReady conditions as current; change the logic that reads
apimeta.FindStatusCondition(cfg.Status.Conditions, HTTPBootReadyConditionType)
and IPXEBootReadyConditionType to treat a condition whose ObservedGeneration !=
cfg.Generation as "pending" (i.e., act like c == nil -> set allReady = false) so
old-generation False/True values don't keep Status.State at Ready/Error; only
set hasError = true when c is present, c.ObservedGeneration == cfg.Generation,
and c.Status == metav1.ConditionFalse, otherwise set allReady = false for
nil/stale/non-True conditions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 302da5dd-e265-4b5a-b345-eb1d05949343
📒 Files selected for processing (7)
cmd/main.gointernal/controller/serverbootconfig_helpers.gointernal/controller/serverbootconfiguration_http_controller.gointernal/controller/serverbootconfiguration_pxe_controller.gointernal/controller/serverbootconfiguration_readiness_controller.gointernal/controller/serverbootconfiguration_readiness_controller_test.gointernal/controller/suite_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/controller/serverbootconfig_helpers.go
- internal/controller/serverbootconfiguration_http_controller.go
2 participants
Summary by CodeRabbit
Release Notes