Remove a shim directory on StopVM() correctly and synchronously

Previously the cleanup logic is in firecracker-control, but it doesn't
work correctly with a runc-based jailer, since the jailer is making
files without namespaces.

Morever the cleanup logic is running after StopVM() that makes our
contracts unclear. We should guarantee all resource are deleted at
the end of StopVM().

Signed-off-by: Kazuyoshi Kato <katokazu@amazon.com>

Author: Kazuyoshi Kato

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,10 +81,19 @@ 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) {

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"
@@ -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

@@ -492,8 +492,13 @@ func (s *service) createVM(requestCtx context.Context, request *proto.CreateVMRe
 		}
 	}()
 
+	namespace, ok := namespaces.Namespace(s.shimCtx)
+	if !ok {
+		namespace = namespaces.Default
+	}
+
 	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, filepath.Join(s.config.ShimBaseDir, namespace, s.vmID), s, request)
 	if err != nil {
 		return errors.Wrap(err, "failed to create jailer")
 	}
@@ -1165,11 +1170,21 @@ func (s *service) shutdown(
 		s.shutdownLoop(requestCtx, timeout, req)
 	}()
 
-	err := s.machine.Wait(context.Background())
-	if err == nil {
+	var result *multierror.Error
+	if err := s.machine.Wait(context.Background()); err != nil {
+		result = multierror.Append(result, err)
+	}
+	if err := s.jailer.Close(); err != nil {
+		result = multierror.Append(result, err)
+	}
+	if result == nil {
 		return nil
 	}
-	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.