Skip to content

Remove inheritance of imagePullPolicy from manager to listeners #3009

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
fhammerl merged 4 commits into from
Nov 7, 2023
Merged

Remove inheritance of imagePullPolicy from manager to listeners #3009

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
24b620b
Remove inheritance of imagePullPolicy from manager to listeners
nikola-jokic Oct 20, 2023
b0c7402
Remove warning from NOTES
nikola-jokic Oct 31, 2023
533ff2b
Merge branch 'master' into nikola-jokic/default-pull-policy
nikola-jokic Oct 31, 2023
2d70bbd
Bump helm chart testing
nikola-jokic Nov 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/arc-publish-chart.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

@fhammerl fhammerl Nov 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated change; the bump is only required because a the dependency of this action (sigstore/cosign) has moved and cannot be downloaded with 2.3.1 anymore.


- name: Run chart-testing (list-changed)
id: list-changed
Expand Down
6 changes: 3 additions & 3 deletions .github/workflows/arc-validate-chart.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 }}
Expand Down Expand Up @@ -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
Expand Down
6 changes: 3 additions & 3 deletions .github/workflows/gha-validate-chart.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 }}
Expand Down Expand Up @@ -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
Expand Down
1 change: 0 additions & 1 deletion charts/gha-runner-scale-set-controller/templates/NOTES.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
2 changes: 0 additions & 2 deletions charts/gha-runner-scale-set-controller/templates/deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 }}
Expand Down
42 changes: 18 additions & 24 deletions charts/gha-runner-scale-set-controller/tests/template_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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) {
Expand Down
5 changes: 0 additions & 5 deletions controllers/actions.github.com/constants.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package actionsgithubcom

import (
"github.com/actions/actions-runner-controller/logging"
corev1 "k8s.io/api/core/v1"
)

const (
Expand Down Expand Up @@ -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)

Expand Down
24 changes: 4 additions & 20 deletions controllers/actions.github.com/resourcebuilder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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",
},
Expand Down Expand Up @@ -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...)
Expand Down
7 changes: 0 additions & 7 deletions main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down