Return failure when `runsc events` queries a stopped container

This was causing gvisor-containerd-shim to crash because the command
suceeded, but there was no stat present.

PiperOrigin-RevId: 340964921

5 files changed, 95 insertions, 4 deletions

diff --git a/pkg/shim/runsc/BUILD b/pkg/shim/runsc/BUILD
index f08599ebd..cb0001852 100644
--- a/pkg/shim/runsc/BUILD
+++ b/pkg/shim/runsc/BUILD
@@ -10,6 +10,7 @@ go_library(
     ],
     visibility = ["//:sandbox"],
     deps = [
+        "@com_github_containerd_containerd//log:go_default_library",
         "@com_github_containerd_go_runc//:go_default_library",
         "@com_github_opencontainers_runtime_spec//specs-go:go_default_library",
     ],
diff --git a/pkg/shim/runsc/runsc.go b/pkg/shim/runsc/runsc.go
index c5cf68efa..e7c9640ba 100644
--- a/pkg/shim/runsc/runsc.go
+++ b/pkg/shim/runsc/runsc.go
@@ -28,10 +28,12 @@ import (
 	"syscall"
 	"time"
 
+	"github.com/containerd/containerd/log"
 	runc "github.com/containerd/go-runc"
 	specs "github.com/opencontainers/runtime-spec/specs-go"
 )
 
+// Monitor is the default process monitor to be used by runsc.
 var Monitor runc.ProcessMonitor = runc.Monitor
 
 // DefaultCommand is the default command for Runsc.
@@ -74,6 +76,7 @@ func (r *Runsc) State(context context.Context, id string) (*runc.Container, erro
 	return &c, nil
 }
 
+// CreateOpts is a set of options to Runsc.Create().
 type CreateOpts struct {
 	runc.IO
 	ConsoleSocket runc.ConsoleSocket
@@ -197,6 +200,7 @@ func (r *Runsc) Wait(context context.Context, id string) (int, error) {
 	return res.ExitStatus, nil
 }
 
+// ExecOpts is a set of options to runsc.Exec().
 type ExecOpts struct {
 	runc.IO
 	PidFile         string
@@ -301,6 +305,7 @@ func (r *Runsc) Run(context context.Context, id, bundle string, opts *CreateOpts
 	return Monitor.Wait(cmd, ec)
 }
 
+// DeleteOpts is a set of options to runsc.Delete().
 type DeleteOpts struct {
 	Force bool
 }
@@ -367,6 +372,13 @@ func (r *Runsc) Stats(context context.Context, id string) (*runc.Stats, error) {
 	if err := json.NewDecoder(rd).Decode(&e); err != nil {
 		return nil, err
 	}
+	log.L.Debugf("Stats returned: %+v", e.Stats)
+	if e.Type != "stats" {
+		return nil, fmt.Errorf(`unexpected event type %q, wanted "stats"`, e.Type)
+	}
+	if e.Stats == nil {
+		return nil, fmt.Errorf(`"runsc events -stat" succeeded but no stat was provided`)
+	}
 	return e.Stats, nil
 }
 
diff --git a/runsc/cmd/events.go b/runsc/cmd/events.go
index 5211ad4ce..3836b7b4e 100644
--- a/runsc/cmd/events.go
+++ b/runsc/cmd/events.go
@@ -85,7 +85,12 @@ func (evs *Events) Execute(ctx context.Context, f *flag.FlagSet, args ...interfa
 		ev, err := c.Event()
 		if err != nil {
 			log.Warningf("Error getting events for container: %v", err)
+			if evs.stats {
+				return subcommands.ExitFailure
+			}
 		}
+		log.Debugf("Events: %+v", ev)
+
 		// err must be preserved because it is used below when breaking
 		// out of the loop.
 		b, err := json.Marshal(ev)
@@ -101,11 +106,9 @@ func (evs *Events) Execute(ctx context.Context, f *flag.FlagSet, args ...interfa
 			if err != nil {
 				return subcommands.ExitFailure
 			}
-			break
+			return subcommands.ExitSuccess
 		}
 
 		time.Sleep(time.Duration(evs.intervalSec) * time.Second)
 	}
-
-	return subcommands.ExitSuccess
 }
diff --git a/runsc/container/container.go b/runsc/container/container.go
index 435d866f5..4aa139c88 100644
--- a/runsc/container/container.go
+++ b/runsc/container/container.go
@@ -587,7 +587,12 @@ func (c *Container) SandboxPid() int {
 // and wait returns immediately.
 func (c *Container) Wait() (syscall.WaitStatus, error) {
 	log.Debugf("Wait on container, cid: %s", c.ID)
-	return c.Sandbox.Wait(c.ID)
+	ws, err := c.Sandbox.Wait(c.ID)
+	if err == nil {
+		// Wait succeeded, container is not running anymore.
+		c.changeStatus(Stopped)
+	}
+	return ws, err
 }
 
 // WaitRootPID waits for process 'pid' in the sandbox's PID namespace and
diff --git a/runsc/container/multi_container_test.go b/runsc/container/multi_container_test.go
index 8f77c2371..cadc63bf3 100644
--- a/runsc/container/multi_container_test.go
+++ b/runsc/container/multi_container_test.go
@@ -15,6 +15,7 @@
 package container
 
 import (
+	"encoding/json"
 	"fmt"
 	"io/ioutil"
 	"math"
@@ -1766,3 +1767,72 @@ func TestMultiContainerHomeEnvDir(t *testing.T) {
 		})
 	}
 }
+
+func TestMultiContainerEvent(t *testing.T) {
+	conf := testutil.TestConfig(t)
+	rootDir, cleanup, err := testutil.SetupRootDir()
+	if err != nil {
+		t.Fatalf("error creating root dir: %v", err)
+	}
+	defer cleanup()
+	conf.RootDir = rootDir
+
+	// Setup the containers.
+	sleep := []string{"/bin/sleep", "100"}
+	quick := []string{"/bin/true"}
+	podSpec, ids := createSpecs(sleep, sleep, quick)
+	containers, cleanup, err := startContainers(conf, podSpec, ids)
+	if err != nil {
+		t.Fatalf("error starting containers: %v", err)
+	}
+	defer cleanup()
+
+	for _, cont := range containers {
+		t.Logf("Running containerd %s", cont.ID)
+	}
+
+	// Wait for last container to stabilize the process count that is checked
+	// further below.
+	if ws, err := containers[2].Wait(); err != nil || ws != 0 {
+		t.Fatalf("Container.Wait, status: %v, err: %v", ws, err)
+	}
+
+	// Check events for running containers.
+	for _, cont := range containers[:2] {
+		evt, err := cont.Event()
+		if err != nil {
+			t.Errorf("Container.Events(): %v", err)
+		}
+		if want := "stats"; evt.Type != want {
+			t.Errorf("Wrong event type, want: %s, got :%s", want, evt.Type)
+		}
+		if cont.ID != evt.ID {
+			t.Errorf("Wrong container ID, want: %s, got :%s", cont.ID, evt.ID)
+		}
+		// Event.Data is an interface, so it comes from the wire was
+		// map[string]string. Marshal and unmarshall again to the correc type.
+		data, err := json.Marshal(evt.Data)
+		if err != nil {
+			t.Fatalf("invalid event data: %v", err)
+		}
+		var stats boot.Stats
+		if err := json.Unmarshal(data, &stats); err != nil {
+			t.Fatalf("invalid event data: %v", err)
+		}
+		// One process per remaining container.
+		if want := uint64(2); stats.Pids.Current != want {
+			t.Errorf("Wrong number of PIDs, want: %d, got :%d", want, stats.Pids.Current)
+		}
+	}
+
+	// Check that stop and destroyed containers return error.
+	if err := containers[1].Destroy(); err != nil {
+		t.Fatalf("container.Destroy: %v", err)
+	}
+	for _, cont := range containers[1:] {
+		_, err := cont.Event()
+		if err == nil {
+			t.Errorf("Container.Events() should have failed, cid:%s, state: %v", cont.ID, cont.Status)
+		}
+	}
+}