refactor(agent): split install and join across separate Reconcile() Calls - #211
Merged
Merged
Conversation
…in (KAAP-2331) The k8s component install step can take long enough for CABPK to rotate the join token in the meantime. bootstrapK8sNode was still using the script fetched before install started, so a rotated token never got picked up. Re-fetch right after install succeeds, since that's the only point a slow operation sits between the original read and the join. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…AAP-2331) reconcileNormal now returns right after install succeeds instead of reading BootstrapSecret and joining in the same call. Install has no fixed time bound, so that read could pick up a token CABPK has since rotated. The next reconcile -- where install is already marked done -- reads it fresh with nothing slow in between, giving a single read that's always adjacent to its use instead of the two-reads-per-install tradeoff of the previous fix. Five existing tests assumed install and join complete within one Reconcile() call; updated to reflect the new two-step convergence. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- Explain why the SkipK8sInstallation and already-installed branches can safely fall through to the same-call read: neither runs anything slow first. - Reword the return-early comment without assuming familiarity with CABPK. - Fix a test comment that overclaimed BootstrapSecret is "only read once install has succeeded" -- SkipK8sInstallation reads it too, without install ever succeeding. - Add RunCmdCallCount/WriteToFileCallCount assertions to the skip-installation test, so it verifies numerically that install never runs rather than only checking for an absent event. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
indradhanush
approved these changes
Aug 20, 2026
| assert.Equal(t, corev1.ConditionTrue, installCond.Status) | ||
|
|
||
| // join lands on its own reconcile (see host_reconciler.go) -- one more, fast, call to reach it | ||
| _, err := r.Reconcile(t.Context(), controllerruntime.Request{NamespacedName: key}) |
Collaborator
There was a problem hiding this comment.
Maybe also assert the result?
srm6867
approved these changes
Aug 21, 2026
srm6867
left a comment
srm6867
left a comment
There was a problem hiding this comment.
lgtm. except the comment for the reconcile trigger.
| conditions.MarkTrue(byoHost, infrastructurev1beta1.K8sComponentsInstallationSucceeded) | ||
|
|
||
| // Stop here instead of reading BootstrapSecret and joining right away: install has no time bound, so the kubeadm join token it contains could be rotated while we wait. Read it on the next reconcile instead, immediately before joining, so nothing slow can happen in between. | ||
| return ctrl.Result{}, nil |
There was a problem hiding this comment.
So we return nil here. So what triggers next reconcile for bootstrap? Currently we have time based reconcile for heartbeat mechanism - which is configured as 30s default interval, so the bootstrap reconcile will happen after ~30s.
This is ok. but in case we increase the heartbeat interval later it will directly affect this.
Review feedback on #211: the Reconcile wrapper unconditionally set RequeueAfter to HeartbeatInterval whenever reconcileNormal returned no error, silently delaying the join attempt after a successful install by up to a full heartbeat interval -- and coupling that delay to a knob meant for liveness reporting, not join latency. The wrapper now only applies that default when reconcileNormal didn't already request something more specific; the post-install return asks for an immediate requeue instead. Also addresses a second review comment: assert the result of the join-completing Reconcile() call in the heartbeat concurrency test, not just its error. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
3 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.
Install has no fixed time bound, so that read could pick up a token CABPK has since rotated.
please review commit-by-commit
downstream: KAAP-2331