diff --git a/controllers/actions.github.com/autoscalingrunnerset_controller.go b/controllers/actions.github.com/autoscalingrunnerset_controller.go index bae05cdf..391ca966 100644 --- a/controllers/actions.github.com/autoscalingrunnerset_controller.go +++ b/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) - if err != nil { - logger.Error(err, "Failed to get runner group by name", "runnerGroup", autoscalingRunnerSet.Spec.RunnerGroup) - return ctrl.Result{}, err + runnerGroupId := 1 + if len(autoscalingRunnerSet.Spec.RunnerGroup) > 0 { + runnerGroup, err := actionsClient.GetRunnerGroupByName(ctx, autoscalingRunnerSet.Spec.RunnerGroup) + 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 diff --git a/controllers/actions.github.com/autoscalingrunnerset_controller_test.go b/controllers/actions.github.com/autoscalingrunnerset_controller_test.go index d281d8cc..911b8c4d 100644 --- a/controllers/actions.github.com/autoscalingrunnerset_controller_test.go +++ b/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") + }) }) }) diff --git a/github/actions/client.go b/github/actions/client.go index 70b02b14..c447f7cf 100644 --- a/github/actions/client.go +++ b/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 diff --git a/github/actions/client_runner_scale_set_test.go b/github/actions/client_runner_scale_set_test.go index 5354a0e5..a1249417 100644 --- a/github/actions/client_runner_scale_set_test.go +++ b/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") + }) +} diff --git a/github/actions/fake/client.go b/github/actions/fake/client.go index 1983180e..29321b84 100644 --- a/github/actions/fake/client.go +++ b/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 } diff --git a/github/actions/mock_ActionsService.go b/github/actions/mock_ActionsService.go index d227e8be..ba10de8d 100644 --- a/github/actions/mock_ActionsService.go +++ b/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)