Merge pull request #571 from Microsoft/shim_mitigations

jterry75 · web-flow · commit 672e52e9209d · 2019-04-17T14:10:21.000-07:00
Handle Process/Container not found errors and force exits
diff --git a/cmd/containerd-shim-runhcs-v1/exec_hcs.go b/cmd/containerd-shim-runhcs-v1/exec_hcs.go
@@ -426,9 +426,28 @@ func (he *hcsExec) Kill(ctx context.Context, signal uint32) error {
 			err = he.p.Kill()
 		}
 		if err != nil {
-			if hcs.IsNotExist(err) {
+			if hcs.IsNotExist(err) && signal == 0x9 || hcs.IsOperationInvalidState(err) {
+				// If we issued a SIGKILL (or terminate) and get ERROR_NOT_FOUND
+				// or ERROR_VMCOMPUTE_INVALID_STATE `he.waitForExit` is either:
+				//
+				// 1. About to transition the state when it is signaled by the
+				// HCS and everything is fine. This was just a simple race where
+				// the SIGKILL came in before the previous signal completed.
+				//
+				// OR
+				//
+				// 2. We are stuck in `he.waitForExit` and the notification is
+				// not going to be delivered. In this case we force the exit by
+				// closing `he.p` and unblocking all waiters.
+				go func() {
+					// Give the HCS 1 second to finish and deliver the notification.
+					time.Sleep(1 * time.Second)
+					// Force the close. This is safe to call if `he.waitForExit` already called it.
+					he.p.Close()
+				}()
 				return errors.Wrapf(errdefs.ErrNotFound, "exec: '%s' in task: '%s' not found", he.id, he.tid)
 			}
+			// Unknown. Return the err from Signal/Kill
 			return err
 		}
 		return nil
diff --git a/cmd/containerd-shim-runhcs-v1/task_hcs.go b/cmd/containerd-shim-runhcs-v1/task_hcs.go
@@ -349,6 +349,20 @@ func (ht *hcsTask) KillExec(ctx context.Context, eid string, signal uint32, all
 			return errors.Wrap(errdefs.ErrFailedPrecondition, "cannot signal init exec with un-exited additional exec's")
 		}
 	}
+	if signal == 0x9 && eid == "" && ht.ownsHost && ht.host != nil {
+		go func() {
+			// The caller has issued a SIGKILL to the init process that owns the
+			// host.
+			//
+			// To mitigate failures that can cause the HCS to never deliver the
+			// exit notification give everything 30 seconds and terminate the
+			// UVM to force all exits.
+			time.Sleep(30 * time.Second)
+			// Safe to call multiple times if called previously on successful
+			// shutdown.
+			ht.host.Close()
+		}()
+	}
 	eg.Go(func() error {
 		return e.Kill(ctx, signal)
 	})
@@ -506,7 +520,7 @@ func (ht *hcsTask) close() {
 		if ht.c != nil {
 			// Do our best attempt to tear down the container.
 			if err := ht.c.Shutdown(); err != nil {
-				if hcs.IsAlreadyClosed(err) || hcs.IsAlreadyStopped(err) {
+				if hcs.IsAlreadyClosed(err) || hcs.IsNotExist(err) || hcs.IsAlreadyStopped(err) {
 					// This is the state we want. Do nothing.
 				} else if !hcs.IsPending(err) {
 					logrus.WithFields(logrus.Fields{
@@ -523,7 +537,7 @@ func (ht *hcsTask) close() {
 					}
 				}
 				if err := ht.c.Terminate(); err != nil {
-					if hcs.IsAlreadyClosed(err) || hcs.IsAlreadyStopped(err) {
+					if hcs.IsAlreadyClosed(err) || hcs.IsNotExist(err) || hcs.IsAlreadyStopped(err) {
 						// This is the state we want. Do nothing.
 					} else if !hcs.IsPending(err) {
 						logrus.WithFields(logrus.Fields{
diff --git a/internal/hcs/errors.go b/internal/hcs/errors.go
@@ -272,6 +272,13 @@ func IsNotSupported(err error) bool {
 		err == ErrVmcomputeUnknownMessage
 }
 
+// IsOperationInvalidState returns true when err is caused by
+// `ErrVmcomputeOperationInvalidState`.
+func IsOperationInvalidState(err error) bool {
+	err = getInnerError(err)
+	return err == ErrVmcomputeOperationInvalidState
+}
+
 func getInnerError(err error) error {
 	switch pe := err.(type) {
 	case nil:
diff --git a/internal/hcs/process.go b/internal/hcs/process.go
@@ -192,7 +192,7 @@ func (process *Process) Wait() (err error) {
 
 	<-process.waitBlock
 	if process.waitError != nil {
-		return makeProcessError(process, operation, err, nil)
+		return makeProcessError(process, operation, process.waitError, nil)
 	}
 	return nil
 }

Original file line number	Diff line number	Diff line change
`@@ -192,7 +192,7 @@ func (process *Process) Wait() (err error) {`
`192`	`192`
`193`	`193`	`<-process.waitBlock`
`194`	`194`	`if process.waitError != nil {`
`195`		`- return makeProcessError(process, operation, err, nil)`
	`195`	`+ return makeProcessError(process, operation, process.waitError, nil)`
`196`	`196`	`}`
`197`	`197`	`return nil`
`198`	`198`	`}`