diff --git a/controllers/runnerreplicaset_controller.go b/controllers/runnerreplicaset_controller.go index 62511914..a9095ead 100644 --- a/controllers/runnerreplicaset_controller.go +++ b/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, - ®istrationOnlyRunner, - ); 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, ®istrationOnlyRunner); 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, + ®istrationOnlyRunner, + ); 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, ®istrationOnlyRunner); 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") diff --git a/controllers/runnerreplicaset_controller_test.go b/controllers/runnerreplicaset_controller_test.go index 5240b5e0..a8019dd4 100644 --- a/controllers/runnerreplicaset_controller_test.go +++ b/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.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), - }) - } + runnersList.Sync(runners.Items) 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.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), - }) - } + runnersList.Sync(runners.Items) return len(runners.Items) }, @@ -283,13 +265,7 @@ var _ = Context("Inside of a new namespace", func() { return -1 } - runnersList.Add(&github.Runner{ - ID: pointer.Int64Ptr(1001), - Name: pointer.StringPtr(regOnly.Name), - OS: pointer.StringPtr("linux"), - Status: pointer.StringPtr("offline"), - Busy: pointer.BoolPtr(false), - }) + runnersList.AddOffline([]actionsv1alpha1.Runner{*updated}) } 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.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), - }) - } + runnersList.Sync(runners.Items) return len(runners.Items) }, diff --git a/github/fake/runners.go b/github/fake/runners.go index d0484ddf..f6f77bf8 100644 --- a/github/fake/runners.go +++ b/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 {