Restore runc kill all behavior for init processes

PR #2538 removed the `runc kill --all` flag, when signaling containers.
However, when cleaning up after the init process exists, the `--all`
flag is still needed to remove any potentially orphaned processes when
using runc before v1.2.

See: opencontainers/runc@f8ad20f#diff-ade6035c3e554d7627cdc368b27f475fc0dad83e02382a1dea9cae9b75871087

Additionally, switch to using error strings directly from runc code in
`internal\guest\runtime\runc\utils.go`: they have been available since
runc v1.1.0-rc.1.

See: opencontainers/runc#3033

Also, add logic to match on container not/still running error strings
and return them for `Kill`, since returning
`ERROR_VMCOMPUTE_SYSTEM_ALREADY_STOPPED` (`0xc0370110`) when killing a
stopped container is expected behavior and handled appropriately in
`"cmd/containerd-shim-runhcs-v1".(*hcsExec).Kill()`.

Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>

Author: helsaawy

cmd/containerd-shim-runhcs-v1/task_hcs.go

@@ -446,7 +446,7 @@ func (ht *hcsTask) KillExec(ctx context.Context, eid string, signal uint32, all
 	if signal == 0x9 && eid == "" && ht.host != nil {
 		// If this is a SIGKILL against the init process we start a background
 		// timer and wait on either the timer expiring or the process exiting
-		// cleanly. If the timer exires first we forcibly close the UVM as we
+		// cleanly. If the timer expires first we forcibly close the UVM as we
 		// assume the guest is misbehaving for some reason.
 		go func() {
 			t := time.NewTimer(30 * time.Second)

internal/bridgeutils/gcserr/errors.go

@@ -10,6 +10,7 @@ import (
 // Hresult is a type corresponding to the HRESULT error type used on Windows.
 type Hresult int32
 
+// ! Must match error values in internal\hcs\errors.go
 // from
 // - https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-erref/705fb797-2175-4a90-b5a3-3918024b10b8
 // - https://docs.microsoft.com/en-us/virtualization/api/hcs/reference/hcshresult
@@ -51,7 +52,7 @@ const (
 	//
 	// The virtual machine or container with the specified identifier is not
 	// running.
-	HrVmcomputeSystemAlreadyStopped = Hresult(-2143878896) // 0x80370110
+	HrVmcomputeSystemAlreadyStopped = Hresult(-1070137072) // 0xC0370110
 )
 
 // TODO: update implementation to use go1.13 style errors with `errors.As` and co.

internal/guest/runtime/hcsv2/container.go

@@ -214,7 +214,10 @@ func (c *Container) GetAllProcessPids(ctx context.Context) ([]int, error) {
 
 // Kill sends 'signal' to the container process.
 func (c *Container) Kill(ctx context.Context, signal syscall.Signal) error {
-	log.G(ctx).WithField(logfields.ContainerID, c.id).Info("opengcs::Container::Kill")
+	log.G(ctx).WithFields(logrus.Fields{
+		logfields.ContainerID: c.id,
+		"signal":              signal.String(),
+	}).Info("opengcs::Container::Kill")
 	err := c.container.Kill(signal)
 	if err != nil {
 		return err

internal/guest/runtime/runc/container.go

@@ -76,14 +76,40 @@ func (c *container) ExecProcess(process *oci.Process, stdioSet *stdio.Connection
 
 // Kill sends the specified signal to the container's init process.
 func (c *container) Kill(signal syscall.Signal) error {
-	logrus.WithField(logfields.ContainerID, c.id).Debug("runc::container::Kill")
+	logrus.WithFields(logrus.Fields{
+		logfields.ContainerID: c.id,
+		"signal":              signal.String(),
+	}).Debug("runc::container::Kill")
+	return c.kill(signal, false)
+}
+
+// killAll terminates all processes started in the container.
+//
+// Note: [runc deprecated] the `kill --all` flag starting in v1.2, but, prior to that, it was required
+// to kill all processes within the container after the init exits.
+// Until we can guarantee that the runc version is greater than 1.1 and runc explicitly removes the option,
+// keep using it here.
+// This mirrors how upstream containerd's runc handles [init exit] via [kill all].
+//
+// [runc deprecated]: https://github.com/opencontainers/runc/pull/3825
+// [init exit]: https://github.com/containerd/containerd/blob/48baa31a0ad1ca1121ddaf968d3b8aa68c40bf84/cmd/containerd-shim-runc-v2/task/service.go#L725
+// [kill all]: https://github.com/containerd/containerd/blob/48baa31a0ad1ca1121ddaf968d3b8aa68c40bf84/cmd/containerd-shim-runc-v2/process/init.go#L375
+func (c *container) killAll() error {
+	logrus.WithField(logfields.ContainerID, c.id).Debug("runc::container::killAll")
+	return c.kill(syscall.SIGKILL, true)
+}
+
+func (c *container) kill(signal syscall.Signal, all bool) error {
 	args := []string{"kill"}
+	if all {
+		args = append(args, "--all")
+	}
 	args = append(args, c.id, strconv.Itoa(int(signal)))
 	cmd := runcCommand(args...)
 	out, err := cmd.CombinedOutput()
 	if err != nil {
 		runcErr := parseRuncError(string(out))
-		return errors.Wrapf(runcErr, "unknown runc error after kill %v: %s", err, string(out))
+		return errors.Wrapf(runcErr, "runc kill failed with %v: %s", err, string(out))
 	}
 	return nil
 }

internal/guest/runtime/runc/process.go

@@ -4,8 +4,6 @@
 package runc
 
 import (
-	"syscall"
-
 	"github.com/Microsoft/hcsshim/internal/guest/runtime"
 	"github.com/Microsoft/hcsshim/internal/guest/stdio"
 	"github.com/Microsoft/hcsshim/internal/logfields"
@@ -65,7 +63,7 @@ func (p *process) Wait() (int, error) {
 		// If the init process of a pid namespace terminates, the kernel
 		// terminates all other processes in the namespace with SIGKILL. We
 		// simulate the same behavior.
-		if err := p.c.Kill(syscall.SIGKILL); err != nil {
+		if err := p.c.killAll(); err != nil {
 			l.WithError(err).Error("failed to terminate container after process wait")
 		}
 	}

internal/guest/runtime/runc/utils.go

@@ -12,6 +12,7 @@ import (
 	"strings"
 	"syscall"
 
+	"github.com/opencontainers/runc/libcontainer"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 
@@ -124,20 +125,19 @@ func (l *standardLogEntry) asError() (err error) {
 }
 
 func parseRuncError(s string) (err error) {
-	// TODO (helsaawy): match with errors from
-	// https://github.com/opencontainers/runc/blob/master/libcontainer/error.go
 	if strings.HasPrefix(s, "container") && strings.HasSuffix(s, "does not exist") {
-		// currently: "container <container id> does not exist"
+		// match "container %q does not exist" and [libcontainer.ErrNotExist]
 		err = runtime.ErrContainerDoesNotExist
-	} else if strings.Contains(s, "container with id exists") ||
-		strings.Contains(s, "container with given ID already exists") {
+	} else if strings.Contains(s, "container with id exists") || strings.Contains(s, libcontainer.ErrExist.Error()) {
 		err = runtime.ErrContainerAlreadyExists
-	} else if strings.Contains(s, "invalid id format") ||
-		strings.Contains(s, "invalid container ID format") {
+	} else if strings.Contains(s, "invalid id format") || strings.Contains(s, libcontainer.ErrInvalidID.Error()) {
 		err = runtime.ErrInvalidContainerID
-	} else if strings.Contains(s, "container") &&
-		strings.Contains(s, "that is not stopped") {
+	} else if strings.Contains(s, "container") && strings.Contains(s, "that is not stopped") {
 		err = runtime.ErrContainerNotStopped
+	} else if strings.Contains(s, libcontainer.ErrRunning.Error()) {
+		err = runtime.ErrContainerStillRunning
+	} else if strings.Contains(s, libcontainer.ErrNotRunning.Error()) {
+		err = runtime.ErrContainerNotRunning
 	} else {
 		err = errors.New(s)
 	}