Re-register the runner scale set when it is gone from the Actions service (#4571)

controllers/actions.github.com/autoscalingrunnerset_controller.go

@@ -18,6 +18,7 @@ package actionsgithubcom
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"maps"
 	"strconv"
@@ -339,6 +340,16 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl
 	)
 	switch {
 	case kerrors.IsNotFound(err):
+		gone, err := r.runnerScaleSetGone(ctx, &autoscalingRunnerSet)
+		if err != nil {
+			log.Error(err, "Failed to confirm the runner scale set exists before creating the listener")
+			return ctrl.Result{}, err
+		}
+		if gone {
+			log.Info("Recorded runner scale set no longer exists on the Actions service. Registering it again before creating the listener")
+			return r.createRunnerScaleSet(ctx, &autoscalingRunnerSet, log)
+		}
+
 		log.Info("AutoscalingListener does not exist, creating autoscaling listener")
 		return r.createAutoScalingListenerForRunnerSet(ctx, &autoscalingRunnerSet, &ephemeralRunnerSet, log)
 	case err != nil:
@@ -729,8 +740,12 @@ func (r *AutoscalingRunnerSetReconciler) deleteRunnerScaleSet(ctx context.Contex
 		return err
 	}
 
-	err = actionsClient.DeleteRunnerScaleSet(ctx, runnerScaleSetID)
-	if err != nil {
+	switch err := actionsClient.DeleteRunnerScaleSet(ctx, runnerScaleSetID); {
+	case errors.Is(err, scaleset.NotFoundError):
+		// Already gone, so the desired state is met. Returning the error instead would leave the
+		// finalizer in place and the autoscaling runner set stuck in Terminating.
+		logger.Info("Runner scale set is already deleted from the Actions service", "runnerScaleSetId", runnerScaleSetID)
+	case err != nil:
 		logger.Error(err, "Failed to delete runner scale set", "runnerScaleSetId", runnerScaleSetID)
 		return err
 	}
@@ -794,6 +809,34 @@ func (r *AutoscalingRunnerSetReconciler) createAutoScalingListenerForRunnerSet(c
 	return ctrl.Result{}, nil
 }
 
+// runnerScaleSetGone reports whether the runner scale set recorded on the autoscaling runner set is
+// no longer present on the Actions service. A scale set can disappear out of band: deleted from the
+// organization settings, or dropped and re-registered with a fresh ID. The listener bakes the ID in
+// at startup and crash-loops when it is gone, so this is checked before the listener is created.
+//
+// Only a definitive not-found counts as gone. Any other error is returned so a transient Actions
+// service outage never causes a scale set to be registered a second time.
+func (r *AutoscalingRunnerSetReconciler) runnerScaleSetGone(ctx context.Context, autoscalingRunnerSet *v1alpha1.AutoscalingRunnerSet) (bool, error) {
+	runnerScaleSetID, err := strconv.Atoi(autoscalingRunnerSet.Annotations[runnerScaleSetIDAnnotationKey])
+	if err != nil {
+		return false, fmt.Errorf("failed to parse runner scale set ID: %w", err)
+	}
+
+	actionsClient, err := r.GetActionsService(ctx, autoscalingRunnerSet)
+	if err != nil {
+		return false, fmt.Errorf("failed to initialize Actions service client: %w", err)
+	}
+
+	switch _, err := actionsClient.GetRunnerScaleSetByID(ctx, runnerScaleSetID); {
+	case errors.Is(err, scaleset.NotFoundError):
+		return true, nil
+	case err != nil:
+		return false, fmt.Errorf("failed to get runner scale set %d: %w", runnerScaleSetID, err)
+	default:
+		return false, nil
+	}
+}
+
 // TODO: change that
 func shouldCreateScaleSet(autoscalingRunnerSet *v1alpha1.AutoscalingRunnerSet) bool {
 	scaleSetIDRaw, ok := autoscalingRunnerSet.Annotations[runnerScaleSetIDAnnotationKey]

controllers/actions.github.com/autoscalingrunnerset_controller_test.go

@@ -8,7 +8,9 @@ import (
 	"net/http/httptest"
 	"os"
 	"path/filepath"
+	"strconv"
 	"sync"
+	"sync/atomic"
 	"time"
 
 	corev1 "k8s.io/api/core/v1"
@@ -2081,3 +2083,255 @@ var _ = Describe("Test resource version and build version mismatch", func() {
 		).Should(BeTrue())
 	})
 })
+
+var _ = Describe("Test AutoscalingRunnerSet with a stale runner scale set", Ordered, func() {
+	const staleRunnerScaleSetID = 42
+	const freshRunnerScaleSetID = 1
+
+	var originalBuildVersion string
+	buildVersion := "0.1.0"
+
+	BeforeAll(func() {
+		originalBuildVersion = build.Version
+		build.Version = buildVersion
+	})
+
+	AfterAll(func() {
+		build.Version = originalBuildVersion
+	})
+
+	newAutoscalingRunnerSet := func(namespace, configSecretName string, annotations map[string]string) *v1alpha1.AutoscalingRunnerSet {
+		min := 1
+		max := 10
+		return &v1alpha1.AutoscalingRunnerSet{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:        "test-asrs",
+				Namespace:   namespace,
+				Labels:      map[string]string{LabelKeyKubernetesVersion: buildVersion},
+				Annotations: annotations,
+			},
+			Spec: v1alpha1.AutoscalingRunnerSetSpec{
+				GitHubConfigUrl:    "https://github.com/owner/repo",
+				GitHubConfigSecret: configSecretName,
+				MaxRunners:         &max,
+				MinRunners:         &min,
+				RunnerGroup:        "testgroup",
+				Template: corev1.PodTemplateSpec{
+					Spec: corev1.PodSpec{
+						Containers: []corev1.Container{
+							{
+								Name:  "runner",
+								Image: "ghcr.io/actions/runner",
+							},
+						},
+					},
+				},
+			},
+		}
+	}
+
+	registeredAnnotations := func(runnerScaleSetID int) map[string]string {
+		return map[string]string{
+			runnerScaleSetIDAnnotationKey:         strconv.Itoa(runnerScaleSetID),
+			AnnotationKeyGitHubRunnerScaleSetName: "test-asrs",
+			AnnotationKeyGitHubRunnerGroupName:    "testgroup",
+		}
+	}
+
+	Context("When the recorded runner scale set no longer exists on the Actions service", func() {
+		var ctx context.Context
+		var mgr ctrl.Manager
+		var autoscalingNS *corev1.Namespace
+		var autoscalingRunnerSet *v1alpha1.AutoscalingRunnerSet
+
+		BeforeEach(func() {
+			ctx = context.Background()
+			autoscalingNS, mgr = createNamespace(GinkgoT(), k8sClient)
+			configSecret := createDefaultSecret(GinkgoT(), k8sClient, autoscalingNS.Name)
+
+			controller := &AutoscalingRunnerSetReconciler{
+				Client:                             mgr.GetClient(),
+				Scheme:                             mgr.GetScheme(),
+				Log:                                logf.Log,
+				ControllerNamespace:                autoscalingNS.Name,
+				DefaultRunnerScaleSetListenerImage: "ghcr.io/actions/arc",
+				ResourceBuilder: ResourceBuilder{
+					SecretResolver: secretresolver.New(mgr.GetClient(), scalefake.NewMultiClient(
+						scalefake.WithClient(
+							scalefake.NewClient(
+								scalefake.WithGetRunnerGroupByName(&scaleset.RunnerGroup{ID: 1, Name: "testgroup"}, nil),
+								scalefake.WithGetRunnerScaleSetByIDFunc(func(_ context.Context, runnerScaleSetID int) (*scaleset.RunnerScaleSet, error) {
+									if runnerScaleSetID == staleRunnerScaleSetID {
+										return nil, scaleset.NotFoundError
+									}
+									return &scaleset.RunnerScaleSet{ID: runnerScaleSetID, Name: "test-asrs", RunnerGroupID: 1, RunnerGroupName: "testgroup"}, nil
+								}),
+								scalefake.WithGetRunnerScaleSet(nil, nil),
+								scalefake.WithCreateRunnerScaleSet(&scaleset.RunnerScaleSet{ID: freshRunnerScaleSetID, Name: "test-asrs", RunnerGroupID: 1, RunnerGroupName: "testgroup"}, nil),
+								scalefake.WithDeleteRunnerScaleSet(nil),
+							),
+						),
+					)),
+				},
+			}
+			Expect(controller.SetupWithManager(mgr)).To(Succeed(), "failed to setup controller")
+			startManagers(GinkgoT(), mgr)
+
+			autoscalingRunnerSet = newAutoscalingRunnerSet(autoscalingNS.Name, configSecret.Name, registeredAnnotations(staleRunnerScaleSetID))
+			Expect(k8sClient.Create(ctx, autoscalingRunnerSet)).To(Succeed(), "failed to create AutoScalingRunnerSet")
+		})
+
+		It("registers the runner scale set again and creates the listener with the new ID", func() {
+			Eventually(
+				func() (string, error) {
+					updated := new(v1alpha1.AutoscalingRunnerSet)
+					if err := k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingRunnerSet.Name, Namespace: autoscalingRunnerSet.Namespace}, updated); err != nil {
+						return "", err
+					}
+					return updated.Annotations[runnerScaleSetIDAnnotationKey], nil
+				},
+				autoscalingRunnerSetTestTimeout,
+				autoscalingRunnerSetTestInterval,
+			).Should(BeEquivalentTo(strconv.Itoa(freshRunnerScaleSetID)), "the stale runner scale set ID should be replaced")
+
+			Eventually(
+				func() (int, error) {
+					listener := new(v1alpha1.AutoscalingListener)
+					if err := k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerName(autoscalingRunnerSet), Namespace: autoscalingRunnerSet.Namespace}, listener); err != nil {
+						return 0, err
+					}
+					return listener.Spec.RunnerScaleSetID, nil
+				},
+				autoscalingRunnerSetTestTimeout,
+				autoscalingRunnerSetTestInterval,
+			).Should(BeEquivalentTo(freshRunnerScaleSetID), "the listener should never be created with the stale runner scale set ID")
+		})
+	})
+
+	Context("When the Actions service cannot confirm whether the runner scale set exists", func() {
+		var ctx context.Context
+		var mgr ctrl.Manager
+		var autoscalingNS *corev1.Namespace
+		var autoscalingRunnerSet *v1alpha1.AutoscalingRunnerSet
+		var createCalls atomic.Int64
+
+		BeforeEach(func() {
+			ctx = context.Background()
+			autoscalingNS, mgr = createNamespace(GinkgoT(), k8sClient)
+			configSecret := createDefaultSecret(GinkgoT(), k8sClient, autoscalingNS.Name)
+			createCalls.Store(0)
+
+			controller := &AutoscalingRunnerSetReconciler{
+				Client:                             mgr.GetClient(),
+				Scheme:                             mgr.GetScheme(),
+				Log:                                logf.Log,
+				ControllerNamespace:                autoscalingNS.Name,
+				DefaultRunnerScaleSetListenerImage: "ghcr.io/actions/arc",
+				ResourceBuilder: ResourceBuilder{
+					SecretResolver: secretresolver.New(mgr.GetClient(), scalefake.NewMultiClient(
+						scalefake.WithClient(
+							scalefake.NewClient(
+								scalefake.WithGetRunnerGroupByName(&scaleset.RunnerGroup{ID: 1, Name: "testgroup"}, nil),
+								scalefake.WithGetRunnerScaleSetByIDFunc(func(_ context.Context, _ int) (*scaleset.RunnerScaleSet, error) {
+									return nil, fmt.Errorf("actions service is unavailable")
+								}),
+								scalefake.WithGetRunnerScaleSet(nil, nil),
+								scalefake.WithCreateRunnerScaleSetFunc(func(_ context.Context, rs *scaleset.RunnerScaleSet) (*scaleset.RunnerScaleSet, error) {
+									createCalls.Add(1)
+									return &scaleset.RunnerScaleSet{ID: freshRunnerScaleSetID, Name: rs.Name, RunnerGroupID: 1, RunnerGroupName: "testgroup"}, nil
+								}),
+								scalefake.WithDeleteRunnerScaleSet(nil),
+							),
+						),
+					)),
+				},
+			}
+			Expect(controller.SetupWithManager(mgr)).To(Succeed(), "failed to setup controller")
+			startManagers(GinkgoT(), mgr)
+
+			autoscalingRunnerSet = newAutoscalingRunnerSet(autoscalingNS.Name, configSecret.Name, registeredAnnotations(staleRunnerScaleSetID))
+			Expect(k8sClient.Create(ctx, autoscalingRunnerSet)).To(Succeed(), "failed to create AutoScalingRunnerSet")
+		})
+
+		It("leaves the runner scale set alone instead of registering a second one", func() {
+			Consistently(
+				func() (string, error) {
+					updated := new(v1alpha1.AutoscalingRunnerSet)
+					if err := k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingRunnerSet.Name, Namespace: autoscalingRunnerSet.Namespace}, updated); err != nil {
+						return "", err
+					}
+					return updated.Annotations[runnerScaleSetIDAnnotationKey], nil
+				},
+				"3s",
+				autoscalingRunnerSetTestInterval,
+			).Should(BeEquivalentTo(strconv.Itoa(staleRunnerScaleSetID)), "a transient failure should never re-register the runner scale set")
+
+			Expect(createCalls.Load()).To(BeEquivalentTo(0), "no runner scale set should be created while the service is unavailable")
+		})
+	})
+
+	Context("When the runner scale set is already deleted from the Actions service", func() {
+		var ctx context.Context
+		var mgr ctrl.Manager
+		var autoscalingNS *corev1.Namespace
+		var autoscalingRunnerSet *v1alpha1.AutoscalingRunnerSet
+
+		BeforeEach(func() {
+			ctx = context.Background()
+			autoscalingNS, mgr = createNamespace(GinkgoT(), k8sClient)
+			configSecret := createDefaultSecret(GinkgoT(), k8sClient, autoscalingNS.Name)
+
+			controller := &AutoscalingRunnerSetReconciler{
+				Client:                             mgr.GetClient(),
+				Scheme:                             mgr.GetScheme(),
+				Log:                                logf.Log,
+				ControllerNamespace:                autoscalingNS.Name,
+				DefaultRunnerScaleSetListenerImage: "ghcr.io/actions/arc",
+				ResourceBuilder: ResourceBuilder{
+					SecretResolver: secretresolver.New(mgr.GetClient(), scalefake.NewMultiClient(
+						scalefake.WithClient(
+							scalefake.NewClient(
+								scalefake.WithGetRunnerGroupByName(&scaleset.RunnerGroup{ID: 1, Name: "testgroup"}, nil),
+								scalefake.WithGetRunnerScaleSetByIDFunc(func(_ context.Context, runnerScaleSetID int) (*scaleset.RunnerScaleSet, error) {
+									return &scaleset.RunnerScaleSet{ID: runnerScaleSetID, Name: "test-asrs", RunnerGroupID: 1, RunnerGroupName: "testgroup"}, nil
+								}),
+								scalefake.WithGetRunnerScaleSet(nil, nil),
+								scalefake.WithCreateRunnerScaleSet(&scaleset.RunnerScaleSet{ID: freshRunnerScaleSetID, Name: "test-asrs", RunnerGroupID: 1, RunnerGroupName: "testgroup"}, nil),
+								scalefake.WithDeleteRunnerScaleSet(scaleset.NotFoundError),
+							),
+						),
+					)),
+				},
+			}
+			Expect(controller.SetupWithManager(mgr)).To(Succeed(), "failed to setup controller")
+			startManagers(GinkgoT(), mgr)
+
+			autoscalingRunnerSet = newAutoscalingRunnerSet(autoscalingNS.Name, configSecret.Name, nil)
+			Expect(k8sClient.Create(ctx, autoscalingRunnerSet)).To(Succeed(), "failed to create AutoScalingRunnerSet")
+		})
+
+		It("does not leave the AutoscalingRunnerSet stuck in Terminating", func() {
+			Eventually(
+				func() error {
+					return k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerName(autoscalingRunnerSet), Namespace: autoscalingRunnerSet.Namespace}, new(v1alpha1.AutoscalingListener))
+				},
+				autoscalingRunnerSetTestTimeout,
+				autoscalingRunnerSetTestInterval,
+			).Should(Succeed(), "Listener should be created")
+
+			Expect(k8sClient.Delete(ctx, autoscalingRunnerSet)).To(Succeed(), "failed to delete AutoScalingRunnerSet")
+
+			Eventually(
+				func() error {
+					err := k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingRunnerSet.Name, Namespace: autoscalingRunnerSet.Namespace}, new(v1alpha1.AutoscalingRunnerSet))
+					if err != nil && errors.IsNotFound(err) {
+						return nil
+					}
+					return fmt.Errorf("AutoScalingRunnerSet is not deleted")
+				},
+				autoscalingRunnerSetTestTimeout,
+				autoscalingRunnerSetTestInterval,
+			).Should(Succeed(), "the finalizer should be removed even though the runner scale set is already gone")
+		})
+	})
+})

controllers/actions.github.com/multiclient/fake/client.go

@@ -26,6 +26,13 @@ func WithGetRunnerScaleSetByID(result *scaleset.RunnerScaleSet, err error) Clien
 	}
 }
 
+// WithGetRunnerScaleSetByIDFunc configures a function to handle GetRunnerScaleSetByID calls dynamically
+func WithGetRunnerScaleSetByIDFunc(fn func(context.Context, int) (*scaleset.RunnerScaleSet, error)) ClientOption {
+	return func(c *Client) {
+		c.getRunnerScaleSetByIDFunc = fn
+	}
+}
+
 // WithGetRunnerGroupByName configures the result of GetRunnerGroupByName
 func WithGetRunnerGroupByName(result *scaleset.RunnerGroup, err error) ClientOption {
 	return func(c *Client) {
@@ -135,6 +142,7 @@ type Client struct {
 		err error
 	}
 	getRunnerGroupByNameFunc   func(context.Context, string) (*scaleset.RunnerGroup, error)
+	getRunnerScaleSetByIDFunc  func(context.Context, int) (*scaleset.RunnerScaleSet, error)
 	createRunnerScaleSetResult struct {
 		*scaleset.RunnerScaleSet
 		err error
@@ -219,6 +227,9 @@ func (c *Client) GetRunnerScaleSet(ctx context.Context, runnerGroupID int, runne
 }
 
 func (c *Client) GetRunnerScaleSetByID(ctx context.Context, runnerScaleSetID int) (*scaleset.RunnerScaleSet, error) {
+	if c.getRunnerScaleSetByIDFunc != nil {
+		return c.getRunnerScaleSetByIDFunc(ctx, runnerScaleSetID)
+	}
 	return c.getRunnerScaleSetByIDResult.RunnerScaleSet, c.getRunnerScaleSetByIDResult.err
 }