From 7f9e0262adf118f1bf2fa5fae51aea2b8307ce9d Mon Sep 17 00:00:00 2001 From: Lantao Liu Date: Wed, 14 Jun 2017 01:57:02 +0000 Subject: [PATCH] Unmount /dev/shm when stop sandbox. Signed-off-by: Lantao Liu --- pkg/os/os.go | 8 +++++++- pkg/server/sandbox_remove.go | 1 - pkg/server/sandbox_run.go | 18 ++++++++++++------ pkg/server/sandbox_stop.go | 5 +++++ pkg/server/sandbox_stop_test.go | 13 +++++++++++++ 5 files changed, 37 insertions(+), 8 deletions(-) diff --git a/pkg/os/os.go b/pkg/os/os.go index 54a650a26..6c562d427 100644 --- a/pkg/os/os.go +++ b/pkg/os/os.go @@ -22,6 +22,7 @@ import ( "os" "github.com/containerd/fifo" + "github.com/docker/docker/pkg/mount" "golang.org/x/net/context" "golang.org/x/sys/unix" ) @@ -90,7 +91,12 @@ func (RealOS) Mount(source string, target string, fstype string, flags uintptr, return unix.Mount(source, target, fstype, flags, data) } -// Unmount will call unix.Unmount to unmount the file. +// Unmount will call unix.Unmount to unmount the file. The function doesn't +// return error if target is not mounted. func (RealOS) Unmount(target string, flags int) error { + // TODO(random-liu): Follow symlink to make sure the result is correct. + if mounted, err := mount.Mounted(target); err != nil || !mounted { + return err + } return unix.Unmount(target, flags) } diff --git a/pkg/server/sandbox_remove.go b/pkg/server/sandbox_remove.go index d490175e2..d7d018aa3 100644 --- a/pkg/server/sandbox_remove.go +++ b/pkg/server/sandbox_remove.go @@ -73,7 +73,6 @@ func (c *criContainerdService) RemovePodSandbox(ctx context.Context, r *runtime. glog.V(5).Infof("Remove called for snapshot %q that does not exist", id) } - // TODO(random-liu): [P0] Cleanup shm created in RunPodSandbox. // TODO(random-liu): [P1] Remove permanent namespace once used. // Cleanup the sandbox root directory. diff --git a/pkg/server/sandbox_run.go b/pkg/server/sandbox_run.go index 1099257ae..3e15e44b7 100644 --- a/pkg/server/sandbox_run.go +++ b/pkg/server/sandbox_run.go @@ -183,7 +183,10 @@ func (c *criContainerdService) RunPodSandbox(ctx context.Context, r *runtime.Run } defer func() { if retErr != nil { - c.cleanupSandboxFiles(sandboxRootDir, config) + if err = c.unmountSandboxFiles(sandboxRootDir, config); err != nil { + glog.Errorf("Failed to unmount sandbox files in %q: %v", + sandboxRootDir, err) + } } }() @@ -403,12 +406,15 @@ 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) { +// unmountSandboxFiles unmount some sandbox files, we rely on the removal of sandbox root directory to +// remove these files. Unmount should *NOT* return error when: +// 1) The mount point is already unmounted. +// 2) The mount point doesn't exist. +func (c *criContainerdService) unmountSandboxFiles(rootDir string, config *runtime.PodSandboxConfig) error { 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) + if err := c.os.Unmount(getSandboxDevShm(rootDir), unix.MNT_DETACH); err != nil && !os.IsNotExist(err) { + return err } } + return nil } diff --git a/pkg/server/sandbox_stop.go b/pkg/server/sandbox_stop.go index 5d6cf9498..8482353da 100644 --- a/pkg/server/sandbox_stop.go +++ b/pkg/server/sandbox_stop.go @@ -58,6 +58,11 @@ func (c *criContainerdService) StopPodSandbox(ctx context.Context, r *runtime.St } glog.V(2).Infof("TearDown network for sandbox %q successfully", id) + sandboxRoot := getSandboxRootDir(c.rootDir, id) + if err = c.unmountSandboxFiles(sandboxRoot, sandbox.Config); err != nil { + return nil, fmt.Errorf("failed to unmount sandbox files in %q: %v", sandboxRoot, err) + } + // TODO(random-liu): [P1] Handle sandbox container graceful deletion. // Delete the sandbox container from containerd. _, err = c.taskService.Delete(ctx, &execution.DeleteRequest{ContainerID: id}) diff --git a/pkg/server/sandbox_stop_test.go b/pkg/server/sandbox_stop_test.go index cb361d734..d039da477 100644 --- a/pkg/server/sandbox_stop_test.go +++ b/pkg/server/sandbox_stop_test.go @@ -58,6 +58,7 @@ func TestStopPodSandbox(t *testing.T) { injectErr error injectStatErr error injectCNIErr error + injectUnmountErr error expectErr bool expectCalls []string expectedCNICalls []string @@ -115,6 +116,15 @@ func TestStopPodSandbox(t *testing.T) { injectCNIErr: errors.New("arbitrary error"), expectCalls: []string{}, }, + "stop sandbox with unmount error": { + sandboxTasks: []task.Task{testContainer}, + injectSandbox: true, + expectErr: true, + expectedCNICalls: []string{"TearDownPod"}, + injectCNIErr: errors.New("arbitrary error"), + injectUnmountErr: errors.New("arbitrary error"), + expectCalls: []string{}, + }, } { t.Logf("TestCase %q", desc) c := newTestCRIContainerdService() @@ -136,6 +146,9 @@ func TestStopPodSandbox(t *testing.T) { if test.injectStatErr != nil { fakeOS.InjectError("Stat", test.injectStatErr) } + if test.injectUnmountErr != nil { + fakeOS.InjectError("Unmount", test.injectUnmountErr) + } fakeCNIPlugin.SetFakePodNetwork(testSandbox.NetNS, testSandbox.Config.GetMetadata().GetNamespace(), testSandbox.Config.GetMetadata().GetName(), testID, sandboxStatusTestIP)