From 63be0223addc4c4ee414732853aae076450f206f Mon Sep 17 00:00:00 2001 From: Yusuke Kuoka Date: Sun, 22 May 2022 10:25:50 +0900 Subject: [PATCH] fix: Avoid duplicate volume and mount name error for generic ephemeral volume as "work" (#1471) * fix: Avoid duplicate volume and mount name error for generic ephemeral volume as "work" While manually testing configurations being documented in #1464, I discovered that the use of dynamic ephemeral volume for "work" directory was not working correctly due to the valiadation error. This fixes the runner pod generation logic to not add the default volume and volume mount for "work" dir, so that the error disappears. Ref #1464 * e2e: Ensure work generic ephemeral volume to work as expected --- acceptance/testdata/runnerset.envsubst.yaml | 28 +++++ controllers/new_runner_pod_test.go | 111 ++++++++++++++++++++ controllers/runner_controller.go | 30 ++++-- test/e2e/e2e_test.go | 8 +- 4 files changed, 163 insertions(+), 14 deletions(-) diff --git a/acceptance/testdata/runnerset.envsubst.yaml b/acceptance/testdata/runnerset.envsubst.yaml index 220cf4e3..8a1edcdd 100644 --- a/acceptance/testdata/runnerset.envsubst.yaml +++ b/acceptance/testdata/runnerset.envsubst.yaml @@ -1,3 +1,14 @@ +--- +apiVersion: storage.k8s.io/v1 +kind: StorageClass +metadata: + name: ${NAME}-runner-work-dir + labels: + content: ${NAME}-runner-work-dir +provisioner: rancher.io/local-path +reclaimPolicy: Delete +volumeBindingMode: WaitForFirstConsumer +--- apiVersion: storage.k8s.io/v1 kind: StorageClass metadata: @@ -109,7 +120,10 @@ spec: value: "${RUNNER_FEATURE_FLAG_EPHEMERAL}" - name: GOMODCACHE value: "/home/runner/.cache/go-mod" + # PV-backed runner work dir volumeMounts: + - name: work + mountPath: /runner/_work # Cache docker image layers, in case dockerdWithinRunnerContainer=true - name: var-lib-docker mountPath: /var/lib/docker @@ -137,7 +151,10 @@ spec: mountPath: "/opt/hostedtoolcache" # Valid only when dockerdWithinRunnerContainer=false - name: docker + # PV-backed runner work dir volumeMounts: + - name: work + mountPath: /runner/_work # Cache docker image layers, in case dockerdWithinRunnerContainer=false - name: var-lib-docker mountPath: /var/lib/docker @@ -146,6 +163,17 @@ spec: # For buildx cache - name: cache mountPath: "/home/runner/.cache" + volumes: + - name: work + ephemeral: + volumeClaimTemplate: + spec: + accessModes: + - ReadWriteOnce + storageClassName: "${NAME}-runner-work-dir" + resources: + requests: + storage: 10Gi volumeClaimTemplates: - metadata: name: vol1 diff --git a/controllers/new_runner_pod_test.go b/controllers/new_runner_pod_test.go index e98f11a5..a4436f36 100644 --- a/controllers/new_runner_pod_test.go +++ b/controllers/new_runner_pod_test.go @@ -7,6 +7,7 @@ import ( "github.com/actions-runner-controller/actions-runner-controller/github" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" clientgoscheme "k8s.io/client-go/kubernetes/scheme" @@ -21,6 +22,11 @@ func TestNewRunnerPod(t *testing.T) { want corev1.Pod } + tenGB, err := resource.ParseQuantity("10Gi") + if err != nil { + t.Fatalf("%v", err) + } + base := corev1.Pod{ ObjectMeta: metav1.ObjectMeta{ Labels: map[string]string{ @@ -319,6 +325,27 @@ func TestNewRunnerPod(t *testing.T) { }, } + workGenericEphemeralVolume := corev1.Volume{ + Name: "work", + VolumeSource: corev1.VolumeSource{ + Ephemeral: &corev1.EphemeralVolumeSource{ + VolumeClaimTemplate: &corev1.PersistentVolumeClaimTemplate{ + Spec: corev1.PersistentVolumeClaimSpec{ + AccessModes: []corev1.PersistentVolumeAccessMode{ + corev1.ReadWriteOnce, + }, + StorageClassName: strPtr("runner-work-dir"), + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceStorage: tenGB, + }, + }, + }, + }, + }, + }, + } + newTestPod := func(base corev1.Pod, f func(*corev1.Pod)) corev1.Pod { pod := base.DeepCopy() if f != nil { @@ -391,6 +418,86 @@ func TestNewRunnerPod(t *testing.T) { p.Spec.Containers[0].SecurityContext.Privileged = boolPtr(true) }), }, + { + description: "Mount generic ephemeral volume onto work (with explicit volumeMount)", + template: corev1.Pod{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "runner", + VolumeMounts: []corev1.VolumeMount{ + { + Name: "work", + MountPath: "/runner/_work", + }, + }, + }, + }, + Volumes: []corev1.Volume{ + workGenericEphemeralVolume, + }, + }, + }, + want: newTestPod(base, func(p *corev1.Pod) { + p.Spec.Volumes = []corev1.Volume{ + workGenericEphemeralVolume, + { + Name: "runner", + VolumeSource: corev1.VolumeSource{ + EmptyDir: &corev1.EmptyDirVolumeSource{}, + }, + }, + { + Name: "certs-client", + VolumeSource: corev1.VolumeSource{ + EmptyDir: &corev1.EmptyDirVolumeSource{}, + }, + }, + } + p.Spec.Containers[0].VolumeMounts = []corev1.VolumeMount{ + { + Name: "work", + MountPath: "/runner/_work", + }, + { + Name: "runner", + MountPath: "/runner", + }, + { + Name: "certs-client", + MountPath: "/certs/client", + ReadOnly: true, + }, + } + }), + }, + { + description: "Mount generic ephemeral volume onto work (without explicit volumeMount)", + template: corev1.Pod{ + Spec: corev1.PodSpec{ + Volumes: []corev1.Volume{ + workGenericEphemeralVolume, + }, + }, + }, + want: newTestPod(base, func(p *corev1.Pod) { + p.Spec.Volumes = []corev1.Volume{ + workGenericEphemeralVolume, + { + Name: "runner", + VolumeSource: corev1.VolumeSource{ + EmptyDir: &corev1.EmptyDirVolumeSource{}, + }, + }, + { + Name: "certs-client", + VolumeSource: corev1.VolumeSource{ + EmptyDir: &corev1.EmptyDirVolumeSource{}, + }, + }, + } + }), + }, } var ( @@ -411,6 +518,10 @@ func TestNewRunnerPod(t *testing.T) { } } +func strPtr(s string) *string { + return &s +} + func TestNewRunnerPodFromRunnerController(t *testing.T) { type testcase struct { description string diff --git a/controllers/runner_controller.go b/controllers/runner_controller.go index 440811a6..cfb0025c 100644 --- a/controllers/runner_controller.go +++ b/controllers/runner_controller.go @@ -741,13 +741,18 @@ func newRunnerPod(runnerName string, template corev1.Pod, runnerSpec v1alpha1.Ru ) } - pod.Spec.Volumes = append(pod.Spec.Volumes, - corev1.Volume{ - Name: "work", - VolumeSource: corev1.VolumeSource{ - EmptyDir: &corev1.EmptyDirVolumeSource{}, + if ok, _ := workVolumePresent(pod.Spec.Volumes); !ok { + pod.Spec.Volumes = append(pod.Spec.Volumes, + corev1.Volume{ + Name: "work", + VolumeSource: corev1.VolumeSource{ + EmptyDir: &corev1.EmptyDirVolumeSource{}, + }, }, - }, + ) + } + + pod.Spec.Volumes = append(pod.Spec.Volumes, corev1.Volume{ Name: "certs-client", VolumeSource: corev1.VolumeSource{ @@ -756,11 +761,16 @@ func newRunnerPod(runnerName string, template corev1.Pod, runnerSpec v1alpha1.Ru }, ) + if ok, _ := workVolumeMountPresent(runnerContainer.VolumeMounts); !ok { + runnerContainer.VolumeMounts = append(runnerContainer.VolumeMounts, + corev1.VolumeMount{ + Name: "work", + MountPath: workDir, + }, + ) + } + runnerContainer.VolumeMounts = append(runnerContainer.VolumeMounts, - corev1.VolumeMount{ - Name: "work", - MountPath: workDir, - }, corev1.VolumeMount{ Name: "certs-client", MountPath: "/certs/client", diff --git a/test/e2e/e2e_test.go b/test/e2e/e2e_test.go index 19b896c9..f570c147 100644 --- a/test/e2e/e2e_test.go +++ b/test/e2e/e2e_test.go @@ -36,21 +36,21 @@ var ( EnableBuildX: true, }, { - Dockerfile: "../../runner/Dockerfile", + Dockerfile: "../../runner/actions-runner.dockerfile", Args: []testing.BuildArg{ { Name: "RUNNER_VERSION", - Value: "2.289.2", + Value: "2.291.1", }, }, Image: runnerImage, }, { - Dockerfile: "../../runner/Dockerfile.dindrunner", + Dockerfile: "../../runner/actions-runner-dind.dockerfile", Args: []testing.BuildArg{ { Name: "RUNNER_VERSION", - Value: "2.289.2", + Value: "2.291.1", }, }, Image: runnerDindImage,