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:
✨ 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 |
CI installs controller-gen@latest which is now v0.21.0. The new version correctly omits the namespace parameter from apply configuration constructors for cluster-scoped resources (Hypervisor, Eviction). Without this regeneration, CI's generate step produces a 1-arg Eviction() signature while the committed code has the stale 2-arg version, causing build failures on any branch that calls apiv1.Eviction(name, namespace).
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 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.
conditions.go (cherry-pick from rework-onboarding):
- ConditionsFromStatus: preserve ObservedGeneration; the taint controller's
skip-check gates on it and it was silently dropped.
- SetApplyConfigurationStatusCondition: guard against nil/empty Type to
prevent a panic when a caller omits the Type field.
- SetApplyConfigurationStatusCondition: propagate ObservedGeneration when
updating an existing condition entry.
hypervisor_controller.go:
- Do not call WithInternalIP("") when the node has no NodeInternalIP address
yet (e.g. during kubelet restart or bootstrap). WithInternalIP combined with
ForceOwnership would overwrite a previously stored valid IP with an empty
string.
- Remove unnecessary metav1.ConditionStatus() conversion (unconvert lint).
eviction_controller.go:
- Remove extra blank line flagged by gofmt.
Note: the 3 pre-existing test failures in hypervisor_maintenance_controller_test.go
and hypervisor_controller_test.go are unrelated to these changes.
With +listType=map / +listMapKey=type on HypervisorStatus.Conditions and EvictionStatus.Conditions, the API server merges the conditions list by the 'type' key on Apply. This means: - Conditions owned by other field managers are untouched (correct). - Conditions previously written by this field manager that are absent from the Apply payload are pruned (also correct — used to delete conditions). The previous ConditionsFromStatus helper round-tripped ALL conditions from status into the apply payload. This was wrong: it caused each controller to claim field ownership over conditions it does not own, interfering with other controllers on the next Apply. Replace ConditionsFromStatus with ConditionFromStatus (single condition), and update every controller to seed only the condition(s) it owns before calling SetApplyConfigurationStatusCondition. For controllers that need to delete a condition by omission (HypervisorMaintenanceController removing ConditionTypeEvicting on MaintenanceUnset), the owned conditions are seeded selectively so the target condition is simply absent from the payload. No behaviour change for the eviction_controller: it is the sole owner of all conditions on the Eviction CR, so seeding all three types is correct.
Cluster-scoped resources (Hypervisor, Eviction) now take only a name parameter in their apply configuration constructors. Remove the previously required but always-empty namespace argument.
HypervisorController: the AgentPodsEvicted=False branch was returning early with a requeue before running syncLabelsAndAnnotations, so node label changes were not propagated to the Hypervisor while terminating. Move the requeue after the spec patch so labels stay in sync. HypervisorMaintenanceController: when enabling a compute service, also clear the forced_down flag. Otherwise, an earlier HA event that set forced_down=true would leave the service reachable but treated as down.
…rWithManager SetupWithManager was inlining the same ctrl.NewControllerManagedBy call that registerWithManager already provides, leaving registerWithManager unreachable and triggering the unused lint error.
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. |
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
1 participant
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.
Migrates all controllers from
PatchHypervisorStatusWithRetryto Server-Side Apply (SSA)Status().Apply()for all status writes.Key design:
+listType=map/+listMapKey=typeon the CRD, the API server merges conditions bytypekey — other controllers' conditions are untouched.ConditionFromStatusconverts a single owned condition for this purpose.HypervisorMaintenanceControllerto removeConditionTypeEvictingonMaintenanceUnset).Controllers migrated:
EvictionReconcilerHypervisorController(also guards against clobbering InternalIP with empty string)HypervisorTaintControllerHypervisorMaintenanceController(status + eviction creation via SSA)OnboardingController(status + statusCfg threading through reconcile chain)HypervisorOffboardingReconcilerAggregatesControllerTraitsControllerready.Controller(removesstatus_patch.go)NodeCertificateController(certificate creation via SSA)Helpers in
internal/utils/conditions.go:ConditionFromStatus: converts a single owned condition to apply-config formSetApplyConfigurationStatusCondition: upserts a condition entry, preservingLastTransitionTimewhen status is unchanged