Rewrite metadata store #66

Random-Liu · 2017-06-06T02:31:27Z

Previously, containerd only managed running container, so we have to do checkpoint to maintain complete lifecycle of a container and sandbox. That's why we introduced the metadata store, which was targeted to be a checkpoint store for container, sandbox and image if needed.

However, it seems that we should not rely on the metadata store now, because:

Containerd added container metadata store, and also supports container label, which could help us checkpoint constant data. We only need to maintain checkpoint of container status now.
The metadata store was originally designed for pure data checkpoint, it only supports update via Update function, which has some limitations:
a) Update function is not flexible enough, e.g. for image, we need CreateOrUpdate operation to create a metadata if it does not exsit, or update it if does.
b) Update and checkpoint doesn't make sense to some in-memory data we need to associate with the metadata lifecycle, e.g. containerd container/image client, cni netns object, stop container channel etc.
c) The whole metadata store logic is unnecessarily complex given that we don't need a general checkpoint solution now.

Thus I made this change. Relatively large, but help us avoid future pain.

Use container store as an example, the store is organized like this:

Store (a simple in-memory map)
   |--> Container (a collection of all resources associated with the container)
             | --> Metadata (constant metadata of the container, will be checkpointed in containerd container label)
             | --> Status (container status maintained in our own checkpoint, the only checkpoint we need now)
             | --> Containerd Container Client (container client of containerd, added soon)
             | --> Container StopCh (a channel indicates container is stopped or not, added soon)
             | --> More... (If needed in the future)

For sandbox, we don't need status.
For image, we don't event need a separate Metadata field, because containerd provided everything we need.

Random-Liu · 2017-06-06T06:37:05Z

CRI Validation test result is not changed with this PR:

Ran 36 of 36 Specs in 94.648 seconds
FAIL! -- 23 Passed | 13 Failed | 0 Pending | 0 Skipped --- FAIL: TestE2ECRI (94.65s)
FAIL

mikebrow · 2017-06-06T16:00:33Z

hack/verify-lint.sh

@@ -24,6 +24,7 @@ for d in $(find . -type d -a \( -iwholename './pkg*' -o -iwholename './cmd*' \))
 		 --exclude='.*_test\.go:.*error return value not checked.*\(errcheck\)$' \
 		 --exclude='duplicate of.*_test.go.*\(dupl\)$' \
 		 --exclude='.*/mock_.*\.go:.*\(golint\)$' \
+		 --exclude='declaration of "err" shadows declaration at.*\(vetshadow\)' \


Removing will introduce the possibility of programming errors with err vars.

The problem is that the check is known to be error prone. It sometimes report error and sometimes not, and I don't think something like:

if err := func1(); err != nil { } if err := func2(); err != nil { }

should be an error.

only with err? Or are you arguing for a different recommended pattern of use of variables in go? Should we always redeclare := go variables instead of assigning via =? What's the point of have var declaration and assignment as separate language features if we are only going to use declaration and assignment?

Only with err. I mean.

Obviously err := foo is just a shortcut for var err error = In your above use case you are repetitivly stating var err error, when all you are really doing is assigning with an =. In your use case the cleaner way is to declare first then just use err = since you don't like using := sometimes and = other times, yes?

I guess my question is then, why is err a special use case of variable declaration and assignment? I understand the thought for keeping each test independent but the variable redeclaration doesn't make the test more independent than using assignment. Symmetry?

Offline discussed. Keep the check and update the code.

mikebrow · 2017-06-06T16:38:02Z

When you refactor you really refactor! :-) Will review from the perspective of it being a complete replacement.

yujuhong · 2017-06-12T18:01:46Z

Will review today.

Random-Liu · 2017-06-12T20:51:53Z

@yujuhong Thanks~

yujuhong · 2017-06-12T23:39:55Z

pkg/store/container/container.go

+	}, nil
+}
+
+// LoadContainer loads the internal used container type.


nit: remove 'used' or s/internal used/internally used

yujuhong · 2017-06-13T15:47:14Z

pkg/store/container/container.go

+	// TODO(random-liu): Add stop channel to get rid of stop poll waiting.
+}
+
+// NewContainer creates a internal used container type.


nit: remove 'used' or s/internal used/internally used

yujuhong · 2017-06-13T15:54:20Z

pkg/store/container/container.go

+
+// NewStore creates a container store.
+func NewStore() *Store {
+	return &Store{containers: make(map[string]Container)}


I noticed that you pass around Container instead of *Container in the Store operations. Is this intentional to prevent callers from modifying the struct (which cannot work completely without deep copying)? This would lead to more memory allocation/copying, so just want to understand the use case.

I use Container rather than *Container because:

The value copy could prevent callers from mutating the struct fields in some way, because most fields are primitive types.

Most importantly, when caller using the api, usually they won't expect to be able to mutate fields of a returned value, but may expect to mutate a returned pointer.

As for the overhead, metadata is small, so there shouldn't be too much overhead. :)

yujuhong · 2017-06-13T16:13:44Z

pkg/store/container/status.go

+}
+
+// Delete deletes the container status from disk atomically.
+func (m *statusStorage) Delete() error {


Why can't we delete m.status in this function? It looks like Delete() is only meaningful if we back the data on the disk.

It doesn't matter whether we delete m.status or not, because once the container is removed from container list, no one should be able to retrieve and access it.

Delete m.status will only cause race condition between operations which already get the reference.

yujuhong · 2017-06-13T16:18:07Z

pkg/store/image/image.go

+
+// NewStore creates an image store.
+func NewStore() *Store {
+	return &Store{images: make(map[string]Image)}


Same question about the rationale of storing Image vs *Image

Same with above.

yujuhong · 2017-06-13T16:19:58Z

pkg/store/image/image.go

+)
+
+// Image contains all resources associated with the image. All fields
+// are **read-only**.


Is that true? In Add(), you merge RepoTags and RepoDigest and write it back to the store.

Will update the comment to "All fields MUST not be mutated directly".

Can we do this change universally on the other read only comments?

yujuhong · 2017-06-13T16:24:15Z

pkg/store/container/container.go

+)
+
+// Container contains all resources associated with the container. All fields of
+// Container are thread safe.


I'm not sure how fields can be thread-safe. I think you mean the methods to mutate the status are thread-safe?

Yeah, thanks. Will update the comment.

yujuhong · 2017-06-13T16:25:27Z

pkg/store/sandbox/sandbox.go

+// Store stores all sandboxes.
+type Store struct {
+	lock      sync.RWMutex
+	sandboxes map[string]Sandbox


Same question about storing pointers vs. structs.

Random-Liu · 2017-06-16T17:06:00Z

Will rebase this and address comments.

Random-Liu · 2017-06-20T17:07:11Z

Moved to v0.2.0, because this PR mainly:

Eases the work to switching to new containerd client, and add permanent network namespace.
Makes it possible to checkpoint most metadata in container label.
Makes it easier to make future change.

There is no new feature introduced.

Since we don't have time to finish the containerd client switch and permanent network namespace work. Let's also punt this one to v0.2.0.

Random-Liu · 2017-07-28T04:11:29Z

@yujuhong Rebased the PR. Could you help me take another look? Maybe focus on the first commit.

@mikebrow Could you help me review this PR?

mikebrow

Please see questions and comments.

mikebrow · 2017-07-28T18:16:53Z

pkg/metadata/container.go

@@ -1,183 +0,0 @@
-/*


Suggest rename from refactor metadata store to rewrite metadata store. Or merge the delete and create commits into one so we can see the diff.

You won't see the diff even after I merge those 2. Will change to "rewrite".

mikebrow · 2017-07-28T18:35:43Z

pkg/server/container_create.go

-		return nil, fmt.Errorf("failed to add container metadata %+v into store: %v",
-			meta, err)
+	if err := c.containerStore.Add(container); err != nil {
+		return nil, fmt.Errorf("failed to add container %q into store: %v", id, err)


Good.. like the pattern using new() (container, error) then calling add with container vs using .create(meta). But it's strange that you had to defer the container.Status.Delete for a container that never gets added to the store?

I see the storing of the status in the NewContainer() call but don't understand why the status is stored before the container. Is that to make sure the container always has status? maybe the container store.add could be changed to add container with status to make it more atomic?

Add need to lock the container list. I don't think we want to lock the list and do some disk operation.

Done. Added Container.Delete function.

mikebrow · 2017-07-28T18:39:35Z

pkg/server/container_execsync.go

@@ -45,15 +45,16 @@ func (c *criContainerdService) ExecSync(ctx context.Context, r *runtime.ExecSync
 		}
 	}()

-	// Get container metadata from our container store.
-	meta, err := c.containerStore.Get(r.GetContainerId())
+	// Get container from our container store.


like the comment but the naming might get confusing down the road over the two container stores, ours and containerd's.

Yeah, that's a problem... :(

mikebrow · 2017-07-28T18:42:27Z

pkg/server/container_list.go


 	var containers []*runtime.Container
-	for _, meta := range metas {
-		containers = append(containers, toCRIContainer(meta))
+	for _, container := range containersInStore {


I think this is why containerd came up with the name task/tasks.

What do you mean here? :)

you refactored from range metas to range containersInStore presumably cause you didn't like the term meta... and didn't have a better option than just calling the data container again... :-)

mikebrow · 2017-07-28T18:59:24Z

pkg/server/sandbox_run.go

-	if err := c.sandboxStore.Create(meta); err != nil {
-		return nil, fmt.Errorf("failed to add sandbox metadata %+v into store: %v",
-			meta, err)
+	sandbox.CreatedAt = time.Now().UnixNano()


defer delete for state of sandbox?

We don't maintain state of sandbox. We infer that directly from sandbox container state.

We could not do that for container because we also need exit code, finished timestamp etc. which we have to checkpoint ourselves. But for sandbox, we only care about whether it is running or not.

ok yeah been having some debug issues on the update PR for sandboxes that have a messed up state (can't be stopped) then later on can't be created cause /pause isn't there.. the rootfs is messed up.

I'll help debug your PR over the weekend. :)

mikebrow · 2017-07-28T19:21:37Z

pkg/store/container/container.go

+	Metadata
+	// Status stores the status of the container.
+	Status StatusStorage
+	// TODO(random-liu): Add containerd container client.


I mean in the future, we could this here, after we start to use containerd client.

mikebrow · 2017-07-28T19:23:36Z

pkg/store/container/metadata.go

+)
+
+// NOTE(random-liu):
+// 1) Metadata is read-only.


? you mean can't be updated after create?

Yeah, will update.

mikebrow · 2017-07-28T19:26:17Z

pkg/store/container/metadata.go

+
+// NOTE(random-liu):
+// 1) Metadata is read-only.
+// 2) Metadata is checkpointed as containerd container label.


just confirming we'll use labels not annotations for all metadata so we can find the container by the metadata labels or... maybe some combination of labels and annotations...?

containerd only has label, there is no notion of annotation in containerd.

And we store metadata into label just for checkpoint, not for filtering.

oci will have the annotations.. and I suspect containerd will let them pass through without providing first class support.. I was thinking about image labels.. my bad.

mikebrow · 2017-07-28T19:31:15Z

pkg/store/image/image.go

+)
+
+// Image contains all resources associated with the image. All fields
+// are **read-only**.


Can we do this change universally on the other read only comments?

Signed-off-by: Lantao Liu <lantaol@google.com>

Random-Liu · 2017-07-28T23:37:33Z

@mikebrow Addressed comments.

The code could be cleaned up more, especially when we start to use containerd client. We could do that in the future.

mikebrow

LGTM

Rewrite metadata store

Respect OCI image config default username

Random-Liu added this to the v0.1.0-alpha.1 milestone Jun 6, 2017

Random-Liu assigned yujuhong and mikebrow Jun 6, 2017

k8s-ci-robot added the cncf-cla: yes label Jun 6, 2017

Random-Liu force-pushed the refactor-metadata-store branch from 6a918d5 to 22d4d80 Compare June 6, 2017 06:38

This was referenced Jun 6, 2017

Add permanent netns #54

Closed

CRI-Containerd Missing Pieces #62

Closed

mikebrow reviewed Jun 6, 2017

View reviewed changes

Random-Liu force-pushed the refactor-metadata-store branch 3 times, most recently from 40aba30 to ea21c3c Compare June 6, 2017 22:32

yujuhong reviewed Jun 13, 2017

View reviewed changes

Random-Liu modified the milestones: v0.2.0, v0.1.0-alpha.1 Jun 20, 2017

Random-Liu force-pushed the refactor-metadata-store branch 2 times, most recently from 3847f17 to 454280a Compare July 28, 2017 03:52

Random-Liu force-pushed the refactor-metadata-store branch from 454280a to 82f82a8 Compare July 28, 2017 17:02

mikebrow reviewed Jul 28, 2017

View reviewed changes

Random-Liu changed the title ~~Refactor metadata store~~ Rewrite metadata store Jul 28, 2017

Random-Liu added 4 commits July 28, 2017 23:35

Add new metadata store.

a393f3a

Signed-off-by: Lantao Liu <lantaol@google.com>

Remove sandbox truncindex.

4317e61

Signed-off-by: Lantao Liu <lantaol@google.com>

Use new metadata store.

7b16a35

Signed-off-by: Lantao Liu <lantaol@google.com>

Remove old metadata store.

f4df66e

Signed-off-by: Lantao Liu <lantaol@google.com>

Random-Liu force-pushed the refactor-metadata-store branch from 82f82a8 to f4df66e Compare July 28, 2017 23:35

mikebrow approved these changes Jul 29, 2017

View reviewed changes

mikebrow added the lgtm label Jul 29, 2017

Random-Liu merged commit cbd936b into containerd:master Jul 31, 2017

Random-Liu deleted the refactor-metadata-store branch July 31, 2017 05:01

lanchongyizu pushed a commit to lanchongyizu/cri-containerd that referenced this pull request Sep 3, 2017

Merge pull request containerd#66 from Random-Liu/refactor-metadata-store

2965213

Rewrite metadata store

dcantah pushed a commit to dcantah/cri that referenced this pull request Jun 12, 2020

Merge pull request containerd#66 from dcantah/image-default-user

c4757bb

Respect OCI image config default username

Rewrite metadata store #66

Rewrite metadata store #66

Conversation

Random-Liu commented Jun 6, 2017 • edited

Random-Liu commented Jun 6, 2017

Choose a reason for hiding this comment

Random-Liu Jun 6, 2017 • edited

Choose a reason for hiding this comment

mikebrow Jun 6, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikebrow commented Jun 6, 2017

yujuhong commented Jun 12, 2017

Random-Liu commented Jun 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Random-Liu commented Jun 16, 2017

Random-Liu commented Jun 20, 2017 • edited

Random-Liu commented Jul 28, 2017

mikebrow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikebrow Jul 28, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Random-Liu Jul 28, 2017 • edited

Choose a reason for hiding this comment

mikebrow Jul 28, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Random-Liu commented Jul 28, 2017

mikebrow left a comment

Choose a reason for hiding this comment

Random-Liu commented Jun 6, 2017 •

edited

Random-Liu Jun 6, 2017 •

edited

mikebrow Jun 6, 2017 •

edited

Random-Liu commented Jun 20, 2017 •

edited

mikebrow Jul 28, 2017 •

edited

Random-Liu Jul 28, 2017 •

edited

mikebrow Jul 28, 2017 •

edited