Merge pull request #67 from Random-Liu/add-sandbox-dev-shm

Add sandbox dev shm

pkg/os/os.go

@@ -21,9 +21,9 @@ import (
 	"io/ioutil"
 	"os"
 
-	"golang.org/x/net/context"
-
 	"github.com/tonistiigi/fifo"
+	"golang.org/x/net/context"
+	"golang.org/x/sys/unix"
 )
 
 // OS collects system level operations that need to be mocked out
@@ -35,6 +35,8 @@ type OS interface {
 	Stat(name string) (os.FileInfo, error)
 	CopyFile(src, dest string, perm os.FileMode) error
 	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
 }
 
 // RealOS is used to dispatch the real system level operations.
@@ -82,3 +84,13 @@ func (RealOS) CopyFile(src, dest string, perm os.FileMode) error {
 func (RealOS) WriteFile(filename string, data []byte, perm os.FileMode) error {
 	return ioutil.WriteFile(filename, data, perm)
 }
+
+// Mount will call unix.Mount to mount the file.
+func (RealOS) Mount(source string, target string, fstype string, flags uintptr, data string) error {
+	return unix.Mount(source, target, fstype, flags, data)
+}
+
+// Unmount will call unix.Unmount to unmount the file.
+func (RealOS) Unmount(target string, flags int) error {
+	return unix.Unmount(target, flags)
+}

pkg/os/testing/fake_os.go

@@ -26,6 +26,14 @@ import (
 	osInterface "github.com/kubernetes-incubator/cri-containerd/pkg/os"
 )
 
+// CalledDetail is the struct contains called function name and arguments.
+type CalledDetail struct {
+	// Name of the function called.
+	Name string
+	// Arguments of the function called.
+	Arguments []interface{}
+}
+
 // FakeOS mocks out certain OS calls to avoid perturbing the filesystem
 // If a member of the form `*Fn` is set, that function will be called in place
 // of the real call.
@@ -37,6 +45,9 @@ type FakeOS struct {
 	StatFn      func(string) (os.FileInfo, error)
 	CopyFileFn  func(string, string, os.FileMode) error
 	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
+	calls       []CalledDetail
 	errors      map[string]error
 }
 
@@ -77,6 +88,19 @@ func (f *FakeOS) ClearErrors() {
 	f.errors = make(map[string]error)
 }
 
+func (f *FakeOS) appendCalls(name string, args ...interface{}) {
+	f.Lock()
+	defer f.Unlock()
+	f.calls = append(f.calls, CalledDetail{Name: name, Arguments: args})
+}
+
+// GetCalls get detail of calls.
+func (f *FakeOS) GetCalls() []CalledDetail {
+	f.Lock()
+	defer f.Unlock()
+	return append([]CalledDetail{}, f.calls...)
+}
+
 // NewFakeOS creates a FakeOS.
 func NewFakeOS() *FakeOS {
 	return &FakeOS{
@@ -86,6 +110,7 @@ func NewFakeOS() *FakeOS {
 
 // MkdirAll is a fake call that invokes MkdirAllFn or just returns nil.
 func (f *FakeOS) MkdirAll(path string, perm os.FileMode) error {
+	f.appendCalls("MkdirAll", path, perm)
 	if err := f.getError("MkdirAll"); err != nil {
 		return err
 	}
@@ -98,6 +123,7 @@ func (f *FakeOS) MkdirAll(path string, perm os.FileMode) error {
 
 // RemoveAll is a fake call that invokes RemoveAllFn or just returns nil.
 func (f *FakeOS) RemoveAll(path string) error {
+	f.appendCalls("RemoveAll", path)
 	if err := f.getError("RemoveAll"); err != nil {
 		return err
 	}
@@ -110,6 +136,7 @@ func (f *FakeOS) RemoveAll(path string) error {
 
 // OpenFifo is a fake call that invokes OpenFifoFn or just returns nil.
 func (f *FakeOS) OpenFifo(ctx context.Context, fn string, flag int, perm os.FileMode) (io.ReadWriteCloser, error) {
+	f.appendCalls("OpenFifo", ctx, fn, flag, perm)
 	if err := f.getError("OpenFifo"); err != nil {
 		return nil, err
 	}
@@ -122,6 +149,7 @@ func (f *FakeOS) OpenFifo(ctx context.Context, fn string, flag int, perm os.File
 
 // Stat is a fake call that invokes StatFn or just return nil.
 func (f *FakeOS) Stat(name string) (os.FileInfo, error) {
+	f.appendCalls("Stat", name)
 	if err := f.getError("Stat"); err != nil {
 		return nil, err
 	}
@@ -134,6 +162,7 @@ func (f *FakeOS) Stat(name string) (os.FileInfo, error) {
 
 // CopyFile is a fake call that invokes CopyFileFn or just return nil.
 func (f *FakeOS) CopyFile(src, dest string, perm os.FileMode) error {
+	f.appendCalls("CopyFile", src, dest, perm)
 	if err := f.getError("CopyFile"); err != nil {
 		return err
 	}
@@ -146,6 +175,7 @@ func (f *FakeOS) CopyFile(src, dest string, perm os.FileMode) error {
 
 // WriteFile is a fake call that invokes WriteFileFn or just return nil.
 func (f *FakeOS) WriteFile(filename string, data []byte, perm os.FileMode) error {
+	f.appendCalls("WriteFile", filename, data, perm)
 	if err := f.getError("WriteFile"); err != nil {
 		return err
 	}
@@ -155,3 +185,29 @@ func (f *FakeOS) WriteFile(filename string, data []byte, perm os.FileMode) error
 	}
 	return nil
 }
+
+// Mount is a fake call that invokes MountFn or just return nil.
+func (f *FakeOS) Mount(source string, target string, fstype string, flags uintptr, data string) error {
+	f.appendCalls("Mount", source, target, fstype, flags, data)
+	if err := f.getError("Mount"); err != nil {
+		return err
+	}
+
+	if f.MountFn != nil {
+		return f.MountFn(source, target, fstype, flags, data)
+	}
+	return nil
+}
+
+// Unmount is a fake call that invokes UnmountFn or just return nil.
+func (f *FakeOS) Unmount(target string, flags int) error {
+	f.appendCalls("Unmount", target, flags)
+	if err := f.getError("Unmount"); err != nil {
+		return err
+	}
+
+	if f.UnmountFn != nil {
+		return f.UnmountFn(target, flags)
+	}
+	return nil
+}

pkg/server/container_start.go

@@ -315,7 +315,15 @@ func (c *criContainerdService) generateContainerMounts(sandboxRootDir string, co
 		Readonly:      securityContext.GetReadonlyRootfs(),
 	})
 
-	// TODO(random-liu): [P0] Mount sandbox /dev/shm.
+	sandboxDevShm := getSandboxDevShm(sandboxRootDir)
+	if securityContext.GetNamespaceOptions().GetHostIpc() {
+		sandboxDevShm = devShm
+	}
+	mounts = append(mounts, &runtime.Mount{
+		ContainerPath: devShm,
+		HostPath:      sandboxDevShm,
+		Readonly:      false,
+	})
 	return mounts
 }
 
@@ -416,6 +424,8 @@ func addOCIDevices(g *generate.Generator, devs []*runtime.Device, privileged boo
 }
 
 // addOCIBindMounts adds bind mounts.
+// TODO(random-liu): Figure out whether we need to change all CRI mounts to readonly when
+// rootfs is readonly. (https://github.com/moby/moby/blob/master/daemon/oci_linux.go)
 func addOCIBindMounts(g *generate.Generator, mounts []*runtime.Mount, privileged bool) {
 	for _, mount := range mounts {
 		dst := mount.GetContainerPath()

pkg/server/container_start_test.go

@@ -324,6 +324,11 @@ func TestGenerateContainerMounts(t *testing.T) {
 					HostPath:      testSandboxRootDir + "/resolv.conf",
 					Readonly:      true,
 				},
+				{
+					ContainerPath: "/dev/shm",
+					HostPath:      testSandboxRootDir + "/shm",
+					Readonly:      false,
+				},
 			},
 		},
 		"should setup rw mount when rootfs is read-write": {
@@ -336,7 +341,34 @@ func TestGenerateContainerMounts(t *testing.T) {
 				},
 				{
 					ContainerPath: resolvConfPath,
-					HostPath:      getResolvPath(testSandboxRootDir),
+					HostPath:      testSandboxRootDir + "/resolv.conf",
+					Readonly:      false,
+				},
+				{
+					ContainerPath: "/dev/shm",
+					HostPath:      testSandboxRootDir + "/shm",
+					Readonly:      false,
+				},
+			},
+		},
+		"should use host /dev/shm when host ipc is set": {
+			securityContext: &runtime.LinuxContainerSecurityContext{
+				NamespaceOptions: &runtime.NamespaceOption{HostIpc: true},
+			},
+			expectedMounts: []*runtime.Mount{
+				{
+					ContainerPath: "/etc/hosts",
+					HostPath:      testSandboxRootDir + "/hosts",
+					Readonly:      false,
+				},
+				{
+					ContainerPath: resolvConfPath,
+					HostPath:      testSandboxRootDir + "/resolv.conf",
+					Readonly:      false,
+				},
+				{
+					ContainerPath: "/dev/shm",
+					HostPath:      "/dev/shm",
 					Readonly:      false,
 				},
 			},

pkg/server/helpers.go

@@ -58,6 +58,8 @@ const (
 	// defaultSandboxImage is the image used by sandbox container.
 	// TODO(random-liu): [P1] Build schema 2 pause image and use it here.
 	defaultSandboxImage = "gcr.io/google.com/noogler-kubernetes/pause-amd64:3.0"
+	// defaultShmSize is the default size of the sandbox shm.
+	defaultShmSize = int64(1024 * 1024 * 64)
 	// relativeRootfsPath is the rootfs path relative to bundle path.
 	relativeRootfsPath = "rootfs"
 	// defaultRuntime is the runtime to use in containerd. We may support
@@ -88,6 +90,8 @@ const (
 	utsNSFormat = "/proc/%v/ns/uts"
 	// pidNSFormat is the format of pid namespace of a process.
 	pidNSFormat = "/proc/%v/ns/pid"
+	// devShm is the default path of /dev/shm.
+	devShm = "/dev/shm"
 	// etcHosts is the default path of /etc/hosts file.
 	etcHosts = "/etc/hosts"
 	// resolvConfPath is the abs path of resolv.conf on host or container.
@@ -159,6 +163,11 @@ func getResolvPath(sandboxRoot string) string {
 	return filepath.Join(sandboxRoot, "resolv.conf")
 }
 
+// getSandboxDevShm returns the shm file path inside the sandbox root directory.
+func getSandboxDevShm(sandboxRootDir string) string {
+	return filepath.Join(sandboxRootDir, "shm")
+}
+
 // prepareStreamingPipes prepares stream named pipe for container. returns nil
 // streaming handler if corresponding stream path is empty.
 func (c *criContainerdService) prepareStreamingPipes(ctx context.Context, stdin, stdout, stderr string) (

pkg/server/sandbox_run.go

@@ -19,6 +19,7 @@ package server
 import (
 	"encoding/json"
 	"fmt"
+	"os"
 	"strings"
 	"time"
 
@@ -31,6 +32,7 @@ import (
 	runtimespec "github.com/opencontainers/runtime-spec/specs-go"
 	"github.com/opencontainers/runtime-tools/generate"
 	"golang.org/x/net/context"
+	"golang.org/x/sys/unix"
 	runtime "k8s.io/kubernetes/pkg/kubelet/apis/cri/v1alpha1"
 
 	"github.com/kubernetes-incubator/cri-containerd/pkg/metadata"
@@ -137,8 +139,11 @@ func (c *criContainerdService) RunPodSandbox(ctx context.Context, r *runtime.Run
 	if err = c.setupSandboxFiles(sandboxRootDir, config); err != nil {
 		return nil, fmt.Errorf("failed to setup sandbox files: %v", err)
 	}
-	// No need to cleanup on error, because the whole sandbox root directory will be removed
+	defer func() {
-	// on error.
+		if retErr != nil {
+			c.cleanupSandboxFiles(sandboxRootDir, config)
+		}
+	}()
 
 	// Start sandbox container.
 	spec, err := c.generateSandboxContainerSpec(id, config, imageMeta.Config)
@@ -301,7 +306,7 @@ func (c *criContainerdService) generateSandboxContainerSpec(id string, config *r
 func (c *criContainerdService) setupSandboxFiles(rootDir string, config *runtime.PodSandboxConfig) error {
 	// TODO(random-liu): Consider whether we should maintain /etc/hosts and /etc/resolv.conf in kubelet.
 	sandboxEtcHosts := getSandboxHosts(rootDir)
-	if err := c.os.CopyFile(etcHosts, sandboxEtcHosts, 0666); err != nil {
+	if err := c.os.CopyFile(etcHosts, sandboxEtcHosts, 0644); err != nil {
 		return fmt.Errorf("failed to generate sandbox hosts file %q: %v", sandboxEtcHosts, err)
 	}
 
@@ -328,8 +333,22 @@ func (c *criContainerdService) setupSandboxFiles(rootDir string, config *runtime
 		}
 	}
 
-	// TODO(random-liu): [P0] Deal with /dev/shm. Use host for HostIpc, and create and mount for
+	// Setup sandbox /dev/shm.
-	// non-HostIpc. What about mqueue?
+	if config.GetLinux().GetSecurityContext().GetNamespaceOptions().GetHostIpc() {
+		if _, err := c.os.Stat(devShm); err != nil {
+			return fmt.Errorf("host %q is not available for host ipc: %v", devShm, err)
+		}
+	} else {
+		sandboxDevShm := getSandboxDevShm(rootDir)
+		if err := c.os.MkdirAll(sandboxDevShm, 0700); err != nil {
+			return fmt.Errorf("failed to create sandbox shm: %v", err)
+		}
+		shmproperty := fmt.Sprintf("mode=1777,size=%d", defaultShmSize)
+		if err := c.os.Mount("shm", sandboxDevShm, "tmpfs", uintptr(unix.MS_NOEXEC|unix.MS_NOSUID|unix.MS_NODEV), shmproperty); err != nil {
+			return fmt.Errorf("failed to mount sandbox shm: %v", err)
+		}
+	}
+
 	return nil
 }
 
@@ -356,3 +375,13 @@ func parseDNSOptions(servers, searches, options []string) (string, error) {
 
 	return resolvContent, nil
 }
+
+// cleanupSandboxFiles only unmount files, we rely on the removal of sandbox root directory to remove files.
+// Each cleanup task should log error instead of returning, so as to keep on cleanup on error.
+func (c *criContainerdService) cleanupSandboxFiles(rootDir string, config *runtime.PodSandboxConfig) {
+	if !config.GetLinux().GetSecurityContext().GetNamespaceOptions().GetHostIpc() {
+		if err := c.os.Unmount(getSandboxDevShm(rootDir), unix.MNT_DETACH); err != nil && os.IsNotExist(err) {
+			glog.Errorf("failed to unmount sandbox shm: %v", err)
+		}
+	}
+}

pkg/server/sandbox_run_test.go

@@ -159,18 +159,112 @@ func TestGenerateSandboxContainerSpec(t *testing.T) {
 
 func TestSetupSandboxFiles(t *testing.T) {
 	testRootDir := "test-sandbox-root"
-	expectedCopys := [][]interface{}{
+	for desc, test := range map[string]struct {
-		{"/etc/hosts", testRootDir + "/hosts", os.FileMode(0666)},
+		dnsConfig     *runtime.DNSConfig
-		{"/etc/resolv.conf", testRootDir + "/resolv.conf", os.FileMode(0644)},
+		hostIpc       bool
+		expectedCalls []ostesting.CalledDetail
+	}{
+		"should check host /dev/shm existence when hostIpc is true": {
+			hostIpc: true,
+			expectedCalls: []ostesting.CalledDetail{
+				{
+					Name: "CopyFile",
+					Arguments: []interface{}{
+						"/etc/hosts", testRootDir + "/hosts", os.FileMode(0644),
+					},
+				},
+				{
+					Name: "CopyFile",
+					Arguments: []interface{}{
+						"/etc/resolv.conf", testRootDir + "/resolv.conf", os.FileMode(0644),
+					},
+				},
+				{
+					Name:      "Stat",
+					Arguments: []interface{}{"/dev/shm"},
+				},
+			},
+		},
+		"should create new /etc/resolv.conf if DNSOptions is set": {
+			dnsConfig: &runtime.DNSConfig{
+				Servers:  []string{"8.8.8.8"},
+				Searches: []string{"114.114.114.114"},
+				Options:  []string{"timeout:1"},
+			},
+			hostIpc: true,
+			expectedCalls: []ostesting.CalledDetail{
+				{
+					Name: "CopyFile",
+					Arguments: []interface{}{
+						"/etc/hosts", testRootDir + "/hosts", os.FileMode(0644),
+					},
+				},
+				{
+					Name: "WriteFile",
+					Arguments: []interface{}{
+						testRootDir + "/resolv.conf", []byte(`search 114.114.114.114
+nameserver 8.8.8.8
+options timeout:1
+`), os.FileMode(0644),
+					},
+				},
+				{
+					Name:      "Stat",
+					Arguments: []interface{}{"/dev/shm"},
+				},
+			},
+		},
+		"should create sandbox shm when hostIpc is false": {
+			hostIpc: false,
+			expectedCalls: []ostesting.CalledDetail{
+				{
+					Name: "CopyFile",
+					Arguments: []interface{}{
+						"/etc/hosts", testRootDir + "/hosts", os.FileMode(0644),
+					},
+				},
+				{
+					Name: "CopyFile",
+					Arguments: []interface{}{
+						"/etc/resolv.conf", testRootDir + "/resolv.conf", os.FileMode(0644),
+					},
+				},
+				{
+					Name: "MkdirAll",
+					Arguments: []interface{}{
+						testRootDir + "/shm", os.FileMode(0700),
+					},
+				},
+				{
+					Name: "Mount",
+					// Ignore arguments which are too complex to check.
+				},
+			},
+		},
+	} {
+		t.Logf("TestCase %q", desc)
+		c := newTestCRIContainerdService()
+		cfg := &runtime.PodSandboxConfig{
+			DnsConfig: test.dnsConfig,
+			Linux: &runtime.LinuxPodSandboxConfig{
+				SecurityContext: &runtime.LinuxSandboxSecurityContext{
+					NamespaceOptions: &runtime.NamespaceOption{
+						HostIpc: test.hostIpc,
+					},
+				},
+			},
+		}
+		c.setupSandboxFiles(testRootDir, cfg)
+		calls := c.os.(*ostesting.FakeOS).GetCalls()
+		assert.Len(t, calls, len(test.expectedCalls))
+		for i, expected := range test.expectedCalls {
+			if expected.Arguments == nil {
+				// Ignore arguments.
+				expected.Arguments = calls[i].Arguments
+			}
+			assert.Equal(t, expected, calls[i])
+		}
 	}
-	c := newTestCRIContainerdService()
-	var copys [][]interface{}
-	c.os.(*ostesting.FakeOS).CopyFileFn = func(src string, dest string, perm os.FileMode) error {
-		copys = append(copys, []interface{}{src, dest, perm})
-		return nil
-	}
-	c.setupSandboxFiles(testRootDir, nil)
-	assert.Equal(t, expectedCopys, copys, "should copy expected files for sandbox")
 }
 
 func TestRunPodSandbox(t *testing.T) {
@@ -184,7 +278,6 @@ func TestRunPodSandbox(t *testing.T) {
 	var pipes []string
 	fakeOS.MkdirAllFn = func(path string, perm os.FileMode) error {
 		dirs = append(dirs, path)
-		assert.Equal(t, os.FileMode(0755), perm)
 		return nil
 	}
 	fakeOS.OpenFifoFn = func(ctx context.Context, fn string, flag int, perm os.FileMode) (io.ReadWriteCloser, error) {
@@ -211,11 +304,11 @@ func TestRunPodSandbox(t *testing.T) {
 	require.NotNil(t, res)
 	id := res.GetPodSandboxId()
 
-	assert.Len(t, dirs, 1)
+	sandboxRootDir := getSandboxRootDir(c.rootDir, id)
-	assert.Equal(t, getSandboxRootDir(c.rootDir, id), dirs[0], "sandbox root directory should be created")
+	assert.Contains(t, dirs[0], sandboxRootDir, "sandbox root directory should be created")
 
 	assert.Len(t, pipes, 2)
-	_, stdout, stderr := getStreamingPipes(getSandboxRootDir(c.rootDir, id))
+	_, stdout, stderr := getStreamingPipes(sandboxRootDir)
 	assert.Contains(t, pipes, stdout, "sandbox stdout pipe should be created")
 	assert.Contains(t, pipes, stderr, "sandbox stderr pipe should be created")