Rework Onboarding#307
Open
fwiesel wants to merge 20 commits into
Open
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 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 switches onboarding condition updates to Kubernetes Server-Side Apply, adds SSA condition utilities, and updates onboarding controller flows and tests to use the new status-writing path. ChangesOnboarding Controller SSA Refactor
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/utils/conditions_test.go`:
- Around line 30-31: The helper function ptr[T any](v T) *T in the tests is
unused and causing golangci-lint failures; remove the entire ptr function
declaration (symbol "ptr") from internal/utils/conditions_test.go so it is no
longer compiled, then run the test/lint to confirm the error is gone.
In `@internal/utils/conditions.go`:
- Around line 61-63: The loop that compares condition types dereferences
newCondition.Type without checking it, which can panic; update the loop in the
function using variables conditions, newCondition and existing so you first
check that newCondition.Type != nil (and optionally (*conditions)[i].Type !=
nil) before comparing *(*conditions)[i].Type == *newCondition.Type, returning or
skipping the comparison when newCondition.Type is nil to avoid the dereference.
- Around line 33-39: The condition conversion currently builds
k8sacmetav1.ConditionApplyConfiguration without copying ObservedGeneration, so
update the struct literal that creates k8sacmetav1.ConditionApplyConfiguration
(where result[i] = k8sacmetav1.ConditionApplyConfiguration{...}) to include
ObservedGeneration: &c.ObservedGeneration; also add/adjust the test that
constructs a metav1.Condition to assert the ObservedGeneration is preserved
after conversion so the omission is covered by tests.
🪄 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: 6f6306d2-89fd-46ac-b2d2-711ee3393c78
📒 Files selected for processing (4)
internal/controller/onboarding_controller.gointernal/controller/onboarding_controller_test.gointernal/utils/conditions.gointernal/utils/conditions_test.go
|
This PR is stale because it has been open for 45 days with no activity. |
fwiesel
force-pushed
the
rework-onboarding
branch
from
311d8e2 to
9980b46
Compare
fwiesel
force-pushed
the
rework-onboarding
branch
from
9980b46 to
ef9b6f1
Compare
fwiesel
force-pushed
the
rework-onboarding
branch
2 times, most recently
from
f757216 to
b6f16a2
Compare
fwiesel
force-pushed
the
rework-onboarding
branch
2 times, most recently
from
aeb6791 to
40cd7f2
Compare
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.
When the test instance creation in the onboarding smoke test fails, the controller used to return a bare error from Reconcile and never updated the Onboarding condition. Operators saw a stale message frozen for days while the loop kept silently retrying — combined with the SSA condition helper preserving LastTransitionTime when Status does not change, the resource looked stuck even though reconciliation was healthy. This change: - In smokeTest, when createOrGetTestServer fails, set the Onboarding condition to Testing with the current error message and requeue with defaultWaitTime instead of returning the error. This makes each retry observable in the resource itself and matches the style used by every other smokeTest error path. - Include the failed server's UUID in the ERROR-state, console-output-failure, timeout, and terminate-failure messages. A fresh attempt allocates a new instance UUID, so each retry yields a different message even when Nova keeps returning the same fault text preventing SetApplyConfigurationStatusCondition from treating successive failures as no-op updates. - Tests: add Ginkgo specs covering POST /servers returning 500 and a server in ERROR state, asserting the condition message, the requeue, the absence of a returned error, and (for ERROR) that the delete is issued and the UUID is in the message.
Covers ConditionFromStatus (including ObservedGeneration preservation) and SetApplyConfigurationStatusCondition (append, update, status-change, LastTransitionTime preservation, no-op detection, multi-condition isolation).
fwiesel
force-pushed
the
rework-onboarding
branch
from
40cd7f2 to
95fad4d
Compare
|
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
|
fwiesel
force-pushed
the
ssa
branch
2 times, most recently
from
b2fd489 to
ad93c0b
Compare
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.
Refines the onboarding controller on top of the SSA migration (
ssabranch, #330).Changes:
createOrGetTestServerfails, the error is surfaced in theOnboardingcondition instead of returning a bare error. This makes each retry visible to operators; previously the condition message was frozen even though reconciliation was healthy.SetApplyConfigurationStatusConditionfrom treating successive failures as no-ops.Depends on:
ssa(#330).