diff --git a/apis/actions.github.com/v1alpha1/autoscalingrunnerset_types.go b/apis/actions.github.com/v1alpha1/autoscalingrunnerset_types.go index dd1bce7fa1..b383c2c49d 100644 --- a/apis/actions.github.com/v1alpha1/autoscalingrunnerset_types.go +++ b/apis/actions.github.com/v1alpha1/autoscalingrunnerset_types.go @@ -319,6 +319,10 @@ type HistogramMetric struct { type AutoscalingRunnerSetStatus struct { // +optional Phase AutoscalingRunnerSetPhase `json:"phase"` + // ObservedGeneration tracks the metadata.generation of this ARS at observation time, + // enabling detection of Pending phase when generation differs. Unset defaults to 0. + // +optional + ObservedGeneration int64 `json:"observedGeneration,omitempty"` } type AutoscalingRunnerSetPhase string diff --git a/apis/actions.github.com/v1alpha1/ephemeralrunnerset_types.go b/apis/actions.github.com/v1alpha1/ephemeralrunnerset_types.go index 641ec02f88..22ed2dd9fe 100644 --- a/apis/actions.github.com/v1alpha1/ephemeralrunnerset_types.go +++ b/apis/actions.github.com/v1alpha1/ephemeralrunnerset_types.go @@ -36,12 +36,28 @@ type EphemeralRunnerSetSpec struct { // but does not apply to existing ephemeral runners. // +optional EphemeralRunnerMetadata *ResourceMeta `json:"ephemeralRunnerMetadata,omitempty"` + // ActionableRevision is a restart-safe applied marker that increments whenever + // Spec.EphemeralRunnerSpec changes, enabling detection of spec updates. + // Unset defaults to 0. + // +optional + ActionableRevision int64 `json:"actionableRevision,omitempty"` } // EphemeralRunnerSetStatus defines the observed state of EphemeralRunnerSet type EphemeralRunnerSetStatus struct { // +optional Phase EphemeralRunnerSetPhase `json:"phase"` + // AppliedActionableRevision is a restart-safe applied marker tracking the last successfully + // applied ActionableRevision value. Advances only after spec cleanup succeeds. + // Unset defaults to 0. + // +optional + AppliedActionableRevision int64 `json:"appliedActionableRevision,omitempty"` + // FinishedRunnerCleanupPatchID records the listener patch ID for which finished + // ephemeral runners were cleaned up. Scale-up is suppressed for the same patch ID + // until the listener publishes a fresh desired-state patch. + // Unset defaults to 0. + // +optional + FinishedRunnerCleanupPatchID int `json:"finishedRunnerCleanupPatchID,omitempty"` } // EphemeralRunnerSetPhase is the phase of the ephemeral runner set resource diff --git a/apis/actions.github.com/v1alpha1/version.go b/apis/actions.github.com/v1alpha1/version.go index 731c601151..4a3dc33284 100644 --- a/apis/actions.github.com/v1alpha1/version.go +++ b/apis/actions.github.com/v1alpha1/version.go @@ -3,7 +3,7 @@ package v1alpha1 import "strings" func IsVersionAllowed(resourceVersion, buildVersion string) bool { - if buildVersion == "dev" || resourceVersion == buildVersion || strings.HasPrefix(buildVersion, "canary-") { + if resourceVersion == buildVersion || buildVersion == "dev" || strings.HasPrefix(buildVersion, "canary-") { return true } diff --git a/charts/gha-runner-scale-set-controller-experimental/crds/actions.github.com_autoscalingrunnersets.yaml b/charts/gha-runner-scale-set-controller-experimental/crds/actions.github.com_autoscalingrunnersets.yaml index a2dfd46d9f..27c74aabee 100644 --- a/charts/gha-runner-scale-set-controller-experimental/crds/actions.github.com_autoscalingrunnersets.yaml +++ b/charts/gha-runner-scale-set-controller-experimental/crds/actions.github.com_autoscalingrunnersets.yaml @@ -16541,6 +16541,12 @@ spec: status: description: AutoscalingRunnerSetStatus defines the observed state of AutoscalingRunnerSet properties: + observedGeneration: + description: |- + ObservedGeneration tracks the metadata.generation of this ARS at observation time, + enabling detection of Pending phase when generation differs. Unset defaults to 0. + format: int64 + type: integer phase: type: string type: object diff --git a/charts/gha-runner-scale-set-controller-experimental/crds/actions.github.com_ephemeralrunnersets.yaml b/charts/gha-runner-scale-set-controller-experimental/crds/actions.github.com_ephemeralrunnersets.yaml index a6f9758bfa..f696ff054d 100644 --- a/charts/gha-runner-scale-set-controller-experimental/crds/actions.github.com_ephemeralrunnersets.yaml +++ b/charts/gha-runner-scale-set-controller-experimental/crds/actions.github.com_ephemeralrunnersets.yaml @@ -46,6 +46,13 @@ spec: spec: description: EphemeralRunnerSetSpec defines the desired state of EphemeralRunnerSet properties: + actionableRevision: + description: |- + ActionableRevision is a restart-safe applied marker that increments whenever + Spec.EphemeralRunnerSpec changes, enabling detection of spec updates. + Unset defaults to 0. + format: int64 + type: integer ephemeralRunnerMetadata: description: |- EphemeralRunnerMetadata is the metadata to be applied to all ephemeral runners created by this set. @@ -8295,6 +8302,20 @@ spec: status: description: EphemeralRunnerSetStatus defines the observed state of EphemeralRunnerSet properties: + appliedActionableRevision: + description: |- + AppliedActionableRevision is a restart-safe applied marker tracking the last successfully + applied ActionableRevision value. Advances only after spec cleanup succeeds. + Unset defaults to 0. + format: int64 + type: integer + finishedRunnerCleanupPatchID: + description: |- + FinishedRunnerCleanupPatchID records the listener patch ID for which finished + ephemeral runners were cleaned up. Scale-up is suppressed for the same patch ID + until the listener publishes a fresh desired-state patch. + Unset defaults to 0. + type: integer phase: description: EphemeralRunnerSetPhase is the phase of the ephemeral runner set resource type: string diff --git a/charts/gha-runner-scale-set-controller/crds/actions.github.com_autoscalingrunnersets.yaml b/charts/gha-runner-scale-set-controller/crds/actions.github.com_autoscalingrunnersets.yaml index a2dfd46d9f..27c74aabee 100644 --- a/charts/gha-runner-scale-set-controller/crds/actions.github.com_autoscalingrunnersets.yaml +++ b/charts/gha-runner-scale-set-controller/crds/actions.github.com_autoscalingrunnersets.yaml @@ -16541,6 +16541,12 @@ spec: status: description: AutoscalingRunnerSetStatus defines the observed state of AutoscalingRunnerSet properties: + observedGeneration: + description: |- + ObservedGeneration tracks the metadata.generation of this ARS at observation time, + enabling detection of Pending phase when generation differs. Unset defaults to 0. + format: int64 + type: integer phase: type: string type: object diff --git a/charts/gha-runner-scale-set-controller/crds/actions.github.com_ephemeralrunnersets.yaml b/charts/gha-runner-scale-set-controller/crds/actions.github.com_ephemeralrunnersets.yaml index a6f9758bfa..f696ff054d 100644 --- a/charts/gha-runner-scale-set-controller/crds/actions.github.com_ephemeralrunnersets.yaml +++ b/charts/gha-runner-scale-set-controller/crds/actions.github.com_ephemeralrunnersets.yaml @@ -46,6 +46,13 @@ spec: spec: description: EphemeralRunnerSetSpec defines the desired state of EphemeralRunnerSet properties: + actionableRevision: + description: |- + ActionableRevision is a restart-safe applied marker that increments whenever + Spec.EphemeralRunnerSpec changes, enabling detection of spec updates. + Unset defaults to 0. + format: int64 + type: integer ephemeralRunnerMetadata: description: |- EphemeralRunnerMetadata is the metadata to be applied to all ephemeral runners created by this set. @@ -8295,6 +8302,20 @@ spec: status: description: EphemeralRunnerSetStatus defines the observed state of EphemeralRunnerSet properties: + appliedActionableRevision: + description: |- + AppliedActionableRevision is a restart-safe applied marker tracking the last successfully + applied ActionableRevision value. Advances only after spec cleanup succeeds. + Unset defaults to 0. + format: int64 + type: integer + finishedRunnerCleanupPatchID: + description: |- + FinishedRunnerCleanupPatchID records the listener patch ID for which finished + ephemeral runners were cleaned up. Scale-up is suppressed for the same patch ID + until the listener publishes a fresh desired-state patch. + Unset defaults to 0. + type: integer phase: description: EphemeralRunnerSetPhase is the phase of the ephemeral runner set resource type: string diff --git a/cmd/ghalistener/scaler/scaler.go b/cmd/ghalistener/scaler/scaler.go index 51cb0362ae..662ac703f0 100644 --- a/cmd/ghalistener/scaler/scaler.go +++ b/cmd/ghalistener/scaler/scaler.go @@ -88,6 +88,7 @@ func (w *Scaler) applyDefaults() error { // It takes a context and a jobInfo parameter which contains the details of the started job. // This update marks the ephemeral runner so that the controller would have more context // about the ephemeral runner that should not be deleted when scaling down. +// It also transitions the phase to Running if the runner is not in a terminal state. // It returns an error if there is any issue with updating the job information. func (w *Scaler) HandleJobStarted(ctx context.Context, jobInfo *scaleset.JobStarted) error { w.logger.Info("Updating job info for the runner", @@ -102,23 +103,50 @@ func (w *Scaler) HandleJobStarted(ctx context.Context, jobInfo *scaleset.JobStar w.dirty = true + // Fetch current EphemeralRunner to check phase and deletion status + currentRunner := &v1alpha1.EphemeralRunner{} + err := w.clientset.RESTClient(). + Get(). + Prefix("apis", v1alpha1.GroupVersion.Group, v1alpha1.GroupVersion.Version). + Namespace(w.config.EphemeralRunnerSetNamespace). + Resource("EphemeralRunners"). + Name(jobInfo.RunnerName). + Do(ctx). + Into(currentRunner) + if err != nil { + if kerrors.IsNotFound(err) { + w.logger.Info("Ephemeral runner not found, skipping job info update", "runnerName", jobInfo.RunnerName) + return nil + } + return fmt.Errorf("failed to get ephemeral runner: %w", err) + } + original, err := json.Marshal(&v1alpha1.EphemeralRunner{}) if err != nil { return fmt.Errorf("failed to marshal empty ephemeral runner: %w", err) } - patch, err := json.Marshal( - &v1alpha1.EphemeralRunner{ - Status: v1alpha1.EphemeralRunnerStatus{ - JobRequestID: jobInfo.RunnerRequestID, - JobRepositoryName: fmt.Sprintf("%s/%s", jobInfo.OwnerName, jobInfo.RepositoryName), - JobID: jobInfo.JobID, - WorkflowRunID: jobInfo.WorkflowRunID, - JobWorkflowRef: jobInfo.JobWorkflowRef, - JobDisplayName: jobInfo.JobDisplayName, - }, + // Build patch with job fields + patchRunner := &v1alpha1.EphemeralRunner{ + Status: v1alpha1.EphemeralRunnerStatus{ + JobRequestID: jobInfo.RunnerRequestID, + JobRepositoryName: fmt.Sprintf("%s/%s", jobInfo.OwnerName, jobInfo.RepositoryName), + JobID: jobInfo.JobID, + WorkflowRunID: jobInfo.WorkflowRunID, + JobWorkflowRef: jobInfo.JobWorkflowRef, + JobDisplayName: jobInfo.JobDisplayName, }, - ) + } + + // Only set Running phase if current phase is not terminal/failure and deletion is not in progress + if currentRunner.DeletionTimestamp == nil && + currentRunner.Status.Phase != v1alpha1.EphemeralRunnerPhaseFailed && + currentRunner.Status.Phase != v1alpha1.EphemeralRunnerPhaseSucceeded && + currentRunner.Status.Phase != v1alpha1.EphemeralRunnerPhaseOutdated { + patchRunner.Status.Phase = v1alpha1.EphemeralRunnerPhaseRunning + } + + patch, err := json.Marshal(patchRunner) if err != nil { return fmt.Errorf("failed to marshal ephemeral runner patch: %w", err) } diff --git a/cmd/ghalistener/scaler/scaler_test.go b/cmd/ghalistener/scaler/scaler_test.go index 7ea3e967d9..bada44fdbc 100644 --- a/cmd/ghalistener/scaler/scaler_test.go +++ b/cmd/ghalistener/scaler/scaler_test.go @@ -1,15 +1,161 @@ package scaler import ( + "context" + "encoding/json" "log/slog" "math" + "net/http" + "net/http/httptest" "testing" + "github.com/actions/actions-runner-controller/apis/actions.github.com/v1alpha1" + "github.com/actions/scaleset" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes" + "k8s.io/client-go/rest" ) var discardLogger = slog.New(slog.DiscardHandler) +func TestHandleJobStarted(t *testing.T) { + jobInfo := &scaleset.JobStarted{ + RunnerName: "runner-1", + JobMessageBase: scaleset.JobMessageBase{ + OwnerName: "actions", + RepositoryName: "actions-runner-controller", + JobID: "job-1", + WorkflowRunID: 456, + JobWorkflowRef: "actions/actions-runner-controller/.github/workflows/ci.yaml@refs/heads/main", + JobDisplayName: "build", + RunnerRequestID: 123, + }, + } + + t.Run("patches job fields and running phase together", func(t *testing.T) { + runner := newTestEphemeralRunner(jobInfo.RunnerName, "") + scaler, shutdown := newTestScaler(t, runner) + defer shutdown() + + require.NoError(t, scaler.HandleJobStarted(context.Background(), jobInfo)) + + assertJobStartedStatus(t, runner, jobInfo) + assert.Equal(t, v1alpha1.EphemeralRunnerPhaseRunning, runner.Status.Phase) + }) + + t.Run("repeated assignment remains idempotent", func(t *testing.T) { + runner := newTestEphemeralRunner(jobInfo.RunnerName, v1alpha1.EphemeralRunnerPhaseRunning) + scaler, shutdown := newTestScaler(t, runner) + defer shutdown() + + require.NoError(t, scaler.HandleJobStarted(context.Background(), jobInfo)) + firstStatus := runner.Status + require.NoError(t, scaler.HandleJobStarted(context.Background(), jobInfo)) + + assert.Equal(t, firstStatus, runner.Status) + assertJobStartedStatus(t, runner, jobInfo) + assert.Equal(t, v1alpha1.EphemeralRunnerPhaseRunning, runner.Status.Phase) + }) + + for _, phase := range []v1alpha1.EphemeralRunnerPhase{ + v1alpha1.EphemeralRunnerPhaseFailed, + v1alpha1.EphemeralRunnerPhaseSucceeded, + v1alpha1.EphemeralRunnerPhaseOutdated, + } { + t.Run("preserves "+string(phase)+" phase while patching job fields", func(t *testing.T) { + runner := newTestEphemeralRunner(jobInfo.RunnerName, phase) + scaler, shutdown := newTestScaler(t, runner) + defer shutdown() + + require.NoError(t, scaler.HandleJobStarted(context.Background(), jobInfo)) + + assertJobStartedStatus(t, runner, jobInfo) + assert.Equal(t, phase, runner.Status.Phase) + }) + } + + t.Run("preserves deleting runner phase while patching job fields", func(t *testing.T) { + runner := newTestEphemeralRunner(jobInfo.RunnerName, v1alpha1.EphemeralRunnerPhasePending) + deletionTimestamp := metav1.Now() + runner.DeletionTimestamp = &deletionTimestamp + scaler, shutdown := newTestScaler(t, runner) + defer shutdown() + + require.NoError(t, scaler.HandleJobStarted(context.Background(), jobInfo)) + + assertJobStartedStatus(t, runner, jobInfo) + assert.Equal(t, v1alpha1.EphemeralRunnerPhasePending, runner.Status.Phase) + }) +} + +func newTestEphemeralRunner(name string, phase v1alpha1.EphemeralRunnerPhase) *v1alpha1.EphemeralRunner { + return &v1alpha1.EphemeralRunner{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: "default", + }, + Status: v1alpha1.EphemeralRunnerStatus{ + Phase: phase, + }, + } +} + +func newTestScaler(t *testing.T, runner *v1alpha1.EphemeralRunner) (*Scaler, func()) { + t.Helper() + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + + switch r.Method { + case http.MethodGet: + require.NoError(t, json.NewEncoder(w).Encode(runner)) + case http.MethodPatch: + var patch v1alpha1.EphemeralRunner + require.NoError(t, json.NewDecoder(r.Body).Decode(&patch)) + + runner.Status.JobRequestID = patch.Status.JobRequestID + runner.Status.JobRepositoryName = patch.Status.JobRepositoryName + runner.Status.JobID = patch.Status.JobID + runner.Status.WorkflowRunID = patch.Status.WorkflowRunID + runner.Status.JobWorkflowRef = patch.Status.JobWorkflowRef + runner.Status.JobDisplayName = patch.Status.JobDisplayName + if patch.Status.Phase != "" { + runner.Status.Phase = patch.Status.Phase + } + + require.NoError(t, json.NewEncoder(w).Encode(runner)) + default: + http.Error(w, "unexpected method", http.StatusMethodNotAllowed) + } + })) + + clientset, err := kubernetes.NewForConfig(&rest.Config{Host: server.URL}) + require.NoError(t, err) + + return &Scaler{ + clientset: clientset, + config: Config{ + EphemeralRunnerSetNamespace: runner.Namespace, + }, + targetRunners: -1, + patchSeq: -1, + logger: discardLogger, + }, server.Close +} + +func assertJobStartedStatus(t *testing.T, runner *v1alpha1.EphemeralRunner, jobInfo *scaleset.JobStarted) { + t.Helper() + + assert.Equal(t, jobInfo.RunnerRequestID, runner.Status.JobRequestID) + assert.Equal(t, jobInfo.JobID, runner.Status.JobID) + assert.Equal(t, jobInfo.OwnerName+"/"+jobInfo.RepositoryName, runner.Status.JobRepositoryName) + assert.Equal(t, jobInfo.WorkflowRunID, runner.Status.WorkflowRunID) + assert.Equal(t, jobInfo.JobWorkflowRef, runner.Status.JobWorkflowRef) + assert.Equal(t, jobInfo.JobDisplayName, runner.Status.JobDisplayName) +} + func TestSetDesiredWorkerState_MinMaxDefaults(t *testing.T) { newEmptyWorker := func() *Scaler { return &Scaler{ diff --git a/config/crd/bases/actions.github.com_autoscalingrunnersets.yaml b/config/crd/bases/actions.github.com_autoscalingrunnersets.yaml index a2dfd46d9f..27c74aabee 100644 --- a/config/crd/bases/actions.github.com_autoscalingrunnersets.yaml +++ b/config/crd/bases/actions.github.com_autoscalingrunnersets.yaml @@ -16541,6 +16541,12 @@ spec: status: description: AutoscalingRunnerSetStatus defines the observed state of AutoscalingRunnerSet properties: + observedGeneration: + description: |- + ObservedGeneration tracks the metadata.generation of this ARS at observation time, + enabling detection of Pending phase when generation differs. Unset defaults to 0. + format: int64 + type: integer phase: type: string type: object diff --git a/config/crd/bases/actions.github.com_ephemeralrunnersets.yaml b/config/crd/bases/actions.github.com_ephemeralrunnersets.yaml index a6f9758bfa..f696ff054d 100644 --- a/config/crd/bases/actions.github.com_ephemeralrunnersets.yaml +++ b/config/crd/bases/actions.github.com_ephemeralrunnersets.yaml @@ -46,6 +46,13 @@ spec: spec: description: EphemeralRunnerSetSpec defines the desired state of EphemeralRunnerSet properties: + actionableRevision: + description: |- + ActionableRevision is a restart-safe applied marker that increments whenever + Spec.EphemeralRunnerSpec changes, enabling detection of spec updates. + Unset defaults to 0. + format: int64 + type: integer ephemeralRunnerMetadata: description: |- EphemeralRunnerMetadata is the metadata to be applied to all ephemeral runners created by this set. @@ -8295,6 +8302,20 @@ spec: status: description: EphemeralRunnerSetStatus defines the observed state of EphemeralRunnerSet properties: + appliedActionableRevision: + description: |- + AppliedActionableRevision is a restart-safe applied marker tracking the last successfully + applied ActionableRevision value. Advances only after spec cleanup succeeds. + Unset defaults to 0. + format: int64 + type: integer + finishedRunnerCleanupPatchID: + description: |- + FinishedRunnerCleanupPatchID records the listener patch ID for which finished + ephemeral runners were cleaned up. Scale-up is suppressed for the same patch ID + until the listener publishes a fresh desired-state patch. + Unset defaults to 0. + type: integer phase: description: EphemeralRunnerSetPhase is the phase of the ephemeral runner set resource type: string diff --git a/controllers/actions.github.com/autoscalinglistener_controller.go b/controllers/actions.github.com/autoscalinglistener_controller.go index c0aa81114c..3dc1c63c7b 100644 --- a/controllers/actions.github.com/autoscalinglistener_controller.go +++ b/controllers/actions.github.com/autoscalinglistener_controller.go @@ -19,11 +19,10 @@ package actionsgithubcom import ( "context" "fmt" - "maps" - "reflect" "time" "github.com/go-logr/logr" + "github.com/google/go-cmp/cmp" kerrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" @@ -78,7 +77,6 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. if err := r.Get(ctx, req.NamespacedName, &autoscalingListener); err != nil { return ctrl.Result{}, client.IgnoreNotFound(err) } - original := autoscalingListener.DeepCopy() if !autoscalingListener.DeletionTimestamp.IsZero() { if !controllerutil.ContainsFinalizer(&autoscalingListener, autoscalingListenerFinalizerName) { @@ -93,29 +91,40 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. } if requeue { log.Info("Waiting for resources to be deleted before removing finalizer") - return ctrl.Result{Requeue: true, RequeueAfter: time.Second}, nil + return ctrl.Result{RequeueAfter: time.Second}, nil } - log.Info("Removing finalizer") - if controllerutil.RemoveFinalizer(&autoscalingListener, autoscalingListenerFinalizerName) { - if err := r.Patch(ctx, &autoscalingListener, client.MergeFrom(original)); err != nil && !kerrors.IsNotFound(err) { + original := newOnce(autoscalingListener.DeepCopy) + if controllerutil.ContainsFinalizer(&autoscalingListener, autoscalingListenerFinalizerName) { + original.Do() + controllerutil.RemoveFinalizer(&autoscalingListener, autoscalingListenerFinalizerName) + } + if original.Called() { + log.Info("Removing finalizer") + if err := r.Patch(ctx, &autoscalingListener, client.MergeFrom(original.Get())); err != nil && !kerrors.IsNotFound(err) { log.Error(err, "Failed to remove finalizer") return ctrl.Result{}, err } } log.Info("Successfully removed finalizer after cleanup") + r.ResourceCache.Delete(&autoscalingListener) return ctrl.Result{}, nil } - if controllerutil.AddFinalizer(&autoscalingListener, autoscalingListenerFinalizerName) { - if err := r.Patch(ctx, &autoscalingListener, client.MergeFrom(original)); err != nil { + original := newOnce(autoscalingListener.DeepCopy) + addFinalizer := !controllerutil.ContainsFinalizer(&autoscalingListener, autoscalingListenerFinalizerName) + if addFinalizer { + original.Do() + controllerutil.AddFinalizer(&autoscalingListener, autoscalingListenerFinalizerName) + } + if original.Called() { + if err := r.Patch(ctx, &autoscalingListener, client.MergeFrom(original.Get())); err != nil { log.Error(err, "Failed to add finalizer") return ctrl.Result{}, err } log.Info("Successfully added finalizer") - return ctrl.Result{}, nil } // Check if the AutoscalingRunnerSet exists @@ -163,26 +172,40 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. return ctrl.Result{}, err } - desiredLabels := r.filterAndMergeLabels(serviceAccount.Labels, desiredServiceAccount.Labels) - labelsModified := !maps.Equal(serviceAccount.Labels, desiredLabels) - desiredAnnotations := r.mergeAnnotations(serviceAccount.Annotations, desiredServiceAccount.Annotations) - annotationsModified := !maps.Equal(serviceAccount.Annotations, desiredAnnotations) - if labelsModified || annotationsModified { - updatedServiceAccount := serviceAccount.DeepCopy() - if labelsModified { - updatedServiceAccount.Labels = desiredLabels - } - if annotationsModified { - updatedServiceAccount.Annotations = desiredAnnotations - } + desiredLabels, labelsModified := r.mergeLabels(serviceAccount.Labels, desiredServiceAccount.Labels) + original := newOnce(serviceAccount.DeepCopy) + if labelsModified { + original.Do() + serviceAccount.Labels = desiredLabels + } + desiredAnnotations, annotationsModified := r.mergeAnnotations(serviceAccount.Annotations, desiredServiceAccount.Annotations) + if annotationsModified { + original.Do() + serviceAccount.Annotations = desiredAnnotations + } + secretsModified := !cmp.Equal(serviceAccount.Secrets, desiredServiceAccount.Secrets) + if secretsModified { + original.Do() + serviceAccount.Secrets = desiredServiceAccount.Secrets + } + imagePullSecretsModified := !cmp.Equal(serviceAccount.ImagePullSecrets, desiredServiceAccount.ImagePullSecrets) + if imagePullSecretsModified { + original.Do() + serviceAccount.ImagePullSecrets = desiredServiceAccount.ImagePullSecrets + } + automountServiceAccountTokenModified := !cmp.Equal(serviceAccount.AutomountServiceAccountToken, desiredServiceAccount.AutomountServiceAccountToken) + if automountServiceAccountTokenModified { + original.Do() + serviceAccount.AutomountServiceAccountToken = desiredServiceAccount.AutomountServiceAccountToken + } + + if original.Called() { log.Info("Updating listener service account") - if err := r.Patch(ctx, updatedServiceAccount, client.MergeFrom(&serviceAccount)); err != nil { + if err := r.Patch(ctx, &serviceAccount, client.MergeFrom(original.Get())); err != nil { log.Error(err, "Failed to update listener service account") return ctrl.Result{}, err } - - return ctrl.Result{Requeue: true}, nil } case kerrors.IsNotFound(err): // Create a service account for the listener pod in the controller namespace @@ -205,29 +228,29 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. ) switch { case err == nil: + original := newOnce(listenerRole.DeepCopy) desiredRole := r.newScaleSetListenerRole(&autoscalingListener) - desiredLabels := r.filterAndMergeLabels(listenerRole.Labels, desiredRole.Labels) - labelsModified := !maps.Equal(listenerRole.Labels, desiredLabels) - desiredAnnotations := r.mergeAnnotations(listenerRole.Annotations, desiredRole.Annotations) - annotationsModified := !maps.Equal(listenerRole.Annotations, desiredAnnotations) - rulesModified := !reflect.DeepEqual(listenerRole.Rules, desiredRole.Rules) - if labelsModified || annotationsModified || rulesModified { - updatedRole := listenerRole.DeepCopy() - if labelsModified { - updatedRole.Labels = desiredLabels - } - if annotationsModified { - updatedRole.Annotations = desiredAnnotations - } - if rulesModified { - updatedRole.Rules = desiredRole.Rules - } + desiredLabels, labelsModified := r.mergeLabels(listenerRole.Labels, desiredRole.Labels) + if labelsModified { + original.Do() + listenerRole.Labels = desiredLabels + } + desiredAnnotations, annotationsModified := r.mergeAnnotations(listenerRole.Annotations, desiredRole.Annotations) + if annotationsModified { + original.Do() + listenerRole.Annotations = desiredAnnotations + } + rulesModified := !cmp.Equal(listenerRole.Rules, desiredRole.Rules) + if rulesModified { + original.Do() + listenerRole.Rules = desiredRole.Rules + } + if original.Called() { log.Info("Updating listener role") - if err := r.Patch(ctx, updatedRole, client.MergeFrom(&listenerRole)); err != nil { + if err := r.Patch(ctx, &listenerRole, client.MergeFrom(original.Get())); err != nil { log.Error(err, "Failed to update listener role") return ctrl.Result{}, err } - return ctrl.Result{Requeue: true}, nil } case kerrors.IsNotFound(err): // Create a role for the listener pod in the AutoScalingRunnerSet namespace @@ -243,33 +266,42 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. err = r.Get(ctx, types.NamespacedName{Namespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace, Name: autoscalingListener.Name}, &listenerRoleBinding) switch { case err == nil: + original := newOnce(listenerRoleBinding.DeepCopy) desiredRoleBinding := r.newScaleSetListenerRoleBinding( &autoscalingListener, &listenerRole, &serviceAccount, ) - desiredLabels := r.filterAndMergeLabels(listenerRoleBinding.Labels, desiredRoleBinding.Labels) - labelsModified := !maps.Equal(listenerRoleBinding.Labels, desiredLabels) - desiredAnnotations := r.mergeAnnotations(listenerRoleBinding.Annotations, desiredRoleBinding.Annotations) - annotationsModified := !maps.Equal(listenerRoleBinding.Annotations, desiredAnnotations) - if labelsModified || annotationsModified { - updatedRoleBinding := listenerRoleBinding.DeepCopy() - if labelsModified { - updatedRoleBinding.Labels = desiredLabels - } - if annotationsModified { - updatedRoleBinding.Annotations = desiredAnnotations - } + desiredLabels, labelsModified := r.mergeLabels(listenerRoleBinding.Labels, desiredRoleBinding.Labels) + if labelsModified { + original.Do() + listenerRoleBinding.Labels = desiredLabels + } + + desiredAnnotations, annotationsModified := r.mergeAnnotations(listenerRoleBinding.Annotations, desiredRoleBinding.Annotations) + if annotationsModified { + original.Do() + listenerRoleBinding.Annotations = desiredAnnotations + } + rulesModified := !cmp.Equal(listenerRoleBinding.RoleRef, desiredRoleBinding.RoleRef) + if rulesModified { + original.Do() + listenerRoleBinding.RoleRef = desiredRoleBinding.RoleRef + } + + subjectsModified := !cmp.Equal(listenerRoleBinding.Subjects, desiredRoleBinding.Subjects) + if subjectsModified { + original.Do() + listenerRoleBinding.Subjects = desiredRoleBinding.Subjects + } + + if original.Called() { log.Info("Updating listener role binding") - if err := r.Patch(ctx, updatedRoleBinding, client.MergeFrom(&listenerRoleBinding)); err != nil { + if err := r.Patch(ctx, &listenerRoleBinding, client.MergeFrom(original.Get())); err != nil { log.Error(err, "Failed to update listener role binding") return ctrl.Result{}, err } - - log.Info("Updated listener role binding") - return ctrl.Result{Requeue: true}, nil } - case kerrors.IsNotFound(err): // Create a role binding for the listener pod in the AutoScalingRunnerSet namespace log.Info("Creating a role binding for the service account and role") @@ -298,29 +330,47 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. ) switch { case err == nil: + original := newOnce(proxySecret.DeepCopy) desiredListenerProxy, err := r.newAutoscalingListenerProxySecret(&autoscalingListener, proxySecret.Data) if err != nil { log.Error(err, "Failed to build desired listener proxy secret") return ctrl.Result{}, err } - desiredLabels := r.filterAndMergeLabels(proxySecret.Labels, desiredListenerProxy.Labels) - labelsModified := !maps.Equal(proxySecret.Labels, desiredLabels) - desiredAnnotations := r.mergeAnnotations(proxySecret.Annotations, desiredListenerProxy.Annotations) - annotationsModified := !maps.Equal(proxySecret.Annotations, desiredAnnotations) - if labelsModified || annotationsModified { - updatedProxySecret := proxySecret.DeepCopy() - if labelsModified { - updatedProxySecret.Labels = desiredLabels - } - if annotationsModified { - updatedProxySecret.Annotations = desiredAnnotations - } + desiredLabels, labelsModified := r.mergeLabels(proxySecret.Labels, desiredListenerProxy.Labels) + if labelsModified { + original.Do() + proxySecret.Labels = desiredLabels + } + desiredAnnotations, annotationsModified := r.mergeAnnotations(proxySecret.Annotations, desiredListenerProxy.Annotations) + if annotationsModified { + original.Do() + proxySecret.Annotations = desiredAnnotations + } + // we set the data so we just need to check other fields are nil + if proxySecret.Immutable != nil { + original.Do() + proxySecret.Immutable = nil + } + if proxySecret.StringData != nil { + original.Do() + proxySecret.StringData = nil + } + if proxySecret.Type != desiredListenerProxy.Type { + original.Do() + proxySecret.Type = desiredListenerProxy.Type + } + dataModified := !cmp.Equal(proxySecret.Data, desiredListenerProxy.Data) + if dataModified { + original.Do() + proxySecret.Data = desiredListenerProxy.Data + } + + if original.Called() { log.Info("Updating listener proxy secret") - if err := r.Patch(ctx, updatedProxySecret, client.MergeFrom(&proxySecret)); err != nil { + if err := r.Patch(ctx, &proxySecret, client.MergeFrom(original.Get())); err != nil { log.Error(err, "Failed to update listener proxy secret") return ctrl.Result{}, err } - return ctrl.Result{Requeue: true}, nil } case kerrors.IsNotFound(err): // Create a mirror secret for the listener pod in the Controller namespace for listener pod to use @@ -343,10 +393,8 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. log.Error( err, "Failed to get app config for AutoscalingRunnerSet.", - "namespace", - autoscalingRunnerSet.Namespace, - "name", - autoscalingRunnerSet.GitHubConfigSecret, + "namespace", autoscalingRunnerSet.Namespace, + "name", autoscalingRunnerSet.GitHubConfigSecret, ) return nil, err } @@ -374,6 +422,7 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. ) switch { case err == nil: + original := newOnce(listenerConfigSecret.DeepCopy) cfg, err := r.GetAppConfig(ctx, &autoscalingRunnerSet) if err != nil { return ctrl.Result{}, err @@ -389,21 +438,33 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. if err != nil { return ctrl.Result{}, fmt.Errorf("failed to build listener config secret: %w", err) } - desiredLabels := r.filterAndMergeLabels(listenerConfigSecret.Labels, desiredSecret.Labels) - labelsModified := !maps.Equal(listenerConfigSecret.Labels, desiredLabels) - desiredAnnotations := r.mergeAnnotations(listenerConfigSecret.Annotations, desiredSecret.Annotations) - annotationsModified := !maps.Equal(listenerConfigSecret.Annotations, desiredAnnotations) + desiredLabels, labelsModified := r.mergeLabels(listenerConfigSecret.Labels, desiredSecret.Labels) + if labelsModified { + original.Do() + listenerConfigSecret.Labels = desiredLabels + } + desiredAnnotations, annotationsModified := r.mergeAnnotations(listenerConfigSecret.Annotations, desiredSecret.Annotations) + if annotationsModified { + original.Do() + listenerConfigSecret.Annotations = desiredAnnotations + } + // we set the data so we just need to check other fields are nil + if listenerConfigSecret.Immutable != nil { + original.Do() + listenerConfigSecret.Immutable = nil + } + if listenerConfigSecret.StringData != nil { + original.Do() + listenerConfigSecret.StringData = nil + } + if listenerConfigSecret.Type != desiredSecret.Type { + original.Do() + listenerConfigSecret.Type = desiredSecret.Type + } - if labelsModified || annotationsModified { - updatedSecret := listenerConfigSecret.DeepCopy() - if labelsModified { - updatedSecret.Labels = desiredLabels - } - if annotationsModified { - updatedSecret.Annotations = desiredAnnotations - } - log.Info("Updating listener config secret", "namespace", updatedSecret.Namespace, "name", updatedSecret.Name) - if err := r.Patch(ctx, updatedSecret, client.MergeFrom(&listenerConfigSecret)); err != nil { + if original.Called() { + log.Info("Updating listener config secret", "namespace", listenerConfigSecret.Namespace, "name", listenerConfigSecret.Name) + if err := r.Patch(ctx, &listenerConfigSecret, client.MergeFrom(original.Get())); err != nil { return ctrl.Result{}, fmt.Errorf("failed to update listener config secret: %w", err) } return ctrl.Result{Requeue: true}, nil @@ -432,7 +493,7 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. } // Requeue to create listener pod with the config secret - return ctrl.Result{Requeue: true}, nil + return ctrl.Result{RequeueAfter: 100 * time.Millisecond}, nil default: log.Error(err, "Unable to get listener config secret", "namespace", autoscalingListener.Namespace, "name", scaleSetListenerConfigName(&autoscalingListener)) return ctrl.Result{}, err @@ -462,7 +523,19 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. return ctrl.Result{}, err } - shouldReCreate := desiredPod.Annotations[annotationKeyIntegrityHash] != listenerPod.Annotations[annotationKeyIntegrityHash] + original := newOnce(listenerPod.DeepCopy) + desiredLabels, labelsModified := r.mergeLabels(listenerPod.Labels, desiredPod.Labels) + if labelsModified { + original.Do() + listenerPod.Labels = desiredLabels + } + desiredAnnotations, annotationsModified := r.mergeAnnotations(listenerPod.Annotations, desiredPod.Annotations) + if annotationsModified { + original.Do() + listenerPod.Annotations = desiredAnnotations + } + + shouldReCreate := listenerPodSpecRequiresRecreation(&listenerPod, desiredPod) if shouldReCreate { log.Info("Listener pod dependency changed, recreating listener pod") if err := r.deleteListenerPod(ctx, &autoscalingListener, &listenerPod, log); err != nil { @@ -473,22 +546,10 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. return ctrl.Result{}, nil } - desiredLabels := r.filterAndMergeLabels(listenerPod.Labels, desiredPod.Labels) - labelsModified := !maps.Equal(listenerPod.Labels, desiredLabels) - desiredAnnotations := r.mergeAnnotations(listenerPod.Annotations, desiredPod.Annotations) - annotationsModified := !maps.Equal(listenerPod.Annotations, desiredAnnotations) - if labelsModified || annotationsModified { - updatedPod := listenerPod.DeepCopy() - if labelsModified { - updatedPod.Labels = desiredLabels - } - if annotationsModified { - updatedPod.Annotations = desiredAnnotations - } - log.Info("Updating listener pod", "namespace", updatedPod.Namespace, "name", updatedPod.Name) - if err := r.Patch(ctx, updatedPod, client.MergeFrom(&listenerPod)); err != nil { - log.Error(err, "Unable to update listener pod", "namespace", updatedPod.Namespace, "name", updatedPod.Name) + log.Info("Updating listener pod", "namespace", listenerPod.Namespace, "name", listenerPod.Name) + if err := r.Patch(ctx, &listenerPod, client.MergeFrom(original.Get())); err != nil { + log.Error(err, "Unable to update listener pod", "namespace", listenerPod.Namespace, "name", listenerPod.Name) return ctrl.Result{}, err } return ctrl.Result{}, nil @@ -500,6 +561,7 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. return ctrl.Result{}, nil } + r.ResourceCache.listenerPod.Delete(&autoscalingListener) desiredPod, err := r.newScaleSetListenerPod( &autoscalingListener, &listenerConfigSecret, @@ -685,6 +747,7 @@ func (r *AutoscalingListenerReconciler) cleanupResources(ctx context.Context, au } func (r *AutoscalingListenerReconciler) createServiceAccountForListener(ctx context.Context, autoscalingListener *v1alpha1.AutoscalingListener, logger logr.Logger) (ctrl.Result, error) { + r.ResourceCache.listenerServiceAccount.Delete(autoscalingListener) newServiceAccount, err := r.newScaleSetListenerServiceAccount(autoscalingListener) if err != nil { return ctrl.Result{}, err @@ -768,6 +831,7 @@ func (r *AutoscalingListenerReconciler) createProxySecret(ctx context.Context, a } func (r *AutoscalingListenerReconciler) createRoleForListener(ctx context.Context, autoscalingListener *v1alpha1.AutoscalingListener, logger logr.Logger) (ctrl.Result, error) { + r.ResourceCache.listenerRole.Delete(autoscalingListener) newRole := r.newScaleSetListenerRole(autoscalingListener) logger.Info("Creating listener role", "namespace", newRole.Namespace, "name", newRole.Name, "rules", newRole.Rules) @@ -781,6 +845,7 @@ func (r *AutoscalingListenerReconciler) createRoleForListener(ctx context.Contex } func (r *AutoscalingListenerReconciler) createRoleBindingForListener(ctx context.Context, autoscalingListener *v1alpha1.AutoscalingListener, listenerRole *rbacv1.Role, serviceAccount *corev1.ServiceAccount, logger logr.Logger) (ctrl.Result, error) { + r.ResourceCache.listenerRoleBinding.Delete(autoscalingListener) newRoleBinding := r.newScaleSetListenerRoleBinding(autoscalingListener, listenerRole, serviceAccount) logger.Info("Creating listener role binding", diff --git a/controllers/actions.github.com/autoscalinglistener_controller_test.go b/controllers/actions.github.com/autoscalinglistener_controller_test.go index d48d613e6c..67f7ecf5e7 100644 --- a/controllers/actions.github.com/autoscalinglistener_controller_test.go +++ b/controllers/actions.github.com/autoscalinglistener_controller_test.go @@ -38,6 +38,7 @@ var _ = Describe("Test AutoScalingListener controller", func() { var autoscalingRunnerSet *v1alpha1.AutoscalingRunnerSet var configSecret *corev1.Secret var autoscalingListener *v1alpha1.AutoscalingListener + var resourceCache *ResourceCache BeforeEach(func() { ctx = context.Background() @@ -49,7 +50,9 @@ var _ = Describe("Test AutoScalingListener controller", func() { scalefake.NewMultiClient(), ) + resourceCache = newTestResourceCache() rb := ResourceBuilder{ + ResourceCache: resourceCache, SecretResolver: secretResolver, } @@ -230,6 +233,17 @@ var _ = Describe("Test AutoScalingListener controller", func() { autoscalingListenerTestTimeout, autoscalingListenerTestInterval, ).Should(BeEquivalentTo(autoscalingListener.Name), "Pod should be created") + + Eventually( + func() bool { + return resourceCacheStateHasMainObjectEntries(resourceCache.listenerServiceAccount, created) && + resourceCacheStateHasMainObjectEntries(resourceCache.listenerRole, created) && + resourceCacheStateHasMainObjectEntries(resourceCache.listenerRoleBinding, created) && + resourceCacheStateHasMainObjectEntries(resourceCache.listenerPod, created) + }, + autoscalingListenerTestTimeout, + autoscalingListenerTestInterval, + ).Should(BeTrue(), "AutoScalingListener service account, role, role binding, and pod resources should be cached after reconciliation") }) }) @@ -250,8 +264,22 @@ var _ = Describe("Test AutoScalingListener controller", func() { autoscalingListenerTestInterval, ).Should(BeEquivalentTo(autoscalingListener.Name), "Pod should be created") + created := new(v1alpha1.AutoscalingListener) + err := k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingListener.Name, Namespace: autoscalingListener.Namespace}, created) + Expect(err).NotTo(HaveOccurred(), "failed to get AutoScalingListener") + Eventually( + func() bool { + return resourceCacheStateHasMainObjectEntries(resourceCache.listenerServiceAccount, created) && + resourceCacheStateHasMainObjectEntries(resourceCache.listenerRole, created) && + resourceCacheStateHasMainObjectEntries(resourceCache.listenerRoleBinding, created) && + resourceCacheStateHasMainObjectEntries(resourceCache.listenerPod, created) + }, + autoscalingListenerTestTimeout, + autoscalingListenerTestInterval, + ).Should(BeTrue(), "AutoScalingListener service account, role, role binding, and pod resources should be cached before deletion") + // Delete the AutoScalingListener - err := k8sClient.Delete(ctx, autoscalingListener) + err = k8sClient.Delete(ctx, autoscalingListener) Expect(err).NotTo(HaveOccurred(), "failed to delete test AutoScalingListener") // Cleanup the listener pod @@ -342,6 +370,17 @@ var _ = Describe("Test AutoScalingListener controller", func() { autoscalingListenerTestTimeout, autoscalingListenerTestInterval, ).ShouldNot(Succeed(), "failed to delete AutoScalingListener") + + Eventually( + func() bool { + return resourceCacheStateHasMainObjectEntries(resourceCache.listenerServiceAccount, created) || + resourceCacheStateHasMainObjectEntries(resourceCache.listenerRole, created) || + resourceCacheStateHasMainObjectEntries(resourceCache.listenerRoleBinding, created) || + resourceCacheStateHasMainObjectEntries(resourceCache.listenerPod, created) + }, + autoscalingListenerTestTimeout, + autoscalingListenerTestInterval, + ).Should(BeFalse(), "AutoScalingListener service account, role, role binding, and pod resources should be removed from cache after deletion") }) }) @@ -593,6 +632,7 @@ var _ = Describe("Test AutoScalingListener customization", func() { secretResolver := secretresolver.New(mgr.GetClient(), scalefake.NewMultiClient()) rb := ResourceBuilder{ + ResourceCache: newTestResourceCache(), SecretResolver: secretResolver, } @@ -922,6 +962,7 @@ var _ = Describe("Test AutoScalingListener controller with proxy", func() { secretResolver := secretresolver.New(mgr.GetClient(), scalefake.NewMultiClient()) rb := ResourceBuilder{ + ResourceCache: newTestResourceCache(), SecretResolver: secretResolver, } @@ -1127,6 +1168,7 @@ var _ = Describe("Test AutoScalingListener controller with template modification secretResolver := secretresolver.New(mgr.GetClient(), scalefake.NewMultiClient()) rb := ResourceBuilder{ + ResourceCache: newTestResourceCache(), SecretResolver: secretResolver, } @@ -1232,6 +1274,7 @@ var _ = Describe("Test GitHub Server TLS configuration", func() { secretResolver := secretresolver.New(mgr.GetClient(), scalefake.NewMultiClient()) rb := ResourceBuilder{ + ResourceCache: newTestResourceCache(), SecretResolver: secretResolver, } diff --git a/controllers/actions.github.com/autoscalingrunnerset_controller.go b/controllers/actions.github.com/autoscalingrunnerset_controller.go index e48f75ff15..f453989a4e 100644 --- a/controllers/actions.github.com/autoscalingrunnerset_controller.go +++ b/controllers/actions.github.com/autoscalingrunnerset_controller.go @@ -19,7 +19,7 @@ package actionsgithubcom import ( "context" "fmt" - "maps" + "reflect" "strconv" "strings" "time" @@ -35,8 +35,10 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + "sigs.k8s.io/controller-runtime/pkg/event" "sigs.k8s.io/controller-runtime/pkg/handler" "sigs.k8s.io/controller-runtime/pkg/predicate" "sigs.k8s.io/controller-runtime/pkg/reconcile" @@ -74,7 +76,7 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl if err := r.Get(ctx, req.NamespacedName, &autoscalingRunnerSet); err != nil { return ctrl.Result{}, client.IgnoreNotFound(err) } - original := autoscalingRunnerSet.DeepCopy() + original := newOnce(autoscalingRunnerSet.DeepCopy) if !autoscalingRunnerSet.DeletionTimestamp.IsZero() { if !controllerutil.ContainsFinalizer(&autoscalingRunnerSet, autoscalingRunnerSetFinalizerName) { @@ -90,7 +92,7 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl if !done { log.Info("Waiting for resources to be cleaned up before removing finalizer") return ctrl.Result{ - RequeueAfter: 5 * time.Second, + RequeueAfter: 2 * time.Second, }, nil } @@ -99,15 +101,21 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl return ctrl.Result{}, err } - if controllerutil.RemoveFinalizer(&autoscalingRunnerSet, autoscalingRunnerSetFinalizerName) { + removeFinalizer := controllerutil.ContainsFinalizer(&autoscalingRunnerSet, autoscalingRunnerSetFinalizerName) + if removeFinalizer { + original.Do() + controllerutil.RemoveFinalizer(&autoscalingRunnerSet, autoscalingRunnerSetFinalizerName) + } + if removeFinalizer { log.Info("Removing finalizer") - if err := r.Patch(ctx, &autoscalingRunnerSet, client.MergeFrom(original)); err != nil && !kerrors.IsNotFound(err) { + if err := r.Patch(ctx, &autoscalingRunnerSet, client.MergeFrom(original.Get())); err != nil && !kerrors.IsNotFound(err) { log.Error(err, "Failed to update autoscaling runner set without finalizer") return ctrl.Result{}, err } } log.Info("Successfully removed finalizer after cleanup") + r.ResourceCache.Delete(&autoscalingRunnerSet) return ctrl.Result{}, nil } @@ -129,10 +137,15 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl return ctrl.Result{}, nil } - if controllerutil.AddFinalizer(&autoscalingRunnerSet, autoscalingRunnerSetFinalizerName) { + addFinalizer := !controllerutil.ContainsFinalizer(&autoscalingRunnerSet, autoscalingRunnerSetFinalizerName) + if addFinalizer { + original.Do() + controllerutil.AddFinalizer(&autoscalingRunnerSet, autoscalingRunnerSetFinalizerName) + } + if addFinalizer { log.Info("Adding finalizer") - if err := r.Patch(ctx, &autoscalingRunnerSet, client.MergeFrom(original)); err != nil { + if err := r.Patch(ctx, &autoscalingRunnerSet, client.MergeFrom(original.Get())); err != nil { log.Error(err, "Failed to update autoscaling runner set with finalizer") return ctrl.Result{}, err } @@ -141,27 +154,17 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl return ctrl.Result{}, nil } - // Something has changed, we need to re-apply the pending phase and change hash annotation to trigger the update of runner scale set and listener. - if targetHash := autoscalingRunnerSet.Hash(); autoscalingRunnerSet.Annotations[annotationKeyIntegrityHash] != targetHash { - // TODO: apply the version label - original := autoscalingRunnerSet.DeepCopy() - if autoscalingRunnerSet.Annotations == nil { - autoscalingRunnerSet.Annotations = map[string]string{} - } - autoscalingRunnerSet.Annotations[annotationKeyIntegrityHash] = targetHash - if err := r.Patch(ctx, &autoscalingRunnerSet, client.MergeFrom(original)); err != nil { - log.Error(err, "Failed to update autoscaling runner set with new change hash and pending phase") - return ctrl.Result{}, err - } - - original = autoscalingRunnerSet.DeepCopy() - autoscalingRunnerSet.Status.Phase = v1alpha1.AutoscalingRunnerSetPhasePending - if err := r.Status().Patch(ctx, &autoscalingRunnerSet, client.MergeFrom(original)); err != nil { + if autoscalingRunnerSet.Generation > autoscalingRunnerSet.Status.ObservedGeneration { + if err := r.updateStatus( + ctx, + &autoscalingRunnerSet, + v1alpha1.AutoscalingRunnerSetPhasePending, + autoscalingRunnerSet.Status.ObservedGeneration, + log, + ); err != nil { log.Error(err, "Failed to update autoscaling runner set status with pending phase") return ctrl.Result{}, err } - - return ctrl.Result{}, nil } outdated := autoscalingRunnerSet.Status.Phase == v1alpha1.AutoscalingRunnerSetPhaseOutdated @@ -204,12 +207,22 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl return ctrl.Result{}, nil } - original := ephemeralRunnerSet.DeepCopy() - ephemeralRunnerSet.Spec.Replicas = 0 - ephemeralRunnerSet.Spec.PatchID = 0 - if err := r.Patch(ctx, &ephemeralRunnerSet, client.MergeFrom(original)); err != nil { - log.Error(err, "Failed to patch ephemeral runner set with 0 replicas and reset patch ID for the outdated runner set") - return ctrl.Result{}, err + original := newOnce(ephemeralRunnerSet.DeepCopy) + ephemeralRunnerReplicasModified := ephemeralRunnerSet.Spec.Replicas != 0 + if ephemeralRunnerReplicasModified { + original.Do() + ephemeralRunnerSet.Spec.Replicas = 0 + } + ephemeralRunnerPatchIDModified := ephemeralRunnerSet.Spec.PatchID != 0 + if ephemeralRunnerPatchIDModified { + original.Do() + ephemeralRunnerSet.Spec.PatchID = 0 + } + if ephemeralRunnerReplicasModified || ephemeralRunnerPatchIDModified { + if err := r.Patch(ctx, &ephemeralRunnerSet, client.MergeFrom(original.Get())); err != nil { + log.Error(err, "Failed to patch ephemeral runner set with 0 replicas and reset patch ID for the outdated runner set") + return ctrl.Result{}, err + } } return ctrl.Result{}, nil @@ -291,39 +304,44 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl return ctrl.Result{}, nil } - if ephemeralRunnerSet.Annotations[annotationKeyIntegrityHash] != desired.Annotations[annotationKeyIntegrityHash] { - original := ephemeralRunnerSet.DeepCopy() - ephemeralRunnerSet.Spec.EphemeralRunnerMetadata = desired.Spec.EphemeralRunnerMetadata - ephemeralRunnerSet.Spec.EphemeralRunnerSpec = desired.Spec.EphemeralRunnerSpec - ephemeralRunnerSet.Labels = r.filterAndMergeLabels(ephemeralRunnerSet.Labels, desired.Labels) - ephemeralRunnerSet.Annotations = r.mergeAnnotations(ephemeralRunnerSet.Annotations, desired.Annotations) - - log.Info("Updating ephemeral runner set spec to match the desired spec") - if err := r.Patch(ctx, &ephemeralRunnerSet, client.MergeFrom(original)); err != nil { - log.Error(err, "Failed to patch ephemeral runner set to match the desired spec") - return ctrl.Result{}, err - } + original := newOnce(ephemeralRunnerSet.DeepCopy) - log.Info("Successfully patched ephemeral runner set spec") - return ctrl.Result{}, nil + ephemeralRunnerActionableSpecModified := !cmp.Equal(ephemeralRunnerSet.Spec.EphemeralRunnerSpec, desired.Spec.EphemeralRunnerSpec) + if ephemeralRunnerActionableSpecModified { + original.Do() + ephemeralRunnerSet.Spec.EphemeralRunnerSpec = desired.Spec.EphemeralRunnerSpec + ephemeralRunnerSet.Spec.ActionableRevision = nextActionableRevision(&ephemeralRunnerSet) } ephemeralRunnerMetadataModified := !cmp.Equal(ephemeralRunnerSet.Spec.EphemeralRunnerMetadata, desired.Spec.EphemeralRunnerMetadata) - ephemeralRunnerLabelsModified := !maps.Equal(ephemeralRunnerSet.Labels, desired.Labels) - ephemeralRunnerAnnotationsModified := !maps.Equal(ephemeralRunnerSet.Annotations, desired.Annotations) - - if ephemeralRunnerLabelsModified || ephemeralRunnerAnnotationsModified || ephemeralRunnerMetadataModified { - original := ephemeralRunnerSet.DeepCopy() - ephemeralRunnerSet.Labels = r.filterAndMergeLabels(ephemeralRunnerSet.Labels, desired.Labels) - ephemeralRunnerSet.Annotations = r.mergeAnnotations(ephemeralRunnerSet.Annotations, desired.Annotations) + if ephemeralRunnerMetadataModified { + original.Do() ephemeralRunnerSet.Spec.EphemeralRunnerMetadata = desired.Spec.EphemeralRunnerMetadata - log.Info("Updating ephemeral runner set metadata to match desired labels and annotations") + } + desiredLabels, ephemeralRunnerLabelsModified := r.mergeLabels(ephemeralRunnerSet.Labels, desired.Labels) + if ephemeralRunnerLabelsModified { + original.Do() + ephemeralRunnerSet.Labels = desiredLabels + } + desiredAnnotations, ephemeralRunnerAnnotationsModified := r.mergeAnnotations(ephemeralRunnerSet.Annotations, desired.Annotations) + if ephemeralRunnerAnnotationsModified { + original.Do() + ephemeralRunnerSet.Annotations = desiredAnnotations + } + + if ephemeralRunnerActionableSpecModified || ephemeralRunnerLabelsModified || ephemeralRunnerAnnotationsModified || ephemeralRunnerMetadataModified { + original := original.Get() + if ephemeralRunnerActionableSpecModified { + log.Info("Updating ephemeral runner set spec to match the desired spec") + } else { + 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") + log.Error(err, "Failed to patch ephemeral runner set to match the desired state") return ctrl.Result{}, err } - log.Info("Successfully patched ephemeral runner set metadata") + log.Info("Successfully patched ephemeral runner set") return ctrl.Result{}, nil } } @@ -357,9 +375,20 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl return ctrl.Result{}, nil } - if !cmp.Equal(listener.Spec, desired.Spec) || - !cmp.Equal(listener.Labels, desired.Labels) || - !cmp.Equal(listener.Annotations, desired.Annotations) { + original := newOnce(listener.DeepCopy) + desiredLabels, listnerLabelsModified := r.mergeLabels(listener.Labels, desired.Labels) + if listnerLabelsModified { + original.Do() + listener.Labels = desiredLabels + } + + desiredAnnotations, listenerAnnotationsModified := r.mergeAnnotations(listener.Annotations, desired.Annotations) + if listenerAnnotationsModified { + original.Do() + listener.Annotations = desiredAnnotations + } + + if !cmp.Equal(listener.Spec, desired.Spec) { log.Info("Deleting AutoscalingListener to re-create with updated spec") if err := r.Delete(ctx, &listener); err != nil { log.Error(err, "Failed to delete AutoscalingListener for re-creation") @@ -368,6 +397,16 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl log.Info("Deleted AutoscalingListener, will re-create on next reconcile") return ctrl.Result{}, nil } + + if original.Called() { + log.Info("Updating AutoscalingListener metadata to match desired labels and annotations") + if err := r.Patch(ctx, &listener, client.MergeFrom(original.Get())); err != nil { + log.Error(err, "Failed to patch AutoscalingListener metadata") + return ctrl.Result{}, err + } + log.Info("Successfully patched AutoscalingListener metadata") + return ctrl.Result{}, nil + } } log.Info("Autoscaling runner set is up to date and ready") @@ -375,6 +414,7 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl ctx, &autoscalingRunnerSet, v1alpha1.AutoscalingRunnerSetPhaseRunning, + autoscalingRunnerSet.Generation, log, ); err != nil { log.Error(err, "Failed to update autoscaling runner set status to running") @@ -419,14 +459,22 @@ func (r *AutoscalingRunnerSetReconciler) cleanUpResources(ctx context.Context, a } // Update the status of autoscaling runner set if necessary -func (r *AutoscalingRunnerSetReconciler) updateStatus(ctx context.Context, autoscalingRunnerSet *v1alpha1.AutoscalingRunnerSet, phase v1alpha1.AutoscalingRunnerSetPhase, log logr.Logger) error { +func (r *AutoscalingRunnerSetReconciler) updateStatus( + ctx context.Context, + autoscalingRunnerSet *v1alpha1.AutoscalingRunnerSet, + phase v1alpha1.AutoscalingRunnerSetPhase, + observedGeneration int64, + log logr.Logger, +) error { phaseDiff := phase != autoscalingRunnerSet.Status.Phase - if !phaseDiff { + observedGenerationDiff := observedGeneration != autoscalingRunnerSet.Status.ObservedGeneration + if !phaseDiff && !observedGenerationDiff { return nil } original := autoscalingRunnerSet.DeepCopy() autoscalingRunnerSet.Status.Phase = phase + autoscalingRunnerSet.Status.ObservedGeneration = observedGeneration if err := r.Status().Patch(ctx, autoscalingRunnerSet, client.MergeFrom(original)); err != nil { log.Error(err, "Failed to patch autoscaling runner set status") @@ -511,10 +559,11 @@ func (r *AutoscalingRunnerSetReconciler) removeFinalizersFromDependentResources( } func (r *AutoscalingRunnerSetReconciler) createRunnerScaleSet(ctx context.Context, autoscalingRunnerSet *v1alpha1.AutoscalingRunnerSet, logger logr.Logger) (ctrl.Result, error) { - original := autoscalingRunnerSet.DeepCopy() + original := newOnce(autoscalingRunnerSet.DeepCopy) logger.Info("Creating a new runner scale set") actionsClient, err := r.GetActionsService(ctx, autoscalingRunnerSet) if len(autoscalingRunnerSet.Spec.RunnerScaleSetName) == 0 { + original.Do() autoscalingRunnerSet.Spec.RunnerScaleSetName = autoscalingRunnerSet.Name } if err != nil { @@ -589,6 +638,7 @@ func (r *AutoscalingRunnerSetReconciler) createRunnerScaleSet(ctx context.Contex actionsClient.SetSystemInfo(info) logger.Info("Created/Reused a runner scale set", "id", runnerScaleSet.ID, "runnerGroupName", runnerScaleSet.RunnerGroupName) + original.Do() if autoscalingRunnerSet.Annotations == nil { autoscalingRunnerSet.Annotations = map[string]string{} } @@ -605,7 +655,7 @@ func (r *AutoscalingRunnerSetReconciler) createRunnerScaleSet(ctx context.Contex } logger.Info("Adding runner scale set ID, name and runner group name as an annotation and url labels") - if err = r.Patch(ctx, autoscalingRunnerSet, client.MergeFrom(original)); err != nil { + if err = r.Patch(ctx, autoscalingRunnerSet, client.MergeFrom(original.Get())); err != nil { logger.Error(err, "Failed to add runner scale set ID, name and runner group name as an annotation") return ctrl.Result{}, err } @@ -748,12 +798,12 @@ func (r *AutoscalingRunnerSetReconciler) deleteRunnerScaleSet(ctx context.Contex } func (r *AutoscalingRunnerSetReconciler) createEphemeralRunnerSet(ctx context.Context, autoscalingRunnerSet *v1alpha1.AutoscalingRunnerSet, log logr.Logger) (ctrl.Result, error) { + r.ResourceCache.ephemeralRunnerSet.Delete(autoscalingRunnerSet) desiredRunnerSet, err := r.newEphemeralRunnerSet(autoscalingRunnerSet) if err != nil { log.Error(err, "Could not create EphemeralRunnerSet") return ctrl.Result{}, err } - log.Info("Creating a new EphemeralRunnerSet resource") if err := r.Create(ctx, desiredRunnerSet); err != nil { log.Error(err, "Failed to create EphemeralRunnerSet resource") @@ -772,6 +822,7 @@ func (r *AutoscalingRunnerSetReconciler) createAutoScalingListenerForRunnerSet(c }) } + r.ResourceCache.autoscalingListener.Delete(autoscalingRunnerSet) autoscalingListener, err := r.newAutoscalingListener( autoscalingRunnerSet, ephemeralRunnerSet, @@ -811,7 +862,7 @@ func (r *AutoscalingRunnerSetReconciler) SetupWithManager(mgr ctrl.Manager, opts return builderWithOptions( ctrl.NewControllerManagedBy(mgr). For(&v1alpha1.AutoscalingRunnerSet{}). - Owns(&v1alpha1.EphemeralRunnerSet{}). + Owns(&v1alpha1.EphemeralRunnerSet{}, builder.WithPredicates(autoscalingRunnerSetOwnedEphemeralRunnerSetPredicate())). Watches(&v1alpha1.AutoscalingListener{}, handler.EnqueueRequestsFromMapFunc( func(_ context.Context, o client.Object) []reconcile.Request { autoscalingListener := o.(*v1alpha1.AutoscalingListener) @@ -830,6 +881,33 @@ func (r *AutoscalingRunnerSetReconciler) SetupWithManager(mgr ctrl.Manager, opts ).Complete(r) } +func autoscalingRunnerSetOwnedEphemeralRunnerSetPredicate() predicate.Predicate { + return predicate.Funcs{ + UpdateFunc: func(e event.UpdateEvent) bool { + oldRunnerSet, oldOk := e.ObjectOld.(*v1alpha1.EphemeralRunnerSet) + newRunnerSet, newOk := e.ObjectNew.(*v1alpha1.EphemeralRunnerSet) + if !oldOk || !newOk { + return false + } + + if !equalStringSlices(oldRunnerSet.GetFinalizers(), newRunnerSet.GetFinalizers()) || + oldRunnerSet.GetDeletionTimestamp() != newRunnerSet.GetDeletionTimestamp() { + return true + } + + oldSpec := *oldRunnerSet.Spec.DeepCopy() + newSpec := *newRunnerSet.Spec.DeepCopy() + oldSpec.PatchID = 0 + newSpec.PatchID = 0 + if !reflect.DeepEqual(oldSpec, newSpec) { + return true + } + + return oldRunnerSet.Status.Phase != newRunnerSet.Status.Phase + }, + } +} + type autoscalingRunnerSetFinalizerDependencyCleaner struct { // configuration fields client client.Client @@ -1091,6 +1169,11 @@ func (c *autoscalingRunnerSetFinalizerDependencyCleaner) removeManagerRoleBindin err := c.client.Get(ctx, types.NamespacedName{Name: managerRoleBindingName, Namespace: c.autoscalingRunnerSet.Namespace}, roleBinding) switch { case err == nil: + if !controllerutil.ContainsFinalizer(roleBinding, AutoscalingRunnerSetCleanupFinalizerName) { + c.logger.Info("Manager role binding finalizer has already been removed", "name", managerRoleBindingName) + return + } + original := roleBinding.DeepCopy() if controllerutil.RemoveFinalizer(roleBinding, AutoscalingRunnerSetCleanupFinalizerName) { if err = c.client.Patch(ctx, roleBinding, client.MergeFrom(original)); err != nil { @@ -1131,6 +1214,11 @@ func (c *autoscalingRunnerSetFinalizerDependencyCleaner) removeManagerRoleFinali err := c.client.Get(ctx, types.NamespacedName{Name: managerRoleName, Namespace: c.autoscalingRunnerSet.Namespace}, role) switch { case err == nil: + if !controllerutil.ContainsFinalizer(role, AutoscalingRunnerSetCleanupFinalizerName) { + c.logger.Info("Manager role finalizer has already been removed", "name", managerRoleName) + return + } + original := role.DeepCopy() if controllerutil.RemoveFinalizer(role, AutoscalingRunnerSetCleanupFinalizerName) { if err := c.client.Patch(ctx, role, client.MergeFrom(original)); err != nil { diff --git a/controllers/actions.github.com/autoscalingrunnerset_controller_test.go b/controllers/actions.github.com/autoscalingrunnerset_controller_test.go index 11a03dc482..a05fb292d3 100644 --- a/controllers/actions.github.com/autoscalingrunnerset_controller_test.go +++ b/controllers/actions.github.com/autoscalingrunnerset_controller_test.go @@ -44,6 +44,7 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { var autoscalingNS *corev1.Namespace var autoscalingRunnerSet *v1alpha1.AutoscalingRunnerSet var configSecret *corev1.Secret + var resourceCache *ResourceCache var originalBuildVersion string buildVersion := "0.1.0" @@ -65,6 +66,7 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { // Track runner group mappings for dynamic responses runnerGroupMap := map[int]string{1: "testgroup"} // ID -> Name mapping runnerGroupMapLock := &sync.RWMutex{} // Thread-safe access + resourceCache = newTestResourceCache() controller = &AutoscalingRunnerSetReconciler{ Client: mgr.GetClient(), @@ -73,6 +75,7 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { ControllerNamespace: autoscalingNS.Name, DefaultRunnerScaleSetListenerImage: "ghcr.io/actions/arc", ResourceBuilder: ResourceBuilder{ + ResourceCache: resourceCache, SecretResolver: secretresolver.New(mgr.GetClient(), scalefake.NewMultiClient( scalefake.WithClient( scalefake.NewClient( @@ -251,6 +254,15 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { autoscalingRunnerSetTestInterval, ).Should(Succeed(), "Listener should be created") + Eventually( + func() bool { + return resourceCacheStateHasMainObjectEntries(resourceCache.ephemeralRunnerSet, created) && + resourceCacheStateHasMainObjectEntries(resourceCache.autoscalingListener, created) + }, + autoscalingRunnerSetTestTimeout, + autoscalingRunnerSetTestInterval, + ).Should(BeTrue(), "AutoScalingRunnerSet EphemeralRunnerSet and AutoScalingListener resources should be cached after reconciliation") + // Check if status is updated runnerSetList := new(v1alpha1.EphemeralRunnerSetList) err := k8sClient.List(ctx, runnerSetList, client.InNamespace(autoscalingRunnerSet.Namespace)) @@ -270,8 +282,20 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { autoscalingRunnerSetTestInterval, ).Should(Succeed(), "Listener should be created") + created := new(v1alpha1.AutoscalingRunnerSet) + err := k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingRunnerSet.Name, Namespace: autoscalingRunnerSet.Namespace}, created) + Expect(err).NotTo(HaveOccurred(), "failed to get AutoScalingRunnerSet") + Eventually( + func() bool { + return resourceCacheStateHasMainObjectEntries(resourceCache.ephemeralRunnerSet, created) && + resourceCacheStateHasMainObjectEntries(resourceCache.autoscalingListener, created) + }, + autoscalingRunnerSetTestTimeout, + autoscalingRunnerSetTestInterval, + ).Should(BeTrue(), "AutoScalingRunnerSet EphemeralRunnerSet and AutoScalingListener resources should be cached before deletion") + // Delete the AutoScalingRunnerSet - err := k8sClient.Delete(ctx, autoscalingRunnerSet) + err = k8sClient.Delete(ctx, autoscalingRunnerSet) Expect(err).NotTo(HaveOccurred(), "failed to delete AutoScalingRunnerSet") // Check if the listener is deleted @@ -320,6 +344,15 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { autoscalingRunnerSetTestTimeout, autoscalingRunnerSetTestInterval, ).Should(Succeed(), "AutoScalingRunnerSet should be deleted") + + Eventually( + func() bool { + return resourceCacheStateHasMainObjectEntries(resourceCache.ephemeralRunnerSet, created) || + resourceCacheStateHasMainObjectEntries(resourceCache.autoscalingListener, created) + }, + autoscalingRunnerSetTestTimeout, + autoscalingRunnerSetTestInterval, + ).Should(BeFalse(), "AutoScalingRunnerSet EphemeralRunnerSet and AutoScalingListener resources should be removed from cache after deletion") }) }) @@ -478,7 +511,7 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { autoscalingRunnerSetTestInterval, ).Should(Succeed(), "EphemeralRunnerSet should be created") originalRunnerSetUID := runnerSet.UID - originalRunnerSetHash := runnerSet.Annotations[annotationKeyIntegrityHash] + originalActionableRevision := runnerSet.Spec.ActionableRevision patched := autoscalingRunnerSet.DeepCopy() patched.Spec.Template.Spec.Containers[0].Image = "ghcr.io/actions/runner:updated" @@ -491,8 +524,8 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { err := k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingRunnerSet.Name, Namespace: autoscalingRunnerSet.Namespace}, current) g.Expect(err).NotTo(HaveOccurred(), "failed to get EphemeralRunnerSet") g.Expect(current.UID).To(Equal(originalRunnerSetUID), "EphemeralRunnerSet should be updated in place") + g.Expect(current.Spec.ActionableRevision).To(BeNumerically(">", originalActionableRevision), "ActionableRevision should increment for actionable spec changes") g.Expect(current.Spec.EphemeralRunnerSpec.PodTemplateSpec.Spec.Containers[0].Image).To(Equal("ghcr.io/actions/runner:updated")) - g.Expect(current.Annotations[annotationKeyIntegrityHash]).NotTo(Equal(originalRunnerSetHash), "EphemeralRunnerSet spec hash should change") }, autoscalingRunnerSetTestTimeout, autoscalingRunnerSetTestInterval, @@ -531,7 +564,7 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { autoscalingRunnerSetTestInterval, ).Should(Succeed(), "EphemeralRunnerSet should be created") originalRunnerSetUID := runnerSet.UID - originalRunnerSetHash := runnerSet.Annotations[annotationKeyIntegrityHash] + originalActionableRevision := runnerSet.Spec.ActionableRevision patched := autoscalingRunnerSet.DeepCopy() max := 20 @@ -557,7 +590,7 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { err := k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingRunnerSet.Name, Namespace: autoscalingRunnerSet.Namespace}, current) g.Expect(err).NotTo(HaveOccurred(), "failed to get EphemeralRunnerSet") g.Expect(current.UID).To(Equal(originalRunnerSetUID), "EphemeralRunnerSet should not be recreated") - g.Expect(current.Annotations[annotationKeyIntegrityHash]).To(Equal(originalRunnerSetHash), "EphemeralRunnerSet spec should not change") + g.Expect(current.Spec.ActionableRevision).To(Equal(originalActionableRevision), "ActionableRevision should not change for non-actionable updates") }, time.Second*5, autoscalingRunnerSetTestInterval, @@ -875,10 +908,6 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { statusUpdate := runnerSet.DeepCopy() statusUpdate.Status.Phase = v1alpha1.EphemeralRunnerSetPhaseRunning - desiredStatus := v1alpha1.AutoscalingRunnerSetStatus{ - Phase: v1alpha1.AutoscalingRunnerSetPhaseRunning, - } - err := k8sClient.Status().Patch(ctx, statusUpdate, client.MergeFrom(&runnerSet)) Expect(err).NotTo(HaveOccurred(), "Failed to patch runner set status") @@ -893,7 +922,10 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { }, autoscalingRunnerSetTestTimeout, autoscalingRunnerSetTestInterval, - ).Should(BeEquivalentTo(desiredStatus), "AutoScalingRunnerSet status should be updated") + ).Should(SatisfyAll( + WithTransform(func(s v1alpha1.AutoscalingRunnerSetStatus) v1alpha1.AutoscalingRunnerSetPhase { return s.Phase }, Equal(v1alpha1.AutoscalingRunnerSetPhaseRunning)), + WithTransform(func(s v1alpha1.AutoscalingRunnerSetStatus) int64 { return s.ObservedGeneration }, BeNumerically(">=", ars.Generation)), + ), "AutoScalingRunnerSet status should be updated") }) }) @@ -961,6 +993,7 @@ var _ = Describe("Test AutoScalingController updates", Ordered, func() { ControllerNamespace: autoscalingNS.Name, DefaultRunnerScaleSetListenerImage: "ghcr.io/actions/arc", ResourceBuilder: ResourceBuilder{ + ResourceCache: newTestResourceCache(), SecretResolver: secretresolver.New(mgr.GetClient(), multiClient), }, } @@ -1078,6 +1111,7 @@ var _ = Describe("Test AutoscalingController creation failures", Ordered, func() ControllerNamespace: autoscalingNS.Name, DefaultRunnerScaleSetListenerImage: "ghcr.io/actions/arc", ResourceBuilder: ResourceBuilder{ + ResourceCache: newTestResourceCache(), SecretResolver: secretresolver.New(mgr.GetClient(), scalefake.NewMultiClient()), }, } @@ -1205,6 +1239,7 @@ var _ = Describe("Test client optional configuration", Ordered, func() { ControllerNamespace: autoscalingNS.Name, DefaultRunnerScaleSetListenerImage: "ghcr.io/actions/arc", ResourceBuilder: ResourceBuilder{ + ResourceCache: newTestResourceCache(), SecretResolver: secretresolver.New(mgr.GetClient(), multiclient.NewScaleset()), }, } @@ -1400,6 +1435,7 @@ var _ = Describe("Test client optional configuration", Ordered, func() { ControllerNamespace: autoscalingNS.Name, DefaultRunnerScaleSetListenerImage: "ghcr.io/actions/arc", ResourceBuilder: ResourceBuilder{ + ResourceCache: newTestResourceCache(), SecretResolver: secretresolver.New(mgr.GetClient(), scalefake.NewMultiClient( scalefake.WithClient( scalefake.NewClient( @@ -1647,6 +1683,7 @@ var _ = Describe("Test external permissions cleanup", Ordered, func() { ControllerNamespace: autoscalingNS.Name, DefaultRunnerScaleSetListenerImage: "ghcr.io/actions/arc", ResourceBuilder: ResourceBuilder{ + ResourceCache: newTestResourceCache(), SecretResolver: secretresolver.New(mgr.GetClient(), scalefake.NewMultiClient()), }, } @@ -1807,6 +1844,7 @@ var _ = Describe("Test external permissions cleanup", Ordered, func() { ControllerNamespace: autoscalingNS.Name, DefaultRunnerScaleSetListenerImage: "ghcr.io/actions/arc", ResourceBuilder: ResourceBuilder{ + ResourceCache: newTestResourceCache(), SecretResolver: secretresolver.New(mgr.GetClient(), scalefake.NewMultiClient()), }, } @@ -2017,6 +2055,7 @@ var _ = Describe("Test resource version and build version mismatch", func() { ControllerNamespace: autoscalingNS.Name, DefaultRunnerScaleSetListenerImage: "ghcr.io/actions/arc", ResourceBuilder: ResourceBuilder{ + ResourceCache: newTestResourceCache(), SecretResolver: secretresolver.New(mgr.GetClient(), scalefake.NewMultiClient()), }, } diff --git a/controllers/actions.github.com/ephemeralrunner_controller.go b/controllers/actions.github.com/ephemeralrunner_controller.go index e78ede68ff..a597127c22 100644 --- a/controllers/actions.github.com/ephemeralrunner_controller.go +++ b/controllers/actions.github.com/ephemeralrunner_controller.go @@ -36,8 +36,10 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + "sigs.k8s.io/controller-runtime/pkg/event" "sigs.k8s.io/controller-runtime/pkg/predicate" ) @@ -94,7 +96,7 @@ func (r *EphemeralRunnerReconciler) Reconcile(ctx context.Context, req ctrl.Requ if err := r.Get(ctx, req.NamespacedName, &ephemeralRunner); err != nil { return ctrl.Result{}, client.IgnoreNotFound(err) } - original := ephemeralRunner.DeepCopy() + original := newOnce(ephemeralRunner.DeepCopy) if !ephemeralRunner.DeletionTimestamp.IsZero() { r.publishEphemeralRunnerPhaseMetric(&ephemeralRunner, "", log) @@ -103,7 +105,8 @@ func (r *EphemeralRunnerReconciler) Reconcile(ctx context.Context, req ctrl.Requ return ctrl.Result{}, nil } - if controllerutil.ContainsFinalizer(&ephemeralRunner, ephemeralRunnerActionsFinalizerName) { + removeActionsFinalizer := controllerutil.ContainsFinalizer(&ephemeralRunner, ephemeralRunnerActionsFinalizerName) + if removeActionsFinalizer { log.Info("Trying to clean up runner from the service") ok, err := r.cleanupRunnerFromService(ctx, &ephemeralRunner, log) if err != nil { @@ -116,14 +119,16 @@ func (r *EphemeralRunnerReconciler) Reconcile(ctx context.Context, req ctrl.Requ } log.Info("Runner is cleaned up from the service, removing finalizer") - if controllerutil.RemoveFinalizer(&ephemeralRunner, ephemeralRunnerActionsFinalizerName) { - log.Info("Removed finalizer from ephemeral runner") - if err := r.Patch(ctx, &ephemeralRunner, client.MergeFrom(original)); err != nil { - log.Error(err, "Failed to update ephemeral runner after removing finalizer") - return ctrl.Result{}, err - } - } + original.Do() + controllerutil.RemoveFinalizer(&ephemeralRunner, ephemeralRunnerActionsFinalizerName) + } + if removeActionsFinalizer { log.Info("Removed finalizer from ephemeral runner") + if err := r.Patch(ctx, &ephemeralRunner, client.MergeFrom(original.Get())); err != nil { + log.Error(err, "Failed to update ephemeral runner after removing finalizer") + return ctrl.Result{}, err + } + return ctrl.Result{}, nil } log.Info("Finalizing ephemeral runner") @@ -142,16 +147,21 @@ func (r *EphemeralRunnerReconciler) Reconcile(ctx context.Context, req ctrl.Requ } } - log.Info("Removing finalizer") - if controllerutil.RemoveFinalizer(&ephemeralRunner, ephemeralRunnerFinalizerName) { + removeFinalizer := controllerutil.ContainsFinalizer(&ephemeralRunner, ephemeralRunnerFinalizerName) + if removeFinalizer { + original.Do() + controllerutil.RemoveFinalizer(&ephemeralRunner, ephemeralRunnerFinalizerName) + } + if removeFinalizer { + log.Info("Removing finalizer") log.Info("Removed finalizer from ephemeral runner") - if err := r.Patch(ctx, &ephemeralRunner, client.MergeFrom(original)); client.IgnoreNotFound(err) != nil { + if err := r.Patch(ctx, &ephemeralRunner, client.MergeFrom(original.Get())); client.IgnoreNotFound(err) != nil { log.Error(err, "Failed to update ephemeral runner after removing finalizer") return ctrl.Result{}, err } } - log.Info("Successfully removed finalizer after cleanup") + r.ResourceCache.Delete(&ephemeralRunner) return ctrl.Result{}, nil } @@ -171,17 +181,21 @@ func (r *EphemeralRunnerReconciler) Reconcile(ctx context.Context, req ctrl.Requ return ctrl.Result{}, nil } - addFinalizers := !controllerutil.ContainsFinalizer(&ephemeralRunner, ephemeralRunnerFinalizerName) || !controllerutil.ContainsFinalizer(&ephemeralRunner, ephemeralRunnerActionsFinalizerName) - if addFinalizers { + ephemeralRunnerFinalizerModified := !controllerutil.ContainsFinalizer(&ephemeralRunner, ephemeralRunnerFinalizerName) + if ephemeralRunnerFinalizerModified { + original.Do() + controllerutil.AddFinalizer(&ephemeralRunner, ephemeralRunnerFinalizerName) + } + ephemeralRunnerActionsFinalizerModified := !controllerutil.ContainsFinalizer(&ephemeralRunner, ephemeralRunnerActionsFinalizerName) + if ephemeralRunnerActionsFinalizerModified { + original.Do() + controllerutil.AddFinalizer(&ephemeralRunner, ephemeralRunnerActionsFinalizerName) + } + if ephemeralRunnerFinalizerModified || ephemeralRunnerActionsFinalizerModified { log.Info("Adding finalizers") - var addedFinalizers bool - addedFinalizers = addedFinalizers || controllerutil.AddFinalizer(&ephemeralRunner, ephemeralRunnerFinalizerName) - addedFinalizers = addedFinalizers || controllerutil.AddFinalizer(&ephemeralRunner, ephemeralRunnerActionsFinalizerName) - if addedFinalizers { - if err := r.Patch(ctx, &ephemeralRunner, client.MergeFrom(original)); err != nil { - log.Error(err, "Failed to update with finalizer set") - return ctrl.Result{}, err - } + if err := r.Patch(ctx, &ephemeralRunner, client.MergeFrom(original.Get())); err != nil { + log.Error(err, "Failed to update with finalizer set") + return ctrl.Result{}, err } log.Info("Successfully added finalizers") } @@ -836,6 +850,7 @@ func (r *EphemeralRunnerReconciler) createSecret(ctx context.Context, runner *v1 // updateRunStatusFromPod is responsible for updating non-exiting statuses. // It should never update phase to Failed or Succeeded +// It should never update phase to Running (listener owns that transition) // // The event should not be re-queued since the termination status should be set // before proceeding with reconciliation logic @@ -853,8 +868,14 @@ func (r *EphemeralRunnerReconciler) updateRunStatusFromPod(ctx context.Context, } } - phase := v1alpha1.EphemeralRunnerPhase(pod.Status.Phase) - phaseChanged := ephemeralRunner.Status.Phase != phase + phase := ephemeralRunner.Status.Phase + if pod.Status.Phase == corev1.PodPending && phase == "" { + phase = v1alpha1.EphemeralRunnerPhasePending + } + + // Controller no longer sets Running phase - listener owns that transition when job is assigned. + // The controller still publishes the initial Pending phase while the runner pod is starting. + phaseChanged := phase != ephemeralRunner.Status.Phase readyChanged := ready != ephemeralRunner.Status.Ready if !phaseChanged && !readyChanged { @@ -954,13 +975,107 @@ func (r *EphemeralRunnerReconciler) SetupWithManager(mgr ctrl.Manager, opts ...O return builderWithOptions( ctrl.NewControllerManagedBy(mgr). - For(&v1alpha1.EphemeralRunner{}). - Owns(&corev1.Pod{}). - WithEventFilter(predicate.ResourceVersionChangedPredicate{}), + For(&v1alpha1.EphemeralRunner{}, builder.WithPredicates(ephemeralRunnerPrimaryPredicate())). + Owns(&corev1.Pod{}, builder.WithPredicates(ephemeralRunnerOwnedPodPredicate())), opts, ).Complete(r) } +func ephemeralRunnerPrimaryPredicate() predicate.Predicate { + return predicate.Funcs{ + UpdateFunc: func(e event.UpdateEvent) bool { + if e.ObjectOld == nil || e.ObjectNew == nil { + return false + } + + return e.ObjectOld.GetGeneration() != e.ObjectNew.GetGeneration() || + !equalStringSlices(e.ObjectOld.GetFinalizers(), e.ObjectNew.GetFinalizers()) || + e.ObjectOld.GetDeletionTimestamp() != e.ObjectNew.GetDeletionTimestamp() + }, + } +} + +func ephemeralRunnerOwnedPodPredicate() predicate.Predicate { + return predicate.Funcs{ + UpdateFunc: func(e event.UpdateEvent) bool { + oldPod, oldOk := e.ObjectOld.(*corev1.Pod) + newPod, newOk := e.ObjectNew.(*corev1.Pod) + if !oldOk || !newOk { + return false + } + + if oldPod.Status.Phase != newPod.Status.Phase { + return true + } + + if !equalContainerStatus(runnerContainerStatus(oldPod), runnerContainerStatus(newPod)) { + return true + } + + if !equalContainerStatusesByName(oldPod.Status.InitContainerStatuses, newPod.Status.InitContainerStatuses) { + return true + } + + return oldPod.GetDeletionTimestamp() != newPod.GetDeletionTimestamp() + }, + } +} + +func equalContainerStatusesByName(old, new []corev1.ContainerStatus) bool { + if len(old) != len(new) { + return false + } + + newByName := make(map[string]*corev1.ContainerStatus, len(new)) + for i := range new { + newByName[new[i].Name] = &new[i] + } + + for i := range old { + newStatus, ok := newByName[old[i].Name] + if !ok { + return false + } + if !equalContainerStatus(&old[i], newStatus) { + return false + } + } + + return true +} + +func equalContainerStatus(old, new *corev1.ContainerStatus) bool { + if old == nil && new == nil { + return true + } + if old == nil || new == nil { + return false + } + + if containerStateString(old.State) != containerStateString(new.State) { + return false + } + + if old.State.Terminated != nil && new.State.Terminated != nil && old.State.Terminated.ExitCode != new.State.Terminated.ExitCode { + return false + } + + return old.Ready == new.Ready +} + +func containerStateString(state corev1.ContainerState) string { + if state.Running != nil { + return "running" + } + if state.Terminated != nil { + return "terminated" + } + if state.Waiting != nil { + return "waiting" + } + return "unknown" +} + func runnerContainerStatus(pod *corev1.Pod) *corev1.ContainerStatus { for i := range pod.Status.ContainerStatuses { cs := &pod.Status.ContainerStatuses[i] diff --git a/controllers/actions.github.com/ephemeralrunner_controller_test.go b/controllers/actions.github.com/ephemeralrunner_controller_test.go index 80c27134a7..57b5304110 100644 --- a/controllers/actions.github.com/ephemeralrunner_controller_test.go +++ b/controllers/actions.github.com/ephemeralrunner_controller_test.go @@ -100,17 +100,20 @@ var _ = Describe("EphemeralRunner", func() { var configSecret *corev1.Secret var controller *EphemeralRunnerReconciler var ephemeralRunner *v1alpha1.EphemeralRunner + var resourceCache *ResourceCache BeforeEach(func() { ctx = context.Background() autoscalingNS, mgr = createNamespace(GinkgoT(), k8sClient) configSecret = createDefaultSecret(GinkgoT(), k8sClient, autoscalingNS.Name) + resourceCache = newTestResourceCache() controller = &EphemeralRunnerReconciler{ Client: mgr.GetClient(), Scheme: mgr.GetScheme(), Log: logf.Log, ResourceBuilder: ResourceBuilder{ + ResourceCache: resourceCache, SecretResolver: secretresolver.New(mgr.GetClient(), scalefake.NewMultiClient( scalefake.WithClient( scalefake.NewClient( @@ -651,6 +654,12 @@ var _ = Describe("EphemeralRunner", func() { return true, nil }).Should(BeEquivalentTo(true)) + created := new(v1alpha1.EphemeralRunner) + err := k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, created) + Expect(err).To(BeNil(), "failed to get ephemeral runner") + resourceCache.listenerPod.Upsert(created, &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "cached-runner-pod", Namespace: created.Namespace}}) + Expect(resourceCacheHasMainObjectEntries(resourceCache, created)).To(BeTrue(), "test setup should cache an EphemeralRunner-owned resource") + // create runner-linked pod runnerLinkedPod := &corev1.Pod{ ObjectMeta: metav1.ObjectMeta{ @@ -670,7 +679,7 @@ var _ = Describe("EphemeralRunner", func() { }, } - err := k8sClient.Create(ctx, runnerLinkedPod) + err = k8sClient.Create(ctx, runnerLinkedPod) Expect(err).To(BeNil(), "failed to create runner linked pod") Eventually( func() (bool, error) { @@ -777,6 +786,14 @@ var _ = Describe("EphemeralRunner", func() { ephemeralRunnerTimeout, ephemeralRunnerInterval, ).Should(BeEquivalentTo(true)) + + Eventually( + func() bool { + return resourceCacheHasMainObjectEntries(resourceCache, created) + }, + ephemeralRunnerTimeout, + ephemeralRunnerInterval, + ).Should(BeFalse(), "EphemeralRunner-owned resources should be removed from cache after deletion") }) It("It should eventually have runner id set", func() { @@ -808,32 +825,47 @@ var _ = Describe("EphemeralRunner", func() { ephemeralRunnerInterval, ).Should(BeEquivalentTo(true)) - for _, phase := range []corev1.PodPhase{corev1.PodRunning, corev1.PodPending} { - podCopy := pod.DeepCopy() - pod.Status.Phase = phase - // set container state to force status update - pod.Status.ContainerStatuses = append(pod.Status.ContainerStatuses, corev1.ContainerStatus{ - Name: v1alpha1.EphemeralRunnerContainerName, - State: corev1.ContainerState{}, - }) + podCopy := pod.DeepCopy() + pod.Status.Phase = corev1.PodPending + // set container state to force status update + pod.Status.ContainerStatuses = append(pod.Status.ContainerStatuses, corev1.ContainerStatus{ + Name: v1alpha1.EphemeralRunnerContainerName, + State: corev1.ContainerState{}, + }) - err := k8sClient.Status().Patch(ctx, pod, client.MergeFrom(podCopy)) - Expect(err).To(BeNil(), "failed to patch pod status") + err := k8sClient.Status().Patch(ctx, pod, client.MergeFrom(podCopy)) + Expect(err).To(BeNil(), "failed to patch pod status") - var updated *v1alpha1.EphemeralRunner - Eventually( - func() (v1alpha1.EphemeralRunnerPhase, error) { - updated = new(v1alpha1.EphemeralRunner) - err := k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, updated) - if err != nil { - return "", err - } - return updated.Status.Phase, nil - }, - ephemeralRunnerTimeout, - ephemeralRunnerInterval, - ).Should(BeEquivalentTo(phase)) - } + Eventually( + func() (v1alpha1.EphemeralRunnerPhase, error) { + updated := new(v1alpha1.EphemeralRunner) + err := k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, updated) + if err != nil { + return "", err + } + return updated.Status.Phase, nil + }, + ephemeralRunnerTimeout, + ephemeralRunnerInterval, + ).Should(BeEquivalentTo(v1alpha1.EphemeralRunnerPhasePending)) + + podCopy = pod.DeepCopy() + pod.Status.Phase = corev1.PodRunning + err = k8sClient.Status().Patch(ctx, pod, client.MergeFrom(podCopy)) + Expect(err).To(BeNil(), "failed to patch pod status") + + Consistently( + func() (v1alpha1.EphemeralRunnerPhase, error) { + updated := new(v1alpha1.EphemeralRunner) + err := k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, updated) + if err != nil { + return "", err + } + return updated.Status.Phase, nil + }, + ephemeralRunnerInterval*3, + ephemeralRunnerInterval, + ).Should(BeEquivalentTo(v1alpha1.EphemeralRunnerPhasePending), "controller should not set Running from pod status") }) It("It should update ready based on the latest condition", func() { @@ -1156,7 +1188,6 @@ var _ = Describe("EphemeralRunner", func() { ephemeralRunnerInterval, ).Should(BeEquivalentTo(true)) - // first set phase to running pod.Status.ContainerStatuses = append(pod.Status.ContainerStatuses, corev1.ContainerStatus{ Name: v1alpha1.EphemeralRunnerContainerName, State: corev1.ContainerState{ @@ -1169,7 +1200,20 @@ var _ = Describe("EphemeralRunner", func() { err := k8sClient.Status().Update(ctx, pod) Expect(err).To(BeNil()) - Eventually( + updated := new(v1alpha1.EphemeralRunner) + err = k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, updated) + Expect(err).To(BeNil()) + + original := updated.DeepCopy() + updated.Status.Phase = v1alpha1.EphemeralRunnerPhaseRunning + err = k8sClient.Status().Patch(ctx, updated, client.MergeFrom(original)) + Expect(err).To(BeNil()) + + pod.Status.Phase = corev1.PodSucceeded + err = k8sClient.Status().Update(ctx, pod) + Expect(err).To(BeNil()) + + Consistently( func() (v1alpha1.EphemeralRunnerPhase, error) { updated := new(v1alpha1.EphemeralRunner) if err := k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, updated); err != nil { @@ -1178,24 +1222,61 @@ var _ = Describe("EphemeralRunner", func() { return updated.Status.Phase, nil }, ephemeralRunnerTimeout, - ephemeralRunnerInterval, ).Should(BeEquivalentTo(v1alpha1.EphemeralRunnerPhaseRunning)) + }) - // set phase to succeeded - pod.Status.Phase = corev1.PodSucceeded - err = k8sClient.Status().Update(ctx, pod) + It("Controller should not set Running phase from pod status - listener owns Running transition", func() { + pod := new(corev1.Pod) + Eventually( + func() (bool, error) { + if err := k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, pod); err != nil { + return false, err + } + return true, nil + }, + ephemeralRunnerTimeout, + ephemeralRunnerInterval, + ).Should(BeEquivalentTo(true)) + + pod.Status.ContainerStatuses = append(pod.Status.ContainerStatuses, corev1.ContainerStatus{ + Name: v1alpha1.EphemeralRunnerContainerName, + State: corev1.ContainerState{ + Running: &corev1.ContainerStateRunning{ + StartedAt: metav1.Now(), + }, + }, + }) + pod.Status.Phase = corev1.PodRunning + pod.Status.Conditions = append(pod.Status.Conditions, corev1.PodCondition{ + Type: corev1.PodReady, + Status: corev1.ConditionTrue, + LastTransitionTime: metav1.Now(), + }) + err := k8sClient.Status().Update(ctx, pod) Expect(err).To(BeNil()) Consistently( func() (v1alpha1.EphemeralRunnerPhase, error) { updated := new(v1alpha1.EphemeralRunner) if err := k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, updated); err != nil { - return "", err + return "Unknown", err } return updated.Status.Phase, nil }, ephemeralRunnerTimeout, - ).Should(BeEquivalentTo(v1alpha1.EphemeralRunnerPhaseRunning)) + ).Should(BeEquivalentTo("")) + + updated := new(v1alpha1.EphemeralRunner) + Eventually( + func() (bool, error) { + if err := k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, updated); err != nil { + return false, err + } + return updated.Status.Ready, nil + }, + ephemeralRunnerTimeout, + ephemeralRunnerInterval, + ).Should(BeEquivalentTo(true)) }) }) @@ -1216,6 +1297,7 @@ var _ = Describe("EphemeralRunner", func() { Scheme: mgr.GetScheme(), Log: logf.Log, ResourceBuilder: ResourceBuilder{ + ResourceCache: newTestResourceCache(), SecretResolver: secretresolver.New( mgr.GetClient(), scalefake.NewMultiClient( @@ -1302,6 +1384,7 @@ var _ = Describe("EphemeralRunner", func() { Scheme: mgr.GetScheme(), Log: logf.Log, ResourceBuilder: ResourceBuilder{ + ResourceCache: newTestResourceCache(), SecretResolver: secretresolver.New(mgr.GetClient(), scalefake.NewMultiClient( scalefake.WithClient( scalefake.NewClient( @@ -1326,6 +1409,7 @@ var _ = Describe("EphemeralRunner", func() { It("uses an actions client with proxy transport", func() { // Use an actual client controller.ResourceBuilder = ResourceBuilder{ + ResourceCache: newTestResourceCache(), SecretResolver: secretresolver.New( mgr.GetClient(), multiclient.NewScaleset(), @@ -1485,6 +1569,7 @@ var _ = Describe("EphemeralRunner", func() { Scheme: mgr.GetScheme(), Log: logf.Log, ResourceBuilder: ResourceBuilder{ + ResourceCache: newTestResourceCache(), SecretResolver: secretresolver.New(mgr.GetClient(), scalefake.NewMultiClient()), }, } @@ -1519,6 +1604,7 @@ var _ = Describe("EphemeralRunner", func() { // Use an actual client controller.ResourceBuilder = ResourceBuilder{ + ResourceCache: newTestResourceCache(), SecretResolver: secretresolver.New( mgr.GetClient(), multiclient.NewScaleset(), diff --git a/controllers/actions.github.com/ephemeralrunnerset_controller.go b/controllers/actions.github.com/ephemeralrunnerset_controller.go index 919a46419f..40924538d8 100644 --- a/controllers/actions.github.com/ephemeralrunnerset_controller.go +++ b/controllers/actions.github.com/ephemeralrunnerset_controller.go @@ -35,9 +35,12 @@ import ( kerrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/util/retry" ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + "sigs.k8s.io/controller-runtime/pkg/event" "sigs.k8s.io/controller-runtime/pkg/predicate" ) @@ -79,7 +82,7 @@ func (r *EphemeralRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl.R if err := r.Get(ctx, req.NamespacedName, &ephemeralRunnerSet); err != nil { return ctrl.Result{}, client.IgnoreNotFound(err) } - original := ephemeralRunnerSet.DeepCopy() + original := newOnce(ephemeralRunnerSet.DeepCopy) // Requested deletion does not need reconciled. if !ephemeralRunnerSet.DeletionTimestamp.IsZero() { @@ -108,22 +111,33 @@ func (r *EphemeralRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl.R return ctrl.Result{RequeueAfter: 1 * time.Second}, nil } - log.Info("Removing finalizer") - if controllerutil.RemoveFinalizer(&ephemeralRunnerSet, EphemeralRunnerSetFinalizerName) { - if err := r.Patch(ctx, &ephemeralRunnerSet, client.MergeFrom(original)); err != nil { + removeFinalizer := controllerutil.ContainsFinalizer(&ephemeralRunnerSet, EphemeralRunnerSetFinalizerName) + if removeFinalizer { + original.Do() + controllerutil.RemoveFinalizer(&ephemeralRunnerSet, EphemeralRunnerSetFinalizerName) + } + if removeFinalizer { + log.Info("Removing finalizer") + if err := r.Patch(ctx, &ephemeralRunnerSet, client.MergeFrom(original.Get())); err != nil { log.Error(err, "Failed to update ephemeral runner set with removed finalizer") return ctrl.Result{}, err } } log.Info("Successfully removed finalizer after cleanup") + r.ResourceCache.Delete(&ephemeralRunnerSet) return ctrl.Result{}, nil } // Add finalizer if not present - if controllerutil.AddFinalizer(&ephemeralRunnerSet, EphemeralRunnerSetFinalizerName) { + addFinalizer := !controllerutil.ContainsFinalizer(&ephemeralRunnerSet, EphemeralRunnerSetFinalizerName) + if addFinalizer { + original.Do() + controllerutil.AddFinalizer(&ephemeralRunnerSet, EphemeralRunnerSetFinalizerName) + } + if addFinalizer { log.Info("Adding finalizer") - if err := r.Patch(ctx, &ephemeralRunnerSet, client.MergeFrom(original)); err != nil { + if err := r.Patch(ctx, &ephemeralRunnerSet, client.MergeFrom(original.Get())); err != nil { log.Error(err, "Failed to update ephemeral runner set with new finalizer") return ctrl.Result{}, err } @@ -132,11 +146,12 @@ func (r *EphemeralRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl.R return ctrl.Result{}, nil } - // If hash spec has changed, delete idle ephemeral runners - // in order to apply the change to the runners that did not yet receive a job. - ephemeralRunnerIntegrityHash := ephemeralRunnerSetIntegrityHash(&ephemeralRunnerSet) - if ephemeralRunnerSet.Annotations[annotationKeyIntegrityHash] != ephemeralRunnerIntegrityHash { - log.Info("EphemeralRunnerSpec has changed, deleting idle ephemeral runners to apply the new spec") + if ephemeralRunnerSet.Spec.ActionableRevision > ephemeralRunnerSet.Status.AppliedActionableRevision { + log.Info( + "EphemeralRunnerSpec revision has changed, deleting idle or pending ephemeral runners to apply the new spec", + "specActionableRevision", ephemeralRunnerSet.Spec.ActionableRevision, + "statusAppliedActionableRevision", ephemeralRunnerSet.Status.AppliedActionableRevision, + ) if _, err := r.cleanUpEphemeralRunners(ctx, &ephemeralRunnerSet, log); err != nil { log.Error(err, "Failed to clean up EphemeralRunners") return ctrl.Result{}, err @@ -147,18 +162,12 @@ func (r *EphemeralRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl.R return ctrl.Result{}, err } - log.Info("Updating EphemeralRunnerSet with new spec hash") - original := ephemeralRunnerSet.DeepCopy() - if ephemeralRunnerSet.Annotations == nil { - ephemeralRunnerSet.Annotations = make(map[string]string) - } - ephemeralRunnerSet.Annotations[annotationKeyIntegrityHash] = ephemeralRunnerIntegrityHash - if err := r.Patch(ctx, &ephemeralRunnerSet, client.MergeFrom(original)); err != nil { - log.Error(err, "Failed to update ephemeral runner set with new spec hash") + if err := r.patchAppliedActionableRevisionStatus(ctx, req.NamespacedName, ephemeralRunnerSet.Spec.ActionableRevision); err != nil { + log.Error(err, "Failed to update EphemeralRunnerSet applied actionable revision status") return ctrl.Result{}, err } - log.Info("Updated ephemeral runner set with new spec hash") + log.Info("Updated EphemeralRunnerSet applied actionable revision status", "appliedActionableRevision", ephemeralRunnerSet.Spec.ActionableRevision) return ctrl.Result{}, nil } @@ -204,15 +213,31 @@ func (r *EphemeralRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl.R total := ephemeralRunnersByState.scaleTotal() if ephemeralRunnerSet.Spec.PatchID == 0 || ephemeralRunnerSet.Spec.PatchID != ephemeralRunnersByState.latestPatchID { - defer func() { + if len(ephemeralRunnersByState.finished) > 0 { if err := r.cleanupFinishedEphemeralRunners(ctx, ephemeralRunnersByState.finished, log); err != nil { log.Error(err, "failed to cleanup finished ephemeral runners") + return ctrl.Result{}, err + } + if err := r.patchFinishedRunnerCleanupPatchIDStatus(ctx, req.NamespacedName, ephemeralRunnerSet.Spec.PatchID); err != nil { + log.Error(err, "failed to update finished runner cleanup patch ID status") + return ctrl.Result{}, err } - }() - log.Info("Scaling comparison", "current", total, "desired", ephemeralRunnerSet.Spec.Replicas) + ephemeralRunnerSet.Status.FinishedRunnerCleanupPatchID = ephemeralRunnerSet.Spec.PatchID + + log.Info("Finished ephemeral runners were cleaned up, deferring scaling decision") + return ctrl.Result{}, r.updateStatus(ctx, &ephemeralRunnerSet, ephemeralRunnersByState, log) + } + + scaleUpTotal := total + len(ephemeralRunnersByState.deleting) + log.Info("Scaling comparison", "current", total, "deleting", len(ephemeralRunnersByState.deleting), "desired", ephemeralRunnerSet.Spec.Replicas) switch { - case total < ephemeralRunnerSet.Spec.Replicas: // Handle scale up - count := ephemeralRunnerSet.Spec.Replicas - total + case scaleUpTotal < ephemeralRunnerSet.Spec.Replicas: // Handle scale up + if ephemeralRunnerSet.Spec.PatchID > 0 && ephemeralRunnerSet.Status.FinishedRunnerCleanupPatchID == ephemeralRunnerSet.Spec.PatchID { + log.Info("Skipping scale up until listener publishes a fresh desired state after finished runner cleanup", "patchID", ephemeralRunnerSet.Spec.PatchID) + return ctrl.Result{}, r.updateStatus(ctx, &ephemeralRunnerSet, ephemeralRunnersByState, log) + } + + count := ephemeralRunnerSet.Spec.Replicas - scaleUpTotal log.Info("Creating new ephemeral runners (scale up)", "count", count) if err := r.createEphemeralRunners(ctx, &ephemeralRunnerSet, count, log); err != nil { log.Error(err, "failed to make ephemeral runner") @@ -244,8 +269,54 @@ func (r *EphemeralRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl.R return ctrl.Result{}, r.updateStatus(ctx, &ephemeralRunnerSet, ephemeralRunnersByState, log) } +func (r *EphemeralRunnerSetReconciler) patchAppliedActionableRevisionStatus(ctx context.Context, key types.NamespacedName, targetAppliedRevision int64) error { + return retry.RetryOnConflict(retry.DefaultBackoff, func() error { + var latest v1alpha1.EphemeralRunnerSet + if err := r.Get(ctx, key, &latest); err != nil { + return err + } + + desiredStatus := latest.Status + desiredStatus.AppliedActionableRevision = targetAppliedRevision + + ephemeralRunnerList := new(v1alpha1.EphemeralRunnerList) + if err := r.List(ctx, ephemeralRunnerList, client.InNamespace(latest.Namespace), client.MatchingFields{resourceOwnerKey: latest.Name}); err != nil { + return fmt.Errorf("failed to list child ephemeral runners: %w", err) + } + + if len(newEphemeralRunnersByStates(ephemeralRunnerList).outdated) == 0 { + desiredStatus.Phase = v1alpha1.EphemeralRunnerSetPhaseRunning + } + + if latest.Status == desiredStatus { + return nil + } + + original := latest.DeepCopy() + latest.Status = desiredStatus + return r.Status().Patch(ctx, &latest, client.MergeFrom(original)) + }) +} + +func (r *EphemeralRunnerSetReconciler) patchFinishedRunnerCleanupPatchIDStatus(ctx context.Context, key types.NamespacedName, patchID int) error { + return retry.RetryOnConflict(retry.DefaultBackoff, func() error { + var latest v1alpha1.EphemeralRunnerSet + if err := r.Get(ctx, key, &latest); err != nil { + return err + } + + original := latest.DeepCopy() + latest.Status.FinishedRunnerCleanupPatchID = patchID + + if original.Status == latest.Status { + return nil + } + + return r.Status().Patch(ctx, &latest, client.MergeFrom(original)) + }) +} + func (r *EphemeralRunnerSetReconciler) updateStatus(ctx context.Context, ephemeralRunnerSet *v1alpha1.EphemeralRunnerSet, state *ephemeralRunnersByState, log logr.Logger) error { - original := ephemeralRunnerSet.DeepCopy() var phase v1alpha1.EphemeralRunnerSetPhase switch { case len(state.outdated) > 0: @@ -256,11 +327,14 @@ func (r *EphemeralRunnerSetReconciler) updateStatus(ctx context.Context, ephemer phase = ephemeralRunnerSet.Status.Phase } desiredStatus := v1alpha1.EphemeralRunnerSetStatus{ - Phase: phase, + Phase: phase, + AppliedActionableRevision: ephemeralRunnerSet.Status.AppliedActionableRevision, + FinishedRunnerCleanupPatchID: ephemeralRunnerSet.Status.FinishedRunnerCleanupPatchID, } // Update the status if needed. if ephemeralRunnerSet.Status != desiredStatus { + original := ephemeralRunnerSet.DeepCopy() ephemeralRunnerSet.Status = desiredStatus if err := r.Status().Patch(ctx, ephemeralRunnerSet, client.MergeFrom(original)); err != nil { log.Error(err, "Failed to update EphemeralRunnerSet status") @@ -466,26 +540,27 @@ func (r *EphemeralRunnerSetReconciler) reconcileEphemeralRunnerSetProxySecret(ct } dataModified := !maps.EqualFunc(proxySecret.Data, desiredRunnerSetProxy.Data, bytes.Equal) - desiredLabels := r.filterAndMergeLabels(proxySecret.Labels, desiredRunnerSetProxy.Labels) - labelsModified := !maps.Equal(proxySecret.Labels, desiredLabels) - desiredAnnotations := r.mergeAnnotations(proxySecret.Annotations, desiredRunnerSetProxy.Annotations) - annotationsModified := !maps.Equal(proxySecret.Annotations, desiredAnnotations) + desiredLabels, labelsModified := r.mergeLabels(proxySecret.Labels, desiredRunnerSetProxy.Labels) + desiredAnnotations, annotationsModified := r.mergeAnnotations(proxySecret.Annotations, desiredRunnerSetProxy.Annotations) + original := newOnce(proxySecret.DeepCopy) + if dataModified { + original.Do() + proxySecret.Data = desiredRunnerSetProxy.Data + } + if labelsModified { + original.Do() + proxySecret.Labels = desiredLabels + } + if annotationsModified { + original.Do() + proxySecret.Annotations = desiredAnnotations + } if dataModified || labelsModified || annotationsModified { - updatedProxySecret := proxySecret.DeepCopy() - if dataModified { - updatedProxySecret.Data = desiredRunnerSetProxy.Data - } - if labelsModified { - updatedProxySecret.Labels = desiredLabels - } - if annotationsModified { - updatedProxySecret.Annotations = desiredAnnotations - } log.Info("Updating ephemeralRunnerSet proxy secret") - if err := r.Patch(ctx, updatedProxySecret, client.MergeFrom(&proxySecret)); err != nil { + if err := r.Patch(ctx, &proxySecret, client.MergeFrom(original.Get())); err != nil { return nil, false, fmt.Errorf("failed to update ephemeralRunnerSet proxy secret: %w", err) } - return updatedProxySecret, true, nil + return &proxySecret, true, nil } return &proxySecret, false, nil case kerrors.IsNotFound(err): @@ -637,13 +712,47 @@ func (r *EphemeralRunnerSetReconciler) SetupWithManager(mgr ctrl.Manager, opts . return builderWithOptions( ctrl.NewControllerManagedBy(mgr). - For(&v1alpha1.EphemeralRunnerSet{}). - Owns(&v1alpha1.EphemeralRunner{}). - WithEventFilter(predicate.ResourceVersionChangedPredicate{}), + For(&v1alpha1.EphemeralRunnerSet{}, builder.WithPredicates(ephemeralRunnerSetPrimaryPredicate())). + Owns(&v1alpha1.EphemeralRunner{}, builder.WithPredicates(ephemeralRunnerSetOwnedEphemeralRunnerPredicate())), opts, ).Complete(r) } +func ephemeralRunnerSetPrimaryPredicate() predicate.Predicate { + return predicate.Funcs{ + UpdateFunc: func(e event.UpdateEvent) bool { + if e.ObjectOld == nil || e.ObjectNew == nil { + return false + } + + return e.ObjectOld.GetGeneration() != e.ObjectNew.GetGeneration() || + !equalStringSlices(e.ObjectOld.GetFinalizers(), e.ObjectNew.GetFinalizers()) || + e.ObjectOld.GetDeletionTimestamp() != e.ObjectNew.GetDeletionTimestamp() + }, + } +} + +func ephemeralRunnerSetOwnedEphemeralRunnerPredicate() predicate.Predicate { + return predicate.Funcs{ + UpdateFunc: func(e event.UpdateEvent) bool { + oldRunner, oldOk := e.ObjectOld.(*v1alpha1.EphemeralRunner) + newRunner, newOk := e.ObjectNew.(*v1alpha1.EphemeralRunner) + if !oldOk || !newOk { + return false + } + + if oldRunner.GetGeneration() != newRunner.GetGeneration() || + !equalStringSlices(oldRunner.GetFinalizers(), newRunner.GetFinalizers()) || + oldRunner.GetDeletionTimestamp() != newRunner.GetDeletionTimestamp() { + return true + } + + return oldRunner.Status.Phase != newRunner.Status.Phase || + oldRunner.Status.RunnerID != newRunner.Status.RunnerID + }, + } +} + type ephemeralRunnerStepper struct { items []*v1alpha1.EphemeralRunner index int diff --git a/controllers/actions.github.com/ephemeralrunnerset_controller_test.go b/controllers/actions.github.com/ephemeralrunnerset_controller_test.go index 76cf98ab88..fed922b4fd 100644 --- a/controllers/actions.github.com/ephemeralrunnerset_controller_test.go +++ b/controllers/actions.github.com/ephemeralrunnerset_controller_test.go @@ -133,17 +133,20 @@ var _ = Describe("Test EphemeralRunnerSet controller", func() { var autoscalingNS *corev1.Namespace var ephemeralRunnerSet *v1alpha1.EphemeralRunnerSet var configSecret *corev1.Secret + var resourceCache *ResourceCache BeforeEach(func() { ctx = context.Background() autoscalingNS, mgr = createNamespace(GinkgoT(), k8sClient) configSecret = createDefaultSecret(GinkgoT(), k8sClient, autoscalingNS.Name) + resourceCache = newTestResourceCache() controller := &EphemeralRunnerSetReconciler{ Client: mgr.GetClient(), Scheme: mgr.GetScheme(), Log: logf.Log, ResourceBuilder: ResourceBuilder{ + ResourceCache: resourceCache, SecretResolver: secretresolver.New(mgr.GetClient(), fake.NewMultiClient( fake.WithClient( fake.NewClient( @@ -275,21 +278,6 @@ var _ = Describe("Test EphemeralRunnerSet controller", func() { ephemeralRunnerSetTestTimeout, ephemeralRunnerSetTestInterval, ).Should(BeEquivalentTo(5), "5 EphemeralRunner should be created") - - // Check if the status stays running - Eventually( - func() (v1alpha1.EphemeralRunnerSetPhase, error) { - runnerSet := new(v1alpha1.EphemeralRunnerSet) - err := k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunnerSet.Name, Namespace: ephemeralRunnerSet.Namespace}, runnerSet) - if err != nil { - return "", err - } - - return runnerSet.Status.Phase, nil - }, - ephemeralRunnerSetTestTimeout, - ephemeralRunnerSetTestInterval, - ).Should(BeEquivalentTo(v1alpha1.EphemeralRunnerSetPhaseRunning), "EphemeralRunnerSet status should be running") }) }) @@ -298,6 +286,8 @@ var _ = Describe("Test EphemeralRunnerSet controller", func() { created := new(v1alpha1.EphemeralRunnerSet) err := k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunnerSet.Name, Namespace: ephemeralRunnerSet.Namespace}, created) Expect(err).NotTo(HaveOccurred(), "failed to get EphemeralRunnerSet") + resourceCache.listenerPod.Upsert(created, &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "cached-runner-set-pod", Namespace: created.Namespace}}) + Expect(resourceCacheHasMainObjectEntries(resourceCache, created)).To(BeTrue(), "test setup should cache an EphemeralRunnerSet-owned resource") // Scale up the EphemeralRunnerSet updated := created.DeepCopy() @@ -374,6 +364,14 @@ var _ = Describe("Test EphemeralRunnerSet controller", func() { ephemeralRunnerSetTestTimeout, ephemeralRunnerSetTestInterval, ).Should(Succeed(), "EphemeralRunnerSet should be deleted") + + Eventually( + func() bool { + return resourceCacheHasMainObjectEntries(resourceCache, created) + }, + ephemeralRunnerSetTestTimeout, + ephemeralRunnerSetTestInterval, + ).Should(BeFalse(), "EphemeralRunnerSet-owned resources should be removed from cache after deletion") }) }) @@ -629,7 +627,7 @@ var _ = Describe("Test EphemeralRunnerSet controller", func() { // confirm they are not deleted runnerList = new(v1alpha1.EphemeralRunnerList) - Consistently( + Eventually( func() (int, error) { err := listEphemeralRunnersAndRemoveFinalizers(ctx, k8sClient, runnerList, ephemeralRunnerSet.Namespace) if err != nil { @@ -692,16 +690,37 @@ var _ = Describe("Test EphemeralRunnerSet controller", func() { Expect(err).NotTo(HaveOccurred(), "failed to update EphemeralRunnerSet") runnerList = new(v1alpha1.EphemeralRunnerList) - // We should have 3 runners, and have no Succeeded ones Eventually( - func() error { + func() (int, error) { err := listEphemeralRunnersAndRemoveFinalizers(ctx, k8sClient, runnerList, ephemeralRunnerSet.Namespace) if err != nil { - return err + return -1, err } - if len(runnerList.Items) != 3 { - return fmt.Errorf("Expected 3 runners, got %d", len(runnerList.Items)) + return len(runnerList.Items), nil + }, + ephemeralRunnerSetTestTimeout, + ephemeralRunnerSetTestInterval, + ).Should(BeEquivalentTo(1), "only the running EphemeralRunner should remain before listener confirms the larger desired count") + + ers = new(v1alpha1.EphemeralRunnerSet) + err = k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunnerSet.Name, Namespace: ephemeralRunnerSet.Namespace}, ers) + Expect(err).NotTo(HaveOccurred(), "failed to get EphemeralRunnerSet") + + updated = ers.DeepCopy() + updated.Spec.Replicas = 3 + updated.Spec.PatchID = 3 + + err = k8sClient.Patch(ctx, updated, client.MergeFrom(ers)) + Expect(err).NotTo(HaveOccurred(), "failed to update EphemeralRunnerSet") + + runnerList = new(v1alpha1.EphemeralRunnerList) + // We should have 3 runners, and have no Succeeded ones after listener confirms. + Eventually( + func() error { + err := listEphemeralRunnersAndRemoveFinalizers(ctx, k8sClient, runnerList, ephemeralRunnerSet.Namespace) + if err != nil { + return err } for _, runner := range runnerList.Items { @@ -710,6 +729,10 @@ var _ = Describe("Test EphemeralRunnerSet controller", func() { } } + if len(runnerList.Items) != 3 { + return fmt.Errorf("Expected 3 runners, got %d", len(runnerList.Items)) + } + return nil }, ephemeralRunnerSetTestTimeout, @@ -1019,7 +1042,7 @@ var _ = Describe("Test EphemeralRunnerSet controller", func() { } } - if succeeded != 1 && running != 1 { + if succeeded != 1 || running != 1 { return fmt.Errorf("Expected 1 runner in Succeeded and 1 in Running, got %d in Succeeded and %d in Running", succeeded, running) } @@ -1029,8 +1052,9 @@ var _ = Describe("Test EphemeralRunnerSet controller", func() { ephemeralRunnerSetTestInterval, ).Should(BeNil(), "1 EphemeralRunner should be in Succeeded and 1 in Running phase") - // Now, let's simulate replacement. The desired count is still 2. - // This simulates that we got 1 job assigned, and 1 job completed. + // Now, let's simulate the listener publishing a stale patch before it has + // accounted for the completed job. The controller should clean up the + // finished runner but not create a replacement for this patch. ers = new(v1alpha1.EphemeralRunnerSet) err = k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunnerSet.Name, Namespace: ephemeralRunnerSet.Namespace}, ers) @@ -1043,6 +1067,33 @@ var _ = Describe("Test EphemeralRunnerSet controller", func() { err = k8sClient.Patch(ctx, updated, client.MergeFrom(ers)) Expect(err).NotTo(HaveOccurred(), "failed to update EphemeralRunnerSet") + runnerList = new(v1alpha1.EphemeralRunnerList) + Consistently( + func() (int, error) { + err := listEphemeralRunnersAndRemoveFinalizers(ctx, k8sClient, runnerList, ephemeralRunnerSet.Namespace) + if err != nil { + return -1, err + } + + return len(runnerList.Items), nil + }, + 2*time.Second, + ephemeralRunnerSetTestInterval, + ).Should(BeEquivalentTo(1), "only the running EphemeralRunner should remain before listener confirms replacement") + + // A fresh listener decision with the same desired count confirms that a + // replacement is still needed. + ers = new(v1alpha1.EphemeralRunnerSet) + err = k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunnerSet.Name, Namespace: ephemeralRunnerSet.Namespace}, ers) + Expect(err).NotTo(HaveOccurred(), "failed to get EphemeralRunnerSet") + + updated = ers.DeepCopy() + updated.Spec.Replicas = 2 + updated.Spec.PatchID = 3 + + err = k8sClient.Patch(ctx, updated, client.MergeFrom(ers)) + Expect(err).NotTo(HaveOccurred(), "failed to update EphemeralRunnerSet") + runnerList = new(v1alpha1.EphemeralRunnerList) Eventually( func() error { @@ -1068,6 +1119,107 @@ var _ = Describe("Test EphemeralRunnerSet controller", func() { ).Should(BeNil(), "2 EphemeralRunner should be created and none should be in Succeeded phase") }) + It("Should not create a replacement when a runner finishes ahead of the listener decrement patch", func() { + ers := new(v1alpha1.EphemeralRunnerSet) + err := k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunnerSet.Name, Namespace: ephemeralRunnerSet.Namespace}, ers) + Expect(err).NotTo(HaveOccurred(), "failed to get EphemeralRunnerSet") + + updated := ers.DeepCopy() + updated.Spec.Replicas = 4 + updated.Spec.PatchID = 1 + + err = k8sClient.Patch(ctx, updated, client.MergeFrom(ers)) + Expect(err).NotTo(HaveOccurred(), "failed to update EphemeralRunnerSet") + + runnerList := new(v1alpha1.EphemeralRunnerList) + Eventually( + func() (int, error) { + err := listEphemeralRunnersAndRemoveFinalizers(ctx, k8sClient, runnerList, ephemeralRunnerSet.Namespace) + if err != nil { + return -1, err + } + + return len(runnerList.Items), nil + }, + ephemeralRunnerSetTestTimeout, + ephemeralRunnerSetTestInterval, + ).Should(BeEquivalentTo(4), "4 EphemeralRunner should be created") + + for i := range 3 { + updatedRunner := runnerList.Items[i].DeepCopy() + updatedRunner.Status.Phase = v1alpha1.EphemeralRunnerPhaseRunning + err = k8sClient.Status().Patch(ctx, updatedRunner, client.MergeFrom(&runnerList.Items[i])) + Expect(err).NotTo(HaveOccurred(), "failed to update EphemeralRunner") + } + + updatedRunner := runnerList.Items[3].DeepCopy() + updatedRunner.Status.Phase = v1alpha1.EphemeralRunnerPhaseSucceeded + err = k8sClient.Status().Patch(ctx, updatedRunner, client.MergeFrom(&runnerList.Items[3])) + Expect(err).NotTo(HaveOccurred(), "failed to update EphemeralRunner") + + ers = new(v1alpha1.EphemeralRunnerSet) + err = k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunnerSet.Name, Namespace: ephemeralRunnerSet.Namespace}, ers) + Expect(err).NotTo(HaveOccurred(), "failed to get EphemeralRunnerSet") + + updated = ers.DeepCopy() + updated.Spec.Replicas = 4 + updated.Spec.PatchID = 2 + + err = k8sClient.Patch(ctx, updated, client.MergeFrom(ers)) + Expect(err).NotTo(HaveOccurred(), "failed to update EphemeralRunnerSet") + + Consistently( + func() (int, error) { + err := listEphemeralRunnersAndRemoveFinalizers(ctx, k8sClient, runnerList, ephemeralRunnerSet.Namespace) + if err != nil { + return -1, err + } + + return len(runnerList.Items), nil + }, + ephemeralRunnerSetTestTimeout, + ephemeralRunnerSetTestInterval, + ).Should(BeEquivalentTo(3), "only the running EphemeralRunners should remain after stale-patch cleanup") + + Consistently( + func() (int, error) { + err := listEphemeralRunnersAndRemoveFinalizers(ctx, k8sClient, runnerList, ephemeralRunnerSet.Namespace) + if err != nil { + return -1, err + } + + return len(runnerList.Items), nil + }, + 12*time.Second, + ephemeralRunnerSetTestInterval, + ).Should(BeEquivalentTo(3), "EphemeralRunnerSet should not create a replacement before listener decrements") + + ers = new(v1alpha1.EphemeralRunnerSet) + err = k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunnerSet.Name, Namespace: ephemeralRunnerSet.Namespace}, ers) + Expect(err).NotTo(HaveOccurred(), "failed to get EphemeralRunnerSet") + + updated = ers.DeepCopy() + updated.Spec.Replicas = 3 + updated.Spec.PatchID = 3 + + err = k8sClient.Patch(ctx, updated, client.MergeFrom(ers)) + Expect(err).NotTo(HaveOccurred(), "failed to update EphemeralRunnerSet") + + runnerList = new(v1alpha1.EphemeralRunnerList) + Eventually( + func() (int, error) { + err := listEphemeralRunnersAndRemoveFinalizers(ctx, k8sClient, runnerList, ephemeralRunnerSet.Namespace) + if err != nil { + return -1, err + } + + return len(runnerList.Items), nil + }, + ephemeralRunnerSetTestTimeout, + ephemeralRunnerSetTestInterval, + ).Should(BeEquivalentTo(3), "EphemeralRunnerSet should converge after listener decrements") + }) + It("Should delete idle runners, keep busy runners, and create new runners when the spec changes", func() { ers := new(v1alpha1.EphemeralRunnerSet) err := k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunnerSet.Name, Namespace: ephemeralRunnerSet.Namespace}, ers) @@ -1094,7 +1246,7 @@ var _ = Describe("Test EphemeralRunnerSet controller", func() { ).Should(BeEquivalentTo(3), "3 EphemeralRunner should be created") idleRunnerNames := map[string]struct{}{} - for i := 0; i < 2; i++ { + for i := range 2 { idleRunner := runnerList.Items[i].DeepCopy() idleRunner.Status.Phase = v1alpha1.EphemeralRunnerPhaseRunning idleRunner.Status.RunnerID = i + 101 @@ -1119,6 +1271,7 @@ var _ = Describe("Test EphemeralRunnerSet controller", func() { updated = ers.DeepCopy() updated.Spec.EphemeralRunnerSpec.PodTemplateSpec.Spec.Containers[0].Image = "ghcr.io/actions/runner:new" + updated.Spec.ActionableRevision = ers.Spec.ActionableRevision + 1 err = k8sClient.Patch(ctx, updated, client.MergeFrom(ers)) Expect(err).NotTo(HaveOccurred(), "failed to patch EphemeralRunnerSet with new spec") @@ -1378,6 +1531,7 @@ var _ = Describe("EphemeralRunner phase metrics", func() { Log: logf.Log, PublishMetrics: true, ResourceBuilder: ResourceBuilder{ + ResourceCache: newTestResourceCache(), SecretResolver: secretresolver.New(k8sClient, fake.NewMultiClient( fake.WithClient( fake.NewClient( @@ -1465,6 +1619,14 @@ var _ = Describe("EphemeralRunner phase metrics", func() { err = k8sClient.Status().Patch(ctx, podRunning, client.MergeFrom(podPending)) Expect(err).NotTo(HaveOccurred(), "failed to patch pod to running") + runnerRunning := new(v1alpha1.EphemeralRunner) + err = k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, runnerRunning) + Expect(err).NotTo(HaveOccurred(), "failed to get ephemeral runner before listener-owned running patch") + runnerRunningOriginal := runnerRunning.DeepCopy() + runnerRunning.Status.Phase = v1alpha1.EphemeralRunnerPhaseRunning + err = k8sClient.Status().Patch(ctx, runnerRunning, client.MergeFrom(runnerRunningOriginal)) + Expect(err).NotTo(HaveOccurred(), "failed to simulate listener running phase patch") + _, err = controller.Reconcile(ctx, request) Expect(err).NotTo(HaveOccurred(), "failed to reconcile running pod") expectEphemeralRunnerPhase(ctx, ephemeralRunner, v1alpha1.EphemeralRunnerPhaseRunning) @@ -1492,6 +1654,489 @@ var _ = Describe("EphemeralRunner phase metrics", func() { }) }) +var _ = Describe("Test EphemeralRunnerSet actionable revision cleanup", func() { + var ctx context.Context + var mgr ctrl.Manager + var autoscalingNS *corev1.Namespace + var configSecret *corev1.Secret + + newRunner := func(name string, ers *v1alpha1.EphemeralRunnerSet) *v1alpha1.EphemeralRunner { + controllerRef := true + return &v1alpha1.EphemeralRunner{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: ers.Namespace, + OwnerReferences: []metav1.OwnerReference{{ + APIVersion: v1alpha1.GroupVersion.String(), + Kind: "EphemeralRunnerSet", + Name: ers.Name, + UID: ers.UID, + Controller: &controllerRef, + }}, + }, + Spec: ers.Spec.EphemeralRunnerSpec, + } + } + + waitForCachedActionableRevision := func(controller *EphemeralRunnerSetReconciler, key types.NamespacedName, specRevision, appliedRevision int64) { + Eventually(func(g Gomega) { + cached := new(v1alpha1.EphemeralRunnerSet) + g.Expect(controller.Get(ctx, key, cached)).To(Succeed()) + g.Expect(cached.Spec.ActionableRevision).To(Equal(specRevision)) + g.Expect(cached.Status.AppliedActionableRevision).To(Equal(appliedRevision)) + g.Expect(cached.Finalizers).To(ContainElement(EphemeralRunnerSetFinalizerName)) + }, ephemeralRunnerSetTestTimeout, ephemeralRunnerSetTestInterval).Should(Succeed()) + } + + waitForCachedRunningRunners := func(controller *EphemeralRunnerSetReconciler, namespace, owner string, expected int) { + Eventually(func(g Gomega) { + runners := new(v1alpha1.EphemeralRunnerList) + g.Expect(controller.List(ctx, runners, client.InNamespace(namespace), client.MatchingFields{resourceOwnerKey: owner})).To(Succeed()) + state := newEphemeralRunnersByStates(runners) + g.Expect(state.running).To(HaveLen(expected)) + for _, runner := range state.running { + g.Expect(runner.Status.RunnerID).NotTo(BeZero()) + } + }, ephemeralRunnerSetTestTimeout, ephemeralRunnerSetTestInterval).Should(Succeed()) + } + + BeforeEach(func() { + ctx = context.Background() + autoscalingNS, mgr = createNamespace(GinkgoT(), k8sClient) + configSecret = createDefaultSecret(GinkgoT(), k8sClient, autoscalingNS.Name) + 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(), + 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-success", Namespace: autoscalingNS.Name}, + Spec: v1alpha1.EphemeralRunnerSetSpec{ + ActionableRevision: 3, + 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()) + + current := new(v1alpha1.EphemeralRunnerSet) + err = k8sClient.Get(ctx, request.NamespacedName, current) + Expect(err).NotTo(HaveOccurred()) + + statusUpdated := current.DeepCopy() + statusUpdated.Status.AppliedActionableRevision = 3 + statusUpdated.Status.Phase = v1alpha1.EphemeralRunnerSetPhaseRunning + err = k8sClient.Status().Patch(ctx, statusUpdated, client.MergeFrom(current)) + Expect(err).NotTo(HaveOccurred()) + + idleRunner := newRunner("runner-a-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 = 101 + err = k8sClient.Status().Patch(ctx, idleUpdated, client.MergeFrom(idleCurrent)) + Expect(err).NotTo(HaveOccurred()) + + busyRunner := newRunner("runner-b-busy", statusUpdated) + err = k8sClient.Create(ctx, busyRunner) + Expect(err).NotTo(HaveOccurred()) + + busyCurrent := new(v1alpha1.EphemeralRunner) + err = k8sClient.Get(ctx, client.ObjectKeyFromObject(busyRunner), busyCurrent) + Expect(err).NotTo(HaveOccurred()) + busyUpdated := busyCurrent.DeepCopy() + busyUpdated.Status.Phase = v1alpha1.EphemeralRunnerPhaseRunning + busyUpdated.Status.RunnerID = 102 + busyUpdated.Status.JobID = "job-1" + busyUpdated.Status.WorkflowRunID = 9001 + err = k8sClient.Status().Patch(ctx, busyUpdated, client.MergeFrom(busyCurrent)) + Expect(err).NotTo(HaveOccurred()) + + err = k8sClient.Get(ctx, request.NamespacedName, current) + Expect(err).NotTo(HaveOccurred()) + specUpdated := current.DeepCopy() + specUpdated.Spec.ActionableRevision = 4 + err = k8sClient.Patch(ctx, specUpdated, client.MergeFrom(current)) + Expect(err).NotTo(HaveOccurred()) + + Eventually(func() bool { + _, err := controller.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + + runner := new(v1alpha1.EphemeralRunner) + return kerrors.IsNotFound(k8sClient.Get(ctx, types.NamespacedName{Namespace: autoscalingNS.Name, Name: "runner-a-idle"}, runner)) + }, ephemeralRunnerSetTestTimeout, ephemeralRunnerSetTestInterval).Should(BeTrue()) + + Consistently(func() error { + runner := new(v1alpha1.EphemeralRunner) + if err := k8sClient.Get(ctx, types.NamespacedName{Namespace: autoscalingNS.Name, Name: "runner-b-busy"}, runner); err != nil { + return err + } + if runner.Status.RunnerID != 102 { + return fmt.Errorf("expected busy runner ID 102, got %d", runner.Status.RunnerID) + } + if !runner.HasJob() { + return fmt.Errorf("expected runner-b-busy to keep its assigned job") + } + return nil + }, time.Second, ephemeralRunnerSetTestInterval).Should(Succeed()) + + Eventually(func() int64 { + _, err := controller.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + + updatedSet := new(v1alpha1.EphemeralRunnerSet) + if err := k8sClient.Get(ctx, request.NamespacedName, updatedSet); err != nil { + return 0 + } + return updatedSet.Status.AppliedActionableRevision + }, ephemeralRunnerSetTestTimeout, ephemeralRunnerSetTestInterval).Should(Equal(int64(4))) + }) + + It("keeps applied actionable revision at 3 when cleanup fails", 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(fmt.Errorf("remove failed")))), + )), + }, + } + + ephemeralRunnerSet := &v1alpha1.EphemeralRunnerSet{ + ObjectMeta: metav1.ObjectMeta{Name: "test-actionable-revision-error", Namespace: autoscalingNS.Name}, + Spec: v1alpha1.EphemeralRunnerSetSpec{ + ActionableRevision: 3, + 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()) + + current := new(v1alpha1.EphemeralRunnerSet) + err = k8sClient.Get(ctx, request.NamespacedName, current) + Expect(err).NotTo(HaveOccurred()) + + statusUpdated := current.DeepCopy() + statusUpdated.Status.AppliedActionableRevision = 3 + err = k8sClient.Status().Patch(ctx, statusUpdated, client.MergeFrom(current)) + Expect(err).NotTo(HaveOccurred()) + + idleRunner := newRunner("runner-a-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 = 101 + err = k8sClient.Status().Patch(ctx, idleUpdated, client.MergeFrom(idleCurrent)) + Expect(err).NotTo(HaveOccurred()) + + err = k8sClient.Get(ctx, request.NamespacedName, current) + Expect(err).NotTo(HaveOccurred()) + specUpdated := current.DeepCopy() + specUpdated.Spec.ActionableRevision = 4 + err = k8sClient.Patch(ctx, specUpdated, client.MergeFrom(current)) + Expect(err).NotTo(HaveOccurred()) + + waitForCachedActionableRevision(controller, request.NamespacedName, 4, 3) + waitForCachedRunningRunners(controller, autoscalingNS.Name, ephemeralRunnerSet.Name, 1) + + _, err = controller.Reconcile(ctx, request) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("remove failed")) + + Consistently(func() int64 { + updatedSet := new(v1alpha1.EphemeralRunnerSet) + if err := k8sClient.Get(ctx, request.NamespacedName, updatedSet); err != nil { + return 0 + } + return updatedSet.Status.AppliedActionableRevision + }, time.Second, ephemeralRunnerSetTestInterval).Should(Equal(int64(3))) + }) + + It("deletes unregistered pending runner during actionable revision cleanup after restart with no cache", func() { + controller := &EphemeralRunnerSetReconciler{ + Client: mgr.GetClient(), + Scheme: mgr.GetScheme(), + Log: logf.Log, + ResourceBuilder: ResourceBuilder{ + ResourceCache: newTestResourceCache(), // fresh empty cache simulating restart + SecretResolver: secretresolver.New(mgr.GetClient(), fake.NewMultiClient()), + }, + } + + ephemeralRunnerSet := &v1alpha1.EphemeralRunnerSet{ + ObjectMeta: metav1.ObjectMeta{Name: "test-restart-no-cache", Namespace: autoscalingNS.Name}, + Spec: v1alpha1.EphemeralRunnerSetSpec{ + ActionableRevision: 4, // spec has been bumped + 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:updated"}}}}, + }, + }, + } + + 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()) + + current := new(v1alpha1.EphemeralRunnerSet) + err = k8sClient.Get(ctx, request.NamespacedName, current) + Expect(err).NotTo(HaveOccurred()) + + statusUpdated := current.DeepCopy() + statusUpdated.Status.AppliedActionableRevision = 3 // status is behind + err = k8sClient.Status().Patch(ctx, statusUpdated, client.MergeFrom(current)) + Expect(err).NotTo(HaveOccurred()) + + pendingRunner := newRunner("runner-restart-pending", statusUpdated) + err = k8sClient.Create(ctx, pendingRunner) + Expect(err).NotTo(HaveOccurred()) + + Eventually(func(g Gomega) { + cachedSet := new(v1alpha1.EphemeralRunnerSet) + err := controller.Get(ctx, request.NamespacedName, cachedSet) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(cachedSet.Status.AppliedActionableRevision).To(Equal(int64(3))) + + cachedRunner := new(v1alpha1.EphemeralRunner) + err = controller.Get(ctx, types.NamespacedName{Namespace: autoscalingNS.Name, Name: "runner-restart-pending"}, cachedRunner) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(cachedRunner.Status.RunnerID).To(BeZero()) + g.Expect(cachedRunner.Status.Phase).To(BeEmpty()) + }, ephemeralRunnerSetTestTimeout, ephemeralRunnerSetTestInterval).Should(Succeed()) + + // Reconcile with fresh cache (simulating restart). Actionable revision cleanup deletes pending runners. + Eventually(func() bool { + _, err := controller.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + + runner := new(v1alpha1.EphemeralRunner) + return kerrors.IsNotFound(k8sClient.Get(ctx, types.NamespacedName{Namespace: autoscalingNS.Name, Name: "runner-restart-pending"}, runner)) + }, ephemeralRunnerSetTestTimeout, ephemeralRunnerSetTestInterval).Should(BeTrue()) + + // AppliedActionableRevision should advance after cleanup completes. + Eventually(func() int64 { + _, err := controller.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + + updatedSet := new(v1alpha1.EphemeralRunnerSet) + if err := k8sClient.Get(ctx, request.NamespacedName, updatedSet); err != nil { + return 0 + } + return updatedSet.Status.AppliedActionableRevision + }, ephemeralRunnerSetTestTimeout, ephemeralRunnerSetTestInterval).Should(Equal(int64(4))) + }) + + It("preserves AppliedActionableRevision during status-only phase updates", 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()), + )), + }, + } + + // Setup: Create ERS with an actionable revision + ephemeralRunnerSet := &v1alpha1.EphemeralRunnerSet{ + ObjectMeta: metav1.ObjectMeta{Name: "test-preserve-applied-revision", Namespace: autoscalingNS.Name}, + Spec: v1alpha1.EphemeralRunnerSetSpec{ + ActionableRevision: 5, + 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()) + + // Set AppliedActionableRevision to 5 + current := new(v1alpha1.EphemeralRunnerSet) + err = k8sClient.Get(ctx, request.NamespacedName, current) + Expect(err).NotTo(HaveOccurred()) + + statusUpdated := current.DeepCopy() + statusUpdated.Status.AppliedActionableRevision = 5 + statusUpdated.Status.Phase = v1alpha1.EphemeralRunnerSetPhaseRunning + err = k8sClient.Status().Patch(ctx, statusUpdated, client.MergeFrom(current)) + Expect(err).NotTo(HaveOccurred()) + + // Create a runner that will cause phase change (outdated runner) + ephemeralRunner := &v1alpha1.EphemeralRunner{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-runner-outdated", + Namespace: autoscalingNS.Name, + Labels: map[string]string{ + LabelKeyGitHubScaleSetName: ephemeralRunnerSet.Name, + LabelKeyGitHubScaleSetNamespace: ephemeralRunnerSet.Namespace, + }, + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: v1alpha1.GroupVersion.String(), + Kind: "EphemeralRunnerSet", + Name: ephemeralRunnerSet.Name, + UID: ephemeralRunnerSet.UID, + Controller: func(b bool) *bool { return &b }(true), + BlockOwnerDeletion: func(b bool) *bool { return &b }(true), + }, + }, + }, + Spec: 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:old"}}}}, + }, + } + err = k8sClient.Create(ctx, ephemeralRunner) + Expect(err).NotTo(HaveOccurred()) + + runnerStatusUpdated := ephemeralRunner.DeepCopy() + runnerStatusUpdated.Status.Phase = v1alpha1.EphemeralRunnerPhaseOutdated + runnerStatusUpdated.Status.RunnerID = 123 + runnerStatusUpdated.Status.JobRequestID = 456 + err = k8sClient.Status().Patch(ctx, runnerStatusUpdated, client.MergeFrom(ephemeralRunner)) + Expect(err).NotTo(HaveOccurred()) + + Eventually(func(g Gomega) { + cachedSet := new(v1alpha1.EphemeralRunnerSet) + err := controller.Get(ctx, request.NamespacedName, cachedSet) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(cachedSet.Status.AppliedActionableRevision).To(Equal(int64(5))) + + cachedRunner := new(v1alpha1.EphemeralRunner) + err = controller.Get(ctx, types.NamespacedName{Namespace: autoscalingNS.Name, Name: "test-runner-outdated"}, cachedRunner) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(cachedRunner.Status.Phase).To(Equal(v1alpha1.EphemeralRunnerPhaseOutdated)) + }, ephemeralRunnerSetTestTimeout, ephemeralRunnerSetTestInterval).Should(Succeed()) + + // Verify: Phase changed to Outdated, but AppliedActionableRevision preserved + Eventually(func(g Gomega) { + _, err := controller.Reconcile(ctx, request) + g.Expect(err).NotTo(HaveOccurred()) + + updatedSet := new(v1alpha1.EphemeralRunnerSet) + err = k8sClient.Get(ctx, request.NamespacedName, updatedSet) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(updatedSet.Status.Phase).To(Equal(v1alpha1.EphemeralRunnerSetPhaseOutdated), "phase should change to Outdated") + g.Expect(updatedSet.Status.AppliedActionableRevision).To(Equal(int64(5)), "AppliedActionableRevision should be preserved") + }, ephemeralRunnerSetTestTimeout, ephemeralRunnerSetTestInterval).Should(Succeed()) + }) +}) + var _ = Describe("Test EphemeralRunnerSet controller with proxy settings", func() { var ctx context.Context var mgr ctrl.Manager @@ -1509,6 +2154,7 @@ var _ = Describe("Test EphemeralRunnerSet controller with proxy settings", func( Scheme: mgr.GetScheme(), Log: logf.Log, ResourceBuilder: ResourceBuilder{ + ResourceCache: newTestResourceCache(), SecretResolver: secretresolver.New(mgr.GetClient(), multiclient.NewScaleset()), }, } @@ -1827,6 +2473,7 @@ var _ = Describe("Test EphemeralRunnerSet controller with custom root CA", func( Scheme: mgr.GetScheme(), Log: logf.Log, ResourceBuilder: ResourceBuilder{ + ResourceCache: newTestResourceCache(), SecretResolver: secretresolver.New(mgr.GetClient(), multiclient.NewScaleset()), }, } diff --git a/controllers/actions.github.com/helpers.go b/controllers/actions.github.com/helpers.go new file mode 100644 index 0000000000..72188ae631 --- /dev/null +++ b/controllers/actions.github.com/helpers.go @@ -0,0 +1,41 @@ +package actionsgithubcom + +import ( + "github.com/actions/actions-runner-controller/apis/actions.github.com/v1alpha1" + "github.com/google/go-cmp/cmp" + corev1 "k8s.io/api/core/v1" + apiequality "k8s.io/apimachinery/pkg/api/equality" +) + +var ( + _ = ephemeralRunnerSetActionableSpecChanged + _ = nextActionableRevision +) + +func ephemeralRunnerSetActionableSpecChanged(current, desired *v1alpha1.EphemeralRunnerSet) bool { + if current == nil || desired == nil { + return current != desired + } + + return !cmp.Equal(current.Spec.EphemeralRunnerSpec, desired.Spec.EphemeralRunnerSpec) +} + +func nextActionableRevision(current *v1alpha1.EphemeralRunnerSet) int64 { + if current == nil { + return 1 + } + + if current.Spec.ActionableRevision > current.Status.AppliedActionableRevision { + return current.Spec.ActionableRevision + 1 + } + + return current.Status.AppliedActionableRevision + 1 +} + +func listenerPodSpecRequiresRecreation(current, desired *corev1.Pod) bool { + if current == nil || desired == nil { + return current != desired + } + + return !apiequality.Semantic.DeepDerivative(desired.Spec, current.Spec) +} diff --git a/controllers/actions.github.com/predicate_helpers.go b/controllers/actions.github.com/predicate_helpers.go new file mode 100644 index 0000000000..7dfff27eae --- /dev/null +++ b/controllers/actions.github.com/predicate_helpers.go @@ -0,0 +1,14 @@ +package actionsgithubcom + +func equalStringSlices(a, b []string) bool { + if len(a) != len(b) { + return false + } + + for i := range a { + if a[i] != b[i] { + return false + } + } + return true +} diff --git a/controllers/actions.github.com/predicate_helpers_test.go b/controllers/actions.github.com/predicate_helpers_test.go new file mode 100644 index 0000000000..28140e5aa8 --- /dev/null +++ b/controllers/actions.github.com/predicate_helpers_test.go @@ -0,0 +1,189 @@ +package actionsgithubcom + +import ( + "testing" + "time" + + "github.com/actions/actions-runner-controller/apis/actions.github.com/v1alpha1" + "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/event" +) + +func TestEphemeralRunnerPrimaryPredicate(t *testing.T) { + g := gomega.NewWithT(t) + predicate := ephemeralRunnerPrimaryPredicate() + runner := &v1alpha1.EphemeralRunner{} + + g.Expect(predicate.Create(event.CreateEvent{Object: runner})).To(gomega.BeTrue()) + g.Expect(predicate.Delete(event.DeleteEvent{Object: runner})).To(gomega.BeTrue()) + + oldRunner := &v1alpha1.EphemeralRunner{ObjectMeta: metav1.ObjectMeta{Generation: 1}} + newRunner := oldRunner.DeepCopy() + newRunner.Generation = 2 + g.Expect(predicate.Update(event.UpdateEvent{ObjectOld: oldRunner, ObjectNew: newRunner})).To(gomega.BeTrue()) + + newRunner = oldRunner.DeepCopy() + newRunner.Finalizers = []string{ephemeralRunnerFinalizerName} + g.Expect(predicate.Update(event.UpdateEvent{ObjectOld: oldRunner, ObjectNew: newRunner})).To(gomega.BeTrue()) + + newRunner = oldRunner.DeepCopy() + deletionTimestamp := metav1.NewTime(time.Now()) + newRunner.DeletionTimestamp = &deletionTimestamp + g.Expect(predicate.Update(event.UpdateEvent{ObjectOld: oldRunner, ObjectNew: newRunner})).To(gomega.BeTrue()) + + newRunner = oldRunner.DeepCopy() + newRunner.Status.JobRequestID = 123 + newRunner.Status.JobID = "job-id" + newRunner.Status.JobRepositoryName = "owner/repo" + newRunner.Status.JobWorkflowRef = "owner/repo/.github/workflows/ci.yaml@refs/heads/main" + newRunner.Status.WorkflowRunID = 456 + newRunner.Status.JobDisplayName = "build" + g.Expect(predicate.Update(event.UpdateEvent{ObjectOld: oldRunner, ObjectNew: newRunner})).To(gomega.BeFalse()) +} + +func TestEphemeralRunnerSetPrimaryPredicate(t *testing.T) { + g := gomega.NewWithT(t) + predicate := ephemeralRunnerSetPrimaryPredicate() + runnerSet := &v1alpha1.EphemeralRunnerSet{} + + g.Expect(predicate.Create(event.CreateEvent{Object: runnerSet})).To(gomega.BeTrue()) + g.Expect(predicate.Delete(event.DeleteEvent{Object: runnerSet})).To(gomega.BeTrue()) + + oldRunnerSet := &v1alpha1.EphemeralRunnerSet{ObjectMeta: metav1.ObjectMeta{Generation: 1}} + newRunnerSet := oldRunnerSet.DeepCopy() + newRunnerSet.Generation = 2 + g.Expect(predicate.Update(event.UpdateEvent{ObjectOld: oldRunnerSet, ObjectNew: newRunnerSet})).To(gomega.BeTrue()) + + newRunnerSet = oldRunnerSet.DeepCopy() + newRunnerSet.Status.Phase = v1alpha1.EphemeralRunnerSetPhaseRunning + newRunnerSet.Status.AppliedActionableRevision = 2 + newRunnerSet.Status.FinishedRunnerCleanupPatchID = 3 + g.Expect(predicate.Update(event.UpdateEvent{ObjectOld: oldRunnerSet, ObjectNew: newRunnerSet})).To(gomega.BeFalse()) +} + +func TestEphemeralRunnerSetOwnedEphemeralRunnerPredicate(t *testing.T) { + g := gomega.NewWithT(t) + predicate := ephemeralRunnerSetOwnedEphemeralRunnerPredicate() + oldRunner := &v1alpha1.EphemeralRunner{ObjectMeta: metav1.ObjectMeta{Generation: 1}} + + g.Expect(predicate.Create(event.CreateEvent{Object: oldRunner})).To(gomega.BeTrue()) + g.Expect(predicate.Delete(event.DeleteEvent{Object: oldRunner})).To(gomega.BeTrue()) + + newRunner := oldRunner.DeepCopy() + newRunner.Generation = 2 + g.Expect(predicate.Update(event.UpdateEvent{ObjectOld: oldRunner, ObjectNew: newRunner})).To(gomega.BeTrue()) + + newRunner = oldRunner.DeepCopy() + newRunner.Status.Phase = v1alpha1.EphemeralRunnerPhaseRunning + g.Expect(predicate.Update(event.UpdateEvent{ObjectOld: oldRunner, ObjectNew: newRunner})).To(gomega.BeTrue()) + + newRunner = oldRunner.DeepCopy() + newRunner.Status.RunnerID = 123 + g.Expect(predicate.Update(event.UpdateEvent{ObjectOld: oldRunner, ObjectNew: newRunner})).To(gomega.BeTrue()) + + newRunner = oldRunner.DeepCopy() + newRunner.Status.JobRequestID = 123 + newRunner.Status.JobID = "job-id" + g.Expect(predicate.Update(event.UpdateEvent{ObjectOld: oldRunner, ObjectNew: newRunner})).To(gomega.BeFalse()) +} + +func TestAutoscalingRunnerSetOwnedEphemeralRunnerSetPredicate(t *testing.T) { + g := gomega.NewWithT(t) + predicate := autoscalingRunnerSetOwnedEphemeralRunnerSetPredicate() + oldRunnerSet := &v1alpha1.EphemeralRunnerSet{ + ObjectMeta: metav1.ObjectMeta{ + Generation: 1, + ResourceVersion: "1000", + }, + Spec: v1alpha1.EphemeralRunnerSetSpec{ + Replicas: 2, + PatchID: 1, + }, + } + + g.Expect(predicate.Create(event.CreateEvent{Object: oldRunnerSet})).To(gomega.BeTrue()) + g.Expect(predicate.Delete(event.DeleteEvent{Object: oldRunnerSet})).To(gomega.BeTrue()) + + newRunnerSet := oldRunnerSet.DeepCopy() + newRunnerSet.Spec.PatchID = 2 + g.Expect(predicate.Update(event.UpdateEvent{ObjectOld: oldRunnerSet, ObjectNew: newRunnerSet})).To(gomega.BeFalse()) + + newRunnerSet = oldRunnerSet.DeepCopy() + newRunnerSet.ResourceVersion = "1001" + g.Expect(predicate.Update(event.UpdateEvent{ObjectOld: oldRunnerSet, ObjectNew: newRunnerSet})).To(gomega.BeFalse()) + + newRunnerSet = oldRunnerSet.DeepCopy() + newRunnerSet.Spec.Replicas = 3 + g.Expect(predicate.Update(event.UpdateEvent{ObjectOld: oldRunnerSet, ObjectNew: newRunnerSet})).To(gomega.BeTrue()) + + newRunnerSet = oldRunnerSet.DeepCopy() + newRunnerSet.Spec.ActionableRevision = 1 + g.Expect(predicate.Update(event.UpdateEvent{ObjectOld: oldRunnerSet, ObjectNew: newRunnerSet})).To(gomega.BeTrue()) + + newRunnerSet = oldRunnerSet.DeepCopy() + newRunnerSet.Status.Phase = v1alpha1.EphemeralRunnerSetPhaseRunning + g.Expect(predicate.Update(event.UpdateEvent{ObjectOld: oldRunnerSet, ObjectNew: newRunnerSet})).To(gomega.BeTrue()) + + newRunnerSet = oldRunnerSet.DeepCopy() + newRunnerSet.Finalizers = []string{"test-finalizer"} + g.Expect(predicate.Update(event.UpdateEvent{ObjectOld: oldRunnerSet, ObjectNew: newRunnerSet})).To(gomega.BeTrue()) + + newRunnerSet = oldRunnerSet.DeepCopy() + deletionTimestamp := metav1.NewTime(time.Now()) + newRunnerSet.DeletionTimestamp = &deletionTimestamp + g.Expect(predicate.Update(event.UpdateEvent{ObjectOld: oldRunnerSet, ObjectNew: newRunnerSet})).To(gomega.BeTrue()) +} + +func TestEphemeralRunnerOwnedPodPredicate(t *testing.T) { + g := gomega.NewWithT(t) + predicate := ephemeralRunnerOwnedPodPredicate() + basePod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pod", + ResourceVersion: "1000", + }, + Status: corev1.PodStatus{ + Phase: corev1.PodPending, + }, + } + + g.Expect(predicate.Create(event.CreateEvent{Object: basePod})).To(gomega.BeTrue()) + g.Expect(predicate.Delete(event.DeleteEvent{Object: basePod})).To(gomega.BeTrue()) + + updatedPod := basePod.DeepCopy() + updatedPod.ResourceVersion = "1001" + g.Expect(predicate.Update(event.UpdateEvent{ObjectOld: basePod, ObjectNew: updatedPod})).To(gomega.BeFalse()) + + updatedPod = basePod.DeepCopy() + updatedPod.Status.Phase = corev1.PodRunning + g.Expect(predicate.Update(event.UpdateEvent{ObjectOld: basePod, ObjectNew: updatedPod})).To(gomega.BeTrue()) + + updatedPod = basePod.DeepCopy() + updatedPod.Status.ContainerStatuses = []corev1.ContainerStatus{ + { + Name: v1alpha1.EphemeralRunnerContainerName, + State: corev1.ContainerState{ + Running: &corev1.ContainerStateRunning{}, + }, + }, + } + g.Expect(predicate.Update(event.UpdateEvent{ObjectOld: basePod, ObjectNew: updatedPod})).To(gomega.BeTrue()) + + updatedPod = basePod.DeepCopy() + updatedPod.Status.InitContainerStatuses = []corev1.ContainerStatus{ + { + Name: "setup", + State: corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{ExitCode: 1}, + }, + }, + } + g.Expect(predicate.Update(event.UpdateEvent{ObjectOld: basePod, ObjectNew: updatedPod})).To(gomega.BeTrue()) + + updatedPod = basePod.DeepCopy() + deletionTimestamp := metav1.Now() + updatedPod.DeletionTimestamp = &deletionTimestamp + g.Expect(predicate.Update(event.UpdateEvent{ObjectOld: basePod, ObjectNew: updatedPod})).To(gomega.BeTrue()) +} diff --git a/controllers/actions.github.com/resourcebuilder.go b/controllers/actions.github.com/resourcebuilder.go index fd456b56f1..8c85a3c596 100644 --- a/controllers/actions.github.com/resourcebuilder.go +++ b/controllers/actions.github.com/resourcebuilder.go @@ -46,15 +46,6 @@ var commonLabelKeys = [...]string{ LabelKeyGitHubRepository, } -// annotationKeyIntegrityHash is used as a hash of the important fields -// of each resource to determine if more drastic action should be taken. -// -// For example, annotations/labels are not something that should modify -// the behavior of a resource, while the change in spec is. Therefore, -// the spec hash should contain the spec fields in order to determine -// modifications. -const annotationKeyIntegrityHash = "actions.github.com/integrity-hash" - const labelValueKubernetesPartOf = "gha-runner-scale-set" var ( @@ -95,7 +86,8 @@ type SecretResolver interface { type ResourceBuilder struct { ExcludeLabelPropagationPrefixes []string SecretResolver - Scheme *runtime.Scheme + Scheme *runtime.Scheme + ResourceCache *ResourceCache } func (b *ResourceBuilder) setSchemeIfUnset(scheme *runtime.Scheme) { @@ -121,6 +113,25 @@ func (b *ResourceBuilder) newAutoscalingListener(autoscalingRunnerSet *v1alpha1. return nil, err } + cacheKeyObject := &v1alpha1.AutoscalingListener{ + ObjectMeta: metav1.ObjectMeta{ + Name: scaleSetListenerName(autoscalingRunnerSet), + Namespace: namespace, + }, + } + inputDependency := resourceCacheInputObject("autoscaling-listener-inputs", struct { + Namespace string + Image string + ImagePullSecrets []corev1.LocalObjectReference + }{ + Namespace: namespace, + Image: image, + ImagePullSecrets: imagePullSecrets, + }) + if cached, ok := b.ResourceCache.autoscalingListener.Get(autoscalingRunnerSet, cacheKeyObject, ephemeralRunnerSet, inputDependency); ok { + return cached, nil + } + effectiveMinRunners := 0 effectiveMaxRunners := math.MaxInt32 if autoscalingRunnerSet.Spec.MaxRunners != nil { @@ -152,7 +163,7 @@ func (b *ResourceBuilder) newAutoscalingListener(autoscalingRunnerSet *v1alpha1. ConfigSecretMetadata: autoscalingRunnerSet.Spec.ListenerConfigSecretMetadata, } - labels := b.filterAndMergeLabels(autoscalingRunnerSet.Labels, map[string]string{ + labels := b.propagateLabels(autoscalingRunnerSet.Labels, map[string]string{ LabelKeyGitHubScaleSetNamespace: autoscalingRunnerSet.Namespace, LabelKeyGitHubScaleSetName: autoscalingRunnerSet.Name, LabelKeyKubernetesPartOf: labelValueKubernetesPartOf, @@ -164,16 +175,18 @@ func (b *ResourceBuilder) newAutoscalingListener(autoscalingRunnerSet *v1alpha1. return nil, fmt.Errorf("failed to apply GitHub URL labels: %v", err) } - annotations := map[string]string{ - annotationKeyIntegrityHash: spec.Hash(), - } + var annotations map[string]string if autoscalingRunnerSet.Spec.AutoscalingListenerMetadata != nil { - labels = b.filterAndMergeLabels(autoscalingRunnerSet.Spec.AutoscalingListenerMetadata.Labels, labels) - annotations = b.mergeAnnotations(autoscalingRunnerSet.Spec.AutoscalingListenerMetadata.Annotations, annotations) + labels, _ = b.mergeLabels(autoscalingRunnerSet.Spec.AutoscalingListenerMetadata.Labels, labels) + annotations, _ = b.mergeAnnotations(autoscalingRunnerSet.Spec.AutoscalingListenerMetadata.Annotations, annotations) } autoscalingListener := &v1alpha1.AutoscalingListener{ + TypeMeta: metav1.TypeMeta{ + APIVersion: v1alpha1.GroupVersion.String(), + Kind: "AutoscalingListener", + }, ObjectMeta: metav1.ObjectMeta{ Name: scaleSetListenerName(autoscalingRunnerSet), Namespace: namespace, @@ -182,10 +195,24 @@ func (b *ResourceBuilder) newAutoscalingListener(autoscalingRunnerSet *v1alpha1. }, Spec: spec, } + b.ResourceCache.autoscalingListener.Upsert(autoscalingRunnerSet, autoscalingListener, ephemeralRunnerSet, inputDependency) return autoscalingListener, nil } +func resourceCacheInputObject(name string, value any) client.Object { + return &corev1.ConfigMap{ + TypeMeta: metav1.TypeMeta{ + APIVersion: corev1.SchemeGroupVersion.String(), + Kind: "ConfigMap", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: name, + ResourceVersion: hash.ComputeTemplateHash(value), + }, + } +} + type listenerMetricsServerConfig struct { addr string endpoint string @@ -258,7 +285,7 @@ func (b *ResourceBuilder) newScaleSetListenerConfig(autoscalingListener *v1alpha var labels map[string]string if autoscalingListener.Spec.ConfigSecretMetadata != nil && len(autoscalingListener.Spec.ConfigSecretMetadata.Labels) > 0 { - labels = b.filterAndMergeLabels(autoscalingListener.Spec.ConfigSecretMetadata.Labels, nil) + labels = autoscalingListener.Spec.ConfigSecretMetadata.Labels } annotations := make(map[string]string) @@ -267,6 +294,10 @@ func (b *ResourceBuilder) newScaleSetListenerConfig(autoscalingListener *v1alpha } desiredSecret := &corev1.Secret{ + TypeMeta: metav1.TypeMeta{ + APIVersion: corev1.SchemeGroupVersion.String(), + Kind: "Secret", + }, ObjectMeta: metav1.ObjectMeta{ Name: scaleSetListenerConfigName(autoscalingListener), Namespace: autoscalingListener.Namespace, @@ -276,10 +307,9 @@ func (b *ResourceBuilder) newScaleSetListenerConfig(autoscalingListener *v1alpha Data: map[string][]byte{ "config.json": buf.Bytes(), }, + Type: corev1.SecretTypeOpaque, } - desiredSecret.Annotations[annotationKeyIntegrityHash] = scaleSetListenerConfigIntegrityHash(desiredSecret) - if err := b.setControllerReference(autoscalingListener, desiredSecret); err != nil { return nil, fmt.Errorf("failed to set controller reference for listener config secret: %w", err) } @@ -287,18 +317,6 @@ func (b *ResourceBuilder) newScaleSetListenerConfig(autoscalingListener *v1alpha return desiredSecret, nil } -func scaleSetListenerConfigIntegrityHash(secret *corev1.Secret) string { - type data struct { - Data map[string][]byte `json:"data,omitempty"` - } - - d := data{ - Data: secret.Data, - } - - return hash.ComputeTemplateHash(&d) -} - func (b *ResourceBuilder) newScaleSetListenerPod( autoscalingListener *v1alpha1.AutoscalingListener, podConfig *corev1.Secret, @@ -307,6 +325,16 @@ func (b *ResourceBuilder) newScaleSetListenerPod( roleBinding *rbacv1.RoleBinding, metricsConfig *listenerMetricsServerConfig, ) (*corev1.Pod, error) { + cacheKeyObject := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: autoscalingListener.Name, + Namespace: autoscalingListener.Namespace, + }, + } + if cached, ok := b.ResourceCache.listenerPod.Get(autoscalingListener, cacheKeyObject, podConfig, serviceAccount, role, roleBinding); ok { + return cached, nil + } + envs := []corev1.EnvVar{ { Name: "LISTENER_CONFIG_PATH", @@ -414,8 +442,8 @@ func (b *ResourceBuilder) newScaleSetListenerPod( newRunnerScaleSetListenerPod := &corev1.Pod{ TypeMeta: metav1.TypeMeta{ + APIVersion: corev1.SchemeGroupVersion.String(), Kind: "Pod", - APIVersion: "v1", }, ObjectMeta: metav1.ObjectMeta{ Name: autoscalingListener.Name, @@ -426,16 +454,6 @@ func (b *ResourceBuilder) newScaleSetListenerPod( Spec: podSpec, } - newRunnerScaleSetListenerPod.Annotations[annotationKeyIntegrityHash] = scaleSetListenerPodIntegrity( - newRunnerScaleSetListenerPod, - autoscalingListener, - podConfig, - serviceAccount, - role, - roleBinding, - metricsConfig, - ) - if err := b.setControllerReference(autoscalingListener, newRunnerScaleSetListenerPod); err != nil { return nil, fmt.Errorf("failed to set controller reference for listener pod: %w", err) } @@ -443,42 +461,11 @@ func (b *ResourceBuilder) newScaleSetListenerPod( if autoscalingListener.Spec.Template != nil { mergeListenerPodWithTemplate(newRunnerScaleSetListenerPod, autoscalingListener.Spec.Template) } + b.ResourceCache.listenerPod.Upsert(autoscalingListener, newRunnerScaleSetListenerPod, podConfig, serviceAccount, role, roleBinding) return newRunnerScaleSetListenerPod, nil } -func scaleSetListenerPodIntegrity( - pod *corev1.Pod, - autoscalingListener *v1alpha1.AutoscalingListener, - podConfig *corev1.Secret, - serviceAccount *corev1.ServiceAccount, - role *rbacv1.Role, - roleBinding *rbacv1.RoleBinding, - metricsConfig *listenerMetricsServerConfig, -) string { - type data struct { - ListenerPodSpec *corev1.PodSpec `json:"listenerPodSpec,omitempty"` - AutoscalingListenerIntegrityHash string `json:"autoscalingListenerIntegrityHash"` - ConfigSecretIntegrityHash string `json:"configSecretIntegrityHash"` - ServiceAccountIntegrityHash string `json:"serviceAccountIntegrityHash"` - RoleIntegrityHash string `json:"roleIntegrityHash"` - RoleBindingIntegrityHash string `json:"roleBindingIntegrityHash"` - MetricsConfig *listenerMetricsServerConfig `json:"metricsConfig,omitempty"` - } - - d := data{ - ListenerPodSpec: &pod.Spec, - AutoscalingListenerIntegrityHash: autoscalingListener.Annotations[annotationKeyIntegrityHash], - ConfigSecretIntegrityHash: podConfig.Annotations[annotationKeyIntegrityHash], - ServiceAccountIntegrityHash: serviceAccount.Annotations[annotationKeyIntegrityHash], - RoleIntegrityHash: role.Annotations[annotationKeyIntegrityHash], - RoleBindingIntegrityHash: roleBinding.Annotations[annotationKeyIntegrityHash], - MetricsConfig: metricsConfig, - } - - return hash.ComputeTemplateHash(&d) -} - func mergeListenerPodWithTemplate(pod *corev1.Pod, tmpl *corev1.PodTemplateSpec) { if pod.Annotations == nil { pod.Annotations = make(map[string]string) @@ -597,50 +584,59 @@ func mergeListenerContainer(base, from *corev1.Container) { } func (b *ResourceBuilder) newScaleSetListenerServiceAccount(autoscalingListener *v1alpha1.AutoscalingListener) (*corev1.ServiceAccount, error) { - base := &corev1.ServiceAccount{ + cacheKeyObject := &corev1.ServiceAccount{ ObjectMeta: metav1.ObjectMeta{ Name: autoscalingListener.Name, Namespace: autoscalingListener.Namespace, - Labels: b.filterAndMergeLabels(autoscalingListener.Labels, map[string]string{ - LabelKeyGitHubScaleSetNamespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace, - LabelKeyGitHubScaleSetName: autoscalingListener.Spec.AutoscalingRunnerSetName, - }), - Annotations: make(map[string]string), }, } + if cached, ok := b.ResourceCache.listenerServiceAccount.Get(autoscalingListener, cacheKeyObject); ok { + return cached, nil + } + + labels := b.propagateLabels(autoscalingListener.Labels, map[string]string{ + LabelKeyGitHubScaleSetNamespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace, + LabelKeyGitHubScaleSetName: autoscalingListener.Spec.AutoscalingRunnerSetName, + }) + annotations := make(map[string]string) if autoscalingListener.Spec.ServiceAccountMetadata != nil { - base.Labels = b.filterAndMergeLabels(autoscalingListener.Spec.ServiceAccountMetadata.Labels, base.Labels) - base.Annotations = b.mergeAnnotations(autoscalingListener.Spec.ServiceAccountMetadata.Annotations, base.Annotations) + labels, _ = b.mergeLabels(autoscalingListener.Spec.ServiceAccountMetadata.Labels, labels) + annotations, _ = b.mergeAnnotations(autoscalingListener.Spec.ServiceAccountMetadata.Annotations, annotations) } - base.Annotations[annotationKeyIntegrityHash] = scaleSetListenerServiceAccountIntegrityHash(base) - + base := &corev1.ServiceAccount{ + TypeMeta: metav1.TypeMeta{ + APIVersion: corev1.SchemeGroupVersion.String(), + Kind: "ServiceAccount", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: autoscalingListener.Name, + Namespace: autoscalingListener.Namespace, + Labels: labels, + Annotations: annotations, + }, + } if err := b.setControllerReference(autoscalingListener, base); err != nil { return nil, fmt.Errorf("failed to set controller reference for listener service account: %w", err) } + b.ResourceCache.listenerServiceAccount.Upsert(autoscalingListener, base) return base, nil } -func scaleSetListenerServiceAccountIntegrityHash(sa *corev1.ServiceAccount) string { - type data struct { - Secrets []corev1.ObjectReference `json:"secrets"` - ImagePullSecrets []corev1.LocalObjectReference `json:"imagePullSecrets"` - AutomountServiceAccountToken *bool `json:"automountServiceAccountToken"` +func (b *ResourceBuilder) newScaleSetListenerRole(autoscalingListener *v1alpha1.AutoscalingListener) *rbacv1.Role { + cacheKeyObject := &rbacv1.Role{ + ObjectMeta: metav1.ObjectMeta{ + Name: autoscalingListener.Name, + Namespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace, + }, } - - d := data{ - Secrets: sa.Secrets, - ImagePullSecrets: sa.ImagePullSecrets, - AutomountServiceAccountToken: sa.AutomountServiceAccountToken, + if cached, ok := b.ResourceCache.listenerRole.Get(autoscalingListener, cacheKeyObject); ok { + return cached } - return hash.ComputeTemplateHash(&d) -} - -func (b *ResourceBuilder) newScaleSetListenerRole(autoscalingListener *v1alpha1.AutoscalingListener) *rbacv1.Role { - labels := b.filterAndMergeLabels(autoscalingListener.Labels, map[string]string{ + labels := b.propagateLabels(autoscalingListener.Labels, map[string]string{ LabelKeyGitHubScaleSetNamespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace, LabelKeyGitHubScaleSetName: autoscalingListener.Spec.AutoscalingRunnerSetName, labelKeyListenerNamespace: autoscalingListener.Namespace, @@ -649,11 +645,15 @@ func (b *ResourceBuilder) newScaleSetListenerRole(autoscalingListener *v1alpha1. annotations := make(map[string]string) if autoscalingListener.Spec.RoleMetadata != nil { - labels = b.filterAndMergeLabels(autoscalingListener.Spec.RoleMetadata.Labels, labels) - annotations = b.mergeAnnotations(autoscalingListener.Spec.RoleMetadata.Annotations, nil) + labels, _ = b.mergeLabels(autoscalingListener.Spec.RoleMetadata.Labels, labels) + annotations = autoscalingListener.Spec.RoleMetadata.Annotations } newRole := &rbacv1.Role{ + TypeMeta: metav1.TypeMeta{ + APIVersion: rbacv1.SchemeGroupVersion.String(), + Kind: "Role", + }, ObjectMeta: metav1.ObjectMeta{ Name: autoscalingListener.Name, Namespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace, @@ -663,27 +663,26 @@ func (b *ResourceBuilder) newScaleSetListenerRole(autoscalingListener *v1alpha1. Rules: rulesForListenerRole([]string{autoscalingListener.Spec.EphemeralRunnerSetName}), } - newRole.Annotations[annotationKeyIntegrityHash] = scaleSetRoleIntegrityHash(newRole) + b.ResourceCache.listenerRole.Upsert(autoscalingListener, newRole) return newRole } -func scaleSetRoleIntegrityHash(role *rbacv1.Role) string { - type data struct { - Rules []rbacv1.PolicyRule `json:"rules"` +func (b *ResourceBuilder) newScaleSetListenerRoleBinding(autoscalingListener *v1alpha1.AutoscalingListener, listenerRole *rbacv1.Role, serviceAccount *corev1.ServiceAccount) *rbacv1.RoleBinding { + cacheKeyObject := &rbacv1.RoleBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: autoscalingListener.Name, + Namespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace, + }, } - - d := data{ - Rules: role.Rules, + if cached, ok := b.ResourceCache.listenerRoleBinding.Get(autoscalingListener, cacheKeyObject, listenerRole, serviceAccount); ok { + return cached } - return hash.ComputeTemplateHash(&d) -} - -func (b *ResourceBuilder) newScaleSetListenerRoleBinding(autoscalingListener *v1alpha1.AutoscalingListener, listenerRole *rbacv1.Role, serviceAccount *corev1.ServiceAccount) *rbacv1.RoleBinding { roleRef := rbacv1.RoleRef{ - Kind: "Role", - Name: listenerRole.Name, + APIGroup: rbacv1.GroupName, + Kind: "Role", + Name: listenerRole.Name, } subjects := []rbacv1.Subject{ @@ -694,7 +693,7 @@ func (b *ResourceBuilder) newScaleSetListenerRoleBinding(autoscalingListener *v1 }, } - labels := b.filterAndMergeLabels(autoscalingListener.Labels, map[string]string{ + labels := b.propagateLabels(autoscalingListener.Labels, map[string]string{ LabelKeyGitHubScaleSetNamespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace, LabelKeyGitHubScaleSetName: autoscalingListener.Spec.AutoscalingRunnerSetName, labelKeyListenerNamespace: autoscalingListener.Namespace, @@ -703,11 +702,15 @@ func (b *ResourceBuilder) newScaleSetListenerRoleBinding(autoscalingListener *v1 annotations := make(map[string]string) if autoscalingListener.Spec.RoleBindingMetadata != nil { - labels = b.filterAndMergeLabels(autoscalingListener.Spec.RoleBindingMetadata.Labels, labels) + labels, _ = b.mergeLabels(autoscalingListener.Spec.RoleBindingMetadata.Labels, labels) annotations = autoscalingListener.Spec.RoleBindingMetadata.Annotations } newRoleBinding := &rbacv1.RoleBinding{ + TypeMeta: metav1.TypeMeta{ + APIVersion: rbacv1.SchemeGroupVersion.String(), + Kind: "RoleBinding", + }, ObjectMeta: metav1.ObjectMeta{ Name: autoscalingListener.Name, Namespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace, @@ -718,31 +721,27 @@ func (b *ResourceBuilder) newScaleSetListenerRoleBinding(autoscalingListener *v1 Subjects: subjects, } - newRoleBinding.Annotations[annotationKeyIntegrityHash] = scaleSetListenerRoleBindingIntegrityHash(newRoleBinding) + b.ResourceCache.listenerRoleBinding.Upsert(autoscalingListener, newRoleBinding, listenerRole, serviceAccount) return newRoleBinding } -func scaleSetListenerRoleBindingIntegrityHash(rb *rbacv1.RoleBinding) string { - type data struct { - RoleRef rbacv1.RoleRef `json:"roleRef"` - Subjects []rbacv1.Subject `json:"subjects"` - } - - d := data{ - RoleRef: rb.RoleRef, - Subjects: rb.Subjects, - } - - return hash.ComputeTemplateHash(&d) -} - func (b *ResourceBuilder) newEphemeralRunnerSet(autoscalingRunnerSet *v1alpha1.AutoscalingRunnerSet) (*v1alpha1.EphemeralRunnerSet, error) { runnerScaleSetID, err := strconv.Atoi(autoscalingRunnerSet.Annotations[runnerScaleSetIDAnnotationKey]) if err != nil { return nil, err } + cacheKeyObject := &v1alpha1.EphemeralRunnerSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: autoscalingRunnerSet.Name, + Namespace: autoscalingRunnerSet.Namespace, + }, + } + if cached, ok := b.ResourceCache.ephemeralRunnerSet.Get(autoscalingRunnerSet, cacheKeyObject); ok { + return cached, nil + } + spec := v1alpha1.EphemeralRunnerSetSpec{ Replicas: 0, EphemeralRunnerSpec: v1alpha1.EphemeralRunnerSpec{ @@ -758,7 +757,7 @@ func (b *ResourceBuilder) newEphemeralRunnerSet(autoscalingRunnerSet *v1alpha1.A EphemeralRunnerMetadata: autoscalingRunnerSet.Spec.EphemeralRunnerMetadata, } - labels := b.filterAndMergeLabels(autoscalingRunnerSet.Labels, map[string]string{ + labels := b.propagateLabels(autoscalingRunnerSet.Labels, map[string]string{ LabelKeyKubernetesPartOf: labelValueKubernetesPartOf, LabelKeyKubernetesComponent: "runner-set", LabelKeyKubernetesVersion: autoscalingRunnerSet.Labels[LabelKeyKubernetesVersion], @@ -776,12 +775,15 @@ func (b *ResourceBuilder) newEphemeralRunnerSet(autoscalingRunnerSet *v1alpha1.A } if autoscalingRunnerSet.Spec.EphemeralRunnerSetMetadata != nil { - labels = b.filterAndMergeLabels(autoscalingRunnerSet.Spec.EphemeralRunnerSetMetadata.Labels, labels) - annotations = b.mergeAnnotations(autoscalingRunnerSet.Spec.EphemeralRunnerSetMetadata.Annotations, annotations) + labels, _ = b.mergeLabels(autoscalingRunnerSet.Spec.EphemeralRunnerSetMetadata.Labels, labels) + annotations, _ = b.mergeAnnotations(autoscalingRunnerSet.Spec.EphemeralRunnerSetMetadata.Annotations, annotations) } newEphemeralRunnerSet := &v1alpha1.EphemeralRunnerSet{ - TypeMeta: metav1.TypeMeta{}, + TypeMeta: metav1.TypeMeta{ + APIVersion: v1alpha1.GroupVersion.String(), + Kind: "EphemeralRunnerSet", + }, ObjectMeta: metav1.ObjectMeta{ Name: autoscalingRunnerSet.Name, Namespace: autoscalingRunnerSet.Namespace, @@ -791,26 +793,14 @@ func (b *ResourceBuilder) newEphemeralRunnerSet(autoscalingRunnerSet *v1alpha1.A Spec: spec, } - newEphemeralRunnerSet.Annotations[annotationKeyIntegrityHash] = ephemeralRunnerSetIntegrityHash(newEphemeralRunnerSet) - if err := b.setControllerReference(autoscalingRunnerSet, newEphemeralRunnerSet); err != nil { return nil, fmt.Errorf("failed to set controller reference for ephemeral runner set: %w", err) } + b.ResourceCache.ephemeralRunnerSet.Upsert(autoscalingRunnerSet, newEphemeralRunnerSet) return newEphemeralRunnerSet, nil } -func ephemeralRunnerSetIntegrityHash(ers *v1alpha1.EphemeralRunnerSet) string { - type data struct { - EphemeralRunnerSpec v1alpha1.EphemeralRunnerSpec `json:"ephemeralRunnerSpec"` - } - - d := data{ - EphemeralRunnerSpec: ers.Spec.EphemeralRunnerSpec, - } - return hash.ComputeTemplateHash(&d) -} - func (b *ResourceBuilder) newAutoscalingListenerProxySecret(autoscalingListener *v1alpha1.AutoscalingListener, data map[string][]byte) (*corev1.Secret, error) { newProxySecret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ @@ -823,10 +813,9 @@ func (b *ResourceBuilder) newAutoscalingListenerProxySecret(autoscalingListener Annotations: make(map[string]string, 1), }, Data: data, + Type: corev1.SecretTypeOpaque, } - newProxySecret.Annotations[annotationKeyIntegrityHash] = autoscalingListenerProxySecretIntegrityHash(newProxySecret) - if err := b.setControllerReference(autoscalingListener, newProxySecret); err != nil { return nil, fmt.Errorf("failed to set controller reference for listener proxy secret: %w", err) } @@ -834,18 +823,6 @@ func (b *ResourceBuilder) newAutoscalingListenerProxySecret(autoscalingListener return newProxySecret, nil } -func autoscalingListenerProxySecretIntegrityHash(secret *corev1.Secret) string { - type data struct { - Data map[string][]byte `json:"data"` - } - - d := data{ - Data: secret.Data, - } - - return hash.ComputeTemplateHash(&d) -} - func (b *ResourceBuilder) newEphemeralRunner(ephemeralRunnerSet *v1alpha1.EphemeralRunnerSet) (*v1alpha1.EphemeralRunner, error) { labels := make(map[string]string, len(ephemeralRunnerSet.Labels)) maps.Copy(labels, ephemeralRunnerSet.Labels) @@ -856,8 +833,8 @@ func (b *ResourceBuilder) newEphemeralRunner(ephemeralRunnerSet *v1alpha1.Epheme annotations[AnnotationKeyPatchID] = strconv.Itoa(ephemeralRunnerSet.Spec.PatchID) if ephemeralRunnerSet.Spec.EphemeralRunnerMetadata != nil { - labels = b.filterAndMergeLabels(ephemeralRunnerSet.Spec.EphemeralRunnerMetadata.Labels, labels) - annotations = b.mergeAnnotations(ephemeralRunnerSet.Spec.EphemeralRunnerMetadata.Annotations, annotations) + labels, _ = b.mergeLabels(ephemeralRunnerSet.Spec.EphemeralRunnerMetadata.Labels, labels) + annotations, _ = b.mergeAnnotations(ephemeralRunnerSet.Spec.EphemeralRunnerMetadata.Annotations, annotations) } ephemeralRunner := &v1alpha1.EphemeralRunner{ @@ -953,7 +930,7 @@ func (b *ResourceBuilder) newEphemeralRunnerJitSecret(ephemeralRunner *v1alpha1. ) if ephemeralRunner.Spec.EphemeralRunnerConfigSecretMetadata != nil { - labels = b.filterAndMergeLabels(ephemeralRunner.Spec.EphemeralRunnerConfigSecretMetadata.Labels, nil) + labels = ephemeralRunner.Spec.EphemeralRunnerConfigSecretMetadata.Labels annotations = ephemeralRunner.Spec.EphemeralRunnerConfigSecretMetadata.Annotations } @@ -992,8 +969,6 @@ func (b *ResourceBuilder) newEphemeralRunnerSetProxySecret(ephemeralRunnerSet *v Data: data, } - runnerPodProxySecret.Annotations[annotationKeyIntegrityHash] = ephemeralRunnerSetProxySecretZIdentityHash(runnerPodProxySecret) - if err := b.setControllerReference(ephemeralRunnerSet, runnerPodProxySecret); err != nil { return nil, fmt.Errorf("failed to set controller reference for ephemeral runner set proxy secret: %w", err) } @@ -1001,18 +976,6 @@ func (b *ResourceBuilder) newEphemeralRunnerSetProxySecret(ephemeralRunnerSet *v return runnerPodProxySecret, nil } -func ephemeralRunnerSetProxySecretZIdentityHash(secret *corev1.Secret) string { - type data struct { - Data map[string][]byte `json:"data"` - } - - d := data{ - Data: secret.Data, - } - - return hash.ComputeTemplateHash(&d) -} - func scaleSetListenerConfigName(autoscalingListener *v1alpha1.AutoscalingListener) string { return autoscalingListener.Name + "-config" } @@ -1059,7 +1022,12 @@ func rulesForListenerRole(resourceNames []string) []rbacv1.PolicyRule { }, { APIGroups: []string{"actions.github.com"}, - Resources: []string{"ephemeralrunners", "ephemeralrunners/status"}, + Resources: []string{"ephemeralrunners"}, + Verbs: []string{"get", "patch"}, + }, + { + APIGroups: []string{"actions.github.com"}, + Resources: []string{"ephemeralrunners/status"}, Verbs: []string{"patch"}, }, } @@ -1093,40 +1061,67 @@ func trimLabelValue(val string) string { return strings.Trim(val, "-_.") } -func (b *ResourceBuilder) filterAndMergeLabels(base, overwrite map[string]string) map[string]string { +func (b *ResourceBuilder) shouldExcludePropagatedLabel(k, _ string) bool { + for _, prefix := range b.ExcludeLabelPropagationPrefixes { + if strings.HasPrefix(k, prefix) { + return true + } + } + return false +} + +// propagateLabels is responsible for filtering labels during propagation. +// It only makes sense when we are creating the resource derived from some other resource. +// Since the desired resource is cached, then we don't need to call this method every time. +func (b *ResourceBuilder) propagateLabels(base, overwrite map[string]string) map[string]string { if base == nil && overwrite == nil { return nil } - mergedLabels := make(map[string]string, len(base)) -base: + labels := make(map[string]string, len(base)+len(overwrite)) for k, v := range base { - for _, prefix := range b.ExcludeLabelPropagationPrefixes { - if strings.HasPrefix(k, prefix) { - continue base - } + if b.shouldExcludePropagatedLabel(k, v) { + continue } - mergedLabels[k] = v + labels[k] = v } + maps.Copy(labels, overwrite) -overwrite: - for k, v := range overwrite { - for _, prefix := range b.ExcludeLabelPropagationPrefixes { - if strings.HasPrefix(k, prefix) { - continue overwrite - } - } - mergedLabels[k] = v + if len(labels) == 0 { + return nil } + return labels +} - return mergedLabels +func (b *ResourceBuilder) mergeLabels(base, overwrite map[string]string) (map[string]string, bool) { + return mergeMaps(base, overwrite) } -func (b *ResourceBuilder) mergeAnnotations(base, overwrite map[string]string) map[string]string { - if base == nil && overwrite == nil { - return nil +func (b *ResourceBuilder) mergeAnnotations(base, overwrite map[string]string) (map[string]string, bool) { + return mergeMaps(base, overwrite) +} + +func mergeMaps[M ~map[K]V, K comparable, V comparable](base M, overwrite M) (M, bool) { + if len(overwrite) == 0 { + return base, false } - base = maps.Clone(base) - maps.Copy(base, overwrite) - return base + + if containsMapEntries(base, overwrite) { + return base, false + } + + merged := make(M, len(base)+len(overwrite)) + maps.Copy(merged, base) + maps.Copy(merged, overwrite) + return merged, true +} + +func containsMapEntries[M ~map[K]V, K comparable, V comparable](base M, entries M) bool { + for k, v := range entries { + current, ok := base[k] + if !ok || current != v { + return false + } + } + return true } diff --git a/controllers/actions.github.com/resourcebuilder_test.go b/controllers/actions.github.com/resourcebuilder_test.go index d08851173a..3a3ad4cf15 100644 --- a/controllers/actions.github.com/resourcebuilder_test.go +++ b/controllers/actions.github.com/resourcebuilder_test.go @@ -10,6 +10,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" + rbacv1 "k8s.io/api/rbac/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -102,18 +103,20 @@ func TestMetadataPropagation(t *testing.T) { }, } + cache := NewResourceCache() b := ResourceBuilder{ ExcludeLabelPropagationPrefixes: []string{ "example.com/", "directly.excluded.org/label", }, + ResourceCache: &cache, } ephemeralRunnerSet, err := b.newEphemeralRunnerSet(&autoscalingRunnerSet) require.NoError(t, err) assert.Equal(t, labelValueKubernetesPartOf, ephemeralRunnerSet.Labels[LabelKeyKubernetesPartOf]) assert.Equal(t, "runner-set", ephemeralRunnerSet.Labels[LabelKeyKubernetesComponent]) assert.Equal(t, autoscalingRunnerSet.Labels[LabelKeyKubernetesVersion], ephemeralRunnerSet.Labels[LabelKeyKubernetesVersion]) - assert.NotEmpty(t, ephemeralRunnerSet.Annotations[annotationKeyIntegrityHash]) + assert.NotContains(t, ephemeralRunnerSet.Annotations, "actions.github.com/integrity-hash") assert.Equal(t, autoscalingRunnerSet.Name, ephemeralRunnerSet.Labels[LabelKeyGitHubScaleSetName]) assert.Equal(t, autoscalingRunnerSet.Namespace, ephemeralRunnerSet.Labels[LabelKeyGitHubScaleSetNamespace]) assert.Equal(t, "", ephemeralRunnerSet.Labels[LabelKeyGitHubEnterprise]) @@ -130,7 +133,7 @@ func TestMetadataPropagation(t *testing.T) { assert.Equal(t, labelValueKubernetesPartOf, listener.Labels[LabelKeyKubernetesPartOf]) assert.Equal(t, "runner-scale-set-listener", listener.Labels[LabelKeyKubernetesComponent]) assert.Equal(t, autoscalingRunnerSet.Labels[LabelKeyKubernetesVersion], listener.Labels[LabelKeyKubernetesVersion]) - assert.NotEmpty(t, ephemeralRunnerSet.Annotations[annotationKeyIntegrityHash]) + assert.NotContains(t, listener.Annotations, "actions.github.com/integrity-hash") assert.Equal(t, autoscalingRunnerSet.Name, listener.Labels[LabelKeyGitHubScaleSetName]) assert.Equal(t, autoscalingRunnerSet.Namespace, listener.Labels[LabelKeyGitHubScaleSetNamespace]) assert.Equal(t, "", listener.Labels[LabelKeyGitHubEnterprise]) @@ -155,6 +158,7 @@ func TestMetadataPropagation(t *testing.T) { assert.Equal(t, "listener-role-annotation", listenerRole.Annotations["test.com/listener-role-annotation"]) listenerRoleBinding := b.newScaleSetListenerRoleBinding(listener, listenerRole, listenerServiceAccount) + assert.Equal(t, rbacv1.GroupName, listenerRoleBinding.RoleRef.APIGroup) assert.Equal(t, "listener-role-binding-label", listenerRoleBinding.Labels["test.com/listener-role-binding-label"]) assert.Equal(t, "listener-role-binding-annotation", listenerRoleBinding.Annotations["test.com/listener-role-binding-annotation"]) @@ -171,6 +175,7 @@ func TestMetadataPropagation(t *testing.T) { ephemeralRunner, err := b.newEphemeralRunner(ephemeralRunnerSet) require.NoError(t, err) + assert.ElementsMatch(t, []string{ephemeralRunnerFinalizerName, ephemeralRunnerActionsFinalizerName}, ephemeralRunner.Finalizers) for _, key := range commonLabelKeys { if key == LabelKeyKubernetesComponent { @@ -203,7 +208,7 @@ func TestMetadataPropagation(t *testing.T) { } } -func TestEphemeralRunnerSetProxySecretZIdentityHash(t *testing.T) { +func TestEphemeralRunnerSetProxySecretMetadata(t *testing.T) { ephemeralRunnerSet := &v1alpha1.EphemeralRunnerSet{ ObjectMeta: metav1.ObjectMeta{ Name: "test-scale-set", @@ -221,13 +226,80 @@ func TestEphemeralRunnerSetProxySecretZIdentityHash(t *testing.T) { }) require.NoError(t, err) - actualHash := proxySecret.Annotations[annotationKeyIntegrityHash] - assert.NotEmpty(t, actualHash) - assert.Equal(t, ephemeralRunnerSetProxySecretZIdentityHash(proxySecret), actualHash) + assert.Equal(t, proxyEphemeralRunnerSetSecretName(ephemeralRunnerSet), proxySecret.Name) + assert.Equal(t, ephemeralRunnerSet.Namespace, proxySecret.Namespace) + assert.Equal(t, ephemeralRunnerSet.Labels[LabelKeyGitHubScaleSetName], proxySecret.Labels[LabelKeyGitHubScaleSetName]) + assert.Equal(t, ephemeralRunnerSet.Labels[LabelKeyGitHubScaleSetNamespace], proxySecret.Labels[LabelKeyGitHubScaleSetNamespace]) + assert.NotContains(t, proxySecret.Annotations, "actions.github.com/integrity-hash") +} + +func TestMergeMaps(t *testing.T) { + t.Run("equal overwrite is not modified", func(t *testing.T) { + base := map[string]string{ + "a": "1", + "b": "2", + } + overwrite := map[string]string{ + "a": "1", + "b": "2", + } + + merged, modified := mergeMaps(base, overwrite) + + require.False(t, modified) + merged["a"] = "changed" + assert.Equal(t, "changed", base["a"]) + }) + + t.Run("base superset is not modified", func(t *testing.T) { + base := map[string]string{ + "a": "1", + "b": "2", + "extra": "base-only", + } + overwrite := map[string]string{ + "a": "1", + "b": "2", + } + + merged, modified := mergeMaps(base, overwrite) + + require.False(t, modified) + merged["a"] = "changed" + assert.Equal(t, "changed", base["a"]) + }) + + t.Run("missing overwrite key is modified", func(t *testing.T) { + base := map[string]string{ + "a": "1", + } + overwrite := map[string]string{ + "a": "1", + "b": "2", + } + + merged, modified := mergeMaps(base, overwrite) + + require.True(t, modified) + assert.Equal(t, map[string]string{"a": "1", "b": "2"}, merged) + assert.NotContains(t, base, "b") + }) + + t.Run("different overwrite value is modified", func(t *testing.T) { + base := map[string]string{ + "a": "1", + "b": "2", + } + overwrite := map[string]string{ + "a": "updated", + } + + merged, modified := mergeMaps(base, overwrite) - changedProxySecret := proxySecret.DeepCopy() - changedProxySecret.Data["http_proxy"] = []byte("http://updated-proxy.example.com") - assert.NotEqual(t, actualHash, ephemeralRunnerSetProxySecretZIdentityHash(changedProxySecret)) + require.True(t, modified) + assert.Equal(t, map[string]string{"a": "updated", "b": "2"}, merged) + assert.Equal(t, "1", base["a"]) + }) } func TestGitHubURLTrimLabelValues(t *testing.T) { @@ -257,7 +329,8 @@ func TestGitHubURLTrimLabelValues(t *testing.T) { GitHubConfigUrl: fmt.Sprintf("https://github.com/%s/%s", organization, repository), } - var b ResourceBuilder + cache := NewResourceCache() + b := ResourceBuilder{ResourceCache: &cache} ephemeralRunnerSet, err := b.newEphemeralRunnerSet(autoscalingRunnerSet) require.NoError(t, err) assert.Len(t, ephemeralRunnerSet.Labels[LabelKeyGitHubEnterprise], 0) @@ -281,7 +354,8 @@ func TestGitHubURLTrimLabelValues(t *testing.T) { GitHubConfigUrl: fmt.Sprintf("https://github.com/enterprises/%s", enterprise), } - var b ResourceBuilder + cache := NewResourceCache() + b := ResourceBuilder{ResourceCache: &cache} ephemeralRunnerSet, err := b.newEphemeralRunnerSet(autoscalingRunnerSet) require.NoError(t, err) assert.Len(t, ephemeralRunnerSet.Labels[LabelKeyGitHubEnterprise], 63) @@ -313,7 +387,6 @@ func TestOwnershipRelationships(t *testing.T) { runnerScaleSetIDAnnotationKey: "1", AnnotationKeyGitHubRunnerGroupName: "test-group", AnnotationKeyGitHubRunnerScaleSetName: "test-scale-set", - annotationKeyIntegrityHash: "test-hash", }, }, Spec: v1alpha1.AutoscalingRunnerSetSpec{ @@ -322,7 +395,8 @@ func TestOwnershipRelationships(t *testing.T) { } // Initialize ResourceBuilder - b := ResourceBuilder{} + cache := NewResourceCache() + b := ResourceBuilder{ResourceCache: &cache} // Create EphemeralRunnerSet ephemeralRunnerSet, err := b.newEphemeralRunnerSet(&autoscalingRunnerSet) @@ -421,7 +495,8 @@ func TestListenerPodNodeSelector(t *testing.T) { }, } - b := ResourceBuilder{} + cache := NewResourceCache() + b := ResourceBuilder{ResourceCache: &cache} ephemeralRunnerSet, err := b.newEphemeralRunnerSet(&autoscalingRunnerSet) require.NoError(t, err) diff --git a/controllers/actions.github.com/resourcecache.go b/controllers/actions.github.com/resourcecache.go new file mode 100644 index 0000000000..ccbec79f6b --- /dev/null +++ b/controllers/actions.github.com/resourcecache.go @@ -0,0 +1,303 @@ +package actionsgithubcom + +import ( + "reflect" + "slices" + "strings" + "sync" + + "github.com/actions/actions-runner-controller/apis/actions.github.com/v1alpha1" + "github.com/actions/actions-runner-controller/hash" + corev1 "k8s.io/api/core/v1" + rbacv1 "k8s.io/api/rbac/v1" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +const ( + resourceCacheInitialEntries = 4096 + resourceCacheInitialMainUIDEntries = 4096 + resourceCacheInitialOwnerEntries = 8 + resourceCacheMaxDependencyRefs = 4 +) + +type ResourceCacheObjectRef struct { + ObjectType schema.GroupVersionKind + Namespace string + Name string + UID types.UID + ResourceVersion string +} + +type ResourceCacheKey struct { + MainUID types.UID + Namespace string + Name string +} + +type ResourceCacheValue[T client.Object] struct { + MainObject ResourceCacheObjectRef + ResourceVersion string + dependencyKey resourceCacheDependencyKey + Object T +} + +type resourceCacheDependencyKey struct { + count int + refs [resourceCacheMaxDependencyRefs]ResourceCacheObjectRef +} + +type ResourceCache struct { + autoscalingListener *resourceCacheState[*v1alpha1.AutoscalingListener] + ephemeralRunnerSet *resourceCacheState[*v1alpha1.EphemeralRunnerSet] + listenerPod *resourceCacheState[*corev1.Pod] + listenerServiceAccount *resourceCacheState[*corev1.ServiceAccount] + listenerRole *resourceCacheState[*rbacv1.Role] + listenerRoleBinding *resourceCacheState[*rbacv1.RoleBinding] +} + +func NewResourceCache() ResourceCache { + return ResourceCache{ + autoscalingListener: newResourceCacheState[*v1alpha1.AutoscalingListener](), + ephemeralRunnerSet: newResourceCacheState[*v1alpha1.EphemeralRunnerSet](), + listenerPod: newResourceCacheState[*corev1.Pod](), + listenerServiceAccount: newResourceCacheState[*corev1.ServiceAccount](), + listenerRole: newResourceCacheState[*rbacv1.Role](), + listenerRoleBinding: newResourceCacheState[*rbacv1.RoleBinding](), + } +} + +type resourceCacheState[T client.Object] struct { + mu sync.RWMutex + entries map[ResourceCacheKey]ResourceCacheValue[T] + entriesByMainUID map[types.UID]map[ResourceCacheKey]struct{} +} + +func newResourceCacheState[T client.Object]() *resourceCacheState[T] { + return &resourceCacheState[T]{ + entries: make(map[ResourceCacheKey]ResourceCacheValue[T], resourceCacheInitialEntries), + entriesByMainUID: make(map[types.UID]map[ResourceCacheKey]struct{}, resourceCacheInitialMainUIDEntries), + } +} + +func (s *resourceCacheState[T]) Get( + mainObject client.Object, + desiredObject T, + dependencies ...client.Object, +) (T, bool) { + var zero T + if s == nil || isNilResourceCacheObject(mainObject) || isNilResourceCacheObject(desiredObject) { + return zero, false + } + dependencyKey, ok := newResourceCacheDependencyKey(dependencies...) + if !ok { + return zero, false + } + if mainObject.GetUID() == "" { + return zero, false + } + + key := newResourceCacheKey(mainObject, desiredObject) + mainObjectRef := newResourceCacheObjectRef(mainObject) + + s.mu.RLock() + value, ok := s.entries[key] + if ok && value.MainObject == mainObjectRef && value.dependencyKey.Equal(dependencyKey) { + s.mu.RUnlock() + return value.Object, true + } + s.mu.RUnlock() + + return zero, false +} + +func (s *resourceCacheState[T]) Upsert( + mainObject client.Object, + desiredObject T, + dependencies ...client.Object, +) (ResourceCacheValue[T], bool) { + var zero ResourceCacheValue[T] + if s == nil || isNilResourceCacheObject(mainObject) || isNilResourceCacheObject(desiredObject) { + return zero, false + } + dependencyKey, ok := newResourceCacheDependencyKey(dependencies...) + if !ok { + return zero, false + } + if mainObject.GetUID() == "" { + return zero, false + } + + key := newResourceCacheKey(mainObject, desiredObject) + mainObjectRef := newResourceCacheObjectRef(mainObject) + resourceVersion := desiredObject.GetResourceVersion() + + s.mu.RLock() + previous, ok := s.entries[key] + if ok && previous.MainObject == mainObjectRef && previous.ResourceVersion == resourceVersion && previous.dependencyKey.Equal(dependencyKey) { + s.mu.RUnlock() + return previous, false + } + s.mu.RUnlock() + + s.mu.Lock() + defer s.mu.Unlock() + + previous, ok = s.entries[key] + if ok && previous.MainObject == mainObjectRef && previous.ResourceVersion == resourceVersion && previous.dependencyKey.Equal(dependencyKey) { + return previous, false + } + + value := ResourceCacheValue[T]{ + MainObject: mainObjectRef, + ResourceVersion: resourceVersion, + dependencyKey: dependencyKey, + Object: desiredObject, + } + s.entries[key] = value + s.indexKeyLocked(key) + return value, true +} + +func (c *ResourceCache) Delete(mainObject client.Object) { + if mainObject == nil { + return + } + + c.autoscalingListener.Delete(mainObject) + c.ephemeralRunnerSet.Delete(mainObject) + c.listenerPod.Delete(mainObject) + c.listenerServiceAccount.Delete(mainObject) + c.listenerRole.Delete(mainObject) + c.listenerRoleBinding.Delete(mainObject) +} + +func (s *resourceCacheState[T]) Delete(mainObject client.Object) { + if s == nil || mainObject == nil { + return + } + + uid := mainObject.GetUID() + if uid == "" { + return + } + + s.mu.Lock() + defer s.mu.Unlock() + + for key := range s.entriesByMainUID[uid] { + delete(s.entries, key) + } + delete(s.entriesByMainUID, uid) +} + +func (s *resourceCacheState[T]) indexKeyLocked(key ResourceCacheKey) { + keys, ok := s.entriesByMainUID[key.MainUID] + if !ok { + keys = make(map[ResourceCacheKey]struct{}, resourceCacheInitialOwnerEntries) + s.entriesByMainUID[key.MainUID] = keys + } + keys[key] = struct{}{} +} + +func newResourceCacheKey(mainObject client.Object, desiredObject client.Object) ResourceCacheKey { + return ResourceCacheKey{ + MainUID: mainObject.GetUID(), + Namespace: desiredObject.GetNamespace(), + Name: resourceCacheObjectName(desiredObject), + } +} + +func newResourceCacheDependencyKey(objects ...client.Object) (resourceCacheDependencyKey, bool) { + if len(objects) > resourceCacheMaxDependencyRefs { + return resourceCacheDependencyKey{}, false + } + + key := resourceCacheDependencyKey{count: len(objects)} + for i, object := range objects { + if isNilResourceCacheObject(object) { + return resourceCacheDependencyKey{}, false + } + key.refs[i] = newResourceCacheObjectRef(object) + } + slices.SortFunc(key.refs[:key.count], func(a, b ResourceCacheObjectRef) int { + return compareResourceCacheObjectRefs(a, b) + }) + return key, true +} + +func (k resourceCacheDependencyKey) Equal(other resourceCacheDependencyKey) bool { + if k.count != other.count { + return false + } + if k.count > len(k.refs) || other.count > len(other.refs) { + return false + } + + for i := 0; i < k.count; i++ { + if k.refs[i] != other.refs[i] { + return false + } + } + + return true +} + +func newResourceCacheObjectRef(object client.Object) ResourceCacheObjectRef { + resourceVersion := object.GetResourceVersion() + if resourceVersion == "" { + resourceVersion = hash.ComputeTemplateHash(object) + } + + return ResourceCacheObjectRef{ + ObjectType: object.GetObjectKind().GroupVersionKind(), + Namespace: object.GetNamespace(), + Name: resourceCacheObjectName(object), + UID: object.GetUID(), + ResourceVersion: resourceVersion, + } +} + +func compareResourceCacheObjectRefs(a, b ResourceCacheObjectRef) int { + if c := compareGroupVersionKinds(a.ObjectType, b.ObjectType); c != 0 { + return c + } + if c := strings.Compare(a.Namespace, b.Namespace); c != 0 { + return c + } + if c := strings.Compare(a.Name, b.Name); c != 0 { + return c + } + if c := strings.Compare(string(a.UID), string(b.UID)); c != 0 { + return c + } + return strings.Compare(a.ResourceVersion, b.ResourceVersion) +} + +func compareGroupVersionKinds(a, b schema.GroupVersionKind) int { + if c := strings.Compare(a.Group, b.Group); c != 0 { + return c + } + if c := strings.Compare(a.Version, b.Version); c != 0 { + return c + } + return strings.Compare(a.Kind, b.Kind) +} + +func resourceCacheObjectName(object client.Object) string { + if object.GetName() != "" { + return object.GetName() + } + return object.GetGenerateName() +} + +func isNilResourceCacheObject[T client.Object](object T) bool { + var clientObject client.Object = object + if clientObject == nil { + return true + } + + value := reflect.ValueOf(clientObject) + return value.Kind() == reflect.Pointer && value.IsNil() +} diff --git a/controllers/actions.github.com/resourcecache_test.go b/controllers/actions.github.com/resourcecache_test.go new file mode 100644 index 0000000000..e187ac5e0d --- /dev/null +++ b/controllers/actions.github.com/resourcecache_test.go @@ -0,0 +1,277 @@ +package actionsgithubcom + +import ( + "fmt" + "testing" + + "github.com/actions/actions-runner-controller/apis/actions.github.com/v1alpha1" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + rbacv1 "k8s.io/api/rbac/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +func newTestResourceCache() *ResourceCache { + cache := NewResourceCache() + return &cache +} + +func resourceCacheHasMainObjectEntries(cache *ResourceCache, mainObject client.Object) bool { + return resourceCacheStateHasMainObjectEntries(cache.autoscalingListener, mainObject) || + resourceCacheStateHasMainObjectEntries(cache.ephemeralRunnerSet, mainObject) || + resourceCacheStateHasMainObjectEntries(cache.listenerPod, mainObject) || + resourceCacheStateHasMainObjectEntries(cache.listenerServiceAccount, mainObject) || + resourceCacheStateHasMainObjectEntries(cache.listenerRole, mainObject) || + resourceCacheStateHasMainObjectEntries(cache.listenerRoleBinding, mainObject) +} + +func resourceCacheStateHasMainObjectEntries[T client.Object](state *resourceCacheState[T], mainObject client.Object) bool { + uid := mainObject.GetUID() + if uid == "" { + return false + } + + state.mu.RLock() + defer state.mu.RUnlock() + + return len(state.entriesByMainUID[uid]) > 0 +} + +func TestResourceCacheUpsertReplacesByDependencyResourceVersion(t *testing.T) { + mainObject := &v1alpha1.AutoscalingListener{ + ObjectMeta: metav1.ObjectMeta{ + Name: "listener", + Namespace: "controller-ns", + UID: "listener-uid", + ResourceVersion: "10", + }, + } + desiredPod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "listener", + Namespace: "controller-ns", + ResourceVersion: "1", + Labels: map[string]string{ + "app": "listener", + }, + }, + } + configSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "listener-config", + Namespace: "controller-ns", + UID: "config-secret-uid", + ResourceVersion: "1", + }, + } + serviceAccount := &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{ + Name: "listener", + Namespace: "controller-ns", + UID: "service-account-uid", + ResourceVersion: "1", + }, + } + role := &rbacv1.Role{ + ObjectMeta: metav1.ObjectMeta{ + Name: "listener", + Namespace: "scale-set-ns", + UID: "role-uid", + ResourceVersion: "1", + }, + } + + cache := NewResourceCache() + value, replaced := cache.listenerPod.Upsert(mainObject, desiredPod, configSecret, serviceAccount, role) + assert.True(t, replaced) + _, ok := cache.listenerPod.Get(mainObject, desiredPod, configSecret, serviceAccount, role) + assert.True(t, ok) + assert.Equal(t, "1", value.ResourceVersion) + + _, replaced = cache.listenerPod.Upsert(mainObject, desiredPod, role, configSecret, serviceAccount) + assert.False(t, replaced, "dependency ordering should not affect the cache value") + _, ok = cache.listenerPod.Get(mainObject, desiredPod, configSecret, serviceAccount, role) + assert.True(t, ok) + + configSecret.ResourceVersion = "2" + value, replaced = cache.listenerPod.Upsert(mainObject, desiredPod, configSecret, serviceAccount, role) + assert.True(t, replaced) + staleConfigSecret := configSecret.DeepCopy() + staleConfigSecret.ResourceVersion = "1" + _, ok = cache.listenerPod.Get(mainObject, desiredPod, staleConfigSecret, serviceAccount, role) + assert.False(t, ok) + _, ok = cache.listenerPod.Get(mainObject, desiredPod, configSecret, serviceAccount, role) + assert.True(t, ok) + + assert.Same(t, desiredPod, value.Object) +} + +func TestResourceCacheDeleteRemovesMainObjectEntries(t *testing.T) { + mainObject := &v1alpha1.AutoscalingListener{ + ObjectMeta: metav1.ObjectMeta{ + Name: "listener", + Namespace: "controller-ns", + UID: "listener-uid", + }, + } + otherMainObject := &v1alpha1.AutoscalingListener{ + ObjectMeta: metav1.ObjectMeta{ + Name: "other-listener", + Namespace: "controller-ns", + UID: "other-listener-uid", + }, + } + listenerPod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "listener", Namespace: "controller-ns"}} + listenerServiceAccount := &corev1.ServiceAccount{ObjectMeta: metav1.ObjectMeta{Name: "listener", Namespace: "controller-ns"}} + otherListenerPod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "other-listener", Namespace: "controller-ns"}} + + cache := NewResourceCache() + cache.listenerPod.Upsert(mainObject, listenerPod) + cache.listenerServiceAccount.Upsert(mainObject, listenerServiceAccount) + cache.listenerPod.Upsert(otherMainObject, otherListenerPod) + + cache.Delete(mainObject) + + _, ok := cache.listenerPod.Get(mainObject, listenerPod) + assert.False(t, ok) + _, ok = cache.listenerServiceAccount.Get(mainObject, listenerServiceAccount) + assert.False(t, ok) + _, ok = cache.listenerPod.Get(otherMainObject, otherListenerPod) + assert.True(t, ok) +} + +func TestResourceCacheDeletePanicsWithNilCache(t *testing.T) { + var cache *ResourceCache + assert.Panics(t, func() { + cache.Delete(&v1alpha1.AutoscalingListener{}) + }) +} + +func TestResourceCacheIgnoresInvalidInputs(t *testing.T) { + cache := NewResourceCache() + desiredPod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "listener", Namespace: "controller-ns"}} + mainObjectWithoutUID := &v1alpha1.AutoscalingListener{ObjectMeta: metav1.ObjectMeta{Name: "listener", Namespace: "controller-ns"}} + mainObject := mainObjectWithoutUID.DeepCopy() + mainObject.UID = "listener-uid" + + _, replaced := cache.listenerPod.Upsert(mainObjectWithoutUID, desiredPod) + assert.False(t, replaced) + _, ok := cache.listenerPod.Get(mainObjectWithoutUID, desiredPod) + assert.False(t, ok) + + var nilDependency *corev1.Secret + assert.NotPanics(t, func() { + _, replaced = cache.listenerPod.Upsert(mainObject, desiredPod, nilDependency) + assert.False(t, replaced) + _, ok = cache.listenerPod.Get(mainObject, desiredPod, nilDependency) + assert.False(t, ok) + }) + + tooManyDependencies := make([]client.Object, resourceCacheMaxDependencyRefs+1) + for i := range tooManyDependencies { + tooManyDependencies[i] = &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: fmt.Sprintf("dependency-%d", i), Namespace: "controller-ns"}} + } + assert.NotPanics(t, func() { + _, replaced = cache.listenerPod.Upsert(mainObject, desiredPod, tooManyDependencies...) + assert.False(t, replaced) + _, ok = cache.listenerPod.Get(mainObject, desiredPod, tooManyDependencies...) + assert.False(t, ok) + }) +} + +func TestResourceBuilderCachesListenerPodDependencies(t *testing.T) { + listener := &v1alpha1.AutoscalingListener{ + ObjectMeta: metav1.ObjectMeta{ + Name: "listener", + Namespace: "controller-ns", + UID: "listener-uid", + }, + Spec: v1alpha1.AutoscalingListenerSpec{ + Image: "listener:latest", + AutoscalingRunnerSetName: "scale-set", + AutoscalingRunnerSetNamespace: "scale-set-ns", + EphemeralRunnerSetName: "scale-set", + }, + } + podConfig := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "listener-config", + Namespace: "controller-ns", + UID: "config-secret-uid", + ResourceVersion: "11", + }, + } + serviceAccount := &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{ + Name: "listener", + Namespace: "controller-ns", + UID: "service-account-uid", + ResourceVersion: "12", + }, + } + role := &rbacv1.Role{ + ObjectMeta: metav1.ObjectMeta{ + Name: "listener", + Namespace: "scale-set-ns", + UID: "role-uid", + ResourceVersion: "13", + }, + } + roleBinding := &rbacv1.RoleBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: "listener", + Namespace: "scale-set-ns", + UID: "role-binding-uid", + ResourceVersion: "14", + }, + } + + cache := NewResourceCache() + b := ResourceBuilder{ResourceCache: &cache} + listenerPod, err := b.newScaleSetListenerPod(listener, podConfig, serviceAccount, role, roleBinding, nil) + require.NoError(t, err) + + cachedPod, ok := b.ResourceCache.listenerPod.Get(listener, listenerPod, podConfig, serviceAccount, role, roleBinding) + require.True(t, ok) + assert.IsType(t, &corev1.Pod{}, cachedPod) + + role.ResourceVersion = "changed" + _, ok = b.ResourceCache.listenerPod.Get(listener, listenerPod, podConfig, serviceAccount, role, roleBinding) + assert.False(t, ok) +} + +func TestResourceBuilderCachesEphemeralRunnerSet(t *testing.T) { + autoscalingRunnerSet := v1alpha1.AutoscalingRunnerSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "scale-set", + Namespace: "default", + UID: "scale-set-uid", + Annotations: map[string]string{ + runnerScaleSetIDAnnotationKey: "1", + }, + }, + Spec: v1alpha1.AutoscalingRunnerSetSpec{ + GitHubConfigUrl: "https://github.com/actions/actions-runner-controller", + }, + } + + cache := NewResourceCache() + b := ResourceBuilder{ResourceCache: &cache} + runnerSet, err := b.newEphemeralRunnerSet(&autoscalingRunnerSet) + require.NoError(t, err) + + cachedRunnerSet, ok := b.ResourceCache.ephemeralRunnerSet.Get(&autoscalingRunnerSet, runnerSet) + require.True(t, ok) + assert.Equal(t, runnerSet.Spec, cachedRunnerSet.Spec) + assert.Same(t, runnerSet, cachedRunnerSet) + + fromBuilder, err := b.newEphemeralRunnerSet(&autoscalingRunnerSet) + require.NoError(t, err) + assert.Same(t, runnerSet, fromBuilder) + + autoscalingRunnerSet.Annotations[runnerScaleSetIDAnnotationKey] = "2" + _, ok = b.ResourceCache.ephemeralRunnerSet.Get(&autoscalingRunnerSet, runnerSet) + assert.False(t, ok) +} diff --git a/controllers/actions.github.com/utils.go b/controllers/actions.github.com/utils.go index a77b24ba17..482c95d45f 100644 --- a/controllers/actions.github.com/utils.go +++ b/controllers/actions.github.com/utils.go @@ -1,8 +1,6 @@ package actionsgithubcom -import ( - "k8s.io/apimachinery/pkg/util/rand" -) +import "sigs.k8s.io/controller-runtime/pkg/client" func FilterLabels(labels map[string]string, filter string) map[string]string { filtered := map[string]string{} @@ -16,12 +14,33 @@ func FilterLabels(labels map[string]string, filter string) map[string]string { return filtered } -var letterRunes = []rune("abcdefghijklmnopqrstuvwxyz1234567890") +type once[T client.Object] struct { + value T + fn func() T + done bool +} -func RandStringRunes(n int) string { - b := make([]rune, n) - for i := range b { - b[i] = letterRunes[rand.Intn(len(letterRunes))] +func newOnce[T client.Object](fn func() T) *once[T] { + return &once[T]{ + fn: fn, } - return string(b) +} + +func (o *once[T]) Do() T { + if !o.done { + o.value = o.fn() + o.done = true + } + return o.value +} + +func (o *once[T]) Get() T { + if !o.Called() { + panic("not done") + } + return o.value +} + +func (o *once[T]) Called() bool { + return o.done } diff --git a/controllers/actions.github.com/utils_test.go b/controllers/actions.github.com/utils_test.go index 9e98b981bd..d0c35d1692 100644 --- a/controllers/actions.github.com/utils_test.go +++ b/controllers/actions.github.com/utils_test.go @@ -3,6 +3,8 @@ package actionsgithubcom import ( "reflect" "testing" + + "k8s.io/apimachinery/pkg/util/rand" ) func Test_filterLabels(t *testing.T) { @@ -32,3 +34,13 @@ func Test_filterLabels(t *testing.T) { }) } } + +var letterRunes = []rune("abcdefghijklmnopqrstuvwxyz1234567890") + +func RandStringRunes(n int) string { + b := make([]rune, n) + for i := range b { + b[i] = letterRunes[rand.Intn(len(letterRunes))] + } + return string(b) +} diff --git a/main.go b/main.go index 72c65f6bbb..5ec4a56645 100644 --- a/main.go +++ b/main.go @@ -218,6 +218,7 @@ func main() { } actionsgithubcom.SetListenerEntrypoint(os.Getenv("LISTENER_ENTRYPOINT")) + resourceCache := actionsgithubcom.NewResourceCache() var webhookServer webhook.Server if port != 0 { @@ -303,6 +304,7 @@ func main() { ExcludeLabelPropagationPrefixes: excludeLabelPropagationPrefixes, SecretResolver: secretResolver, Scheme: mgr.GetScheme(), + ResourceCache: &resourceCache, } log.Info("Resource builder initializing")