diff --git a/charts/gha-runner-scale-set/templates/autoscalingrunnerset.yaml b/charts/gha-runner-scale-set/templates/autoscalingrunnerset.yaml index 27460cc2..39ebfef8 100644 --- a/charts/gha-runner-scale-set/templates/autoscalingrunnerset.yaml +++ b/charts/gha-runner-scale-set/templates/autoscalingrunnerset.yaml @@ -13,6 +13,7 @@ metadata: app.kubernetes.io/component: "autoscaling-runner-set" {{- include "gha-runner-scale-set.labels" . | nindent 4 }} annotations: + actions.github.com/values-hash: {{ toJson .Values | sha256sum | trunc 63 }} {{- $containerMode := .Values.containerMode }} {{- if not (kindIs "string" .Values.githubConfigSecret) }} actions.github.com/cleanup-github-secret-name: {{ include "gha-runner-scale-set.githubsecret" . }} diff --git a/charts/gha-runner-scale-set/tests/template_test.go b/charts/gha-runner-scale-set/tests/template_test.go index 10f3beb6..070f1ef1 100644 --- a/charts/gha-runner-scale-set/tests/template_test.go +++ b/charts/gha-runner-scale-set/tests/template_test.go @@ -2088,3 +2088,58 @@ func TestRunnerContainerVolumeNotEmptyMap(t *testing.T) { _, ok := m.Spec.Template.Spec.Containers[0]["volumeMounts"] assert.False(t, ok, "volumeMounts should not be set") } + +func TestAutoscalingRunnerSetAnnotationValuesHash(t *testing.T) { + t.Parallel() + + const valuesHash = "actions.github.com/values-hash" + + // Path to the helm chart we will test + helmChartPath, err := filepath.Abs("../../gha-runner-scale-set") + require.NoError(t, err) + + releaseName := "test-runners" + namespaceName := "test-" + strings.ToLower(random.UniqueId()) + + options := &helm.Options{ + Logger: logger.Discard, + SetValues: map[string]string{ + "githubConfigUrl": "https://github.com/actions", + "githubConfigSecret.github_token": "gh_token12345", + "controllerServiceAccount.name": "arc", + "controllerServiceAccount.namespace": "arc-system", + }, + KubectlOptions: k8s.NewKubectlOptions("", "", namespaceName), + } + + output := helm.RenderTemplate(t, options, helmChartPath, releaseName, []string{"templates/autoscalingrunnerset.yaml"}) + + var autoscalingRunnerSet v1alpha1.AutoscalingRunnerSet + helm.UnmarshalK8SYaml(t, output, &autoscalingRunnerSet) + + firstHash := autoscalingRunnerSet.Annotations["actions.github.com/values-hash"] + assert.NotEmpty(t, firstHash) + assert.LessOrEqual(t, len(firstHash), 63) + + helmChartPath, err = filepath.Abs("../../gha-runner-scale-set") + require.NoError(t, err) + + options = &helm.Options{ + Logger: logger.Discard, + SetValues: map[string]string{ + "githubConfigUrl": "https://github.com/actions", + "githubConfigSecret.github_token": "gh_token1234567890", + "controllerServiceAccount.name": "arc", + "controllerServiceAccount.namespace": "arc-system", + }, + KubectlOptions: k8s.NewKubectlOptions("", "", namespaceName), + } + + output = helm.RenderTemplate(t, options, helmChartPath, releaseName, []string{"templates/autoscalingrunnerset.yaml"}) + + helm.UnmarshalK8SYaml(t, output, &autoscalingRunnerSet) + secondHash := autoscalingRunnerSet.Annotations[valuesHash] + assert.NotEmpty(t, secondHash) + assert.NotEqual(t, firstHash, secondHash) + assert.LessOrEqual(t, len(secondHash), 63) +} diff --git a/controllers/actions.github.com/autoscalingrunnerset_controller.go b/controllers/actions.github.com/autoscalingrunnerset_controller.go index 773541b8..1d476e37 100644 --- a/controllers/actions.github.com/autoscalingrunnerset_controller.go +++ b/controllers/actions.github.com/autoscalingrunnerset_controller.go @@ -42,7 +42,12 @@ import ( ) const ( - labelKeyRunnerSpecHash = "runner-spec-hash" + annotationKeyRunnerSpecHash = "actions.github.com/runner-spec-hash" + // annotationKeyValuesHash is hash of the entire values json. + // This is used to determine if the values have changed, so we can + // re-create listener. + annotationKeyValuesHash = "actions.github.com/values-hash" + autoscalingRunnerSetFinalizerName = "autoscalingrunnerset.actions.github.com/finalizer" runnerScaleSetIdAnnotationKey = "runner-scale-set-id" ) @@ -230,9 +235,8 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl return r.createEphemeralRunnerSet(ctx, autoscalingRunnerSet, log) } - desiredSpecHash := autoscalingRunnerSet.RunnerSetSpecHash() for _, runnerSet := range existingRunnerSets.all() { - log.Info("Find existing ephemeral runner set", "name", runnerSet.Name, "specHash", runnerSet.Labels[labelKeyRunnerSpecHash]) + log.Info("Find existing ephemeral runner set", "name", runnerSet.Name, "specHash", runnerSet.Annotations[annotationKeyRunnerSpecHash]) } // Make sure the AutoscalingListener is up and running in the controller namespace @@ -249,7 +253,9 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl } // Our listener pod is out of date, so we need to delete it to get a new recreate. - if listenerFound && (listener.Labels[labelKeyRunnerSpecHash] != autoscalingRunnerSet.ListenerSpecHash()) { + listenerValuesHashChanged := listener.Annotations[annotationKeyValuesHash] != autoscalingRunnerSet.Annotations[annotationKeyValuesHash] + listenerSpecHashChanged := listener.Annotations[annotationKeyRunnerSpecHash] != autoscalingRunnerSet.ListenerSpecHash() + if listenerFound && (listenerValuesHashChanged || listenerSpecHashChanged) { log.Info("RunnerScaleSetListener is out of date. Deleting it so that it is recreated", "name", listener.Name) if err := r.Delete(ctx, listener); err != nil { if kerrors.IsNotFound(err) { @@ -263,7 +269,7 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl return ctrl.Result{}, nil } - if desiredSpecHash != latestRunnerSet.Labels[labelKeyRunnerSpecHash] { + if latestRunnerSet.Annotations[annotationKeyRunnerSpecHash] != autoscalingRunnerSet.RunnerSetSpecHash() { if r.drainingJobs(&latestRunnerSet.Status) { log.Info("Latest runner set spec hash does not match the current autoscaling runner set. Waiting for the running and pending runners to finish:", "running", latestRunnerSet.Status.RunningEphemeralRunners, "pending", latestRunnerSet.Status.PendingEphemeralRunners) log.Info("Scaling down the number of desired replicas to 0") diff --git a/controllers/actions.github.com/autoscalingrunnerset_controller_test.go b/controllers/actions.github.com/autoscalingrunnerset_controller_test.go index ab325185..783d7def 100644 --- a/controllers/actions.github.com/autoscalingrunnerset_controller_test.go +++ b/controllers/actions.github.com/autoscalingrunnerset_controller_test.go @@ -280,6 +280,10 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { // This should trigger re-creation of EphemeralRunnerSet and Listener patched := autoscalingRunnerSet.DeepCopy() patched.Spec.Template.Spec.PriorityClassName = "test-priority-class" + if patched.ObjectMeta.Annotations == nil { + patched.ObjectMeta.Annotations = make(map[string]string) + } + patched.ObjectMeta.Annotations[annotationKeyValuesHash] = "test-hash" err = k8sClient.Patch(ctx, patched, client.MergeFrom(autoscalingRunnerSet)) Expect(err).NotTo(HaveOccurred(), "failed to patch AutoScalingRunnerSet") autoscalingRunnerSet = patched.DeepCopy() @@ -297,10 +301,10 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { return "", fmt.Errorf("We should have only 1 EphemeralRunnerSet, but got %v", len(runnerSetList.Items)) } - return runnerSetList.Items[0].Labels[labelKeyRunnerSpecHash], nil + return runnerSetList.Items[0].Annotations[annotationKeyRunnerSpecHash], nil }, autoscalingRunnerSetTestTimeout, - autoscalingRunnerSetTestInterval).ShouldNot(BeEquivalentTo(runnerSet.Labels[labelKeyRunnerSpecHash]), "New EphemeralRunnerSet should be created") + autoscalingRunnerSetTestInterval).ShouldNot(BeEquivalentTo(runnerSet.Annotations[annotationKeyRunnerSpecHash]), "New EphemeralRunnerSet should be created") // We should create a new listener Eventually( @@ -334,6 +338,55 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { err = k8sClient.Patch(ctx, patched, client.MergeFrom(autoscalingRunnerSet)) Expect(err).NotTo(HaveOccurred(), "failed to patch AutoScalingRunnerSet") + // We should not re-create a new EphemeralRunnerSet + Consistently( + func() (string, error) { + runnerSetList := new(v1alpha1.EphemeralRunnerSetList) + err := k8sClient.List(ctx, runnerSetList, client.InNamespace(autoscalingRunnerSet.Namespace)) + if err != nil { + return "", err + } + + if len(runnerSetList.Items) != 1 { + return "", fmt.Errorf("We should have only 1 EphemeralRunnerSet, but got %v", len(runnerSetList.Items)) + } + + return string(runnerSetList.Items[0].UID), nil + }, + autoscalingRunnerSetTestTimeout, + autoscalingRunnerSetTestInterval).Should(BeEquivalentTo(string(runnerSet.UID)), "New EphemeralRunnerSet should not be created") + + // We should only re-create a new listener + Eventually( + func() (string, error) { + listener := new(v1alpha1.AutoscalingListener) + err := k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerName(autoscalingRunnerSet), Namespace: autoscalingRunnerSet.Namespace}, listener) + if err != nil { + return "", err + } + + return string(listener.UID), nil + }, + autoscalingRunnerSetTestTimeout, + autoscalingRunnerSetTestInterval).ShouldNot(BeEquivalentTo(string(listener.UID)), "New Listener should be created") + + // Only update the values hash for the autoscaling runner set + // This should trigger re-creation of the Listener only + runnerSetList = new(v1alpha1.EphemeralRunnerSetList) + err = k8sClient.List(ctx, runnerSetList, client.InNamespace(autoscalingRunnerSet.Namespace)) + Expect(err).NotTo(HaveOccurred(), "failed to list EphemeralRunnerSet") + Expect(len(runnerSetList.Items)).To(Equal(1), "There should be 1 EphemeralRunnerSet") + runnerSet = runnerSetList.Items[0] + + listener = new(v1alpha1.AutoscalingListener) + err = k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerName(autoscalingRunnerSet), Namespace: autoscalingRunnerSet.Namespace}, listener) + Expect(err).NotTo(HaveOccurred(), "failed to get Listener") + + patched = autoscalingRunnerSet.DeepCopy() + patched.ObjectMeta.Annotations[annotationKeyValuesHash] = "hash-changes" + err = k8sClient.Patch(ctx, patched, client.MergeFrom(autoscalingRunnerSet)) + Expect(err).NotTo(HaveOccurred(), "failed to patch AutoScalingRunnerSet") + // We should not re-create a new EphemeralRunnerSet Consistently( func() (string, error) { @@ -493,6 +546,10 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { // Patch the AutoScalingRunnerSet image which should trigger // the recreation of the Listener and EphemeralRunnerSet patched := autoscalingRunnerSet.DeepCopy() + if patched.ObjectMeta.Annotations == nil { + patched.ObjectMeta.Annotations = make(map[string]string) + } + patched.ObjectMeta.Annotations[annotationKeyValuesHash] = "testgroup2" patched.Spec.Template.Spec = corev1.PodSpec{ Containers: []corev1.Container{ { @@ -501,7 +558,6 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { }, }, } - // patched.Spec.Template.Spec.PriorityClassName = "test-priority-class" err = k8sClient.Patch(ctx, patched, client.MergeFrom(autoscalingRunnerSet)) Expect(err).NotTo(HaveOccurred(), "failed to patch AutoScalingRunnerSet") autoscalingRunnerSet = patched.DeepCopy() diff --git a/controllers/actions.github.com/resourcebuilder.go b/controllers/actions.github.com/resourcebuilder.go index b8cff486..9ab5e218 100644 --- a/controllers/actions.github.com/resourcebuilder.go +++ b/controllers/actions.github.com/resourcebuilder.go @@ -91,7 +91,11 @@ func (b *resourceBuilder) newAutoScalingListener(autoscalingRunnerSet *v1alpha1. LabelKeyKubernetesPartOf: labelValueKubernetesPartOf, LabelKeyKubernetesComponent: "runner-scale-set-listener", LabelKeyKubernetesVersion: autoscalingRunnerSet.Labels[LabelKeyKubernetesVersion], - labelKeyRunnerSpecHash: autoscalingRunnerSet.ListenerSpecHash(), + } + + annotations := map[string]string{ + annotationKeyRunnerSpecHash: autoscalingRunnerSet.ListenerSpecHash(), + annotationKeyValuesHash: autoscalingRunnerSet.Annotations[annotationKeyValuesHash], } if err := applyGitHubURLLabels(autoscalingRunnerSet.Spec.GitHubConfigUrl, labels); err != nil { @@ -100,9 +104,10 @@ func (b *resourceBuilder) newAutoScalingListener(autoscalingRunnerSet *v1alpha1. autoscalingListener := &v1alpha1.AutoscalingListener{ ObjectMeta: metav1.ObjectMeta{ - Name: scaleSetListenerName(autoscalingRunnerSet), - Namespace: namespace, - Labels: labels, + Name: scaleSetListenerName(autoscalingRunnerSet), + Namespace: namespace, + Labels: labels, + Annotations: annotations, }, Spec: v1alpha1.AutoscalingListenerSpec{ GitHubConfigUrl: autoscalingRunnerSet.Spec.GitHubConfigUrl, @@ -498,7 +503,6 @@ func (b *resourceBuilder) newEphemeralRunnerSet(autoscalingRunnerSet *v1alpha1.A runnerSpecHash := autoscalingRunnerSet.RunnerSetSpecHash() labels := map[string]string{ - labelKeyRunnerSpecHash: runnerSpecHash, LabelKeyKubernetesPartOf: labelValueKubernetesPartOf, LabelKeyKubernetesComponent: "runner-set", LabelKeyKubernetesVersion: autoscalingRunnerSet.Labels[LabelKeyKubernetesVersion], @@ -511,8 +515,10 @@ func (b *resourceBuilder) newEphemeralRunnerSet(autoscalingRunnerSet *v1alpha1.A } newAnnotations := map[string]string{ + AnnotationKeyGitHubRunnerGroupName: autoscalingRunnerSet.Annotations[AnnotationKeyGitHubRunnerGroupName], AnnotationKeyGitHubRunnerScaleSetName: autoscalingRunnerSet.Annotations[AnnotationKeyGitHubRunnerScaleSetName], + annotationKeyRunnerSpecHash: runnerSpecHash, } newEphemeralRunnerSet := &v1alpha1.EphemeralRunnerSet{ diff --git a/controllers/actions.github.com/resourcebuilder_test.go b/controllers/actions.github.com/resourcebuilder_test.go index 4257ae31..9039de95 100644 --- a/controllers/actions.github.com/resourcebuilder_test.go +++ b/controllers/actions.github.com/resourcebuilder_test.go @@ -39,7 +39,7 @@ func TestLabelPropagation(t *testing.T) { 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.Labels[labelKeyRunnerSpecHash]) + assert.NotEmpty(t, ephemeralRunnerSet.Annotations[annotationKeyRunnerSpecHash]) assert.Equal(t, autoscalingRunnerSet.Name, ephemeralRunnerSet.Labels[LabelKeyGitHubScaleSetName]) assert.Equal(t, autoscalingRunnerSet.Namespace, ephemeralRunnerSet.Labels[LabelKeyGitHubScaleSetNamespace]) assert.Equal(t, "", ephemeralRunnerSet.Labels[LabelKeyGitHubEnterprise]) @@ -53,7 +53,7 @@ func TestLabelPropagation(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.Labels[labelKeyRunnerSpecHash]) + assert.NotEmpty(t, ephemeralRunnerSet.Annotations[annotationKeyRunnerSpecHash]) assert.Equal(t, autoscalingRunnerSet.Name, listener.Labels[LabelKeyGitHubScaleSetName]) assert.Equal(t, autoscalingRunnerSet.Namespace, listener.Labels[LabelKeyGitHubScaleSetNamespace]) assert.Equal(t, "", listener.Labels[LabelKeyGitHubEnterprise])