Aggregate HTTPBoot and iPXE Conditions to Compute ServerBootConfiguration Readiness#292
Open
Aggregate HTTPBoot and iPXE Conditions to Compute ServerBootConfiguration Readiness#292
Conversation
Contributor
|
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:
WalkthroughAdds a readiness controller that computes ServerBootConfiguration.Status.State from HTTP/IPXE readiness conditions; refactors HTTP/PXE controllers to set per-mode readiness conditions; adds a conflict-safe condition patch helper; records observedGeneration on boot config statuses; and includes tests and CRD schema updates. Changes
Sequence Diagram(s)sequenceDiagram
actor K8s as Kubernetes API
participant HC as HTTP Controller
participant PC as PXE Controller
participant SBC as ServerBootConfiguration
participant RC as Readiness Controller
K8s->>HC: Reconcile HTTPBootConfig
HC->>HC: validate/build UKI URL & state
HC->>SBC: Patch `HTTPBootReady` condition (True/False/Unknown)
K8s->>PC: Reconcile IPXEBootConfig
PC->>PC: resolve image details & state
PC->>SBC: Patch `IPXEBootReady` condition (True/False/Unknown)
K8s->>RC: Reconcile ServerBootConfiguration (on condition change)
RC->>SBC: GET current conditions
RC->>RC: compute desired Status.State from required conditions
RC->>SBC: Status.Patch Status.State if changed (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 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}
Contributor
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, |
Contributor
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.
Comment on lines
+55
to
+76
| 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 | ||
| } |
Contributor
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.
Suggested change
| 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
Contributor
|
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
Contributor
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
Comment on lines
+1
to
+2
| // SPDX-FileCopyrightText: 2026 SAP SE or an SAP affiliate company and IronCore contributors | ||
| // SPDX-License-Identifier: Apache-2.0 |
Contributor
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
Contributor
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
Comment on lines
+1
to
+2
| // SPDX-FileCopyrightText: 2026 SAP SE or an SAP affiliate company and IronCore contributors | ||
| // SPDX-License-Identifier: Apache-2.0 |
Contributor
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.
Suggested change
| // 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
Contributor
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
hardikdr
force-pushed
the
enh/dual-mode-httpboot-ipxe
branch
3 times, most recently
from
0b4afa4 to
43874a6
Compare
Contributor
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 (2)
internal/controller/serverbootconfiguration_http_controller.go (1)
76-149:⚠️ Potential issue | 🟠 MajorClear stale
HTTPBootReadyon every owned error path.Now that readiness is driven from
HTTPBootReadyConditionType, only the UKI-construction failure updates the condition. If either server lookup,SetControllerReference, the SSA patch, or the post-applyGetfails, the previous condition is left untouched.computeDesiredStateonly looks at condition status, so an olderTruecan keep the parent Ready after this reconcile has already failed.🤖 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 76 - 149, The reconcile path leaves an older HTTPBootReady condition unchanged on many error branches; ensure we clear/patch the HTTPBootReadyConditionType to False on every owned error path (not just UKI construction). For each failure in reconcile (errors from r.getSystemUUIDFromServer, r.getSystemNetworkIDsFromServer, controllerutil.SetControllerReference, json.Marshal of httpBootConfig, r.Patch (SSA apply), and r.Get of httpBootConfig) call PatchServerBootConfigCondition with a metav1.Condition having Type HTTPBootReadyConditionType, Status False, an appropriate Reason (e.g., "LookupFailed","NetworkIDsFailed","SetControllerRefFailed","MarshalFailed","ApplyFailed","GetFailed"), Message from err.Error(), and ObservedGeneration config.Generation before returning the original error so the parent readiness is cleared consistently; keep existing UKI-construction patch logic as-is and reuse PatchServerBootConfigCondition to implement this.internal/controller/serverbootconfiguration_pxe_controller.go (1)
94-168:⚠️ Potential issue | 🟠 MajorClear stale
IPXEBootReadyon every owned error path.Now that readiness is driven from
IPXEBootReadyConditionType, only the image-details failure updates the condition. If either server lookup,SetControllerReference, the SSA patch, or the post-applyGetfails, the previous condition is left untouched.computeDesiredStateonly looks at condition status, so an olderTruecan keep the parent Ready after this reconcile has already failed.🤖 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 94 - 168, The reconcile function leaves a stale IPXEBootReady condition on several error paths (server lookup via r.getSystemUUIDFromBootConfig / r.getSystemIPFromBootConfig, controllerutil.SetControllerReference, the SSA apply via r.Patch, and the post-apply r.Get), so update each of those error return paths to call PatchServerBootConfigCondition with IPXEBootReadyConditionType set to metav1.ConditionFalse (Reason like "ReconcileFailed" and Message set to the error message) before returning; ensure you call PatchServerBootConfigCondition with the same NamespacedName used elsewhere and propagate any patch errors by wrapping them (similar to the existing image-details error handling) so failures still return useful context.
🤖 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_http_controller.go`:
- Around line 155-189: When patching the HTTPBootReady condition in
patchHTTPBootReadyConditionFromHTTPState, guard against stamping a newer
ServerBootConfiguration generation from an older reconcile: after re-fetching
cur (the fetched metalv1alpha1.ServerBootConfiguration) compare cur.Generation
to cfg.Generation (the generation this reconcile processed); if they differ, set
cond.ObservedGeneration = cfg.Generation and force cond.Status =
metav1.ConditionUnknown with a Reason like "StaleReconcile" and a message such
as "ServerBootConfiguration generation changed during reconcile; deferring
update." (alternatively return nil to bail out), then continue to
apimeta.SetStatusCondition and Patch; otherwise keep the current logic but
ensure ObservedGeneration is tied to cfg.Generation so we never claim readiness
for a generation we didn't process.
In `@internal/controller/serverbootconfiguration_pxe_controller.go`:
- Around line 174-208: In patchIPXEBootReadyConditionFromIPXEState, avoid
stamping a newer ServerBootConfiguration generation by comparing cur.Generation
(fresh from the retry fetch) with the bootConfig.Generation (the generation this
reconcile processed): if they differ, either return without patching or set
cond.Status = metav1.ConditionUnknown and cond.Reason = "StaleReconcile" before
calling SetStatusCondition so you don't mark the newer generation based on an
older IPXEBootConfig; also ensure ObservedGeneration on the cond is set to the
generation actually processed (use bootConfig.Generation) rather than
cur.Generation.
---
Outside diff comments:
In `@internal/controller/serverbootconfiguration_http_controller.go`:
- Around line 76-149: The reconcile path leaves an older HTTPBootReady condition
unchanged on many error branches; ensure we clear/patch the
HTTPBootReadyConditionType to False on every owned error path (not just UKI
construction). For each failure in reconcile (errors from
r.getSystemUUIDFromServer, r.getSystemNetworkIDsFromServer,
controllerutil.SetControllerReference, json.Marshal of httpBootConfig, r.Patch
(SSA apply), and r.Get of httpBootConfig) call PatchServerBootConfigCondition
with a metav1.Condition having Type HTTPBootReadyConditionType, Status False, an
appropriate Reason (e.g.,
"LookupFailed","NetworkIDsFailed","SetControllerRefFailed","MarshalFailed","ApplyFailed","GetFailed"),
Message from err.Error(), and ObservedGeneration config.Generation before
returning the original error so the parent readiness is cleared consistently;
keep existing UKI-construction patch logic as-is and reuse
PatchServerBootConfigCondition to implement this.
In `@internal/controller/serverbootconfiguration_pxe_controller.go`:
- Around line 94-168: The reconcile function leaves a stale IPXEBootReady
condition on several error paths (server lookup via
r.getSystemUUIDFromBootConfig / r.getSystemIPFromBootConfig,
controllerutil.SetControllerReference, the SSA apply via r.Patch, and the
post-apply r.Get), so update each of those error return paths to call
PatchServerBootConfigCondition with IPXEBootReadyConditionType set to
metav1.ConditionFalse (Reason like "ReconcileFailed" and Message set to the
error message) before returning; ensure you call PatchServerBootConfigCondition
with the same NamespacedName used elsewhere and propagate any patch errors by
wrapping them (similar to the existing image-details error handling) so failures
still return useful 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
Run ID: bbc93ab7-80c0-4072-8429-e3fbbe669383
📒 Files selected for processing (8)
api/v1alpha1/httpbootconfig_types.goapi/v1alpha1/ipxebootconfig_types.goconfig/crd/bases/boot.ironcore.dev_httpbootconfigs.yamlconfig/crd/bases/boot.ironcore.dev_ipxebootconfigs.yamlinternal/controller/httpbootconfig_controller.gointernal/controller/ipxebootconfig_controller.gointernal/controller/serverbootconfiguration_http_controller.gointernal/controller/serverbootconfiguration_pxe_controller.go
✅ Files skipped from review due to trivial changes (1)
- api/v1alpha1/httpbootconfig_types.go
hardikdr
force-pushed
the
enh/dual-mode-httpboot-ipxe
branch
from
43874a6 to
cfd8ede
Compare
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
internal/controller/serverbootconfiguration_readiness_controller.go (2)
89-111:⚠️ Potential issue | 🟠 MajorStale conditions should be treated as pending.
The condition evaluation doesn't check
ObservedGeneration. After aServerBootConfigurationspec change, oldHTTPBootReady/IPXEBootReadyconditions (with staleObservedGeneration) can incorrectly keepStatus.StateatReadyorErroruntil the mode controllers rewrite them.🛡️ 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 89 - 111, The condition checks for HTTPBootReadyConditionType and IPXEBootReadyConditionType must treat stale conditions (where condition.ObservedGeneration != cfg.Generation) as pending; update the logic in the blocks using apimeta.FindStatusCondition(cfg.Status.Conditions, ...) to first check c != nil && c.ObservedGeneration == cfg.Generation before interpreting c.Status, and if the observed generation is stale set allReady = false (do not set hasError). Ensure you reference ObservedGeneration and cfg.Generation when deciding pending vs error for HTTPBootReadyConditionType and IPXEBootReadyConditionType so stale conditions don't keep State as Ready or Error.
1-2:⚠️ Potential issue | 🟡 MinorUse the repository-standard SPDX header year.
The SPDX header uses
2026but the repository standard requires2024.🪪 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🤖 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, Update the SPDX header year in internal/controller/serverbootconfiguration_readiness_controller.go: change the top comment line that currently reads "2026 SAP SE or an SAP affiliate company and IronCore contributors" to use the repository-standard year "2024"; ensure the exact header format remains "// SPDX-FileCopyrightText: 2024 SAP SE or an SAP affiliate company and IronCore contributors" to match other Go files.
🤖 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_readiness_controller.go`:
- Around line 89-111: The condition checks for HTTPBootReadyConditionType and
IPXEBootReadyConditionType must treat stale conditions (where
condition.ObservedGeneration != cfg.Generation) as pending; update the logic in
the blocks using apimeta.FindStatusCondition(cfg.Status.Conditions, ...) to
first check c != nil && c.ObservedGeneration == cfg.Generation before
interpreting c.Status, and if the observed generation is stale set allReady =
false (do not set hasError). Ensure you reference ObservedGeneration and
cfg.Generation when deciding pending vs error for HTTPBootReadyConditionType and
IPXEBootReadyConditionType so stale conditions don't keep State as Ready or
Error.
- Around line 1-2: Update the SPDX header year in
internal/controller/serverbootconfiguration_readiness_controller.go: change the
top comment line that currently reads "2026 SAP SE or an SAP affiliate company
and IronCore contributors" to use the repository-standard year "2024"; ensure
the exact header format remains "// SPDX-FileCopyrightText: 2024 SAP SE or an
SAP affiliate company and IronCore contributors" to match other Go files.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3184fe2d-17c5-4ca5-a2dd-3cac6ff3cd98
⛔ Files ignored due to path filters (2)
dist/chart/templates/crd/boot.ironcore.dev_httpbootconfigs.yamlis excluded by!**/dist/**dist/chart/templates/crd/boot.ironcore.dev_ipxebootconfigs.yamlis excluded by!**/dist/**
📒 Files selected for processing (10)
api/v1alpha1/httpbootconfig_types.goapi/v1alpha1/ipxebootconfig_types.goconfig/crd/bases/boot.ironcore.dev_httpbootconfigs.yamlconfig/crd/bases/boot.ironcore.dev_ipxebootconfigs.yamldocs/api-reference/api.mdinternal/controller/httpbootconfig_controller.gointernal/controller/ipxebootconfig_controller.gointernal/controller/serverbootconfiguration_http_controller.gointernal/controller/serverbootconfiguration_pxe_controller.gointernal/controller/serverbootconfiguration_readiness_controller.go
✅ Files skipped from review due to trivial changes (2)
- internal/controller/ipxebootconfig_controller.go
- internal/controller/httpbootconfig_controller.go
🚧 Files skipped from review as they are similar to previous changes (4)
- api/v1alpha1/httpbootconfig_types.go
- api/v1alpha1/ipxebootconfig_types.go
- config/crd/bases/boot.ironcore.dev_ipxebootconfigs.yaml
- internal/controller/serverbootconfiguration_http_controller.go
hardikdr
force-pushed
the
enh/dual-mode-httpboot-ipxe
branch
from
cfd8ede to
18c7d15
Compare
hardikdr
force-pushed
the
enh/dual-mode-httpboot-ipxe
branch
from
18c7d15 to
36fd51b
Compare
hardikdr
removed
ok-to-publish
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation