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 <stephen.day@docker.com>

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 {

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 {

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()
+	}
+}