Merge pull request #52 from Random-Liu/apply-container-image-config

Apply container image config

cmd/cri-containerd/cri_containerd.go

@@ -48,6 +48,9 @@ func main() {
 	if err != nil {
 		glog.Exitf("Failed to create CRI containerd service %+v: %v", o, err)
 	}
+	if err := service.Start(); err != nil {
+		glog.Exitf("Failed to start CRI containerd service: %v", err)
+	}
 
 	s := server.NewCRIContainerdServer(o.SocketPath, service, service)
 	if err := s.Run(); err != nil {

pkg/server/container_create.go

@@ -20,9 +20,10 @@ import (
 	"fmt"
 	"time"
 
+	rootfsapi "github.com/containerd/containerd/api/services/rootfs"
 	"github.com/golang/glog"
+	imagedigest "github.com/opencontainers/go-digest"
 	"golang.org/x/net/context"
-
 	runtime "k8s.io/kubernetes/pkg/kubelet/apis/cri/v1alpha1"
 
 	"github.com/kubernetes-incubator/cri-containerd/pkg/metadata"
@@ -68,9 +69,26 @@ func (c *criContainerdService) CreateContainer(ctx context.Context, r *runtime.C
 		Config:    config,
 	}
 
-	// TODO(random-liu): [P0] Prepare container rootfs.
+	// Prepare container image snapshot. For container, the image should have
-
+	// been pulled before creating the container, so do not ensure the image.
-	// TODO(random-liu): [P0] Set ImageRef in ContainerMetadata with image id.
+	image := config.GetImage().GetImage()
+	imageMeta, err := c.localResolve(ctx, image)
+	if err != nil {
+		return nil, fmt.Errorf("failed to resolve image %q: %v", image, err)
+	}
+	if imageMeta == nil {
+		return nil, fmt.Errorf("image %q not found", image)
+	}
+	if _, err := c.rootfsService.Prepare(ctx, &rootfsapi.PrepareRequest{
+		Name: id,
+		// We are sure that ChainID must be a digest.
+		ChainID:  imagedigest.Digest(imageMeta.ChainID),
+		Readonly: config.GetLinux().GetSecurityContext().GetReadonlyRootfs(),
+	}); err != nil {
+		return nil, fmt.Errorf("failed to prepare container rootfs %q: %v", imageMeta.ChainID, err)
+	}
+	// TODO(random-liu): [P0] Cleanup snapshot on failure after switching to new rootfs api.
+	meta.ImageRef = imageMeta.ID
 
 	// Create container root directory.
 	containerRootDir := getContainerRootDir(c.rootDir, id)

pkg/server/container_create_test.go

@@ -21,6 +21,9 @@ import (
 	"os"
 	"testing"
 
+	rootfsapi "github.com/containerd/containerd/api/services/rootfs"
+	imagedigest "github.com/opencontainers/go-digest"
+	imagespec "github.com/opencontainers/image-spec/specs-go/v1"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"golang.org/x/net/context"
@@ -28,6 +31,7 @@ import (
 
 	"github.com/kubernetes-incubator/cri-containerd/pkg/metadata"
 	ostesting "github.com/kubernetes-incubator/cri-containerd/pkg/os/testing"
+	servertesting "github.com/kubernetes-incubator/cri-containerd/pkg/server/testing"
 )
 
 func TestCreateContainer(t *testing.T) {
@@ -42,10 +46,20 @@ func TestCreateContainer(t *testing.T) {
 		Namespace: "test-sandbox-namespace",
 		Attempt:   2,
 	}
+	// Use an image id to avoid image name resolution.
+	// TODO(random-liu): Change this to image name after we have complete image
+	// management unit test framework.
+	testImage := "sha256:c75bebcdd211f41b3a460c7bf82970ed6c75acaab9cd4c9a4e125b03ca113799"
+	testChainID := imagedigest.Digest("test-chain-id")
+	testImageMetadata := metadata.ImageMetadata{
+		ID:      testImage,
+		ChainID: testChainID.String(),
+		Config:  &imagespec.ImageConfig{},
+	}
 	testConfig := &runtime.ContainerConfig{
 		Metadata: testNameMeta,
 		Image: &runtime.ImageSpec{
-			Image: "test-image",
+			Image: testImage,
 		},
 		Labels:      map[string]string{"a": "b"},
 		Annotations: map[string]string{"c": "d"},
@@ -55,12 +69,14 @@ func TestCreateContainer(t *testing.T) {
 	}
 
 	for desc, test := range map[string]struct {
-		sandboxMetadata   *metadata.SandboxMetadata
+		sandboxMetadata    *metadata.SandboxMetadata
-		reserveNameErr    bool
+		reserveNameErr     bool
-		createRootDirErr  error
+		imageMetadataErr   bool
-		createMetadataErr bool
+		prepareSnapshotErr error
-		expectErr         bool
+		createRootDirErr   error
-		expectMeta        *metadata.ContainerMetadata
+		createMetadataErr  bool
+		expectErr          bool
+		expectMeta         *metadata.ContainerMetadata
 	}{
 		"should return error if sandbox does not exist": {
 			sandboxMetadata: nil,
@@ -84,6 +100,24 @@ func TestCreateContainer(t *testing.T) {
 			createRootDirErr: errors.New("random error"),
 			expectErr:        true,
 		},
+		"should return error if image is not pulled": {
+			sandboxMetadata: &metadata.SandboxMetadata{
+				ID:     testSandboxID,
+				Name:   makeSandboxName(testSandboxNameMeta),
+				Config: testSandboxConfig,
+			},
+			imageMetadataErr: true,
+			expectErr:        true,
+		},
+		"should return error if prepare snapshot fails": {
+			sandboxMetadata: &metadata.SandboxMetadata{
+				ID:     testSandboxID,
+				Name:   makeSandboxName(testSandboxNameMeta),
+				Config: testSandboxConfig,
+			},
+			prepareSnapshotErr: errors.New("random error"),
+			expectErr:          true,
+		},
 		"should be able to create container successfully": {
 			sandboxMetadata: &metadata.SandboxMetadata{
 				ID:     testSandboxID,
@@ -94,12 +128,14 @@ func TestCreateContainer(t *testing.T) {
 			expectMeta: &metadata.ContainerMetadata{
 				Name:      makeContainerName(testNameMeta, testSandboxNameMeta),
 				SandboxID: testSandboxID,
+				ImageRef:  testImage,
 				Config:    testConfig,
 			},
 		},
 	} {
 		t.Logf("TestCase %q", desc)
 		c := newTestCRIContainerdService()
+		fakeRootfsClient := c.rootfsService.(*servertesting.FakeRootfsClient)
 		fakeOS := c.os.(*ostesting.FakeOS)
 		if test.sandboxMetadata != nil {
 			assert.NoError(t, c.sandboxStore.Create(*test.sandboxMetadata))
@@ -108,6 +144,13 @@ func TestCreateContainer(t *testing.T) {
 		if test.reserveNameErr {
 			assert.NoError(t, c.containerNameIndex.Reserve(containerName, "random id"))
 		}
+		if !test.imageMetadataErr {
+			assert.NoError(t, c.imageMetadataStore.Create(testImageMetadata))
+		}
+		if test.prepareSnapshotErr != nil {
+			fakeRootfsClient.InjectError("prepare", test.prepareSnapshotErr)
+		}
+		fakeRootfsClient.SetFakeChainIDs([]imagedigest.Digest{testChainID})
 		rootExists := false
 		rootPath := ""
 		fakeOS.MkdirAllFn = func(path string, perm os.FileMode) error {
@@ -153,5 +196,15 @@ func TestCreateContainer(t *testing.T) {
 		// TODO(random-liu): Use fake clock to test CreatedAt.
 		test.expectMeta.CreatedAt = meta.CreatedAt
 		assert.Equal(t, test.expectMeta, meta, "container metadata should be created")
+
+		assert.Equal(t, []string{"prepare"}, fakeRootfsClient.GetCalledNames(), "prepare should be called")
+		calls := fakeRootfsClient.GetCalledDetails()
+		prepareOpts := calls[0].Argument.(*rootfsapi.PrepareRequest)
+		assert.Equal(t, &rootfsapi.PrepareRequest{
+			Name:    id,
+			ChainID: testChainID,
+			// TODO(random-liu): Test readonly rootfs.
+			Readonly: false,
+		}, prepareOpts, "prepare request should be correct")
 	}
 }

pkg/server/container_remove.go

@@ -69,7 +69,7 @@ func (c *criContainerdService) RemoveContainer(ctx context.Context, r *runtime.R
 	// kubelet implementation, we'll never start a container once we decide to remove it,
 	// so we don't need the "Dead" state for now.
 
-	// TODO(random-liu): [P0] Cleanup container rootfs.
+	// TODO(random-liu): [P0] Cleanup snapshot after switching to new snapshot api.
 
 	// Cleanup container root directory.
 	containerRootDir := getContainerRootDir(c.rootDir, id)

pkg/server/container_start.go

@@ -23,16 +23,15 @@ import (
 	"os"
 	"time"
 
+	"github.com/containerd/containerd/api/services/execution"
+	rootfsapi "github.com/containerd/containerd/api/services/rootfs"
+	"github.com/containerd/containerd/api/types/container"
 	prototypes "github.com/gogo/protobuf/types"
 	"github.com/golang/glog"
+	imagespec "github.com/opencontainers/image-spec/specs-go/v1"
 	runtimespec "github.com/opencontainers/runtime-spec/specs-go"
 	"github.com/opencontainers/runtime-tools/generate"
 	"golang.org/x/net/context"
-
-	"github.com/containerd/containerd/api/services/execution"
-	"github.com/containerd/containerd/api/types/container"
-	"github.com/containerd/containerd/api/types/mount"
-
 	runtime "k8s.io/kubernetes/pkg/kubelet/apis/cri/v1alpha1"
 
 	"github.com/kubernetes-incubator/cri-containerd/pkg/metadata"
@@ -114,12 +113,11 @@ func (c *criContainerdService) startContainer(ctx context.Context, id string, me
 	glog.V(2).Infof("Sandbox container %q is running with pid %d", sandboxID, sandboxPid)
 
 	// Generate containerd container create options.
-	// TODO(random-liu): [P0] Create container rootfs with image ref.
+	imageMeta, err := c.imageMetadataStore.Get(meta.ImageRef)
-	// TODO(random-liu): [P0] Apply default image config.
+	if err != nil {
-	// Use fixed rootfs path for now.
+		return fmt.Errorf("failed to get container image %q: %v", meta.ImageRef, err)
-	const rootPath = "/"
+	}
-
+	spec, err := c.generateContainerSpec(id, sandboxPid, config, sandboxConfig, imageMeta.Config)
-	spec, err := c.generateContainerSpec(id, sandboxPid, config, sandboxConfig)
 	if err != nil {
 		return fmt.Errorf("failed to generate container %q spec: %v", id, err)
 	}
@@ -169,6 +167,12 @@ func (c *criContainerdService) startContainer(ctx context.Context, id string, me
 		}(stderrPipe)
 	}
 
+	// Get rootfs mounts.
+	mountsResp, err := c.rootfsService.Mounts(ctx, &rootfsapi.MountsRequest{Name: id})
+	if err != nil {
+		return fmt.Errorf("failed to get rootfs mounts %q: %v", id, err)
+	}
+
 	// Create containerd container.
 	createOpts := &execution.CreateRequest{
 		ID: id,
@@ -176,24 +180,14 @@ func (c *criContainerdService) startContainer(ctx context.Context, id string, me
 			TypeUrl: runtimespec.Version,
 			Value:   rawSpec,
 		},
-		// TODO(random-liu): [P0] Get rootfs mount from containerd.
+		Rootfs:   mountsResp.Mounts,
-		Rootfs: []*mount.Mount{
-			{
-				Type:   "bind",
-				Source: rootPath,
-				Options: []string{
-					"rw",
-					"rbind",
-				},
-			},
-		},
 		Runtime:  defaultRuntime,
 		Stdin:    stdin,
 		Stdout:   stdout,
 		Stderr:   stderr,
 		Terminal: config.GetTty(),
 	}
-	glog.V(2).Infof("Create containerd container (id=%q, name=%q) with options %+v.",
+	glog.V(5).Infof("Create containerd container (id=%q, name=%q) with options %+v.",
 		id, meta.Name, createOpts)
 	createResp, err := c.containerService.Create(ctx, createOpts)
 	if err != nil {
@@ -219,7 +213,8 @@ func (c *criContainerdService) startContainer(ctx context.Context, id string, me
 	return nil
 }
 
-func (c *criContainerdService) generateContainerSpec(id string, sandboxPid uint32, config *runtime.ContainerConfig, sandboxConfig *runtime.PodSandboxConfig) (*runtimespec.Spec, error) {
+func (c *criContainerdService) generateContainerSpec(id string, sandboxPid uint32, config *runtime.ContainerConfig,
+	sandboxConfig *runtime.PodSandboxConfig, imageConfig *imagespec.ImageConfig) (*runtimespec.Spec, error) {
 	// Creates a spec Generator with the default spec.
 	// TODO(random-liu): [P2] Move container runtime spec generation into a helper function.
 	g := generate.New()
@@ -228,14 +223,21 @@ func (c *criContainerdService) generateContainerSpec(id string, sandboxPid uint3
 	// pre-defined directory.
 	g.SetRootPath(relativeRootfsPath)
 
-	if len(config.GetCommand()) != 0 || len(config.GetArgs()) != 0 {
+	if err := setOCIProcessArgs(&g, config, imageConfig); err != nil {
-		g.SetProcessArgs(append(config.GetCommand(), config.GetArgs()...))
+		return nil, err
 	}
 
 	if config.GetWorkingDir() != "" {
 		g.SetProcessCwd(config.GetWorkingDir())
+	} else if imageConfig.WorkingDir != "" {
+		g.SetProcessCwd(imageConfig.WorkingDir)
 	}
 
+	// Apply envs from image config first, so that envs from container config
+	// can override them.
+	if err := addImageEnvs(&g, imageConfig.Env); err != nil {
+		return nil, err
+	}
 	for _, e := range config.GetEnvs() {
 		g.AddProcessEnv(e.GetKey(), e.GetValue())
 	}
@@ -288,6 +290,27 @@ func (c *criContainerdService) generateContainerSpec(id string, sandboxPid uint3
 	return g.Spec(), nil
 }
 
+// setOCIProcessArgs sets process args. It returns error if the final arg list
+// is empty.
+func setOCIProcessArgs(g *generate.Generator, config *runtime.ContainerConfig, imageConfig *imagespec.ImageConfig) error {
+	command, args := config.GetCommand(), config.GetArgs()
+	// The following logic is migrated from https://github.com/moby/moby/blob/master/daemon/commit.go
+	// TODO(random-liu): Clearly define the commands overwrite behavior.
+	if len(command) == 0 {
+		if len(args) == 0 {
+			args = imageConfig.Cmd
+		}
+		if command == nil {
+			command = imageConfig.Entrypoint
+		}
+	}
+	if len(command) == 0 && len(args) == 0 {
+		return fmt.Errorf("no command specified")
+	}
+	g.SetProcessArgs(append(command, args...))
+	return nil
+}
+
 // addOCIBindMounts adds bind mounts.
 func addOCIBindMounts(g *generate.Generator, mounts []*runtime.Mount) {
 	for _, mount := range mounts {

pkg/server/container_start_test.go

@@ -26,7 +26,10 @@ import (
 
 	"github.com/containerd/containerd/api/services/execution"
 	"github.com/containerd/containerd/api/types/container"
+	"github.com/containerd/containerd/api/types/mount"
+	imagespec "github.com/opencontainers/image-spec/specs-go/v1"
 	runtimespec "github.com/opencontainers/runtime-spec/specs-go"
+	"github.com/opencontainers/runtime-tools/generate"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"golang.org/x/net/context"
@@ -38,7 +41,7 @@ import (
 )
 
 func getStartContainerTestData() (*runtime.ContainerConfig, *runtime.PodSandboxConfig,
-	func(*testing.T, string, uint32, *runtimespec.Spec)) {
+	*imagespec.ImageConfig, func(*testing.T, string, uint32, *runtimespec.Spec)) {
 	config := &runtime.ContainerConfig{
 		Metadata: &runtime.ContainerMetadata{
 			Name:    "test-name",
@@ -92,11 +95,17 @@ func getStartContainerTestData() (*runtime.ContainerConfig, *runtime.PodSandboxC
 			CgroupParent: "/test/cgroup/parent",
 		},
 	}
+	imageConfig := &imagespec.ImageConfig{
+		Env:        []string{"ik1=iv1", "ik2=iv2"},
+		Entrypoint: []string{"/entrypoint"},
+		Cmd:        []string{"cmd"},
+		WorkingDir: "/workspace",
+	}
 	specCheck := func(t *testing.T, id string, sandboxPid uint32, spec *runtimespec.Spec) {
 		assert.Equal(t, relativeRootfsPath, spec.Root.Path)
 		assert.Equal(t, []string{"test", "command", "test", "args"}, spec.Process.Args)
 		assert.Equal(t, "test-cwd", spec.Process.Cwd)
-		assert.Contains(t, spec.Process.Env, "k1=v1", "k2=v2")
+		assert.Contains(t, spec.Process.Env, "k1=v1", "k2=v2", "ik1=iv1", "ik2=iv2")
 
 		t.Logf("Check bind mount")
 		found1, found2 := false, false
@@ -159,15 +168,15 @@ func getStartContainerTestData() (*runtime.ContainerConfig, *runtime.PodSandboxC
 			Path: getPIDNamespace(sandboxPid),
 		})
 	}
-	return config, sandboxConfig, specCheck
+	return config, sandboxConfig, imageConfig, specCheck
 }
 
 func TestGeneralContainerSpec(t *testing.T) {
 	testID := "test-id"
 	testPid := uint32(1234)
-	config, sandboxConfig, specCheck := getStartContainerTestData()
+	config, sandboxConfig, imageConfig, specCheck := getStartContainerTestData()
 	c := newTestCRIContainerdService()
-	spec, err := c.generateContainerSpec(testID, testPid, config, sandboxConfig)
+	spec, err := c.generateContainerSpec(testID, testPid, config, sandboxConfig, imageConfig)
 	assert.NoError(t, err)
 	specCheck(t, testID, testPid, spec)
 }
@@ -175,11 +184,11 @@ func TestGeneralContainerSpec(t *testing.T) {
 func TestContainerSpecTty(t *testing.T) {
 	testID := "test-id"
 	testPid := uint32(1234)
-	config, sandboxConfig, specCheck := getStartContainerTestData()
+	config, sandboxConfig, imageConfig, specCheck := getStartContainerTestData()
 	c := newTestCRIContainerdService()
 	for _, tty := range []bool{true, false} {
 		config.Tty = tty
-		spec, err := c.generateContainerSpec(testID, testPid, config, sandboxConfig)
+		spec, err := c.generateContainerSpec(testID, testPid, config, sandboxConfig, imageConfig)
 		assert.NoError(t, err)
 		specCheck(t, testID, testPid, spec)
 		assert.Equal(t, tty, spec.Process.Terminal)
@@ -189,27 +198,90 @@ func TestContainerSpecTty(t *testing.T) {
 func TestContainerSpecReadonlyRootfs(t *testing.T) {
 	testID := "test-id"
 	testPid := uint32(1234)
-	config, sandboxConfig, specCheck := getStartContainerTestData()
+	config, sandboxConfig, imageConfig, specCheck := getStartContainerTestData()
 	c := newTestCRIContainerdService()
 	for _, readonly := range []bool{true, false} {
 		config.Linux.SecurityContext.ReadonlyRootfs = readonly
-		spec, err := c.generateContainerSpec(testID, testPid, config, sandboxConfig)
+		spec, err := c.generateContainerSpec(testID, testPid, config, sandboxConfig, imageConfig)
 		assert.NoError(t, err)
 		specCheck(t, testID, testPid, spec)
 		assert.Equal(t, readonly, spec.Root.Readonly)
 	}
 }
 
+func TestContainerSpecCommand(t *testing.T) {
+	for desc, test := range map[string]struct {
+		criEntrypoint   []string
+		criArgs         []string
+		imageEntrypoint []string
+		imageArgs       []string
+		expected        []string
+		expectErr       bool
+	}{
+		"should use cri entrypoint if it's specified": {
+			criEntrypoint:   []string{"a", "b"},
+			imageEntrypoint: []string{"c", "d"},
+			imageArgs:       []string{"e", "f"},
+			expected:        []string{"a", "b"},
+		},
+		"should use cri entrypoint if it's specified even if it's empty": {
+			criEntrypoint:   []string{},
+			criArgs:         []string{"a", "b"},
+			imageEntrypoint: []string{"c", "d"},
+			imageArgs:       []string{"e", "f"},
+			expected:        []string{"a", "b"},
+		},
+		"should use cri entrypoint and args if they are specified": {
+			criEntrypoint:   []string{"a", "b"},
+			criArgs:         []string{"c", "d"},
+			imageEntrypoint: []string{"e", "f"},
+			imageArgs:       []string{"g", "h"},
+			expected:        []string{"a", "b", "c", "d"},
+		},
+		"should use image entrypoint if cri entrypoint is not specified": {
+			criArgs:         []string{"a", "b"},
+			imageEntrypoint: []string{"c", "d"},
+			imageArgs:       []string{"e", "f"},
+			expected:        []string{"c", "d", "a", "b"},
+		},
+		"should use image args if both cri entrypoint and args are not specified": {
+			imageEntrypoint: []string{"c", "d"},
+			imageArgs:       []string{"e", "f"},
+			expected:        []string{"c", "d", "e", "f"},
+		},
+		"should return error if both entrypoint and args are empty": {
+			expectErr: true,
+		},
+	} {
+
+		config, _, imageConfig, _ := getStartContainerTestData()
+		g := generate.New()
+		config.Command = test.criEntrypoint
+		config.Args = test.criArgs
+		imageConfig.Entrypoint = test.imageEntrypoint
+		imageConfig.Cmd = test.imageArgs
+		err := setOCIProcessArgs(&g, config, imageConfig)
+		if test.expectErr {
+			assert.Error(t, err)
+			continue
+		}
+		assert.NoError(t, err)
+		assert.Equal(t, test.expected, g.Spec().Process.Args, desc)
+	}
+}
+
 func TestStartContainer(t *testing.T) {
 	testID := "test-id"
 	testSandboxID := "test-sandbox-id"
 	testSandboxPid := uint32(4321)
-	config, sandboxConfig, specCheck := getStartContainerTestData()
+	testImageID := "sha256:c75bebcdd211f41b3a460c7bf82970ed6c75acaab9cd4c9a4e125b03ca113799"
+	config, sandboxConfig, imageConfig, specCheck := getStartContainerTestData()
 	testMetadata := &metadata.ContainerMetadata{
 		ID:        testID,
 		Name:      "test-name",
 		SandboxID: testSandboxID,
 		Config:    config,
+		ImageRef:  testImageID,
 		CreatedAt: time.Now().UnixNano(),
 	}
 	testSandboxMetadata := &metadata.SandboxMetadata{
@@ -222,10 +294,13 @@ func TestStartContainer(t *testing.T) {
 		Pid:    testSandboxPid,
 		Status: container.Status_RUNNING,
 	}
+	testMounts := []*mount.Mount{{Type: "bind", Source: "test-source"}}
 	for desc, test := range map[string]struct {
 		containerMetadata          *metadata.ContainerMetadata
 		sandboxMetadata            *metadata.SandboxMetadata
 		sandboxContainerdContainer *container.Container
+		imageMetadataErr           bool
+		snapshotMountsErr          bool
 		prepareFIFOErr             error
 		createContainerErr         error
 		startContainerErr          error
@@ -288,6 +363,24 @@ func TestStartContainer(t *testing.T) {
 			expectCalls:       []string{"info"},
 			expectErr:         true,
 		},
+		"should return error when image doesn't exist": {
+			containerMetadata:          testMetadata,
+			sandboxMetadata:            testSandboxMetadata,
+			sandboxContainerdContainer: testSandboxContainer,
+			imageMetadataErr:           true,
+			expectStateChange:          true,
+			expectCalls:                []string{"info"},
+			expectErr:                  true,
+		},
+		"should return error when snapshot mounts fails": {
+			containerMetadata:          testMetadata,
+			sandboxMetadata:            testSandboxMetadata,
+			sandboxContainerdContainer: testSandboxContainer,
+			snapshotMountsErr:          true,
+			expectStateChange:          true,
+			expectCalls:                []string{"info"},
+			expectErr:                  true,
+		},
 		"should return error when fail to open streaming pipes": {
 			containerMetadata:          testMetadata,
 			sandboxMetadata:            testSandboxMetadata,
@@ -329,6 +422,7 @@ func TestStartContainer(t *testing.T) {
 		c := newTestCRIContainerdService()
 		fake := c.containerService.(*servertesting.FakeExecutionClient)
 		fakeOS := c.os.(*ostesting.FakeOS)
+		fakeRootfsClient := c.rootfsService.(*servertesting.FakeRootfsClient)
 		if test.containerMetadata != nil {
 			assert.NoError(t, c.containerStore.Create(*test.containerMetadata))
 		}
@@ -338,6 +432,15 @@ func TestStartContainer(t *testing.T) {
 		if test.sandboxContainerdContainer != nil {
 			fake.SetFakeContainers([]container.Container{*test.sandboxContainerdContainer})
 		}
+		if !test.imageMetadataErr {
+			assert.NoError(t, c.imageMetadataStore.Create(metadata.ImageMetadata{
+				ID:     testImageID,
+				Config: imageConfig,
+			}))
+		}
+		if !test.snapshotMountsErr {
+			fakeRootfsClient.SetFakeMounts(testID, testMounts)
+		}
 		// TODO(random-liu): Test behavior with different streaming config.
 		fakeOS.OpenFifoFn = func(context.Context, string, int, os.FileMode) (io.ReadWriteCloser, error) {
 			return nopReadWriteCloser{}, test.prepareFIFOErr
@@ -394,6 +497,7 @@ func TestStartContainer(t *testing.T) {
 		calls := fake.GetCalledDetails()
 		createOpts, ok := calls[1].Argument.(*execution.CreateRequest)
 		assert.True(t, ok, "2nd call should be create")
+		assert.Equal(t, testMounts, createOpts.Rootfs, "rootfs mounts should be correct")
 		// TODO(random-liu): Test other create options.
 		spec := &runtimespec.Spec{}
 		assert.NoError(t, json.Unmarshal(createOpts.Spec.Value, spec))

pkg/server/sandbox_run.go

@@ -219,12 +219,8 @@ func (c *criContainerdService) generateSandboxContainerSpec(id string, config *r
 	g := generate.New()
 
 	// Apply default config from image config.
-	for _, e := range imageConfig.Env {
+	if err := addImageEnvs(&g, imageConfig.Env); err != nil {
-		kv := strings.Split(e, "=")
+		return nil, err
-		if len(kv) != 2 {
-			return nil, fmt.Errorf("invalid environment variable in image config %+v", imageConfig)
-		}
-		g.AddProcessEnv(kv[0], kv[1])
 	}
 
 	if imageConfig.WorkingDir != "" {
@@ -300,3 +296,16 @@ func (c *criContainerdService) generateSandboxContainerSpec(id string, config *r
 
 	return g.Spec(), nil
 }
+
+// addImageEnvs adds environment variables from image config. It returns error if
+// an invalid environment variable is encountered.
+func addImageEnvs(g *generate.Generator, imageEnvs []string) error {
+	for _, e := range imageEnvs {
+		kv := strings.Split(e, "=")
+		if len(kv) != 2 {
+			return fmt.Errorf("invalid environment variable %q", e)
+		}
+		g.AddProcessEnv(kv[0], kv[1])
+	}
+	return nil
+}

pkg/server/sandbox_run_test.go

@@ -126,6 +126,7 @@ func TestGenerateSandboxContainerSpec(t *testing.T) {
 			expectErr: true,
 		},
 		"should return error when env is invalid ": {
+			// Also covers addImageEnvs.
 			imageConfigChange: func(c *imagespec.ImageConfig) {
 				c.Env = []string{"a"}
 			},