OCPBUGS-64626: fix the files in pvc getting overwritten on each MG run

controllers/mustgather/template.go

@@ -3,7 +3,9 @@ package mustgather
 import (
 	"fmt"
 	"math"
+	"path"
 	"strconv"
+	"strings"
 
 	"time"
 
@@ -47,8 +49,41 @@ const (
 	// SSH directory and known hosts file
 	sshDir         = "/tmp/must-gather-operator/.ssh"
 	knownHostsFile = "/tmp/must-gather-operator/.ssh/known_hosts"
+
+	// Environment variable specifying the must-gather image
+	defaultMustGatherImageEnv = "DEFAULT_MUST_GATHER_IMAGE"
+
+	// Downward API environment variables used for subPathExpr expansion.
+	podNameEnvVar = "POD_NAME"
 )
 
+func outputSubPathExpr(storage *v1alpha1.Storage) (string, bool) {
+	if storage == nil || storage.Type != v1alpha1.StorageTypePersistentVolume {
+		return "", false
+	}
+
+	base := strings.TrimSpace(storage.PersistentVolume.SubPath)
+	base = strings.Trim(base, "/")
+	if base == "" {
+		return "", false
+	}
+
+	// Use user-provided base path, but isolate each run using the pod name
+	// to avoid overwriting prior collections on the PVC.
+	return path.Join(base, fmt.Sprintf("$(%s)", podNameEnvVar)), true
+}
+
+func podNameEnvVars() []corev1.EnvVar {
+	return []corev1.EnvVar{
+		{
+			Name: podNameEnvVar,
+			ValueFrom: &corev1.EnvVarSource{
+				FieldRef: &corev1.ObjectFieldSelector{FieldPath: "metadata.name"},
+			},
+		},
+	}
+}
+
 func getJobTemplate(image string, operatorImage string, mustGather v1alpha1.MustGather, trustedCAConfigMapName string) *batchv1.Job {
 	job := initializeJobTemplate(mustGather.Name, mustGather.Namespace, mustGather.Spec.ServiceAccountName, mustGather.Spec.Storage, trustedCAConfigMapName)
 
@@ -100,6 +135,7 @@ func getJobTemplate(image string, operatorImage string, mustGather v1alpha1.Must
 					s.CaseID,
 					s.Host,
 					s.InternalUser,
+					mustGather.Spec.Storage,
 					httpProxy,
 					httpsProxy,
 					noProxy,
@@ -205,8 +241,9 @@ func getGatherContainer(image string, audit bool, timeout time.Duration, storage
 		Name:      outputVolumeName,
 	}
 
-	if storage != nil && storage.Type == v1alpha1.StorageTypePersistentVolume && storage.PersistentVolume.SubPath != "" {
-		volumeMount.SubPath = storage.PersistentVolume.SubPath
+	subPathExpr, hasSubPathExpr := outputSubPathExpr(storage)
+	if hasSubPathExpr {
+		volumeMount.SubPathExpr = subPathExpr
 	}
 
 	volumeMounts := []corev1.VolumeMount{volumeMount}
@@ -240,6 +277,11 @@ func getGatherContainer(image string, audit bool, timeout time.Duration, storage
 		container.Args = args
 	}
 
+	// Provide pod name env var only when SubPathExpr is used (PVC subPath is set).
+	if hasSubPathExpr {
+		container.Env = append(container.Env, podNameEnvVars()...)
+	}
+
 	return container
 }
 
@@ -248,6 +290,7 @@ func getUploadContainer(
 	caseId string,
 	host string,
 	internalUser bool,
+	storage *v1alpha1.Storage,
 	httpProxy string,
 	httpsProxy string,
 	noProxy string,
@@ -258,11 +301,17 @@ func getUploadContainer(
 	uploadCommandWithSSH := fmt.Sprintf("mkdir -p %s; touch %s; chmod 700 %s; chmod 600 %s; %s",
 		sshDir, knownHostsFile, sshDir, knownHostsFile, uploadCommand)
 
+	outputMount := corev1.VolumeMount{
+		MountPath: volumeMountPath,
+		Name:      outputVolumeName,
+	}
+	subPathExpr, hasSubPathExpr := outputSubPathExpr(storage)
+	if hasSubPathExpr {
+		outputMount.SubPathExpr = subPathExpr
+	}
+
 	volumeMounts := []corev1.VolumeMount{
-		{
-			MountPath: volumeMountPath,
-			Name:      outputVolumeName,
-		},
+		outputMount,
 		{
 			MountPath: volumeUploadMountPath,
 			Name:      uploadVolumeName,
@@ -328,6 +377,11 @@ func getUploadContainer(
 		},
 	}
 
+	// Provide pod name env var only when SubPathExpr is used (PVC subPath is set).
+	if hasSubPathExpr {
+		container.Env = append(container.Env, podNameEnvVars()...)
+	}
+
 	if httpProxy != "" {
 		container.Env = append(container.Env, corev1.EnvVar{Name: uploadEnvHttpProxy, Value: httpProxy})
 	}

controllers/mustgather/template_test.go

@@ -138,6 +138,39 @@ func Test_getGatherContainer(t *testing.T) {
 				},
 			},
 		},
+		{
+			name:    "with PVC empty subPath does not set subPathExpr",
+			timeout: 5 * time.Second,
+			storage: &mustgatherv1alpha1.Storage{
+				Type: mustgatherv1alpha1.StorageTypePersistentVolume,
+				PersistentVolume: mustgatherv1alpha1.PersistentVolumeConfig{
+					Claim:   mustgatherv1alpha1.PersistentVolumeClaimReference{Name: "test-pvc"},
+					SubPath: "",
+				},
+			},
+		},
+		{
+			name:    "with PVC whitespace subPath does not set subPathExpr",
+			timeout: 5 * time.Second,
+			storage: &mustgatherv1alpha1.Storage{
+				Type: mustgatherv1alpha1.StorageTypePersistentVolume,
+				PersistentVolume: mustgatherv1alpha1.PersistentVolumeConfig{
+					Claim:   mustgatherv1alpha1.PersistentVolumeClaimReference{Name: "test-pvc"},
+					SubPath: "   ",
+				},
+			},
+		},
+		{
+			name:    "with PVC slash-only subPath does not set subPathExpr",
+			timeout: 5 * time.Second,
+			storage: &mustgatherv1alpha1.Storage{
+				Type: mustgatherv1alpha1.StorageTypePersistentVolume,
+				PersistentVolume: mustgatherv1alpha1.PersistentVolumeConfig{
+					Claim:   mustgatherv1alpha1.PersistentVolumeClaimReference{Name: "test-pvc"},
+					SubPath: "/",
+				},
+			},
+		},
 		{
 			name:            "robust timeout",
 			timeout:         6*time.Hour + 5*time.Minute + 3*time.Second, // 6h5m3s
@@ -188,8 +221,43 @@ func Test_getGatherContainer(t *testing.T) {
 				if volumeMount.Name != outputVolumeName {
 					t.Fatalf("volume mount name was not correctly set. got %v, wanted %v", volumeMount.Name, outputVolumeName)
 				}
-				if volumeMount.SubPath != tt.storage.PersistentVolume.SubPath {
-					t.Fatalf("volume mount subpath was not correctly set. got %v, wanted %v", volumeMount.SubPath, tt.storage.PersistentVolume.SubPath)
+				base := strings.Trim(strings.TrimSpace(tt.storage.PersistentVolume.SubPath), "/")
+				if base == "" {
+					if volumeMount.SubPathExpr != "" {
+						t.Fatalf("did not expect volume mount subPathExpr to be set when base subPath is empty, got %q", volumeMount.SubPathExpr)
+					}
+				} else {
+					wantExpr := fmt.Sprintf("%s/$(POD_NAME)", base)
+					if volumeMount.SubPathExpr != wantExpr {
+						t.Fatalf("volume mount subPathExpr was not correctly set. got %q, wanted %q", volumeMount.SubPathExpr, wantExpr)
+					}
+				}
+				if volumeMount.SubPath != "" {
+					t.Fatalf("did not expect volume mount subPath to be set when using subPathExpr, got %q", volumeMount.SubPath)
+				}
+			}
+
+			// POD_NAME env var should be present only when SubPathExpr is used.
+			hasPodNameEnv := false
+			for _, env := range container.Env {
+				if env.Name == podNameEnvVar {
+					hasPodNameEnv = true
+					if env.ValueFrom == nil || env.ValueFrom.FieldRef == nil || env.ValueFrom.FieldRef.FieldPath != "metadata.name" {
+						t.Fatalf("expected %s env var to be sourced from metadata.name via fieldRef, got %#v", podNameEnvVar, env)
+					}
+				}
+			}
+			subPathBase := ""
+			if tt.storage != nil && tt.storage.Type == mustgatherv1alpha1.StorageTypePersistentVolume {
+				subPathBase = strings.Trim(strings.TrimSpace(tt.storage.PersistentVolume.SubPath), "/")
+			}
+			if subPathBase == "" {
+				if hasPodNameEnv {
+					t.Fatalf("did not expect %s env var when PVC subPath is empty", podNameEnvVar)
+				}
+			} else {
+				if !hasPodNameEnv {
+					t.Fatalf("expected %s env var when PVC subPath is set (base=%q)", podNameEnvVar, subPathBase)
 				}
 			}
 		})
@@ -203,6 +271,7 @@ func Test_getUploadContainer(t *testing.T) {
 		caseId           string
 		host             string
 		internalUser     bool
+		storage          *mustgatherv1alpha1.Storage
 		httpProxy        string
 		httpsProxy       string
 		noProxy          string
@@ -262,11 +331,73 @@ func Test_getUploadContainer(t *testing.T) {
 			secretKeyRefName: v1.LocalObjectReference{Name: "testSecretKeyRefName"},
 			mountCAConfigMap: true,
 		},
+		{
+			name:          "With PVC subPath",
+			operatorImage: "testImage",
+			caseId:        "1234",
+			secretKeyRefName: v1.LocalObjectReference{
+				Name: "testSecretKeyRefName",
+			},
+			storage: &mustgatherv1alpha1.Storage{
+				Type: mustgatherv1alpha1.StorageTypePersistentVolume,
+				PersistentVolume: mustgatherv1alpha1.PersistentVolumeConfig{
+					Claim: mustgatherv1alpha1.PersistentVolumeClaimReference{
+						Name: "test-pvc",
+					},
+					SubPath: "test-path",
+				},
+			},
+		},
+		{
+			name:          "With PVC empty subPath does not set subPathExpr",
+			operatorImage: "testImage",
+			caseId:        "1234",
+			secretKeyRefName: v1.LocalObjectReference{
+				Name: "testSecretKeyRefName",
+			},
+			storage: &mustgatherv1alpha1.Storage{
+				Type: mustgatherv1alpha1.StorageTypePersistentVolume,
+				PersistentVolume: mustgatherv1alpha1.PersistentVolumeConfig{
+					Claim:   mustgatherv1alpha1.PersistentVolumeClaimReference{Name: "test-pvc"},
+					SubPath: "",
+				},
+			},
+		},
+		{
+			name:          "With PVC whitespace subPath does not set subPathExpr",
+			operatorImage: "testImage",
+			caseId:        "1234",
+			secretKeyRefName: v1.LocalObjectReference{
+				Name: "testSecretKeyRefName",
+			},
+			storage: &mustgatherv1alpha1.Storage{
+				Type: mustgatherv1alpha1.StorageTypePersistentVolume,
+				PersistentVolume: mustgatherv1alpha1.PersistentVolumeConfig{
+					Claim:   mustgatherv1alpha1.PersistentVolumeClaimReference{Name: "test-pvc"},
+					SubPath: "   ",
+				},
+			},
+		},
+		{
+			name:          "With PVC slash-only subPath does not set subPathExpr",
+			operatorImage: "testImage",
+			caseId:        "1234",
+			secretKeyRefName: v1.LocalObjectReference{
+				Name: "testSecretKeyRefName",
+			},
+			storage: &mustgatherv1alpha1.Storage{
+				Type: mustgatherv1alpha1.StorageTypePersistentVolume,
+				PersistentVolume: mustgatherv1alpha1.PersistentVolumeConfig{
+					Claim:   mustgatherv1alpha1.PersistentVolumeClaimReference{Name: "test-pvc"},
+					SubPath: "/",
+				},
+			},
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			testFailed := false
-			container := getUploadContainer(tt.operatorImage, tt.caseId, tt.host, tt.internalUser, tt.httpProxy, tt.httpsProxy, tt.noProxy, tt.secretKeyRefName, tt.mountCAConfigMap)
+			container := getUploadContainer(tt.operatorImage, tt.caseId, tt.host, tt.internalUser, tt.storage, tt.httpProxy, tt.httpsProxy, tt.noProxy, tt.secretKeyRefName, tt.mountCAConfigMap)
 
 			if container.Image != tt.operatorImage {
 				t.Fatalf("expected container image %v but got %v", tt.operatorImage, container.Image)
@@ -285,6 +416,57 @@ func Test_getUploadContainer(t *testing.T) {
 				}
 			}
 
+			if tt.storage != nil && tt.storage.Type == mustgatherv1alpha1.StorageTypePersistentVolume {
+				var outputMount *v1.VolumeMount
+				for i := range container.VolumeMounts {
+					if container.VolumeMounts[i].Name == outputVolumeName {
+						outputMount = &container.VolumeMounts[i]
+						break
+					}
+				}
+				if outputMount == nil {
+					t.Fatalf("expected output volume mount %q to be present", outputVolumeName)
+				}
+				base := strings.Trim(strings.TrimSpace(tt.storage.PersistentVolume.SubPath), "/")
+				if base == "" {
+					if outputMount.SubPathExpr != "" {
+						t.Fatalf("did not expect output volume mount subPathExpr to be set when base subPath is empty, got %q", outputMount.SubPathExpr)
+					}
+				} else {
+					wantExpr := fmt.Sprintf("%s/$(POD_NAME)", base)
+					if outputMount.SubPathExpr != wantExpr {
+						t.Fatalf("expected output volume mount subPathExpr %q but got %q", wantExpr, outputMount.SubPathExpr)
+					}
+				}
+				if outputMount.SubPath != "" {
+					t.Fatalf("did not expect output volume mount subPath to be set when using subPathExpr, got %q", outputMount.SubPath)
+				}
+			}
+
+			// POD_NAME env var should be present only when SubPathExpr is used.
+			hasPodNameEnv := false
+			for _, env := range container.Env {
+				if env.Name == podNameEnvVar {
+					hasPodNameEnv = true
+					if env.ValueFrom == nil || env.ValueFrom.FieldRef == nil || env.ValueFrom.FieldRef.FieldPath != "metadata.name" {
+						t.Fatalf("expected %s env var to be sourced from metadata.name via fieldRef, got %#v", podNameEnvVar, env)
+					}
+				}
+			}
+			subPathBase := ""
+			if tt.storage != nil && tt.storage.Type == mustgatherv1alpha1.StorageTypePersistentVolume {
+				subPathBase = strings.Trim(strings.TrimSpace(tt.storage.PersistentVolume.SubPath), "/")
+			}
+			if subPathBase == "" {
+				if hasPodNameEnv {
+					t.Fatalf("did not expect %s env var when PVC subPath is empty", podNameEnvVar)
+				}
+			} else {
+				if !hasPodNameEnv {
+					t.Fatalf("expected %s env var when PVC subPath is set (base=%q)", podNameEnvVar, subPathBase)
+				}
+			}
+
 			for _, env := range container.Env {
 				switch env.Name {
 				case uploadEnvCaseId:

deploy/must-gather-pvc.yaml

@@ -0,0 +1,11 @@
+apiVersion: v1
+kind: PersistentVolumeClaim
+metadata:
+  name: must-gather-pvc
+  namespace: must-gather-operator
+spec:
+  accessModes:
+    - ReadWriteOnce
+  resources:
+    requests:
+      storage: 5Gi

examples/mustgather_with_pvc_subpath.yaml

@@ -0,0 +1,19 @@
+apiVersion: operator.openshift.io/v1alpha1
+kind: MustGather
+metadata:
+  name: example-mustgather-pvc-subpath
+spec:
+  serviceAccountName: must-gather-admin
+  storage:
+    type: PersistentVolume
+    persistentVolume:
+      claim:
+        name: must-gather-pvc
+      subPath: must-gather-data
+  uploadTarget:
+    type: SFTP
+    sftp:
+      caseID: '04230315'
+      caseManagementAccountSecretRef:
+        name: case-management-creds
+      internalUser: true

test/e2e/must_gather_operator_test.go

@@ -1473,7 +1473,8 @@ var _ = ginkgo.Describe("MustGather resource", ginkgo.Ordered, func() {
 				}
 			}
 			Expect(outputMount).NotTo(BeNil(), "Gather container should have output volume mount")
-			Expect(outputMount.SubPath).To(Equal(subPath), "Volume mount should have subPath configured")
+			Expect(outputMount.SubPathExpr).To(Equal(subPath+"/$(POD_NAME)"), "Volume mount should have subPathExpr configured")
+			Expect(outputMount.SubPath).To(BeEmpty(), "Volume mount subPath should be empty when using subPathExpr")
 		})
 
 		ginkgo.It("should create MustGather with PVC storage, configure Job correctly, and persist data", func() {