Remove PatchHypervisorStatusWithRetry after all controllers migrated to SSA#346
Open
fwiesel wants to merge 12 commits into
Open
Remove PatchHypervisorStatusWithRetry after all controllers migrated to SSA#346fwiesel wants to merge 12 commits into
fwiesel wants to merge 12 commits into
Conversation
|
Warning Review limit reached
Next review available in: 59 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
✨ 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 |
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Introduce ConditionFromStatus and SetApplyConfigurationStatusCondition as the foundation for migrating all controllers to server-side apply of status subresources. ConditionFromStatus converts a single metav1.Condition (as stored in a status subresource) to a ConditionApplyConfiguration suitable for SSA. ObservedGeneration is preserved so gating logic (e.g. the taint controller's skip-check) keeps working. SetApplyConfigurationStatusCondition mirrors the semantics of meta.SetStatusCondition on the apply-configuration slice: append or update by Type, refresh LastTransitionTime only when Status changes, and guard against nil/empty Type to prevent panics. Together they enable per-condition seeding so each controller claims field ownership only over the conditions it actually manages, without disturbing conditions owned by other field managers on Apply.
Replace MergeFromWithOptimisticLock + retry with Status().Apply() for all eviction status updates. The evictionStatusCfg helper seeds every apply with the full set of owned fields so SSA never prunes values between reconcile cycles. As the sole owner of every condition on the Eviction CR, the reconciler seeds all three condition types on each apply. The apply configuration constructor for the cluster-scoped Eviction resource takes only a name (no namespace).
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. Seed only ConditionTypeTerminating (the sole condition owned by this controller) so SSA does not claim ownership over conditions written by other controllers. WithInternalIP is only called when the node reports a NodeInternalIP. Combined with ForceOwnership, applying an empty string would clobber a previously stored valid IP whenever the node briefly reports no IP (e.g. during kubelet restart or bootstrap). The AgentPodsEvicted=False branch previously returned 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.
Replace PatchHypervisorStatusWithRetry with Status().Apply() for the ConditionTypeTainted condition. Only ConditionTypeTainted is seeded into the apply so SSA does not claim ownership over conditions written by other controllers.
Replace PatchHypervisorStatusWithRetry with Status().Apply() for ConditionTypeHypervisorDisabled, ConditionTypeEvicting, and the Evicted scalar. ConditionTypeEvicting is removed by omitting it from the apply — SSA prunes map-list entries no longer claimed by the sole owner. The status config is built once in Reconcile and passed to reconcileComputeService and reconcileEviction which mutate it directly. This eliminates 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. Seed only the conditions owned by this controller so SSA does not claim ownership over conditions written by others. For the delete-by-omission case, owned conditions are seeded selectively so the target condition is simply absent from the payload. Also clear the forced_down flag when enabling a compute service so an earlier HA event that set forced_down=true does not leave the service reachable but treated as down. SetupWithManager now delegates to registerWithManager instead of inlining the same ctrl.NewControllerManagedBy call, which had left registerWithManager unreachable.
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. The apply configuration constructor for the cluster-scoped Eviction resource takes only a name (no namespace).
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. 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. The flow becomes fully declarative: compute desired state, then apply once. Seed only the condition owned by this controller (ConditionTypeOnboarding) so SSA does not claim ownership over conditions written by other controllers.
Replace PatchHypervisorStatusWithRetry with Status().Apply() for the ConditionTypeOffboarded condition. Seed only that condition so SSA does not claim ownership over conditions written by other controllers.
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. Seed only ConditionTypeAggregatesUpdated so SSA does not claim ownership over conditions written by other controllers.
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. Seed only ConditionTypeTraitsUpdated so SSA does not claim ownership over conditions written by other controllers.
Replace PatchHypervisorStatusWithRetry with Status().Apply() for ConditionTypeReady. Seed only that condition so SSA does not claim ownership over conditions written by other controllers.
With all controllers migrated to Status().Apply(), the last caller of PatchHypervisorStatusWithRetry is gone. Remove the helper and the StatusPatchBackoff it used.
Merging this branch will increase overall coverage
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. Changed unit test files
|
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.
With all controllers migrated to
Status().Apply(), the last caller ofPatchHypervisorStatusWithRetryis gone. Remove the helper and theStatusPatchBackoffit used.This PR must merge last, after all controller-migration PRs are merged:
ssa-conditions-helpersssa-evictionssa-hypervisor-controllerssa-hypervisor-taintssa-hypervisor-maintenancessa-onboardingssa-offboardingssa-aggregatesssa-traitsssa-readyssa-node-certificateThe GitHub PR view will show this branch as containing all those commits until they merge into main, at which point only the
internal/utils/status_patch.godeletion remains.