diff --git a/plugins/scheduler-k3s/chart.go b/plugins/scheduler-k3s/chart.go index b254b57d3..b019cf0aa 100644 --- a/plugins/scheduler-k3s/chart.go +++ b/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 } diff --git a/plugins/scheduler-k3s/chart_render_test.go b/plugins/scheduler-k3s/chart_render_test.go new file mode 100644 index 000000000..b280bfb69 --- /dev/null +++ b/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"]) + } +} diff --git a/plugins/scheduler-k3s/storage.go b/plugins/scheduler-k3s/storage.go index 61f08f447..72d150d4c 100644 --- a/plugins/scheduler-k3s/storage.go +++ b/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, diff --git a/plugins/scheduler-k3s/storage_test.go b/plugins/scheduler-k3s/storage_test.go index a43d607eb..566163754 100644 --- a/plugins/scheduler-k3s/storage_test.go +++ b/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)) diff --git a/plugins/scheduler-k3s/template.go b/plugins/scheduler-k3s/template.go index c506bfb40..c98045fc6 100644 --- a/plugins/scheduler-k3s/template.go +++ b/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"` diff --git a/plugins/scheduler-k3s/templates/chart/cron-job.yaml b/plugins/scheduler-k3s/templates/chart/cron-job.yaml index 2076ca590..765a020f4 100644 --- a/plugins/scheduler-k3s/templates/chart/cron-job.yaml +++ b/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 }} diff --git a/plugins/scheduler-k3s/templates/chart/deployment.yaml b/plugins/scheduler-k3s/templates/chart/deployment.yaml index b8a25c3dc..45151e564 100644 --- a/plugins/scheduler-k3s/templates/chart/deployment.yaml +++ b/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 }}