Delete RunnerScaleSet on service when AutoScalingRunnerSet is deleted. (#2223)

controllers/actions.github.com/autoscalingrunnerset_controller.go

@@ -112,6 +112,12 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl
 				return ctrl.Result{}, nil
 			}
 
+			err = r.deleteRunnerScaleSet(ctx, autoscalingRunnerSet, log)
+			if err != nil {
+				log.Error(err, "Failed to delete runner scale set")
+				return ctrl.Result{}, err
+			}
+
 			log.Info("Removing finalizer")
 			err = patch(ctx, r.Client, autoscalingRunnerSet, func(obj *v1alpha1.AutoscalingRunnerSet) {
 				controllerutil.RemoveFinalizer(obj, autoscalingRunnerSetFinalizerName)
@@ -154,7 +160,7 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl
 
 	// Make sure the runner group of the scale set is up to date
 	currentRunnerGroupName, ok := autoscalingRunnerSet.Annotations[runnerScaleSetRunnerGroupNameKey]
-	if !ok || !strings.EqualFold(currentRunnerGroupName, autoscalingRunnerSet.Spec.RunnerGroup) {
+	if !ok || (len(autoscalingRunnerSet.Spec.RunnerGroup) > 0 && !strings.EqualFold(currentRunnerGroupName, autoscalingRunnerSet.Spec.RunnerGroup)) {
 		log.Info("AutoScalingRunnerSet runner group changed. Updating the runner scale set.")
 		return r.updateRunnerScaleSetRunnerGroup(ctx, autoscalingRunnerSet, log)
 	}
@@ -185,7 +191,7 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl
 	}
 
 	if desiredSpecHash != latestRunnerSet.Labels[LabelKeyRunnerSpecHash] {
-		log.Info("Latest runner set spec hash does not match the current autoscaling runner set. Creating a new runner set ")
+		log.Info("Latest runner set spec hash does not match the current autoscaling runner set. Creating a new runner set")
 		return r.createEphemeralRunnerSet(ctx, autoscalingRunnerSet, log)
 	}
 
@@ -342,7 +348,7 @@ func (r *AutoscalingRunnerSetReconciler) createRunnerScaleSet(ctx context.Contex
 		}
 	}
 
-	logger.Info("Created/Reused a runner scale set", "id", runnerScaleSet.Id)
+	logger.Info("Created/Reused a runner scale set", "id", runnerScaleSet.Id, "runnerGroupName", runnerScaleSet.RunnerGroupName)
 	if autoscalingRunnerSet.Annotations == nil {
 		autoscalingRunnerSet.Annotations = map[string]string{}
 	}
@@ -356,7 +362,7 @@ func (r *AutoscalingRunnerSetReconciler) createRunnerScaleSet(ctx context.Contex
 		return ctrl.Result{}, err
 	}
 
-	logger.Info("Updated with runner scale set ID as an annotation")
+	logger.Info("Updated with runner scale set ID and runner group name as an annotation")
 	return ctrl.Result{}, nil
 }
 
@@ -373,13 +379,18 @@ func (r *AutoscalingRunnerSetReconciler) updateRunnerScaleSetRunnerGroup(ctx con
 		return ctrl.Result{}, err
 	}
 
-	runnerGroup, err := actionsClient.GetRunnerGroupByName(ctx, autoscalingRunnerSet.Spec.RunnerGroup)
+	runnerGroupId := 1
-	if err != nil {
+	if len(autoscalingRunnerSet.Spec.RunnerGroup) > 0 {
-		logger.Error(err, "Failed to get runner group by name", "runnerGroup", autoscalingRunnerSet.Spec.RunnerGroup)
+		runnerGroup, err := actionsClient.GetRunnerGroupByName(ctx, autoscalingRunnerSet.Spec.RunnerGroup)
-		return ctrl.Result{}, err
+		if err != nil {
+			logger.Error(err, "Failed to get runner group by name", "runnerGroup", autoscalingRunnerSet.Spec.RunnerGroup)
+			return ctrl.Result{}, err
+		}
+
+		runnerGroupId = int(runnerGroup.ID)
 	}
 
-	updatedRunnerScaleSet, err := actionsClient.UpdateRunnerScaleSet(ctx, runnerScaleSetId, &actions.RunnerScaleSet{Name: autoscalingRunnerSet.Name, RunnerGroupId: int(runnerGroup.ID)})
+	updatedRunnerScaleSet, err := actionsClient.UpdateRunnerScaleSet(ctx, runnerScaleSetId, &actions.RunnerScaleSet{Name: autoscalingRunnerSet.Name, RunnerGroupId: runnerGroupId})
 	if err != nil {
 		logger.Error(err, "Failed to update runner scale set", "runnerScaleSetId", runnerScaleSetId)
 		return ctrl.Result{}, err
@@ -397,6 +408,30 @@ func (r *AutoscalingRunnerSetReconciler) updateRunnerScaleSetRunnerGroup(ctx con
 	return ctrl.Result{}, nil
 }
 
+func (r *AutoscalingRunnerSetReconciler) deleteRunnerScaleSet(ctx context.Context, autoscalingRunnerSet *v1alpha1.AutoscalingRunnerSet, logger logr.Logger) error {
+	logger.Info("Deleting the runner scale set from Actions service")
+	runnerScaleSetId, err := strconv.Atoi(autoscalingRunnerSet.Annotations[runnerScaleSetIdKey])
+	if err != nil {
+		logger.Error(err, "Failed to parse runner scale set ID")
+		return err
+	}
+
+	actionsClient, err := r.actionsClientFor(ctx, autoscalingRunnerSet)
+	if err != nil {
+		logger.Error(err, "Failed to initialize Actions service client for updating a existing runner scale set")
+		return err
+	}
+
+	err = actionsClient.DeleteRunnerScaleSet(ctx, runnerScaleSetId)
+	if err != nil {
+		logger.Error(err, "Failed to delete runner scale set", "runnerScaleSetId", runnerScaleSetId)
+		return err
+	}
+
+	logger.Info("Deleted the runner scale set from Actions service")
+	return nil
+}
+
 func (r *AutoscalingRunnerSetReconciler) createEphemeralRunnerSet(ctx context.Context, autoscalingRunnerSet *v1alpha1.AutoscalingRunnerSet, log logr.Logger) (ctrl.Result, error) {
 	desiredRunnerSet, err := r.resourceBuilder.newEphemeralRunnerSet(autoscalingRunnerSet)
 	if err != nil {
@@ -409,7 +444,7 @@ func (r *AutoscalingRunnerSetReconciler) createEphemeralRunnerSet(ctx context.Co
 		return ctrl.Result{}, err
 	}
 
-	log.Info("Creating a new EphemeralRunnerSet resource", "name", desiredRunnerSet.Name)
+	log.Info("Creating a new EphemeralRunnerSet resource")
 	if err := r.Create(ctx, desiredRunnerSet); err != nil {
 		log.Error(err, "Failed to create EphemeralRunnerSet resource")
 		return ctrl.Result{}, err

controllers/actions.github.com/autoscalingrunnerset_controller_test.go

@@ -368,5 +368,61 @@ var _ = Describe("Test AutoScalingRunnerSet controller", func() {
 				autoscalingRunnerSetTestTimeout,
 				autoscalingRunnerSetTestInterval).ShouldNot(BeEquivalentTo(string(listener.UID)), "New Listener should be created")
 		})
+
+		It("It should update RunnerScaleSet's runner group on service when it changes", func() {
+			updated := new(actionsv1alpha1.AutoscalingRunnerSet)
+			// Wait till the listener is created
+			Eventually(
+				func() error {
+					return k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerName(autoscalingRunnerSet), Namespace: autoscalingRunnerSet.Namespace}, new(actionsv1alpha1.AutoscalingListener))
+				},
+				autoscalingRunnerSetTestTimeout,
+				autoscalingRunnerSetTestInterval).Should(Succeed(), "Listener should be created")
+
+			patched := autoscalingRunnerSet.DeepCopy()
+			patched.Spec.RunnerGroup = "testgroup2"
+			err := k8sClient.Patch(ctx, patched, client.MergeFrom(autoscalingRunnerSet))
+			Expect(err).NotTo(HaveOccurred(), "failed to patch AutoScalingRunnerSet")
+
+			// Check if AutoScalingRunnerSet has the new runner group in its annotation
+			Eventually(
+				func() (string, error) {
+					err := k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingRunnerSet.Name, Namespace: autoscalingRunnerSet.Namespace}, updated)
+					if err != nil {
+						return "", err
+					}
+
+					if _, ok := updated.Annotations[runnerScaleSetRunnerGroupNameKey]; !ok {
+						return "", nil
+					}
+
+					return updated.Annotations[runnerScaleSetRunnerGroupNameKey], nil
+				},
+				autoscalingRunnerSetTestTimeout,
+				autoscalingRunnerSetTestInterval).Should(BeEquivalentTo("testgroup2"), "AutoScalingRunnerSet should have the new runner group in its annotation")
+
+			// delete the annotation and it should be re-added
+			patched = autoscalingRunnerSet.DeepCopy()
+			delete(patched.Annotations, runnerScaleSetRunnerGroupNameKey)
+			err = k8sClient.Patch(ctx, patched, client.MergeFrom(autoscalingRunnerSet))
+			Expect(err).NotTo(HaveOccurred(), "failed to patch AutoScalingRunnerSet")
+
+			// Check if AutoScalingRunnerSet still has the runner group in its annotation
+			Eventually(
+				func() (string, error) {
+					err := k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingRunnerSet.Name, Namespace: autoscalingRunnerSet.Namespace}, updated)
+					if err != nil {
+						return "", err
+					}
+
+					if _, ok := updated.Annotations[runnerScaleSetRunnerGroupNameKey]; !ok {
+						return "", nil
+					}
+
+					return updated.Annotations[runnerScaleSetRunnerGroupNameKey], nil
+				},
+				autoscalingRunnerSetTestTimeout,
+				autoscalingRunnerSetTestInterval).Should(BeEquivalentTo("testgroup2"), "AutoScalingRunnerSet should have the runner group in its annotation")
+		})
 	})
 })

github/actions/client.go

@@ -35,6 +35,7 @@ type ActionsService interface {
 	GetRunnerGroupByName(ctx context.Context, runnerGroup string) (*RunnerGroup, error)
 	CreateRunnerScaleSet(ctx context.Context, runnerScaleSet *RunnerScaleSet) (*RunnerScaleSet, error)
 	UpdateRunnerScaleSet(ctx context.Context, runnerScaleSetId int, runnerScaleSet *RunnerScaleSet) (*RunnerScaleSet, error)
+	DeleteRunnerScaleSet(ctx context.Context, runnerScaleSetId int) error
 
 	CreateMessageSession(ctx context.Context, runnerScaleSetId int, owner string) (*RunnerScaleSetSession, error)
 	DeleteMessageSession(ctx context.Context, runnerScaleSetId int, sessionId *uuid.UUID) error

github/actions/client_runner_scale_set_test.go

@@ -379,3 +379,39 @@ func TestUpdateRunnerScaleSet(t *testing.T) {
 		require.NoError(t, err)
 	})
 }
+
+func TestDeleteRunnerScaleSet(t *testing.T) {
+	ctx := context.Background()
+	auth := &actions.ActionsAuth{
+		Token: "token",
+	}
+
+	t.Run("Delete runner scale set", func(t *testing.T) {
+		server := newActionsServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			assert.Equal(t, "DELETE", r.Method)
+			assert.Contains(t, r.URL.String(), "/_apis/runtime/runnerscalesets/10?api-version=6.0-preview")
+			w.WriteHeader(http.StatusNoContent)
+		}))
+
+		client, err := actions.NewClient(ctx, server.configURLForOrg("my-org"), auth)
+		require.NoError(t, err)
+
+		err = client.DeleteRunnerScaleSet(ctx, 10)
+		assert.NoError(t, err)
+	})
+
+	t.Run("Delete calls with error", func(t *testing.T) {
+		server := newActionsServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			assert.Equal(t, "DELETE", r.Method)
+			assert.Contains(t, r.URL.String(), "/_apis/runtime/runnerscalesets/10?api-version=6.0-preview")
+			w.WriteHeader(http.StatusBadRequest)
+			w.Write([]byte(`{"message": "test error"}`))
+		}))
+
+		client, err := actions.NewClient(ctx, server.configURLForOrg("my-org"), auth)
+		require.NoError(t, err)
+
+		err = client.DeleteRunnerScaleSet(ctx, 10)
+		assert.ErrorContains(t, err, "test error")
+	})
+}

github/actions/fake/client.go

@@ -17,6 +17,13 @@ func WithGetRunnerScaleSetResult(scaleSet *actions.RunnerScaleSet, err error) Op
 	}
 }
 
+func WithGetRunnerGroup(runnerGroup *actions.RunnerGroup, err error) Option {
+	return func(f *FakeClient) {
+		f.getRunnerGroupByNameResult.RunnerGroup = runnerGroup
+		f.getRunnerGroupByNameResult.err = err
+	}
+}
+
 func WithGetRunner(runner *actions.RunnerReference, err error) Option {
 	return func(f *FakeClient) {
 		f.getRunnerResult.RunnerReference = runner
@@ -40,7 +47,7 @@ var defaultUpdatedRunnerScaleSet = &actions.RunnerScaleSet{
 	Id:                 1,
 	Name:               "testset",
 	RunnerGroupId:      2,
-	RunnerGroupName:    "testgroup",
+	RunnerGroupName:    "testgroup2",
 	Labels:             []actions.Label{{Type: "test", Name: "test"}},
 	RunnerSetting:      actions.RunnerSetting{},
 	CreatedOn:          time.Now(),
@@ -123,6 +130,9 @@ type FakeClient struct {
 		*actions.RunnerScaleSet
 		err error
 	}
+	deleteRunnerScaleSetResult struct {
+		err error
+	}
 	createMessageSessionResult struct {
 		*actions.RunnerScaleSetSession
 		err error
@@ -211,6 +221,10 @@ func (f *FakeClient) UpdateRunnerScaleSet(ctx context.Context, runnerScaleSetId
 	return f.updateRunnerScaleSetResult.RunnerScaleSet, f.updateRunnerScaleSetResult.err
 }
 
+func (f *FakeClient) DeleteRunnerScaleSet(ctx context.Context, runnerScaleSetId int) error {
+	return f.deleteRunnerScaleSetResult.err
+}
+
 func (f *FakeClient) CreateMessageSession(ctx context.Context, runnerScaleSetId int, owner string) (*actions.RunnerScaleSetSession, error) {
 	return f.createMessageSessionResult.RunnerScaleSetSession, f.createMessageSessionResult.err
 }

github/actions/mock_ActionsService.go

@@ -111,6 +111,20 @@ func (_m *MockActionsService) DeleteMessageSession(ctx context.Context, runnerSc
 	return r0
 }
 
+// DeleteRunnerScaleSet provides a mock function with given fields: ctx, runnerScaleSetId
+func (_m *MockActionsService) DeleteRunnerScaleSet(ctx context.Context, runnerScaleSetId int) error {
+	ret := _m.Called(ctx, runnerScaleSetId)
+
+	var r0 error
+	if rf, ok := ret.Get(0).(func(context.Context, int) error); ok {
+		r0 = rf(ctx, runnerScaleSetId)
+	} else {
+		r0 = ret.Error(0)
+	}
+
+	return r0
+}
+
 // GenerateJitRunnerConfig provides a mock function with given fields: ctx, jitRunnerSetting, scaleSetId
 func (_m *MockActionsService) GenerateJitRunnerConfig(ctx context.Context, jitRunnerSetting *RunnerScaleSetJitRunnerSetting, scaleSetId int) (*RunnerScaleSetJitRunnerConfig, error) {
 	ret := _m.Called(ctx, jitRunnerSetting, scaleSetId)