Make runsc kill and delete more conformant to the "spec"

PiperOrigin-RevId: 214976251
Change-Id: I631348c3886f41f63d0e77e7c4f21b3ede2ab521

3 files changed, 125 insertions, 16 deletions

diff --git a/runsc/container/container.go b/runsc/container/container.go
index 44b7dad8a..e65800b8d 100644
--- a/runsc/container/container.go
+++ b/runsc/container/container.go
@@ -72,6 +72,21 @@ func validateID(id string) error {
 // Containers must write their metadata files after any change to their internal
 // states. The entire container directory is deleted when the container is
 // destroyed.
+//
+// When the container is stopped, all processes that belong to the container
+// must be stopped before Destroy() returns. containerd makes roughly the
+// following calls to stop a container:
+//   - First it attempts to kill the container process with
+//     'runsc kill SIGTERM'. After some time, it escalates to SIGKILL. In a
+//     separate thread, it's waiting on the container. As soon as the wait
+//     returns, it moves on to the next step:
+//   - It calls 'runsc kill --all SIGKILL' to stop every process that belongs to
+//     the container. 'kill --all SIGKILL' waits for all processes before
+//     returning.
+//   - Containerd waits for stdin, stdout and stderr to drain and be closed.
+//   - It calls 'runsc delete'. runc implementation kills --all SIGKILL once
+//     again just to be sure, waits, and then proceeds with remaining teardown.
+//
 type Container struct {
 	// ID is the container ID.
 	ID string `json:"id"`
@@ -451,7 +466,8 @@ func (c *Container) WaitPID(pid int32, clearStatus bool) (syscall.WaitStatus, er
 	return c.Sandbox.WaitPID(c.ID, pid, clearStatus)
 }
 
-// Signal sends the signal to the container.
+// Signal sends the signal to the container. If all is true and signal is
+// SIGKILL, then waits for all processes to exit before returning.
 // Signal returns an error if the container is already stopped.
 // TODO: Distinguish different error types.
 func (c *Container) Signal(sig syscall.Signal, all bool) error {
@@ -534,8 +550,8 @@ func (c *Container) Processes() ([]*control.Process, error) {
 	return c.Sandbox.Processes(c.ID)
 }
 
-// Destroy frees all resources associated with the container. It fails fast and
-// is idempotent.
+// Destroy stops all processes and frees all resources associated with the
+// container. It fails fast and is idempotent.
 func (c *Container) Destroy() error {
 	log.Debugf("Destroy container %q", c.ID)
 
diff --git a/runsc/container/multi_container_test.go b/runsc/container/multi_container_test.go
index 8c98bed22..3d7385a82 100644
--- a/runsc/container/multi_container_test.go
+++ b/runsc/container/multi_container_test.go
@@ -25,6 +25,7 @@ import (
 	"sync"
 	"syscall"
 	"testing"
+	"time"
 
 	specs "github.com/opencontainers/runtime-spec/specs-go"
 	"gvisor.googlesource.com/gvisor/pkg/sentry/control"
@@ -403,12 +404,18 @@ func TestMultiContainerSignal(t *testing.T) {
 // TestMultiContainerDestroy checks that container are properly cleaned-up when
 // they are destroyed.
 func TestMultiContainerDestroy(t *testing.T) {
+	app, err := testutil.FindFile("runsc/container/test_app")
+	if err != nil {
+		t.Fatal("error finding test_app:", err)
+	}
+
 	for _, conf := range configs(all...) {
 		t.Logf("Running test with conf: %+v", conf)
 
-		// Two containers that will run for a long time. We will
-		// destroy the second one.
-		specs, ids := createSpecs([]string{"sleep", "100"}, []string{"sleep", "100"})
+		// First container will remain intact while the second container is killed.
+		specs, ids := createSpecs(
+			[]string{app, "reaper"},
+			[]string{app, "fork-bomb"})
 		containers, cleanup, err := startContainers(conf, specs, ids)
 		if err != nil {
 			t.Fatalf("error starting containers: %v", err)
@@ -416,26 +423,48 @@ func TestMultiContainerDestroy(t *testing.T) {
 		defer cleanup()
 
 		// Exec in the root container to check for the existence of the
-		// second containers root filesystem directory.
+		// second container's root filesystem directory.
 		contDir := path.Join(boot.ChildContainersDir, containers[1].ID)
-		args := &control.ExecArgs{
+		dirArgs := &control.ExecArgs{
 			Filename: "/usr/bin/test",
 			Argv:     []string{"test", "-d", contDir},
 		}
-		if ws, err := containers[0].executeSync(args); err != nil {
-			t.Fatalf("error executing %+v: %v", args, err)
+		if ws, err := containers[0].executeSync(dirArgs); err != nil {
+			t.Fatalf("error executing %+v: %v", dirArgs, err)
 		} else if ws.ExitStatus() != 0 {
 			t.Errorf("exec 'test -f %q' got exit status %d, wanted 0", contDir, ws.ExitStatus())
 		}
 
-		// Destory the second container.
+		// Exec more processes to ensure signal all works for exec'd processes too.
+		args := &control.ExecArgs{
+			Filename: app,
+			Argv:     []string{app, "fork-bomb"},
+		}
+		if _, err := containers[1].Execute(args); err != nil {
+			t.Fatalf("error exec'ing: %v", err)
+		}
+
+		// Let it brew...
+		time.Sleep(500 * time.Millisecond)
+
 		if err := containers[1].Destroy(); err != nil {
 			t.Fatalf("error destroying container: %v", err)
 		}
 
+		// Check that destroy killed all processes belonging to the container and
+		// waited for them to exit before returning.
+		pss, err := containers[0].Sandbox.Processes("")
+		if err != nil {
+			t.Fatalf("error getting process data from sandbox: %v", err)
+		}
+		expectedPL := []*control.Process{{PID: 1, Cmd: "test_app"}}
+		if !procListsEqual(pss, expectedPL) {
+			t.Errorf("container got process list: %s, want: %s", procListToString(pss), procListToString(expectedPL))
+		}
+
 		// Now the container dir should be gone.
-		if ws, err := containers[0].executeSync(args); err != nil {
-			t.Fatalf("error executing %+v: %v", args, err)
+		if ws, err := containers[0].executeSync(dirArgs); err != nil {
+			t.Fatalf("error executing %+v: %v", dirArgs, err)
 		} else if ws.ExitStatus() == 0 {
 			t.Errorf("exec 'test -f %q' got exit status 0, wanted non-zero", contDir)
 		}
diff --git a/runsc/container/test_app.go b/runsc/container/test_app.go
index a99eb97c4..f69cfdf83 100644
--- a/runsc/container/test_app.go
+++ b/runsc/container/test_app.go
@@ -36,6 +36,8 @@ func main() {
 	subcommands.Register(subcommands.FlagsCommand(), "")
 	subcommands.Register(new(uds), "")
 	subcommands.Register(new(taskTree), "")
+	subcommands.Register(new(forkBomb), "")
+	subcommands.Register(new(reaper), "")
 
 	flag.Parse()
 
@@ -151,9 +153,7 @@ func (c *taskTree) Execute(ctx context.Context, f *flag.FlagSet, args ...interfa
 
 	if c.depth == 0 {
 		log.Printf("Child sleeping, PID: %d\n", os.Getpid())
-		for {
-			time.Sleep(24 * time.Hour)
-		}
+		select {}
 	}
 	log.Printf("Parent %d sleeping, PID: %d\n", c.depth, os.Getpid())
 
@@ -177,3 +177,67 @@ func (c *taskTree) Execute(ctx context.Context, f *flag.FlagSet, args ...interfa
 	}
 	return subcommands.ExitSuccess
 }
+
+type forkBomb struct {
+	delay time.Duration
+}
+
+// Name implements subcommands.Command.
+func (*forkBomb) Name() string {
+	return "fork-bomb"
+}
+
+// Synopsis implements subcommands.Command.
+func (*forkBomb) Synopsis() string {
+	return "creates child process until the end of times"
+}
+
+// Usage implements subcommands.Command.
+func (*forkBomb) Usage() string {
+	return "fork-bomb <flags>"
+}
+
+// SetFlags implements subcommands.Command.
+func (c *forkBomb) SetFlags(f *flag.FlagSet) {
+	f.DurationVar(&c.delay, "delay", 100*time.Millisecond, "amount of time to delay creation of child")
+}
+
+// Execute implements subcommands.Command.
+func (c *forkBomb) Execute(ctx context.Context, f *flag.FlagSet, args ...interface{}) subcommands.ExitStatus {
+	time.Sleep(c.delay)
+
+	cmd := exec.Command("/proc/self/exe", c.Name())
+	cmd.Stdout = os.Stdout
+	cmd.Stderr = os.Stderr
+	if err := cmd.Run(); err != nil {
+		log.Fatal("failed to call self:", err)
+	}
+	return subcommands.ExitSuccess
+}
+
+type reaper struct{}
+
+// Name implements subcommands.Command.
+func (*reaper) Name() string {
+	return "reaper"
+}
+
+// Synopsis implements subcommands.Command.
+func (*reaper) Synopsis() string {
+	return "reaps all children in a loop"
+}
+
+// Usage implements subcommands.Command.
+func (*reaper) Usage() string {
+	return "reaper <flags>"
+}
+
+// SetFlags implements subcommands.Command.
+func (*reaper) SetFlags(*flag.FlagSet) {}
+
+// Execute implements subcommands.Command.
+func (c *reaper) Execute(ctx context.Context, f *flag.FlagSet, args ...interface{}) subcommands.ExitStatus {
+	stop := testutil.StartReaper()
+	defer stop()
+	select {}
+}