Merge pull request #2672 from amscanne:shim-integrated

PiperOrigin-RevId: 321053634

4 files changed, 304 insertions, 161 deletions

diff --git a/test/packetimpact/testbench/BUILD b/test/packetimpact/testbench/BUILD
index d19ec07d4..5a0ee1367 100644
--- a/test/packetimpact/testbench/BUILD
+++ b/test/packetimpact/testbench/BUILD
@@ -23,8 +23,8 @@ go_library(
         "//pkg/usermem",
         "//test/packetimpact/netdevs",
         "//test/packetimpact/proto:posix_server_go_proto",
-        "@com_github_google_go-cmp//cmp:go_default_library",
-        "@com_github_google_go-cmp//cmp/cmpopts:go_default_library",
+        "@com_github_google_go_cmp//cmp:go_default_library",
+        "@com_github_google_go_cmp//cmp/cmpopts:go_default_library",
         "@com_github_mohae_deepcopy//:go_default_library",
         "@org_golang_google_grpc//:go_default_library",
         "@org_golang_google_grpc//keepalive:go_default_library",
diff --git a/test/root/BUILD b/test/root/BUILD
index a9e91ccd6..a9130b34f 100644
--- a/test/root/BUILD
+++ b/test/root/BUILD
@@ -41,7 +41,7 @@ go_test(
         "//runsc/container",
         "//runsc/specutils",
         "@com_github_cenkalti_backoff//:go_default_library",
-        "@com_github_opencontainers_runtime-spec//specs-go:go_default_library",
+        "@com_github_opencontainers_runtime_spec//specs-go:go_default_library",
         "@com_github_syndtr_gocapability//capability:go_default_library",
         "@org_golang_x_sys//unix:go_default_library",
     ],
@@ -51,8 +51,5 @@ vm_test(
     name = "root_vm_test",
     size = "large",
     shard_count = 1,
-    targets = [
-        "//tools/installers:shim",
-        ":root_test",
-    ],
+    targets = [":root_test"],
 )
diff --git a/test/root/crictl_test.go b/test/root/crictl_test.go
index 732fae821..193705ab8 100644
--- a/test/root/crictl_test.go
+++ b/test/root/crictl_test.go
@@ -20,13 +20,14 @@ import (
 	"fmt"
 	"io"
 	"io/ioutil"
-	"log"
 	"net/http"
 	"os"
 	"os/exec"
 	"path"
-	"path/filepath"
+	"regexp"
+	"strconv"
 	"strings"
+	"sync"
 	"testing"
 	"time"
 
@@ -75,6 +76,8 @@ func SimpleSpec(name, image string, cmd []string, extra map[string]interface{})
 		// Log files are not deleted after root tests are run. Log to random
 		// paths to ensure logs are fresh.
 		"log_path": fmt.Sprintf("%s.log", testutil.RandomID(name)),
+		"stdin":    false,
+		"tty":      false,
 	}
 	if len(cmd) > 0 { // Omit if empty.
 		s["command"] = cmd
@@ -95,25 +98,29 @@ var Httpd = SimpleSpec("httpd", "basic/httpd", nil, nil)
 
 // TestCrictlSanity refers to b/112433158.
 func TestCrictlSanity(t *testing.T) {
-	// Setup containerd and crictl.
-	crictl, cleanup, err := setup(t)
-	if err != nil {
-		t.Fatalf("failed to setup crictl: %v", err)
-	}
-	defer cleanup()
-	podID, contID, err := crictl.StartPodAndContainer("basic/httpd", Sandbox("default"), Httpd)
-	if err != nil {
-		t.Fatalf("start failed: %v", err)
-	}
-
-	// Look for the httpd page.
-	if err = httpGet(crictl, podID, "index.html"); err != nil {
-		t.Fatalf("failed to get page: %v", err)
-	}
-
-	// Stop everything.
-	if err := crictl.StopPodAndContainer(podID, contID); err != nil {
-		t.Fatalf("stop failed: %v", err)
+	for _, version := range allVersions {
+		t.Run(version, func(t *testing.T) {
+			// Setup containerd and crictl.
+			crictl, cleanup, err := setup(t, version)
+			if err != nil {
+				t.Fatalf("failed to setup crictl: %v", err)
+			}
+			defer cleanup()
+			podID, contID, err := crictl.StartPodAndContainer(containerdRuntime, "basic/httpd", Sandbox("default"), Httpd)
+			if err != nil {
+				t.Fatalf("start failed: %v", err)
+			}
+
+			// Look for the httpd page.
+			if err = httpGet(crictl, podID, "index.html"); err != nil {
+				t.Fatalf("failed to get page: %v", err)
+			}
+
+			// Stop everything.
+			if err := crictl.StopPodAndContainer(podID, contID); err != nil {
+				t.Fatalf("stop failed: %v", err)
+			}
+		})
 	}
 }
 
@@ -147,146 +154,179 @@ var HttpdMountPaths = SimpleSpec("httpd", "basic/httpd", nil, map[string]interfa
 
 // TestMountPaths refers to b/117635704.
 func TestMountPaths(t *testing.T) {
-	// Setup containerd and crictl.
-	crictl, cleanup, err := setup(t)
-	if err != nil {
-		t.Fatalf("failed to setup crictl: %v", err)
-	}
-	defer cleanup()
-	podID, contID, err := crictl.StartPodAndContainer("basic/httpd", Sandbox("default"), HttpdMountPaths)
-	if err != nil {
-		t.Fatalf("start failed: %v", err)
-	}
-
-	// Look for the directory available at /test.
-	if err = httpGet(crictl, podID, "test"); err != nil {
-		t.Fatalf("failed to get page: %v", err)
-	}
-
-	// Stop everything.
-	if err := crictl.StopPodAndContainer(podID, contID); err != nil {
-		t.Fatalf("stop failed: %v", err)
+	for _, version := range allVersions {
+		t.Run(version, func(t *testing.T) {
+			// Setup containerd and crictl.
+			crictl, cleanup, err := setup(t, version)
+			if err != nil {
+				t.Fatalf("failed to setup crictl: %v", err)
+			}
+			defer cleanup()
+			podID, contID, err := crictl.StartPodAndContainer(containerdRuntime, "basic/httpd", Sandbox("default"), HttpdMountPaths)
+			if err != nil {
+				t.Fatalf("start failed: %v", err)
+			}
+
+			// Look for the directory available at /test.
+			if err = httpGet(crictl, podID, "test"); err != nil {
+				t.Fatalf("failed to get page: %v", err)
+			}
+
+			// Stop everything.
+			if err := crictl.StopPodAndContainer(podID, contID); err != nil {
+				t.Fatalf("stop failed: %v", err)
+			}
+		})
 	}
 }
 
 // TestMountPaths refers to b/118728671.
 func TestMountOverSymlinks(t *testing.T) {
-	// Setup containerd and crictl.
-	crictl, cleanup, err := setup(t)
-	if err != nil {
-		t.Fatalf("failed to setup crictl: %v", err)
-	}
-	defer cleanup()
-
-	spec := SimpleSpec("busybox", "basic/resolv", []string{"sleep", "1000"}, nil)
-	podID, contID, err := crictl.StartPodAndContainer("basic/resolv", Sandbox("default"), spec)
-	if err != nil {
-		t.Fatalf("start failed: %v", err)
-	}
-
-	out, err := crictl.Exec(contID, "readlink", "/etc/resolv.conf")
-	if err != nil {
-		t.Fatalf("readlink failed: %v, out: %s", err, out)
-	}
-	if want := "/tmp/resolv.conf"; !strings.Contains(string(out), want) {
-		t.Fatalf("/etc/resolv.conf is not pointing to %q: %q", want, string(out))
-	}
-
-	etc, err := crictl.Exec(contID, "cat", "/etc/resolv.conf")
-	if err != nil {
-		t.Fatalf("cat failed: %v, out: %s", err, etc)
-	}
-	tmp, err := crictl.Exec(contID, "cat", "/tmp/resolv.conf")
-	if err != nil {
-		t.Fatalf("cat failed: %v, out: %s", err, out)
-	}
-	if tmp != etc {
-		t.Fatalf("file content doesn't match:\n\t/etc/resolv.conf: %s\n\t/tmp/resolv.conf: %s", string(etc), string(tmp))
-	}
-
-	// Stop everything.
-	if err := crictl.StopPodAndContainer(podID, contID); err != nil {
-		t.Fatalf("stop failed: %v", err)
+	for _, version := range allVersions {
+		t.Run(version, func(t *testing.T) {
+			// Setup containerd and crictl.
+			crictl, cleanup, err := setup(t, version)
+			if err != nil {
+				t.Fatalf("failed to setup crictl: %v", err)
+			}
+			defer cleanup()
+
+			spec := SimpleSpec("busybox", "basic/resolv", []string{"sleep", "1000"}, nil)
+			podID, contID, err := crictl.StartPodAndContainer(containerdRuntime, "basic/resolv", Sandbox("default"), spec)
+			if err != nil {
+				t.Fatalf("start failed: %v", err)
+			}
+
+			out, err := crictl.Exec(contID, "readlink", "/etc/resolv.conf")
+			if err != nil {
+				t.Fatalf("readlink failed: %v, out: %s", err, out)
+			}
+			if want := "/tmp/resolv.conf"; !strings.Contains(string(out), want) {
+				t.Fatalf("/etc/resolv.conf is not pointing to %q: %q", want, string(out))
+			}
+
+			etc, err := crictl.Exec(contID, "cat", "/etc/resolv.conf")
+			if err != nil {
+				t.Fatalf("cat failed: %v, out: %s", err, etc)
+			}
+			tmp, err := crictl.Exec(contID, "cat", "/tmp/resolv.conf")
+			if err != nil {
+				t.Fatalf("cat failed: %v, out: %s", err, out)
+			}
+			if tmp != etc {
+				t.Fatalf("file content doesn't match:\n\t/etc/resolv.conf: %s\n\t/tmp/resolv.conf: %s", string(etc), string(tmp))
+			}
+
+			// Stop everything.
+			if err := crictl.StopPodAndContainer(podID, contID); err != nil {
+				t.Fatalf("stop failed: %v", err)
+			}
+		})
 	}
 }
 
 // TestHomeDir tests that the HOME environment variable is set for
 // Pod containers.
 func TestHomeDir(t *testing.T) {
-	// Setup containerd and crictl.
-	crictl, cleanup, err := setup(t)
-	if err != nil {
-		t.Fatalf("failed to setup crictl: %v", err)
+	for _, version := range allVersions {
+		t.Run(version, func(t *testing.T) {
+			// Setup containerd and crictl.
+			crictl, cleanup, err := setup(t, version)
+			if err != nil {
+				t.Fatalf("failed to setup crictl: %v", err)
+			}
+			defer cleanup()
+
+			// Note that container ID returned here is a sub-container. All Pod
+			// containers are sub-containers. The root container of the sandbox is the
+			// pause container.
+			t.Run("sub-container", func(t *testing.T) {
+				contSpec := SimpleSpec("subcontainer", "basic/busybox", []string{"sh", "-c", "echo $HOME"}, nil)
+				podID, contID, err := crictl.StartPodAndContainer(containerdRuntime, "basic/busybox", Sandbox("subcont-sandbox"), contSpec)
+				if err != nil {
+					t.Fatalf("start failed: %v", err)
+				}
+
+				out, err := crictl.Logs(contID)
+				if err != nil {
+					t.Fatalf("failed retrieving container logs: %v, out: %s", err, out)
+				}
+				if got, want := strings.TrimSpace(string(out)), "/root"; got != want {
+					t.Fatalf("Home directory invalid. Got %q, Want : %q", got, want)
+				}
+
+				// Stop everything; note that the pod may have already stopped.
+				crictl.StopPodAndContainer(podID, contID)
+			})
+
+			// Tests that HOME is set for the exec process.
+			t.Run("exec", func(t *testing.T) {
+				contSpec := SimpleSpec("exec", "basic/busybox", []string{"sleep", "1000"}, nil)
+				podID, contID, err := crictl.StartPodAndContainer(containerdRuntime, "basic/busybox", Sandbox("exec-sandbox"), contSpec)
+				if err != nil {
+					t.Fatalf("start failed: %v", err)
+				}
+
+				out, err := crictl.Exec(contID, "sh", "-c", "echo $HOME")
+				if err != nil {
+					t.Fatalf("failed retrieving container logs: %v, out: %s", err, out)
+				}
+				if got, want := strings.TrimSpace(string(out)), "/root"; got != want {
+					t.Fatalf("Home directory invalid. Got %q, Want : %q", got, want)
+				}
+
+				// Stop everything.
+				if err := crictl.StopPodAndContainer(podID, contID); err != nil {
+					t.Fatalf("stop failed: %v", err)
+				}
+			})
+		})
 	}
-	defer cleanup()
-
-	// Note that container ID returned here is a sub-container. All Pod
-	// containers are sub-containers. The root container of the sandbox is the
-	// pause container.
-	t.Run("sub-container", func(t *testing.T) {
-		contSpec := SimpleSpec("subcontainer", "basic/busybox", []string{"sh", "-c", "echo $HOME"}, nil)
-		podID, contID, err := crictl.StartPodAndContainer("basic/busybox", Sandbox("subcont-sandbox"), contSpec)
-		if err != nil {
-			t.Fatalf("start failed: %v", err)
-		}
-
-		out, err := crictl.Logs(contID)
-		if err != nil {
-			t.Fatalf("failed retrieving container logs: %v, out: %s", err, out)
-		}
-		if got, want := strings.TrimSpace(string(out)), "/root"; got != want {
-			t.Fatalf("Home directory invalid. Got %q, Want : %q", got, want)
-		}
-
-		// Stop everything.
-		if err := crictl.StopPodAndContainer(podID, contID); err != nil {
-			t.Fatalf("stop failed: %v", err)
-		}
-	})
-
-	// Tests that HOME is set for the exec process.
-	t.Run("exec", func(t *testing.T) {
-		contSpec := SimpleSpec("exec", "basic/busybox", []string{"sleep", "1000"}, nil)
-		podID, contID, err := crictl.StartPodAndContainer("basic/busybox", Sandbox("exec-sandbox"), contSpec)
-		if err != nil {
-			t.Fatalf("start failed: %v", err)
-		}
-
-		out, err := crictl.Exec(contID, "sh", "-c", "echo $HOME")
-		if err != nil {
-			t.Fatalf("failed retrieving container logs: %v, out: %s", err, out)
-		}
-		if got, want := strings.TrimSpace(string(out)), "/root"; got != want {
-			t.Fatalf("Home directory invalid. Got %q, Want : %q", got, want)
-		}
-
-		// Stop everything.
-		if err := crictl.StopPodAndContainer(podID, contID); err != nil {
-			t.Fatalf("stop failed: %v", err)
-		}
-	})
 }
 
-// containerdConfigTemplate is a .toml config for containerd. It contains a
-// formatting verb so the runtime field can be set via fmt.Sprintf.
-const containerdConfigTemplate = `
+const containerdRuntime = "runsc"
+
+const v1Template = `
 disabled_plugins = ["restart"]
+[plugins.cri]
+  disable_tcp_service = true
 [plugins.linux]
-  runtime = "%s"
-  runtime_root = "/tmp/test-containerd/runsc"
-  shim = "/usr/local/bin/gvisor-containerd-shim"
+  shim = "%s"
   shim_debug = true
-
-[plugins.cri.containerd.runtimes.runsc]
+[plugins.cri.containerd.runtimes.` + containerdRuntime + `]
   runtime_type = "io.containerd.runtime.v1.linux"
   runtime_engine = "%s"
+  runtime_root = "%s/root/runsc"
 `
 
+const v2Template = `
+disabled_plugins = ["restart"]
+[plugins.cri]
+  disable_tcp_service = true
+[plugins.linux]
+  shim_debug = true
+[plugins.cri.containerd.runtimes.` + containerdRuntime + `]
+  runtime_type = "io.containerd.` + containerdRuntime + `.v1"
+[plugins.cri.containerd.runtimes.` + containerdRuntime + `.options]
+  TypeUrl = "io.containerd.` + containerdRuntime + `.v1.options"
+`
+
+const (
+	// v1 is the containerd API v1.
+	v1 string = "v1"
+
+	// v1 is the containerd API v21.
+	v2 string = "v2"
+)
+
+// allVersions is the set of known versions.
+var allVersions = []string{v1, v2}
+
 // setup sets up before a test. Specifically it:
 // * Creates directories and a socket for containerd to utilize.
 // * Runs containerd and waits for it to reach a "ready" state for testing.
 // * Returns a cleanup function that should be called at the end of the test.
-func setup(t *testing.T) (*criutil.Crictl, func(), error) {
+func setup(t *testing.T, version string) (*criutil.Crictl, func(), error) {
 	// Create temporary containerd root and state directories, and a socket
 	// via which crictl and containerd communicate.
 	containerdRoot, err := ioutil.TempDir(testutil.TmpDir(), "containerd-root")
@@ -295,13 +335,43 @@ func setup(t *testing.T) (*criutil.Crictl, func(), error) {
 	}
 	cu := cleanup.Make(func() { os.RemoveAll(containerdRoot) })
 	defer cu.Clean()
+	t.Logf("Using containerd root: %s", containerdRoot)
 
 	containerdState, err := ioutil.TempDir(testutil.TmpDir(), "containerd-state")
 	if err != nil {
 		t.Fatalf("failed to create containerd state: %v", err)
 	}
 	cu.Add(func() { os.RemoveAll(containerdState) })
-	sockAddr := filepath.Join(testutil.TmpDir(), "containerd-test.sock")
+	t.Logf("Using containerd state: %s", containerdState)
+
+	sockDir, err := ioutil.TempDir(testutil.TmpDir(), "containerd-sock")
+	if err != nil {
+		t.Fatalf("failed to create containerd socket directory: %v", err)
+	}
+	cu.Add(func() { os.RemoveAll(sockDir) })
+	sockAddr := path.Join(sockDir, "test.sock")
+	t.Logf("Using containerd socket: %s", sockAddr)
+
+	// Extract the containerd version.
+	versionCmd := exec.Command(getContainerd(), "-v")
+	out, err := versionCmd.CombinedOutput()
+	if err != nil {
+		t.Fatalf("error extracting containerd version: %v (%s)", err, string(out))
+	}
+	r := regexp.MustCompile(" v([0-9]+)\\.([0-9]+)\\.([0-9+])")
+	vs := r.FindStringSubmatch(string(out))
+	if len(vs) != 4 {
+		t.Fatalf("error unexpected version string: %s", string(out))
+	}
+	major, err := strconv.ParseUint(vs[1], 10, 64)
+	if err != nil {
+		t.Fatalf("error parsing containerd major version: %v (%s)", err, string(out))
+	}
+	minor, err := strconv.ParseUint(vs[2], 10, 64)
+	if err != nil {
+		t.Fatalf("error parsing containerd minor version: %v (%s)", err, string(out))
+	}
+	t.Logf("Using containerd version: %d.%d", major, minor)
 
 	// We rewrite a configuration. This is based on the current docker
 	// configuration for the runtime under test.
@@ -309,28 +379,100 @@ func setup(t *testing.T) (*criutil.Crictl, func(), error) {
 	if err != nil {
 		t.Fatalf("error discovering runtime path: %v", err)
 	}
-	config, configCleanup, err := testutil.WriteTmpFile("containerd-config", fmt.Sprintf(containerdConfigTemplate, runtime, runtime))
+	t.Logf("Using runtime: %v", runtime)
+
+	// Construct a PATH that includes the runtime directory. This is
+	// because the shims will be installed there, and containerd may infer
+	// the binary name and search the PATH.
+	runtimeDir := path.Dir(runtime)
+	modifiedPath := os.Getenv("PATH")
+	if modifiedPath != "" {
+		modifiedPath = ":" + modifiedPath // We prepend below.
+	}
+	modifiedPath = path.Dir(getContainerd()) + modifiedPath
+	modifiedPath = runtimeDir + ":" + modifiedPath
+	t.Logf("Using PATH: %v", modifiedPath)
+
+	var (
+		config   string
+		runpArgs []string
+	)
+	switch version {
+	case v1:
+		// This is only supported less than 1.3.
+		if major > 1 || (major == 1 && minor >= 3) {
+			t.Skipf("skipping unsupported containerd (want less than 1.3, got %d.%d)", major, minor)
+		}
+
+		// We provide the shim, followed by the runtime, and then a
+		// temporary root directory.  Note that we can safely assume
+		// that the shim has been installed in the same directory as
+		// the runtime (for test installs and for normal installs).
+		// Since this is v1, the binary name will be fixed.
+		config = fmt.Sprintf(v1Template, path.Join(runtimeDir, "gvisor-containerd-shim"), runtime, runtimeDir)
+	case v2:
+		// This is only supported past 1.2.
+		if major < 1 || (major == 1 && minor <= 1) {
+			t.Skipf("skipping incompatible containerd (want at least 1.2, got %d.%d)", major, minor)
+		}
+
+		// The runtime is provided via parameter. Note that the v2 shim
+		// binary name is always containerd-shim-* so we don't actually
+		// care about the docker runtime name.
+		config = v2Template
+	default:
+		t.Fatalf("unknown version: %d", version)
+	}
+	t.Logf("Using config: %s", config)
+
+	// Generate the configuration for the test.
+	configFile, configCleanup, err := testutil.WriteTmpFile("containerd-config", config)
 	if err != nil {
 		t.Fatalf("failed to write containerd config")
 	}
 	cu.Add(configCleanup)
 
 	// Start containerd.
-	cmd := exec.Command(getContainerd(),
-		"--config", config,
+	args := []string{
+		getContainerd(),
+		"--config", configFile,
 		"--log-level", "debug",
 		"--root", containerdRoot,
 		"--state", containerdState,
-		"--address", sockAddr)
+		"--address", sockAddr,
+	}
+	t.Logf("Using args: %s", strings.Join(args, " "))
+	cmd := exec.Command(args[0], args[1:]...)
+	cmd.Env = append(os.Environ(), "PATH="+modifiedPath)
+
+	// Include output in logs.
+	stderrPipe, err := cmd.StderrPipe()
+	if err != nil {
+		t.Fatalf("failed to create stderr pipe: %v", err)
+	}
+	cu.Add(func() { stderrPipe.Close() })
+	stdoutPipe, err := cmd.StdoutPipe()
+	if err != nil {
+		t.Fatalf("failed to create stdout pipe: %v", err)
+	}
+	cu.Add(func() { stdoutPipe.Close() })
+	var (
+		wg     sync.WaitGroup
+		stderr bytes.Buffer
+		stdout bytes.Buffer
+	)
 	startupR, startupW := io.Pipe()
-	defer startupR.Close()
-	defer startupW.Close()
-	stderr := &bytes.Buffer{}
-	stdout := &bytes.Buffer{}
-	cmd.Stderr = io.MultiWriter(startupW, stderr)
-	cmd.Stdout = io.MultiWriter(startupW, stdout)
+	wg.Add(2)
+	go func() {
+		defer wg.Done()
+		io.Copy(io.MultiWriter(startupW, &stderr), stderrPipe)
+	}()
+	go func() {
+		defer wg.Done()
+		io.Copy(io.MultiWriter(startupW, &stdout), stdoutPipe)
+	}()
 	cu.Add(func() {
-		// Log output in case of failure.
+		wg.Wait()
 		t.Logf("containerd stdout: %s", stdout.String())
 		t.Logf("containerd stderr: %s", stderr.String())
 	})
@@ -345,13 +487,17 @@ func setup(t *testing.T) (*criutil.Crictl, func(), error) {
 		t.Fatalf("failed to start containerd: %v", err)
 	}
 
+	// Discard all subsequent data.
+	go io.Copy(ioutil.Discard, startupR)
+
+	// Create the crictl interface.
+	cc := criutil.NewCrictl(t, sockAddr, runpArgs)
+	cu.Add(cc.CleanUp)
+
 	// Kill must be the last cleanup (as it will be executed first).
-	cc := criutil.NewCrictl(t, sockAddr)
 	cu.Add(func() {
-		cc.CleanUp() // Remove tmp files, etc.
-		if err := testutil.KillCommand(cmd); err != nil {
-			log.Printf("error killing containerd: %v", err)
-		}
+		// Best effort: ignore errors.
+		testutil.KillCommand(cmd)
 	})
 
 	return cc, cu.Release(), nil
diff --git a/test/runner/BUILD b/test/runner/BUILD
index 6833c9986..1f45a6922 100644
--- a/test/runner/BUILD
+++ b/test/runner/BUILD
@@ -16,7 +16,7 @@ go_binary(
         "//runsc/specutils",
         "//test/runner/gtest",
         "//test/uds",
-        "@com_github_opencontainers_runtime-spec//specs-go:go_default_library",
+        "@com_github_opencontainers_runtime_spec//specs-go:go_default_library",
         "@org_golang_x_sys//unix:go_default_library",
     ],
 )