Create/remove sandbox container.

Signed-off-by: Lantao Liu <lantaol@google.com>

pkg/server/sandbox_remove.go

@@ -19,6 +19,7 @@ package server
 import (
 	"fmt"
 
+	"github.com/containerd/containerd/api/services/containers"
 	"github.com/containerd/containerd/api/services/execution"
 	"github.com/containerd/containerd/snapshot"
 	"github.com/golang/glog"
@@ -82,6 +83,14 @@ func (c *criContainerdService) RemovePodSandbox(ctx context.Context, r *runtime.
 			sandboxRootDir, err)
 	}
 
+	// Delete sandbox container.
+	if _, err := c.containerService.Delete(ctx, &containers.DeleteContainerRequest{ID: id}); err != nil {
+		if !isContainerdGRPCNotFoundError(err) {
+			return nil, fmt.Errorf("failed to delete sandbox container %q: %v", id, err)
+		}
+		glog.V(5).Infof("Remove called for sandbox container %q that does not exist", id, err)
+	}
+
 	// Remove sandbox metadata from metadata store. Note that once the sandbox
 	// metadata is successfully deleted:
 	// 1) ListPodSandbox will not include this sandbox.

pkg/server/sandbox_remove_test.go

@@ -18,6 +18,7 @@ import (
 	"fmt"
 	"testing"
 
+	"github.com/containerd/containerd/api/services/containers"
 	snapshotapi "github.com/containerd/containerd/api/services/snapshot"
 	"github.com/containerd/containerd/api/types/mount"
 	"github.com/containerd/containerd/api/types/task"
@@ -38,20 +39,22 @@ func TestRemovePodSandbox(t *testing.T) {
 		Name: testName,
 	}
 	for desc, test := range map[string]struct {
-		sandboxTasks        []task.Task
+		sandboxTasks       []task.Task
-		injectMetadata      bool
+		injectMetadata     bool
-		removeSnapshotErr   error
+		removeSnapshotErr  error
-		injectContainerdErr error
+		deleteContainerErr error
-		injectFSErr         error
+		taskInfoErr        error
-		expectErr           bool
+		injectFSErr        error
-		expectRemoved       string
+		expectErr          bool
-		expectCalls         []string
+		expectRemoved      string
+		expectCalls        []string
 	}{
 		"should not return error if sandbox does not exist": {
-			injectMetadata:    false,
+			injectMetadata:     false,
-			removeSnapshotErr: servertesting.SnapshotNotExistError,
+			removeSnapshotErr:  servertesting.SnapshotNotExistError,
-			expectErr:         false,
+			deleteContainerErr: servertesting.ContainerNotExistError,
-			expectCalls:       []string{},
+			expectErr:          false,
+			expectCalls:        []string{},
 		},
 		"should not return error if snapshot does not exist": {
 			injectMetadata:    true,
@@ -65,17 +68,17 @@ func TestRemovePodSandbox(t *testing.T) {
 			expectErr:         true,
 			expectCalls:       []string{"info"},
 		},
-		"should return error when sandbox container is not deleted": {
+		"should return error when sandbox container task is not deleted": {
 			injectMetadata: true,
 			sandboxTasks:   []task.Task{{ID: testID}},
 			expectErr:      true,
 			expectCalls:    []string{"info"},
 		},
 		"should return error when arbitrary containerd error is injected": {
-			injectMetadata:      true,
+			injectMetadata: true,
-			injectContainerdErr: fmt.Errorf("arbitrary error"),
+			taskInfoErr:    fmt.Errorf("arbitrary error"),
-			expectErr:           true,
+			expectErr:      true,
-			expectCalls:         []string{"info"},
+			expectCalls:    []string{"info"},
 		},
 		"should return error when error fs error is injected": {
 			injectMetadata: true,
@@ -84,6 +87,19 @@ func TestRemovePodSandbox(t *testing.T) {
 			expectErr:      true,
 			expectCalls:    []string{"info"},
 		},
+		"should not return error if sandbox container does not exist": {
+			injectMetadata:     true,
+			deleteContainerErr: servertesting.ContainerNotExistError,
+			expectRemoved:      getSandboxRootDir(testRootDir, testID),
+			expectCalls:        []string{"info"},
+		},
+		"should return error if delete sandbox container fails": {
+			injectMetadata:     true,
+			deleteContainerErr: fmt.Errorf("arbitrary error"),
+			expectRemoved:      getSandboxRootDir(testRootDir, testID),
+			expectErr:          true,
+			expectCalls:        []string{"info"},
+		},
 		"should be able to successfully delete": {
 			injectMetadata: true,
 			expectRemoved:  getSandboxRootDir(testRootDir, testID),
@@ -92,10 +108,11 @@ func TestRemovePodSandbox(t *testing.T) {
 	} {
 		t.Logf("TestCase %q", desc)
 		c := newTestCRIContainerdService()
-		fake := c.taskService.(*servertesting.FakeExecutionClient)
+		fake := c.containerService.(*servertesting.FakeContainersClient)
 		fakeOS := c.os.(*ostesting.FakeOS)
+		fakeExecutionClient := c.taskService.(*servertesting.FakeExecutionClient)
 		fakeSnapshotClient := WithFakeSnapshotClient(c)
-		fake.SetFakeTasks(test.sandboxTasks)
+		fakeExecutionClient.SetFakeTasks(test.sandboxTasks)
 		if test.injectMetadata {
 			c.sandboxNameIndex.Reserve(testName, testID)
 			c.sandboxIDIndex.Add(testID)
@@ -112,8 +129,16 @@ func TestRemovePodSandbox(t *testing.T) {
 		} else {
 			fakeSnapshotClient.InjectError("remove", test.removeSnapshotErr)
 		}
-		if test.injectContainerdErr != nil {
+		if test.deleteContainerErr == nil {
-			fake.InjectError("info", test.injectContainerdErr)
+			_, err := fake.Create(context.Background(), &containers.CreateContainerRequest{
+				Container: containers.Container{ID: testID},
+			})
+			assert.NoError(t, err)
+		} else {
+			fake.InjectError("delete", test.deleteContainerErr)
+		}
+		if test.taskInfoErr != nil {
+			fakeExecutionClient.InjectError("info", test.taskInfoErr)
 		}
 		fakeOS.RemoveAllFn = func(path string) error {
 			assert.Equal(t, test.expectRemoved, path)
@@ -122,7 +147,7 @@ func TestRemovePodSandbox(t *testing.T) {
 		res, err := c.RemovePodSandbox(context.Background(), &runtime.RemovePodSandboxRequest{
 			PodSandboxId: testID,
 		})
-		assert.Equal(t, test.expectCalls, fake.GetCalledNames())
+		assert.Equal(t, test.expectCalls, fakeExecutionClient.GetCalledNames())
 		if test.expectErr {
 			assert.Error(t, err)
 			assert.Nil(t, res)
@@ -141,6 +166,9 @@ func TestRemovePodSandbox(t *testing.T) {
 		mountsResp, err := fakeSnapshotClient.Mounts(context.Background(), &snapshotapi.MountsRequest{Key: testID})
 		assert.Equal(t, servertesting.SnapshotNotExistError, err, "snapshot should be removed")
 		assert.Nil(t, mountsResp)
+		getResp, err := fake.Get(context.Background(), &containers.GetContainerRequest{ID: testID})
+		assert.Equal(t, servertesting.ContainerNotExistError, err, "containerd container should be removed")
+		assert.Nil(t, getResp)
 		res, err = c.RemovePodSandbox(context.Background(), &runtime.RemovePodSandboxRequest{
 			PodSandboxId: testID,
 		})

pkg/server/sandbox_run.go

@@ -23,8 +23,10 @@ import (
 	"strings"
 	"time"
 
+	"github.com/containerd/containerd/api/services/containers"
 	"github.com/containerd/containerd/api/services/execution"
 	"github.com/containerd/containerd/api/types/mount"
+	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"
@@ -85,6 +87,7 @@ func (c *criContainerdService) RunPodSandbox(ctx context.Context, r *runtime.Run
 	if err != nil {
 		return nil, fmt.Errorf("failed to get sandbox image %q: %v", defaultSandboxImage, err)
 	}
+
 	rootfsMounts, err := c.snapshotService.View(ctx, id, imageMeta.ChainID)
 	if err != nil {
 		return nil, fmt.Errorf("failed to prepare sandbox rootfs %q: %v", imageMeta.ChainID, err)
@@ -105,6 +108,39 @@ func (c *criContainerdService) RunPodSandbox(ctx context.Context, r *runtime.Run
 		})
 	}
 
+	// Create sandbox container.
+	spec, err := c.generateSandboxContainerSpec(id, config, imageMeta.Config)
+	if err != nil {
+		return nil, fmt.Errorf("failed to generate sandbox container spec: %v", err)
+	}
+	rawSpec, err := json.Marshal(spec)
+	if err != nil {
+		return nil, fmt.Errorf("failed to marshal oci spec %+v: %v", spec, err)
+	}
+	glog.V(4).Infof("Sandbox container spec: %+v", spec)
+	if _, err = c.containerService.Create(ctx, &containers.CreateContainerRequest{
+		Container: containers.Container{
+			ID: id,
+			// TODO(random-liu): Checkpoint metadata into container labels.
+			Image:   imageMeta.ID,
+			Runtime: defaultRuntime,
+			Spec: &prototypes.Any{
+				TypeUrl: runtimespec.Version,
+				Value:   rawSpec,
+			},
+			RootFS: id,
+		},
+	}); err != nil {
+		return nil, fmt.Errorf("failed to create containerd container: %v", err)
+	}
+	defer func() {
+		if retErr != nil {
+			if _, err := c.containerService.Delete(ctx, &containers.DeleteContainerRequest{ID: id}); err != nil {
+				glog.Errorf("Failed to delete containerd container%q: %v", id, err)
+			}
+		}
+	}()
+
 	// Create sandbox container root directory.
 	// Prepare streaming named pipe.
 	sandboxRootDir := getSandboxRootDir(c.rootDir, id)
@@ -151,16 +187,6 @@ func (c *criContainerdService) RunPodSandbox(ctx context.Context, r *runtime.Run
 		}
 	}()
 
-	// Start sandbox container.
-	spec, err := c.generateSandboxContainerSpec(id, config, imageMeta.Config)
-	if err != nil {
-		return nil, fmt.Errorf("failed to generate sandbox container spec: %v", err)
-	}
-	_, err = json.Marshal(spec)
-	if err != nil {
-		return nil, fmt.Errorf("failed to marshal oci spec %+v: %v", spec, err)
-	}
-	glog.V(4).Infof("Sandbox container spec: %+v", spec)
 	createOpts := &execution.CreateRequest{
 		ContainerID: id,
 		Rootfs:      rootfs,
@@ -168,8 +194,7 @@ func (c *criContainerdService) RunPodSandbox(ctx context.Context, r *runtime.Run
 		Stdout: stdout,
 		Stderr: stderr,
 	}
-
+	// Create sandbox task in containerd.
-	// Create sandbox container in containerd.
 	glog.V(5).Infof("Create sandbox container (id=%q, name=%q) with options %+v.",
 		id, name, createOpts)
 	createResp, err := c.taskService.Create(ctx, createOpts)

pkg/server/sandbox_run_test.go

@@ -17,11 +17,13 @@ limitations under the License.
 package server
 
 import (
+	"encoding/json"
 	"io"
 	"os"
 	"syscall"
 	"testing"
 
+	"github.com/containerd/containerd/api/services/containers"
 	"github.com/containerd/containerd/api/services/execution"
 	snapshotapi "github.com/containerd/containerd/api/services/snapshot"
 	imagespec "github.com/opencontainers/image-spec/specs-go/v1"
@@ -266,8 +268,9 @@ options timeout:1
 }
 
 func TestRunPodSandbox(t *testing.T) {
-	config, imageConfig, _ := getRunPodSandboxTestData() // TODO: declare and test specCheck see below
+	config, imageConfig, specCheck := getRunPodSandboxTestData()
 	c := newTestCRIContainerdService()
+	fake := c.containerService.(*servertesting.FakeContainersClient)
 	fakeSnapshotClient := WithFakeSnapshotClient(c)
 	fakeExecutionClient := c.taskService.(*servertesting.FakeExecutionClient)
 	fakeCNIPlugin := c.netPlugin.(*servertesting.FakeCNIPlugin)
@@ -292,6 +295,7 @@ func TestRunPodSandbox(t *testing.T) {
 	}
 	// Insert sandbox image metadata.
 	assert.NoError(t, c.imageMetadataStore.Create(imageMetadata))
+	expectContainersClientCalls := []string{"create"}
 	expectSnapshotClientCalls := []string{"view"}
 	expectExecutionClientCalls := []string{"create", "start"}
 
@@ -316,21 +320,29 @@ func TestRunPodSandbox(t *testing.T) {
 		Parent: testChainID,
 	}, prepareOpts, "prepare request should be correct")
 
-	assert.Equal(t, expectExecutionClientCalls, fakeExecutionClient.GetCalledNames(), "expect containerd functions should be called")
+	assert.Equal(t, expectContainersClientCalls, fake.GetCalledNames(), "expect containers functions should be called")
+	calls = fake.GetCalledDetails()
+	createOpts, ok := calls[0].Argument.(*containers.CreateContainerRequest)
+	assert.True(t, ok, "should create sandbox container")
+	assert.Equal(t, id, createOpts.Container.ID, "container id should be correct")
+	assert.Equal(t, testSandboxImage, createOpts.Container.Image, "test image should be correct")
+	assert.Equal(t, id, createOpts.Container.RootFS, "rootfs should be correct")
+	spec := &runtimespec.Spec{}
+	assert.NoError(t, json.Unmarshal(createOpts.Container.Spec.Value, spec))
+	t.Logf("oci spec check")
+	specCheck(t, id, spec)
+
+	assert.Equal(t, expectExecutionClientCalls, fakeExecutionClient.GetCalledNames(), "expect execution functions should be called")
 	calls = fakeExecutionClient.GetCalledDetails()
-	createOpts := calls[0].Argument.(*execution.CreateRequest)
+	createTaskOpts := calls[0].Argument.(*execution.CreateRequest)
-	assert.Equal(t, id, createOpts.ContainerID, "create id should be correct")
-	assert.Equal(t, stdout, createOpts.Stdout, "stdout pipe should be passed to containerd")
-	assert.Equal(t, stderr, createOpts.Stderr, "stderr pipe should be passed to containerd")
 	mountsResp, err := fakeSnapshotClient.Mounts(context.Background(), &snapshotapi.MountsRequest{Key: id})
 	assert.NoError(t, err)
-	assert.Equal(t, mountsResp.Mounts, createOpts.Rootfs, "rootfs mount should be correct")
+	assert.Equal(t, &execution.CreateRequest{
-
+		ContainerID: id,
-	// TODO: Need to create container first.. see Create in containerd/containerd/apsi/services/containers spec is no longer in the create request
+		Rootfs:      mountsResp.Mounts,
-	//spec := &runtimespec.Spec{}
+		Stdout:      stdout,
-	//assert.NoError(t, json.Unmarshal(createOpts.Spec.Value, spec))
+		Stderr:      stderr,
-	//t.Logf("oci spec check")
+	}, createTaskOpts, "create options should be correct")
-	//specCheck(t, id, spec)
 
 	startID := calls[1].Argument.(*execution.StartRequest).ContainerID
 	assert.Equal(t, id, startID, "start id should be correct")