From 8a8dedce17881e89e8f546231e1e7d03622b7af3 Mon Sep 17 00:00:00 2001 From: lifupan Date: Thu, 27 Jun 2019 16:43:18 +0800 Subject: [PATCH] api: add a CleanupContainer api for VC When shimv2 was killed by accident, containerd would try to launch a new shimv2 binarry to cleanup the container. In order to avoid race condition, the cleanup should be done serialized in a sandbox. Thus adding a new api to do this by locking the sandbox. Fixes:#1832 Signed-off-by: lifupan --- containerd-shim-v2/delete.go | 2 +- containerd-shim-v2/utils.go | 43 +++------------------- containerd-shim-v2/wait.go | 2 +- virtcontainers/api.go | 54 +++++++++++++++++++++++++++- virtcontainers/api_test.go | 37 +++++++++++++++++++ virtcontainers/implementation.go | 7 ++++ virtcontainers/interfaces.go | 4 ++- virtcontainers/pkg/vcmock/mock.go | 7 ++++ virtcontainers/pkg/vcmock/sandbox.go | 2 +- virtcontainers/pkg/vcmock/types.go | 11 +++--- virtcontainers/sandbox.go | 4 +-- 11 files changed, 122 insertions(+), 51 deletions(-) diff --git a/containerd-shim-v2/delete.go b/containerd-shim-v2/delete.go index 59e7faad41..ddf44a8192 100644 --- a/containerd-shim-v2/delete.go +++ b/containerd-shim-v2/delete.go @@ -23,7 +23,7 @@ func deleteContainer(ctx context.Context, s *service, c *container) error { return err } if status.State.State != types.StateStopped { - _, err = s.sandbox.StopContainer(c.id) + _, err = s.sandbox.StopContainer(c.id, false) if err != nil { return err } diff --git a/containerd-shim-v2/utils.go b/containerd-shim-v2/utils.go index c9c7a15b5e..4645dcd558 100644 --- a/containerd-shim-v2/utils.go +++ b/containerd-shim-v2/utils.go @@ -11,7 +11,6 @@ import ( "fmt" "os" "path/filepath" - "syscall" "time" "github.com/containerd/containerd/mount" @@ -36,51 +35,17 @@ func cReap(s *service, status int, id, execid string, exitat time.Time) { func cleanupContainer(ctx context.Context, sid, cid, bundlePath string) error { logrus.WithField("Service", "Cleanup").WithField("container", cid).Info("Cleanup container") - rootfs := filepath.Join(bundlePath, "rootfs") - sandbox, err := vci.FetchSandbox(ctx, sid) - if err != nil { - return err - } - - status, err := sandbox.StatusContainer(cid) + err := vci.CleanupContainer(ctx, sid, cid) if err != nil { - logrus.WithError(err).WithField("container", cid).Warn("failed to get container status") - return err - } - - if oci.StateToOCIState(status.State.State) != oci.StateStopped { - err := sandbox.KillContainer(cid, syscall.SIGKILL, true) - if err != nil { - logrus.WithError(err).WithField("container", cid).Warn("failed to kill container") - return err - } - } - - if _, err = sandbox.StopContainer(cid); err != nil { - logrus.WithError(err).WithField("container", cid).Warn("failed to stop container") + logrus.WithError(err).WithField("container", cid).Warn("failed to cleanup container") return err } - if _, err := sandbox.DeleteContainer(cid); err != nil { - logrus.WithError(err).WithField("container", cid).Warn("failed to remove container") - } + rootfs := filepath.Join(bundlePath, "rootfs") if err := mount.UnmountAll(rootfs, 0); err != nil { logrus.WithError(err).WithField("container", cid).Warn("failed to cleanup container rootfs") - } - - if len(sandbox.GetAllContainers()) == 0 { - err = sandbox.Stop(true) - if err != nil { - logrus.WithError(err).WithField("sandbox", sid).Warn("failed to stop sandbox") - return err - } - - err = sandbox.Delete() - if err != nil { - logrus.WithError(err).WithField("sandbox", sid).Warnf("failed to delete sandbox") - return err - } + return err } return nil diff --git a/containerd-shim-v2/wait.go b/containerd-shim-v2/wait.go index 7b5e80f6a6..b2299343f4 100644 --- a/containerd-shim-v2/wait.go +++ b/containerd-shim-v2/wait.go @@ -65,7 +65,7 @@ func wait(s *service, c *container, execID string) (int32, error) { logrus.WithField("sandbox", s.sandbox.ID()).Error("failed to delete sandbox") } } else { - if _, err = s.sandbox.StopContainer(c.id); err != nil { + if _, err = s.sandbox.StopContainer(c.id, false); err != nil { logrus.WithError(err).WithField("container", c.id).Warn("stop container failed") } } diff --git a/virtcontainers/api.go b/virtcontainers/api.go index cfdc861b30..08eca09163 100644 --- a/virtcontainers/api.go +++ b/virtcontainers/api.go @@ -494,7 +494,7 @@ func StopContainer(ctx context.Context, sandboxID, containerID string) (VCContai } defer s.releaseStatelessSandbox() - return s.StopContainer(containerID) + return s.StopContainer(containerID, false) } // EnterContainer is the virtcontainers container command execution entry point. @@ -929,3 +929,55 @@ func ListRoutes(ctx context.Context, sandboxID string) ([]*vcTypes.Route, error) return s.ListRoutes() } + +// CleanupContaienr is used by shimv2 to stop and delete a container exclusively, once there is no container +// in the sandbox left, do stop the sandbox and delete it. Those serial operations will be done exclusively by +// locking the sandbox. +func CleanupContainer(ctx context.Context, sandboxID, containerID string) error { + span, ctx := trace(ctx, "CleanupContainer") + defer span.Finish() + + if sandboxID == "" { + return vcTypes.ErrNeedSandboxID + } + + if containerID == "" { + return vcTypes.ErrNeedContainerID + } + + lockFile, err := rwLockSandbox(ctx, sandboxID) + if err != nil { + return err + } + defer unlockSandbox(ctx, sandboxID, lockFile) + + s, err := fetchSandbox(ctx, sandboxID) + if err != nil { + return err + } + + defer s.Release() + + _, err = s.StopContainer(containerID, true) + if err != nil { + return err + } + + if _, err := s.DeleteContainer(containerID); err != nil { + return err + } + + if len(s.GetAllContainers()) > 0 { + return nil + } + + if err = s.Stop(true); err != nil { + return err + } + + if err = s.Delete(); err != nil { + return err + } + + return nil +} diff --git a/virtcontainers/api_test.go b/virtcontainers/api_test.go index dff982d534..9c2dfb2f43 100644 --- a/virtcontainers/api_test.go +++ b/virtcontainers/api_test.go @@ -1704,3 +1704,40 @@ func TestNetworkOperation(t *testing.T) { _, err = ListRoutes(ctx, s.ID()) assert.NoError(err) } + +func TestCleanupContainer(t *testing.T) { + config := newTestSandboxConfigNoop() + + ctx := context.Background() + + p, _, err := createAndStartSandbox(ctx, config) + if p == nil || err != nil { + t.Fatal(err) + } + + contIDs := []string{"100", "101", "102", "103", "104"} + for _, contID := range contIDs { + contConfig := newTestContainerConfigNoop(contID) + + c, err := p.CreateContainer(contConfig) + if c == nil || err != nil { + t.Fatal(err) + } + + c, err = p.StartContainer(c.ID()) + if c == nil || err != nil { + t.Fatal(err) + } + } + + for _, c := range p.GetAllContainers() { + CleanupContainer(ctx, p.ID(), c.ID()) + } + + sandboxDir := store.SandboxConfigurationRootPath(p.ID()) + + _, err = os.Stat(sandboxDir) + if err == nil { + t.Fatal(err) + } +} diff --git a/virtcontainers/implementation.go b/virtcontainers/implementation.go index fd91535444..6692a8640e 100644 --- a/virtcontainers/implementation.go +++ b/virtcontainers/implementation.go @@ -176,3 +176,10 @@ func (impl *VCImpl) UpdateRoutes(ctx context.Context, sandboxID string, routes [ func (impl *VCImpl) ListRoutes(ctx context.Context, sandboxID string) ([]*vcTypes.Route, error) { return ListRoutes(ctx, sandboxID) } + +// CleanupContaienr is used by shimv2 to stop and delete a container exclusively, once there is no container +// in the sandbox left, do stop the sandbox and delete it. Those serial operations will be done exclusively by +// locking the sandbox. +func (impl *VCImpl) CleanupContainer(ctx context.Context, sandboxID, containerID string) error { + return CleanupContainer(ctx, sandboxID, containerID) +} diff --git a/virtcontainers/interfaces.go b/virtcontainers/interfaces.go index 38dfc5f4ed..fbb2dd0c6f 100644 --- a/virtcontainers/interfaces.go +++ b/virtcontainers/interfaces.go @@ -54,6 +54,8 @@ type VC interface { ListInterfaces(ctx context.Context, sandboxID string) ([]*vcTypes.Interface, error) UpdateRoutes(ctx context.Context, sandboxID string, routes []*vcTypes.Route) ([]*vcTypes.Route, error) ListRoutes(ctx context.Context, sandboxID string) ([]*vcTypes.Route, error) + + CleanupContainer(ctx context.Context, sandboxID, containerID string) error } // VCSandbox is the Sandbox interface @@ -78,7 +80,7 @@ type VCSandbox interface { CreateContainer(contConfig ContainerConfig) (VCContainer, error) DeleteContainer(contID string) (VCContainer, error) StartContainer(containerID string) (VCContainer, error) - StopContainer(containerID string) (VCContainer, error) + StopContainer(containerID string, force bool) (VCContainer, error) KillContainer(containerID string, signal syscall.Signal, all bool) error StatusContainer(containerID string) (ContainerStatus, error) StatsContainer(containerID string) (ContainerStats, error) diff --git a/virtcontainers/pkg/vcmock/mock.go b/virtcontainers/pkg/vcmock/mock.go index 4b60575b9e..77e8b35588 100644 --- a/virtcontainers/pkg/vcmock/mock.go +++ b/virtcontainers/pkg/vcmock/mock.go @@ -298,3 +298,10 @@ func (m *VCMock) ListRoutes(ctx context.Context, sandboxID string) ([]*vcTypes.R return nil, fmt.Errorf("%s: %s (%+v): sandboxID: %v", mockErrorPrefix, getSelf(), m, sandboxID) } + +func (m *VCMock) CleanupContainer(ctx context.Context, sandboxID, containerID string) error { + if m.CleanupContainerFunc != nil { + return m.CleanupContainerFunc(ctx, sandboxID, containerID) + } + return fmt.Errorf("%s: %s (%+v): sandboxID: %v", mockErrorPrefix, getSelf(), m, sandboxID) +} diff --git a/virtcontainers/pkg/vcmock/sandbox.go b/virtcontainers/pkg/vcmock/sandbox.go index 3d770db8fd..677457efe2 100644 --- a/virtcontainers/pkg/vcmock/sandbox.go +++ b/virtcontainers/pkg/vcmock/sandbox.go @@ -109,7 +109,7 @@ func (s *Sandbox) StartContainer(contID string) (vc.VCContainer, error) { } // StopContainer implements the VCSandbox function of the same name. -func (s *Sandbox) StopContainer(contID string) (vc.VCContainer, error) { +func (s *Sandbox) StopContainer(contID string, force bool) (vc.VCContainer, error) { return &Container{}, nil } diff --git a/virtcontainers/pkg/vcmock/types.go b/virtcontainers/pkg/vcmock/types.go index 9a65389cd3..3ea6e58415 100644 --- a/virtcontainers/pkg/vcmock/types.go +++ b/virtcontainers/pkg/vcmock/types.go @@ -70,9 +70,10 @@ type VCMock struct { AddDeviceFunc func(ctx context.Context, sandboxID string, info config.DeviceInfo) (api.Device, error) - AddInterfaceFunc func(ctx context.Context, sandboxID string, inf *vcTypes.Interface) (*vcTypes.Interface, error) - RemoveInterfaceFunc func(ctx context.Context, sandboxID string, inf *vcTypes.Interface) (*vcTypes.Interface, error) - ListInterfacesFunc func(ctx context.Context, sandboxID string) ([]*vcTypes.Interface, error) - UpdateRoutesFunc func(ctx context.Context, sandboxID string, routes []*vcTypes.Route) ([]*vcTypes.Route, error) - ListRoutesFunc func(ctx context.Context, sandboxID string) ([]*vcTypes.Route, error) + AddInterfaceFunc func(ctx context.Context, sandboxID string, inf *vcTypes.Interface) (*vcTypes.Interface, error) + RemoveInterfaceFunc func(ctx context.Context, sandboxID string, inf *vcTypes.Interface) (*vcTypes.Interface, error) + ListInterfacesFunc func(ctx context.Context, sandboxID string) ([]*vcTypes.Interface, error) + UpdateRoutesFunc func(ctx context.Context, sandboxID string, routes []*vcTypes.Route) ([]*vcTypes.Route, error) + ListRoutesFunc func(ctx context.Context, sandboxID string) ([]*vcTypes.Route, error) + CleanupContainerFunc func(ctx context.Context, sandboxID, containerID string) error } diff --git a/virtcontainers/sandbox.go b/virtcontainers/sandbox.go index d941a675b6..2e4d5b8620 100644 --- a/virtcontainers/sandbox.go +++ b/virtcontainers/sandbox.go @@ -1181,7 +1181,7 @@ func (s *Sandbox) StartContainer(containerID string) (VCContainer, error) { } // StopContainer stops a container in the sandbox -func (s *Sandbox) StopContainer(containerID string) (VCContainer, error) { +func (s *Sandbox) StopContainer(containerID string, force bool) (VCContainer, error) { // Fetch the container. c, err := s.findContainer(containerID) if err != nil { @@ -1189,7 +1189,7 @@ func (s *Sandbox) StopContainer(containerID string) (VCContainer, error) { } // Stop it. - if err := c.stop(false); err != nil { + if err := c.stop(force); err != nil { return nil, err }