Skip to content

Apply calico-node before Typha during upgrades to prevent cluster-wide NotReady #5110

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
AdheipSingh wants to merge 1 commit into
base: master
Choose a base branch
from
+47 −3
Open

Apply calico-node before Typha during upgrades to prevent cluster-wide NotReady #5110

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 40 additions & 2 deletions pkg/controller/installation/core_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1503,7 +1503,7 @@ func (r *ReconcileInstallation) Reconcile(ctx context.Context, request reconcile
NonClusterHost: nonclusterhost,
FelixHealthPort: *felixConfiguration.Spec.HealthPort,
}
components = append(components, render.Typha(&typhaCfg))
typhaComponent := render.Typha(&typhaCfg)

// See the section 'Use of Finalizers for graceful termination' at the top of this file for terminating details.
canRemoveCNI := false
Expand Down Expand Up @@ -1613,6 +1613,8 @@ func (r *ReconcileInstallation) Reconcile(ctx context.Context, request reconcile
}
}

components = append(components, typhaComponent)

@caseydavenport caseydavenport Jul 29, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


// Build a configuration for rendering calico/node.
nodeCfg := render.NodeConfiguration{
GoldmaneRunning: goldmaneRunning,
Expand Down Expand Up @@ -1671,7 +1673,7 @@ func (r *ReconcileInstallation) Reconcile(ctx context.Context, request reconcile
warnOnce.Reset()
}

components = append(components, render.Node(&nodeCfg))
nodeComponent := render.Node(&nodeCfg)

csiCfg := render.CSIConfiguration{
Installation: &instance.Spec,
Expand Down Expand Up @@ -1772,6 +1774,42 @@ func (r *ReconcileInstallation) Reconcile(ctx context.Context, request reconcile
return reconcile.Result{}, err
}

// Apply calico-node before the remaining components so that its rollout

@caseydavenport caseydavenport Jul 29, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// status can be checked before Typha is applied. Per the Calico version
// skew policy, Felix may be at most one minor version ahead of Typha, but
// not behind it. Updating Typha while older calico-node pods are still
// running would leave those pods unable to sync, marking them NotReady
// and allowing the DaemonSet controller to exceed the configured surge
// limits when replacing them.
if err = imageset.ResolveImages(imageSet, nodeComponent); err != nil {
r.status.SetDegraded(operatorv1.ResourceValidationError, "Error resolving ImageSet for calico-node", err, reqLogger)
return reconcile.Result{}, err
}
if err := handler.CreateOrUpdateOrDelete(ctx, nodeComponent, nil); err != nil {
r.status.SetDegraded(operatorv1.ResourceUpdateError, "Error creating / updating calico-node", err, reqLogger)
return reconcile.Result{}, err
}

// Check whether the calico-node rollout is complete; Typha is only

@caseydavenport caseydavenport Jul 29, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// applied once it is. Read the DaemonSet directly from the API server
// rather than the informer cache: immediately after the update above,

@caseydavenport caseydavenport Jul 29, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// the cache may still hold the pre-update object whose status reports
// fully rolled out, which would open the gate before the rollout has
// started.
nodeRolledOut := true
nodeDS, err := r.clientset.AppsV1().DaemonSets(common.CalicoNamespace).Get(ctx, common.NodeDaemonSetName, metav1.GetOptions{})
if err != nil {
if !apierrors.IsNotFound(err) {
r.status.SetDegraded(operatorv1.ResourceReadError, "Unable to read calico-node DaemonSet", err, reqLogger)
return reconcile.Result{}, err
}
} else {
nodeRolledOut = nodeDS.Status.ObservedGeneration >= nodeDS.Generation &&
nodeDS.Status.UpdatedNumberScheduled == nodeDS.Status.DesiredNumberScheduled &&
nodeDS.Status.NumberReady == nodeDS.Status.DesiredNumberScheduled

@caseydavenport caseydavenport Jul 29, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

@caseydavenport caseydavenport Jul 29, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

typhaCfg.NodeRolledOut = nodeRolledOut

@caseydavenport caseydavenport Jul 29, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


if err = imageset.ResolveImages(imageSet, components...); err != nil {
r.status.SetDegraded(operatorv1.ResourceValidationError, "Error resolving ImageSet for components", err, reqLogger)
return reconcile.Result{}, err
Expand Down
8 changes: 7 additions & 1 deletion pkg/render/typha.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,12 @@ type TyphaConfiguration struct {
// The health port that Felix is bound to. We configure Typha to bind to the port
// that is one less.
FelixHealthPort int

// NodeRolledOut indicates whether the calico-node DaemonSet rollout is
// complete. Typha is not updated until it is, so that calico-node rolls
// out first during upgrades: per the Calico version skew policy, Felix
// may be newer than Typha, but not older.
NodeRolledOut bool
}

// Typha creates the typha daemonset and other resources for the daemonset to operate normally.
Expand Down Expand Up @@ -165,7 +171,7 @@ func (c *typhaComponent) typhaPodDisruptionBudget() *policyv1.PodDisruptionBudge
}

func (c *typhaComponent) Ready() bool {
return true
return c.cfg.NodeRolledOut

@caseydavenport caseydavenport Jul 29, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

// typhaServiceAccount creates the typha's service account.
Expand Down
Loading