From 5398a3b7ec321ac7e0a2115f86a200c87ac331e4 Mon Sep 17 00:00:00 2001 From: Lantao Liu Date: Wed, 7 Jun 2017 02:28:51 +0000 Subject: [PATCH 1/2] Add mount/unmount in os interface Signed-off-by: Lantao Liu --- pkg/os/os.go | 16 ++++++++++-- pkg/os/testing/fake_os.go | 55 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 2 deletions(-) diff --git a/pkg/os/os.go b/pkg/os/os.go index 1caed5eeb..bc279d9b6 100644 --- a/pkg/os/os.go +++ b/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) +} diff --git a/pkg/os/testing/fake_os.go b/pkg/os/testing/fake_os.go index 10709915a..84d8cef20 100644 --- a/pkg/os/testing/fake_os.go +++ b/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 } @@ -153,5 +183,30 @@ func (f *FakeOS) WriteFile(filename string, data []byte, perm os.FileMode) error if f.WriteFileFn != nil { return f.WriteFileFn(filename, data, perm) } +} + +// 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 } From 9d5990fe4f2d0fc0d5561eafc319ab70794be9de Mon Sep 17 00:00:00 2001 From: Lantao Liu Date: Wed, 7 Jun 2017 02:28:53 +0000 Subject: [PATCH 2/2] Add sandbox /dev/shm. Signed-off-by: Lantao Liu --- pkg/os/testing/fake_os.go | 1 + pkg/server/container_start.go | 12 ++- pkg/server/container_start_test.go | 34 +++++++- pkg/server/helpers.go | 9 +++ pkg/server/sandbox_run.go | 39 +++++++-- pkg/server/sandbox_run_test.go | 123 +++++++++++++++++++++++++---- 6 files changed, 196 insertions(+), 22 deletions(-) diff --git a/pkg/os/testing/fake_os.go b/pkg/os/testing/fake_os.go index 84d8cef20..c1d5aa9b9 100644 --- a/pkg/os/testing/fake_os.go +++ b/pkg/os/testing/fake_os.go @@ -183,6 +183,7 @@ func (f *FakeOS) WriteFile(filename string, data []byte, perm os.FileMode) error if f.WriteFileFn != nil { return f.WriteFileFn(filename, data, perm) } + return nil } // Mount is a fake call that invokes MountFn or just return nil. diff --git a/pkg/server/container_start.go b/pkg/server/container_start.go index 0a4ecf2d2..aedf3bcec 100644 --- a/pkg/server/container_start.go +++ b/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() diff --git a/pkg/server/container_start_test.go b/pkg/server/container_start_test.go index 101cfceb7..e05d9398d 100644 --- a/pkg/server/container_start_test.go +++ b/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, }, }, diff --git a/pkg/server/helpers.go b/pkg/server/helpers.go index 32d70bf2e..218e944ab 100644 --- a/pkg/server/helpers.go +++ b/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) ( diff --git a/pkg/server/sandbox_run.go b/pkg/server/sandbox_run.go index 8a8a5f23a..a29298b61 100644 --- a/pkg/server/sandbox_run.go +++ b/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 - // on error. + defer func() { + 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 - // non-HostIpc. What about mqueue? + // Setup sandbox /dev/shm. + 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) + } + } +} diff --git a/pkg/server/sandbox_run_test.go b/pkg/server/sandbox_run_test.go index c45a4f6c5..48109cd94 100644 --- a/pkg/server/sandbox_run_test.go +++ b/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{}{ - {"/etc/hosts", testRootDir + "/hosts", os.FileMode(0666)}, - {"/etc/resolv.conf", testRootDir + "/resolv.conf", os.FileMode(0644)}, + for desc, test := range map[string]struct { + dnsConfig *runtime.DNSConfig + 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) - assert.Equal(t, getSandboxRootDir(c.rootDir, id), dirs[0], "sandbox root directory should be created") + sandboxRootDir := getSandboxRootDir(c.rootDir, id) + 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")