Merge pull request #1366 from stevvooe/containers-metadata-tests

metadata: ensure correct updates on Container

client.go

@@ -192,6 +192,7 @@ func defaultRemoteContext() *RemoteContext {
 		Resolver: docker.NewResolver(docker.ResolverOptions{
 			Client: http.DefaultClient,
 		}),
+		Snapshotter: DefaultSnapshotter,
 	}
 }
 

container.go

@@ -174,6 +174,10 @@ func (c *container) NewTask(ctx context.Context, ioCreate IOCreation, opts ...Ne
 		Stderr:      cfg.Stderr,
 	}
 	if c.c.RootFS != "" {
+		if c.c.Snapshotter == "" {
+			return nil, errors.Wrapf(errdefs.ErrInvalidArgument, "unable to resolve rootfs mounts without snapshotter on container")
+		}
+
 		// get the rootfs from the snapshotter and add it to the request
 		mounts, err := c.client.SnapshotService(c.c.Snapshotter).Mounts(ctx, c.c.RootFS)
 		if err != nil {

container_opts.go

@@ -4,7 +4,9 @@ import (
 	"context"
 
 	"github.com/containerd/containerd/containers"
+	"github.com/containerd/containerd/errdefs"
 	"github.com/opencontainers/image-spec/identity"
+	"github.com/pkg/errors"
 )
 
 // NewContainerOpts allows the caller to set additional options when creating a container
@@ -38,6 +40,8 @@ func WithContainerLabels(labels map[string]string) NewContainerOpts {
 }
 
 // WithSnapshotter sets the provided snapshotter for use by the container
+//
+// This option must appear before other snapshotter options to have an effect.
 func WithSnapshotter(name string) NewContainerOpts {
 	return func(ctx context.Context, client *Client, c *containers.Container) error {
 		c.Snapshotter = name
@@ -48,6 +52,7 @@ func WithSnapshotter(name string) NewContainerOpts {
 // WithSnapshot uses an existing root filesystem for the container
 func WithSnapshot(id string) NewContainerOpts {
 	return func(ctx context.Context, client *Client, c *containers.Container) error {
+		setSnapshotterIfEmpty(c)
 		// check that the snapshot exists, if not, fail on creation
 		if _, err := client.SnapshotService(c.Snapshotter).Mounts(ctx, id); err != nil {
 			return err
@@ -65,6 +70,7 @@ func WithNewSnapshot(id string, i Image) NewContainerOpts {
 		if err != nil {
 			return err
 		}
+		setSnapshotterIfEmpty(c)
 		if _, err := client.SnapshotService(c.Snapshotter).Prepare(ctx, id, identity.ChainID(diffIDs).String()); err != nil {
 			return err
 		}
@@ -77,6 +83,9 @@ func WithNewSnapshot(id string, i Image) NewContainerOpts {
 // WithSnapshotCleanup deletes the rootfs allocated for the container
 func WithSnapshotCleanup(ctx context.Context, client *Client, c containers.Container) error {
 	if c.RootFS != "" {
+		if c.Snapshotter == "" {
+			return errors.Wrapf(errdefs.ErrInvalidArgument, "container.Snapshotter must be set to cleanup rootfs")
+		}
 		return client.SnapshotService(c.Snapshotter).Remove(ctx, c.RootFS)
 	}
 	return nil
@@ -90,6 +99,7 @@ func WithNewSnapshotView(id string, i Image) NewContainerOpts {
 		if err != nil {
 			return err
 		}
+		setSnapshotterIfEmpty(c)
 		if _, err := client.SnapshotService(c.Snapshotter).View(ctx, id, identity.ChainID(diffIDs).String()); err != nil {
 			return err
 		}
@@ -98,3 +108,9 @@ func WithNewSnapshotView(id string, i Image) NewContainerOpts {
 		return nil
 	}
 }
+
+func setSnapshotterIfEmpty(c *containers.Container) {
+	if c.Snapshotter == "" {
+		c.Snapshotter = DefaultSnapshotter
+	}
+}

container_opts_unix.go

@@ -46,6 +46,7 @@ func WithCheckpoint(desc v1.Descriptor, rootfsID string) NewContainerOpts {
 				if err != nil {
 					return err
 				}
+				setSnapshotterIfEmpty(c)
 				if _, err := client.SnapshotService(c.Snapshotter).Prepare(ctx, rootfsID, identity.ChainID(diffIDs).String()); err != nil {
 					if !errdefs.IsAlreadyExists(err) {
 						return err

containers/containers.go

@@ -12,15 +12,51 @@ import (
 //
 // The resources specified in this object are used to create tasks from the container.
 type Container struct {
-	ID          string
+	// ID uniquely identifies the container in a nameapace.
-	Labels      map[string]string
+	//
-	Image       string
+	// This property is required and cannot be changed after creation.
-	Runtime     RuntimeInfo
+	ID string
-	Spec        *types.Any
+
-	RootFS      string
+	// Labels provide metadata extension for a contaienr.
+	//
+	// These are optional and fully mutable.
+	Labels map[string]string
+
+	// Image specifies the image reference used for a container.
+	//
+	// This property is optional but immutable.
+	Image string
+
+	// Runtime specifies which runtime should be used when launching container
+	// tasks.
+	//
+	// This property is required and immutable.
+	Runtime RuntimeInfo
+
+	// Spec should carry the the runtime specification used to implement the
+	// container.
+	//
+	// This field is required but mutable.
+	Spec *types.Any
+
+	// RootFS specifies the snapshot key to use for the container's root
+	// filesystem. When starting a task from this container, a caller should
+	// look up the mounts from the snapshot service and include those on the
+	// task create request.
+	//
+	// This field is not required but immutable.
+	RootFS string
+
+	// Snapshotter specifies the snapshotter name used for rootfs
+	//
+	// This field is not required but immutable.
 	Snapshotter string
-	CreatedAt   time.Time
+
-	UpdatedAt   time.Time
+	// CreatedAt is the time at which the container was created.
+	CreatedAt time.Time
+
+	// UpdatedAt is the time at which the container was updated.
+	UpdatedAt time.Time
 }
 
 type RuntimeInfo struct {
@@ -34,6 +70,7 @@ type Store interface {
 	// List returns containers that match one or more of the provided filters.
 	List(ctx context.Context, filters ...string) ([]Container, error)
 
+	// Create a container in the store from the provided container.
 	Create(ctx context.Context, container Container) (Container, error)
 
 	// Update the container with the provided container object. ID must be set.
@@ -42,5 +79,9 @@ type Store interface {
 	// the fieldpaths will be mutated.
 	Update(ctx context.Context, container Container, fieldpaths ...string) (Container, error)
 
+	// Delete a container using the id.
+	//
+	// nil will be returned on success. If the container is not known to the
+	// store, ErrNotFound will be returned.
 	Delete(ctx context.Context, id string) error
 }

metadata/buckets.go

@@ -38,17 +38,18 @@ var (
 	bucketKeyObjectBlob       = []byte("blob")       // stores content links
 	bucketKeyObjectIngest     = []byte("ingest")     // stores ingest links
 
-	bucketKeyDigest    = []byte("digest")
+	bucketKeyDigest      = []byte("digest")
-	bucketKeyMediaType = []byte("mediatype")
+	bucketKeyMediaType   = []byte("mediatype")
-	bucketKeySize      = []byte("size")
+	bucketKeySize        = []byte("size")
-	bucketKeyImage     = []byte("image")
+	bucketKeyImage       = []byte("image")
-	bucketKeyRuntime   = []byte("runtime")
+	bucketKeyRuntime     = []byte("runtime")
-	bucketKeyName      = []byte("name")
+	bucketKeyName        = []byte("name")
-	bucketKeyParent    = []byte("parent")
+	bucketKeyParent      = []byte("parent")
-	bucketKeyOptions   = []byte("options")
+	bucketKeyOptions     = []byte("options")
-	bucketKeySpec      = []byte("spec")
+	bucketKeySpec        = []byte("spec")
-	bucketKeyRootFS    = []byte("rootfs")
+	bucketKeyRootFS      = []byte("rootfs")
-	bucketKeyTarget    = []byte("target")
+	bucketKeySnapshotter = []byte("snapshotter")
+	bucketKeyTarget      = []byte("target")
 )
 
 func getBucket(tx *bolt.Tx, keys ...[]byte) *bolt.Bucket {

metadata/containers.go

@@ -91,8 +91,8 @@ func (s *containerStore) Create(ctx context.Context, container containers.Contai
 		return containers.Container{}, err
 	}
 
-	if err := identifiers.Validate(container.ID); err != nil {
+	if err := validateContainer(&container); err != nil {
-		return containers.Container{}, err
+		return containers.Container{}, errors.Wrap(err, "create container failed validation")
 	}
 
 	bkt, err := createContainersBucket(s.tx, namespace)
@@ -144,37 +144,55 @@ func (s *containerStore) Update(ctx context.Context, container containers.Contai
 	createdat := updated.CreatedAt
 	updated.ID = container.ID
 
+	if len(fieldpaths) == 0 {
+		// only allow updates to these field on full replace.
+		fieldpaths = []string{"labels", "spec"}
+
+		// Fields that are immutable must cause an error when no field paths
+		// are provided. This allows these fields to become mutable in the
+		// future.
+		if updated.Image != container.Image {
+			return containers.Container{}, errors.Wrapf(errdefs.ErrInvalidArgument, "container.Image field is immutable")
+		}
+
+		if updated.RootFS != container.RootFS {
+			return containers.Container{}, errors.Wrapf(errdefs.ErrInvalidArgument, "container.RootFS field is immutable")
+		}
+
+		if updated.Snapshotter != container.Snapshotter {
+			return containers.Container{}, errors.Wrapf(errdefs.ErrInvalidArgument, "container.Snapshotter field is immutable")
+		}
+
+		if updated.Runtime.Name != container.Runtime.Name {
+			return containers.Container{}, errors.Wrapf(errdefs.ErrInvalidArgument, "container.Runtime.Name field is immutable")
+		}
+	}
+
 	// apply the field mask. If you update this code, you better follow the
 	// field mask rules in field_mask.proto. If you don't know what this
 	// is, do not update this code.
-	if len(fieldpaths) > 0 {
+	for _, path := range fieldpaths {
-		// TODO(stevvooe): Move this logic into the store itself.
+		if strings.HasPrefix(path, "labels.") {
-		for _, path := range fieldpaths {
+			if updated.Labels == nil {
-			if strings.HasPrefix(path, "labels.") {
+				updated.Labels = map[string]string{}
-				if updated.Labels == nil {
-					updated.Labels = map[string]string{}
-				}
-				key := strings.TrimPrefix(path, "labels.")
-				updated.Labels[key] = container.Labels[key]
-				continue
-			}
-
-			switch path {
-			case "labels":
-				updated.Labels = container.Labels
-			case "image":
-				updated.Image = container.Image
-			case "spec":
-				updated.Spec = container.Spec
-			case "rootfs":
-				updated.RootFS = container.RootFS
-			default:
-				return containers.Container{}, errors.Wrapf(errdefs.ErrInvalidArgument, "cannot update %q field on %q", path, container.ID)
 			}
+			key := strings.TrimPrefix(path, "labels.")
+			updated.Labels[key] = container.Labels[key]
+			continue
 		}
-	} else {
+
-		// no field mask present, just replace everything
+		switch path {
-		updated = container
+		case "labels":
+			updated.Labels = container.Labels
+		case "spec":
+			updated.Spec = container.Spec
+		default:
+			return containers.Container{}, errors.Wrapf(errdefs.ErrInvalidArgument, "cannot update %q field on %q", path, container.ID)
+		}
+	}
+
+	if err := validateContainer(&updated); err != nil {
+		return containers.Container{}, errors.Wrap(err, "update failed validation")
 	}
 
 	updated.CreatedAt = createdat
@@ -183,7 +201,7 @@ func (s *containerStore) Update(ctx context.Context, container containers.Contai
 		return containers.Container{}, errors.Wrap(err, "failed to write container")
 	}
 
-	return container, nil
+	return updated, nil
 }
 
 func (s *containerStore) Delete(ctx context.Context, id string) error {
@@ -203,6 +221,27 @@ func (s *containerStore) Delete(ctx context.Context, id string) error {
 	return err
 }
 
+func validateContainer(container *containers.Container) error {
+	if err := identifiers.Validate(container.ID); err != nil {
+		return errors.Wrapf(err, "container.ID validation error")
+	}
+
+	// labels and image have no validation
+	if container.Runtime.Name == "" {
+		return errors.Wrapf(errdefs.ErrInvalidArgument, "container.Runtime.Name must be set")
+	}
+
+	if container.Spec == nil {
+		return errors.Wrapf(errdefs.ErrInvalidArgument, "container.Spec must be set")
+	}
+
+	if container.RootFS != "" && container.Snapshotter == "" {
+		return errors.Wrapf(errdefs.ErrInvalidArgument, "container.Snapshotter must be set if container.RootFS is set")
+	}
+
+	return nil
+}
+
 func readContainer(container *containers.Container, bkt *bolt.Bucket) error {
 	labels, err := boltutil.ReadLabels(bkt)
 	if err != nil {
@@ -247,7 +286,8 @@ func readContainer(container *containers.Container, bkt *bolt.Bucket) error {
 			container.Spec = &any
 		case string(bucketKeyRootFS):
 			container.RootFS = string(v)
-
+		case string(bucketKeySnapshotter):
+			container.Snapshotter = string(v)
 		}
 
 		return nil
@@ -272,6 +312,7 @@ func writeContainer(bkt *bolt.Bucket, container *containers.Container) error {
 
 	for _, v := range [][2][]byte{
 		{bucketKeyImage, []byte(container.Image)},
+		{bucketKeySnapshotter, []byte(container.Snapshotter)},
 		{bucketKeyRootFS, []byte(container.RootFS)},
 	} {
 		if err := bkt.Put(v[0], v[1]); err != nil {

metadata/containers_test.go

@@ -0,0 +1,557 @@
+package metadata
+
+import (
+	"context"
+	"fmt"
+	"io/ioutil"
+	"os"
+	"path/filepath"
+	"reflect"
+	"testing"
+	"time"
+
+	"github.com/boltdb/bolt"
+	"github.com/containerd/containerd/containers"
+	"github.com/containerd/containerd/errdefs"
+	"github.com/containerd/containerd/filters"
+	"github.com/containerd/containerd/namespaces"
+	"github.com/containerd/containerd/typeurl"
+	specs "github.com/opencontainers/runtime-spec/specs-go"
+	"github.com/pkg/errors"
+)
+
+func init() {
+	typeurl.Register(&specs.Spec{}, "opencontainers/runtime-spec", "v1", "Spec")
+}
+
+func TestContainersList(t *testing.T) {
+	ctx, db, cancel := testEnv(t)
+	defer cancel()
+
+	spec := &specs.Spec{}
+	encoded, err := typeurl.MarshalAny(spec)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	testset := map[string]*containers.Container{}
+	for i := 0; i < 4; i++ {
+		id := "container-" + fmt.Sprint(i)
+		testset[id] = &containers.Container{
+			ID: id,
+			Labels: map[string]string{
+				"idlabel": id,
+				"even":    fmt.Sprint(i%2 == 0),
+				"odd":     fmt.Sprint(i%2 != 0),
+			},
+			Spec:        encoded,
+			RootFS:      "test-rootfs",
+			Snapshotter: "snapshotter",
+			Runtime: containers.RuntimeInfo{
+				Name: "testruntime",
+			},
+			Image: "test image",
+		}
+
+		if err := db.Update(func(tx *bolt.Tx) error {
+			store := NewContainerStore(tx)
+			now := time.Now()
+			result, err := store.Create(ctx, *testset[id])
+			if err != nil {
+				return err
+			}
+
+			checkContainerTimestamps(t, &result, now, true)
+			testset[id].UpdatedAt, testset[id].CreatedAt = result.UpdatedAt, result.CreatedAt
+			checkContainersEqual(t, &result, testset[id], "ensure that containers were created as expected for list")
+			return nil
+		}); err != nil {
+			t.Fatal(err)
+		}
+	}
+
+	for _, testcase := range []struct {
+		name    string
+		filters []string
+	}{
+		{
+			name: "FullSet",
+		},
+		{
+			name:    "FullSetFiltered", // full set, but because we have OR filter
+			filters: []string{"labels.even==true", "labels.odd==true"},
+		},
+		{
+			name:    "Even",
+			filters: []string{"labels.even==true"},
+		},
+		{
+			name:    "Odd",
+			filters: []string{"labels.odd==true"},
+		},
+		{
+			name:    "ByID",
+			filters: []string{"id==container-0"},
+		},
+		{
+			name:    "ByIDLabelEven",
+			filters: []string{"labels.idlabel==container-0,labels.even==true"},
+		},
+		{
+			name:    "ByRuntime",
+			filters: []string{"runtime.name==testruntime"},
+		},
+	} {
+		t.Run(testcase.name, func(t *testing.T) {
+			testset := testset
+			if len(testcase.filters) > 0 {
+				fs, err := filters.ParseAll(testcase.filters...)
+				if err != nil {
+					t.Fatal(err)
+				}
+
+				newtestset := make(map[string]*containers.Container, len(testset))
+				for k, v := range testset {
+					if fs.Match(adaptContainer(*v)) {
+						newtestset[k] = v
+					}
+				}
+				testset = newtestset
+			}
+
+			if err := db.View(func(tx *bolt.Tx) error {
+				store := NewContainerStore(tx)
+				results, err := store.List(ctx, testcase.filters...)
+				if err != nil {
+					t.Fatal(err)
+				}
+
+				if len(results) == 0 { // all tests return a non-empty result set
+					t.Fatalf("not results returned")
+				}
+
+				if len(results) != len(testset) {
+					t.Fatalf("length of result does not match testset: %v != %v", len(results), len(testset))
+				}
+
+				for _, result := range results {
+					checkContainersEqual(t, &result, testset[result.ID], "list results did not match")
+				}
+
+				return nil
+			}); err != nil {
+				t.Fatal(err)
+			}
+		})
+	}
+
+	// delete everything to test it
+	for id := range testset {
+		if err := db.Update(func(tx *bolt.Tx) error {
+			store := NewContainerStore(tx)
+			return store.Delete(ctx, id)
+		}); err != nil {
+			t.Fatal(err)
+		}
+
+		// try it again, get NotFound
+		if err := db.Update(func(tx *bolt.Tx) error {
+			store := NewContainerStore(tx)
+			return store.Delete(ctx, id)
+		}); errors.Cause(err) != errdefs.ErrNotFound {
+			t.Fatalf("unexpected error %v", err)
+		}
+	}
+}
+
+// TestContainersUpdate ensures that updates are taken in an expected manner.
+func TestContainersCreateUpdateDelete(t *testing.T) {
+	ctx, db, cancel := testEnv(t)
+	defer cancel()
+
+	spec := &specs.Spec{}
+	encoded, err := typeurl.MarshalAny(spec)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	spec.Annotations = map[string]string{"updated": "true"}
+	encodedUpdated, err := typeurl.MarshalAny(spec)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	for _, testcase := range []struct {
+		name       string
+		original   containers.Container
+		createerr  error
+		input      containers.Container
+		fieldpaths []string
+		expected   containers.Container
+		cause      error
+	}{
+		{
+			name: "UpdateIDFail",
+			original: containers.Container{
+				Spec:        encoded,
+				RootFS:      "test-rootfs",
+				Snapshotter: "snapshotter",
+				Runtime: containers.RuntimeInfo{
+					Name: "testruntime",
+				},
+			},
+			input: containers.Container{
+				ID:   "newid",
+				Spec: encoded,
+				Runtime: containers.RuntimeInfo{
+					Name: "testruntime",
+				},
+			},
+			fieldpaths: []string{"id"},
+			cause:      errdefs.ErrNotFound,
+		},
+		{
+			name: "UpdateRuntimeFail",
+			original: containers.Container{
+				RootFS:      "test-rootfs",
+				Snapshotter: "snapshotter",
+				Spec:        encoded,
+				Runtime: containers.RuntimeInfo{
+					Name: "testruntime",
+				},
+			},
+			input: containers.Container{
+				Spec: encoded,
+				Runtime: containers.RuntimeInfo{
+					Name: "testruntimedifferent",
+				},
+			},
+			fieldpaths: []string{"runtime"},
+			cause:      errdefs.ErrInvalidArgument,
+		},
+		{
+			name: "UpdateRuntimeClearFail",
+			original: containers.Container{
+				Spec:        encoded,
+				RootFS:      "test-rootfs",
+				Snapshotter: "snapshotter",
+				Runtime: containers.RuntimeInfo{
+					Name: "testruntime",
+				},
+			},
+			input: containers.Container{
+				Spec: encoded,
+			},
+			fieldpaths: []string{"runtime"},
+			cause:      errdefs.ErrInvalidArgument,
+		},
+		{
+			name: "UpdateFail",
+			original: containers.Container{
+				Spec:        encoded,
+				RootFS:      "test-rootfs",
+				Snapshotter: "snapshotter",
+
+				Runtime: containers.RuntimeInfo{
+					Name: "testruntime",
+				},
+				Image: "test image",
+			},
+			input: containers.Container{
+				Spec: encoded,
+				Runtime: containers.RuntimeInfo{
+					Name: "testruntime",
+				},
+				RootFS:      "test-rootfs",
+				Snapshotter: "snapshotter",
+				// try to clear image field
+			},
+			cause: errdefs.ErrInvalidArgument,
+		},
+		{
+			name: "UpdateSpec",
+			original: containers.Container{
+				Spec:        encoded,
+				RootFS:      "test-rootfs",
+				Snapshotter: "snapshotter",
+				Runtime: containers.RuntimeInfo{
+					Name: "testruntime",
+				},
+				Image: "test image",
+			},
+			input: containers.Container{
+				Spec: encodedUpdated,
+			},
+			fieldpaths: []string{"spec"},
+			expected: containers.Container{
+				Runtime: containers.RuntimeInfo{
+					Name: "testruntime",
+				},
+				Spec:        encodedUpdated,
+				RootFS:      "test-rootfs",
+				Snapshotter: "snapshotter",
+				Image:       "test image",
+			},
+		},
+		{
+			name: "UpdateLabel",
+			original: containers.Container{
+				Labels: map[string]string{
+					"foo": "one",
+					"bar": "two",
+				},
+				Spec:        encoded,
+				RootFS:      "test-rootfs",
+				Snapshotter: "snapshotter",
+				Runtime: containers.RuntimeInfo{
+					Name: "testruntime",
+				},
+				Image: "test image",
+			},
+			input: containers.Container{
+				Labels: map[string]string{
+					"bar": "baz",
+				},
+			},
+			fieldpaths: []string{"labels.bar"},
+			expected: containers.Container{
+				Labels: map[string]string{
+					"foo": "one",
+					"bar": "baz",
+				},
+				Spec:        encoded,
+				RootFS:      "test-rootfs",
+				Snapshotter: "snapshotter",
+				Runtime: containers.RuntimeInfo{
+					Name: "testruntime",
+				},
+				Image: "test image",
+			},
+		},
+		{
+			name: "DeleteAllLabels",
+			original: containers.Container{
+				Labels: map[string]string{
+					"foo": "one",
+					"bar": "two",
+				},
+				Spec:        encoded,
+				RootFS:      "test-rootfs",
+				Snapshotter: "snapshotter",
+				Runtime: containers.RuntimeInfo{
+					Name: "testruntime",
+				},
+				Image: "test image",
+			},
+			input: containers.Container{
+				Labels: nil,
+			},
+			fieldpaths: []string{"labels"},
+			expected: containers.Container{
+				Spec:        encoded,
+				RootFS:      "test-rootfs",
+				Snapshotter: "snapshotter",
+				Runtime: containers.RuntimeInfo{
+					Name: "testruntime",
+				},
+				Image: "test image",
+			},
+		},
+		{
+			name: "DeleteLabel",
+			original: containers.Container{
+				Labels: map[string]string{
+					"foo": "one",
+					"bar": "two",
+				},
+				Spec:        encoded,
+				RootFS:      "test-rootfs",
+				Snapshotter: "snapshotter",
+				Runtime: containers.RuntimeInfo{
+					Name: "testruntime",
+				},
+				Image: "test image",
+			},
+			input: containers.Container{
+				Labels: map[string]string{
+					"bar": "",
+				},
+			},
+			fieldpaths: []string{"labels.bar"},
+			expected: containers.Container{
+				Labels: map[string]string{
+					"foo": "one",
+				},
+				Spec:        encoded,
+				RootFS:      "test-rootfs",
+				Snapshotter: "snapshotter",
+				Runtime: containers.RuntimeInfo{
+					Name: "testruntime",
+				},
+				Image: "test image",
+			},
+		},
+		{
+			name: "UpdateRootFSImmutable",
+			original: containers.Container{
+				Spec:        encoded,
+				RootFS:      "",
+				Snapshotter: "",
+				Runtime: containers.RuntimeInfo{
+					Name: "testruntime",
+				},
+			},
+			input: containers.Container{
+				RootFS:      "something",
+				Snapshotter: "something",
+			},
+			fieldpaths: []string{"rootfs", "snapshotter"},
+			cause:      errdefs.ErrInvalidArgument,
+		},
+		{
+			name: "RootFSWithoutSnapshot",
+			original: containers.Container{
+				Spec:        encoded,
+				RootFS:      "/nosnapshot",
+				Snapshotter: "",
+				Runtime: containers.RuntimeInfo{
+					Name: "testruntime",
+				},
+			},
+			createerr: errdefs.ErrInvalidArgument,
+		},
+	} {
+		t.Run(testcase.name, func(t *testing.T) {
+			testcase.original.ID = testcase.name
+			if testcase.input.ID == "" {
+				testcase.input.ID = testcase.name
+			}
+			testcase.expected.ID = testcase.name
+
+			done := errors.New("test complete")
+			if err := db.Update(func(tx *bolt.Tx) error {
+				var (
+					now   = time.Now().UTC()
+					store = NewContainerStore(tx)
+				)
+
+				result, err := store.Create(ctx, testcase.original)
+				if errors.Cause(err) != testcase.createerr {
+					if testcase.createerr == nil {
+						t.Fatalf("unexpected error: %v", err)
+					} else {
+						t.Fatalf("cause of %v (cause: %v) != %v", err, errors.Cause(err), testcase.createerr)
+					}
+				} else if testcase.createerr != nil {
+					return done
+				}
+
+				checkContainerTimestamps(t, &result, now, true)
+
+				// ensure that createdat is never tampered with
+				testcase.original.CreatedAt = result.CreatedAt
+				testcase.expected.CreatedAt = result.CreatedAt
+				testcase.original.UpdatedAt = result.UpdatedAt
+				testcase.expected.UpdatedAt = result.UpdatedAt
+
+				checkContainersEqual(t, &result, &testcase.original, "unexpected result on container update")
+				return nil
+			}); err != nil {
+				if err == done {
+					return
+				}
+				t.Fatal(err)
+			}
+
+			if err := db.Update(func(tx *bolt.Tx) error {
+				now := time.Now()
+				store := NewContainerStore(tx)
+				result, err := store.Update(ctx, testcase.input, testcase.fieldpaths...)
+				if errors.Cause(err) != testcase.cause {
+					if testcase.cause == nil {
+						t.Fatalf("unexpected error: %v", err)
+					} else {
+						t.Fatalf("cause of %v (cause: %v) != %v", err, errors.Cause(err), testcase.cause)
+					}
+				} else if testcase.cause != nil {
+					return done
+				}
+
+				checkContainerTimestamps(t, &result, now, false)
+				testcase.expected.UpdatedAt = result.UpdatedAt
+				checkContainersEqual(t, &result, &testcase.expected, "updated failed to get expected result")
+				return nil
+			}); err != nil {
+				if err == done {
+					return
+				}
+				t.Fatal(err)
+			}
+
+			if err := db.View(func(tx *bolt.Tx) error {
+				store := NewContainerStore(tx)
+				result, err := store.Get(ctx, testcase.original.ID)
+				if err != nil {
+					t.Fatal(err)
+				}
+
+				checkContainersEqual(t, &result, &testcase.expected, "get after failed to get expected result")
+				return nil
+			}); err != nil {
+				t.Fatal(err)
+			}
+
+		})
+	}
+}
+
+func checkContainerTimestamps(t *testing.T, c *containers.Container, now time.Time, oncreate bool) {
+	if c.UpdatedAt.IsZero() || c.CreatedAt.IsZero() {
+		t.Fatalf("timestamps not set")
+	}
+
+	if oncreate {
+		if !c.CreatedAt.Equal(c.UpdatedAt) {
+			t.Fatal("timestamps should be equal on create")
+		}
+
+	} else {
+		// ensure that updatedat is always after createdat
+		if !c.UpdatedAt.After(c.CreatedAt) {
+			t.Fatalf("timestamp for updatedat not after createdat: %v <= %v", c.UpdatedAt, c.CreatedAt)
+		}
+	}
+
+	if c.UpdatedAt.Before(now) {
+		t.Fatal("createdat time incorrect should be after the start of the operation")
+	}
+}
+
+func checkContainersEqual(t *testing.T, a, b *containers.Container, format string, args ...interface{}) {
+	if !reflect.DeepEqual(a, b) {
+		t.Fatalf("containers not equal %v != %v: "+format, append([]interface{}{a, b}, args...)...)
+	}
+}
+
+func testEnv(t *testing.T) (context.Context, *bolt.DB, func()) {
+	ctx, cancel := context.WithCancel(context.Background())
+	ctx = namespaces.WithNamespace(ctx, "testing")
+
+	dirname, err := ioutil.TempDir("", t.Name()+"-")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	db, err := bolt.Open(filepath.Join(dirname, "meta.db"), 0644, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	return ctx, db, func() {
+		db.Close()
+		if err := os.RemoveAll(dirname); err != nil {
+			t.Log("failed removing temp dir", err)
+		}
+		cancel()
+	}
+}

services/snapshot/default_linux.go

@@ -1,5 +0,0 @@
-package snapshot
-
-const (
-	defaultSnapshotter = "overlayfs"
-)

services/snapshot/default_unix.go

@@ -1,7 +0,0 @@
-// +build darwin freebsd solaris
-
-package snapshot
-
-const (
-	defaultSnapshotter = "naive"
-)

services/snapshot/default_windows.go

@@ -1,5 +0,0 @@
-package snapshot
-
-const (
-	defaultSnapshotter = "windows"
-)

services/snapshot/service.go

@@ -20,11 +20,6 @@ import (
 	"google.golang.org/grpc"
 )
 
-type config struct {
-	// Default is the default snapshotter to use for the service
-	Default string `toml:"default,omitempty"`
-}
-
 func init() {
 	plugin.Register(&plugin.Registration{
 		Type: plugin.GRPCPlugin,
@@ -33,9 +28,6 @@ func init() {
 			plugin.SnapshotPlugin,
 			plugin.MetadataPlugin,
 		},
-		Config: &config{
-			Default: defaultSnapshotter,
-		},
 		Init: newService,
 	})
 }
@@ -43,9 +35,8 @@ func init() {
 var empty = &protoempty.Empty{}
 
 type service struct {
-	snapshotters           map[string]snapshot.Snapshotter
+	snapshotters map[string]snapshot.Snapshotter
-	defaultSnapshotterName string
+	publisher    events.Publisher
-	publisher              events.Publisher
 }
 
 func newService(ic *plugin.InitContext) (interface{}, error) {
@@ -62,26 +53,24 @@ func newService(ic *plugin.InitContext) (interface{}, error) {
 		snapshotters[name] = metadata.NewSnapshotter(md.(*bolt.DB), name, sn.(snapshot.Snapshotter))
 	}
 
-	cfg := ic.Config.(*config)
+	if len(snapshotters) == 0 {
-	_, ok := snapshotters[cfg.Default]
+		return nil, errors.Errorf("failed to create snapshotter service: no snapshotters loaded")
-	if !ok {
-		return nil, errors.Errorf("default snapshotter not loaded: %s", cfg.Default)
 	}
 
 	return &service{
-		snapshotters:           snapshotters,
+		snapshotters: snapshotters,
-		defaultSnapshotterName: cfg.Default,
+		publisher:    ic.Events,
-		publisher:              ic.Events,
 	}, nil
 }
 
 func (s *service) getSnapshotter(name string) (snapshot.Snapshotter, error) {
 	if name == "" {
-		name = s.defaultSnapshotterName
+		return nil, errdefs.ToGRPCf(errdefs.ErrInvalidArgument, "snapshotter argument missing")
 	}
+
 	sn, ok := s.snapshotters[name]
 	if !ok {
-		return nil, errors.Errorf("snapshotter not loaded: %s", name)
+		return nil, errdefs.ToGRPCf(errdefs.ErrInvalidArgument, "snapshotter not loaded: %s", name)
 	}
 	return sn, nil
 }

snapshot_test.go

@@ -15,7 +15,7 @@ func newSnapshotter(ctx context.Context, root string) (snapshot.Snapshotter, fun
 		return nil, nil, err
 	}
 
-	sn := client.SnapshotService("")
+	sn := client.SnapshotService(DefaultSnapshotter)
 
 	return sn, func() {
 		client.Close()

snapshotter_default_linux.go

@@ -0,0 +1,8 @@
+package containerd
+
+const (
+	// DefaultSnapshotter will set the default snapshotter for the platform.
+	// This will be based on the client compilation target, so take that into
+	// account when choosing this value.
+	DefaultSnapshotter = "overlayfs"
+)

snapshotter_default_unix.go

@@ -0,0 +1,10 @@
+// +build darwin freebsd solaris
+
+package containerd
+
+const (
+	// DefaultSnapshotter will set the default snapshotter for the platform.
+	// This will be based on the client compilation target, so take that into
+	// account when choosing this value.
+	DefaultSnapshotter = "naive"
+)

snapshotter_default_windows.go

@@ -0,0 +1,8 @@
+package containerd
+
+const (
+	// DefaultSnapshotter will set the default snapshotter for the platform.
+	// This will be based on the client compilation target, so take that into
+	// account when choosing this value.
+	DefaultSnapshotter = "windows"
+)

spec_opts_unix.go

@@ -233,6 +233,9 @@ func WithRemappedSnapshot(id string, i Image, uid, gid uint32) NewContainerOpts
 		if err != nil {
 			return err
 		}
+
+		setSnapshotterIfEmpty(c)
+
 		var (
 			snapshotter = client.SnapshotService(c.Snapshotter)
 			parent      = identity.ChainID(diffIDs).String()