validation: Allow mounting the same volume at multiple paths

cmd/ateapi/internal/controlapi/actor_template.go

@@ -421,20 +421,15 @@ func actorTemplateObjectRef(actor *ateapipb.Actor) *ateapipb.ObjectRef {
 	return &ateapipb.ObjectRef{Atespace: ref.GetAtespace(), Name: ref.GetName()}
 }
 
-// ValidateCustom_Container_VolumeMounts rejects two mounts at the same path
-// within one container. The list is keyed by volume name (one mount per
-// volume), so path uniqueness cannot come from the list-map key
+// ValidateCustom_Container_VolumeMounts rejects mounts that nest under one
+// another.
 func ValidateCustom_Container_VolumeMounts(_ context.Context, _ operation.Operation, fldPath *field.Path, value, _ []*ateapipb.VolumeMount) field.ErrorList {
 	var errs field.ErrorList
-	seen := make(map[string]bool, len(value))
 	for i, m := range value {
 		path := m.GetMountPath()
 		if path == "" {
 			continue // required is enforced by tags
 		}
-		if seen[path] {
-			errs = append(errs, field.Duplicate(fldPath.Index(i).Child("mount_path"), path))
-		}
 		// Nested mounts are unsupported (volumes cannot mount onto
 		// other volumes).
 		for j := 0; j < i; j++ {
@@ -447,7 +442,6 @@ func ValidateCustom_Container_VolumeMounts(_ context.Context, _ operation.Operat
 					fmt.Sprintf("must not nest under or over another mount (%q)", prior)))
 			}
 		}
-		seen[path] = true
 	}
 	return errs
 }

cmd/ateapi/internal/controlapi/actor_template_test.go

@@ -567,15 +567,13 @@ func TestValidateActorTemplate(t *testing.T) {
 		},
 		want: field.ErrorList{field.Duplicate(field.NewPath("containers").Index(0).Child("env").Index(1), nil)},
 	}, {
-		// One mount per volume for now; see the TODO on volume_mounts.
-		name: "same volume mounted twice is rejected",
+		name: "the same volume mounted at two paths is allowed",
 		mutate: func(tmpl *ateapipb.ActorTemplate) {
 			tmpl.Containers[0].VolumeMounts = []*ateapipb.VolumeMount{
 				{Name: "data", MountPath: "/var/data"},
 				{Name: "data", MountPath: "/mnt/data"},
 			}
 		},
-		want: field.ErrorList{field.Duplicate(field.NewPath("containers").Index(0).Child("volume_mounts").Index(1), nil)},
 	}, {
 		name: "two volumes at the same path are rejected",
 		mutate: func(tmpl *ateapipb.ActorTemplate) {
@@ -584,7 +582,7 @@ func TestValidateActorTemplate(t *testing.T) {
 				{Name: "other", MountPath: "/var/data"},
 			}
 		},
-		want: field.ErrorList{field.Duplicate(field.NewPath("containers").Index(0).Child("volume_mounts").Index(1).Child("mount_path"), nil)},
+		want: field.ErrorList{field.Duplicate(field.NewPath("containers").Index(0).Child("volume_mounts").Index(1), nil)},
 	}, {
 		name: "nested mount paths are rejected",
 		mutate: func(tmpl *ateapipb.ActorTemplate) {

internal/e2e/suites/imagevolume/imagevolume_test.go → internal/e2e/suites/combinedvolumes/combinedvolumes_test.go

@@ -12,8 +12,10 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-// Package imagevolume exercises image volumes against a live cluster.
-package imagevolume
+// Package combinedvolumes exercises the three volume sources -- image volumes,
+// durable dirs, and external volumes -- mounted together on a single actor,
+// along with the mount semantics they share, against a live cluster.
+package combinedvolumes
 
 import (
 	"archive/tar"
@@ -38,13 +40,33 @@ import (
 	"github.com/google/go-containerregistry/pkg/v1/mutate"
 	"github.com/google/go-containerregistry/pkg/v1/remote"
 	"github.com/google/go-containerregistry/pkg/v1/tarball"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
 const (
-	atespace = "imagevolume"
+	atespace = "combinedvolumes"
 
 	// mountPath must not collide with anything the probe's own image ships.
 	mountPath = "/mnt/ate-image-volume"
+	// mountPathAlias is a second mount of the same image volume: one volume
+	// may be mounted at multiple paths.
+	mountPathAlias = "/mnt/ate-image-volume-alias"
+
+	// The scratch durable-dir volume is mounted at both of these paths; a
+	// write through one must be readable through the other.
+	scratchPathA = "/mnt/ate-scratch-a"
+	scratchPathB = "/mnt/ate-scratch-b"
+
+	// The external (CSI-backed) volume is mounted at both of these paths.
+	// Unlike the durable dir it is provisioned per actor and detached on
+	// suspend, so it must come back attached once and visible at both.
+	extVolume   = "external"
+	extPathA    = "/mnt/ate-external-a"
+	extPathB    = "/mnt/ate-external-b"
+	extCapacity = "1Gi"
+
+	// probeWrittenContent is the fixed string the probe's /writefile writes.
+	probeWrittenContent = "written by probe"
 
 	payloadName    = "payload.txt"
 	payloadContent = "delivered by an image volume"
@@ -109,7 +131,7 @@ func buildFixtureImage(t *testing.T, repo string) string {
 	// A unique tag per run: some registries refuse to overwrite an existing
 	// tag. The returned reference is digest-pinned, so the tag itself is
 	// throwaway.
-	ref := fmt.Sprintf("%s/e2e-imagevolume-fixture:%d", strings.TrimSuffix(repo, "/"), time.Now().UnixNano())
+	ref := fmt.Sprintf("%s/e2e-combinedvolumes-fixture:%d", strings.TrimSuffix(repo, "/"), time.Now().UnixNano())
 	tag, err := name.ParseReference(ref, name.Insecure)
 	if err != nil {
 		t.Fatalf("parsing %q: %v", ref, err)
@@ -128,11 +150,26 @@ func buildFixtureImage(t *testing.T, repo string) string {
 	return fmt.Sprintf("%s@%s", tag.Context().Name(), digest)
 }
 
+// storageClassOrEmpty returns the configured StorageClass if the cluster has
+// one, and "" if it does not. The CSI driver is optional, so a missing class
+// drops the external volume from the template instead of failing every test
+// in the suite.
+func storageClassOrEmpty(ctx context.Context, t *testing.T, clients *e2e.Clients) string {
+	t.Helper()
+
+	if _, err := clients.K8s.StorageV1().StorageClasses().Get(ctx, e2e.StorageClass, metav1.GetOptions{}); err != nil {
+		t.Logf("StorageClass %q not found (%v); the external-volume case will be skipped", e2e.StorageClass, err)
+		return ""
+	}
+	return e2e.StorageClass
+}
+
 // createTemplate builds a probe ActorTemplate with the fixture attached as an
 // image volume, copying the resolved runtime from the shared probe template.
 // The template's name is suffixed per test run: it lives in the suite's
-// shared atespace, which outlives the per-test k8s namespace.
-func createTemplate(ctx context.Context, t *testing.T, clients *e2e.Clients, ns *e2e.Namespace, fixtureImage string) *ateapipb.ActorTemplate {
+// shared atespace, which outlives the per-test k8s namespace. A non-empty
+// storageClass adds the external volume.
+func createTemplate(ctx context.Context, t *testing.T, clients *e2e.Clients, ns *e2e.Namespace, fixtureImage, storageClass string) *ateapipb.ActorTemplate {
 	t.Helper()
 
 	env, err := e2e.CheckEnv("BUCKET_NAME")
@@ -141,13 +178,13 @@ func createTemplate(ctx context.Context, t *testing.T, clients *e2e.Clients, ns
 	}
 
 	// The probe supplies this suite's container image and resolved runtime.
-	probeAtespace, _ := e2e.DeployProbe(t, env["BUCKET_NAME"], "imagevolume")
+	probeAtespace, _ := e2e.DeployProbe(t, env["BUCKET_NAME"], "combinedvolumes")
 	src := e2e.SubstrateFixture{
 		Atespace:      probeAtespace,
 		Name:          probeName,
 		PoolNamespace: probeAtespace,
 		PoolName:      probeName,
-		DeployWith:    "the imagevolume suite's own DeployProbe",
+		DeployWith:    "the combinedvolumes suite's own DeployProbe",
 	}
 
 	return e2e.CreateSubstrateTemplateFrom(ctx, t, clients, ns.Name, src, e2e.SubstrateTemplateOptions{
@@ -157,16 +194,40 @@ func createTemplate(ctx context.Context, t *testing.T, clients *e2e.Clients, ns
 		PoolReplicas: 2,
 		// The pool is labeled uniquely to this namespace so the cluster-wide
 		// scheduler cannot hand its workers to another suite's actors.
-		Labels: map[string]string{"imagevolume": ns.Name},
+		Labels: map[string]string{"combinedvolumes": ns.Name},
 		SnapshotsConfig: &ateapipb.SnapshotsConfig{
 			StorageLocation: fmt.Sprintf("gs://%s/%s/", env["BUCKET_NAME"], ns.Name),
 		},
 		Modify: func(tmpl *ateapipb.ActorTemplate) {
+			// Every volume is mounted at two paths, covering the read-only
+			// (image), writable (durable-dir) and per-actor provisioned
+			// (external) multi-path cases.
+			tmpl.Containers[0].VolumeMounts = append(tmpl.Containers[0].VolumeMounts,
+				&ateapipb.VolumeMount{Name: "fixture", MountPath: mountPath},
+				&ateapipb.VolumeMount{Name: "fixture", MountPath: mountPathAlias},
+				&ateapipb.VolumeMount{Name: "scratch", MountPath: scratchPathA},
+				&ateapipb.VolumeMount{Name: "scratch", MountPath: scratchPathB})
+			tmpl.Volumes = append(tmpl.Volumes,
+				&ateapipb.Volume{
+					Name:  "fixture",
+					Image: &ateapipb.ImageVolumeSource{Reference: fixtureImage},
+				},
+				&ateapipb.Volume{
+					Name:       "scratch",
+					DurableDir: &ateapipb.DurableDirVolumeSource{},
+				})
+			if storageClass == "" {
+				return
+			}
 			tmpl.Containers[0].VolumeMounts = append(tmpl.Containers[0].VolumeMounts,
-				&ateapipb.VolumeMount{Name: "fixture", MountPath: mountPath})
+				&ateapipb.VolumeMount{Name: extVolume, MountPath: extPathA},
+				&ateapipb.VolumeMount{Name: extVolume, MountPath: extPathB})
 			tmpl.Volumes = append(tmpl.Volumes, &ateapipb.Volume{
-				Name:  "fixture",
-				Image: &ateapipb.ImageVolumeSource{Reference: fixtureImage},
+				Name: extVolume,
+				ExternalVolumeTemplate: &ateapipb.ExternalVolumeTemplate{
+					Capacity:         extCapacity,
+					StorageClassName: storageClass,
+				},
 			})
 		},
 	})
@@ -193,7 +254,60 @@ func probeJSON(ctx context.Context, t *testing.T, router *e2e.RouterClient, acto
 	return out
 }
 
-func TestImageVolume(t *testing.T) {
+// requireContent fails unless path holds want. Used to verify a mount
+// delivers what the volume behind it should.
+func requireContent(ctx context.Context, t *testing.T, router *e2e.RouterClient, actorRef resources.ActorRef, path, want string) {
+	t.Helper()
+
+	got := probeJSON(ctx, t, router, actorRef, "/readfile?path="+path)
+	if got["error"] != "" {
+		t.Fatalf("reading %s: %s", path, got["error"])
+	}
+	if got["content"] != want {
+		t.Errorf("content at %s = %q, want %q", path, got["content"], want)
+	}
+}
+
+// requireContentAtBoth fails unless both paths hold want, so both mounts must
+// be live and backed by the same volume.
+func requireContentAtBoth(ctx context.Context, t *testing.T, router *e2e.RouterClient, actorRef resources.ActorRef, pathA, pathB, want string) {
+	t.Helper()
+
+	requireContent(ctx, t, router, actorRef, pathA, want)
+	requireContent(ctx, t, router, actorRef, pathB, want)
+}
+
+// requireUnreadable fails if path can be read.
+func requireUnreadable(ctx context.Context, t *testing.T, router *e2e.RouterClient, actorRef resources.ActorRef, path string) {
+	t.Helper()
+
+	if got := probeJSON(ctx, t, router, actorRef, "/readfile?path="+path); got["error"] == "" {
+		t.Errorf("%s is readable (%q), want it hidden", path, got["content"])
+	}
+}
+
+// requireWriteRejected fails if path can be written.
+func requireWriteRejected(ctx context.Context, t *testing.T, router *e2e.RouterClient, actorRef resources.ActorRef, path string) {
+	t.Helper()
+
+	if got := probeJSON(ctx, t, router, actorRef, "/writefile?path="+path); got["error"] == "" {
+		t.Errorf("write to %s succeeded, want it rejected as read-only", path)
+	}
+}
+
+// requireSharedWrite writes through writePath and requires the content at both
+// paths: writePath confirms the write persisted, aliasPath that the two mounts
+// reach the same volume.
+func requireSharedWrite(ctx context.Context, t *testing.T, router *e2e.RouterClient, actorRef resources.ActorRef, writePath, aliasPath string) {
+	t.Helper()
+
+	if got := probeJSON(ctx, t, router, actorRef, "/writefile?path="+writePath); got["error"] != "" {
+		t.Fatalf("writing %s: %s", writePath, got["error"])
+	}
+	requireContentAtBoth(ctx, t, router, actorRef, writePath, aliasPath, probeWrittenContent)
+}
+
+func TestCombinedVolumes(t *testing.T) {
 	repo := os.Getenv("KO_DOCKER_REPO")
 	if repo == "" {
 		t.Skip("KO_DOCKER_REPO is unset; it names the registry both this host and the cluster can reach")
@@ -205,9 +319,10 @@ func TestImageVolume(t *testing.T) {
 
 	fixtureImage := buildFixtureImage(t, repo)
 	t.Logf("fixture image: %s", fixtureImage)
-	tmpl := createTemplate(ctx, t, clients, ns, fixtureImage)
+	storageClass := storageClassOrEmpty(ctx, t, clients)
+	tmpl := createTemplate(ctx, t, clients, ns, fixtureImage, storageClass)
 
-	actorRef := resources.ActorRef{Atespace: atespace, Name: "iv-" + ns.Name}
+	actorRef := resources.ActorRef{Atespace: atespace, Name: "cv-" + ns.Name}
 	if _, err := clients.SubstrateAPI.CreateActor(ctx, &ateapipb.CreateActorRequest{
 		Actor: &ateapipb.Actor{
 			Metadata:      &ateapipb.ResourceMetadata{Atespace: actorRef.Atespace, Name: actorRef.Name},
@@ -235,53 +350,55 @@ func TestImageVolume(t *testing.T) {
 	payloadPath := mountPath + "/" + payloadName
 
 	t.Run("DeliversImageContents", func(t *testing.T) {
-		got := probeJSON(ctx, t, router, actorRef, "/readfile?path="+payloadPath)
-		if got["error"] != "" {
-			t.Fatalf("reading %s: %s", payloadPath, got["error"])
-		}
-		if got["content"] != payloadContent {
-			t.Errorf("content = %q, want %q", got["content"], payloadContent)
-		}
+		requireContent(ctx, t, router, actorRef, payloadPath, payloadContent)
 	})
 
 	t.Run("UpperLayerWins", func(t *testing.T) {
-		got := probeJSON(ctx, t, router, actorRef, "/readfile?path="+mountPath+"/"+shadowedName)
-		if got["error"] != "" {
-			t.Fatalf("reading %s: %s", shadowedName, got["error"])
-		}
-		if got["content"] != shadowedContent {
-			t.Errorf("content = %q, want %q from the upper layer", got["content"], shadowedContent)
-		}
+		requireContent(ctx, t, router, actorRef, mountPath+"/"+shadowedName, shadowedContent)
 	})
 
 	t.Run("WhiteoutHidesLowerLayerFile", func(t *testing.T) {
-		got := probeJSON(ctx, t, router, actorRef, "/readfile?path="+mountPath+"/"+deletedName)
-		if got["error"] == "" {
-			t.Errorf("%s is readable (%q), want it hidden by the whiteout", deletedName, got["content"])
-		}
+		requireUnreadable(ctx, t, router, actorRef, mountPath+"/"+deletedName)
 	})
 
 	t.Run("MountIsReadOnly", func(t *testing.T) {
-		got := probeJSON(ctx, t, router, actorRef, "/writefile?path="+mountPath+"/should-not-exist")
-		if got["error"] == "" {
-			t.Errorf("write to the image volume succeeded, want it rejected as read-only")
+		requireWriteRejected(ctx, t, router, actorRef, mountPath+"/should-not-exist")
+	})
+
+	t.Run("SameImageVolumeAtTwoPaths", func(t *testing.T) {
+		requireContentAtBoth(ctx, t, router, actorRef, payloadPath, mountPathAlias+"/"+payloadName, payloadContent)
+	})
+
+	t.Run("SameDurableVolumeAtTwoPathsSharesWrites", func(t *testing.T) {
+		requireSharedWrite(ctx, t, router, actorRef, scratchPathA+"/multi.txt", scratchPathB+"/multi.txt")
+	})
+
+	t.Run("SameExternalVolumeAtTwoPathsSharesWrites", func(t *testing.T) {
+		if storageClass == "" {
+			t.Skipf("StorageClass %q is not installed", e2e.StorageClass)
 		}
+		requireSharedWrite(ctx, t, router, actorRef, extPathA+"/multi.txt", extPathB+"/multi.txt")
 	})
 
 	t.Run("SurvivesSuspendResume", func(t *testing.T) {
 		if _, err := clients.SubstrateAPI.SuspendActor(ctx, &ateapipb.SuspendActorRequest{Actor: actorRef.ToObjectRef()}); err != nil {
 			t.Fatalf("SuspendActor: %v", err)
 		}
 
-		// No explicit resume: routing to the actor is what wakes it.
+		// No explicit resume: routing to the actor is what wakes it. Restore
+		// must re-establish all mounts and preserve shared writes across them.
 		resumeCtx, cancel := context.WithTimeout(ctx, 2*time.Minute)
 		defer cancel()
-		got := probeJSON(resumeCtx, t, router, actorRef, "/readfile?path="+payloadPath)
-		if got["error"] != "" {
-			t.Fatalf("reading %s after resume: %s", payloadPath, got["error"])
-		}
-		if got["content"] != payloadContent {
-			t.Errorf("content after resume = %q, want %q", got["content"], payloadContent)
-		}
+		requireContentAtBoth(resumeCtx, t, router, actorRef, payloadPath, mountPathAlias+"/"+payloadName, payloadContent)
+		requireContentAtBoth(resumeCtx, t, router, actorRef, scratchPathA+"/multi.txt", scratchPathB+"/multi.txt", probeWrittenContent)
+
+		// Suspend detached the external volume; the resume must reattach it
+		// once and restore both of its mounts.
+		t.Run("ExternalVolumeReattached", func(t *testing.T) {
+			if storageClass == "" {
+				t.Skipf("StorageClass %q is not installed", e2e.StorageClass)
+			}
+			requireContentAtBoth(resumeCtx, t, router, actorRef, extPathA+"/multi.txt", extPathB+"/multi.txt", probeWrittenContent)
+		})
 	})
 }

internal/e2e/suites/imagevolume/testmain_test.go → internal/e2e/suites/combinedvolumes/testmain_test.go

@@ -12,7 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package imagevolume
+package combinedvolumes
 
 import (
 	"os"