Merge pull request #389 from kzys/stopvm-sync-2

Make StopVM() synchronous

Author: kzys

go.mod

@@ -17,7 +17,7 @@ require (
 	github.com/docker/distribution v2.7.1+incompatible // indirect
 	github.com/docker/go-events v0.0.0-20170721190031-9461782956ad // indirect
 	github.com/docker/go-metrics v0.0.0-20181218153428-b84716841b82 // indirect
-	github.com/firecracker-microvm/firecracker-go-sdk v0.19.1-0.20191114205152-9e2ff62839b2
+	github.com/firecracker-microvm/firecracker-go-sdk v0.20.1-0.20200204230548-c56932849923
 	github.com/go-ole/go-ole v1.2.4 // indirect
 	github.com/godbus/dbus v0.0.0-20181025153459-66d97aec3384 // indirect
 	github.com/gofrs/uuid v3.2.0+incompatible

go.sum

@@ -59,8 +59,8 @@ github.com/docker/go-metrics v0.0.0-20181218153428-b84716841b82 h1:X0fj836zx99zF
 github.com/docker/go-metrics v0.0.0-20181218153428-b84716841b82/go.mod h1:/u0gXw0Gay3ceNrsHubL3BtdOL2fHf93USgMTe0W5dI=
 github.com/docker/go-units v0.3.3 h1:Xk8S3Xj5sLGlG5g67hJmYMmUgXv5N4PhkjJHHqrwnTk=
 github.com/docker/go-units v0.3.3/go.mod h1:fgPhTUdO+D/Jk86RDLlptpiXQzgHJF7gydDDbaIK4Dk=
-github.com/firecracker-microvm/firecracker-go-sdk v0.19.1-0.20191114205152-9e2ff62839b2 h1:Ab52E0UlBOmMIAK/igRycsxSAJVDBDMYq+Wr/n7z2E0=
-github.com/firecracker-microvm/firecracker-go-sdk v0.19.1-0.20191114205152-9e2ff62839b2/go.mod h1:kW0gxvPpPvMukUxxTO9DrpSlScrtrTDGY3VgjAj/Qwc=
+github.com/firecracker-microvm/firecracker-go-sdk v0.20.1-0.20200204230548-c56932849923 h1:tMx0TVfjMRjHrB2L5Y3qMEfAwzFtwv+CnNUQA/i0838=
+github.com/firecracker-microvm/firecracker-go-sdk v0.20.1-0.20200204230548-c56932849923/go.mod h1:kW0gxvPpPvMukUxxTO9DrpSlScrtrTDGY3VgjAj/Qwc=
 github.com/globalsign/mgo v0.0.0-20180905125535-1ca0a4f7cbcb h1:D4uzjWwKYQ5XnAvUbuvHW93esHg7F8N/OYeBBcJoTr0=
 github.com/globalsign/mgo v0.0.0-20180905125535-1ca0a4f7cbcb/go.mod h1:xkRDCp4j0OGD1HRkm4kmhM+pmpv3AKq5SU7GMg4oO/Q=
 github.com/go-ole/go-ole v1.2.4 h1:nNBDSCOigTSiarFpYE9J/KtEA1IOW4CNeqT9TQDqCxI=

runtime/drive_handler_test.go

@@ -66,7 +66,7 @@ func TestContainerStubs(t *testing.T) {
 	err = os.MkdirAll(patchedSrcDir, 0700)
 	require.NoError(t, err, "failed to create patched src dir")
 
-	noopJailer := noopJailer{
+	noopJailer := &noopJailer{
 		shimDir: vm.Dir(stubDir),
 		ctx:     ctx,
 		logger:  logger,
@@ -139,7 +139,7 @@ func TestDriveMountStubs(t *testing.T) {
 	err = os.MkdirAll(patchedSrcDir, 0700)
 	require.NoError(t, err, "failed to create patched src dir")
 
-	noopJailer := noopJailer{
+	noopJailer := &noopJailer{
 		shimDir: vm.Dir(stubDir),
 		ctx:     ctx,
 		logger:  logger,

runtime/jailer.go

@@ -55,6 +55,10 @@ type jailer interface {
 	// StubDrivesOptions will return a set of options used to create a new stub
 	// drive file
 	StubDrivesOptions() []FileOpt
+
+	// Stop the jailer as a way that is visible from the user-level process (e.g. SIGTERM).
+	Stop() error
+
 	// Close will do any necessary cleanup that the jailer has accrued.
 	Close() error
 }

runtime/noop_jailer.go

@@ -15,8 +15,11 @@ package main
 
 import (
 	"context"
+	"os"
+	"syscall"
 
 	"github.com/firecracker-microvm/firecracker-go-sdk"
+	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 
 	"github.com/firecracker-microvm/firecracker-containerd/config"
@@ -29,17 +32,19 @@ type noopJailer struct {
 	logger  *logrus.Entry
 	shimDir vm.Dir
 	ctx     context.Context
+	pid     int
 }
 
-func newNoopJailer(ctx context.Context, logger *logrus.Entry, shimDir vm.Dir) noopJailer {
-	return noopJailer{
+func newNoopJailer(ctx context.Context, logger *logrus.Entry, shimDir vm.Dir) *noopJailer {
+	return &noopJailer{
 		logger:  logger,
 		shimDir: shimDir,
 		ctx:     ctx,
+		pid:     0,
 	}
 }
 
-func (j noopJailer) BuildJailedMachine(cfg *config.Config, machineConfig *firecracker.Config, vmID string) ([]firecracker.Opt, error) {
+func (j *noopJailer) BuildJailedMachine(cfg *config.Config, machineConfig *firecracker.Config, vmID string) ([]firecracker.Opt, error) {
 	if len(cfg.FirecrackerBinaryPath) == 0 {
 		return []firecracker.Opt{}, nil
 	}
@@ -59,25 +64,58 @@ func (j noopJailer) BuildJailedMachine(cfg *config.Config, machineConfig *firecr
 		cmd.Stderr = j.logger.WithField("vmm_stream", "stderr").WriterLevel(logrus.DebugLevel)
 	}
 
+	pidHandler := firecracker.Handler{
+		Name: "firecracker-containerd-jail-pid-handler",
+		Fn: func(ctx context.Context, m *firecracker.Machine) error {
+			pid, err := m.PID()
+			if err != nil {
+				return err
+			}
+			j.pid = pid
+			return nil
+		},
+	}
+
 	j.logger.Debug("noop operation for BuildJailedMachine")
 	return []firecracker.Opt{
 		firecracker.WithProcessRunner(cmd),
+		func(m *firecracker.Machine) {
+			m.Handlers.FcInit = m.Handlers.FcInit.Append(pidHandler)
+		},
 	}, nil
 }
 
-func (j noopJailer) JailPath() vm.Dir {
+func (j *noopJailer) JailPath() vm.Dir {
 	j.logger.Debug("noop operation returning shim dir for JailPath")
 	return j.shimDir
 }
 
-func (j noopJailer) ExposeFileToJail(path string) error {
+func (j *noopJailer) ExposeFileToJail(path string) error {
 	j.logger.Debug("noop operation for ExposeFileToJail")
 	return nil
 }
 
-func (j noopJailer) StubDrivesOptions() []FileOpt {
+func (j *noopJailer) StubDrivesOptions() []FileOpt {
 	j.logger.Debug("noop operation for StubDrivesOptions")
 	return []FileOpt{}
 }
 
-func (j noopJailer) Close() error { return nil }
+func (j *noopJailer) Stop() error {
+	if j.pid == 0 {
+		return errors.New("the machine hasn't been started")
+	}
+
+	j.logger.Debugf("sending SIGTERM to %d", j.pid)
+	p, err := os.FindProcess(j.pid)
+	if err != nil {
+		return err
+	}
+
+	err = p.Signal(syscall.SIGTERM)
+	if err == nil || err.Error() == "os: process already finished" {
+		return nil
+	}
+	return err
+}
+
+func (j *noopJailer) Close() error { return nil }

runtime/runc_jailer.go

@@ -501,3 +501,7 @@ func getNetNS(spec specs.Spec) string {
 
 	return ""
 }
+
+func (j runcJailer) Stop() error {
+	return j.runcClient.Kill(j.ctx, j.vmID, int(syscall.SIGTERM), &runc.KillOpts{All: true})
+}

runtime/service.go

@@ -74,6 +74,7 @@ const (
 	firecrackerStartTimeout = 5 * time.Second
 	defaultStopVMTimeout    = 5 * time.Second
 	defaultShutdownTimeout  = 5 * time.Second
+	jailerStopTimeout       = 3 * time.Second
 
 	// StartEventName is the topic published to when a VM starts
 	StartEventName = "/firecracker-vm/start"
@@ -1171,54 +1172,80 @@ func (s *service) Shutdown(requestCtx context.Context, req *taskAPI.ShutdownRequ
 	return &ptypes.Empty{}, nil
 }
 
-// shutdown will stop the VMM within the provided timeout. It attempts to shutdown gracefully by having
-// agent stop (which is presumed to cause the VM to begin a reboot) and then waiting for the VMM process
-// to exit (via the s.shimCtx.Done() channel). If that fails, StopVMM will be called to force a shutdown (currently
-// via sending SIGTERM). If that fails, the VMM will still be killed via SIGKILL when the shimCtx is canceled.
 func (s *service) shutdown(
 	requestCtx context.Context,
 	timeout time.Duration,
 	req *taskAPI.ShutdownRequest,
 ) error {
-	shutdownCtx, cancel := context.WithTimeout(requestCtx, timeout)
-	defer cancel()
+	s.logger.Info("stopping the VM")
+
 	go func() {
-		// Once the shutdown procedure is done, the shim needs to shutdown too.
-		// This also ensures that if the VMM is still alive, it will receive a
-		// SIGKILL via exec.CommandContext
-		<-shutdownCtx.Done()
-		s.shimCancel()
+		s.shutdownLoop(requestCtx, timeout, req)
 	}()
 
-	s.logger.Info("stopping the VM")
-
-	// Try to tell agent to exit, causing the VM to begin a reboot. If that
-	// fails, try to forcibly stop the VMM. If that too fails, just cancel
-	// the shutdownCtx to fast-path to the VMM getting SIGKILL.
-	_, shutdownErr := s.agentClient.Shutdown(shutdownCtx, req)
-	if shutdownErr != nil {
-		s.logger.WithError(shutdownErr).Error("failed to shutdown VM agent")
-		stopVMMErr := s.machine.StopVMM()
-		if stopVMMErr != nil {
-			s.logger.WithError(stopVMMErr).Error("failed to forcibly stop VMM")
-			cancel()
-		}
+	err := s.machine.Wait(context.Background())
+	if err == nil {
+		return nil
 	}
+	return status.Error(codes.Internal, fmt.Sprintf("the VMM was killed forcibly: %v", err))
+}
 
-	// wait for the shimCtx to be done, which means the VM has exited and we're ready
-	// to shutdown
-	<-s.shimCtx.Done()
-	if shutdownCtx.Err() == context.DeadlineExceeded {
-		return status.Error(codes.DeadlineExceeded,
-			"timed out waiting for VM shutdown, VMM was sent SIGKILL")
-	}
+// shutdownLoop sends multiple different shutdown requests to stop the VMM.
+// 1) send a request to the in-VM agent, which is presumed to cause the VM to begin a reboot.
+// 2) stop the VM through jailer#Stop(). The signal should be visible from the VMM (e.g. SIGTERM)
+// 3) stop the VM through cancelling the associated context. The signal would not be visible from the VMM (e.g. SIGKILL)
+func (s *service) shutdownLoop(
+	requestCtx context.Context,
+	timeout time.Duration,
+	req *taskAPI.ShutdownRequest,
+) {
+	actions := []struct {
+		name     string
+		shutdown func() error
+		timeout  time.Duration
+	}{
+		{
+			name: "send a request to the in-VM agent",
+			shutdown: func() error {
+				_, err := s.agentClient.Shutdown(requestCtx, req)
+				if err != nil {
+					return err
+				}
+				return nil
+			},
+			timeout: timeout,
+		},
+		{
+			name: "stop the jailer",
+			shutdown: func() error {
+				return s.jailer.Stop()
+			},
+			timeout: jailerStopTimeout,
+		},
+		{
+			name: "cancel the context",
+			shutdown: func() error {
+				s.shimCancel()
+				return nil
+			},
+		},
+	}
+
+	for _, action := range actions {
+		pid, err := s.machine.PID()
+		if pid == 0 && err != nil {
+			break // we have nothing to kill
+		}
 
-	if s.vmExitErr != nil {
-		return status.Error(codes.Internal,
-			fmt.Sprintf("VMM exit errors: %v", s.vmExitErr))
+		s.logger.Debug(action.name)
+		err = action.shutdown()
+		if err != nil {
+			// if sending an request doesn't succeed, don't wait and carry on.
+			s.logger.WithError(err).Errorf("failed to %s", action.name)
+		} else {
+			time.Sleep(action.timeout)
+		}
 	}
-
-	return nil
 }
 
 func (s *service) Stats(requestCtx context.Context, req *taskAPI.StatsRequest) (*taskAPI.StatsResponse, error) {