Apply calico-node before Typha during upgrades to prevent cluster-wide NotReady - #5110
Apply calico-node before Typha during upgrades to prevent cluster-wide NotReady#5110AdheipSingh wants to merge 1 commit into
Conversation
During upgrades, applying Typha and calico-node in the same reconcile rolls both simultaneously. Felix may be newer than Typha but not older, so once Typha completes its rollout first, still-old calico-node pods can no longer sync, go NotReady after their readiness probe window, and the DaemonSet controller stops honoring surge limits and replaces pods cluster-wide. Apply calico-node first, then gate Typha on the DaemonSet being fully rolled out. Read the DaemonSet directly from the API server since the informer cache may still hold the pre-update object immediately after the write.
|
|
|
@AdheipSingh thanks for the PR - was there a specific issue you noticed that prompted this? We try to make the upgrade not depend on ordering regardless, so curious if we introduced an unexpected dependency in v3.32. |
| return reconcile.Result{}, err | ||
| } | ||
|
|
||
| // Check whether the calico-node rollout is complete; Typha is only |
There was a problem hiding this comment.
Is this the right behavior for a fresh rollout?
On a brand new cluster, we don't want to wait for calico/node to roll out before deplying Typha, since typha is needed for calico/node to become ready.
I suspect this will cause a deadlock on a fresh cluster.
|
|
||
| // Check whether the calico-node rollout is complete; Typha is only | ||
| // applied once it is. Read the DaemonSet directly from the API server | ||
| // rather than the informer cache: immediately after the update above, |
There was a problem hiding this comment.
I don't think we should be doing a direct read against the API server here for perf reasons - we can use the cache if we get the existing node DS generation prior to applying the new version, I think.
| } else { | ||
| nodeRolledOut = nodeDS.Status.ObservedGeneration >= nodeDS.Generation && | ||
| nodeDS.Status.UpdatedNumberScheduled == nodeDS.Status.DesiredNumberScheduled && | ||
| nodeDS.Status.NumberReady == nodeDS.Status.DesiredNumberScheduled |
There was a problem hiding this comment.
NumberReady is a steady state health check rather than a rollout check. One permanently unready calico-node pod (cordoned node, wedged kubelet, unrelated crashloop) leaves this false forever, so Typha stops getting config updates entirely, not just during upgrades.
What we actually care about is that no old-version Felix is left, which is the UpdatedNumberScheduled term on its own. We should drop the NumberReady check.
| nodeRolledOut = nodeDS.Status.ObservedGeneration >= nodeDS.Generation && | ||
| nodeDS.Status.UpdatedNumberScheduled == nodeDS.Status.DesiredNumberScheduled && | ||
| nodeDS.Status.NumberReady == nodeDS.Status.DesiredNumberScheduled | ||
| } |
There was a problem hiding this comment.
Nothing reliably wakes us back up when the rollout finishes. The DaemonSet watch from secondaryResources() goes through createPredicateForObject, whose UpdateFunc filters on generation change, and rollout progress is status only. So the gate stays shut until the 5 minute periodic reconcile fires.
I suspect the testing here looked fast because other watches (FelixConfiguration, secrets) are noisy enough to mask it. We should return a RequeueAfter while gated, and log that we're deliberately holding Typha back.
| } | ||
| } | ||
|
|
||
| components = append(components, typhaComponent) |
There was a problem hiding this comment.
Moving the append down here also reorders Typha relative to the cleanup passthrough above. Probably harmless, but it doesn't look intentional - can we leave the append where it was?
|
|
||
| func (c *typhaComponent) Ready() bool { | ||
| return true | ||
| return c.cfg.NodeRolledOut |
There was a problem hiding this comment.
Worth keeping in mind that CreateOrUpdateOrDelete skips the whole component when Ready() is false, not just the Deployment. So this also stops reconciling the typha Service, ServiceAccount, RBAC, PDB, cert secret and NetworkPolicy.
That's my main concern with gating rather than just ordering the two applies: any state where calico-node is unready because something on the Typha side is missing or broken now becomes unrecoverable, since the operator won't touch Typha until node is healthy.
| return reconcile.Result{}, err | ||
| } | ||
|
|
||
| // Apply calico-node before the remaining components so that its rollout |
There was a problem hiding this comment.
This ordering is inverted for downgrades, where node rolls to the older Felix first while Typha is still new, which is exactly the direction the comment says doesn't work.
Wonder if we need more awareness on if this is an upgrade / downgrade / or simply a configuration change.
| nodeDS.Status.UpdatedNumberScheduled == nodeDS.Status.DesiredNumberScheduled && | ||
| nodeDS.Status.NumberReady == nodeDS.Status.DesiredNumberScheduled | ||
| } | ||
| typhaCfg.NodeRolledOut = nodeRolledOut |
There was a problem hiding this comment.
This mutates typhaCfg a few hundred lines after we handed it to render.Typha() - I think we need to adjust this so that we are setting this field before passing the config to the renderer.
|
/sem-approve |
4 participants
Description
Type of change: bug fix
Problem: During upgrades, the operator applies the Typha Deployment and the calico-node DaemonSet in the same reconcile, so both roll simultaneously. Typha (typically 3 replicas) finishes its rollout in about a minute, while calico-node on a large cluster takes much longer. Felix is compatible with an older Typha, but an older Felix cannot sync with a newer Typha — so once all Typha replicas are on the new version, every still-old calico-node pod loses sync, fails its readiness probe after ~90s (3 × 30s), and goes NotReady. The DaemonSet controller does not count already-unavailable pods against
maxUnavailable/maxSurge, so it then replaces pods across the whole cluster at once instead of honoring the configured rolling-update limits. On a ~200 node production cluster this presented as a cluster-wide wave of NotReady calico-node pods during a 3.31 → 3.32 upgrade.Fix:
ObservedGeneration >= Generation, all pods updated and ready) using the existing componentReady()mechanism.With this ordering, new calico-node pods connect to old Typha (the compatible direction) while rolling under the configured limits, and Typha updates last, when every Felix is already on the new version. First installs are unaffected (no DaemonSet exists, so the gate is open), and in steady state the check passes immediately because the DaemonSet generation is unchanged.
Testing: Validated on a 30-node cluster upgrading Calico v3.31.3 → v3.32.1:
nodeUpdateStrategy: {maxSurge: 4, maxUnavailable: 0}(the affected user's configuration): calico-node Ready stayed 30/30 for the entire upgrade across every DaemonSet status transition; peak 34 concurrent pods (30 + exactly 4 surge), no limit bypass; node phase ~4 minutes; Typha updated only after all 30 nodes completed.maxUnavailable: 1): Ready never dropped below 29/30 (the single pod being replaced), Typha gated for the full ~18 minute rollout and updated within seconds of the last node becoming ready.Affected components: installation controller (core_controller.go), Typha render component.
Release Note
For PR author
make gen-filesmake gen-versionsFor PR reviewers
A note for code reviewers - all pull requests must have the following:
kind/bugif this is a bugfix.kind/enhancementif this is a a new feature.enterpriseif this PR applies to Calico Enterprise only.