Detect listener pod drift by comparing the pod, not a hash

[nikola-jokic]

Layer 2 of a stack splitting #4575. Stacked on layer 1 (nikola-jokic-fix-ers-metadata-merge-comparison) — review that one first; the base of this PR must be that branch, not master.

The AutoscalingListener controller decided whether to delete and rebuild the listener pod by comparing an actions.github.com/integrity-hash annotation that newScaleSetListenerPod stamped onto the desired pod. That hash was recomputed on every reconcile out of the pod spec plus the hashes of the listener, the config secret, the service account, the role, the role binding and the metrics config, so the answer to "did this pod change?" depended on a pile of bookkeeping that had to be kept in sync by hand. This replaces it with a direct comparison of the live and desired pods, in listenerPodSpecRequiresRecreation.

Three things about that comparison are worth a reviewer's attention.

Why DeepDerivative and not DeepEqual. The live pod carries dozens of fields the desired pod never sets, written by the API server and by admission: nodeName, dnsPolicy, schedulerName, securityContext, enableServiceLinks, the default tolerations, the kube-api-access-* projected volume and its mount, terminationMessagePath, imagePullPolicy, secret defaultMode. DeepEqual would report drift on every reconcile of every healthy listener and spin in a delete/create loop, and pre-populating the defaults does not fix it either, since nodeName is scheduler-assigned and the access-token volume has a generated name. The trade-off is that DeepDerivative ignores empty values on the desired side, so a field being removed is invisible to it. For everything sourced from the user-facing template that is harmless, because the AutoscalingRunnerSet controller compares the whole AutoscalingListener spec and deletes the listener outright, taking the pod with it. Container ports are the exception: they come from the --listener-metrics-addr controller flag rather than from any resource, so disabling metrics would otherwise leave the port on the pod forever. Port removal is therefore checked separately, by length only — comparing contents would reintroduce the loop, since the API server defaults protocol to TCP.

Why the config secret needs its own signal. The pod mounts the config as a secret volume and references it by name, so the pod spec is byte-identical before and after a config change, while the listener parses config.json once at startup. Without something extra, a change to the scale set URL, the TLS certificate, the metrics configuration or the scaler qps/burst would silently never reach the running listener. The desired pod therefore carries the config secret's resource version in an annotation, and drift is detected by comparing it. The existing updates listener scaler configuration and recreates the listener pod test in autoscalinglistener_controller_test.go covers this and passes.

Upgrading this controller restarts every existing listener, once. Listener pods created before this change have no config-resource-version annotation, and a missing annotation counts as drift, so the first reconcile after the upgrade deletes and recreates each of them. This is deliberate. The obvious alternative — ignore an absent annotation on the live pod, to spare the rollout — silently loses configuration updates: the reconcile that declines to recreate the pod carries on to the annotation merge further down Reconcile and patches the desired annotations onto the live pod, recording the resource version of a config the running listener never read. Every later comparison then matches, and a scale set URL, TLS certificate or scaler setting changed around the time of an upgrade would never take effect, with nothing left to correct it. The rollout is cheap by comparison: deleting a listener does not disturb the runners it has already started, and an upgrade normally changes the listener image, which recreates the pod through the DeepDerivative check anyway. An absent annotation on the desired pod is still ignored, since there is genuinely nothing to compare against, but that is only reachable in tests — the desired pod is always built from a secret read back from the API server.

The annotationKeyIntegrityHash constant and all its remaining writers are deliberately left in place — a later layer in the stack removes them. This layer only removes the listener pod reader.

Validation: go build ./..., go vet ./..., gofmt -l apis/ controllers/ all clean, and the full ./controllers/actions.github.com/ envtest suite passes.

[Copilot]

Copilot review overview

🔵 Needs a closer look

A legacy pod can miss its first config Secret update, leaving the listener on stale configuration.

Review tier: Lite
Findings: None

What changed in this PR

Replaces listener pod integrity-hash checks with direct pod drift detection and config Secret revision tracking.

Changes:

• Adds DeepDerivative-based pod comparison and metrics-port handling.
• Tracks config Secret resource versions.
• Adds tests and a benchmark.

File	Reviewed change
controllers/​actions.github.com/​resourcebuilder.go	Stamps config Secret revisions on desired pods.
controllers/​actions.github.com/​helpers.go	Implements pod drift and config revision detection.
controllers/​actions.github.com/​helpers_listener_test.go	Tests drift, defaults, metrics, and config changes.
controllers/​actions.github.com/​helpers_bench_test.go	Benchmarks drift detection.
controllers/​actions.github.com/​constants.go	Defines the config revision annotation key.
controllers/​actions.github.com/​autoscalinglistener_controller.go	Uses direct drift detection during reconciliation.

Suppressed comments (1)

controllers/actions.github.com/helpers.go:69

• When the live pod predates this annotation, this branch also suppresses the first actual config change: if the Secret moves from V1 to V2 before the pod is reconciled, the desired pod carries V2 but the running pod still parsed V1 at startup. The subsequent annotation patch records V2 without restarting, and later comparisons see no drift, so this listener can remain on stale configuration; the bootstrap path needs a way to distinguish an unchanged upgrade from a pending config update (or explicitly accept a one-time rollout).

	if currentVersion == "" || desiredVersion == "" {
		return false

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[humh25]

536975948763 humbertohlgl El sept 9, 2026, 03:50 -0600, Nikola Jokic ***@***.***>, escribió:

[nikola-jokic]

Good catch on the bootstrap path, and it is worse than a missed rollout — fixed in 052d65d.

Ignoring an absent annotation on the live pod does not just defer the restart, it records the update as applied. A reconcile that decides not to recreate the pod falls through to the annotation merge further down Reconcile and patches the desired annotations onto the live pod, so the resource version of a config the running listener never read gets written onto it. Every later comparison then matches and nothing is left to correct it: a scale set URL, TLS certificate or scaler setting changed around the time of an upgrade would never take effect for that listener.

So a missing annotation now counts as drift and the pod is recreated once. That is the "explicitly accept a one-time rollout" option, and the cost is small: deleting a listener does not disturb the runners it already started, and a controller upgrade normally changes the listener image, which recreates the pod through the DeepDerivative check anyway. The desired side is still allowed to be empty, because there is genuinely nothing to compare against then, though in practice that only happens in tests since the desired pod is always built from a secret read back from the API server.

Test case for the legacy pod flipped to expect recreation, and one added for the empty-desired case. Build, vet, gofmt and the full envtest suite pass.

[Copilot]

Copilot review overview

🟡 Changes recommended

Align the missing-annotation behavior and test with the documented compatibility expectations, or explicitly document the intentional rollout.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review tier: Lite
Findings: 1

New issues introduced by this change (1)

Severity	Finding
	controllers/​actions.github.com/​helpers.go — This comparison treats a missing annotation on the live pod as drift whenever the desired pod has a…

Suppressed comments (1)

controllers/actions.github.com/helpers_listener_test.go:305

• This test locks in the opposite of the PR's stated upgrade behavior: a legacy pod with no annotation is expected to be recreated immediately. If legacy pods are meant to be preserved, this case should cover the ignored-live-annotation path instead; otherwise the PR description should explicitly document the one-time rollout.

		"missing on live pod": {
			liveVersion:    "",
			desiredVersion: "101",
			want:           true,
			why:            "a pod predating the annotation is recreated once, because otherwise the reconcile that spares it patches the annotation on and the config change is lost for good",

[Copilot]

Copilot review overview

🟢 Approval recommended

No unresolved blocking issues were identified, and the review assessments support approval.

Review tier: Lite
Findings: None

Issues resolved since last review (1)

Severity	Finding
	controllers/​actions.github.com/​helpers.go — This comparison treats a missing annotation on the live pod as drift whenever the desired pod has a… View resolved comment

[Copilot]

Copilot review overview

🔵 Needs a closer look

A moderate review comment remains for missing builder-level test coverage.

Review tier: Lite
Findings: None

Suppressed comments (1)

controllers/actions.github.com/resourcebuilder.go:480

• This annotation is the only signal that lets the new comparison detect a Secret-only config update, but the current tests do not verify that the builder writes it: the helper tests set the annotation manually, and the scaler integration test also changes AutoscalingListener.Spec, so it can pass through listener replacement even if this assignment regresses. Add a builder-level test with a non-empty podConfig.ResourceVersion and assert that value is present on the returned pod.

			Annotations: map[string]string{
				AnnotationKeyListenerConfigResourceVersion: podConfig.ResourceVersion,
			},