Refactor mount annotations code base.

This change groups the following refactor work:
* Use volumeKeyPrefix consistently in shim/utils/volumes.go.
* Export boot.PodMountHints and boot.MountHint.
* Rename MountHint.isSupported() to isShared() to make it more accurate.
* Plumb MountHint more consistently throughout containerMounter methods. We do
  this by calling c.hints.findMount() once and caching the result in a new
  abstraction called `mountInfo` (earlier weirdly named mountAndFD). This also
  helps in avoiding duplicate calls to c.hints.findMount() for the same volume.

This change does not impact behavior. This is in preparation for a following
change that adds overlay on top of disk-backed EmptyDir volumes. The intent is
to reduce clutter from the more important change for ease of review.

PiperOrigin-RevId: 529009982

pkg/shim/utils/volumes.go

@@ -185,14 +185,14 @@ func configureShm(s *specs.Spec) (bool, error) {
 		m := &s.Mounts[i]
 		if m.Destination == shmPath && m.Type == "bind" {
 			if IsSandbox(s) {
-				s.Annotations["dev.gvisor.spec.mount."+devshmName+".source"] = m.Source
-				s.Annotations["dev.gvisor.spec.mount."+devshmName+".type"] = devshmType
-				s.Annotations["dev.gvisor.spec.mount."+devshmName+".share"] = "pod"
+				s.Annotations[volumeKeyPrefix+devshmName+".source"] = m.Source
+				s.Annotations[volumeKeyPrefix+devshmName+".type"] = devshmType
+				s.Annotations[volumeKeyPrefix+devshmName+".share"] = "pod"
 				// Given that we don't have visibility into mount options for all
 				// containers, assume broad access for the master mount (it's tmpfs
 				// inside the sandbox anyways) and apply options to subcontainers as
 				// they bind mount individually.
-				s.Annotations["dev.gvisor.spec.mount."+devshmName+".options"] = "rw"
+				s.Annotations[volumeKeyPrefix+devshmName+".options"] = "rw"
 			}
 
 			changeMountType(m, devshmType)

runsc/boot/loader.go

@@ -147,7 +147,7 @@ type Loader struct {
 
 	// mountHints provides extra information about mounts for containers that
 	// apply to the entire pod.
-	mountHints *podMountHints
+	mountHints *PodMountHints
 
 	// productName is the value to show in
 	// /sys/devices/virtual/dmi/id/product_name.
@@ -442,7 +442,7 @@ func New(args Args) (*Loader, error) {
 		return nil, fmt.Errorf("initializing compat logs: %w", err)
 	}
 
-	mountHints, err := newPodMountHints(args.Spec)
+	mountHints, err := NewPodMountHints(args.Spec)
 	if err != nil {
 		return nil, fmt.Errorf("creating pod mount hints: %w", err)
 	}

runsc/boot/mount_hints.go

@@ -74,13 +74,14 @@ func (s shareType) String() string {
 	}
 }
 
-// podMountHints contains a collection of mountHints for the pod.
-type podMountHints struct {
-	mounts map[string]*mountHint
+// PodMountHints contains a collection of mountHints for the pod.
+type PodMountHints struct {
+	mounts map[string]*MountHint
 }
 
-func newPodMountHints(spec *specs.Spec) (*podMountHints, error) {
-	mnts := make(map[string]*mountHint)
+// NewPodMountHints instantiates PodMountHints using spec.
+func NewPodMountHints(spec *specs.Spec) (*PodMountHints, error) {
+	mnts := make(map[string]*MountHint)
 	for k, v := range spec.Annotations {
 		// Look for 'dev.gvisor.spec.mount' annotations and parse them.
 		if strings.HasPrefix(k, MountPrefix) {
@@ -95,7 +96,7 @@ func newPodMountHints(spec *specs.Spec) (*podMountHints, error) {
 			}
 			mnt := mnts[name]
 			if mnt == nil {
-				mnt = &mountHint{name: name}
+				mnt = &MountHint{name: name}
 				mnts[name] = mnt
 			}
 			if err := mnt.setField(parts[1], v); err != nil {
@@ -121,13 +122,13 @@ func newPodMountHints(spec *specs.Spec) (*podMountHints, error) {
 		}
 	}
 
-	return &podMountHints{mounts: mnts}, nil
+	return &PodMountHints{mounts: mnts}, nil
 }
 
-// mountHint represents extra information about mounts that are provided via
+// MountHint represents extra information about mounts that are provided via
 // annotations. They can override mount type, and provide sharing information
 // so that mounts can be correctly shared inside the pod.
-type mountHint struct {
+type MountHint struct {
 	name  string
 	share shareType
 	mount specs.Mount
@@ -137,7 +138,7 @@ type mountHint struct {
 	vfsMount *vfs.Mount
 }
 
-func (m *mountHint) setField(key, val string) error {
+func (m *MountHint) setField(key, val string) error {
 	switch key {
 	case "source":
 		if len(val) == 0 {
@@ -160,9 +161,9 @@ func (m *mountHint) setField(key, val string) error {
 	return nil
 }
 
-func (m *mountHint) setType(val string) error {
+func (m *MountHint) setType(val string) error {
 	switch val {
-	case "tmpfs", "bind":
+	case tmpfs.Name, Bind:
 		m.mount.Type = val
 	default:
 		return fmt.Errorf("invalid type %q", val)
@@ -170,7 +171,8 @@ func (m *mountHint) setType(val string) error {
 	return nil
 }
 
-func (m *mountHint) isSupported() bool {
+// isShared returns true if this mount should be configured as a shared mount.
+func (m *MountHint) isShared() bool {
 	// TODO(b/142076984): Only support tmpfs for now. Bind mounts require a
 	// common gofer to mount all shared volumes.
 	return m.mount.Type == tmpfs.Name && m.share == pod
@@ -179,7 +181,7 @@ func (m *mountHint) isSupported() bool {
 // checkCompatible verifies that shared mount is compatible with master.
 // Master options must be the same or less restrictive than the container mount,
 // e.g. master can be 'rw' while container mounts as 'ro'.
-func (m *mountHint) checkCompatible(replica *specs.Mount) error {
+func (m *MountHint) checkCompatible(replica *specs.Mount) error {
 	masterOpts := ParseMountOptions(m.mount.Options)
 	replicaOpts := ParseMountOptions(replica.Options)
 
@@ -195,14 +197,15 @@ func (m *mountHint) checkCompatible(replica *specs.Mount) error {
 	return nil
 }
 
-func (m *mountHint) fileAccessType() config.FileAccessType {
+func (m *MountHint) fileAccessType() config.FileAccessType {
 	if m.share == container {
 		return config.FileAccessExclusive
 	}
 	return config.FileAccessShared
 }
 
-func (p *podMountHints) findMount(mount *specs.Mount) *mountHint {
+// FindMount finds the MountHint that applies to this mount.
+func (p *PodMountHints) FindMount(mount *specs.Mount) *MountHint {
 	for _, m := range p.mounts {
 		if m.mount.Source == mount.Source {
 			return m

runsc/boot/mount_hints_test.go

@@ -35,7 +35,7 @@ func TestPodMountHintsHappy(t *testing.T) {
 			MountPrefix + "mount2.options": "rw,private",
 		},
 	}
-	podHints, err := newPodMountHints(spec)
+	podHints, err := NewPodMountHints(spec)
 	if err != nil {
 		t.Fatalf("newPodMountHints failed: %v", err)
 	}
@@ -112,7 +112,7 @@ func TestPodMountHintsErrors(t *testing.T) {
 	} {
 		t.Run(tst.name, func(t *testing.T) {
 			spec := &specs.Spec{Annotations: tst.annotations}
-			podHints, err := newPodMountHints(spec)
+			podHints, err := NewPodMountHints(spec)
 			if err == nil || !strings.Contains(err.Error(), tst.error) {
 				t.Errorf("newPodMountHints invalid error, want: .*%s.*, got: %v", tst.error, err)
 			}
@@ -157,7 +157,7 @@ func TestPodMountHintsIgnore(t *testing.T) {
 	} {
 		t.Run(tst.name, func(t *testing.T) {
 			spec := &specs.Spec{Annotations: tst.annotations}
-			podHints, err := newPodMountHints(spec)
+			podHints, err := NewPodMountHints(spec)
 			if err != nil {
 				t.Errorf("newPodMountHints() failed: %v", err)
 			} else if podHints != nil {
@@ -178,7 +178,7 @@ func TestIgnoreInvalidMountOptions(t *testing.T) {
 			MountPrefix + "mount1.options": "rw,invalid,private",
 		},
 	}
-	podHints, err := newPodMountHints(spec)
+	podHints, err := NewPodMountHints(spec)
 	if err != nil {
 		t.Fatalf("newPodMountHints failed: %v", err)
 	}
@@ -233,7 +233,7 @@ func TestHintsCheckCompatible(t *testing.T) {
 		},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
-			master := mountHint{mount: specs.Mount{Options: tc.masterOpts}}
+			master := MountHint{mount: specs.Mount{Options: tc.masterOpts}}
 			replica := specs.Mount{Options: tc.replicaOpts}
 			if err := master.checkCompatible(&replica); err != nil {
 				if !strings.Contains(err.Error(), tc.err) {

runsc/boot/vfs.go

@@ -351,7 +351,7 @@ type containerMounter struct {
 
 	k *kernel.Kernel
 
-	hints *podMountHints
+	hints *PodMountHints
 
 	// productName is the value to show in
 	// /sys/devices/virtual/dmi/id/product_name.
@@ -361,7 +361,7 @@ type containerMounter struct {
 	sandboxID string
 }
 
-func newContainerMounter(info *containerInfo, k *kernel.Kernel, hints *podMountHints, productName string, sandboxID string) *containerMounter {
+func newContainerMounter(info *containerInfo, k *kernel.Kernel, hints *PodMountHints, productName string, sandboxID string) *containerMounter {
 	return &containerMounter{
 		root:                info.spec.Root,
 		mounts:              compileMounts(info.spec, info.conf),
@@ -381,8 +381,8 @@ func (c *containerMounter) checkDispenser() error {
 	return nil
 }
 
-func (c *containerMounter) getMountAccessType(conf *config.Config, mount *specs.Mount) config.FileAccessType {
-	if hint := c.hints.findMount(mount); hint != nil {
+func (c *containerMounter) getMountAccessType(conf *config.Config, mount *specs.Mount, hint *MountHint) config.FileAccessType {
+	if hint != nil {
 		return hint.fileAccessType()
 	}
 	return conf.FileAccessMounts
@@ -624,10 +624,10 @@ func (c *containerMounter) mountSubmounts(ctx context.Context, conf *config.Conf
 			err error
 		)
 
-		if hint := c.hints.findMount(submount.mount); hint != nil && hint.isSupported() {
-			mnt, err = c.mountSharedSubmount(ctx, conf, mns, creds, submount.mount, hint)
+		if submount.hint != nil && submount.hint.isShared() {
+			mnt, err = c.mountSharedSubmount(ctx, conf, mns, creds, submount.mount, submount.hint)
 			if err != nil {
-				return fmt.Errorf("mount shared mount %q to %q: %v", hint.name, submount.mount.Destination, err)
+				return fmt.Errorf("mount shared mount %q to %q: %v", submount.hint.name, submount.mount.Destination, err)
 			}
 		} else {
 			mnt, err = c.mountSubmount(ctx, conf, mns, creds, submount)
@@ -656,20 +656,21 @@ func (c *containerMounter) mountSubmounts(ctx context.Context, conf *config.Conf
 	return nil
 }
 
-type mountAndFD struct {
+type mountInfo struct {
 	mount *specs.Mount
 	fd    int
+	hint  *MountHint
 }
 
-func newNonGoferMountAndFD(mnt *specs.Mount) *mountAndFD {
-	return &mountAndFD{mount: mnt, fd: -1}
+func newNonGoferMountInfo(mount *specs.Mount) *mountInfo {
+	return &mountInfo{mount: mount, fd: -1}
 }
 
-func (c *containerMounter) prepareMounts() ([]mountAndFD, error) {
+func (c *containerMounter) prepareMounts() ([]mountInfo, error) {
 	// Associate bind mounts with their FDs before sorting since there is an
 	// undocumented assumption that FDs are dispensed in the order in which
 	// they are required by mounts.
-	var mounts []mountAndFD
+	var mounts []mountInfo
 	for i := range c.mounts {
 		m := &c.mounts[i]
 		specutils.MaybeConvertToBindMount(m)
@@ -680,9 +681,10 @@ func (c *containerMounter) prepareMounts() ([]mountAndFD, error) {
 		if m.Type == Bind {
 			fd = c.fds.remove()
 		}
-		mounts = append(mounts, mountAndFD{
+		mounts = append(mounts, mountInfo{
 			mount: m,
 			fd:    fd,
+			hint:  c.hints.FindMount(m),
 		})
 	}
 	if err := c.checkDispenser(); err != nil {
@@ -697,7 +699,7 @@ func (c *containerMounter) prepareMounts() ([]mountAndFD, error) {
 	return mounts, nil
 }
 
-func (c *containerMounter) mountSubmount(ctx context.Context, conf *config.Config, mns *vfs.MountNamespace, creds *auth.Credentials, submount *mountAndFD) (*vfs.Mount, error) {
+func (c *containerMounter) mountSubmount(ctx context.Context, conf *config.Config, mns *vfs.MountNamespace, creds *auth.Credentials, submount *mountInfo) (*vfs.Mount, error) {
 	fsName, opts, useOverlay, err := c.getMountNameAndOptions(conf, submount)
 	if err != nil {
 		return nil, fmt.Errorf("mountOptions failed: %w", err)
@@ -740,7 +742,7 @@ func (c *containerMounter) mountSubmount(ctx context.Context, conf *config.Confi
 
 // getMountNameAndOptions retrieves the fsName, opts, and useOverlay values
 // used for mounts.
-func (c *containerMounter) getMountNameAndOptions(conf *config.Config, m *mountAndFD) (string, *vfs.MountOptions, bool, error) {
+func (c *containerMounter) getMountNameAndOptions(conf *config.Config, m *mountInfo) (string, *vfs.MountOptions, bool, error) {
 	fsName := m.mount.Type
 	useOverlay := false
 	var (
@@ -774,7 +776,7 @@ func (c *containerMounter) getMountNameAndOptions(conf *config.Config, m *mountA
 			// Check that an FD was provided to fails fast.
 			return "", nil, false, fmt.Errorf("gofer mount requires a connection FD")
 		}
-		data = goferMountData(m.fd, c.getMountAccessType(conf, m.mount), conf)
+		data = goferMountData(m.fd, c.getMountAccessType(conf, m.mount, m.hint), conf)
 		internalData = gofer.InternalFilesystemOptions{
 			UniqueID: m.mount.Destination,
 		}
@@ -896,7 +898,7 @@ func (c *containerMounter) mountTmp(ctx context.Context, conf *config.Config, cr
 			// another user. This is normally done for /tmp.
 			Options: []string{"mode=01777"},
 		}
-		if _, err := c.mountSubmount(ctx, conf, mns, creds, newNonGoferMountAndFD(&tmpMount)); err != nil {
+		if _, err := c.mountSubmount(ctx, conf, mns, creds, newNonGoferMountInfo(&tmpMount)); err != nil {
 			return fmt.Errorf("mountSubmount failed: %v", err)
 		}
 		return nil
@@ -916,7 +918,7 @@ func (c *containerMounter) mountTmp(ctx context.Context, conf *config.Config, cr
 func (c *containerMounter) processHints(conf *config.Config, creds *auth.Credentials) error {
 	ctx := c.k.SupervisorContext()
 	for _, hint := range c.hints.mounts {
-		if !hint.isSupported() {
+		if !hint.isShared() {
 			continue
 		}
 
@@ -932,11 +934,11 @@ func (c *containerMounter) processHints(conf *config.Config, creds *auth.Credent
 
 // mountSharedMaster mounts the master of a volume that is shared among
 // containers in a pod.
-func (c *containerMounter) mountSharedMaster(ctx context.Context, conf *config.Config, hint *mountHint, creds *auth.Credentials) (*vfs.Mount, error) {
+func (c *containerMounter) mountSharedMaster(ctx context.Context, conf *config.Config, hint *MountHint, creds *auth.Credentials) (*vfs.Mount, error) {
 	// Map mount type to filesystem name, and parse out the options that we are
 	// capable of dealing with.
-	mntFD := newNonGoferMountAndFD(&hint.mount)
-	fsName, opts, useOverlay, err := c.getMountNameAndOptions(conf, mntFD)
+	mntInfo := newNonGoferMountInfo(&hint.mount)
+	fsName, opts, useOverlay, err := c.getMountNameAndOptions(conf, mntInfo)
 	if err != nil {
 		return nil, err
 	}
@@ -945,14 +947,14 @@ func (c *containerMounter) mountSharedMaster(ctx context.Context, conf *config.C
 	}
 
 	if useOverlay {
-		log.Infof("Adding overlay on top of shared mount %q", mntFD.mount.Destination)
+		log.Infof("Adding overlay on top of shared mount %q", mntInfo.mount.Destination)
 		var cleanup func()
 		// TODO(b/142076984): Use an overlay for a shared EmptyDir mount. Such a
 		// mount should be backed by a self filestore, so limits can be enforced
 		// by k8s on the host. For now pass nil for useFilestoreFD.
 		opts, cleanup, err = c.configureOverlay(ctx, conf, creds, opts, fsName, nil /* useFilestoreFD */)
 		if err != nil {
-			return nil, fmt.Errorf("mounting shared volume with overlay at %q: %w", mntFD.mount.Destination, err)
+			return nil, fmt.Errorf("mounting shared volume with overlay at %q: %w", mntInfo.mount.Destination, err)
 		}
 		defer cleanup()
 		fsName = overlay.Name
@@ -963,14 +965,14 @@ func (c *containerMounter) mountSharedMaster(ctx context.Context, conf *config.C
 
 // mountSharedSubmount binds mount to a previously mounted volume that is shared
 // among containers in the same pod.
-func (c *containerMounter) mountSharedSubmount(ctx context.Context, conf *config.Config, mns *vfs.MountNamespace, creds *auth.Credentials, mount *specs.Mount, source *mountHint) (*vfs.Mount, error) {
+func (c *containerMounter) mountSharedSubmount(ctx context.Context, conf *config.Config, mns *vfs.MountNamespace, creds *auth.Credentials, mount *specs.Mount, source *MountHint) (*vfs.Mount, error) {
 	if err := source.checkCompatible(mount); err != nil {
 		return nil, err
 	}
 
 	// Ignore data and useOverlay because these were already applied to
 	// the master mount.
-	_, opts, _, err := c.getMountNameAndOptions(conf, newNonGoferMountAndFD(mount))
+	_, opts, _, err := c.getMountNameAndOptions(conf, newNonGoferMountInfo(mount))
 	if err != nil {
 		return nil, err
 	}

runsc/boot/vfs_test.go

@@ -67,13 +67,14 @@ func TestGetMountAccessType(t *testing.T) {
 	} {
 		t.Run(tst.name, func(t *testing.T) {
 			spec := &specs.Spec{Annotations: tst.annotations}
-			podHints, err := newPodMountHints(spec)
+			podHints, err := NewPodMountHints(spec)
 			if err != nil {
 				t.Fatalf("newPodMountHints failed: %v", err)
 			}
 			mounter := containerMounter{hints: podHints}
 			conf := &config.Config{FileAccessMounts: config.FileAccessShared}
-			if got := mounter.getMountAccessType(conf, &specs.Mount{Source: source}); got != tst.want {
+			mnt := &specs.Mount{Source: source}
+			if got := mounter.getMountAccessType(conf, mnt, podHints.FindMount(mnt)); got != tst.want {
 				t.Errorf("getMountAccessType(), want: %v, got: %v", tst.want, got)
 			}
 		})