From d0a730c6e861d95899a1a46cf0a78b10d861adcc Mon Sep 17 00:00:00 2001 From: Peng Tao Date: Mon, 16 Mar 2020 17:03:59 +0800 Subject: [PATCH] shimv2: move container rootfs mounted flag to container level It is in fact a container specific info not sandbox level info. We are assuming that all containers use the same snapshotter but it may not be the fact in reality. Fixes: #2532 Signed-off-by: Peng Tao --- containerd-shim-v2/container.go | 4 ++- containerd-shim-v2/container_test.go | 7 ++-- containerd-shim-v2/create.go | 49 +++++++++++++--------------- containerd-shim-v2/delete.go | 2 +- containerd-shim-v2/delete_test.go | 2 +- containerd-shim-v2/exec_test.go | 2 +- containerd-shim-v2/pause_test.go | 4 +-- containerd-shim-v2/service.go | 5 --- containerd-shim-v2/start_test.go | 6 ++-- containerd-shim-v2/wait.go | 15 +++++---- 10 files changed, 45 insertions(+), 51 deletions(-) diff --git a/containerd-shim-v2/container.go b/containerd-shim-v2/container.go index bcf89c524d..6b5e994824 100644 --- a/containerd-shim-v2/container.go +++ b/containerd-shim-v2/container.go @@ -33,9 +33,10 @@ type container struct { exit uint32 status task.Status terminal bool + mounted bool } -func newContainer(s *service, r *taskAPI.CreateTaskRequest, containerType vc.ContainerType, spec *specs.Spec) (*container, error) { +func newContainer(s *service, r *taskAPI.CreateTaskRequest, containerType vc.ContainerType, spec *specs.Spec, mounted bool) (*container, error) { if r == nil { return nil, errdefs.ToGRPCf(errdefs.ErrInvalidArgument, " CreateTaskRequest points to nil") } @@ -59,6 +60,7 @@ func newContainer(s *service, r *taskAPI.CreateTaskRequest, containerType vc.Con status: task.StatusCreated, exitIOch: make(chan struct{}), exitCh: make(chan uint32, 1), + mounted: mounted, } return c, nil } diff --git a/containerd-shim-v2/container_test.go b/containerd-shim-v2/container_test.go index 06734d0843..1cb0984494 100644 --- a/containerd-shim-v2/container_test.go +++ b/containerd-shim-v2/container_test.go @@ -6,15 +6,16 @@ package containerdshim import ( + "testing" + taskAPI "github.com/containerd/containerd/runtime/v2/task" "github.com/stretchr/testify/assert" - "testing" ) func TestNewContainer(t *testing.T) { assert := assert.New(t) - _, err := newContainer(nil, nil, "", nil) + _, err := newContainer(nil, nil, "", nil, false) assert.Error(err) } @@ -24,7 +25,7 @@ func TestGetExec(t *testing.T) { r := &taskAPI.CreateTaskRequest{} - c, err := newContainer(nil, r, "", nil) + c, err := newContainer(nil, r, "", nil, true) assert.NoError(err) _, err = c.getExec("") diff --git a/containerd-shim-v2/create.go b/containerd-shim-v2/create.go index a46962b92b..affdbae2b4 100644 --- a/containerd-shim-v2/create.go +++ b/containerd-shim-v2/create.go @@ -20,6 +20,7 @@ import ( "github.com/opencontainers/runtime-spec/specs-go" "github.com/pkg/errors" "github.com/sirupsen/logrus" + // only register the proto type _ "github.com/containerd/containerd/runtime/linux/runctypes" crioption "github.com/containerd/cri-containerd/pkg/api/runtimeoptions/v1" @@ -31,7 +32,7 @@ import ( ) func create(ctx context.Context, s *service, r *taskAPI.CreateTaskRequest) (*container, error) { - rootFs := vc.RootFs{Mounted: s.mount} + rootFs := vc.RootFs{} if len(r.Rootfs) == 1 { m := r.Rootfs[0] rootFs.Source = m.Source @@ -64,21 +65,18 @@ func create(ctx context.Context, s *service, r *taskAPI.CreateTaskRequest) (*con return nil, err } + if rootFs.Mounted, err = checkAndMount(s, r); err != nil { + return nil, err + } + defer func() { - if err != nil && s.mount { + if err != nil && rootFs.Mounted { if err2 := mount.UnmountAll(rootfs, 0); err2 != nil { logrus.WithError(err2).Warn("failed to cleanup rootfs mount") } } }() - s.mount = true - if err = checkAndMount(s, r); err != nil { - return nil, err - } - - rootFs.Mounted = s.mount - katautils.HandleFactory(ctx, vci, s.config) // Pass service's context instead of local ctx to CreateSandbox(), since local @@ -96,27 +94,25 @@ func create(ctx context.Context, s *service, r *taskAPI.CreateTaskRequest) (*con return nil, fmt.Errorf("BUG: Cannot start the container, since the sandbox hasn't been created") } - if s.mount { - defer func() { - if err != nil { - if err2 := mount.UnmountAll(rootfs, 0); err2 != nil { - logrus.WithError(err2).Warn("failed to cleanup rootfs mount") - } - } - }() - - if err = doMount(r.Rootfs, rootfs); err != nil { - return nil, err - } + if rootFs.Mounted, err = checkAndMount(s, r); err != nil { + return nil, err } + defer func() { + if err != nil && rootFs.Mounted { + if err2 := mount.UnmountAll(rootfs, 0); err2 != nil { + logrus.WithError(err2).Warn("failed to cleanup rootfs mount") + } + } + }() + _, err = katautils.CreateContainer(ctx, vci, s.sandbox, *ociSpec, rootFs, r.ID, bundlePath, "", disableOutput, true) if err != nil { return nil, err } } - container, err := newContainer(s, r, containerType, ociSpec) + container, err := newContainer(s, r, containerType, ociSpec, rootFs.Mounted) if err != nil { return nil, err } @@ -184,20 +180,19 @@ func loadRuntimeConfig(s *service, r *taskAPI.CreateTaskRequest, anno map[string return &runtimeConfig, nil } -func checkAndMount(s *service, r *taskAPI.CreateTaskRequest) error { +func checkAndMount(s *service, r *taskAPI.CreateTaskRequest) (bool, error) { if len(r.Rootfs) == 1 { m := r.Rootfs[0] if katautils.IsBlockDevice(m.Source) && !s.config.HypervisorConfig.DisableBlockDeviceUse { - s.mount = false - return nil + return false, nil } } rootfs := filepath.Join(r.Bundle, "rootfs") if err := doMount(r.Rootfs, rootfs); err != nil { - return err + return false, err } - return nil + return true, nil } func doMount(mounts []*containerd_types.Mount, rootfs string) error { diff --git a/containerd-shim-v2/delete.go b/containerd-shim-v2/delete.go index 0e4377124e..d1a47e43d4 100644 --- a/containerd-shim-v2/delete.go +++ b/containerd-shim-v2/delete.go @@ -39,7 +39,7 @@ func deleteContainer(ctx context.Context, s *service, c *container) error { return err } - if s.mount { + if c.mounted { rootfs := path.Join(c.bundle, "rootfs") if err := mount.UnmountAll(rootfs, 0); err != nil { logrus.WithError(err).Warn("failed to cleanup rootfs mount") diff --git a/containerd-shim-v2/delete_test.go b/containerd-shim-v2/delete_test.go index c77c1adae1..3ecc71c517 100644 --- a/containerd-shim-v2/delete_test.go +++ b/containerd-shim-v2/delete_test.go @@ -40,7 +40,7 @@ func TestDeleteContainerSuccessAndFail(t *testing.T) { reqCreate := &taskAPI.CreateTaskRequest{ ID: testContainerID, } - s.containers[testContainerID], err = newContainer(s, reqCreate, "", nil) + s.containers[testContainerID], err = newContainer(s, reqCreate, "", nil, true) assert.NoError(err) } diff --git a/containerd-shim-v2/exec_test.go b/containerd-shim-v2/exec_test.go index 254ec576e6..36106720d2 100644 --- a/containerd-shim-v2/exec_test.go +++ b/containerd-shim-v2/exec_test.go @@ -36,7 +36,7 @@ func TestExecNoSpecFail(t *testing.T) { } var err error - s.containers[testContainerID], err = newContainer(s, reqCreate, "", nil) + s.containers[testContainerID], err = newContainer(s, reqCreate, "", nil, false) assert.NoError(err) reqExec := &taskAPI.ExecProcessRequest{ diff --git a/containerd-shim-v2/pause_test.go b/containerd-shim-v2/pause_test.go index ff15ab99a2..081c70f7c9 100644 --- a/containerd-shim-v2/pause_test.go +++ b/containerd-shim-v2/pause_test.go @@ -57,7 +57,7 @@ func TestPauseContainerSuccess(t *testing.T) { reqCreate := &taskAPI.CreateTaskRequest{ ID: testContainerID, } - s.containers[testContainerID], err = newContainer(s, reqCreate, "", nil) + s.containers[testContainerID], err = newContainer(s, reqCreate, "", nil, true) assert.NoError(err) reqPause := &taskAPI.PauseRequest{ @@ -149,7 +149,7 @@ func TestResumeContainerSuccess(t *testing.T) { reqCreate := &taskAPI.CreateTaskRequest{ ID: testContainerID, } - s.containers[testContainerID], err = newContainer(s, reqCreate, "", nil) + s.containers[testContainerID], err = newContainer(s, reqCreate, "", nil, true) assert.NoError(err) reqResume := &taskAPI.ResumeRequest{ diff --git a/containerd-shim-v2/service.go b/containerd-shim-v2/service.go index af26d94ef5..8e9b9499ee 100644 --- a/containerd-shim-v2/service.go +++ b/containerd-shim-v2/service.go @@ -82,7 +82,6 @@ func New(ctx context.Context, id string, publisher events.Publisher) (cdshim.Shi events: make(chan interface{}, chSize), ec: make(chan exit, bufferSize), cancel: cancel, - mount: false, } go s.processExits() @@ -110,10 +109,6 @@ type service struct { // pid directly. pid uint32 - // if the container's rootfs is block device backed, kata shimv2 - // will not do the rootfs mount. - mount bool - ctx context.Context sandbox vc.VCSandbox containers map[string]*container diff --git a/containerd-shim-v2/start_test.go b/containerd-shim-v2/start_test.go index 994576eb38..bbc89b9de8 100644 --- a/containerd-shim-v2/start_test.go +++ b/containerd-shim-v2/start_test.go @@ -50,7 +50,7 @@ func TestStartStartSandboxSuccess(t *testing.T) { reqCreate := &taskAPI.CreateTaskRequest{ ID: testSandboxID, } - s.containers[testSandboxID], err = newContainer(s, reqCreate, vc.PodSandbox, nil) + s.containers[testSandboxID], err = newContainer(s, reqCreate, vc.PodSandbox, nil, false) assert.NoError(err) reqStart := &taskAPI.StartRequest{ @@ -98,7 +98,7 @@ func TestStartMissingAnnotation(t *testing.T) { reqCreate := &taskAPI.CreateTaskRequest{ ID: testSandboxID, } - s.containers[testSandboxID], err = newContainer(s, reqCreate, "", nil) + s.containers[testSandboxID], err = newContainer(s, reqCreate, "", nil, false) assert.NoError(err) reqStart := &taskAPI.StartRequest{ @@ -164,7 +164,7 @@ func TestStartStartContainerSucess(t *testing.T) { reqCreate := &taskAPI.CreateTaskRequest{ ID: testContainerID, } - s.containers[testContainerID], err = newContainer(s, reqCreate, vc.PodContainer, nil) + s.containers[testContainerID], err = newContainer(s, reqCreate, vc.PodContainer, nil, false) assert.NoError(err) reqStart := &taskAPI.StartRequest{ diff --git a/containerd-shim-v2/wait.go b/containerd-shim-v2/wait.go index b2299343f4..88b0d198f7 100644 --- a/containerd-shim-v2/wait.go +++ b/containerd-shim-v2/wait.go @@ -112,13 +112,14 @@ func watchSandbox(s *service) { logrus.WithError(err).Warn("delete sandbox failed") } - if s.mount { - for _, c := range s.containers { - rootfs := path.Join(c.bundle, "rootfs") - logrus.WithField("rootfs", rootfs).WithField("id", c.id).Debug("container umount rootfs") - if err := mount.UnmountAll(rootfs, 0); err != nil { - logrus.WithError(err).Warn("failed to cleanup rootfs mount") - } + for _, c := range s.containers { + if !c.mounted { + continue + } + rootfs := path.Join(c.bundle, "rootfs") + logrus.WithField("rootfs", rootfs).WithField("id", c.id).Debug("container umount rootfs") + if err := mount.UnmountAll(rootfs, 0); err != nil { + logrus.WithError(err).Warn("failed to cleanup rootfs mount") } }