Track AutoscalingRunnerSet updates with metadata.generation

apis/actions.github.com/v1alpha1/autoscalingrunnerset_types.go

@@ -23,7 +23,6 @@ import (
 	"net/url"
 	"strings"
 
-	"github.com/actions/actions-runner-controller/hash"
 	"github.com/actions/actions-runner-controller/vault"
 	"golang.org/x/net/http/httpproxy"
 	corev1 "k8s.io/api/core/v1"
@@ -322,6 +321,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
@@ -334,26 +337,6 @@ const (
 	AutoscalingRunnerSetPhaseOutdated AutoscalingRunnerSetPhase = "Outdated"
 )
 
-func (ars *AutoscalingRunnerSet) Hash() string {
-	type data struct {
-		Spec   *AutoscalingRunnerSetSpec
-		Labels map[string]string
-	}
-
-	d := &data{
-		Spec:   ars.Spec.DeepCopy(),
-		Labels: ars.Labels,
-	}
-
-	return hash.ComputeTemplateHash(d)
-}
-
-func (ars *AutoscalingRunnerSet) ListenerSpecHash() string {
-	arsSpec := ars.Spec.DeepCopy()
-	spec := arsSpec
-	return hash.ComputeTemplateHash(&spec)
-}
-
 func (ars *AutoscalingRunnerSet) GitHubConfigSecret() string {
 	return ars.Spec.GitHubConfigSecret
 }
@@ -381,28 +364,6 @@ func (ars *AutoscalingRunnerSet) VaultProxy() *ProxyConfig {
 	return nil
 }
 
-func (ars *AutoscalingRunnerSet) RunnerSetSpecHash() string {
-	type runnerSetSpec struct {
-		GitHubConfigUrl    string
-		GitHubConfigSecret string
-		RunnerGroup        string
-		RunnerScaleSetName string
-		Proxy              *ProxyConfig
-		GitHubServerTLS    *TLSConfig
-		Template           corev1.PodTemplateSpec
-	}
-	spec := &runnerSetSpec{
-		GitHubConfigUrl:    ars.Spec.GitHubConfigUrl,
-		GitHubConfigSecret: ars.Spec.GitHubConfigSecret,
-		RunnerGroup:        ars.Spec.RunnerGroup,
-		RunnerScaleSetName: ars.Spec.RunnerScaleSetName,
-		Proxy:              ars.Spec.Proxy,
-		GitHubServerTLS:    ars.Spec.GitHubServerTLS,
-		Template:           ars.Spec.Template,
-	}
-	return hash.ComputeTemplateHash(&spec)
-}
-
 // +kubebuilder:object:root=true
 
 // AutoscalingRunnerSetList contains a list of AutoscalingRunnerSet

controllers/actions.github.com/autoscalingrunnerset_controller.go

@@ -143,27 +143,22 @@ 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 {
+	// The spec changed since we last observed it, so move back to the pending
+	// phase. The observed generation is deliberately left at its old value here:
+	// it only catches up at the end of a successful reconcile, so a reconcile
+	// that fails half way through is retried as pending rather than being
+	// mistaken for settled.
+	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
@@ -378,6 +373,25 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl
 		if !cmp.Equal(listener.Spec, desired.Spec) ||
 			!cmp.Equal(listener.Labels, desired.Labels) ||
 			!cmp.Equal(listener.Annotations, desired.Annotations) {
+			// The listener is about to be torn down and rebuilt, which is what
+			// the pending phase means. Report it here rather than relying on the
+			// generation check above: the desired listener is derived from the
+			// AutoscalingRunnerSet's labels and annotations as well as its spec,
+			// and metadata writes do not bump metadata.generation. Without this,
+			// a label-only edit would leave the scale set claiming to be running
+			// while it has no listener at all, and it would keep claiming that
+			// if the rebuild never succeeded.
+			if err := r.updateStatus(
+				ctx,
+				&autoscalingRunnerSet,
+				v1alpha1.AutoscalingRunnerSetPhasePending,
+				autoscalingRunnerSet.Status.ObservedGeneration,
+				log,
+			); err != nil {
+				log.Error(err, "Failed to update autoscaling runner set status before re-creating the listener")
+				return ctrl.Result{}, err
+			}
+
 			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")
@@ -393,6 +407,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")
@@ -437,14 +452,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")