Skip to content

Hold bpfKubeProxyHealthzPort=0 until every calico-node accepts it - #5303

Closed
tomastigera wants to merge 1 commit into
masterfrom
tomas/core-13540-healthz-version-gate
Closed

tomastigera wants to merge 1 commit into
masterfrom
tomas/core-13540-healthz-version-gate

Conversation

@tomastigera

Copy link
Copy Markdown
Contributor

Description

Bug fix. Connecting an OSS v3.31.x BPF cluster to Calico Cloud leaves calico-node in CrashLoopBackOff and deadlocks the manifest->operator migration:

Unable to set global default configuration error=error with field
BPFKubeProxyHealthzPort = '0' (Reason: failed to validate Field:
BPFKubeProxyHealthzPort because of Tag: gte )

0 meaning "disabled" is only accepted from Calico v3.32.0 and Enterprise v3.23.0-2.0. Validation is client-side in libcalico-go and startup.go writes the global defaults back on every pod start, so a node predating the value crashloops on a field it does not even use. #4768 (CORE-12755) defaults the port to 0 on BPF clusters where the operator does not manage kube-proxy, gated only on BPF being enabled, kube-proxy being unmanaged and the field being nil — nothing about what version is running. The value therefore lands in the shared FelixConfiguration while old pods are still serving nodes.

Two paths are exposed:

  • manifest -> operator (the reported one): old pods live in kube-system, and the migration recreates them when it edits the nodeSelector, so they re-read FelixConfiguration and die. The migration then waits for them and times out.
  • operator -> operator: FelixConfiguration is patched early in Reconcile, before the DaemonSet is rendered. The rolling update itself mostly survives, since each terminated pod returns on the new image, but an old pod restarting out of turn does not — and neither does a rollback, where the 0 stays in the datastore and every node crashloops with no way forward.

The fix gates the write on allNodesRunTargetVersion:

  • ns-migration pending -> do not write; pods from the manifest install are still out there.
  • no calico-node DaemonSet -> write; a fresh install has nothing older running, so Default BPFKubeProxyHealthzPort to 0 when kube-proxy is not managed #4768's AKS behaviour is unchanged.
  • otherwise -> write only when Status.Variant and Status.CalicoVersion report the version this operator rolls out. Those are written only on a reconcile that got past status.IsAvailable(), so they mean "the whole stack was seen up on that version".

The existing calico-node DaemonSet lookup moves above the healthz block so it and the bpfEnabled derivation share one Get.

isRolloutCompleteWithBPFVolumes was considered and does not fit: it inspects the calico-system DaemonSet, which during migration is nodeSelector-limited to already-migrated nodes and so reports a complete rollout while unmigrated pods run. It is also count-based, hence trivially true on a DaemonSet the operator has not touched yet this reconcile — safe for the eBPF flip only because the bpffs volume marks "new spec applied", and there is no equivalent marker for a version bump.

The helper is written to be reusable: the hazard is generic to any FelixConfiguration value whose validator is newer than the oldest running node. Only this call site is wired up here; whether other operator-written defaults want the same gate is worth a separate look.

Testing. A 7-entry table on the helper plus three reconcile specs — upgrade in flight (not written), status reports the target (written), ns-migration pending (not written). fakeNamespaceMigration gained a needsMigration field. go test ./pkg/controller/installation/ is green at 303 specs; the 22 CEL validation specs need envtest assets and are skipped locally. Every branch of the new helper was mutation-tested: four are caught by a failing test, and a fifth turned out redundant and was removed. Removing the fresh-install branch breaks #4768's original AKS spec, which confirms that path is untouched.

Internal: CORE-13540.

Release Note

Fixed a bug where the operator could set FelixConfiguration bpfKubeProxyHealthzPort to 0 while older calico-node pods were still running. Those pods reject the value and crashloop, which could deadlock a migration to an operator-managed install.

For PR author

  • Tests for change.
  • If changing pkg/apis/, run make gen-files
  • If changing versions, run make gen-versions

…cepts it

Connecting an OSS v3.31.x BPF cluster to Calico Cloud left calico-node in
CrashLoopBackOff and deadlocked the manifest->operator migration:

  Unable to set global default configuration error=error with field
  BPFKubeProxyHealthzPort = '0' (Reason: failed to validate Field:
  BPFKubeProxyHealthzPort because of Tag: gte)

0 meaning "disabled" is only accepted from Calico v3.32.0 and Enterprise
v3.23.0-2.0. Validation is client-side in libcalico-go, so an older node
fails it while writing back the global defaults at startup and crashloops,
even though the field means nothing to it. The operator stamped the value
into the shared FelixConfiguration while v3.31.7 pods were still serving
nodes out of kube-system.

Gate the default on allNodesRunTargetVersion: skip the write while the
kube-system migration is pending and, once an operator-managed DaemonSet
exists, until Status.Variant and Status.CalicoVersion report the version
this operator rolls out. A fresh install still writes it immediately, so
the AKS case CORE-12755 fixed is unchanged.

CORE-13540
@caseydavenport

Copy link
Copy Markdown
Member

Heads up - operator development on master has moved into the Calico monorepo at projectcalico/calico, under operator/. This repo only carries the release-v1.41 through release-v1.44 branches now, so I'm closing this out here. Please re-open it against projectcalico/calico and we'll review it there.

The move shifted every file down a directory and changed the import prefix, so a plain cherry-pick won't apply. This gets most of the way:

# in your tigera/operator checkout
git format-patch master..HEAD --stdout > /tmp/pr.patch

# in a projectcalico/calico checkout
git apply --directory=operator /tmp/pr.patch
grep -rl 'github.com/tigera/operator' operator | xargs sed -i 's|github.com/tigera/operator|github.com/projectcalico/calico/operator|g'

Anything the PR touched outside the operator tree (.semaphore/, .github/) doesn't map into operator/, so that part needs redoing by hand. Sorry for the churn.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants