Detect listener pod drift by comparing the pod, not a hash (#4635)

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
Nikola Jokic
2026-09-10 22:56:43 +02:00
committed by GitHub
co-authored by Copilot App
parent c475023e28
commit 0cfedfbb2c
6 changed files with 478 additions and 47 deletions
@@ -467,7 +467,7 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl.
return ctrl.Result{}, err
}
shouldReCreate := desiredPod.Annotations[annotationKeyIntegrityHash] != listenerPod.Annotations[annotationKeyIntegrityHash]
shouldReCreate := listenerPodSpecRequiresRecreation(&listenerPod, desiredPod)
if shouldReCreate {
log.Info("Listener pod dependency changed, recreating listener pod")
if err := r.deleteListenerPod(ctx, &autoscalingListener, &listenerPod, log); err != nil {
@@ -50,6 +50,13 @@ const (
AnnotationKeyGitHubRunnerGroupName = "actions.github.com/runner-group-name"
AnnotationKeyGitHubRunnerScaleSetName = "actions.github.com/runner-scale-set-name"
AnnotationKeyPatchID = "actions.github.com/patch-id"
// AnnotationKeyListenerConfigResourceVersion records the resource version of
// the listener config secret the listener pod was created from. The pod
// mounts that secret and parses it once at startup, so a change to its
// contents only takes effect after a restart. Nothing about the change is
// visible in the pod spec, which references the secret by name, so the
// resource version is carried on the pod to make the drift observable.
AnnotationKeyListenerConfigResourceVersion = "actions.github.com/listener-config-resource-version"
)
// Labels applied to listener roles
+108
View File
@@ -0,0 +1,108 @@
package actionsgithubcom
import (
corev1 "k8s.io/api/core/v1"
apiequality "k8s.io/apimachinery/pkg/api/equality"
)
// listenerPodSpecRequiresRecreation reports whether the live listener pod must be
// deleted and rebuilt to match the desired spec.
//
// The config secret is checked first. The pod mounts it as a volume and the
// listener parses it once at startup, so a change to the scale set URL, the TLS
// certificate, the metrics configuration or the scaler tuning only reaches the
// listener after a restart. None of that is visible in the pod spec, which
// references the secret by name and is byte-identical before and after, so the
// desired pod carries the secret's resource version as an annotation and drift
// is detected by comparing it.
//
// A pod that predates this annotation counts as drift and is recreated once, on
// the first reconcile after the controller is upgraded. Ignoring it instead is
// tempting, to avoid that rollout, but it loses updates: the reconcile that
// declines to recreate the pod goes on to patch the desired annotations onto it,
// so a config change that landed while the old controller was running is
// recorded as already applied and the listener keeps serving the configuration
// it parsed at startup, with nothing to ever correct it. The rollout is cheap by
// comparison - deleting a listener does not disturb the runners it started, and
// an upgrade normally changes the listener image and so recreates the pod
// anyway.
//
// The resource version also moves when only the secret's labels or annotations
// change, which does not affect the listener. That is accepted rather than
// worked around by hashing the secret data: those fields come from the
// AutoscalingListener spec, and a change to that spec already makes the
// AutoscalingRunnerSet controller replace the listener wholesale.
//
// DeepDerivative, not DeepEqual: the live pod carries a large number 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 therefore report drift on every reconcile of every healthy listener and
// spin in a delete/create loop. It cannot be made to work by pre-populating the
// defaults either, since nodeName is scheduler-assigned and the access-token
// volume has a generated name. See TestListenerPodSpecRequiresRecreation.
//
// The cost of DeepDerivative is that it 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: the AutoscalingRunnerSet controller
// compares the whole AutoscalingListener spec with cmp.Equal and deletes the
// listener outright, which takes the pod with it. Container ports are the
// exception, because 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. Comparing port length is enough: additions and value
// changes are already caught by DeepDerivative, only removal is blind. The
// contents are deliberately not compared, because the API server defaults
// protocol to TCP and that would reintroduce the delete/create loop.
func listenerPodSpecRequiresRecreation(current, desired *corev1.Pod) bool {
if current == nil || desired == nil {
return current != desired
}
if listenerConfigChanged(current, desired) {
return true
}
if listenerContainerPortsRemoved(current, desired) {
return true
}
return !apiequality.Semantic.DeepDerivative(desired.Spec, current.Spec)
}
func listenerConfigChanged(current, desired *corev1.Pod) bool {
desiredVersion := desired.Annotations[AnnotationKeyListenerConfigResourceVersion]
if desiredVersion == "" {
// Nothing to compare against, so there is nothing this check can say.
// The desired pod is always built from a secret read back from the API
// server, so this only happens in tests.
return false
}
return current.Annotations[AnnotationKeyListenerConfigResourceVersion] != desiredVersion
}
func listenerContainerPortsRemoved(current, desired *corev1.Pod) bool {
for i := range desired.Spec.Containers {
desiredContainer := &desired.Spec.Containers[i]
currentContainer := findContainerByName(current.Spec.Containers, desiredContainer.Name)
if currentContainer == nil {
// A container the live pod does not have at all is drift that
// DeepDerivative already reports; nothing to decide here.
continue
}
if len(desiredContainer.Ports) < len(currentContainer.Ports) {
return true
}
}
return false
}
func findContainerByName(containers []corev1.Container, name string) *corev1.Container {
for i := range containers {
if containers[i].Name == name {
return &containers[i]
}
}
return nil
}
@@ -0,0 +1,19 @@
package actionsgithubcom
import (
"testing"
)
// BenchmarkListenerPodSpecRequiresRecreation measures the listener pod drift
// check, which also runs on every AutoscalingListener reconcile.
func BenchmarkListenerPodSpecRequiresRecreation(b *testing.B) {
desired := desiredListenerPod()
live := livePodFromDesired(desired)
b.ReportAllocs()
b.ResetTimer()
for range b.N {
if listenerPodSpecRequiresRecreation(live, desired) {
b.Fatal("expected no recreation")
}
}
}
@@ -0,0 +1,337 @@
package actionsgithubcom
import (
"testing"
"github.com/actions/actions-runner-controller/apis/actions.github.com/v1alpha1"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/utils/ptr"
)
// desiredListenerPod mirrors the shape produced by newScaleSetListenerPod: only
// the handful of fields the builder actually sets.
func desiredListenerPod() *corev1.Pod {
grace := int64(60)
return &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{Name: "listener", Namespace: "arc-systems"},
Spec: corev1.PodSpec{
ServiceAccountName: "listener",
NodeSelector: map[string]string{"kubernetes.io/os": "linux"},
Containers: []corev1.Container{{
Name: autoscalingListenerContainerName,
Image: "ghcr.io/actions/arc:0.1.0",
Command: []string{"/ghalistener"},
Env: []corev1.EnvVar{
{Name: "LISTENER_CONFIG_PATH", Value: "/etc/gha-listener/config.json"},
},
Ports: []corev1.ContainerPort{{ContainerPort: 8080}},
VolumeMounts: []corev1.VolumeMount{
{Name: "listener-config", MountPath: "/etc/gha-listener", ReadOnly: true},
},
}},
Volumes: []corev1.Volume{{
Name: "listener-config",
VolumeSource: corev1.VolumeSource{
Secret: &corev1.SecretVolumeSource{SecretName: "listener-config"},
},
}},
RestartPolicy: corev1.RestartPolicyNever,
TerminationGracePeriodSeconds: &grace,
},
}
}
// livePodFromDesired approximates what the API server and admission return after
// the desired pod is created: every field the controller set, plus the defaults
// and injections it did not.
func livePodFromDesired(desired *corev1.Pod) *corev1.Pod {
live := desired.DeepCopy()
defaultMode := int32(420)
enableServiceLinks := true
tolerationSeconds := int64(300)
live.Spec.NodeName = "node-1"
live.Spec.DNSPolicy = corev1.DNSClusterFirst
live.Spec.SchedulerName = "default-scheduler"
live.Spec.SecurityContext = &corev1.PodSecurityContext{}
live.Spec.DeprecatedServiceAccount = desired.Spec.ServiceAccountName
live.Spec.EnableServiceLinks = &enableServiceLinks
live.Spec.PreemptionPolicy = ptr.To(corev1.PreemptLowerPriority)
live.Spec.Priority = ptr.To(int32(0))
live.Spec.Tolerations = []corev1.Toleration{
{Key: "node.kubernetes.io/not-ready", Operator: corev1.TolerationOpExists,
Effect: corev1.TaintEffectNoExecute, TolerationSeconds: &tolerationSeconds},
{Key: "node.kubernetes.io/unreachable", Operator: corev1.TolerationOpExists,
Effect: corev1.TaintEffectNoExecute, TolerationSeconds: &tolerationSeconds},
}
live.Spec.Volumes[0].Secret.DefaultMode = &defaultMode
live.Spec.Volumes = append(live.Spec.Volumes, corev1.Volume{
Name: "kube-api-access-x7f2k",
VolumeSource: corev1.VolumeSource{
Projected: &corev1.ProjectedVolumeSource{DefaultMode: &defaultMode},
},
})
live.Spec.Containers[0].TerminationMessagePath = corev1.TerminationMessagePathDefault
live.Spec.Containers[0].TerminationMessagePolicy = corev1.TerminationMessageReadFile
live.Spec.Containers[0].ImagePullPolicy = corev1.PullIfNotPresent
live.Spec.Containers[0].Ports[0].Protocol = corev1.ProtocolTCP
live.Spec.Containers[0].VolumeMounts = append(live.Spec.Containers[0].VolumeMounts, corev1.VolumeMount{
Name: "kube-api-access-x7f2k", MountPath: "/var/run/secrets/kubernetes.io/serviceaccount", ReadOnly: true,
})
return live
}
// TestListenerPodSpecRequiresRecreation_SteadyState is the guard rail against
// swapping DeepDerivative for DeepEqual.
//
// A healthy listener pod carries many API-server defaults and admission
// injections that the desired pod never sets. If this comparison ever becomes
// strict, the controller deletes and recreates the pod on every reconcile, the
// listener never lives long enough to poll jobs, and every scale set in the
// cluster stops functioning. This test must keep passing.
func TestListenerPodSpecRequiresRecreation_SteadyState(t *testing.T) {
desired := desiredListenerPod()
live := livePodFromDesired(desired)
assert.False(t, listenerPodSpecRequiresRecreation(live, desired),
"a healthy pod carrying only API-server defaults must never be recreated; "+
"if this fails, the comparison became too strict and will spin in a delete/create loop")
}
func TestListenerPodSpecRequiresRecreation(t *testing.T) {
tests := map[string]struct {
mutateDesired func(*corev1.Pod)
want bool
why string
}{
"identical": {
mutateDesired: func(*corev1.Pod) {},
want: false,
why: "no drift",
},
"image changed": {
mutateDesired: func(p *corev1.Pod) { p.Spec.Containers[0].Image = "ghcr.io/actions/arc:0.2.0" },
want: true,
why: "an upgraded listener image must roll the pod",
},
"metrics port removed": {
mutateDesired: func(p *corev1.Pod) { p.Spec.Containers[0].Ports = nil },
want: true,
why: "disabling --listener-metrics-addr must remove the port from the running pod",
},
"metrics port value changed": {
mutateDesired: func(p *corev1.Pod) { p.Spec.Containers[0].Ports[0].ContainerPort = 9090 },
want: true,
why: "changing the metrics port must roll the pod",
},
"metrics port added": {
mutateDesired: func(p *corev1.Pod) {
p.Spec.Containers[0].Ports = append(p.Spec.Containers[0].Ports,
corev1.ContainerPort{ContainerPort: 9090})
},
want: true,
why: "an added port must roll the pod",
},
"env var added": {
mutateDesired: func(p *corev1.Pod) {
p.Spec.Containers[0].Env = append(p.Spec.Containers[0].Env,
corev1.EnvVar{Name: "HTTP_PROXY", Value: "http://proxy:8080"})
},
want: true,
why: "proxy configuration must roll the pod",
},
"env value changed": {
mutateDesired: func(p *corev1.Pod) { p.Spec.Containers[0].Env[0].Value = "/etc/other/config.json" },
want: true,
why: "a changed env value must roll the pod",
},
"config secret name changed": {
mutateDesired: func(p *corev1.Pod) { p.Spec.Volumes[0].Secret.SecretName = "other-config" },
want: true,
why: "pointing at a different config secret must roll the pod",
},
"service account changed": {
mutateDesired: func(p *corev1.Pod) { p.Spec.ServiceAccountName = "other-sa" },
want: true,
why: "a changed service account must roll the pod",
},
"nodeSelector key added": {
mutateDesired: func(p *corev1.Pod) { p.Spec.NodeSelector["pool"] = "listeners" },
want: true,
why: "an added nodeSelector key must roll the pod",
},
}
for name, tc := range tests {
t.Run(name, func(t *testing.T) {
desired := desiredListenerPod()
live := livePodFromDesired(desired)
tc.mutateDesired(desired)
assert.Equal(t, tc.want, listenerPodSpecRequiresRecreation(live, desired), tc.why)
})
}
t.Run("nil handling", func(t *testing.T) {
desired := desiredListenerPod()
assert.False(t, listenerPodSpecRequiresRecreation(nil, nil))
assert.True(t, listenerPodSpecRequiresRecreation(nil, desired))
assert.True(t, listenerPodSpecRequiresRecreation(desired, nil))
})
}
// TestListenerPodSpecRequiresRecreation_KnownDeepDerivativeLimits documents,
// rather than asserts away, the removals DeepDerivative cannot see. These are
// all sourced from the user-facing listener template, so they are handled
// upstream: the AutoscalingRunnerSet controller compares the whole
// AutoscalingListener spec with cmp.Equal and deletes the listener, which
// deletes the pod. If that upstream behaviour ever changes to a derivative
// comparison, these become real bugs.
func TestListenerPodSpecRequiresRecreation_KnownDeepDerivativeLimits(t *testing.T) {
tests := map[string]func(*corev1.Pod){
"all tolerations removed": func(p *corev1.Pod) { p.Spec.Tolerations = nil },
"trailing volume removed": func(p *corev1.Pod) {
p.Spec.Volumes = p.Spec.Volumes[:len(p.Spec.Volumes)-1]
},
"nodeSelector emptied": func(p *corev1.Pod) { p.Spec.NodeSelector = nil },
"grace period removed": func(p *corev1.Pod) { p.Spec.TerminationGracePeriodSeconds = nil },
}
for name, mutate := range tests {
t.Run(name, func(t *testing.T) {
desired := desiredListenerPod()
live := livePodFromDesired(desired)
mutate(desired)
assert.False(t, listenerPodSpecRequiresRecreation(live, desired),
"documented DeepDerivative limitation: removal is invisible here and is "+
"instead handled by AutoscalingListener re-creation upstream")
})
}
}
// TestListenerPodSpecRequiresRecreation_MetricsToggleUsesRealBuilder exercises
// the drift check against genuine newScaleSetListenerPod output rather than a
// hand-written fixture, closing the gap between the fixture above and reality.
//
// metricsConfig is the one pod-spec input that does not come from any resource:
// it is derived from the --listener-metrics-addr controller flag. Because
// nothing in the AutoscalingListener spec changes when an operator disables
// metrics, the AutoscalingListener is not re-created and this comparison is the
// only thing that can remove the stale port from the running pod.
func TestListenerPodSpecRequiresRecreation_MetricsToggleUsesRealBuilder(t *testing.T) {
autoscalingRunnerSet := v1alpha1.AutoscalingRunnerSet{
ObjectMeta: metav1.ObjectMeta{
Name: "test-scale-set",
Namespace: "test-ns",
Labels: map[string]string{
LabelKeyKubernetesPartOf: labelValueKubernetesPartOf,
LabelKeyKubernetesVersion: "0.2.0",
},
Annotations: map[string]string{
runnerScaleSetIDAnnotationKey: "1",
AnnotationKeyGitHubRunnerGroupName: "test-group",
AnnotationKeyGitHubRunnerScaleSetName: "test-scale-set",
},
},
Spec: v1alpha1.AutoscalingRunnerSetSpec{GitHubConfigUrl: "https://github.com/org/repo"},
}
cache := NewResourceCache()
b := ResourceBuilder{ResourceCache: &cache}
ephemeralRunnerSet, err := b.newEphemeralRunnerSet(&autoscalingRunnerSet)
require.NoError(t, err)
listener, err := b.newAutoscalingListener(&autoscalingRunnerSet, ephemeralRunnerSet, autoscalingRunnerSet.Namespace, "test:latest", nil)
require.NoError(t, err)
sa, err := b.newScaleSetListenerServiceAccount(listener)
require.NoError(t, err)
role := b.newScaleSetListenerRole(listener)
roleBinding := b.newScaleSetListenerRoleBinding(listener, role, sa)
build := func(metrics *listenerMetricsServerConfig) *corev1.Pod {
// The cache keys on the listener object, not on metricsConfig, so it must
// be bypassed to build both variants.
b.ResourceCache.listenerPod.Delete(listener)
pod, err := b.newScaleSetListenerPod(listener, &corev1.Secret{}, sa, role, roleBinding, metrics)
require.NoError(t, err)
return pod
}
withMetrics := build(&listenerMetricsServerConfig{addr: ":8080", endpoint: "/metrics"})
withoutMetrics := build(nil)
require.NotEmpty(t, withMetrics.Spec.Containers[0].Ports,
"builder should expose a container port when metrics are enabled")
require.Empty(t, withoutMetrics.Spec.Containers[0].Ports,
"builder should expose no container port when metrics are disabled")
assert.True(t, listenerPodSpecRequiresRecreation(withMetrics, withoutMetrics),
"disabling --listener-metrics-addr must recreate a pod that still has the metrics port")
assert.True(t, listenerPodSpecRequiresRecreation(withoutMetrics, withMetrics),
"enabling --listener-metrics-addr must recreate a pod that has no metrics port")
assert.False(t, listenerPodSpecRequiresRecreation(withMetrics, withMetrics),
"an unchanged metrics configuration must not recreate the pod")
}
// The listener pod mounts its config as a secret volume and parses it once at
// startup, so a change to the secret contents is invisible in the pod spec but
// still requires a restart to take effect.
func TestListenerPodSpecRequiresRecreation_ConfigSecretChanged(t *testing.T) {
tests := map[string]struct {
liveVersion string
desiredVersion string
want bool
why string
}{
"unchanged": {
liveVersion: "100",
desiredVersion: "100",
want: false,
why: "the config the listener is running is still the desired one",
},
"changed": {
liveVersion: "100",
desiredVersion: "101",
want: true,
why: "the listener only reads its config at startup, so it must be restarted",
},
"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",
},
"missing on desired pod": {
liveVersion: "100",
desiredVersion: "",
want: false,
why: "there is nothing to compare against, so this check must not claim drift",
},
}
for name, tt := range tests {
t.Run(name, func(t *testing.T) {
desired := desiredListenerPod()
live := livePodFromDesired(desired)
setListenerConfigVersion(live, tt.liveVersion)
setListenerConfigVersion(desired, tt.desiredVersion)
assert.Equal(t, tt.want, listenerPodSpecRequiresRecreation(live, desired), tt.why)
})
}
}
func setListenerConfigVersion(pod *corev1.Pod, version string) {
if version == "" {
delete(pod.Annotations, AnnotationKeyListenerConfigResourceVersion)
return
}
if pod.Annotations == nil {
pod.Annotations = map[string]string{}
}
pod.Annotations[AnnotationKeyListenerConfigResourceVersion] = version
}
@@ -472,24 +472,16 @@ func (b *ResourceBuilder) newScaleSetListenerPod(
Kind: "Pod",
},
ObjectMeta: metav1.ObjectMeta{
Name: autoscalingListener.Name,
Namespace: autoscalingListener.Namespace,
Labels: labels,
Annotations: make(map[string]string),
Name: autoscalingListener.Name,
Namespace: autoscalingListener.Namespace,
Labels: labels,
Annotations: map[string]string{
AnnotationKeyListenerConfigResourceVersion: podConfig.ResourceVersion,
},
},
Spec: podSpec,
}
newRunnerScaleSetListenerPod.Annotations[annotationKeyIntegrityHash] = scaleSetListenerPodIntegrity(
newRunnerScaleSetListenerPod,
autoscalingListener,
podConfig,
serviceAccount,
role,
roleBinding,
metricsConfig,
)
if err := b.setControllerReference(autoscalingListener, newRunnerScaleSetListenerPod); err != nil {
return nil, fmt.Errorf("failed to set controller reference for listener pod: %w", err)
}
@@ -502,38 +494,6 @@ func (b *ResourceBuilder) newScaleSetListenerPod(
return newRunnerScaleSetListenerPod, nil
}
func scaleSetListenerPodIntegrity(
pod *corev1.Pod,
autoscalingListener *v1alpha1.AutoscalingListener,
podConfig *corev1.Secret,
serviceAccount *corev1.ServiceAccount,
role *rbacv1.Role,
roleBinding *rbacv1.RoleBinding,
metricsConfig *listenerMetricsServerConfig,
) string {
type data struct {
ListenerPodSpec *corev1.PodSpec `json:"listenerPodSpec,omitempty"`
AutoscalingListenerIntegrityHash string `json:"autoscalingListenerIntegrityHash"`
ConfigSecretIntegrityHash string `json:"configSecretIntegrityHash"`
ServiceAccountIntegrityHash string `json:"serviceAccountIntegrityHash"`
RoleIntegrityHash string `json:"roleIntegrityHash"`
RoleBindingIntegrityHash string `json:"roleBindingIntegrityHash"`
MetricsConfig *listenerMetricsServerConfig `json:"metricsConfig,omitempty"`
}
d := data{
ListenerPodSpec: &pod.Spec,
AutoscalingListenerIntegrityHash: autoscalingListener.Annotations[annotationKeyIntegrityHash],
ConfigSecretIntegrityHash: podConfig.Annotations[annotationKeyIntegrityHash],
ServiceAccountIntegrityHash: serviceAccount.Annotations[annotationKeyIntegrityHash],
RoleIntegrityHash: role.Annotations[annotationKeyIntegrityHash],
RoleBindingIntegrityHash: roleBinding.Annotations[annotationKeyIntegrityHash],
MetricsConfig: metricsConfig,
}
return hash.ComputeTemplateHash(&d)
}
func mergeListenerPodWithTemplate(pod *corev1.Pod, tmpl *corev1.PodTemplateSpec) {
if pod.Annotations == nil {
pod.Annotations = make(map[string]string)