From 95487735a2cf065d8212497964949adaee8634be Mon Sep 17 00:00:00 2001 From: Nikola Jokic Date: Tue, 7 Nov 2023 15:08:36 +0100 Subject: [PATCH] Remove inheritance of imagePullPolicy from manager to listeners (#3009) --- .github/workflows/arc-publish-chart.yaml | 2 +- .github/workflows/arc-validate-chart.yaml | 6 +-- .github/workflows/gha-validate-chart.yaml | 6 +-- .../templates/NOTES.txt | 1 - .../templates/deployment.yaml | 2 - .../tests/template_test.go | 42 ++++++++----------- controllers/actions.github.com/constants.go | 5 --- .../actions.github.com/resourcebuilder.go | 24 ++--------- main.go | 7 ---- 9 files changed, 29 insertions(+), 66 deletions(-) diff --git a/.github/workflows/arc-publish-chart.yaml b/.github/workflows/arc-publish-chart.yaml index f0290797..56356986 100644 --- a/.github/workflows/arc-publish-chart.yaml +++ b/.github/workflows/arc-publish-chart.yaml @@ -63,7 +63,7 @@ jobs: python-version: '3.11' - name: Set up chart-testing - uses: helm/chart-testing-action@v2.3.1 + uses: helm/chart-testing-action@v2.6 - name: Run chart-testing (list-changed) id: list-changed diff --git a/.github/workflows/arc-validate-chart.yaml b/.github/workflows/arc-validate-chart.yaml index 7cdc5833..dc04eab4 100644 --- a/.github/workflows/arc-validate-chart.yaml +++ b/.github/workflows/arc-validate-chart.yaml @@ -28,7 +28,7 @@ permissions: contents: read concurrency: - # This will make sure we only apply the concurrency limits on pull requests + # This will make sure we only apply the concurrency limits on pull requests # but not pushes to master branch by making the concurrency group name unique # for pushes group: ${{ github.workflow }}-${{ github.head_ref || github.run_id }} @@ -69,10 +69,10 @@ jobs: # python is a requirement for the chart-testing action below (supports yamllint among other tests) - uses: actions/setup-python@v4 with: - python-version: '3.7' + python-version: '3.11' - name: Set up chart-testing - uses: helm/chart-testing-action@v2.4.0 + uses: helm/chart-testing-action@v2.6.0 - name: Run chart-testing (list-changed) id: list-changed diff --git a/.github/workflows/gha-validate-chart.yaml b/.github/workflows/gha-validate-chart.yaml index 0dd64fb2..ab6735ec 100644 --- a/.github/workflows/gha-validate-chart.yaml +++ b/.github/workflows/gha-validate-chart.yaml @@ -24,7 +24,7 @@ permissions: contents: read concurrency: - # This will make sure we only apply the concurrency limits on pull requests + # This will make sure we only apply the concurrency limits on pull requests # but not pushes to master branch by making the concurrency group name unique # for pushes group: ${{ github.workflow }}-${{ github.head_ref || github.run_id }} @@ -65,10 +65,10 @@ jobs: # python is a requirement for the chart-testing action below (supports yamllint among other tests) - uses: actions/setup-python@v4 with: - python-version: '3.7' + python-version: '3.11' - name: Set up chart-testing - uses: helm/chart-testing-action@v2.4.0 + uses: helm/chart-testing-action@v2.6.0 - name: Run chart-testing (list-changed) id: list-changed diff --git a/charts/gha-runner-scale-set-controller/templates/NOTES.txt b/charts/gha-runner-scale-set-controller/templates/NOTES.txt index f1af5b23..cb50ff58 100644 --- a/charts/gha-runner-scale-set-controller/templates/NOTES.txt +++ b/charts/gha-runner-scale-set-controller/templates/NOTES.txt @@ -2,4 +2,3 @@ Thank you for installing {{ .Chart.Name }}. Your release is named {{ .Release.Name }}. -WARNING: value specified under image.pullPolicy will be ignored and no longer be applied to the listener pod spec as of gha-runner-scale-set-0.7.0. Please use the listenerTemplate in the gha-runner-scale-set chart to control the image pull policy of the listener. diff --git a/charts/gha-runner-scale-set-controller/templates/deployment.yaml b/charts/gha-runner-scale-set-controller/templates/deployment.yaml index bdec673d..1b857b37 100644 --- a/charts/gha-runner-scale-set-controller/templates/deployment.yaml +++ b/charts/gha-runner-scale-set-controller/templates/deployment.yaml @@ -94,8 +94,6 @@ spec: valueFrom: fieldRef: fieldPath: metadata.namespace - - name: CONTROLLER_MANAGER_LISTENER_IMAGE_PULL_POLICY - value: "{{ .Values.image.pullPolicy | default "IfNotPresent" }}" {{- with .Values.env }} {{- if kindIs "slice" . }} {{- toYaml . | nindent 8 }} diff --git a/charts/gha-runner-scale-set-controller/tests/template_test.go b/charts/gha-runner-scale-set-controller/tests/template_test.go index d689c17b..a2709b2b 100644 --- a/charts/gha-runner-scale-set-controller/tests/template_test.go +++ b/charts/gha-runner-scale-set-controller/tests/template_test.go @@ -368,14 +368,12 @@ func TestTemplate_ControllerDeployment_Defaults(t *testing.T) { } assert.ElementsMatch(t, expectedArgs, deployment.Spec.Template.Spec.Containers[0].Args) - assert.Len(t, deployment.Spec.Template.Spec.Containers[0].Env, 3) + assert.Len(t, deployment.Spec.Template.Spec.Containers[0].Env, 2) assert.Equal(t, "CONTROLLER_MANAGER_CONTAINER_IMAGE", deployment.Spec.Template.Spec.Containers[0].Env[0].Name) assert.Equal(t, managerImage, deployment.Spec.Template.Spec.Containers[0].Env[0].Value) assert.Equal(t, "CONTROLLER_MANAGER_POD_NAMESPACE", deployment.Spec.Template.Spec.Containers[0].Env[1].Name) assert.Equal(t, "metadata.namespace", deployment.Spec.Template.Spec.Containers[0].Env[1].ValueFrom.FieldRef.FieldPath) - assert.Equal(t, "CONTROLLER_MANAGER_LISTENER_IMAGE_PULL_POLICY", deployment.Spec.Template.Spec.Containers[0].Env[2].Name) - assert.Equal(t, "IfNotPresent", deployment.Spec.Template.Spec.Containers[0].Env[2].Value) // default value. Needs to align with controllers/actions.github.com/resourcebuilder.go assert.Empty(t, deployment.Spec.Template.Spec.Containers[0].Resources) assert.Nil(t, deployment.Spec.Template.Spec.Containers[0].SecurityContext) @@ -459,8 +457,8 @@ func TestTemplate_ControllerDeployment_Customize(t *testing.T) { assert.Equal(t, "bar", deployment.Spec.Template.Annotations["foo"]) assert.Equal(t, "manager", deployment.Spec.Template.Annotations["kubectl.kubernetes.io/default-container"]) - assert.Equal(t, "ENV_VAR_NAME_1", deployment.Spec.Template.Spec.Containers[0].Env[3].Name) - assert.Equal(t, "ENV_VAR_VALUE_1", deployment.Spec.Template.Spec.Containers[0].Env[3].Value) + assert.Equal(t, "ENV_VAR_NAME_1", deployment.Spec.Template.Spec.Containers[0].Env[2].Name) + assert.Equal(t, "ENV_VAR_VALUE_1", deployment.Spec.Template.Spec.Containers[0].Env[2].Value) assert.Len(t, deployment.Spec.Template.Spec.ImagePullSecrets, 1) assert.Equal(t, "dockerhub", deployment.Spec.Template.Spec.ImagePullSecrets[0].Name) @@ -505,17 +503,15 @@ func TestTemplate_ControllerDeployment_Customize(t *testing.T) { assert.ElementsMatch(t, expectArgs, deployment.Spec.Template.Spec.Containers[0].Args) - assert.Len(t, deployment.Spec.Template.Spec.Containers[0].Env, 4) + assert.Len(t, deployment.Spec.Template.Spec.Containers[0].Env, 3) assert.Equal(t, "CONTROLLER_MANAGER_CONTAINER_IMAGE", deployment.Spec.Template.Spec.Containers[0].Env[0].Name) assert.Equal(t, managerImage, deployment.Spec.Template.Spec.Containers[0].Env[0].Value) assert.Equal(t, "CONTROLLER_MANAGER_POD_NAMESPACE", deployment.Spec.Template.Spec.Containers[0].Env[1].Name) assert.Equal(t, "metadata.namespace", deployment.Spec.Template.Spec.Containers[0].Env[1].ValueFrom.FieldRef.FieldPath) - assert.Equal(t, "CONTROLLER_MANAGER_LISTENER_IMAGE_PULL_POLICY", deployment.Spec.Template.Spec.Containers[0].Env[2].Name) - assert.Equal(t, "Always", deployment.Spec.Template.Spec.Containers[0].Env[2].Value) // default value. Needs to align with controllers/actions.github.com/resourcebuilder.go - assert.Equal(t, "ENV_VAR_NAME_1", deployment.Spec.Template.Spec.Containers[0].Env[3].Name) - assert.Equal(t, "ENV_VAR_VALUE_1", deployment.Spec.Template.Spec.Containers[0].Env[3].Value) + assert.Equal(t, "ENV_VAR_NAME_1", deployment.Spec.Template.Spec.Containers[0].Env[2].Name) + assert.Equal(t, "ENV_VAR_VALUE_1", deployment.Spec.Template.Spec.Containers[0].Env[2].Value) assert.Equal(t, "500m", deployment.Spec.Template.Spec.Containers[0].Resources.Limits.Cpu().String()) assert.True(t, *deployment.Spec.Template.Spec.Containers[0].SecurityContext.RunAsNonRoot) @@ -762,14 +758,12 @@ func TestTemplate_ControllerDeployment_WatchSingleNamespace(t *testing.T) { assert.ElementsMatch(t, expectedArgs, deployment.Spec.Template.Spec.Containers[0].Args) - assert.Len(t, deployment.Spec.Template.Spec.Containers[0].Env, 3) + assert.Len(t, deployment.Spec.Template.Spec.Containers[0].Env, 2) assert.Equal(t, "CONTROLLER_MANAGER_CONTAINER_IMAGE", deployment.Spec.Template.Spec.Containers[0].Env[0].Name) assert.Equal(t, managerImage, deployment.Spec.Template.Spec.Containers[0].Env[0].Value) assert.Equal(t, "CONTROLLER_MANAGER_POD_NAMESPACE", deployment.Spec.Template.Spec.Containers[0].Env[1].Name) assert.Equal(t, "metadata.namespace", deployment.Spec.Template.Spec.Containers[0].Env[1].ValueFrom.FieldRef.FieldPath) - assert.Equal(t, "CONTROLLER_MANAGER_LISTENER_IMAGE_PULL_POLICY", deployment.Spec.Template.Spec.Containers[0].Env[2].Name) - assert.Equal(t, "IfNotPresent", deployment.Spec.Template.Spec.Containers[0].Env[2].Value) // default value. Needs to align with controllers/actions.github.com/resourcebuilder.go assert.Empty(t, deployment.Spec.Template.Spec.Containers[0].Resources) assert.Nil(t, deployment.Spec.Template.Spec.Containers[0].SecurityContext) @@ -812,17 +806,17 @@ func TestTemplate_ControllerContainerEnvironmentVariables(t *testing.T) { assert.Equal(t, namespaceName, deployment.Namespace) assert.Equal(t, "test-arc-gha-rs-controller", deployment.Name) - assert.Len(t, deployment.Spec.Template.Spec.Containers[0].Env, 7) - assert.Equal(t, "ENV_VAR_NAME_1", deployment.Spec.Template.Spec.Containers[0].Env[3].Name) - assert.Equal(t, "ENV_VAR_VALUE_1", deployment.Spec.Template.Spec.Containers[0].Env[3].Value) - assert.Equal(t, "ENV_VAR_NAME_2", deployment.Spec.Template.Spec.Containers[0].Env[4].Name) - assert.Equal(t, "secret-name", deployment.Spec.Template.Spec.Containers[0].Env[4].ValueFrom.SecretKeyRef.Name) - assert.Equal(t, "ENV_VAR_NAME_2", deployment.Spec.Template.Spec.Containers[0].Env[4].ValueFrom.SecretKeyRef.Key) - assert.True(t, *deployment.Spec.Template.Spec.Containers[0].Env[4].ValueFrom.SecretKeyRef.Optional) - assert.Equal(t, "ENV_VAR_NAME_3", deployment.Spec.Template.Spec.Containers[0].Env[5].Name) - assert.Empty(t, deployment.Spec.Template.Spec.Containers[0].Env[5].Value) - assert.Equal(t, "ENV_VAR_NAME_4", deployment.Spec.Template.Spec.Containers[0].Env[6].Name) - assert.Empty(t, deployment.Spec.Template.Spec.Containers[0].Env[6].ValueFrom) + assert.Len(t, deployment.Spec.Template.Spec.Containers[0].Env, 6) + assert.Equal(t, "ENV_VAR_NAME_1", deployment.Spec.Template.Spec.Containers[0].Env[2].Name) + assert.Equal(t, "ENV_VAR_VALUE_1", deployment.Spec.Template.Spec.Containers[0].Env[2].Value) + assert.Equal(t, "ENV_VAR_NAME_2", deployment.Spec.Template.Spec.Containers[0].Env[3].Name) + assert.Equal(t, "secret-name", deployment.Spec.Template.Spec.Containers[0].Env[3].ValueFrom.SecretKeyRef.Name) + assert.Equal(t, "ENV_VAR_NAME_2", deployment.Spec.Template.Spec.Containers[0].Env[3].ValueFrom.SecretKeyRef.Key) + assert.True(t, *deployment.Spec.Template.Spec.Containers[0].Env[3].ValueFrom.SecretKeyRef.Optional) + assert.Equal(t, "ENV_VAR_NAME_3", deployment.Spec.Template.Spec.Containers[0].Env[4].Name) + assert.Empty(t, deployment.Spec.Template.Spec.Containers[0].Env[4].Value) + assert.Equal(t, "ENV_VAR_NAME_4", deployment.Spec.Template.Spec.Containers[0].Env[5].Name) + assert.Empty(t, deployment.Spec.Template.Spec.Containers[0].Env[5].ValueFrom) } func TestTemplate_WatchSingleNamespace_NotCreateManagerClusterRole(t *testing.T) { diff --git a/controllers/actions.github.com/constants.go b/controllers/actions.github.com/constants.go index 5e8445ce..6484f9e3 100644 --- a/controllers/actions.github.com/constants.go +++ b/controllers/actions.github.com/constants.go @@ -2,7 +2,6 @@ package actionsgithubcom import ( "github.com/actions/actions-runner-controller/logging" - corev1 "k8s.io/api/core/v1" ) const ( @@ -59,10 +58,6 @@ const ( AnnotationKeyNoPermissionServiceAccountName = "actions.github.com/cleanup-no-permission-service-account-name" ) -// DefaultScaleSetListenerImagePullPolicy is the default pull policy applied -// to the listener when ImagePullPolicy is not specified -const DefaultScaleSetListenerImagePullPolicy = corev1.PullIfNotPresent - // DefaultScaleSetListenerLogLevel is the default log level applied const DefaultScaleSetListenerLogLevel = string(logging.LogLevelDebug) diff --git a/controllers/actions.github.com/resourcebuilder.go b/controllers/actions.github.com/resourcebuilder.go index e42f3a60..305af7cb 100644 --- a/controllers/actions.github.com/resourcebuilder.go +++ b/controllers/actions.github.com/resourcebuilder.go @@ -41,18 +41,6 @@ const labelValueKubernetesPartOf = "gha-runner-scale-set" var scaleSetListenerLogLevel = DefaultScaleSetListenerLogLevel var scaleSetListenerLogFormat = DefaultScaleSetListenerLogFormat -var scaleSetListenerImagePullPolicy = DefaultScaleSetListenerImagePullPolicy - -func SetListenerImagePullPolicy(pullPolicy string) bool { - switch p := corev1.PullPolicy(pullPolicy); p { - case corev1.PullAlways, corev1.PullIfNotPresent, corev1.PullNever: - scaleSetListenerImagePullPolicy = p - return true - default: - return false - } -} - func SetListenerLoggingParameters(level string, format string) bool { switch level { case logging.LogLevelDebug, logging.LogLevelInfo, logging.LogLevelWarn, logging.LogLevelError: @@ -233,10 +221,9 @@ func (b *resourceBuilder) newScaleSetListenerPod(autoscalingListener *v1alpha1.A ServiceAccountName: serviceAccount.Name, Containers: []corev1.Container{ { - Name: autoscalingListenerContainerName, - Image: autoscalingListener.Spec.Image, - Env: listenerEnv, - ImagePullPolicy: scaleSetListenerImagePullPolicy, + Name: autoscalingListenerContainerName, + Image: autoscalingListener.Spec.Image, + Env: listenerEnv, Command: []string{ "/github-runnerscaleset-listener", }, @@ -380,10 +367,7 @@ func mergeListenerContainer(base, from *corev1.Container) { base.Env = append(base.Env, from.Env...) - if from.ImagePullPolicy != "" { - base.ImagePullPolicy = from.ImagePullPolicy - } - + base.ImagePullPolicy = from.ImagePullPolicy base.Args = append(base.Args, from.Args...) base.WorkingDir = from.WorkingDir base.Ports = append(base.Ports, from.Ports...) diff --git a/main.go b/main.go index a8755d99..b30fb460 100644 --- a/main.go +++ b/main.go @@ -185,13 +185,6 @@ func main() { } } - listenerPullPolicy := os.Getenv("CONTROLLER_MANAGER_LISTENER_IMAGE_PULL_POLICY") - if actionsgithubcom.SetListenerImagePullPolicy(listenerPullPolicy) { - log.Info("AutoscalingListener image pull policy changed", "ImagePullPolicy", listenerPullPolicy) - } else { - log.Info("Using default AutoscalingListener image pull policy", "ImagePullPolicy", actionsgithubcom.DefaultScaleSetListenerImagePullPolicy) - } - if actionsgithubcom.SetListenerLoggingParameters(logLevel, logFormat) { log.Info("AutoscalingListener logging parameters changed", "LogLevel", logLevel, "LogFormat", logFormat) } else {