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

@@ -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

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

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",

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,