Merge pull request #230 from miaoyq/ensure-mount-shared-slave

Ensure the mount point is propagated

pkg/os/os.go

@@ -40,6 +40,7 @@ type OS interface {
 	WriteFile(filename string, data []byte, perm os.FileMode) error
 	Mount(source string, target string, fstype string, flags uintptr, data string) error
 	Unmount(target string, flags int) error
+	GetMounts() ([]*mount.Info, error)
 }
 
 // RealOS is used to dispatch the real system level operations.
@@ -114,3 +115,8 @@ func (RealOS) Unmount(target string, flags int) error {
 	}
 	return unix.Unmount(target, flags)
 }
+
+// GetMounts retrieves a list of mounts for the current running process.
+func (RealOS) GetMounts() ([]*mount.Info, error) {
+	return mount.GetMounts()
+}

pkg/os/testing/fake_os.go

@@ -21,6 +21,7 @@ import (
 	"os"
 	"sync"
 
+	"github.com/docker/docker/pkg/mount"
 	"golang.org/x/net/context"
 
 	osInterface "github.com/kubernetes-incubator/cri-containerd/pkg/os"
@@ -48,6 +49,7 @@ type FakeOS struct {
 	WriteFileFn           func(string, []byte, os.FileMode) error
 	MountFn               func(source string, target string, fstype string, flags uintptr, data string) error
 	UnmountFn             func(target string, flags int) error
+	GetMountsFn           func() ([]*mount.Info, error)
 	calls                 []CalledDetail
 	errors                map[string]error
 }
@@ -225,3 +227,16 @@ func (f *FakeOS) Unmount(target string, flags int) error {
 	}
 	return nil
 }
+
+// GetMounts retrieves a list of mounts for the current running process.
+func (f *FakeOS) GetMounts() ([]*mount.Info, error) {
+	f.appendCalls("GetMounts")
+	if err := f.getError("GetMounts"); err != nil {
+		return nil, err
+	}
+
+	if f.GetMountsFn != nil {
+		return f.GetMountsFn()
+	}
+	return nil, nil
+}

pkg/server/container_create.go

@@ -25,6 +25,7 @@ import (
 
 	"github.com/containerd/containerd"
 	"github.com/containerd/containerd/contrib/apparmor"
+	"github.com/docker/docker/pkg/mount"
 	"github.com/golang/glog"
 	imagespec "github.com/opencontainers/image-spec/specs-go/v1"
 	"github.com/opencontainers/runc/libcontainer/devices"
@@ -551,6 +552,11 @@ func (c *criContainerdService) addOCIBindMounts(g *generate.Generator, mounts []
 		if err != nil {
 			return fmt.Errorf("failed to resolve symlink %q: %v", src, err)
 		}
+
+		mountInfos, err := c.os.GetMounts()
+		if err != nil {
+			return err
+		}
 		options := []string{"rbind"}
 		switch mount.GetPropagation() {
 		case runtime.MountPropagation_PROPAGATION_PRIVATE:
@@ -558,9 +564,15 @@ func (c *criContainerdService) addOCIBindMounts(g *generate.Generator, mounts []
 			// Since default root propogation in runc is rprivate ignore
 			// setting the root propagation
 		case runtime.MountPropagation_PROPAGATION_BIDIRECTIONAL:
+			if err := ensureShared(src, mountInfos); err != nil {
+				return err
+			}
 			options = append(options, "rshared")
 			g.SetLinuxRootPropagation("rshared") // nolint: errcheck
 		case runtime.MountPropagation_PROPAGATION_HOST_TO_CONTAINER:
+			if err := ensureSharedOrSlave(src, mountInfos); err != nil {
+				return err
+			}
 			options = append(options, "rslave")
 			if g.Spec().Linux.RootfsPropagation != "rshared" &&
 				g.Spec().Linux.RootfsPropagation != "rslave" {
@@ -703,3 +715,39 @@ func defaultRuntimeSpec() (*runtimespec.Spec, error) {
 	spec.Mounts = mounts
 	return spec, nil
 }
+
+// Ensure mount point on which path is mounted, is shared.
+func ensureShared(path string, mountInfos []*mount.Info) error {
+	sourceMount, optionalOpts, err := getSourceMount(path, mountInfos)
+	if err != nil {
+		return err
+	}
+
+	// Make sure source mount point is shared.
+	optsSplit := strings.Split(optionalOpts, " ")
+	for _, opt := range optsSplit {
+		if strings.HasPrefix(opt, "shared:") {
+			return nil
+		}
+	}
+
+	return fmt.Errorf("path %q is mounted on %q but it is not a shared mount", path, sourceMount)
+}
+
+// Ensure mount point on which path is mounted, is either shared or slave.
+func ensureSharedOrSlave(path string, mountInfos []*mount.Info) error {
+	sourceMount, optionalOpts, err := getSourceMount(path, mountInfos)
+	if err != nil {
+		return err
+	}
+	// Make sure source mount point is shared.
+	optsSplit := strings.Split(optionalOpts, " ")
+	for _, opt := range optsSplit {
+		if strings.HasPrefix(opt, "shared:") {
+			return nil
+		} else if strings.HasPrefix(opt, "master:") {
+			return nil
+		}
+	}
+	return fmt.Errorf("path %q is mounted on %q but it is not a shared or slave mount", path, sourceMount)
+}

pkg/server/container_create_test.go

@@ -20,6 +20,7 @@ import (
 	"path/filepath"
 	"testing"
 
+	"github.com/docker/docker/pkg/mount"
 	imagespec "github.com/opencontainers/image-spec/specs-go/v1"
 	runtimespec "github.com/opencontainers/runtime-spec/specs-go"
 	"github.com/opencontainers/runtime-tools/generate"
@@ -27,6 +28,7 @@ import (
 	"github.com/stretchr/testify/require"
 	"k8s.io/kubernetes/pkg/kubelet/apis/cri/v1alpha1/runtime"
 
+	ostesting "github.com/kubernetes-incubator/cri-containerd/pkg/os/testing"
 	"github.com/kubernetes-incubator/cri-containerd/pkg/util"
 )
 
@@ -80,37 +82,6 @@ func getCreateContainerTestData() (*runtime.ContainerConfig, *runtime.PodSandbox
 				HostPath:      "host-path-2",
 				Readonly:      true,
 			},
-			// Propagation private
-			{
-				ContainerPath: "container-path-3",
-				HostPath:      "host-path-3",
-				Propagation:   runtime.MountPropagation_PROPAGATION_PRIVATE,
-			},
-			// Propagation rslave
-			{
-				ContainerPath: "container-path-4",
-				HostPath:      "host-path-4",
-				Propagation:   runtime.MountPropagation_PROPAGATION_HOST_TO_CONTAINER,
-			},
-			// Propagation rshared
-			{
-				ContainerPath: "container-path-5",
-				HostPath:      "host-path-5",
-				Propagation:   runtime.MountPropagation_PROPAGATION_BIDIRECTIONAL,
-			},
-			// Propagation unknown (falls back to private)
-			{
-				ContainerPath: "container-path-6",
-				HostPath:      "host-path-6",
-				Propagation:   runtime.MountPropagation(42),
-			},
-			// Everything
-			{
-				ContainerPath: "container-path-7",
-				HostPath:      "host-path-7",
-				Readonly:      true,
-				Propagation:   runtime.MountPropagation_PROPAGATION_BIDIRECTIONAL,
-			},
 		},
 		Labels:      map[string]string{"a": "b"},
 		Annotations: map[string]string{"c": "d"},
@@ -158,11 +129,6 @@ func getCreateContainerTestData() (*runtime.ContainerConfig, *runtime.PodSandbox
 		t.Logf("Check bind mount")
 		checkMount(t, spec.Mounts, "host-path-1", "container-path-1", "bind", []string{"rbind", "rprivate", "rw"}, nil)
 		checkMount(t, spec.Mounts, "host-path-2", "container-path-2", "bind", []string{"rbind", "rprivate", "ro"}, nil)
-		checkMount(t, spec.Mounts, "host-path-3", "container-path-3", "bind", []string{"rbind", "rprivate", "rw"}, nil)
-		checkMount(t, spec.Mounts, "host-path-4", "container-path-4", "bind", []string{"rbind", "rslave", "rw"}, nil)
-		checkMount(t, spec.Mounts, "host-path-5", "container-path-5", "bind", []string{"rbind", "rshared", "rw"}, nil)
-		checkMount(t, spec.Mounts, "host-path-6", "container-path-6", "bind", []string{"rbind", "rprivate", "rw"}, nil)
-		checkMount(t, spec.Mounts, "host-path-7", "container-path-7", "bind", []string{"rbind", "rshared", "ro"}, nil)
 
 		t.Logf("Check resource limits")
 		assert.EqualValues(t, *spec.Linux.Resources.CPU.Period, 100)
@@ -610,6 +576,113 @@ func TestPrivilegedBindMount(t *testing.T) {
 	}
 }
 
+func TestMountPropagation(t *testing.T) {
+	sharedGetMountsFn := func() ([]*mount.Info, error) {
+		return []*mount.Info{
+			{
+				Mountpoint: "host-path",
+				Optional:   "shared:",
+			},
+		}, nil
+	}
+
+	slaveGetMountsFn := func() ([]*mount.Info, error) {
+		return []*mount.Info{
+			{
+				Mountpoint: "host-path",
+				Optional:   "master:",
+			},
+		}, nil
+	}
+
+	othersGetMountsFn := func() ([]*mount.Info, error) {
+		return []*mount.Info{
+			{
+				Mountpoint: "host-path",
+				Optional:   "others",
+			},
+		}, nil
+	}
+
+	for desc, test := range map[string]struct {
+		criMount        *runtime.Mount
+		fakeGetMountsFn func() ([]*mount.Info, error)
+		optionsCheck    []string
+		expectErr       bool
+	}{
+		"HostPath should mount as 'rprivate' if propagation is MountPropagation_PROPAGATION_PRIVATE": {
+			criMount: &runtime.Mount{
+				ContainerPath: "container-path",
+				HostPath:      "host-path",
+				Propagation:   runtime.MountPropagation_PROPAGATION_PRIVATE,
+			},
+			fakeGetMountsFn: nil,
+			optionsCheck:    []string{"rbind", "rprivate"},
+			expectErr:       false,
+		},
+		"HostPath should mount as 'rslave' if propagation is MountPropagation_PROPAGATION_HOST_TO_CONTAINER": {
+			criMount: &runtime.Mount{
+				ContainerPath: "container-path",
+				HostPath:      "host-path",
+				Propagation:   runtime.MountPropagation_PROPAGATION_HOST_TO_CONTAINER,
+			},
+			fakeGetMountsFn: slaveGetMountsFn,
+			optionsCheck:    []string{"rbind", "rslave"},
+			expectErr:       false,
+		},
+		"HostPath should mount as 'rshared' if propagation is MountPropagation_PROPAGATION_BIDIRECTIONAL": {
+			criMount: &runtime.Mount{
+				ContainerPath: "container-path",
+				HostPath:      "host-path",
+				Propagation:   runtime.MountPropagation_PROPAGATION_BIDIRECTIONAL,
+			},
+			fakeGetMountsFn: sharedGetMountsFn,
+			optionsCheck:    []string{"rbind", "rshared"},
+			expectErr:       false,
+		},
+		"HostPath should mount as 'rprivate' if propagation is illegal": {
+			criMount: &runtime.Mount{
+				ContainerPath: "container-path",
+				HostPath:      "host-path",
+				Propagation:   runtime.MountPropagation(42),
+			},
+			fakeGetMountsFn: nil,
+			optionsCheck:    []string{"rbind", "rprivate"},
+			expectErr:       false,
+		},
+		"Expect an error if HostPath isn't shared and mount propagation is MountPropagation_PROPAGATION_BIDIRECTIONAL": {
+			criMount: &runtime.Mount{
+				ContainerPath: "container-path",
+				HostPath:      "host-path",
+				Propagation:   runtime.MountPropagation_PROPAGATION_BIDIRECTIONAL,
+			},
+			fakeGetMountsFn: slaveGetMountsFn,
+			expectErr:       true,
+		},
+		"Expect an error if HostPath isn't slave or shared and mount propagation is MountPropagation_PROPAGATION_HOST_TO_CONTAINER": {
+			criMount: &runtime.Mount{
+				ContainerPath: "container-path",
+				HostPath:      "host-path",
+				Propagation:   runtime.MountPropagation_PROPAGATION_HOST_TO_CONTAINER,
+			},
+			fakeGetMountsFn: othersGetMountsFn,
+			expectErr:       true,
+		},
+	} {
+		t.Logf("TestCase %q", desc)
+		g := generate.New()
+		c := newTestCRIContainerdService()
+		c.os.(*ostesting.FakeOS).GetMountsFn = test.fakeGetMountsFn
+		err := c.addOCIBindMounts(&g, []*runtime.Mount{test.criMount}, "")
+		if test.expectErr {
+			require.Error(t, err)
+		} else {
+			require.NoError(t, err)
+			checkMount(t, g.Spec().Mounts, test.criMount.HostPath, test.criMount.ContainerPath, "bind", test.optionsCheck, nil)
+		}
+	}
+}
+
 func TestPidNamespace(t *testing.T) {
 	testID := "test-id"
 	testPid := uint32(1234)

pkg/server/helpers.go

@@ -29,6 +29,7 @@ import (
 	"github.com/containerd/containerd/content"
 	"github.com/containerd/containerd/errdefs"
 	"github.com/docker/distribution/reference"
+	"github.com/docker/docker/pkg/mount"
 	imagedigest "github.com/opencontainers/go-digest"
 	"github.com/opencontainers/image-spec/identity"
 	imagespec "github.com/opencontainers/image-spec/specs-go/v1"
@@ -384,3 +385,36 @@ func isInCRIMounts(dst string, mounts []*runtime.Mount) bool {
 	}
 	return false
 }
+
+//TODO: Replace with `mount.Lookup()`in containerd after #257 is marged
+func getMountInfo(mountInfos []*mount.Info, dir string) *mount.Info {
+	for _, m := range mountInfos {
+		if m.Mountpoint == dir {
+			return m
+		}
+	}
+	return nil
+}
+
+func getSourceMount(source string, mountInfos []*mount.Info) (string, string, error) {
+	mountinfo := getMountInfo(mountInfos, source)
+	if mountinfo != nil {
+		return source, mountinfo.Optional, nil
+	}
+
+	path := source
+	for {
+		path = filepath.Dir(path)
+		mountinfo = getMountInfo(mountInfos, path)
+		if mountinfo != nil {
+			return path, mountinfo.Optional, nil
+		}
+
+		if path == "/" {
+			break
+		}
+	}
+
+	// If we are here, we did not find parent mount. Something is wrong.
+	return "", "", fmt.Errorf("Could not find source mount of %s", source)
+}