From 783ed050578523526945e3bb0f23d103a45010da Mon Sep 17 00:00:00 2001 From: Stephen J Day Date: Tue, 15 Aug 2017 18:24:02 -0700 Subject: [PATCH 1/2] metadata: ensure correct updates on Container This fixes a few bugs in the container store related to reading and writing fields. Specifically, on update, the full field set wasn't being returned to the caller, making it appear that the store was corrupted. We now return the correctly updated field and store the missing field that was omitted in the original implementation. In course, we also have defined the update semantics of each field, as well as whether or not they are required. The big addition here is really the container metadata testsuite. It covers listing, filtering, creates, updates and deletes in a vareity of scenarios. Signed-off-by: Stephen J Day --- containers/containers.go | 57 +++- metadata/buckets.go | 23 +- metadata/containers.go | 101 +++++-- metadata/containers_test.go | 557 ++++++++++++++++++++++++++++++++++++ 4 files changed, 689 insertions(+), 49 deletions(-) create mode 100644 metadata/containers_test.go diff --git a/containers/containers.go b/containers/containers.go index b5c262d26..cc44f2bbf 100644 --- a/containers/containers.go +++ b/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 - Labels map[string]string - Image string - Runtime RuntimeInfo - Spec *types.Any - RootFS string + // ID uniquely identifies the container in a nameapace. + // + // This property is required and cannot be changed after creation. + ID 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 } diff --git a/metadata/buckets.go b/metadata/buckets.go index 4bcfb203c..d66c17277 100644 --- a/metadata/buckets.go +++ b/metadata/buckets.go @@ -38,17 +38,18 @@ var ( bucketKeyObjectBlob = []byte("blob") // stores content links bucketKeyObjectIngest = []byte("ingest") // stores ingest links - bucketKeyDigest = []byte("digest") - bucketKeyMediaType = []byte("mediatype") - bucketKeySize = []byte("size") - bucketKeyImage = []byte("image") - bucketKeyRuntime = []byte("runtime") - bucketKeyName = []byte("name") - bucketKeyParent = []byte("parent") - bucketKeyOptions = []byte("options") - bucketKeySpec = []byte("spec") - bucketKeyRootFS = []byte("rootfs") - bucketKeyTarget = []byte("target") + bucketKeyDigest = []byte("digest") + bucketKeyMediaType = []byte("mediatype") + bucketKeySize = []byte("size") + bucketKeyImage = []byte("image") + bucketKeyRuntime = []byte("runtime") + bucketKeyName = []byte("name") + bucketKeyParent = []byte("parent") + bucketKeyOptions = []byte("options") + bucketKeySpec = []byte("spec") + bucketKeyRootFS = []byte("rootfs") + bucketKeySnapshotter = []byte("snapshotter") + bucketKeyTarget = []byte("target") ) func getBucket(tx *bolt.Tx, keys ...[]byte) *bolt.Bucket { diff --git a/metadata/containers.go b/metadata/containers.go index 15b2936da..7e0e7a68a 100644 --- a/metadata/containers.go +++ b/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 { - return containers.Container{}, err + if err := validateContainer(&container); err != nil { + 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 { - // TODO(stevvooe): Move this logic into the store itself. - for _, path := range fieldpaths { - if strings.HasPrefix(path, "labels.") { - 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) + for _, path := range fieldpaths { + if strings.HasPrefix(path, "labels.") { + if updated.Labels == nil { + updated.Labels = map[string]string{} } + key := strings.TrimPrefix(path, "labels.") + updated.Labels[key] = container.Labels[key] + continue } - } else { - // no field mask present, just replace everything - updated = container + + switch path { + 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 { diff --git a/metadata/containers_test.go b/metadata/containers_test.go new file mode 100644 index 000000000..f5fa6bd01 --- /dev/null +++ b/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() + } +} From 677257f032d9d67d13b4fd0007bd2d45aa0d5b74 Mon Sep 17 00:00:00 2001 From: Stephen J Day Date: Fri, 18 Aug 2017 14:40:41 -0700 Subject: [PATCH 2/2] service/snapshotter: move default to client In order to enforce strict handling of snapshotter values on the container object, the defaults have been moved to the client side. This ensures that we correctly qualify the snapshotter under use when from the container at the time it was created, rather than possibly losing the metadata on a change of default. Signed-off-by: Stephen J Day --- client.go | 1 + container.go | 4 ++++ container_opts.go | 16 +++++++++++++++ container_opts_unix.go | 1 + services/snapshot/default_linux.go | 5 ----- services/snapshot/default_unix.go | 7 ------- services/snapshot/default_windows.go | 5 ----- services/snapshot/service.go | 29 +++++++++------------------- snapshot_test.go | 2 +- snapshotter_default_linux.go | 8 ++++++++ snapshotter_default_unix.go | 10 ++++++++++ snapshotter_default_windows.go | 8 ++++++++ spec_opts_unix.go | 3 +++ 13 files changed, 61 insertions(+), 38 deletions(-) delete mode 100644 services/snapshot/default_linux.go delete mode 100644 services/snapshot/default_unix.go delete mode 100644 services/snapshot/default_windows.go create mode 100644 snapshotter_default_linux.go create mode 100644 snapshotter_default_unix.go create mode 100644 snapshotter_default_windows.go diff --git a/client.go b/client.go index d0a57199c..65c28b8b3 100644 --- a/client.go +++ b/client.go @@ -192,6 +192,7 @@ func defaultRemoteContext() *RemoteContext { Resolver: docker.NewResolver(docker.ResolverOptions{ Client: http.DefaultClient, }), + Snapshotter: DefaultSnapshotter, } } diff --git a/container.go b/container.go index 9eb66adef..9d74a9932 100644 --- a/container.go +++ b/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 { diff --git a/container_opts.go b/container_opts.go index 8140c7889..8e90be36c 100644 --- a/container_opts.go +++ b/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 + } +} diff --git a/container_opts_unix.go b/container_opts_unix.go index be69500bd..73e5486bd 100644 --- a/container_opts_unix.go +++ b/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 diff --git a/services/snapshot/default_linux.go b/services/snapshot/default_linux.go deleted file mode 100644 index 13f75af41..000000000 --- a/services/snapshot/default_linux.go +++ /dev/null @@ -1,5 +0,0 @@ -package snapshot - -const ( - defaultSnapshotter = "overlayfs" -) diff --git a/services/snapshot/default_unix.go b/services/snapshot/default_unix.go deleted file mode 100644 index 3489cf8cf..000000000 --- a/services/snapshot/default_unix.go +++ /dev/null @@ -1,7 +0,0 @@ -// +build darwin freebsd solaris - -package snapshot - -const ( - defaultSnapshotter = "naive" -) diff --git a/services/snapshot/default_windows.go b/services/snapshot/default_windows.go deleted file mode 100644 index 680631e32..000000000 --- a/services/snapshot/default_windows.go +++ /dev/null @@ -1,5 +0,0 @@ -package snapshot - -const ( - defaultSnapshotter = "windows" -) diff --git a/services/snapshot/service.go b/services/snapshot/service.go index 6c6ef8818..293a643e3 100644 --- a/services/snapshot/service.go +++ b/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 - defaultSnapshotterName string - publisher events.Publisher + snapshotters map[string]snapshot.Snapshotter + 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) - _, ok := snapshotters[cfg.Default] - if !ok { - return nil, errors.Errorf("default snapshotter not loaded: %s", cfg.Default) + if len(snapshotters) == 0 { + return nil, errors.Errorf("failed to create snapshotter service: no snapshotters loaded") } return &service{ - snapshotters: snapshotters, - defaultSnapshotterName: cfg.Default, - publisher: ic.Events, + snapshotters: snapshotters, + 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 } diff --git a/snapshot_test.go b/snapshot_test.go index d52bc79cc..ccf28c70c 100644 --- a/snapshot_test.go +++ b/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() diff --git a/snapshotter_default_linux.go b/snapshotter_default_linux.go new file mode 100644 index 000000000..c432a2d55 --- /dev/null +++ b/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" +) diff --git a/snapshotter_default_unix.go b/snapshotter_default_unix.go new file mode 100644 index 000000000..cb8b08ac1 --- /dev/null +++ b/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" +) diff --git a/snapshotter_default_windows.go b/snapshotter_default_windows.go new file mode 100644 index 000000000..3b7358218 --- /dev/null +++ b/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" +) diff --git a/spec_opts_unix.go b/spec_opts_unix.go index bf90577ff..3e35c18c8 100644 --- a/spec_opts_unix.go +++ b/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()