Switch to containerd extension #260

yanxuean · 2017-09-19T07:55:37Z

Signed-off-by: yanxuean [email protected]

yanxuean · 2017-09-19T07:57:22Z

wait to rebase after #209 is merged.

ijc · 2017-09-19T10:30:43Z

pkg/server/helpers.go

 	// sandboxMetadataLabel is label name that identify metadata of sandbox in CreateContainerRequest
 	sandboxMetadataLabel = "io.cri-containerd.sandbox.metadata"
-	// sandboxMetadataLabel is label name that identify metadata of container in CreateContainerRequest
+	// containerMetadataLabel is label name that identify metadata of container in CreateContainerRequest


it's not a label name any more, so perhaps "...is an extension name..."?

BTW The "sandboxMetadataLabel is label name" should really be "sandboxMetadataLabel is a label name".

done. Thanks for review

Random-Liu · 2017-09-19T22:51:44Z

pkg/server/container_create.go

 		containerd.WithSpec(spec, specOpts...),
 		containerd.WithRuntime(defaultRuntime, nil),
-		containerd.WithContainerLabels(labels))
+		containerd.WithContainerExtension(containerMetadataExtension, &meta))


We still need to handle versioning ourselves, I think.

Containerd is using json marshal/unmarshal for non-proto message now. Could we update Encode/Decode in container/sandbox metadata and container status to MarshalJSON, and UnmarshalJSON? So that golang will call our customized marshal/unmarshal functions which should automatically handle versioning.

And please add unit test to make sure typeurl.Marshal and typeurl.Unmarshal does properly handle the versioning.

Random-Liu · 2017-09-19T22:52:57Z

pkg/server/helpers.go

+	// sandboxMetadataExtension is a extension name that identify metadata of sandbox in CreateContainerRequest
+	sandboxMetadataExtension = "io.cri-containerd.sandbox.metadata"
+	// containerMetadataExtension is a extension name that identify metadata of container in CreateContainerRequest
+	containerMetadataExtension = "io.cri-containerd.container.metadata"


We still need a label to select containers and sandboxes.

Can we add a label containerKindLabel = "io.cri-containerd.kind"? And we should put "sandbox" on sandbox container, and "container" on application container.

What do you mean? Should we use containerKindLabel but containerMetadataExtension? @Random-Liu

I mean instead of extension, we still need a label to indicate whether a container is sandbox container or regular container.

We could add a label io.cri-containerd.kind on each container, io.cri-containerd.kind=sandbox means it is a sandbox container, io.cri-containerd.kind=container means it is a regular container.

Do you mean we shoule add both lable and extension?

For example

opts := []containerd.NewContainerOpts{ containerd.WithSnapshotter(c.config.ContainerdSnapshotter), containerd.WithNewSnapshot(id, image.Image), containerd.WithSpec(spec, specOpts...), containerd.WithContainerLabels(map[string]string{containerKindLabel: "sandbox"}) containerd.WithContainerExtension(sandboxMetadataExtension, &sandbox.Metadata), containerd.WithRuntime(defaultRuntime, nil)} container, err := c.client.NewContainer(ctx, id, opts...)

Random-Liu · 2017-09-19T22:55:06Z

pkg/server/container_create.go

 )

+func init() {
+	typeurl.Register(&containerstore.Metadata{}, "github.com/kubernetes-incubator/cri-containerd/pkg/store/container", "Metadata")


nit: Line too long, add new line?

Random-Liu · 2017-09-19T22:58:14Z

pkg/server/sandbox_run.go

 )

+func init() {
+	typeurl.Register(&sandboxstore.Metadata{}, "github.com/kubernetes-incubator/cri-containerd/pkg/store/sandbox", "Metadata")


Random-Liu · 2017-09-20T06:35:21Z

pkg/store/container/metadata.go

-	Version string
-	Metadata
+	Version  string
+	Metadata metadataInternal


Why do we need this?

There is a recursive call if we use Metadata in versionedMetadata derectly.

Make sense.

Random-Liu · 2017-09-20T09:17:52Z

pkg/store/container/metadata_test.go

+	data, err := json.Marshal(meta)
+	assert.NoError(err)
+	assert.NoError(json.Unmarshal(data, newMeta))
+	assert.Equal(meta, newMeta)


We don't know whether MarshalJSON or UnmarshalJSON is actually called during this.
The test is a little bit confusing.

How about this?

Your first case. (verify that data could be stored and recovered)

Make sure json.Marshal(Metadata) and json.Marshal(versionedMetadata) get the same bytes result; (verify that data is stored as versioned data, case1) has proved that versioned data could be recovered)

json.Marshal(unsupportedVersionedMetadata) and then json.Unmarshal(data) should get an error; (verify that unsupported versioned data won't be unmarshalled)

We may want to add some t.Logf to make the test more clear.

Random-Liu · 2017-09-20T09:19:39Z

LGTM overall with a comment for the test.
Please update the test, and I'll merge this tomorrow! @yanxuean Thanks for working on this.

abhi · 2017-09-20T14:58:25Z

pkg/server/helpers.go

-	// sandboxMetadataLabel is label name that identify metadata of container in CreateContainerRequest
-	containerMetadataLabel = "io.cri-containerd.container.metadata"
+	// containerKindLabel is a label key indicating container is sandbox container or application container
+	containerKindLabel = "io.cri-containerd.kind"


minor nit: Can we add a constant prefix = "io.cri-containerd"
And then use this for all the constants ?
for eg: containerKindLabel= prefix + "kind"

fix containerd#251 Signed-off-by: yanxuean <[email protected]>

Random-Liu · 2017-09-20T17:36:32Z

LGTM

k8s-ci-robot added size/M cncf-cla: yes labels Sep 19, 2017

yanxuean force-pushed the use-containerd-extension branch from bd64262 to 76154bc Compare September 19, 2017 08:10

ijc reviewed Sep 19, 2017

View reviewed changes

yanxuean force-pushed the use-containerd-extension branch from 76154bc to e222056 Compare September 19, 2017 10:53

Random-Liu reviewed Sep 19, 2017

View reviewed changes

Random-Liu self-assigned this Sep 19, 2017

Random-Liu added this to the v1.0.0-alpha.0 milestone Sep 19, 2017

yanxuean force-pushed the use-containerd-extension branch from e222056 to 4479718 Compare September 20, 2017 06:32

k8s-ci-robot added size/L and removed size/M labels Sep 20, 2017

Random-Liu reviewed Sep 20, 2017

View reviewed changes

yanxuean force-pushed the use-containerd-extension branch 4 times, most recently from 461969c to ee4667a Compare September 20, 2017 08:03

Random-Liu reviewed Sep 20, 2017

View reviewed changes

abhi reviewed Sep 20, 2017

View reviewed changes

Switch to containerd extension

e1a7a0e

fix containerd#251 Signed-off-by: yanxuean <[email protected]>

yanxuean force-pushed the use-containerd-extension branch from ee4667a to e1a7a0e Compare September 20, 2017 16:23

Random-Liu added the lgtm label Sep 20, 2017

Random-Liu merged commit 5dbba59 into containerd:master Sep 20, 2017

yanxuean deleted the use-containerd-extension branch September 21, 2017 02:23

Switch to containerd extension #260

Switch to containerd extension #260

Uh oh!

Conversation

yanxuean commented Sep 19, 2017

Uh oh!

yanxuean commented Sep 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yanxuean Sep 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yanxuean Sep 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Random-Liu commented Sep 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Random-Liu commented Sep 20, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

yanxuean Sep 19, 2017 •

edited

Loading

yanxuean Sep 20, 2017 •

edited

Loading

Random-Liu commented Sep 20, 2017 •

edited

Loading