Finish snapshot support.

Signed-off-by: Lantao Liu <lantaol@google.com>
containerd · Jun 16, 2017 · bad279e · bad279e
1 parent 484a326
commit bad279e
Show file tree

Hide file tree

Showing 14 changed files with 231 additions and 81 deletions.
diff --git a/pkg/server/container_create.go b/pkg/server/container_create.go
@@ -20,9 +20,7 @@ import (
 	"fmt"
 	"time"
 
-	snapshotapi "github.com/containerd/containerd/api/services/snapshot"
 	"github.com/golang/glog"
-	imagedigest "github.com/opencontainers/go-digest"
 	"golang.org/x/net/context"
 	runtime "k8s.io/kubernetes/pkg/kubelet/apis/cri/v1alpha1"
 
@@ -79,15 +77,22 @@ func (c *criContainerdService) CreateContainer(ctx context.Context, r *runtime.C
 	if imageMeta == nil {
 		return nil, fmt.Errorf("image %q not found", image)
 	}
-	if _, err := c.snapshotService.Prepare(ctx, &snapshotapi.PrepareRequest{
-		Key: id,
-		// We are sure that ChainID must be a digest.
-		Parent: imagedigest.Digest(imageMeta.ChainID).String(),
-		//Readonly: config.GetLinux().GetSecurityContext().GetReadonlyRootfs(),
-	}); err != nil {
-		return nil, fmt.Errorf("failed to prepare container rootfs %q: %v", imageMeta.ChainID, err)
+	if config.GetLinux().GetSecurityContext().GetReadonlyRootfs() {
+		if _, err := c.snapshotService.View(ctx, id, imageMeta.ChainID); err != nil {
+			return nil, fmt.Errorf("failed to view container rootfs %q: %v", imageMeta.ChainID, err)
+		}
+	} else {
+		if _, err := c.snapshotService.Prepare(ctx, id, imageMeta.ChainID); 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 snapshot api.
+	defer func() {
+		if retErr != nil {
+			if err := c.snapshotService.Remove(ctx, id); err != nil {
+				glog.Errorf("Failed to remove container snapshot %q: %v", id, err)
+			}
+		}
+	}()
 	meta.ImageRef = imageMeta.ID
 
 	// Create container root directory.

diff --git a/pkg/server/container_create_test.go b/pkg/server/container_create_test.go
@@ -134,7 +134,7 @@ func TestCreateContainer(t *testing.T) {
 	} {
 		t.Logf("TestCase %q", desc)
 		c := newTestCRIContainerdService()
-		fakeSnapshotClient := c.snapshotService.(*servertesting.FakeSnapshotClient)
+		fakeSnapshotClient := WithFakeSnapshotClient(c)
 		fakeOS := c.os.(*ostesting.FakeOS)
 		if test.sandboxMetadata != nil {
 			assert.NoError(t, c.sandboxStore.Create(*test.sandboxMetadata))
@@ -177,6 +177,7 @@ func TestCreateContainer(t *testing.T) {
 				assert.NoError(t, c.containerNameIndex.Reserve(containerName, "random id"),
 					"container name should be released")
 			}
+			assert.Empty(t, fakeSnapshotClient.ListMounts(), "snapshot should be cleaned up")
 			metas, err := c.containerStore.List()
 			assert.NoError(t, err)
 			assert.Empty(t, metas, "container metadata should not be created")

diff --git a/pkg/server/container_remove.go b/pkg/server/container_remove.go
@@ -19,9 +19,9 @@ package server
 import (
 	"fmt"
 
+	"github.com/containerd/containerd/snapshot"
 	"github.com/golang/glog"
 	"golang.org/x/net/context"
-
 	runtime "k8s.io/kubernetes/pkg/kubelet/apis/cri/v1alpha1"
 
 	"github.com/kubernetes-incubator/cri-containerd/pkg/metadata"
@@ -69,7 +69,13 @@ 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 snapshot after switching to new snapshot api.
+	// Remove container snapshot.
+	if err := c.snapshotService.Remove(ctx, id); err != nil {
+		if !snapshot.IsNotExist(err) {
+			return nil, fmt.Errorf("failed to remove container snapshot %q: %v", id, err)
+		}
+		glog.V(5).Infof("Remove called for snapshot %q that does not exist", id)
+	}
 
 	// Cleanup container root directory.
 	containerRootDir := getContainerRootDir(c.rootDir, id)

diff --git a/pkg/server/container_remove_test.go b/pkg/server/container_remove_test.go
@@ -21,13 +21,16 @@ import (
 	"testing"
 	"time"
 
+	snapshotapi "github.com/containerd/containerd/api/services/snapshot"
+	"github.com/containerd/containerd/api/types/mount"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"golang.org/x/net/context"
 	runtime "k8s.io/kubernetes/pkg/kubelet/apis/cri/v1alpha1"
 
 	"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"
 )
 
 // TestSetContainerRemoving tests setContainerRemoving sets removing
@@ -87,8 +90,16 @@ func TestSetContainerRemoving(t *testing.T) {
 func TestRemoveContainer(t *testing.T) {
 	testID := "test-id"
 	testName := "test-name"
+	testContainerMetadata := &metadata.ContainerMetadata{
+		ID:         testID,
+		CreatedAt:  time.Now().UnixNano(),
+		StartedAt:  time.Now().UnixNano(),
+		FinishedAt: time.Now().UnixNano(),
+	}
+
 	for desc, test := range map[string]struct {
 		metadata            *metadata.ContainerMetadata
+		removeSnapshotErr   error
 		removeDirErr        error
 		expectErr           bool
 		expectUnsetRemoving bool
@@ -112,32 +123,34 @@ func TestRemoveContainer(t *testing.T) {
 			expectErr: true,
 		},
 		"should not return error if container does not exist": {
-			metadata:  nil,
-			expectErr: false,
+			metadata:          nil,
+			removeSnapshotErr: servertesting.SnapshotNotExistError,
+			expectErr:         false,
+		},
+		"should not return error if snapshot does not exist": {
+			metadata:          testContainerMetadata,
+			removeSnapshotErr: servertesting.SnapshotNotExistError,
+			expectErr:         false,
+		},
+		"should return error if remove snapshot fails": {
+			metadata:          testContainerMetadata,
+			removeSnapshotErr: errors.New("random error"),
+			expectErr:         true,
 		},
 		"should return error if remove container root fails": {
-			metadata: &metadata.ContainerMetadata{
-				ID:         testID,
-				CreatedAt:  time.Now().UnixNano(),
-				StartedAt:  time.Now().UnixNano(),
-				FinishedAt: time.Now().UnixNano(),
-			},
+			metadata:            testContainerMetadata,
 			removeDirErr:        errors.New("random error"),
 			expectErr:           true,
 			expectUnsetRemoving: true,
 		},
 		"should be able to remove container successfully": {
-			metadata: &metadata.ContainerMetadata{
-				ID:         testID,
-				CreatedAt:  time.Now().UnixNano(),
-				StartedAt:  time.Now().UnixNano(),
-				FinishedAt: time.Now().UnixNano(),
-			},
+			metadata:  testContainerMetadata,
 			expectErr: false,
 		},
 	} {
 		t.Logf("TestCase %q", desc)
 		c := newTestCRIContainerdService()
+		fakeSnapshotClient := WithFakeSnapshotClient(c)
 		fakeOS := c.os.(*ostesting.FakeOS)
 		if test.metadata != nil {
 			assert.NoError(t, c.containerNameIndex.Reserve(testName, testID))
@@ -147,6 +160,17 @@ func TestRemoveContainer(t *testing.T) {
 			assert.Equal(t, getContainerRootDir(c.rootDir, testID), path)
 			return test.removeDirErr
 		}
+		if test.removeSnapshotErr == nil {
+			fakeSnapshotClient.SetFakeMounts(testID, []*mount.Mount{
+				{
+					Type:   "bind",
+					Source: "/test/source",
+					Target: "/test/target",
+				},
+			})
+		} else {
+			fakeSnapshotClient.InjectError("remove", test.removeSnapshotErr)
+		}
 		resp, err := c.RemoveContainer(context.Background(), &runtime.RemoveContainerRequest{
 			ContainerId: testID,
 		})
@@ -171,5 +195,8 @@ func TestRemoveContainer(t *testing.T) {
 		assert.Nil(t, meta, "container metadata should be removed")
 		assert.NoError(t, c.containerNameIndex.Reserve(testName, testID),
 			"container name should be released")
+		mountsResp, err := fakeSnapshotClient.Mounts(context.Background(), &snapshotapi.MountsRequest{Key: testID})
+		assert.Equal(t, servertesting.SnapshotNotExistError, err, "snapshot should be removed")
+		assert.Nil(t, mountsResp)
 	}
 }
diff --git a/pkg/server/container_start.go b/pkg/server/container_start.go
@@ -26,7 +26,7 @@ import (
 	"time"
 
 	"github.com/containerd/containerd/api/services/execution"
-	snapshotapi "github.com/containerd/containerd/api/services/snapshot"
+	"github.com/containerd/containerd/api/types/mount"
 	"github.com/containerd/containerd/api/types/task"
 	"github.com/golang/glog"
 	imagespec "github.com/opencontainers/image-spec/specs-go/v1"
@@ -176,15 +176,23 @@ func (c *criContainerdService) startContainer(ctx context.Context, id string, me
 	}
 
 	// Get rootfs mounts.
-	mountsResp, err := c.snapshotService.Mounts(ctx, &snapshotapi.MountsRequest{Key: id})
+	rootfsMounts, err := c.snapshotService.Mounts(ctx, id)
 	if err != nil {
 		return fmt.Errorf("failed to get rootfs mounts %q: %v", id, err)
 	}
+	var rootfs []*mount.Mount
+	for _, m := range rootfsMounts {
+		rootfs = append(rootfs, &mount.Mount{
+			Type:    m.Type,
+			Source:  m.Source,
+			Options: m.Options,
+		})
+	}
 
 	// Create containerd container.
 	createOpts := &execution.CreateRequest{
 		ContainerID: id,
-		Rootfs:      mountsResp.Mounts,
+		Rootfs:      rootfs,
 		Stdin:       stdin,
 		Stdout:      stdout,
 		Stderr:      stderr,

diff --git a/pkg/server/container_start_test.go b/pkg/server/container_start_test.go
@@ -590,7 +590,7 @@ func TestStartContainer(t *testing.T) {
 		c := newTestCRIContainerdService()
 		fake := c.containerService.(*servertesting.FakeExecutionClient)
 		fakeOS := c.os.(*ostesting.FakeOS)
-		fakeSnapshotClient := c.snapshotService.(*servertesting.FakeSnapshotClient)
+		fakeSnapshotClient := WithFakeSnapshotClient(c)
 		if test.containerMetadata != nil {
 			assert.NoError(t, c.containerStore.Create(*test.containerMetadata))
 		}

diff --git a/pkg/server/image_pull.go b/pkg/server/image_pull.go
@@ -24,11 +24,11 @@ import (
 	"sync"
 	"time"
 
-	snapshotapi "github.com/containerd/containerd/api/services/snapshot"
 	"github.com/containerd/containerd/content"
 	containerdimages "github.com/containerd/containerd/images"
 	"github.com/containerd/containerd/remotes"
 	"github.com/containerd/containerd/remotes/docker"
+	containerdrootfs "github.com/containerd/containerd/rootfs"
 	"github.com/golang/glog"
 	imagedigest "github.com/opencontainers/go-digest"
 	imagespec "github.com/opencontainers/image-spec/specs-go/v1"
@@ -187,6 +187,7 @@ func (r *resourceSet) all() map[string]struct{} {
 // pullImage pulls image and returns image id (config digest) and manifest digest.
 // The ref should be normalized image reference.
 func (c *criContainerdService) pullImage(ctx context.Context, ref string) (
+	// TODO(random-liu): Replace with client.Pull.
 	imagedigest.Digest, imagedigest.Digest, error) {
 	// Resolve the image reference to get descriptor and fetcher.
 	resolver := docker.NewResolver(docker.ResolverOptions{
@@ -250,6 +251,8 @@ func (c *criContainerdService) pullImage(ctx context.Context, ref string) (
 	}
 	glog.V(4).Infof("Finished downloading resources for image %q", ref)
 
+	// TODO(random-liu): Replace with image.Unpack.
+	// Unpack the image layers into snapshots.
 	image, err := c.imageStoreService.Get(ctx, ref)
 	if err != nil {
 		return "", "", fmt.Errorf("failed to get image %q from containerd image store: %v", ref, err)
@@ -265,18 +268,24 @@ func (c *criContainerdService) pullImage(ctx context.Context, ref string) (
 		return "", "", fmt.Errorf("unmarshal blob to manifest failed for manifest digest %q: %v",
 			manifestDigest, err)
 	}
-
-	// Unpack the image layers into snapshots.
-	/* snapshotUnpacker := snapshotservice.NewUnpackerFromClient(c.snapshotService)
-	if _, err = snapshotUnpacker.Unpack(ctx, manifest.Layers); err != nil {
-		return "", "", fmt.Errorf("unpack failed for manifest layers %+v: %v", manifest.Layers, err)
-	} TODO(mikebrow): WIP replacing the commented Unpack with the below Prepare request */
-	_, err = image.RootFS(ctx, c.contentStoreService)
+	diffIDs, err := image.RootFS(ctx, c.contentStoreService)
 	if err != nil {
 		return "", "", err
 	}
-	if _, err = c.snapshotService.Prepare(ctx, &snapshotapi.PrepareRequest{Key: ref, Parent: ""}); err != nil {
-		return "", "", err
+	if len(diffIDs) != len(manifest.Layers) {
+		return "", "", fmt.Errorf("mismatched image rootfs and manifest layers")
+	}
+	layers := make([]containerdrootfs.Layer, len(diffIDs))
+	for i := range diffIDs {
+		layers[i].Diff = imagespec.Descriptor{
+			// TODO: derive media type from compressed type
+			MediaType: imagespec.MediaTypeImageLayer,
+			Digest:    diffIDs[i],
+		}
+		layers[i].Blob = manifest.Layers[i]
+	}
+	if _, err = containerdrootfs.ApplyLayers(ctx, layers, c.snapshotService, c.diffService); err != nil {
+		return "", "", fmt.Errorf("failed to apply layers %+v: %v", layers, err)
 	}
 
 	// TODO(random-liu): Considering how to deal with the disk usage of content.

diff --git a/pkg/server/sandbox_remove.go b/pkg/server/sandbox_remove.go
@@ -19,11 +19,10 @@ package server
 import (
 	"fmt"
 
+	"github.com/containerd/containerd/api/services/execution"
+	"github.com/containerd/containerd/snapshot"
 	"github.com/golang/glog"
 	"golang.org/x/net/context"
-
-	"github.com/containerd/containerd/api/services/execution"
-
 	runtime "k8s.io/kubernetes/pkg/kubelet/apis/cri/v1alpha1"
 
 	"github.com/kubernetes-incubator/cri-containerd/pkg/metadata"
@@ -65,7 +64,14 @@ func (c *criContainerdService) RemovePodSandbox(ctx context.Context, r *runtime.
 		return nil, fmt.Errorf("sandbox container %q is not fully stopped", id)
 	}
 
-	// TODO(random-liu): [P0] Cleanup snapshot after switching to new snapshot api.
+	// Remove sandbox container snapshot.
+	if err := c.snapshotService.Remove(ctx, id); err != nil {
+		if !snapshot.IsNotExist(err) {
+			return nil, fmt.Errorf("failed to remove sandbox container snapshot %q: %v", id, err)
+		}
+		glog.V(5).Infof("Remove called for snapshot %q that does not exist", id)
+	}
+
 	// TODO(random-liu): [P0] Cleanup shm created in RunPodSandbox.
 	// TODO(random-liu): [P1] Remove permanent namespace once used.