Fix empty collection drift across scale-set resources (#4695)

This commit is contained in:
Nikola Jokic
2026-09-29 16:38:32 +02:00
committed by GitHub
parent ce0c6b7437
commit 6a04f3c6d7
6 changed files with 348 additions and 12 deletions
@@ -17,13 +17,14 @@ limitations under the License.
package actionsgithubcom
import (
"bytes"
"context"
"fmt"
"maps"
"reflect"
"time"
"github.com/go-logr/logr"
apiequality "k8s.io/apimachinery/pkg/api/equality"
kerrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/types"
@@ -242,7 +243,7 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl.
labelsModified := !maps.Equal(listenerRole.Labels, desiredLabels)
desiredAnnotations := r.mergeAnnotations(listenerRole.Annotations, desiredRole.Annotations)
annotationsModified := !maps.Equal(listenerRole.Annotations, desiredAnnotations)
rulesModified := !reflect.DeepEqual(listenerRole.Rules, desiredRole.Rules)
rulesModified := !apiequality.Semantic.DeepEqual(listenerRole.Rules, desiredRole.Rules)
if labelsModified || annotationsModified || rulesModified {
updatedRole := listenerRole.DeepCopy()
if labelsModified {
@@ -433,7 +434,7 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl.
labelsModified := !maps.Equal(listenerConfigSecret.Labels, desiredLabels)
desiredAnnotations := r.mergeAnnotations(listenerConfigSecret.Annotations, desiredSecret.Annotations)
annotationsModified := !maps.Equal(listenerConfigSecret.Annotations, desiredAnnotations)
dataModified := !reflect.DeepEqual(listenerConfigSecret.Data, desiredSecret.Data)
dataModified := !maps.EqualFunc(listenerConfigSecret.Data, desiredSecret.Data, bytes.Equal)
if labelsModified || annotationsModified || dataModified {
updatedSecret := listenerConfigSecret.DeepCopy()
@@ -29,7 +29,6 @@ import (
"github.com/actions/actions-runner-controller/build"
"github.com/actions/scaleset"
"github.com/go-logr/logr"
"github.com/google/go-cmp/cmp"
corev1 "k8s.io/api/core/v1"
rbacv1 "k8s.io/api/rbac/v1"
apiequality "k8s.io/apimachinery/pkg/api/equality"
@@ -300,7 +299,7 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl
desiredLabels := r.filterAndMergeLabels(ephemeralRunnerSet.Labels, desired.Labels)
desiredAnnotations := r.mergeAnnotations(ephemeralRunnerSet.Annotations, desired.Annotations)
ephemeralRunnerMetadataModified := !cmp.Equal(ephemeralRunnerSet.Spec.EphemeralRunnerMetadata, desired.Spec.EphemeralRunnerMetadata)
ephemeralRunnerMetadataModified := !apiequality.Semantic.DeepEqual(ephemeralRunnerSet.Spec.EphemeralRunnerMetadata, desired.Spec.EphemeralRunnerMetadata)
ephemeralRunnerLabelsModified := !maps.Equal(ephemeralRunnerSet.Labels, desiredLabels)
ephemeralRunnerAnnotationsModified := !maps.Equal(ephemeralRunnerSet.Annotations, desiredAnnotations)
@@ -380,8 +379,8 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl
// instead re-creates the listener with the phase unset, which means
// running, so it comes back correct in one step.
if listenerSpecChanged(&listener, desired) ||
!cmp.Equal(listener.Labels, desired.Labels) ||
!cmp.Equal(listener.Annotations, desired.Annotations) {
!maps.Equal(listener.Labels, desired.Labels) ||
!maps.Equal(listener.Annotations, desired.Annotations) {
// The listener is about to be torn down and rebuilt, which is what
// the pending phase means. Report it here rather than relying on the
// generation check above: the desired listener is derived from the
@@ -0,0 +1,282 @@
package actionsgithubcom
import (
"context"
"fmt"
"time"
"github.com/actions/actions-runner-controller/apis/actions.github.com/v1alpha1"
"github.com/actions/actions-runner-controller/build"
scalefake "github.com/actions/actions-runner-controller/controllers/actions.github.com/multiclient/fake"
"github.com/actions/actions-runner-controller/controllers/actions.github.com/secretresolver"
"github.com/actions/scaleset"
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
corev1 "k8s.io/api/core/v1"
rbacv1 "k8s.io/api/rbac/v1"
kerrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/types"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/client"
logf "sigs.k8s.io/controller-runtime/pkg/log"
)
type metadataPatchRecorder struct {
client.Client
patches []string
}
func (c *metadataPatchRecorder) Patch(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error {
c.patches = append(c.patches, fmt.Sprintf("%T/%s", obj, obj.GetName()))
return c.Client.Patch(ctx, obj, patch, opts...)
}
var _ = Describe("Resource metadata empty collection convergence", func() {
fields := []string{
"autoscalingListener",
"listenerServiceAccountMetadata",
"listenerRoleMetadata",
"listenerRoleBindingMetadata",
"listenerConfigSecretMetadata",
"ephemeralRunnerSetMetadata",
"ephemeralRunnerMetadata",
"ephemeralRunnerConfigSecretMetadata",
}
for _, field := range append(fields, "all") {
for _, keys := range [][]string{{"labels"}, {"annotations"}, {"labels", "annotations"}} {
It(fmt.Sprintf("%s with empty %v", field, keys), func() {
ctx, cancel := context.WithTimeout(context.Background(), autoscalingRunnerSetTestTimeout)
defer cancel()
ns, mgr := createNamespace(GinkgoT(), k8sClient)
secret := createDefaultSecret(GinkgoT(), k8sClient, ns.Name)
const name = "empty-metadata"
scaleSet := &scaleset.RunnerScaleSet{ID: 1, Name: name, RunnerGroupID: 1, RunnerGroupName: "Default"}
cache := newTestResourceCache()
builder := ResourceBuilder{
Scheme: mgr.GetScheme(), ResourceCache: cache,
SecretResolver: secretresolver.New(k8sClient, scalefake.NewMultiClient(scalefake.WithClient(
scalefake.NewClient(
scalefake.WithCreateRunnerScaleSet(scaleSet, nil),
scalefake.WithGetRunnerScaleSetByID(scaleSet, nil),
scalefake.WithGenerateJitRunnerConfig(&scaleset.RunnerScaleSetJitRunnerConfig{
Runner: &scaleset.RunnerReference{ID: 1, Name: "test-runner", RunnerScaleSetID: 1},
EncodedJITConfig: "fake-jit-config",
}, nil),
),
))),
}
cachedClient := &metadataPatchRecorder{Client: mgr.GetClient()}
directClient := &metadataPatchRecorder{Client: k8sClient}
arsController := &AutoscalingRunnerSetReconciler{
Client: cachedClient, Scheme: mgr.GetScheme(), Log: logf.Log,
ControllerNamespace: ns.Name, DefaultRunnerScaleSetListenerImage: "listener:latest",
ResourceBuilder: builder,
}
listenerController := &AutoscalingListenerReconciler{
Client: directClient, Scheme: mgr.GetScheme(), Log: logf.Log,
ListenerMetricsAddr: "0", ResourceBuilder: builder,
}
ersController := &EphemeralRunnerSetReconciler{
Client: cachedClient, APIReader: k8sClient, Scheme: mgr.GetScheme(), Log: logf.Log,
ResourceBuilder: builder,
}
runnerController := &EphemeralRunnerReconciler{
Client: directClient, APIReader: k8sClient, Scheme: mgr.GetScheme(), Log: logf.Log,
ResourceBuilder: builder,
}
startManagers(GinkgoT(), mgr)
spec := map[string]any{
"githubConfigUrl": "https://github.com/owner/repo", "githubConfigSecret": secret.Name,
"template": map[string]any{"spec": map[string]any{
"containers": []any{map[string]any{"name": "runner", "image": "runner:latest"}},
}},
}
selected := []string{field}
if field == "all" {
selected = fields
}
for _, resource := range selected {
metadata := map[string]any{}
for _, key := range keys {
metadata[key] = map[string]any{}
}
spec[resource] = metadata
}
if field == "all" {
Expect(unstructured.SetNestedMap(spec, spec["autoscalingListener"].(map[string]any), "template", "metadata")).To(Succeed())
Expect(unstructured.SetNestedMap(spec, spec["autoscalingListener"].(map[string]any), "listenerTemplate", "metadata")).To(Succeed())
Expect(unstructured.SetNestedSlice(spec, []any{map[string]any{"name": "listener"}}, "listenerTemplate", "spec", "containers")).To(Succeed())
spec["proxy"] = map[string]any{
"http": map[string]any{"url": "http://proxy.example.com:8080", "credentialSecretRef": "proxy-auth"},
}
Expect(k8sClient.Create(ctx, &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{Name: "proxy-auth", Namespace: ns.Name},
Data: map[string][]byte{"username": []byte("user"), "password": []byte("password")},
})).To(Succeed())
}
raw := &unstructured.Unstructured{Object: map[string]any{
"apiVersion": v1alpha1.GroupVersion.String(), "kind": "AutoscalingRunnerSet",
"metadata": map[string]any{
"name": name, "namespace": ns.Name,
"labels": map[string]any{LabelKeyKubernetesVersion: build.Version},
},
"spec": spec,
}}
// Typed Create erases these maps before admission, hiding the regression.
Expect(k8sClient.Create(ctx, raw)).To(Succeed())
arsKey := client.ObjectKeyFromObject(raw)
ars := new(v1alpha1.AutoscalingRunnerSet)
Expect(k8sClient.Get(ctx, arsKey, ars)).To(Succeed())
listenerKey := client.ObjectKey{Namespace: ns.Name, Name: scaleSetListenerName(ars)}
waitForCache := func(key client.ObjectKey, obj client.Object) {
err := k8sClient.Get(ctx, key, obj)
if kerrors.IsNotFound(err) {
Eventually(func() bool {
return kerrors.IsNotFound(mgr.GetClient().Get(ctx, key, obj))
}, autoscalingRunnerSetTestTimeout, 10*time.Millisecond).Should(BeTrue())
return
}
Expect(err).NotTo(HaveOccurred())
version := obj.GetResourceVersion()
Eventually(func(g Gomega) {
g.Expect(mgr.GetClient().Get(ctx, key, obj)).To(Succeed())
g.Expect(obj.GetResourceVersion()).To(Equal(version))
}, autoscalingRunnerSetTestTimeout, 10*time.Millisecond).Should(Succeed())
}
reconcileARS := func() {
waitForCache(arsKey, new(v1alpha1.AutoscalingRunnerSet))
waitForCache(arsKey, new(v1alpha1.EphemeralRunnerSet))
waitForCache(listenerKey, new(v1alpha1.AutoscalingListener))
_, err := arsController.Reconcile(ctx, ctrl.Request{NamespacedName: arsKey})
Expect(err).NotTo(HaveOccurred())
}
reconcileListener := func() {
_, err := listenerController.Reconcile(ctx, ctrl.Request{NamespacedName: listenerKey})
Expect(err).NotTo(HaveOccurred())
}
for range 10 {
reconcileARS()
reconcileListener()
}
Expect(k8sClient.Get(ctx, arsKey, ars)).To(Succeed())
Expect(ars.Status.Phase).To(Equal(v1alpha1.AutoscalingRunnerSetPhaseRunning))
Expect(ars.Status.ObservedGeneration).To(Equal(ars.Generation))
ers := new(v1alpha1.EphemeralRunnerSet)
Expect(k8sClient.Get(ctx, arsKey, ers)).To(Succeed())
Expect(k8sClient.Patch(ctx, ers, client.RawPatch(types.MergePatchType, []byte(`{"spec":{"replicas":1}}`)))).To(Succeed())
reconcileERS := func() {
waitForCache(arsKey, new(v1alpha1.EphemeralRunnerSet))
_, err := ersController.Reconcile(ctx, ctrl.Request{NamespacedName: arsKey})
Expect(err).NotTo(HaveOccurred())
}
for range 2 {
reconcileERS()
}
runners := new(v1alpha1.EphemeralRunnerList)
Expect(k8sClient.List(ctx, runners, client.InNamespace(ns.Name))).To(Succeed())
Expect(runners.Items).To(HaveLen(1))
runnerKey := client.ObjectKeyFromObject(&runners.Items[0])
reconcileRunner := func() {
_, err := runnerController.Reconcile(ctx, ctrl.Request{NamespacedName: runnerKey})
Expect(err).NotTo(HaveOccurred())
waitForCache(runnerKey, new(v1alpha1.EphemeralRunner))
}
reconcileRunner()
objects := []client.Object{
ars, ers,
&v1alpha1.AutoscalingListener{ObjectMeta: metav1.ObjectMeta{Name: listenerKey.Name, Namespace: ns.Name}},
&corev1.ServiceAccount{ObjectMeta: metav1.ObjectMeta{Name: listenerKey.Name, Namespace: ns.Name}},
&rbacv1.Role{ObjectMeta: metav1.ObjectMeta{Name: listenerKey.Name, Namespace: ns.Name}},
&rbacv1.RoleBinding{ObjectMeta: metav1.ObjectMeta{Name: listenerKey.Name, Namespace: ns.Name}},
&corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: listenerKey.Name + "-config", Namespace: ns.Name}},
&corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: listenerKey.Name, Namespace: ns.Name}},
&runners.Items[0],
&corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: runnerKey.Name, Namespace: ns.Name}},
&corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: runnerKey.Name, Namespace: ns.Name}},
}
if field == "all" {
objects = append(objects,
&corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: listenerKey.Name + "-proxy", Namespace: ns.Name}},
&corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: proxyEphemeralRunnerSetSecretName(ers), Namespace: ns.Name}},
)
}
reconcileAll := func() {
reconcileARS()
reconcileListener()
reconcileERS()
reconcileRunner()
}
for range 3 {
reconcileAll()
}
for _, obj := range objects {
Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(obj), obj)).To(Succeed())
Expect(obj.GetDeletionTimestamp().IsZero()).To(BeTrue())
// Creation normalizes cached desired pointers. Rebuild them as a
// restart or dependency update would, rather than testing only hits.
cache.Delete(obj)
}
cachedClient.patches, directClient.patches = nil, nil
for range 5 {
reconcileAll()
for _, obj := range objects {
fresh := obj.DeepCopyObject().(client.Object)
Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(obj), fresh)).To(Succeed())
Expect(fresh.GetUID()).To(Equal(obj.GetUID()), "%T must not be replaced", obj)
Expect(fresh.GetResourceVersion()).To(Equal(obj.GetResourceVersion()), "%T must not be rewritten", obj)
Expect(fresh.GetDeletionTimestamp().IsZero()).To(BeTrue())
}
}
Expect(cachedClient.patches).To(BeEmpty(), "even no-op patches must not starve later reconciliation")
Expect(directClient.patches).To(BeEmpty())
Expect(k8sClient.Get(ctx, arsKey, raw)).To(Succeed())
for _, resource := range selected {
for _, key := range keys {
value, found, err := unstructured.NestedStringMap(raw.Object, "spec", resource, key)
Expect(err).NotTo(HaveOccurred())
Expect(found).To(BeTrue(), "controller patches must retain the explicit empty input")
Expect(value).To(BeEmpty())
}
}
By("propagating a genuine spec change while keeping all empty metadata inputs")
listener := new(v1alpha1.AutoscalingListener)
Expect(k8sClient.Get(ctx, listenerKey, listener)).To(Succeed())
oldUID := listener.UID
Expect(k8sClient.Patch(ctx, raw, client.RawPatch(types.MergePatchType, []byte(`{"spec":{"maxRunners":2}}`)))).To(Succeed())
for range 5 {
reconcileARS()
Expect(k8sClient.Get(ctx, listenerKey, listener)).To(Succeed())
if !listener.DeletionTimestamp.IsZero() {
break
}
}
Expect(listener.DeletionTimestamp.IsZero()).To(BeFalse(), "empty runner metadata must not block listener updates")
for range 10 {
reconcileListener()
err := k8sClient.Get(ctx, listenerKey, listener)
if kerrors.IsNotFound(err) {
break
}
Expect(err).NotTo(HaveOccurred())
}
Expect(kerrors.IsNotFound(k8sClient.Get(ctx, listenerKey, listener))).To(BeTrue())
for range 10 {
reconcileARS()
reconcileListener()
}
Expect(k8sClient.Get(ctx, listenerKey, listener)).To(Succeed())
Expect(listener.UID).NotTo(Equal(oldUID))
Expect(listener.Spec.MaxRunners).To(Equal(2))
Expect(listener.DeletionTimestamp.IsZero()).To(BeTrue())
Expect(k8sClient.Get(ctx, arsKey, ars)).To(Succeed())
Expect(ars.Status.Phase).To(Equal(v1alpha1.AutoscalingRunnerSetPhaseRunning))
})
}
}
})
@@ -147,6 +147,60 @@ func TestEphemeralRunnerSetActionableSpecChanged_RealChangeStillDetected(t *test
})
}
func TestEphemeralRunnerSetDesiredSpecChanged_Metadata(t *testing.T) {
for _, tc := range []struct {
name string
current *v1alpha1.ResourceMeta
desired *v1alpha1.ResourceMeta
changed bool
}{
{name: "both omitted"},
{
name: "empty labels", current: &v1alpha1.ResourceMeta{},
desired: &v1alpha1.ResourceMeta{Labels: map[string]string{}},
},
{
name: "empty annotations", current: &v1alpha1.ResourceMeta{},
desired: &v1alpha1.ResourceMeta{Annotations: map[string]string{}},
},
{
name: "empty maps", current: &v1alpha1.ResourceMeta{},
desired: &v1alpha1.ResourceMeta{Labels: map[string]string{}, Annotations: map[string]string{}},
},
{
name: "metadata object added", desired: &v1alpha1.ResourceMeta{}, changed: true,
},
{
name: "label added", current: &v1alpha1.ResourceMeta{},
desired: &v1alpha1.ResourceMeta{Labels: map[string]string{"team": "arc"}}, changed: true,
},
{
name: "label changed",
current: &v1alpha1.ResourceMeta{Labels: map[string]string{"team": "old"}},
desired: &v1alpha1.ResourceMeta{Labels: map[string]string{"team": "arc"}}, changed: true,
},
{
name: "annotation added", current: &v1alpha1.ResourceMeta{},
desired: &v1alpha1.ResourceMeta{Annotations: map[string]string{"team": "arc"}}, changed: true,
},
{
name: "annotation changed",
current: &v1alpha1.ResourceMeta{Annotations: map[string]string{"team": "old"}},
desired: &v1alpha1.ResourceMeta{Annotations: map[string]string{"team": "arc"}}, changed: true,
},
} {
t.Run(tc.name, func(t *testing.T) {
current := &v1alpha1.EphemeralRunnerSet{Spec: v1alpha1.EphemeralRunnerSetSpec{EphemeralRunnerMetadata: tc.current}}
desired := &v1alpha1.EphemeralRunnerSet{Spec: v1alpha1.EphemeralRunnerSetSpec{EphemeralRunnerMetadata: tc.desired}}
currentBefore, desiredBefore := current.DeepCopy(), desired.DeepCopy()
assert.Equal(t, tc.changed, ephemeralRunnerSetDesiredSpecChanged(current, desired))
assert.Equal(t, tc.changed, ephemeralRunnerSetDesiredSpecChanged(desired, current), "removals must also be detected")
assert.Equal(t, currentBefore, current)
assert.Equal(t, desiredBefore, desired)
})
}
}
type listenerEmptyCollectionTestCase struct {
name string
runnerSetSpec string
@@ -309,7 +309,7 @@ func TestListenerPodSpecRequiresRecreation_Containers(t *testing.T) {
// 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
// AutoscalingListener spec with Semantic.DeepEqual 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) {
@@ -1,10 +1,10 @@
package actionsgithubcom
import (
"maps"
"testing"
"github.com/actions/actions-runner-controller/apis/actions.github.com/v1alpha1"
"github.com/google/go-cmp/cmp"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
corev1 "k8s.io/api/core/v1"
@@ -43,7 +43,7 @@ func newLegacyAnnotationTestAutoscalingRunnerSet() *v1alpha1.AutoscalingRunnerSe
// upgrade consequence of no longer stamping the integrity hash.
//
// AutoscalingRunnerSetReconciler compares the live listener's annotations
// against the desired ones with cmp.Equal and deletes the listener when they
// against the desired ones with maps.Equal and deletes the listener when they
// differ. A listener created by an older controller carries the legacy
// annotation, the desired listener no longer does, so the first reconcile after
// an upgrade recreates it.
@@ -72,7 +72,7 @@ func TestLegacyIntegrityHashAnnotationCausesOneTimeListenerRecreation(t *testing
assert.False(
t,
cmp.Equal(live.Annotations, desired.Annotations),
maps.Equal(live.Annotations, desired.Annotations),
"a listener carrying the legacy annotation must not compare equal to the desired listener, otherwise it would never be replaced",
)
@@ -83,7 +83,7 @@ func TestLegacyIntegrityHashAnnotationCausesOneTimeListenerRecreation(t *testing
assert.NotContains(t, replacement.Annotations, legacyIntegrityHashAnnotation)
assert.True(
t,
cmp.Equal(replacement.Annotations, desired.Annotations),
maps.Equal(replacement.Annotations, desired.Annotations),
"the recreated listener must match the desired one, otherwise the controller would rebuild it forever",
)
}