Jailer leaves containers behind

The jailer will leave containers behind if the jailing process was
killed with SIGKILL.

Signed-off-by: xibz <[email protected]>

Author: xibz

go.mod

@@ -8,7 +8,7 @@ require (
 	github.com/containerd/containerd v1.3.3
 	github.com/containerd/continuity v0.0.0-20181027224239-bea7585dbfac // indirect
 	github.com/containerd/fifo v0.0.0-20190816180239-bda0ff6ed73c
-	github.com/containerd/go-runc v0.0.0-20190226155025-7d11b49dc076 // indirect
+	github.com/containerd/go-runc v0.0.0-20190226155025-7d11b49dc076
 	github.com/containerd/ttrpc v0.0.0-20190613183316-1fb3814edf44
 	github.com/containerd/typeurl v0.0.0-20181015155603-461401dc8f19
 	github.com/containernetworking/cni v0.7.2-0.20190807151350-8c6c47d1c7fc

runtime/jailer.go

@@ -55,6 +55,8 @@ type jailer interface {
 	// StubDrivesOptions will return a set of options used to create a new stub
 	// drive file
 	StubDrivesOptions() []FileOpt
+	// Close will do any necessary cleanup that the jailer has accrued.
+	Close() error
 }
 
 type cgroupPather interface {

runtime/noop_jailer.go

@@ -79,3 +79,5 @@ func (j noopJailer) StubDrivesOptions() []FileOpt {
 	j.logger.Debug("noop operation for StubDrivesOptions")
 	return []FileOpt{}
 }
+
+func (j noopJailer) Close() error { return nil }

runtime/runc_jailer.go

@@ -24,6 +24,7 @@ import (
 	"strings"
 	"syscall"
 
+	"github.com/containerd/go-runc"
 	"github.com/firecracker-microvm/firecracker-go-sdk"
 	"github.com/opencontainers/runtime-spec/specs-go"
 	"github.com/pkg/errors"
@@ -46,6 +47,7 @@ type runcJailer struct {
 	Config     runcJailerConfig
 	vmID       string
 	configSpec specs.Spec
+	runcClient runc.Runc
 }
 
 const firecrackerFileName = "firecracker"
@@ -64,10 +66,11 @@ func newRuncJailer(ctx context.Context, logger *logrus.Entry, vmID string, cfg r
 		WithField("runcBinaryPath", cfg.RuncBinPath)
 
 	j := &runcJailer{
-		ctx:    ctx,
-		logger: l,
-		Config: cfg,
-		vmID:   vmID,
+		ctx:        ctx,
+		logger:     l,
+		Config:     cfg,
+		vmID:       vmID,
+		runcClient: runc.Runc{},
 	}
 
 	spec := specs.Spec{}
@@ -447,6 +450,14 @@ func (j *runcJailer) setDefaultConfigValues(cfg *config.Config, socketPath strin
 	return spec
 }
 
+// Close will cleanup the container that may be left behind if the jailing
+// process was killed via SIGKILL.
+func (j *runcJailer) Close() error {
+	return j.runcClient.Delete(j.ctx, j.vmID, &runc.DeleteOpts{
+		Force: true,
+	})
+}
+
 func mkdirAndChown(path string, mode os.FileMode, uid, gid uint32) error {
 	if err := os.Mkdir(path, mode); err != nil {
 		return err

runtime/service.go

@@ -1278,8 +1278,16 @@ func (s *service) Cleanup(requestCtx context.Context) (*taskAPI.DeleteResponse,
 }
 
 func (s *service) monitorVMExit() {
-	// once the VM shuts down, the shim should too
-	defer s.shimCancel()
+	defer func() {
+		// we ignore the error here due to cleanup will only succeed if the jailing
+		// process was killed via SIGKILL
+		if err := s.jailer.Close(); err != nil {
+			s.logger.WithError(err).Error("failed to close jailer")
+		}
+
+		// once the VM shuts down, the shim should too
+		s.shimCancel()
+	}()
 
 	// Block until the VM exits
 	s.vmExitErr = s.machine.Wait(s.shimCtx)

runtime/service_integ_test.go

@@ -35,6 +35,7 @@ import (
 	"github.com/containerd/containerd/oci"
 	"github.com/containerd/containerd/pkg/ttrpcutil"
 	"github.com/containerd/containerd/runtime"
+	"github.com/containerd/go-runc"
 	"github.com/containerd/typeurl"
 	"github.com/containernetworking/plugins/pkg/ns"
 	"github.com/opencontainers/runtime-spec/specs-go"
@@ -64,6 +65,7 @@ const (
 	firecrackerRuntime     = "aws.firecracker"
 	shimProcessName        = "containerd-shim-aws-firecracker"
 	firecrackerProcessName = "firecracker"
+	jailerProcessName      = "runc"
 
 	defaultVMRootfsPath = "/var/lib/firecracker-containerd/runtime/default-rootfs.img"
 	defaultVMNetDevName = "eth0"
@@ -74,6 +76,7 @@ const (
 var (
 	findShim        = findProcWithName(shimProcessName)
 	findFirecracker = findProcWithName(firecrackerProcessName)
+	findJailer      = findProcWithName(jailerProcessName)
 )
 
 // Images are presumed by the isolated tests to have already been pulled
@@ -1364,6 +1367,33 @@ func TestStopVM_Isolated(t *testing.T) {
 				require.Equal(status.Code(err), codes.OK)
 			},
 		},
+		{
+			name: "Jailer SIGKILL",
+			createVMRequest: proto.CreateVMRequest{
+				JailerConfig: &proto.JailerConfig{
+					UID: 300000,
+					GID: 300000,
+				},
+			},
+			stopFunc: func(ctx context.Context, fcClient fccontrol.FirecrackerService, vmID string) {
+				firecrackerProcesses, err := internal.WaitForProcessToExist(ctx, time.Second, findJailer)
+				require.NoError(err, "failed waiting for expected firecracker process %q to come up", firecrackerProcessName)
+				require.Len(firecrackerProcesses, 1, "expected only one firecracker process to exist")
+				firecrackerProcess := firecrackerProcesses[0]
+
+				err = firecrackerProcess.KillWithContext(ctx)
+				require.NoError(err, "failed to kill firecracker process")
+
+				// Sleep here to ensure runc finishes execution
+				time.Sleep(500 * time.Millisecond)
+
+				// ensure that the jailer has cleaned up all of the containers
+				runcClient := &runc.Runc{}
+				containers, err := runcClient.List(ctx)
+				require.NoError(err, "failed to run 'runc list'")
+				assert.Equal(0, len(containers))
+			},
+		},
 
 		// Firecracker is too fast to test a case where we hit the timeout on a StopVMRequest.
 		// The rootfs below explicitly sleeps 60 seconds after shutting down the agent to simulate the case.