Fix scale-from-zero to retain the reg-only runner until other pods come up (#523)

Fixes #516

controllers/runnerreplicaset_controller.go

@@ -68,65 +68,6 @@ func (r *RunnerReplicaSetReconciler) Reconcile(req ctrl.Request) (ctrl.Result, e
 		return ctrl.Result{}, nil
 	}
 
-	registrationOnlyRunnerNeeded := rs.Spec.Replicas != nil && *rs.Spec.Replicas == 0
-	registrationOnlyRunner := v1alpha1.Runner{}
-	registrationOnlyRunnerNsName := req.NamespacedName
-	registrationOnlyRunnerNsName.Name = registrationOnlyRunnerNameFor(rs.Name)
-
-	registrationOnlyRunnerExists := false
-	if err := r.Get(
-		ctx,
-		registrationOnlyRunnerNsName,
-		&registrationOnlyRunner,
-	); err != nil {
-		if !kerrors.IsNotFound(err) {
-			return ctrl.Result{}, err
-		}
-	} else {
-		registrationOnlyRunnerExists = true
-	}
-
-	if registrationOnlyRunnerNeeded {
-		if registrationOnlyRunnerExists {
-			if registrationOnlyRunner.Status.Phase == "" {
-				log.Info("Still waiting for the registration-only runner to be registered")
-
-				return ctrl.Result{}, nil
-			}
-		} else {
-			// A registration-only runner does not exist and is needed, hence create it.
-
-			runnerForScaleFromToZero, err := r.newRunner(rs)
-			if err != nil {
-				return ctrl.Result{}, fmt.Errorf("failed to create runner for scale from/to zero: %v", err)
-			}
-
-			runnerForScaleFromToZero.ObjectMeta.Name = registrationOnlyRunnerNsName.Name
-			runnerForScaleFromToZero.ObjectMeta.GenerateName = ""
-			runnerForScaleFromToZero.ObjectMeta.Labels = nil
-			metav1.SetMetaDataAnnotation(&runnerForScaleFromToZero.ObjectMeta, annotationKeyRegistrationOnly, "true")
-
-			if err := r.Client.Create(ctx, &runnerForScaleFromToZero); err != nil {
-				log.Error(err, "Failed to create runner for scale from/to zero")
-
-				return ctrl.Result{}, err
-			}
-
-			// We can continue to deleting runner pods only after the
-			// registration-only runner gets registered.
-			return ctrl.Result{}, nil
-		}
-	} else {
-		// A registration-only runner exists and is not needed, hence delete it.
-		if registrationOnlyRunnerExists {
-			if err := r.Client.Delete(ctx, &registrationOnlyRunner); err != nil {
-				log.Error(err, "Retrying soon because we failed to delete registration-only runner")
-
-				return ctrl.Result{Requeue: true}, nil
-			}
-		}
-	}
-
 	selector, err := metav1.LabelSelectorAsSelector(rs.Spec.Selector)
 	if err != nil {
 		return ctrl.Result{}, err
@@ -173,6 +114,71 @@ func (r *RunnerReplicaSetReconciler) Reconcile(req ctrl.Request) (ctrl.Result, e
 		desired = 1
 	}
 
+	registrationOnlyRunnerNsName := req.NamespacedName
+	registrationOnlyRunnerNsName.Name = registrationOnlyRunnerNameFor(rs.Name)
+	registrationOnlyRunner := v1alpha1.Runner{}
+	registrationOnlyRunnerExists := false
+	if err := r.Get(
+		ctx,
+		registrationOnlyRunnerNsName,
+		&registrationOnlyRunner,
+	); err != nil {
+		if !kerrors.IsNotFound(err) {
+			return ctrl.Result{}, err
+		}
+	} else {
+		registrationOnlyRunnerExists = true
+	}
+
+	// On scale to zero, we must have fully registered registration-only runner before we start deleting other runners, hence `desired == 0`
+	// On scale from zero, we must retain the registratoin-only runner until one or more other runners get registered, hence `registrationOnlyRunnerExists && available == 0`.
+	// On RunnerReplicaSet creation, it have always 0 replics and no registration-only runner.
+	// In this case We don't need to bother creating a registration-only runner which gets deleted soon after we have 1 or more available repolicas,
+	// hence it's not `available == 0`, but `registrationOnlyRunnerExists && available == 0`.
+	// See https://github.com/actions-runner-controller/actions-runner-controller/issues/516
+	registrationOnlyRunnerNeeded := desired == 0 || (registrationOnlyRunnerExists && available == 0)
+
+	if registrationOnlyRunnerNeeded {
+		if registrationOnlyRunnerExists {
+			if registrationOnlyRunner.Status.Phase == "" {
+				log.Info("Still waiting for the registration-only runner to be registered")
+
+				return ctrl.Result{}, nil
+			}
+		} else {
+			// A registration-only runner does not exist and is needed, hence create it.
+
+			runnerForScaleFromToZero, err := r.newRunner(rs)
+			if err != nil {
+				return ctrl.Result{}, fmt.Errorf("failed to create runner for scale from/to zero: %v", err)
+			}
+
+			runnerForScaleFromToZero.ObjectMeta.Name = registrationOnlyRunnerNsName.Name
+			runnerForScaleFromToZero.ObjectMeta.GenerateName = ""
+			runnerForScaleFromToZero.ObjectMeta.Labels = nil
+			metav1.SetMetaDataAnnotation(&runnerForScaleFromToZero.ObjectMeta, annotationKeyRegistrationOnly, "true")
+
+			if err := r.Client.Create(ctx, &runnerForScaleFromToZero); err != nil {
+				log.Error(err, "Failed to create runner for scale from/to zero")
+
+				return ctrl.Result{}, err
+			}
+
+			// We can continue to deleting runner pods only after the
+			// registration-only runner gets registered.
+			return ctrl.Result{}, nil
+		}
+	} else {
+		// A registration-only runner exists and is not needed, hence delete it.
+		if registrationOnlyRunnerExists {
+			if err := r.Client.Delete(ctx, &registrationOnlyRunner); err != nil {
+				log.Error(err, "Retrying soon because we failed to delete registration-only runner")
+
+				return ctrl.Result{Requeue: true}, nil
+			}
+		}
+	}
+
 	if available > desired {
 		n := available - desired
 
@@ -244,6 +250,8 @@ func (r *RunnerReplicaSetReconciler) Reconcile(req ctrl.Request) (ctrl.Result, e
 			n = len(deletionCandidates)
 		}
 
+		log.V(0).Info(fmt.Sprintf("Deleting %d runner(s)", n), "desired", desired, "available", available, "ready", ready)
+
 		for i := 0; i < n; i++ {
 			if err := r.Client.Delete(ctx, &deletionCandidates[i]); client.IgnoreNotFound(err) != nil {
 				log.Error(err, "Failed to delete runner resource")

controllers/runnerreplicaset_controller_test.go

@@ -7,11 +7,9 @@ import (
 	"net/http/httptest"
 	"time"
 
-	"github.com/google/go-github/v33/github"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/kubernetes/scheme"
-	"k8s.io/utils/pointer"
 	ctrl "sigs.k8s.io/controller-runtime"
 	logf "sigs.k8s.io/controller-runtime/pkg/log"
 
@@ -170,15 +168,7 @@ var _ = Context("Inside of a new namespace", func() {
 							return -1
 						}
 
-						for i, runner := range runners.Items {
+						runnersList.Sync(runners.Items)
-							runnersList.Add(&github.Runner{
-								ID:     pointer.Int64Ptr(int64(i) + 1),
-								Name:   pointer.StringPtr(runner.Name),
-								OS:     pointer.StringPtr("linux"),
-								Status: pointer.StringPtr("online"),
-								Busy:   pointer.BoolPtr(false),
-							})
-						}
 
 						return len(runners.Items)
 					},
@@ -227,15 +217,7 @@ var _ = Context("Inside of a new namespace", func() {
 							logf.Log.Error(err, "list runners")
 						}
 
-						for i, runner := range runners.Items {
+						runnersList.Sync(runners.Items)
-							runnersList.Add(&github.Runner{
-								ID:     pointer.Int64Ptr(int64(i) + 1),
-								Name:   pointer.StringPtr(runner.Name),
-								OS:     pointer.StringPtr("linux"),
-								Status: pointer.StringPtr("online"),
-								Busy:   pointer.BoolPtr(false),
-							})
-						}
 
 						return len(runners.Items)
 					},
@@ -283,13 +265,7 @@ var _ = Context("Inside of a new namespace", func() {
 								return -1
 							}
 
-							runnersList.Add(&github.Runner{
+							runnersList.AddOffline([]actionsv1alpha1.Runner{*updated})
-								ID:     pointer.Int64Ptr(1001),
-								Name:   pointer.StringPtr(regOnly.Name),
-								OS:     pointer.StringPtr("linux"),
-								Status: pointer.StringPtr("offline"),
-								Busy:   pointer.BoolPtr(false),
-							})
 						}
 
 						if err := k8sClient.List(ctx, &runners, client.InNamespace(ns.Name), client.MatchingLabelsSelector{Selector: selector}); err != nil {
@@ -297,15 +273,7 @@ var _ = Context("Inside of a new namespace", func() {
 							return -1
 						}
 
-						for i, runner := range runners.Items {
+						runnersList.Sync(runners.Items)
-							runnersList.Add(&github.Runner{
-								ID:     pointer.Int64Ptr(int64(i) + 1),
-								Name:   pointer.StringPtr(runner.Name),
-								OS:     pointer.StringPtr("linux"),
-								Status: pointer.StringPtr("online"),
-								Busy:   pointer.BoolPtr(false),
-							})
-						}
 
 						return len(runners.Items)
 					},

github/fake/runners.go

@@ -2,11 +2,12 @@ package fake
 
 import (
 	"encoding/json"
-	"github.com/summerwind/actions-runner-controller/api/v1alpha1"
 	"net/http"
 	"net/http/httptest"
 	"strconv"
 
+	"github.com/summerwind/actions-runner-controller/api/v1alpha1"
+
 	"github.com/google/go-github/v33/github"
 	"github.com/gorilla/mux"
 )
@@ -79,6 +80,18 @@ func (r *RunnersList) Sync(runners []v1alpha1.Runner) {
 	}
 }
 
+func (r *RunnersList) AddOffline(runners []v1alpha1.Runner) {
+	for i, want := range runners {
+		r.Add(&github.Runner{
+			ID:     github.Int64(int64(1000 + i)),
+			Name:   github.String(want.Name),
+			OS:     github.String("linux"),
+			Status: github.String("offline"),
+			Busy:   github.Bool(false),
+		})
+	}
+}
+
 func exists(runners []*github.Runner, runner *github.Runner) bool {
 	for _, r := range runners {
 		if *r.Name == *runner.Name {