Make loading container in a sandbox more robust

PiperOrigin-RevId: 262071646

3 files changed, 116 insertions, 25 deletions

diff --git a/runsc/container/container.go b/runsc/container/container.go
index 27e9c2e0f..2a8453931 100644
--- a/runsc/container/container.go
+++ b/runsc/container/container.go
@@ -138,6 +138,34 @@ type Container struct {
 	RootContainerDir string
 }
 
+// loadSandbox loads all containers that belong to the sandbox with the given
+// ID.
+func loadSandbox(rootDir, id string) ([]*Container, error) {
+	cids, err := List(rootDir)
+	if err != nil {
+		return nil, err
+	}
+
+	// Load the container metadata.
+	var containers []*Container
+	for _, cid := range cids {
+		container, err := Load(rootDir, cid)
+		if err != nil {
+			// Container file may not exist if it raced with creation/deletion or
+			// directory was left behind. Load provides a snapshot in time, so it's
+			// fine to skip it.
+			if os.IsNotExist(err) {
+				continue
+			}
+			return nil, fmt.Errorf("loading container %q: %v", id, err)
+		}
+		if container.Sandbox.ID == id {
+			containers = append(containers, container)
+		}
+	}
+	return containers, nil
+}
+
 // Load loads a container with the given id from a metadata file. id may be an
 // abbreviation of the full container id, in which case Load loads the
 // container to which id unambiguously refers to.
@@ -180,7 +208,7 @@ func Load(rootDir, id string) (*Container, error) {
 	// If the status is "Running" or "Created", check that the sandbox
 	// process still exists, and set it to Stopped if it does not.
 	//
-	// This is inherently racey.
+	// This is inherently racy.
 	if c.Status == Running || c.Status == Created {
 		// Check if the sandbox process is still running.
 		if !c.isSandboxRunning() {
@@ -237,7 +265,13 @@ func List(rootDir string) ([]string, error) {
 	}
 	var out []string
 	for _, f := range fs {
-		out = append(out, f.Name())
+		// Filter out directories that do no belong to a container.
+		cid := f.Name()
+		if validateID(cid) == nil {
+			if _, err := os.Stat(filepath.Join(rootDir, cid, metadataFilename)); err == nil {
+				out = append(out, f.Name())
+			}
+		}
 	}
 	return out, nil
 }
@@ -1108,6 +1142,10 @@ func runInCgroup(cg *cgroup.Cgroup, fn func() error) error {
 // adjustOOMScoreAdj sets the oom_score_adj for the sandbox and all gofers.
 // oom_score_adj is set to the lowest oom_score_adj among the containers
 // running in the sandbox.
+//
+// TODO(gvisor.dev/issue/512): This call could race with other containers being
+// created at the same time and end up setting the wrong oom_score_adj to the
+// sandbox.
 func (c *Container) adjustOOMScoreAdj(conf *boot.Config) error {
 	// If this container's OOMScoreAdj is nil then we can exit early as no
 	// change should be made to oom_score_adj for the sandbox.
@@ -1115,21 +1153,9 @@ func (c *Container) adjustOOMScoreAdj(conf *boot.Config) error {
 		return nil
 	}
 
-	ids, err := List(conf.RootDir)
+	containers, err := loadSandbox(conf.RootDir, c.Sandbox.ID)
 	if err != nil {
-		return err
-	}
-
-	// Load the container metadata.
-	var containers []*Container
-	for _, id := range ids {
-		container, err := Load(conf.RootDir, id)
-		if err != nil {
-			return fmt.Errorf("loading container %q: %v", id, err)
-		}
-		if container.Sandbox.ID == c.Sandbox.ID {
-			containers = append(containers, container)
-		}
+		return fmt.Errorf("loading sandbox containers: %v", err)
 	}
 
 	// Get the lowest score for all containers.
diff --git a/runsc/container/multi_container_test.go b/runsc/container/multi_container_test.go
index 2ef065a15..2d51fecc6 100644
--- a/runsc/container/multi_container_test.go
+++ b/runsc/container/multi_container_test.go
@@ -60,11 +60,14 @@ func createSpecs(cmds ...[]string) ([]*specs.Spec, []string) {
 }
 
 func startContainers(conf *boot.Config, specs []*specs.Spec, ids []string) ([]*Container, func(), error) {
-	rootDir, err := testutil.SetupRootDir()
-	if err != nil {
-		return nil, nil, fmt.Errorf("error creating root dir: %v", err)
+	// Setup root dir if one hasn't been provided.
+	if len(conf.RootDir) == 0 {
+		rootDir, err := testutil.SetupRootDir()
+		if err != nil {
+			return nil, nil, fmt.Errorf("error creating root dir: %v", err)
+		}
+		conf.RootDir = rootDir
 	}
-	conf.RootDir = rootDir
 
 	var containers []*Container
 	var bundles []string
@@ -75,7 +78,7 @@ func startContainers(conf *boot.Config, specs []*specs.Spec, ids []string) ([]*C
 		for _, b := range bundles {
 			os.RemoveAll(b)
 		}
-		os.RemoveAll(rootDir)
+		os.RemoveAll(conf.RootDir)
 	}
 	for i, spec := range specs {
 		bundleDir, err := testutil.SetupBundleDir(spec)
@@ -1423,3 +1426,62 @@ func TestMultiContainerGoferKilled(t *testing.T) {
 		}
 	}
 }
+
+func TestMultiContainerLoadSandbox(t *testing.T) {
+	sleep := []string{"sleep", "100"}
+	specs, ids := createSpecs(sleep, sleep, sleep)
+	conf := testutil.TestConfig()
+
+	// Create containers for the sandbox.
+	wants, cleanup, err := startContainers(conf, specs, ids)
+	if err != nil {
+		t.Fatalf("error starting containers: %v", err)
+	}
+	defer cleanup()
+
+	// Then create unrelated containers.
+	for i := 0; i < 3; i++ {
+		specs, ids = createSpecs(sleep, sleep, sleep)
+		_, cleanup, err = startContainers(conf, specs, ids)
+		if err != nil {
+			t.Fatalf("error starting containers: %v", err)
+		}
+		defer cleanup()
+	}
+
+	// Create an unrelated directory under root.
+	dir := filepath.Join(conf.RootDir, "not-a-container")
+	if err := os.MkdirAll(dir, 0755); err != nil {
+		t.Fatalf("os.MkdirAll(%q)=%v", dir, err)
+	}
+
+	// Create a valid but empty container directory.
+	randomCID := testutil.UniqueContainerID()
+	dir = filepath.Join(conf.RootDir, randomCID)
+	if err := os.MkdirAll(dir, 0755); err != nil {
+		t.Fatalf("os.MkdirAll(%q)=%v", dir, err)
+	}
+
+	// Load the sandbox and check that the correct containers were returned.
+	id := wants[0].Sandbox.ID
+	gots, err := loadSandbox(conf.RootDir, id)
+	if err != nil {
+		t.Fatalf("loadSandbox()=%v", err)
+	}
+	wantIDs := make(map[string]struct{})
+	for _, want := range wants {
+		wantIDs[want.ID] = struct{}{}
+	}
+	for _, got := range gots {
+		if got.Sandbox.ID != id {
+			t.Errorf("wrong sandbox ID, got: %v, want: %v", got.Sandbox.ID, id)
+		}
+		if _, ok := wantIDs[got.ID]; !ok {
+			t.Errorf("wrong container ID, got: %v, wants: %v", got.ID, wantIDs)
+		}
+		delete(wantIDs, got.ID)
+	}
+	if len(wantIDs) != 0 {
+		t.Errorf("containers not found: %v", wantIDs)
+	}
+}
diff --git a/runsc/test/testutil/testutil.go b/runsc/test/testutil/testutil.go
index 406f6d08a..e288c7758 100644
--- a/runsc/test/testutil/testutil.go
+++ b/runsc/test/testutil/testutil.go
@@ -189,11 +189,14 @@ func SetupRootDir() (string, error) {
 // SetupContainer creates a bundle and root dir for the container, generates a
 // test config, and writes the spec to config.json in the bundle dir.
 func SetupContainer(spec *specs.Spec, conf *boot.Config) (rootDir, bundleDir string, err error) {
-	rootDir, err = SetupRootDir()
-	if err != nil {
-		return "", "", err
+	// Setup root dir if one hasn't been provided.
+	if len(conf.RootDir) == 0 {
+		rootDir, err = SetupRootDir()
+		if err != nil {
+			return "", "", err
+		}
+		conf.RootDir = rootDir
 	}
-	conf.RootDir = rootDir
 	bundleDir, err = SetupBundleDir(spec)
 	return rootDir, bundleDir, err
 }