diff --git a/controllers/actions.github.com/autoscalingrunnerset_controller.go b/controllers/actions.github.com/autoscalingrunnerset_controller.go index 0cbf1b49..a632c3f0 100644 --- a/controllers/actions.github.com/autoscalingrunnerset_controller.go +++ b/controllers/actions.github.com/autoscalingrunnerset_controller.go @@ -303,16 +303,12 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl ephemeralRunnerMetadataModified := !cmp.Equal(ephemeralRunnerSet.Spec.EphemeralRunnerMetadata, desired.Spec.EphemeralRunnerMetadata) ephemeralRunnerLabelsModified := !maps.Equal(ephemeralRunnerSet.Labels, desired.Labels) ephemeralRunnerAnnotationsModified := !maps.Equal(ephemeralRunnerSet.Annotations, desired.Annotations) - ephemeralRunnerReplicasModified := ephemeralRunnerSet.Spec.Replicas != desired.Spec.Replicas - ephemeralRunnerPatchIDModified := ephemeralRunnerSet.Spec.PatchID != desired.Spec.PatchID - if ephemeralRunnerLabelsModified || ephemeralRunnerAnnotationsModified || ephemeralRunnerMetadataModified || ephemeralRunnerReplicasModified || ephemeralRunnerPatchIDModified { + if ephemeralRunnerLabelsModified || ephemeralRunnerAnnotationsModified || ephemeralRunnerMetadataModified { original := ephemeralRunnerSet.DeepCopy() ephemeralRunnerSet.Labels = r.filterAndMergeLabels(ephemeralRunnerSet.Labels, desired.Labels) ephemeralRunnerSet.Annotations = desired.Annotations ephemeralRunnerSet.Spec.EphemeralRunnerMetadata = desired.Spec.EphemeralRunnerMetadata - ephemeralRunnerSet.Spec.Replicas = desired.Spec.Replicas - ephemeralRunnerSet.Spec.PatchID = desired.Spec.PatchID log.Info("Updating ephemeral runner set metadata to match desired labels and annotations") if err := r.Patch(ctx, &ephemeralRunnerSet, client.MergeFrom(original)); err != nil { log.Error(err, "Failed to patch ephemeral runner set metadata to match desired labels and annotations") @@ -759,8 +755,6 @@ func (r *AutoscalingRunnerSetReconciler) createEphemeralRunnerSet(ctx context.Co log.Error(err, "Could not create EphemeralRunnerSet") return ctrl.Result{}, err } - desiredRunnerSet.Spec.ActionableRevision = 1 - log.Info("Creating a new EphemeralRunnerSet resource") if err := r.Create(ctx, desiredRunnerSet); err != nil { log.Error(err, "Failed to create EphemeralRunnerSet resource") diff --git a/controllers/actions.github.com/ephemeralrunnerset_controller.go b/controllers/actions.github.com/ephemeralrunnerset_controller.go index 964c7271..e25959d9 100644 --- a/controllers/actions.github.com/ephemeralrunnerset_controller.go +++ b/controllers/actions.github.com/ephemeralrunnerset_controller.go @@ -140,7 +140,7 @@ func (r *EphemeralRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl.R "specActionableRevision", ephemeralRunnerSet.Spec.ActionableRevision, "statusAppliedActionableRevision", ephemeralRunnerSet.Status.AppliedActionableRevision, ) - if err := r.cleanUpIdleAndPendingEphemeralRunners(ctx, &ephemeralRunnerSet, log); err != nil { + if _, err := r.cleanUpEphemeralRunners(ctx, &ephemeralRunnerSet, log); err != nil { log.Error(err, "Failed to clean up EphemeralRunners") return ctrl.Result{}, err } @@ -422,60 +422,6 @@ func (r *EphemeralRunnerSetReconciler) cleanUpEphemeralRunners(ctx context.Conte return false, nil } -func (r *EphemeralRunnerSetReconciler) cleanUpIdleAndPendingEphemeralRunners(ctx context.Context, ephemeralRunnerSet *v1alpha1.EphemeralRunnerSet, log logr.Logger) error { - ephemeralRunnerList := new(v1alpha1.EphemeralRunnerList) - err := r.List(ctx, ephemeralRunnerList, client.InNamespace(ephemeralRunnerSet.Namespace), client.MatchingFields{resourceOwnerKey: ephemeralRunnerSet.Name}) - if err != nil { - return fmt.Errorf("failed to list child ephemeral runners: %w", err) - } - - ephemeralRunnerState := newEphemeralRunnersByStates(ephemeralRunnerList) - if len(ephemeralRunnerState.running) == 0 && len(ephemeralRunnerState.pending) == 0 { - return nil - } - - actionsClient, err := r.GetActionsService(ctx, ephemeralRunnerSet) - if err != nil { - return err - } - - log.Info("Cleanup pending or idle ephemeral runners", "pending", len(ephemeralRunnerState.pending), "running", len(ephemeralRunnerState.running)) - var errs []error - for _, ephemeralRunner := range ephemeralRunnerState.pending { - log.Info("Removing the pending ephemeral runner from the service", "name", ephemeralRunner.Name) - _, err := r.deleteEphemeralRunnerWithActionsClient(ctx, ephemeralRunner, actionsClient, log) - if err != nil { - errs = append(errs, err) - } - } - - for _, ephemeralRunner := range ephemeralRunnerState.running { - if ephemeralRunner.HasJob() { - log.Info( - "Skipping ephemeral runner since it is running a job", - "name", ephemeralRunner.Name, - "workflowRunId", ephemeralRunner.Status.WorkflowRunID, - "jobId", ephemeralRunner.Status.JobID, - ) - continue - } - - log.Info("Removing the idle ephemeral runner from the service", "name", ephemeralRunner.Name) - _, err := r.deleteEphemeralRunnerWithActionsClient(ctx, ephemeralRunner, actionsClient, log) - if err != nil { - errs = append(errs, err) - } - } - - if len(errs) > 0 { - mergedErrs := multierr.Combine(errs...) - log.Error(mergedErrs, "Failed to remove idle or pending ephemeral runners from the service") - return mergedErrs - } - - return nil -} - func (r *EphemeralRunnerSetReconciler) cleanUpEphemeralRunnerSetProxySecret(ctx context.Context, ephemeralRunnerSet *v1alpha1.EphemeralRunnerSet, log logr.Logger) (done bool, err error) { if ephemeralRunnerSet.Spec.EphemeralRunnerSpec.Proxy == nil { return true, nil diff --git a/controllers/actions.github.com/ephemeralrunnerset_controller_test.go b/controllers/actions.github.com/ephemeralrunnerset_controller_test.go index 1d25203e..87a93d1d 100644 --- a/controllers/actions.github.com/ephemeralrunnerset_controller_test.go +++ b/controllers/actions.github.com/ephemeralrunnerset_controller_test.go @@ -1523,6 +1523,59 @@ var _ = Describe("Test EphemeralRunnerSet actionable revision cleanup", func() { startManagers(GinkgoT(), mgr) }) + It("does not clean up runners on initial creation without an actionable revision", func() { + controller := &EphemeralRunnerSetReconciler{ + Client: mgr.GetClient(), + Scheme: mgr.GetScheme(), + Log: logf.Log, + ResourceBuilder: ResourceBuilder{ + ResourceCache: newTestResourceCache(), + SecretResolver: secretresolver.New(mgr.GetClient(), fake.NewMultiClient( + fake.WithClient(fake.NewClient(fake.WithRemoveRunner(nil))), + )), + }, + } + + ephemeralRunnerSet := &v1alpha1.EphemeralRunnerSet{ + ObjectMeta: metav1.ObjectMeta{Name: "test-actionable-revision-initial", Namespace: autoscalingNS.Name}, + Spec: v1alpha1.EphemeralRunnerSetSpec{ + EphemeralRunnerSpec: v1alpha1.EphemeralRunnerSpec{ + GitHubConfigURL: "https://github.com/owner/repo", + GitHubConfigSecret: configSecret.Name, + RunnerScaleSetID: 100, + PodTemplateSpec: corev1.PodTemplateSpec{Spec: corev1.PodSpec{Containers: []corev1.Container{{Name: "runner", Image: "ghcr.io/actions/runner"}}}}, + }, + }, + } + + err := k8sClient.Create(ctx, ephemeralRunnerSet) + Expect(err).NotTo(HaveOccurred()) + + request := ctrl.Request{NamespacedName: types.NamespacedName{Name: ephemeralRunnerSet.Name, Namespace: ephemeralRunnerSet.Namespace}} + _, err = controller.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + + pendingRunner := newRunner("runner-pending-initial", ephemeralRunnerSet) + err = k8sClient.Create(ctx, pendingRunner) + Expect(err).NotTo(HaveOccurred()) + + _, err = controller.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + + Consistently(func() error { + runner := new(v1alpha1.EphemeralRunner) + return k8sClient.Get(ctx, types.NamespacedName{Namespace: autoscalingNS.Name, Name: pendingRunner.Name}, runner) + }, time.Second, ephemeralRunnerSetTestInterval).Should(Succeed()) + + Consistently(func() int64 { + updatedSet := new(v1alpha1.EphemeralRunnerSet) + if err := k8sClient.Get(ctx, request.NamespacedName, updatedSet); err != nil { + return -1 + } + return updatedSet.Status.AppliedActionableRevision + }, time.Second, ephemeralRunnerSetTestInterval).Should(Equal(int64(0))) + }) + It("deletes runner-a-idle, keeps runner-b-busy, and advances applied actionable revision 3 to 4", func() { controller := &EphemeralRunnerSetReconciler{ Client: mgr.GetClient(), @@ -1698,7 +1751,7 @@ var _ = Describe("Test EphemeralRunnerSet actionable revision cleanup", func() { }, time.Second, ephemeralRunnerSetTestInterval).Should(Equal(int64(3))) }) - It("deletes idle runner and advances revision after restart with no cache", func() { + It("does not delete unregistered pending runner after restart with no cache", func() { controller := &EphemeralRunnerSetReconciler{ Client: mgr.GetClient(), Scheme: mgr.GetScheme(), @@ -1738,31 +1791,20 @@ var _ = Describe("Test EphemeralRunnerSet actionable revision cleanup", func() { err = k8sClient.Status().Patch(ctx, statusUpdated, client.MergeFrom(current)) Expect(err).NotTo(HaveOccurred()) - // Create idle runner (running but no job) - idleRunner := newRunner("runner-restart-idle", statusUpdated) - err = k8sClient.Create(ctx, idleRunner) - Expect(err).NotTo(HaveOccurred()) - - idleCurrent := new(v1alpha1.EphemeralRunner) - err = k8sClient.Get(ctx, client.ObjectKeyFromObject(idleRunner), idleCurrent) - Expect(err).NotTo(HaveOccurred()) - idleUpdated := idleCurrent.DeepCopy() - idleUpdated.Status.Phase = v1alpha1.EphemeralRunnerPhaseRunning - idleUpdated.Status.RunnerID = 201 - err = k8sClient.Status().Patch(ctx, idleUpdated, client.MergeFrom(idleCurrent)) + pendingRunner := newRunner("runner-restart-pending", statusUpdated) + err = k8sClient.Create(ctx, pendingRunner) Expect(err).NotTo(HaveOccurred()) // Reconcile with fresh cache (simulating restart) _, err = controller.Reconcile(ctx, request) Expect(err).NotTo(HaveOccurred()) - // Idle runner should be deleted - Eventually(func() bool { + Consistently(func() error { runner := new(v1alpha1.EphemeralRunner) - return kerrors.IsNotFound(k8sClient.Get(ctx, types.NamespacedName{Namespace: autoscalingNS.Name, Name: "runner-restart-idle"}, runner)) - }, ephemeralRunnerSetTestTimeout, ephemeralRunnerSetTestInterval).Should(BeTrue()) + return k8sClient.Get(ctx, types.NamespacedName{Namespace: autoscalingNS.Name, Name: "runner-restart-pending"}, runner) + }, time.Second, ephemeralRunnerSetTestInterval).Should(Succeed()) - // AppliedActionableRevision should advance to 4 + // AppliedActionableRevision should advance because there was nothing safe to clean up. Eventually(func() int64 { updatedSet := new(v1alpha1.EphemeralRunnerSet) if err := k8sClient.Get(ctx, request.NamespacedName, updatedSet); err != nil {