From 783ed050578523526945e3bb0f23d103a45010da Mon Sep 17 00:00:00 2001 From: Stephen J Day Date: Tue, 15 Aug 2017 18:24:02 -0700 Subject: [PATCH] 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 | 99 +++++-- metadata/containers_test.go | 557 ++++++++++++++++++++++++++++++++++++ 4 files changed, 688 insertions(+), 48 deletions(-) create mode 100644 metadata/containers_test.go diff --git a/containers/containers.go b/containers/containers.go index b5c262d26ab2..cc44f2bbfe13 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 4bcfb203cd55..d66c172775c2 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 15b2936dacc3..7e0e7a68add7 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 + 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) - } + 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) } - } else { - // no field mask present, just replace everything - updated = container + } + + 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 000000000000..f5fa6bd01b80 --- /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() + } +}