Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR systematically migrates status update patterns across eleven controller files from retry-based HTTP PATCH operations to Kubernetes server-side apply (SSA) using ApplyConfigurations. A new utility module provides helpers for working with ApplyConfiguration-style conditions, and the old patch-with-retry infrastructure is removed. ChangesUnified Status Apply Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Replace MergeFromWithOptimisticLock + retry with Status().Apply() for all eviction status updates. Introduces ConditionsFromStatus and SetApplyConfigurationStatusCondition helpers to utils — mirroring meta.SetStatusCondition semantics — as the foundation for all SSA condition management. The evictionStatusCfg helper seeds every apply with the full set of owned fields so SSA never prunes values between reconcile cycles.
Replace PatchHypervisorStatusWithRetry with Status().Apply() for the InternalIP scalar and ConditionTypeTerminating condition. Re-fetch the hypervisor after the status apply so the subsequent spec patch sees a fresh resourceVersion.
Replace PatchHypervisorStatusWithRetry with Status().Apply() for the ConditionTypeTainted condition.
Replace PatchHypervisorStatusWithRetry with Status().Apply() for the ConditionTypeHaEnabled condition.
Replace PatchHypervisorStatusWithRetry with Status().Apply() for ConditionTypeHypervisorDisabled, ConditionTypeEvicting, and the Evicted scalar. ConditionTypeEvicting is removed by omitting it from the apply config — SSA prunes map-list entries no longer claimed by the sole owner.
Replace PatchHypervisorStatusWithRetry with Status().Apply(). The Nova ID lookup is inlined into Reconcile and combined with the initial condition in a single apply, avoiding two applies from the same field manager in one reconcile cycle which would cause SSA to prune the IDs. HypervisorID and ServiceID are included in every subsequent apply to retain ownership.
Replace PatchHypervisorStatusWithRetry with Status().Apply() for the ConditionTypeOffboarded condition.
Replace PatchHypervisorStatusWithRetry with Status().Apply() for ConditionTypeAggregatesUpdated and the Aggregates slice. The error path includes the current Aggregates in the apply to prevent SSA from pruning previously-set values on a transient OpenStack error. Clearing aggregates to an empty slice uses a targeted merge patch to work around the omitempty limitation in the generated apply configuration type.
Replace PatchHypervisorStatusWithRetry with Status().Apply() for ConditionTypeTraitsUpdated and the Traits slice. Error paths pass the current hv.Status.Traits to retain ownership and avoid SSA releasing it on transient placement API failures.
…rStatusWithRetry Replace the last caller of PatchHypervisorStatusWithRetry with Status().Apply() for ConditionTypeReady. With no remaining callers, PatchHypervisorStatusWithRetry and StatusPatchBackoff are removed.
Replace Create + SetControllerReference with Apply for the Eviction CR. The owner reference and labels are set in the apply configuration metadata, and SSA handles the upsert. A Get is still performed after the apply to read the current eviction status conditions.
Replace CreateOrUpdate + SetOwnerReference with Apply for the cert-manager Certificate CR. The owner reference is set in the apply configuration metadata. The retry-on-conflict loop is no longer needed since SSA handles concurrent updates correctly. The controller now applies the full certificate spec on every reconcile so IP/DNS changes are picked up immediately.
…ctions With SSA the status config is built once in Reconcile and passed to reconcileComputeService and reconcileEviction which mutate it directly. This eliminates the intermediate condition return values and makes the single-apply-per-reconcile explicit. The early-return guard for already-enabled/disabled state is inlined into each branch.
With SSA the status config is built once at the top of Reconcile and threaded through to all sub-functions (abortOnboarding, initialOnboarding, smokeTest, completeOnboarding) which mutate it directly. An apply closure is also passed so each function calls apply() when it has determined the desired condition, rather than building and applying the config itself. This removes the applyOnboardingCondition helper and makes the flow fully declarative: compute desired state, then apply once.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
internal/utils/conditions.go (1)
108-116: 💤 Low value
strChangeddoesn't need a**stringparameter.The function only reads through
a(no*a = …write), so*stringwould suffice and the call sites can drop the&:♻️ Proposed simplification
-// strChanged reports whether the pointer value of b differs from the current -// value at a. -func strChanged(a **string, b *string) bool { - if *a == nil && b == nil { +// strChanged reports whether b differs from a. +func strChanged(a, b *string) bool { + if a == nil && b == nil { return false } - if *a == nil || b == nil { + if a == nil || b == nil { return true } - return **a != *b + return *a != *b }And at call sites (Lines 94, 98):
- if strChanged(&existing.Reason, newCondition.Reason) { + if strChanged(existing.Reason, newCondition.Reason) { existing.Reason = newCondition.Reason changed = true } - if strChanged(&existing.Message, newCondition.Message) { + if strChanged(existing.Message, newCondition.Message) { existing.Message = newCondition.Message changed = true }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/utils/conditions.go` around lines 108 - 116, The strChanged function unnecessarily takes a **string (pointer to pointer) even though it only reads the value; change its signature to accept *string instead and update its logic to check a == nil, b == nil, and compare *a vs *b accordingly; then update all call sites that currently pass an address-of (&someString) to pass the plain *string value (i.e., drop the &) so callers match the new signature (adjust any callers named in the diff to stop taking the address).internal/controller/onboarding_controller.go (2)
166-183: 💤 Low value
Status=TruewithReason=Abortedis semantically unusual — confirm intent.In the
deleteTestServersfailure branch the condition is set toStatus=ConditionTruewithReason=ConditionReasonAbortedand the error message. The accompanying comment ("No cleanup, so we are still 'onboarding'") makes the intent clear, but downstream consumers — both within this PR (e.g.aggregates_controller.determineDesiredStatekeying onConditionReasonInitial/Testing/Handover) and any external dashboards/queries — typically treatAbortedas a terminal reason. Mixing it withStatus=Truemay produce surprising behavior for any code path that filters on either dimension alone.If "we tried to abort but cleanup failed" is meant to be a distinct state, a dedicated reason (e.g.
ConditionReasonCleanupFailedor keeping the previousReasonwhile overlaying the message) would model this more cleanly. The same pattern is repeated incompleteOnboarding(lines 327-334).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/onboarding_controller.go` around lines 166 - 183, The branch that handles deleteTestServers failures sets the onboarding condition to Status=True with Reason=ConditionReasonAborted which is semantically confusing; update the failure handling in the deleteTestServers branch (and the analogous block in completeOnboarding) to use a distinct, non-terminal reason (e.g. ConditionReasonCleanupFailed) or preserve the prior non-terminal reason and append the error text, and ensure the condition status reflects the intended "still onboarding" state via statusCfg.Conditions and the k8sacmetav1.Condition() builder rather than using ConditionReasonAborted.
91-102: 💤 Low valueAlways-include for
HypervisorID/ServiceIDis correct — but be aware of the empty-string force-write.Setting
WithHypervisorID(hv.Status.HypervisorID).WithServiceID(hv.Status.ServiceID)upfront ensures this controller stays the SSA owner of these fields across reconciles. One subtle implication withForceOwnership: when both are empty in the in-memory copy, the apply still asserts them as"". That's only safe because the only path that callsapply()while they are empty isabortOnboarding, which is gated byif status == nil { return nil }(so we never abort a never-onboarded hypervisor) and by the priorlookupNovaPropertiesblock updating them on first onboarding. A short comment near the constructor noting this invariant would prevent a future edit from accidentally callingapply()before either Lookup runs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/onboarding_controller.go` around lines 91 - 102, Add a short comment above the statusCfg/applier construction (the block that calls apiv1.HypervisorStatus().WithHypervisorID(...).WithServiceID(...) and defines apply) documenting the invariant: the SSA owner intentionally force-writes HypervisorID and ServiceID (possibly as empty strings) and apply() must not be invoked before lookupNovaProperties fills them — the only safe call path when they are empty is abortOnboarding which is already guarded by if status == nil { return nil } and the prior lookupNovaProperties. Mention the specific symbols: statusCfg, WithHypervisorID, WithServiceID, apply, abortOnboarding, and lookupNovaProperties so future editors won’t call apply() earlier.internal/controller/hypervisor_maintenance_controller.go (1)
70-84: ⚖️ Poor tradeoffSSA pattern: copying all conditions +
ForceOwnershipmakes per-condition ownership tracking essentially meaningless.
utils.ConditionsFromStatuscopies every condition currently inhv.Status.Conditionsinto the apply config, and the finalStatus().Apply(...)usesk8sclient.ForceOwnership. Each reconcile of this controller therefore force-claims ownership of every condition on the resource — including ones written byaggregates,onboarding,traits, etc. The values still converge as long as each controller only mutates its own conditions viaSetApplyConfigurationStatusCondition, but managedFields will thrash between controllers and you lose the main benefit of SSA's per-field tracking.The idiomatic SSA approach here is to include in each apply only the conditions this controller actually owns (
HypervisorDisabled,Evicting) plus the scalar fields it owns (Evicted), and let+listType=map/+listMapKey=typeon the conditions slice handle merging across managers. That can be deferred, but it is worth confirming the intent before this lands across all controllers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/hypervisor_maintenance_controller.go` around lines 70 - 84, The current code copies all conditions via utils.ConditionsFromStatus and calls Status().Apply with k8sclient.ForceOwnership, which force-claims every condition and breaks per-condition SSA ownership; change the apply to only set the scalar Evicted via apiv1.Hypervisor(...).WithStatus(...).WithEvicted(...) (or apiv1.HypervisorStatus().WithEvicted) and construct the Status apply config to include only the condition entries this controller owns (e.g. HypervisorDisabled and Evicting) instead of utils.ConditionsFromStatus; remove ForceOwnership (or avoid forcing ownership of conditions) and use SetApplyConfigurationStatusCondition/explicit apiv1 condition builders when updating conditions in reconcileComputeService/reconcileEviction so only owned conditions are applied and SSA per-condition merging works.internal/controller/aggregates_controller.go (1)
153-169: 💤 Low valueRedundant
Getbefore raw merge patch; consider cleaner alternatives.A few small things in the empty-aggregates clear path:
ac.Get(ctx, ..., fresh)is unnecessary —Status().Patchonly needsmetav1.ObjectMeta{Name: hv.Name}to identify the resource for aRawPatch. The extra round trip can be dropped.- The two-step "SSA apply, then merge-patch to clear" is fragile: if the apply succeeds and the patch fails, the controller has reported
AggregatesUpdated=Truewhile aggregates are still populated. On the next reconcile this branch should self-heal, but it's worth confirming.- An alternative to the merge-patch workaround is to drop
omitemptyonHypervisorStatus.Aggregates(or set the apply-config field to a non-nil empty slice via a small wrapper) so the SSA pathway alone is sufficient. That avoids a second writer for this field entirely.♻️ Minimal cleanup of the Get
- fresh := &kvmv1.Hypervisor{} - if err := ac.Get(ctx, k8sclient.ObjectKey{Name: hv.Name}, fresh); err != nil { - return ctrl.Result{}, err - } - return ctrl.Result{}, ac.Status().Patch(ctx, fresh, k8sclient.RawPatch(types.MergePatchType, patch)) + stub := &kvmv1.Hypervisor{ObjectMeta: metav1.ObjectMeta{Name: hv.Name}} + return ctrl.Result{}, ac.Status().Patch(ctx, stub, k8sclient.RawPatch(types.MergePatchType, patch))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/aggregates_controller.go` around lines 153 - 169, The current clear-path does an unnecessary ac.Get and then a RawPatch; remove the round-trip by passing a minimal object with only metadata to Status().Patch (e.g., a Hypervisor object whose ObjectMeta has Name: hv.Name) instead of fetching fresh, and keep the existing merge patch payload; alternatively consider fixing the root cause by ensuring HypervisorStatus.Aggregates is serialized as a non-nil empty slice in the SSA apply (remove `omitempty` on HypervisorStatus.Aggregates or wrap the apply to set an explicit empty slice) so the second writer/patch is unnecessary — reference symbols: aggregatesChanged, newAggregates, ac.Status().Patch, Hypervisor, HypervisorStatus.Aggregates, SSA.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/controller/aggregates_controller.go`:
- Around line 126-151: The success apply path omits Aggregates when
aggregatesChanged==false causing ownership loss; always set statusCfg.Aggregates
to the effective list by building aggregate apply configs from newAggregates
when aggregatesChanged && len(newAggregates)>0, otherwise from
hv.Status.Aggregates, then assign that slice to statusCfg.Aggregates before
calling ac.Status().Apply; extract the duplicated construction logic into a
helper (e.g., buildAggregateApplyConfigs that returns
[]apiv1.AggregateApplyConfiguration given the input aggregate list and uses
apiv1.Aggregate().WithName(...).WithUUID(...).WithMetadata(...) as needed) and
replace both places that currently duplicate the loop with calls to that helper;
finally remove the redundant Get() in the merge-patch flow (the already-fetched
hv should be used instead of re-calling Get).
In `@internal/controller/eviction_controller.go`:
- Around line 254-265: The OutstandingInstances field on the apply config can be
omitted when it becomes an empty slice, so modify the eviction status update
logic around evictionStatusCfg(eviction) /
statusCfg.WithHypervisorServiceId(hypervisor.ID) /
statusCfg.WithOutstandingRamMb(...) to detect when OutstandingInstances should
transition to an empty slice (i.e., hypervisor.Servers is nil or len == 0 and
previously there was a non-empty value) and, in that case, send a targeted PATCH
using MergePatchType that explicitly clears that field on the server rather than
relying on the generated apply config (mirror the MergePatchType workaround used
in aggregates_controller.go for OutstandingInstances). Ensure other paths still
use the normal apply/update path when OutstandingInstances is non-empty.
In `@internal/controller/hypervisor_controller.go`:
- Around line 107-119: The current code always calls
apiv1.HypervisorStatus().WithInternalIP(newInternalIP) even when newInternalIP
is empty, which can overwrite a valid stored IP due to ForceOwnership; change it
to only apply WithInternalIP when newInternalIP != "" (i.e., after the loop that
extracts NodeInternalIP), otherwise do not mutate the InternalIP field on
statusCfg so existing IP is preserved (use the existing apiv1.HypervisorStatus()
as-is when newInternalIP == ""). Reference symbols: newInternalIP,
node.Status.Addresses loop, statusCfg :=
apiv1.HypervisorStatus().WithInternalIP(newInternalIP), and the ForceOwnership
apply behavior.
In `@internal/controller/offboarding_controller.go`:
- Around line 151-158: applyStatus currently copies hv.Status.Conditions and
calls Apply with k8sclient.ForceOwnership, which steals ownership of unrelated
condition entries; instead construct the status apply payload to include only
the condition(s) this reconciler owns (the provided cond/Offboarded condition)
and stop forcing ownership of the whole conditions list. Concretely, in
HypervisorOffboardingReconciler.applyStatus replace
utils.ConditionsFromStatus/SetApplyConfigurationStatusCondition usage and
building from hv.Status.Conditions with a statusCfg whose Conditions contains
only the incoming cond (the Offboarded condition this controller owns) and call
r.Status().Apply with k8sclient.FieldOwner(OffboardingControllerName) but remove
k8sclient.ForceOwnership so SSA preserves other controllers' condition entries.
Ensure references are to applyStatus, OffboardingControllerName,
apiv1.HypervisorStatus, and the cond parameter.
In `@internal/controller/traits_controller.go`:
- Around line 143-158: The applyTraitsStatus function conflates nil and empty
trait slices and therefore may not express intent to clear status.traits; remove
the conditional guard and call statusCfg.WithTraits(traits...) unconditionally
inside applyTraitsStatus (i.e., always invoke WithTraits in the
TraitsController.applyTraitsStatus before applying status) so the controller
explicitly sets the desired trait set (including empty) and lets SSA +
k8sclient.ForceOwnership clear stale entries; if some call sites should not
manage traits, instead split into two helpers (one that only sets the condition
and one that sets condition+traits) and call the appropriate helper.
In `@internal/utils/conditions.go`:
- Around line 29-42: ConditionsFromStatus currently omits
metav1.Condition.ObservedGeneration and thus can strip that field during apply;
update ConditionsFromStatus to set ObservedGeneration using a fresh pointer
(e.g., ptr.To(c.ObservedGeneration) or equivalent) alongside the other fields to
avoid aliasing, and modify SetApplyConfigurationStatusCondition to compare and
propagate ObservedGeneration when deciding to replace/merge conditions; also
update the doc comment to state all metav1.Condition fields (including
ObservedGeneration) are preserved verbatim.
---
Nitpick comments:
In `@internal/controller/aggregates_controller.go`:
- Around line 153-169: The current clear-path does an unnecessary ac.Get and
then a RawPatch; remove the round-trip by passing a minimal object with only
metadata to Status().Patch (e.g., a Hypervisor object whose ObjectMeta has Name:
hv.Name) instead of fetching fresh, and keep the existing merge patch payload;
alternatively consider fixing the root cause by ensuring
HypervisorStatus.Aggregates is serialized as a non-nil empty slice in the SSA
apply (remove `omitempty` on HypervisorStatus.Aggregates or wrap the apply to
set an explicit empty slice) so the second writer/patch is unnecessary —
reference symbols: aggregatesChanged, newAggregates, ac.Status().Patch,
Hypervisor, HypervisorStatus.Aggregates, SSA.
In `@internal/controller/hypervisor_maintenance_controller.go`:
- Around line 70-84: The current code copies all conditions via
utils.ConditionsFromStatus and calls Status().Apply with
k8sclient.ForceOwnership, which force-claims every condition and breaks
per-condition SSA ownership; change the apply to only set the scalar Evicted via
apiv1.Hypervisor(...).WithStatus(...).WithEvicted(...) (or
apiv1.HypervisorStatus().WithEvicted) and construct the Status apply config to
include only the condition entries this controller owns (e.g. HypervisorDisabled
and Evicting) instead of utils.ConditionsFromStatus; remove ForceOwnership (or
avoid forcing ownership of conditions) and use
SetApplyConfigurationStatusCondition/explicit apiv1 condition builders when
updating conditions in reconcileComputeService/reconcileEviction so only owned
conditions are applied and SSA per-condition merging works.
In `@internal/controller/onboarding_controller.go`:
- Around line 166-183: The branch that handles deleteTestServers failures sets
the onboarding condition to Status=True with Reason=ConditionReasonAborted which
is semantically confusing; update the failure handling in the deleteTestServers
branch (and the analogous block in completeOnboarding) to use a distinct,
non-terminal reason (e.g. ConditionReasonCleanupFailed) or preserve the prior
non-terminal reason and append the error text, and ensure the condition status
reflects the intended "still onboarding" state via statusCfg.Conditions and the
k8sacmetav1.Condition() builder rather than using ConditionReasonAborted.
- Around line 91-102: Add a short comment above the statusCfg/applier
construction (the block that calls
apiv1.HypervisorStatus().WithHypervisorID(...).WithServiceID(...) and defines
apply) documenting the invariant: the SSA owner intentionally force-writes
HypervisorID and ServiceID (possibly as empty strings) and apply() must not be
invoked before lookupNovaProperties fills them — the only safe call path when
they are empty is abortOnboarding which is already guarded by if status == nil {
return nil } and the prior lookupNovaProperties. Mention the specific symbols:
statusCfg, WithHypervisorID, WithServiceID, apply, abortOnboarding, and
lookupNovaProperties so future editors won’t call apply() earlier.
In `@internal/utils/conditions.go`:
- Around line 108-116: The strChanged function unnecessarily takes a **string
(pointer to pointer) even though it only reads the value; change its signature
to accept *string instead and update its logic to check a == nil, b == nil, and
compare *a vs *b accordingly; then update all call sites that currently pass an
address-of (&someString) to pass the plain *string value (i.e., drop the &) so
callers match the new signature (adjust any callers named in the diff to stop
taking the address).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b53adc2b-6a4b-4ae2-b5b2-c5b89ee46516
📒 Files selected for processing (13)
internal/controller/aggregates_controller.gointernal/controller/eviction_controller.gointernal/controller/hypervisor_controller.gointernal/controller/hypervisor_instance_ha_controller.gointernal/controller/hypervisor_maintenance_controller.gointernal/controller/hypervisor_taint_controller.gointernal/controller/node_certificate_controller.gointernal/controller/offboarding_controller.gointernal/controller/onboarding_controller.gointernal/controller/ready/controller.gointernal/controller/traits_controller.gointernal/utils/conditions.gointernal/utils/status_patch.go
💤 Files with no reviewable changes (1)
- internal/utils/status_patch.go
| statusCfg := apiv1.HypervisorStatus() | ||
| statusCfg.Conditions = utils.ConditionsFromStatus(hv.Status.Conditions) | ||
| utils.SetApplyConfigurationStatusCondition(&statusCfg.Conditions, | ||
| *k8sacmetav1.Condition(). | ||
| WithType(desiredCondition.Type). | ||
| WithStatus(desiredCondition.Status). | ||
| WithReason(desiredCondition.Reason). | ||
| WithMessage(desiredCondition.Message)) | ||
|
|
||
| if aggregatesChanged && len(newAggregates) > 0 { | ||
| aggCfgs := make([]apiv1.AggregateApplyConfiguration, len(newAggregates)) | ||
| for i, agg := range newAggregates { | ||
| a := apiv1.Aggregate().WithName(agg.Name).WithUUID(agg.UUID) | ||
| if len(agg.Metadata) > 0 { | ||
| a.WithMetadata(agg.Metadata) | ||
| } | ||
| aggCfgs[i] = *a | ||
| } | ||
| statusCfg.Aggregates = aggCfgs | ||
| } | ||
|
|
||
| if err := ac.Status().Apply(ctx, | ||
| apiv1.Hypervisor(hv.Name, "").WithStatus(statusCfg), | ||
| k8sclient.ForceOwnership, k8sclient.FieldOwner(AggregatesControllerName)); err != nil { | ||
| return ctrl.Result{}, err | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp && find . -name "aggregates_controller.go" -type f 2>/dev/nullRepository: cobaltcore-dev/openstack-hypervisor-operator
Length of output: 70
🏁 Script executed:
# Search for the aggregates_controller.go file
git ls-files | grep -i aggregates_controllerRepository: cobaltcore-dev/openstack-hypervisor-operator
Length of output: 183
🏁 Script executed:
# Let's get the file path and read the relevant sections
git ls-files | grep controller | head -20Repository: cobaltcore-dev/openstack-hypervisor-operator
Length of output: 1117
🏁 Script executed:
wc -l internal/controller/aggregates_controller.goRepository: cobaltcore-dev/openstack-hypervisor-operator
Length of output: 137
🏁 Script executed:
cat -n internal/controller/aggregates_controller.goRepository: cobaltcore-dev/openstack-hypervisor-operator
Length of output: 12968
Aggregates may be inconsistently omitted from apply when condition alone changes.
When aggregatesChanged == false but conditionUnchanged == false (e.g., the handover phase transitions from Reason=TestAggregates to Reason=WaitingForTraits while aggregate names [zone, testAggregate] remain the same), the success path at lines 126–151 builds and applies a statusCfg with only conditions. The aggregates field is not set because the condition at line 135 (if aggregatesChanged && len(newAggregates) > 0) evaluates to false.
This creates an inconsistency with the error path (lines 92–102), which explicitly preserves hv.Status.Aggregates in the apply even when aggregates haven't changed. Under SSA + ForceOwnership where this controller is the sole owner of aggregate entries, the success path's omission could cause issues. For consistency and robustness, always populate statusCfg.Aggregates from the effective aggregate list (new ones if changed, otherwise current hv.Status.Aggregates) so ownership is maintained throughout reconciliation.
Additionally, the aggregate-building logic at lines 93–100 and 136–143 is duplicated; extracting it as a helper would improve maintainability. The Get() call at line 165 is redundant in the merge-patch flow (line 168 uses the already-fetched hv object).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/controller/aggregates_controller.go` around lines 126 - 151, The
success apply path omits Aggregates when aggregatesChanged==false causing
ownership loss; always set statusCfg.Aggregates to the effective list by
building aggregate apply configs from newAggregates when aggregatesChanged &&
len(newAggregates)>0, otherwise from hv.Status.Aggregates, then assign that
slice to statusCfg.Aggregates before calling ac.Status().Apply; extract the
duplicated construction logic into a helper (e.g., buildAggregateApplyConfigs
that returns []apiv1.AggregateApplyConfiguration given the input aggregate list
and uses apiv1.Aggregate().WithName(...).WithUUID(...).WithMetadata(...) as
needed) and replace both places that currently duplicate the loop with calls to
that helper; finally remove the redundant Get() in the merge-patch flow (the
already-fetched hv should be used instead of re-calling Get).
| var instances []string | ||
| if hypervisor.Servers != nil { | ||
| uuids := make([]string, len(*hypervisor.Servers)) | ||
| instances = make([]string, len(*hypervisor.Servers)) | ||
| for i, server := range *hypervisor.Servers { | ||
| uuids[i] = server.UUID | ||
| instances[i] = server.UUID | ||
| } | ||
| eviction.Status.OutstandingInstances = uuids | ||
| } | ||
|
|
||
| // Update status | ||
| eviction.Status.HypervisorServiceId = hypervisor.ID | ||
| eviction.Status.OutstandingRamMb = int64(hypervisor.MemoryMBUsed) | ||
| meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{ | ||
| Type: kvmv1.ConditionTypePreflight, | ||
| Status: metav1.ConditionTrue, | ||
| Message: "Preflight checks passed, hypervisor is disabled and ready for eviction", | ||
| Reason: kvmv1.ConditionReasonSucceeded, | ||
| }) | ||
| return ctrl.Result{}, r.updateStatus(ctx, eviction) | ||
| statusCfg := evictionStatusCfg(eviction) | ||
| statusCfg.WithHypervisorServiceId(hypervisor.ID) | ||
| statusCfg.WithOutstandingRamMb(int64(hypervisor.MemoryMBUsed)) | ||
| statusCfg.OutstandingInstances = instances |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm omitempty on the generated apply-configuration field
fd -e go -p 'applyconfigurations/api/v1' --exec rg -nC2 'OutstandingInstances'Repository: cobaltcore-dev/openstack-hypervisor-operator
Length of output: 1076
🏁 Script executed:
#!/bin/bash
# Find and examine the aggregates_controller.go to see the merge-patch workaround
fd -type f -name 'aggregates_controller.go' -exec cat {} \;Repository: cobaltcore-dev/openstack-hypervisor-operator
Length of output: 259
🏁 Script executed:
#!/bin/bash
# Search for how OutstandingInstances is used after being set
rg -n 'OutstandingInstances' --type go -B3 -A3Repository: cobaltcore-dev/openstack-hypervisor-operator
Length of output: 9119
🏁 Script executed:
#!/bin/bash
# Check how evictionStatusCfg is used in the controller
rg -n 'statusCfg|evictionStatusCfg' internal/controller/eviction_controller.go -B2 -A5 | head -100Repository: cobaltcore-dev/openstack-hypervisor-operator
Length of output: 4842
🏁 Script executed:
#!/bin/bash
# Find aggregates_controller.go
fd -name 'aggregates_controller.go'Repository: cobaltcore-dev/openstack-hypervisor-operator
Length of output: 319
🏁 Script executed:
#!/bin/bash
# Search for merge-patch or omitempty references in the codebase
rg -n 'merge.patch|omitempty' --type-list | head -5
rg -n 'merge.patch|omitempty' -iRepository: cobaltcore-dev/openstack-hypervisor-operator
Length of output: 17767
🏁 Script executed:
#!/bin/bash
# Look at the aggregates_controller.go to see the merge-patch workaround
cat -n internal/controller/aggregates_controller.go | sed -n '150,170p'Repository: cobaltcore-dev/openstack-hypervisor-operator
Length of output: 902
🏁 Script executed:
#!/bin/bash
# Get more context around the merge patch workaround
cat -n internal/controller/aggregates_controller.go | sed -n '145,180p'Repository: cobaltcore-dev/openstack-hypervisor-operator
Length of output: 1752
The omitempty hazard on OutstandingInstances is real and reachable.
When the last instance is popped in the eviction loop (line 369), statusCfg.OutstandingInstances becomes an empty slice []string{}. With the omitempty tag on this field in the generated apply config, SSA will not include an empty slice in its JSON patch, preventing it from clearing any previously-applied value that exists on the server.
The same issue and workaround pattern are documented in aggregates_controller.go (lines 153-169). Apply a targeted merge patch with MergePatchType when OutstandingInstances transitions to empty to ensure the field is properly cleared.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/controller/eviction_controller.go` around lines 254 - 265, The
OutstandingInstances field on the apply config can be omitted when it becomes an
empty slice, so modify the eviction status update logic around
evictionStatusCfg(eviction) / statusCfg.WithHypervisorServiceId(hypervisor.ID) /
statusCfg.WithOutstandingRamMb(...) to detect when OutstandingInstances should
transition to an empty slice (i.e., hypervisor.Servers is nil or len == 0 and
previously there was a non-empty value) and, in that case, send a targeted PATCH
using MergePatchType that explicitly clears that field on the server rather than
relying on the generated apply config (mirror the MergePatchType workaround used
in aggregates_controller.go for OutstandingInstances). Ensure other paths still
use the normal apply/update path when OutstandingInstances is non-empty.
| var newInternalIP string | ||
| for _, address := range node.Status.Addresses { | ||
| if address.Type == corev1.NodeInternalIP && hypervisor.Status.InternalIP != address.Address { | ||
| hypervisor.Status.InternalIP = address.Address | ||
| if address.Type == corev1.NodeInternalIP { | ||
| newInternalIP = address.Address | ||
| break | ||
| } | ||
| } | ||
|
|
||
| // update terminating condition | ||
| nodeTerminationCondition := FindNodeStatusCondition(node.Status.Conditions, "Terminating") | ||
|
|
||
| // Capture values to apply - only mutate fields this controller owns | ||
| statusCfg := apiv1.HypervisorStatus().WithInternalIP(newInternalIP) |
There was a problem hiding this comment.
Don't clobber a valid InternalIP with an empty string.
If node.Status.Addresses does not contain a NodeInternalIP (transient state during node bootstrap, kubelet restart, etc.), newInternalIP stays "". WithInternalIP("") then becomes part of the apply payload, and combined with ForceOwnership on Line 133 it will overwrite a previously stored valid IP with the empty string and take ownership of the field.
🛡️ Proposed guard
- // Capture values to apply - only mutate fields this controller owns
- statusCfg := apiv1.HypervisorStatus().WithInternalIP(newInternalIP)
+ // Capture values to apply - only mutate fields this controller owns
+ statusCfg := apiv1.HypervisorStatus()
+ if newInternalIP != "" {
+ statusCfg.WithInternalIP(newInternalIP)
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/controller/hypervisor_controller.go` around lines 107 - 119, The
current code always calls apiv1.HypervisorStatus().WithInternalIP(newInternalIP)
even when newInternalIP is empty, which can overwrite a valid stored IP due to
ForceOwnership; change it to only apply WithInternalIP when newInternalIP != ""
(i.e., after the loop that extracts NodeInternalIP), otherwise do not mutate the
InternalIP field on statusCfg so existing IP is preserved (use the existing
apiv1.HypervisorStatus() as-is when newInternalIP == ""). Reference symbols:
newInternalIP, node.Status.Addresses loop, statusCfg :=
apiv1.HypervisorStatus().WithInternalIP(newInternalIP), and the ForceOwnership
apply behavior.
| func (r *HypervisorOffboardingReconciler) applyStatus(ctx context.Context, hv *kvmv1.Hypervisor, cond *k8sacmetav1.ConditionApplyConfiguration) error { | ||
| statusCfg := apiv1.HypervisorStatus() | ||
| statusCfg.Conditions = utils.ConditionsFromStatus(hv.Status.Conditions) | ||
| utils.SetApplyConfigurationStatusCondition(&statusCfg.Conditions, *cond) | ||
| return r.Status().Apply(ctx, | ||
| apiv1.Hypervisor(hv.Name, "").WithStatus(statusCfg), | ||
| k8sclient.ForceOwnership, k8sclient.FieldOwner(OffboardingControllerName)) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Kubernetes server-side apply listType=map conditions per-entry ownership ForceOwnership best practice
💡 Result:
Best practice: for Kubernetes server-side apply (SSA), ensure your “conditions” list is modeled as a listType=map with a stable per-entry key (typically +listMapKey=type / x-kubernetes-list-map-keys=["type"]). This makes SSA treat each condition entry as independently addressable so different managers can own different entries without conflicts. [1][2][3] About “ForceOwnership” / forcing ownership: - SSA’s mechanism is “force conflicts” (kubectl --force-conflicts or force=true on the apply request). When you force, the operation succeeds, updates the field value, and removes the field from all other managers’ managedFields entries. [1][4] - Therefore, “force ownership” is not a general best practice; it should be used cautiously, typically only as a last resort when you intentionally want to override another actor’s desired state (e.g., an operator/automation takeover scenario). [4] Concrete guidance for “server-side apply listType=map conditions per-entry ownership” 1) Use associative list semantics for conditions - In CRD/OpenAPI or Go types, declare the conditions list as listType=map and listMapKey as the field(s) that uniquely identify each condition entry (commonly “type”). [1][3] - Kubernetes documents that with x-kubernetes-list-type=map, “different managers can manage entries separately”, whereas atomic lists are owned as a whole. [1] 2) Ensure the key is truly unique and stable - listMapKey applies only when listType=map and the key fields must be scalars; the key uniquely identifies entries. [1] 3) Don’t rely on forcing ownership to avoid modeling conflicts - If you leave conditions as the default/atomic list behavior, then a single manager can end up owning the entire list (and concurrent updates from different managers can conflict or overwrite). Proper listType=map + listMapKey avoids this by enabling per-entry ownership. [1][3] 4) When forcing is needed, force the smallest scope possible - Forcing conflicts causes the applying manager to become sole owner for the forced fields by removing ownership from other managers. That’s why Kubernetes guidance frames it as a case where the applier/controller intends to override conflicting desired state, not as a routine technique. [4][1] Answer to your “ForceOwnership best practice” phrasing - Best practice is: avoid “ForceOwnership” except for deliberate override scenarios; instead, structure the API schema (conditions listType=map with listMapKey=type) so SSA can naturally assign per-entry ownership without needing to force. [1][4][3]
Citations:
- 1: https://kubernetes.io/docs/reference/using-api/server-side-apply/
- 2: https://k8s.ritovision.com/docs/reference/using-api/server-side-apply/
- 3: Add +listType=map and +listMapKey=type to all CRD Conditions fields stacklok/toolhive#4535
- 4: https://kubernetes.io/blog/2022/10/20/advanced-server-side-apply
🏁 Script executed:
# Find the CRD or API schema definition to confirm conditions listType
fd -t f -e yaml -e yml | head -20Repository: cobaltcore-dev/openstack-hypervisor-operator
Length of output: 1405
🏁 Script executed:
# Search for HypervisorStatus definition and conditions field schema
rg "HypervisorStatus|conditions.*listType" -A 5 -B 2 --max-count 20Repository: cobaltcore-dev/openstack-hypervisor-operator
Length of output: 33372
🏁 Script executed:
# Examine the utility functions mentioned
fd "utils" -t f | head -10Repository: cobaltcore-dev/openstack-hypervisor-operator
Length of output: 180
🏁 Script executed:
# Check ConditionsFromStatus and SetApplyConfigurationStatusCondition implementations
rg "ConditionsFromStatus|SetApplyConfigurationStatusCondition" -B 3 -A 10Repository: cobaltcore-dev/openstack-hypervisor-operator
Length of output: 43195
🏁 Script executed:
# Verify ForceOwnership is truly applied with all conditions copied in other controllers
rg "ForceOwnership" -B 10 -A 2 | head -100Repository: cobaltcore-dev/openstack-hypervisor-operator
Length of output: 7739
🏁 Script executed:
# Check the conditions field definition in the CRD YAML
cat charts/openstack-hypervisor-operator/crds/kvm.cloud.sap_hypervisors.yaml | grep -A 50 "conditions:" | head -60Repository: cobaltcore-dev/openstack-hypervisor-operator
Length of output: 3008
🏁 Script executed:
# Look for WithConditions method in the HypervisorStatusApplyConfiguration
rg "WithConditions" applyconfigurations/api/v1/hypervisorstatus.goRepository: cobaltcore-dev/openstack-hypervisor-operator
Length of output: 376
🏁 Script executed:
# Check the full HypervisorStatusApplyConfiguration struct definition
cat applyconfigurations/api/v1/hypervisorstatus.go | head -100Repository: cobaltcore-dev/openstack-hypervisor-operator
Length of output: 6026
🏁 Script executed:
# Check if the CRD has x-kubernetes-list-type or x-kubernetes-list-map-keys for conditions
cat charts/openstack-hypervisor-operator/crds/kvm.cloud.sap_hypervisors.yaml | grep -B 5 -A 80 "conditions:" | grep -E "(x-kubernetes|type:|mapKeys)"Repository: cobaltcore-dev/openstack-hypervisor-operator
Length of output: 632
🏁 Script executed:
# Get the full conditions definition from CRD
cat charts/openstack-hypervisor-operator/crds/kvm.cloud.sap_hypervisors.yaml | sed -n '/conditions:/,/^\s\s[a-z]/p' | head -120Repository: cobaltcore-dev/openstack-hypervisor-operator
Length of output: 5834
🏁 Script executed:
# Verify the proposed fix would work - check if WithConditions accepts the right parameter type
rg "func.*WithConditions" -A 15 applyconfigurations/api/v1/hypervisorstatus.goRepository: cobaltcore-dev/openstack-hypervisor-operator
Length of output: 817
Avoid taking ownership of conditions you don't manage.
The conditions field in the CRD is properly configured as x-kubernetes-list-type: map with x-kubernetes-list-map-keys=[type], which means Kubernetes SSA treats each condition entry as independently addressable. However, applyStatus copies the entire hv.Status.Conditions slice into the apply payload and then applies it with ForceOwnership. This forcibly transfers ownership of every existing condition (e.g. Onboarding, Ready, Tainted, HaEnabled, Evicting, …) to OffboardingControllerName on every reconcile. The next time another controller reconciles its own condition, it forces ownership back, and so on — producing field-management churn and unnecessary resourceVersion bumps even though the visible status data is unchanged.
The idiomatic SSA pattern is "fully specified intent over the fields I manage" – include only the Offboarded condition you actually own:
♻️ Proposed simplification
func (r *HypervisorOffboardingReconciler) applyStatus(ctx context.Context, hv *kvmv1.Hypervisor, cond *k8sacmetav1.ConditionApplyConfiguration) error {
- statusCfg := apiv1.HypervisorStatus()
- statusCfg.Conditions = utils.ConditionsFromStatus(hv.Status.Conditions)
- utils.SetApplyConfigurationStatusCondition(&statusCfg.Conditions, *cond)
+ statusCfg := apiv1.HypervisorStatus().WithConditions(cond)
return r.Status().Apply(ctx,
apiv1.Hypervisor(hv.Name, "").WithStatus(statusCfg),
k8sclient.ForceOwnership, k8sclient.FieldOwner(OffboardingControllerName))
}The same anti-pattern (copy all existing conditions + ForceOwnership) is repeated in hypervisor_controller.go, hypervisor_taint_controller.go, hypervisor_instance_ha_controller.go, ready/controller.go, traits_controller.go, hypervisor_maintenance_controller.go, aggregates_controller.go, and onboarding_controller.go. Each controller should apply only the condition type(s) it owns. This will also let you remove utils.ConditionsFromStatus and utils.SetApplyConfigurationStatusCondition entirely, since SSA's merge semantics already preserve unrelated entries.
Per the Kubernetes SSA documentation, conflicts forced by one controller will undo another collaborator's changes; controllers should not compete for fields they don't intend to manage.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/controller/offboarding_controller.go` around lines 151 - 158,
applyStatus currently copies hv.Status.Conditions and calls Apply with
k8sclient.ForceOwnership, which steals ownership of unrelated condition entries;
instead construct the status apply payload to include only the condition(s) this
reconciler owns (the provided cond/Offboarded condition) and stop forcing
ownership of the whole conditions list. Concretely, in
HypervisorOffboardingReconciler.applyStatus replace
utils.ConditionsFromStatus/SetApplyConfigurationStatusCondition usage and
building from hv.Status.Conditions with a statusCfg whose Conditions contains
only the incoming cond (the Offboarded condition this controller owns) and call
r.Status().Apply with k8sclient.FieldOwner(OffboardingControllerName) but remove
k8sclient.ForceOwnership so SSA preserves other controllers' condition entries.
Ensure references are to applyStatus, OffboardingControllerName,
apiv1.HypervisorStatus, and the cond parameter.
| func (tc *TraitsController) applyTraitsStatus(ctx context.Context, hv *kvmv1.Hypervisor, traits []string, cond metav1.Condition) error { | ||
| statusCfg := apiv1.HypervisorStatus() | ||
| statusCfg.Conditions = utils.ConditionsFromStatus(hv.Status.Conditions) | ||
| utils.SetApplyConfigurationStatusCondition(&statusCfg.Conditions, | ||
| *k8sacmetav1.Condition(). | ||
| WithType(cond.Type). | ||
| WithStatus(cond.Status). | ||
| WithReason(cond.Reason). | ||
| WithMessage(cond.Message)) | ||
| if traits != nil { | ||
| statusCfg.WithTraits(traits...) | ||
| } | ||
| return tc.Status().Apply(ctx, | ||
| apiv1.Hypervisor(hv.Name, "").WithStatus(statusCfg), | ||
| k8sclient.ForceOwnership, k8sclient.FieldOwner(TraitsControllerName)) | ||
| } |
There was a problem hiding this comment.
traits == nil and traits == []string{} are conflated, and neither clears the field.
if traits != nil skips the WithTraits call only when the slice is nil; a non-nil empty slice still calls WithTraits() with zero variadic args, which is also a no-op (the apply config's Traits stays nil). In both cases the controller does not express intent over status.traits in this apply. After a successful sync where targetTraits ends up empty (e.g., all custom traits removed and current.Traits was empty), hv.Status.Traits will retain the previous (now-stale) values until something else nudges the apply payload, since SSA "owner doesn't manage this field" preserves the existing value.
If the goal is "always reflect the current desired set, even if empty", explicitly set traits unconditionally (and rely on SSA + ForceOwnership to clear stale entries when this controller is the sole owner):
🛠️ Proposed change
- if traits != nil {
- statusCfg.WithTraits(traits...)
- }
+ statusCfg.WithTraits(traits...)If you instead want certain call sites (e.g., the failure paths on Lines 108-109 and 134-135 that pass hv.Status.Traits) to not manage traits, consider splitting them into two helpers — one that sets the condition only, one that sets the condition + traits — to make the intent explicit at the call site.
📝 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.
| func (tc *TraitsController) applyTraitsStatus(ctx context.Context, hv *kvmv1.Hypervisor, traits []string, cond metav1.Condition) error { | |
| statusCfg := apiv1.HypervisorStatus() | |
| statusCfg.Conditions = utils.ConditionsFromStatus(hv.Status.Conditions) | |
| utils.SetApplyConfigurationStatusCondition(&statusCfg.Conditions, | |
| *k8sacmetav1.Condition(). | |
| WithType(cond.Type). | |
| WithStatus(cond.Status). | |
| WithReason(cond.Reason). | |
| WithMessage(cond.Message)) | |
| if traits != nil { | |
| statusCfg.WithTraits(traits...) | |
| } | |
| return tc.Status().Apply(ctx, | |
| apiv1.Hypervisor(hv.Name, "").WithStatus(statusCfg), | |
| k8sclient.ForceOwnership, k8sclient.FieldOwner(TraitsControllerName)) | |
| } | |
| func (tc *TraitsController) applyTraitsStatus(ctx context.Context, hv *kvmv1.Hypervisor, traits []string, cond metav1.Condition) error { | |
| statusCfg := apiv1.HypervisorStatus() | |
| statusCfg.Conditions = utils.ConditionsFromStatus(hv.Status.Conditions) | |
| utils.SetApplyConfigurationStatusCondition(&statusCfg.Conditions, | |
| *k8sacmetav1.Condition(). | |
| WithType(cond.Type). | |
| WithStatus(cond.Status). | |
| WithReason(cond.Reason). | |
| WithMessage(cond.Message)) | |
| statusCfg.WithTraits(traits...) | |
| return tc.Status().Apply(ctx, | |
| apiv1.Hypervisor(hv.Name, "").WithStatus(statusCfg), | |
| k8sclient.ForceOwnership, k8sclient.FieldOwner(TraitsControllerName)) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/controller/traits_controller.go` around lines 143 - 158, The
applyTraitsStatus function conflates nil and empty trait slices and therefore
may not express intent to clear status.traits; remove the conditional guard and
call statusCfg.WithTraits(traits...) unconditionally inside applyTraitsStatus
(i.e., always invoke WithTraits in the TraitsController.applyTraitsStatus before
applying status) so the controller explicitly sets the desired trait set
(including empty) and lets SSA + k8sclient.ForceOwnership clear stale entries;
if some call sites should not manage traits, instead split into two helpers (one
that only sets the condition and one that sets condition+traits) and call the
appropriate helper.
| func ConditionsFromStatus(conditions []metav1.Condition) []k8sacmetav1.ConditionApplyConfiguration { | ||
| result := make([]k8sacmetav1.ConditionApplyConfiguration, len(conditions)) | ||
| for i := range conditions { | ||
| c := &conditions[i] | ||
| result[i] = k8sacmetav1.ConditionApplyConfiguration{ | ||
| Type: &c.Type, | ||
| Status: &c.Status, | ||
| Reason: &c.Reason, | ||
| Message: &c.Message, | ||
| LastTransitionTime: &c.LastTransitionTime, | ||
| } | ||
| } | ||
| return result | ||
| } |
There was a problem hiding this comment.
ObservedGeneration is silently dropped during the conversion.
metav1.Condition has six exported fields, but only five are copied: ObservedGeneration is omitted. This is inconsistent with the doc comment which claims "All fields ... are preserved verbatim", and it has real consequences when combined with the per-controller apply pattern in this PR:
- The taint controller writes
TaintedwithWithObservedGeneration(hypervisor.Generation)and gates its skip check on it (seehypervisor_taint_controller.goLines 67–73). - Any other controller that calls
ConditionsFromStatusand applies (even just to update its own condition) will not express interest inObservedGeneration, so ownership stays with the original setter — that part is fine — but if the taint controller itself ever round-trips through this helper for its own condition,ObservedGenerationbecomesnilin the apply payload and gets defaulted to 0 by the API server, breaking the skip check on the next reconcile.
🐛 Proposed fix
result[i] = k8sacmetav1.ConditionApplyConfiguration{
Type: &c.Type,
Status: &c.Status,
+ ObservedGeneration: ptr.To(c.ObservedGeneration),
Reason: &c.Reason,
Message: &c.Message,
LastTransitionTime: &c.LastTransitionTime,
}(Use a fresh pointer here — &c.ObservedGeneration would alias the source slice, just like the other fields already do; consider switching all of them to ptr.To for consistency and to avoid surprising aliasing if the input is later mutated.)
You should also extend SetApplyConfigurationStatusCondition (Lines 78–101) to compare and propagate ObservedGeneration, and update its doc comment to match — meta.SetStatusCondition does propagate it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/utils/conditions.go` around lines 29 - 42, ConditionsFromStatus
currently omits metav1.Condition.ObservedGeneration and thus can strip that
field during apply; update ConditionsFromStatus to set ObservedGeneration using
a fresh pointer (e.g., ptr.To(c.ObservedGeneration) or equivalent) alongside the
other fields to avoid aliasing, and modify SetApplyConfigurationStatusCondition
to compare and propagate ObservedGeneration when deciding to replace/merge
conditions; also update the doc comment to state all metav1.Condition fields
(including ObservedGeneration) are preserved verbatim.
Merging this branch changes the coverage (1 decrease, 2 increase)
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. |
1 participant
Summary by CodeRabbit