fix: declare one k3s pod volume per storage entry

Mounting one storage entry at two container paths named the same pod volume twice, which Kubernetes rejects as a duplicate, so the chart never applied even though the k3s documentation offers mounting a claim "into the same app at multiple subpaths" as a supported shape. A pod now declares each entry once and binds it at each path it was mounted at. Rendering the chart in a test also surfaced that a cron schedule was emitted unquoted, so any schedule beginning with `*` produced a manifest that could not be parsed at all.

plugins/scheduler-k3s/chart.go

@@ -359,6 +359,15 @@ func BuildAppChart(ctx context.Context, appName, imageTag string, opts BuildOpti
 		return cleanup(err)
 	}
 
+	// Every cron job gets the same volumes: a cron ID is a hash of the app
+	// name, command and schedule, so it is not something an operator can name
+	// in --process-type and it churns whenever the task changes. Cron jobs
+	// therefore sit with `dokku run` and app.json tasks: default scope only.
+	cronVolumes, err := processVolumesFor(baseVolumes, deployMountPairs, storage.DefaultProcessType)
+	if err != nil {
+		return cleanup(fmt.Errorf("Error building volumes for cron tasks: %w", err))
+	}
+
 	for processType, processCount := range processes {
 		healthchecks, ok := appJSON.Healthchecks[processType]
 		if !ok {
@@ -403,6 +412,11 @@ func BuildAppChart(ctx context.Context, appName, imageTag string, opts BuildOpti
 			return cleanup(fmt.Errorf("Error getting autoscaling: %w", err))
 		}
 
+		processVolumes, err := processVolumesFor(baseVolumes, deployMountPairs, processType)
+		if err != nil {
+			return cleanup(fmt.Errorf("Error building volumes for process type %s: %w", processType, err))
+		}
+
 		processValues := ProcessValues{
 			Annotations:  annotations,
 			Autoscaling:  autoscaling,
@@ -413,7 +427,7 @@ func BuildAppChart(ctx context.Context, appName, imageTag string, opts BuildOpti
 			ProcessType:  ProcessType_Worker,
 			Replicas:     int32(processCount),
 			Resources:    processResources,
-			Volumes:      processVolumesFor(baseVolumes, deployMountPairs, processType),
+			Volumes:      processVolumes,
 		}
 
 		if processType == "web" {
@@ -591,11 +605,7 @@ func BuildAppChart(ctx context.Context, appName, imageTag string, opts BuildOpti
 			ProcessType: ProcessType_Cron,
 			Replicas:    1,
 			Resources:   processResources,
-			// A cron ID is a hash of the app name, command and schedule, so it
-			// is not something an operator can name in --process-type and it
-			// churns whenever the task changes. Cron jobs therefore sit with
-			// `dokku run` and app.json tasks: default-scoped mounts only.
-			Volumes: processVolumesFor(baseVolumes, deployMountPairs, storage.DefaultProcessType),
+			Volumes:     cronVolumes,
 		}
 		values.Processes[cronTask.ID] = processValues
 	}

plugins/scheduler-k3s/chart_render_test.go

@@ -0,0 +1,240 @@
+package scheduler_k3s
+
+import (
+	"strings"
+	"testing"
+
+	"github.com/dokku/dokku/plugins/storage"
+	"gopkg.in/yaml.v3"
+	"helm.sh/helm/v3/pkg/chart"
+	"helm.sh/helm/v3/pkg/chartutil"
+	"helm.sh/helm/v3/pkg/engine"
+)
+
+func storageEntryK3s(name string) *storage.Entry {
+	return &storage.Entry{Name: name, Scheduler: storage.SchedulerK3s}
+}
+
+func storageAttachment(containerPath string, subpath string) *storage.Attachment {
+	return &storage.Attachment{ContainerPath: containerPath, Subpath: subpath}
+}
+
+// renderProcessChart renders one of the chart templates against the given
+// process values, going through YAML the same way a real deploy does. That
+// round trip is the point: it exercises the struct's yaml tags against the
+// keys the template actually reads, so a rename on one side without the other
+// fails here instead of at helm upgrade time.
+func renderProcessChart(t *testing.T, templateName string, processName string, values ProcessValues) string {
+	t.Helper()
+
+	appValues := AppValues{
+		Global: GlobalValues{
+			AppName:      "demo",
+			DeploymentID: "1",
+			Namespace:    "default",
+			Image: GlobalImage{
+				Name: "dokku/demo:latest",
+				Type: "herokuish",
+			},
+		},
+		Processes: map[string]ProcessValues{processName: values},
+	}
+
+	encoded, err := yaml.Marshal(appValues)
+	if err != nil {
+		t.Fatalf("marshal values: %v", err)
+	}
+	rendered := map[string]interface{}{}
+	if err := yaml.Unmarshal(encoded, &rendered); err != nil {
+		t.Fatalf("unmarshal values: %v", err)
+	}
+
+	templateBody, err := templates.ReadFile("templates/chart/" + templateName)
+	if err != nil {
+		t.Fatalf("read template: %v", err)
+	}
+	helpers, err := templates.ReadFile("templates/chart/_helpers.tpl")
+	if err != nil {
+		t.Fatalf("read helpers: %v", err)
+	}
+
+	c := &chart.Chart{
+		Metadata: &chart.Metadata{APIVersion: chart.APIVersionV2, Name: "demo", Version: "0.0.1"},
+		Templates: []*chart.File{
+			{Name: "templates/" + templateName, Data: templateBody},
+			{Name: "templates/_helpers.tpl", Data: helpers},
+		},
+	}
+
+	top, err := chartutil.ToRenderValues(c, rendered, chartutil.ReleaseOptions{Name: "demo", Namespace: "default"}, nil)
+	if err != nil {
+		t.Fatalf("render values: %v", err)
+	}
+	out, err := engine.Render(c, top)
+	if err != nil {
+		t.Fatalf("render chart: %v", err)
+	}
+
+	return out["demo/templates/"+templateName]
+}
+
+// podSpecFrom decodes the rendered manifest and returns the pod spec at the
+// given path, so the assertions below read the same structure the API server
+// would rather than counting indentation.
+func podSpecFrom(t *testing.T, manifest string, path ...string) map[string]interface{} {
+	t.Helper()
+
+	decoder := yaml.NewDecoder(strings.NewReader(manifest))
+	doc := map[string]interface{}{}
+	for {
+		candidate := map[string]interface{}{}
+		if err := decoder.Decode(&candidate); err != nil {
+			break
+		}
+		if len(candidate) > 0 {
+			doc = candidate
+			break
+		}
+	}
+	if len(doc) == 0 {
+		t.Fatalf("no document decoded from manifest:\n%s", manifest)
+	}
+
+	current := doc
+	for _, key := range path {
+		next, ok := current[key].(map[string]interface{})
+		if !ok {
+			t.Fatalf("key %q missing while walking %v in manifest:\n%s", key, path, manifest)
+		}
+		current = next
+	}
+	return current
+}
+
+// volumeNamesFrom returns the pod-level volume names, and mountsFrom the
+// container-level mounts, of a decoded pod spec.
+func volumeNamesFrom(t *testing.T, podSpec map[string]interface{}) []string {
+	t.Helper()
+
+	names := []string{}
+	volumes, _ := podSpec["volumes"].([]interface{})
+	for _, entry := range volumes {
+		volume, ok := entry.(map[string]interface{})
+		if !ok {
+			t.Fatalf("unexpected volume entry %#v", entry)
+		}
+		names = append(names, volume["name"].(string))
+	}
+	return names
+}
+
+func mountsFrom(t *testing.T, podSpec map[string]interface{}) []map[string]interface{} {
+	t.Helper()
+
+	containers, _ := podSpec["containers"].([]interface{})
+	if len(containers) == 0 {
+		t.Fatalf("no containers in pod spec %#v", podSpec)
+	}
+	container, ok := containers[0].(map[string]interface{})
+	if !ok {
+		t.Fatalf("unexpected container entry %#v", containers[0])
+	}
+
+	mounts := []map[string]interface{}{}
+	raw, _ := container["volumeMounts"].([]interface{})
+	for _, entry := range raw {
+		mount, ok := entry.(map[string]interface{})
+		if !ok {
+			t.Fatalf("unexpected volumeMount entry %#v", entry)
+		}
+		mounts = append(mounts, mount)
+	}
+	return mounts
+}
+
+// TestDeploymentRendersOneVolumePerNameAndOneMountPerPath is the regression
+// for a storage entry mounted at two container paths. Kubernetes requires pod
+// volume names to be unique, so the pod must declare the PVC once and bind it
+// twice - emitting it twice is rejected with "Duplicate value".
+func TestDeploymentRendersOneVolumePerNameAndOneMountPerPath(t *testing.T) {
+	pairs := []AppMountPair{
+		{
+			Entry:      storageEntryK3s("demo-pvc"),
+			Attachment: storageAttachment("/one", ""),
+		},
+		{
+			Entry:      storageEntryK3s("demo-pvc"),
+			Attachment: storageAttachment("/two", "sub"),
+		},
+	}
+
+	manifest := renderProcessChart(t, "deployment.yaml", "web", ProcessValues{
+		ProcessType: ProcessType_Worker,
+		Replicas:    1,
+		Volumes:     ToProcessVolumes(pairs, "web"),
+	})
+
+	podSpec := podSpecFrom(t, manifest, "spec", "template", "spec")
+
+	names := volumeNamesFrom(t, podSpec)
+	if len(names) != 1 || names[0] != "demo-pvc" {
+		t.Fatalf("expected a single pod volume named demo-pvc, got %v\n%s", names, manifest)
+	}
+
+	mounts := mountsFrom(t, podSpec)
+	if len(mounts) != 2 {
+		t.Fatalf("expected 2 volume mounts, got %d\n%s", len(mounts), manifest)
+	}
+	if mounts[0]["mountPath"] != "/one" || mounts[1]["mountPath"] != "/two" {
+		t.Errorf("expected mounts at /one and /two, got %v and %v", mounts[0]["mountPath"], mounts[1]["mountPath"])
+	}
+	if mounts[1]["subPath"] != "sub" {
+		t.Errorf("expected subPath sub on the second mount, got %v", mounts[1]["subPath"])
+	}
+	if !strings.Contains(manifest, "claimName: demo-pvc") {
+		t.Errorf("expected the PVC to be referenced, got:\n%s", manifest)
+	}
+}
+
+// TestCronJobRendersOneVolumePerName covers the same rule in the cron-job
+// template, which renders its volumes from the same two lists.
+func TestCronJobRendersOneVolumePerName(t *testing.T) {
+	pairs := []AppMountPair{
+		{
+			Entry:      storageEntryK3s("demo-pvc"),
+			Attachment: storageAttachment("/one", ""),
+		},
+		{
+			Entry:      storageEntryK3s("demo-pvc"),
+			Attachment: storageAttachment("/two", ""),
+		},
+	}
+
+	manifest := renderProcessChart(t, "cron-job.yaml", "cron-1", ProcessValues{
+		ProcessType: ProcessType_Cron,
+		Replicas:    1,
+		Cron: ProcessCron{
+			ID:       "abc123",
+			Hash:     "abc123",
+			Schedule: "*/5 * * * *",
+			Suffix:   "aaaaa",
+		},
+		Volumes: ToProcessVolumes(pairs, ""),
+	})
+
+	podSpec := podSpecFrom(t, manifest, "spec", "jobTemplate", "spec", "template", "spec")
+
+	if names := volumeNamesFrom(t, podSpec); len(names) != 1 || names[0] != "demo-pvc" {
+		t.Fatalf("expected a single pod volume named demo-pvc, got %v\n%s", names, manifest)
+	}
+	if mounts := mountsFrom(t, podSpec); len(mounts) != 2 {
+		t.Fatalf("expected 2 volume mounts, got %d\n%s", len(mounts), manifest)
+	}
+
+	// A cron expression starting with `*` is not a valid plain YAML scalar, so
+	// the schedule has to render quoted or the whole manifest fails to parse.
+	spec := podSpecFrom(t, manifest, "spec")
+	if spec["schedule"] != "*/5 * * * *" {
+		t.Errorf("expected the schedule to survive rendering, got %v", spec["schedule"])
+	}
+}

plugins/scheduler-k3s/storage.go

@@ -293,8 +293,9 @@ func ValidateMountPairs(pairs []AppMountPair) error {
 }
 
 // ToProcessVolumes converts the AppMountPairs that apply to processType into
-// ProcessVolumes. K3s app deployments reference the PVC by name; the PVC
-// itself is owned by the storage entry's separate helm release.
+// ProcessVolumes, one per container mount. K3s app deployments reference the
+// PVC by name; the PVC itself is owned by the storage entry's separate helm
+// release.
 //
 // An empty processType names a workload that belongs to no process type - a
 // cron job - and so receives default-scoped attachments only.
@@ -320,20 +321,54 @@ func ToProcessVolumes(pairs []AppMountPair, processType string) []ProcessVolume
 	return volumes
 }
 
-// processVolumesFor returns the volumes one process type's pods receive: the
-// volumes every workload in the app gets, plus the storage attachments scoped
-// to that process type. The slice is freshly allocated rather than appended
-// onto base, so one workload's volumes can never alias another's.
-func processVolumesFor(base []ProcessVolume, pairs []AppMountPair, processType string) []ProcessVolume {
+// processVolumesFor returns the volumes one process type's pods receive: what
+// every workload in the app gets, plus the storage attachments scoped to that
+// process type. The slice is freshly allocated rather than appended onto base,
+// so one workload's volumes can never alias another's.
+//
+// Entries repeat a Name once per container path, which is what the chart wants:
+// it renders a volumeMounts entry per element and a pod volume per distinct
+// Name. Two different sources claiming one Name cannot be rendered that way, so
+// they are rejected rather than silently collapsed to whichever came first.
+func processVolumesFor(base []ProcessVolume, pairs []AppMountPair, processType string) ([]ProcessVolume, error) {
 	scoped := ToProcessVolumes(pairs, processType)
+
 	volumes := make([]ProcessVolume, 0, len(base)+len(scoped))
 	volumes = append(volumes, base...)
 	volumes = append(volumes, scoped...)
-	return volumes
+
+	sources := map[string]ProcessVolume{}
+	for _, volume := range volumes {
+		existing, seen := sources[volume.Name]
+		if seen && !sameVolumeSource(existing, volume) {
+			return nil, fmt.Errorf("volume name %q is claimed by two different sources in one pod", volume.Name)
+		}
+		sources[volume.Name] = volume
+	}
+
+	return volumes, nil
 }
 
-// asK8sVolumeMount is a small helper used by tests / other callers that
-// want a corev1.VolumeMount from a ProcessVolume.
+// sameVolumeSource reports whether two same-named volumes resolve to the same
+// backing source and can therefore share one pod volume.
+func sameVolumeSource(first ProcessVolume, second ProcessVolume) bool {
+	if (first.PersistentClaim == nil) != (second.PersistentClaim == nil) {
+		return false
+	}
+	if first.PersistentClaim != nil && *first.PersistentClaim != *second.PersistentClaim {
+		return false
+	}
+	if (first.EmptyDir == nil) != (second.EmptyDir == nil) {
+		return false
+	}
+	if first.EmptyDir != nil && *first.EmptyDir != *second.EmptyDir {
+		return false
+	}
+	return true
+}
+
+// asK8sVolumeMount converts a ProcessVolume into the corev1 mount shape for
+// callers that work against the Kubernetes API types directly.
 func asK8sVolumeMount(v ProcessVolume) corev1.VolumeMount {
 	return corev1.VolumeMount{
 		Name:      v.Name,

plugins/scheduler-k3s/storage_test.go

@@ -111,6 +111,63 @@ func TestToProcessVolumesAcceptsK3sEntry(t *testing.T) {
 	}
 }
 
+// TestToProcessVolumesCollapsesOneEntryMountedTwice covers the pod-volume
+// uniqueness rule: mounting one storage entry at two container paths has to
+// produce two volumeMounts and a single volume, or the API server rejects the
+// chart with a duplicate volume name.
+func TestToProcessVolumesCollapsesOneEntryMountedTwice(t *testing.T) {
+	pairs := []AppMountPair{
+		{
+			Entry:      &storage.Entry{Name: "demo-pvc", Scheduler: storage.SchedulerK3s},
+			Attachment: &storage.Attachment{ContainerPath: "/one", Subpath: "one"},
+		},
+		{
+			Entry:      &storage.Entry{Name: "demo-pvc", Scheduler: storage.SchedulerK3s},
+			Attachment: &storage.Attachment{ContainerPath: "/two", Subpath: "two", Readonly: true},
+		},
+	}
+
+	volumes := ToProcessVolumes(pairs, "web")
+	if len(volumes) != 2 {
+		t.Fatalf("expected 2 entries, got %d", len(volumes))
+	}
+	if volumes[0].Name != volumes[1].Name {
+		t.Fatalf("expected both entries to name one volume, got %q and %q", volumes[0].Name, volumes[1].Name)
+	}
+	if volumes[0].MountPath != "/one" || volumes[1].MountPath != "/two" {
+		t.Fatalf("expected mounts at /one and /two, got %q and %q", volumes[0].MountPath, volumes[1].MountPath)
+	}
+	if !volumes[1].ReadOnly || volumes[0].ReadOnly {
+		t.Errorf("expected only the second mount to be read only")
+	}
+}
+
+// TestProcessVolumesForRejectsSameNameDifferentSource guards the name collapse
+// the chart performs: the pod volume is emitted from the first entry with a
+// given name, so two different sources sharing one name would silently mount
+// whichever came first. An app with a storage entry called "shmem" alongside a
+// configured shm size is the way to reach it.
+func TestProcessVolumesForRejectsSameNameDifferentSource(t *testing.T) {
+	base := []ProcessVolume{
+		{
+			Name:      "shmem",
+			MountPath: "/dev/shm",
+			EmptyDir:  &ProcessVolumeEmptyDir{Medium: "Memory", SizeLimit: "64Mi"},
+		},
+	}
+
+	pairs := []AppMountPair{
+		{
+			Entry:      &storage.Entry{Name: "shmem", Scheduler: storage.SchedulerK3s},
+			Attachment: &storage.Attachment{ContainerPath: "/data"},
+		},
+	}
+
+	if _, err := processVolumesFor(base, pairs, "web"); err == nil {
+		t.Fatal("expected an error for two sources claiming one volume name, got nil")
+	}
+}
+
 func TestToProcessVolumesSkipsNilPairs(t *testing.T) {
 	pairs := []AppMountPair{
 		{Entry: nil, Attachment: nil},
@@ -150,9 +207,8 @@ func TestToProcessVolumesScopesToProcessType(t *testing.T) {
 		{processType: "worker", expected: []string{"shared", "legacy"}},
 		{processType: "", expected: []string{"shared", "legacy"}},
 	} {
-		volumes := ToProcessVolumes(pairs, tc.processType)
 		names := []string{}
-		for _, volume := range volumes {
+		for _, volume := range ToProcessVolumes(pairs, tc.processType) {
 			names = append(names, volume.Name)
 		}
 		if len(names) != len(tc.expected) {
@@ -171,7 +227,7 @@ func TestToProcessVolumesScopesToProcessType(t *testing.T) {
 // storage mounts into another's pod spec.
 func TestProcessVolumesForDoesNotAliasBase(t *testing.T) {
 	base := make([]ProcessVolume, 0, 8)
-	base = append(base, ProcessVolume{Name: "shmem", MountPath: "/dev/shm"})
+	base = append(base, ProcessVolume{Name: "shmem", MountPath: "/dev/shm", EmptyDir: &ProcessVolumeEmptyDir{Medium: "Memory"}})
 
 	pairs := []AppMountPair{
 		{
@@ -180,8 +236,14 @@ func TestProcessVolumesForDoesNotAliasBase(t *testing.T) {
 		},
 	}
 
-	web := processVolumesFor(base, pairs, "web")
-	worker := processVolumesFor(base, pairs, "worker")
+	web, err := processVolumesFor(base, pairs, "web")
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	worker, err := processVolumesFor(base, pairs, "worker")
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
 
 	if len(web) != 2 {
 		t.Fatalf("expected web to get 2 volumes, got %d", len(web))

plugins/scheduler-k3s/template.go

@@ -115,9 +115,18 @@ type ProcessValues struct {
 	Replicas     int32               `yaml:"replicas"`
 	Resources    ProcessResourcesMap `yaml:"resources,omitempty"`
 	Web          ProcessWeb          `yaml:"web,omitempty"`
-	Volumes      []ProcessVolume     `yaml:"volumes,omitempty"`
+
+	// Volumes holds one entry per container mount. The chart renders a
+	// volumeMounts entry for each of them and a pod volume for each distinct
+	// Name, so mounting one storage entry at two container paths belongs here
+	// twice: Kubernetes allows repeated volumeMounts names but requires pod
+	// volume names to be unique.
+	Volumes []ProcessVolume `yaml:"volumes,omitempty"`
 }
 
+// ProcessVolume is one container mount together with the source backing it.
+// Entries sharing a Name must agree on their source - they describe one pod
+// volume bound at several paths.
 type ProcessVolume struct {
 	Name            string                        `yaml:"name"`
 	MountPath       string                        `yaml:"mount_path"`

plugins/scheduler-k3s/templates/chart/cron-job.yaml

@@ -163,7 +163,10 @@ spec:
           serviceAccountName: {{ $.Values.global.app_name }}
           {{- if $config.volumes }}
           volumes:
+            {{- $seen := dict }}
             {{- range $volume := $config.volumes }}
+            {{- if not (hasKey $seen $volume.name) }}
+            {{- $_ := set $seen $volume.name true }}
             - name: {{ $volume.name }}
               {{- if $volume.empty_dir }}
               emptyDir:
@@ -179,8 +182,9 @@ spec:
                 claimName: {{ $volume.persistent_claim.claim_name }}
               {{- end }}
             {{- end }}
+            {{- end }}
           {{- end }}
-  schedule: {{ $config.cron.schedule }}
+  schedule: {{ $config.cron.schedule | quote }}
   startingDeadlineSeconds: 60
   successfulJobsHistoryLimit: 10
   suspend: {{ $config.cron.suspend }}

plugins/scheduler-k3s/templates/chart/deployment.yaml

@@ -179,7 +179,10 @@ spec:
       serviceAccountName: {{ $.Values.global.app_name }}
       {{- if $config.volumes }}
       volumes:
+        {{- $seen := dict }}
         {{- range $volume := $config.volumes }}
+        {{- if not (hasKey $seen $volume.name) }}
+        {{- $_ := set $seen $volume.name true }}
         - name: {{ $volume.name }}
           {{- if $volume.empty_dir }}
           emptyDir:
@@ -195,5 +198,6 @@ spec:
             claimName: {{ $volume.persistent_claim.claim_name }}
           {{- end }}
         {{- end }}
+        {{- end }}
       {{- end }}
 {{- end }}