Merge pull request #414 from kzys/jailer-cleanup

Remove a shim directory on StopVM() correctly and synchronously

Author: kzys

runtime/jailer_integ_test.go

@@ -15,12 +15,15 @@ package main
 
 import (
 	"context"
+	"os"
+	"path/filepath"
 	"testing"
 
 	"github.com/containerd/containerd"
 	"github.com/containerd/containerd/namespaces"
 	"github.com/containerd/containerd/oci"
 	"github.com/containerd/containerd/pkg/ttrpcutil"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
 	_ "github.com/firecracker-microvm/firecracker-containerd/firecracker-control"
@@ -78,24 +81,31 @@ func testJailer(t *testing.T, jailerConfig *proto.JailerConfig) {
 	stdout := startAndWaitTask(ctx, t, c)
 	require.Equal("hello", stdout)
 
-	defer func() {
-		err := c.Delete(ctx, containerd.WithSnapshotCleanup)
-		require.NoError(err, "failed to delete a container")
-	}()
+	stat, err := os.Stat(filepath.Join(shimBaseDir(), "default", vmID))
+	require.NoError(err)
+	assert.True(t, stat.IsDir())
+
+	err = c.Delete(ctx, containerd.WithSnapshotCleanup)
+	require.NoError(err, "failed to delete a container")
+
+	_, err = fcClient.StopVM(ctx, &proto.StopVMRequest{VMID: vmID})
+	require.NoError(err)
+
+	_, err = os.Stat(filepath.Join(shimBaseDir(), "default", vmID))
+	assert.Error(t, err)
+	assert.True(t, os.IsNotExist(err))
 }
 
 func TestJailerCPUSet_Isolated(t *testing.T) {
 	prepareIntegTest(t)
 
-	t.Run("TestJailerCPUSet_Isolated", func(t *testing.T) {
-		b := cpuset.Builder{}
-		cset := b.AddCPU(0).AddMem(0).Build()
-		config := &proto.JailerConfig{
-			CPUs: cset.CPUs(),
-			Mems: cset.Mems(),
-			UID:  300000,
-			GID:  300000,
-		}
-		testJailer(t, config)
-	})
+	b := cpuset.Builder{}
+	cset := b.AddCPU(0).AddMem(0).Build()
+	config := &proto.JailerConfig{
+		CPUs: cset.CPUs(),
+		Mems: cset.Mems(),
+		UID:  300000,
+		GID:  300000,
+	}
+	testJailer(t, config)
 }

runtime/noop_jailer.go

@@ -118,4 +118,6 @@ func (j *noopJailer) Stop() error {
 	return err
 }
 
-func (j *noopJailer) Close() error { return nil }
+func (j *noopJailer) Close() error {
+	return os.RemoveAll(j.shimDir.RootPath())
+}

runtime/runc_jailer.go

@@ -23,9 +23,11 @@ import (
 	"path/filepath"
 	"strings"
 	"syscall"
+	"time"
 
 	"github.com/containerd/go-runc"
 	"github.com/firecracker-microvm/firecracker-go-sdk"
+	"github.com/hashicorp/go-multierror"
 	"github.com/opencontainers/runtime-spec/specs-go"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
@@ -154,7 +156,7 @@ func (j *runcJailer) BuildJailedMachine(cfg *config.Config, machineConfig *firec
 func (j *runcJailer) BuildJailedRootHandler(cfg *config.Config, machineConfig *firecracker.Config, vmID string) firecracker.Handler {
 	ociBundlePath := j.OCIBundlePath()
 	rootPath := j.RootPath()
-	machineConfig.SocketPath = filepath.Join(rootPath, "api.socket")
+	machineConfig.SocketPath = filepath.Join(rootfsFolder, "api.socket")
 
 	return firecracker.Handler{
 		Name: jailerHandlerName,
@@ -466,9 +468,18 @@ func (j *runcJailer) setDefaultConfigValues(cfg *config.Config, socketPath strin
 // 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,
-	})
+	// Even the jailer's associated context is cancelled,
+	// we'd like to do the cleanups below just in case.
+	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+	defer cancel()
+
+	// Delete the container, if it is still running.
+	runcErr := j.runcClient.Delete(ctx, j.vmID, &runc.DeleteOpts{Force: true})
+
+	// Regardless of the result, remove the directory.
+	removeErr := os.RemoveAll(j.OCIBundlePath())
+
+	return multierror.Append(runcErr, removeErr).ErrorOrNil()
 }
 
 func mkdirAndChown(path string, mode os.FileMode, uid, gid uint32) error {

runtime/service.go

@@ -20,7 +20,6 @@ import (
 	"math"
 	"net"
 	"os"
-	"path/filepath"
 	"runtime/debug"
 	"strconv"
 	"strings"
@@ -122,10 +121,7 @@ type service struct {
 	config *config.Config
 
 	// vmReady is closed once CreateVM has been successfully called
-	vmReady chan struct{}
-	// vmExitErr is set with the error returned by machine.Wait(). It should
-	// only be read after shimCtx.Done() is closed
-	vmExitErr                error
+	vmReady                  chan struct{}
 	vmStartOnce              sync.Once
 	agentClient              taskAPI.TaskService
 	eventBridgeClient        eventbridge.Getter
@@ -135,6 +131,9 @@ type service struct {
 	driveMountStubs          []MountableStubDrive
 	exitAfterAllTasksDeleted bool // exit the VM and shim when all tasks are deleted
 
+	cleanupErr  error
+	cleanupOnce sync.Once
+
 	machine          *firecracker.Machine
 	machineConfig    *firecracker.Config
 	vsockIOPortCount uint32
@@ -492,8 +491,18 @@ func (s *service) createVM(requestCtx context.Context, request *proto.CreateVMRe
 		}
 	}()
 
+	namespace, ok := namespaces.Namespace(s.shimCtx)
+	if !ok {
+		namespace = namespaces.Default
+	}
+
+	dir, err := vm.ShimDir(s.config.ShimBaseDir, namespace, s.vmID)
+	if err != nil {
+		return err
+	}
+
 	s.logger.Info("creating new VM")
-	s.jailer, err = newJailer(s.shimCtx, s.logger, filepath.Join(s.config.ShimBaseDir, s.vmID), s, request)
+	s.jailer, err = newJailer(s.shimCtx, s.logger, dir.RootPath(), s, request)
 	if err != nil {
 		return errors.Wrap(err, "failed to create jailer")
 	}
@@ -574,7 +583,6 @@ func (s *service) StopVM(requestCtx context.Context, request *proto.StopVMReques
 		timeout = time.Duration(request.TimeoutSeconds) * time.Second
 	}
 
-	defer s.shimCancel()
 	err = s.waitVMReady()
 	if err != nil {
 		return nil, err
@@ -1165,11 +1173,18 @@ func (s *service) shutdown(
 		s.shutdownLoop(requestCtx, timeout, req)
 	}()
 
-	err := s.machine.Wait(context.Background())
-	if err == nil {
-		return nil
+	var result *multierror.Error
+	if err := s.machine.Wait(context.Background()); err != nil {
+		result = multierror.Append(result, err)
+	}
+	if err := s.cleanup(); err != nil {
+		result = multierror.Append(result, err)
 	}
-	return status.Error(codes.Internal, fmt.Sprintf("the VMM was killed forcibly: %v", err))
+
+	if err := result.ErrorOrNil(); err != nil {
+		return status.Error(codes.Internal, fmt.Sprintf("the VMM was killed forcibly: %v", err))
+	}
+	return nil
 }
 
 // shutdownLoop sends multiple different shutdown requests to stop the VMM.
@@ -1204,13 +1219,6 @@ func (s *service) shutdownLoop(
 			},
 			timeout: jailerStopTimeout,
 		},
-		{
-			name: "cancel the context",
-			shutdown: func() error {
-				s.shimCancel()
-				return nil
-			},
-		},
 	}
 
 	for _, action := range actions {
@@ -1286,26 +1294,38 @@ func (s *service) Cleanup(requestCtx context.Context) (*taskAPI.DeleteResponse,
 	}, nil
 }
 
-func (s *service) monitorVMExit() {
-	defer func() {
+// cleanup resources
+func (s *service) cleanup() error {
+	s.cleanupOnce.Do(func() {
+		var result *multierror.Error
 		// 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 {
+			result = multierror.Append(result, err)
 			s.logger.WithError(err).Error("failed to close jailer")
 		}
 
+		if err := s.publishVMStop(); err != nil {
+			result = multierror.Append(result, err)
+			s.logger.WithError(err).Error("failed to publish stop VM event")
+		}
+
 		// once the VM shuts down, the shim should too
 		s.shimCancel()
-	}()
 
+		s.cleanupErr = result.ErrorOrNil()
+	})
+	return s.cleanupErr
+}
+
+// monitorVMExit watches the VM and cleanup resources when it terminates.
+func (s *service) monitorVMExit() {
 	// Block until the VM exits
-	s.vmExitErr = s.machine.Wait(s.shimCtx)
-	if s.vmExitErr != nil && s.vmExitErr != context.Canceled {
-		s.logger.WithError(s.vmExitErr).Error("error returned from VM wait")
+	if err := s.machine.Wait(s.shimCtx); err != nil && err != context.Canceled {
+		s.logger.WithError(err).Error("error returned from VM wait")
 	}
 
-	publishErr := s.publishVMStop()
-	if publishErr != nil {
-		s.logger.WithError(publishErr).Error("failed to publish stop VM event")
+	if err := s.cleanup(); err != nil {
+		s.logger.WithError(err).Error("failed to clean up the VM")
 	}
 }

runtime/service_integ_test.go

@@ -470,9 +470,12 @@ func testMultipleExecs(
 	close(execStdouts)
 
 	if jailerConfig != nil {
+		dir, err := vm.ShimDir(shimBaseDir(), "default", vmIDStr)
+		require.NoError(t, err)
+
 		jailer := &runcJailer{
 			Config: runcJailerConfig{
-				OCIBundlePath: filepath.Join(shimBaseDir(), vmIDStr),
+				OCIBundlePath: dir.RootPath(),
 			},
 			vmID: vmIDStr,
 		}
@@ -580,35 +583,69 @@ func TestLongUnixSocketPath_Isolated(t *testing.T) {
 	// default location we store state results in a path like
 	// "/run/firecracker-containerd/default/<vmID>" (with len 112).
 	const maxUnixSockLen = 108
-	vmID := strings.Repeat("x", 76)
+	vmID := strings.Repeat("x", 72)
 
 	ctx := namespaces.WithNamespace(context.Background(), "default")
 
 	pluginClient, err := ttrpcutil.NewClient(containerdSockPath + ".ttrpc")
 	require.NoError(t, err, "failed to create ttrpc client")
 
-	fcClient := fccontrol.NewFirecrackerClient(pluginClient.Client())
-	_, err = fcClient.CreateVM(ctx, &proto.CreateVMRequest{
-		VMID:              vmID,
-		NetworkInterfaces: []*proto.FirecrackerNetworkInterface{},
-	})
-	require.NoError(t, err, "failed to create VM")
+	subtests := []struct {
+		name    string
+		request proto.CreateVMRequest
+	}{
+		{
+			name: "Without Jailer",
+			request: proto.CreateVMRequest{
+				VMID:              vmID + "noop",
+				NetworkInterfaces: []*proto.FirecrackerNetworkInterface{},
+			},
+		},
+		{
+			name: "With Jailer",
+			request: proto.CreateVMRequest{
+				// We somehow cannot use the same VM ID here.
+				// https://github.com/firecracker-microvm/firecracker-containerd/issues/409
+				VMID:              vmID + "jail",
+				NetworkInterfaces: []*proto.FirecrackerNetworkInterface{},
+				JailerConfig: &proto.JailerConfig{
+					UID: 30000,
+					GID: 30000,
+				},
+			},
+		},
+	}
 
-	// double-check that the sockets are at the expected path and that their absolute
-	// length exceeds 108 bytes
-	shimDir, err := vm.ShimDir(cfg.ShimBaseDir, "default", vmID)
-	require.NoError(t, err, "failed to get shim dir")
+	fcClient := fccontrol.NewFirecrackerClient(pluginClient.Client())
+	for _, subtest := range subtests {
+		request := subtest.request
+		vmID := request.VMID
+		t.Run(subtest.name, func(t *testing.T) {
+			_, err = fcClient.CreateVM(ctx, &request)
+			require.NoError(t, err, "failed to create VM")
+
+			// double-check that the sockets are at the expected path and that their absolute
+			// length exceeds 108 bytes
+			shimDir, err := vm.ShimDir(cfg.ShimBaseDir, "default", vmID)
+			require.NoError(t, err, "failed to get shim dir")
+
+			if request.JailerConfig == nil {
+				_, err = os.Stat(shimDir.FirecrackerSockPath())
+				require.NoError(t, err, "failed to stat firecracker socket path")
+				if len(shimDir.FirecrackerSockPath()) <= maxUnixSockLen {
+					assert.Failf(t, "firecracker sock absolute path %q is not greater than max unix socket path length", shimDir.FirecrackerSockPath())
+				}
 
-	_, err = os.Stat(shimDir.FirecrackerSockPath())
-	require.NoError(t, err, "failed to stat firecracker socket path")
-	if len(shimDir.FirecrackerSockPath()) <= maxUnixSockLen {
-		assert.Failf(t, "firecracker sock absolute path %q is not greater than max unix socket path length", shimDir.FirecrackerSockPath())
-	}
+				_, err = os.Stat(shimDir.FirecrackerVSockPath())
+				require.NoError(t, err, "failed to stat firecracker vsock path")
+				if len(shimDir.FirecrackerVSockPath()) <= maxUnixSockLen {
+					assert.Failf(t, "firecracker vsock absolute path %q is not greater than max unix socket path length", shimDir.FirecrackerVSockPath())
+				}
+			}
 
-	_, err = os.Stat(shimDir.FirecrackerVSockPath())
-	require.NoError(t, err, "failed to stat firecracker vsock path")
-	if len(shimDir.FirecrackerVSockPath()) <= maxUnixSockLen {
-		assert.Failf(t, "firecracker vsock absolute path %q is not greater than max unix socket path length", shimDir.FirecrackerVSockPath())
+			_, err = fcClient.StopVM(ctx, &proto.StopVMRequest{VMID: vmID})
+			require.NoError(t, err)
+		})
 	}
 }