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 <[email protected]>
kata-containers · Aug 23, 2019 · 8a8dedc · 8a8dedc
1 parent 346d96c
commit 8a8dedc
Show file tree

Hide file tree

Showing 11 changed files with 122 additions and 51 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -1181,15 +1181,15 @@ 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 {
 		return nil, err
 	}
 
 	// Stop it.
-	if err := c.stop(false); err != nil {
+	if err := c.stop(force); err != nil {
 		return nil, err
 	}