Add `Container` field to store client-defined data by cpuguy83 · Pull Request #1378 · containerd/containerd

cpuguy83 · 2017-08-17T19:54:02Z

This field allows a client to store specialized information in the
container metadata rather than having to store this itself and keep
the data in sync with containerd.

mlaventure · 2017-08-17T20:59:18Z

aren't labels sufficient for this?

cpuguy83 · 2017-08-17T21:07:09Z

I don't think map<string,string> would work well for this.

stevvooe · 2017-08-17T21:08:23Z

I think extensions would be okay,

stevvooe · 2017-08-17T21:09:56Z

@dmcgowan Any reason these weren't a part of the previous PR? Did we have a bad merge? Basically, it looks like we merged around the same time and missed fields.

@cpuguy83 I'm going to update these in a separate PR.

Random-Liu · 2017-08-17T23:56:03Z

@cpuguy83 What kind of data in your mind could not put into label? Could you give an example? Thanks! :)

cpuguy83 · 2017-08-18T00:10:51Z

@Random-Liu My specific case would be for https://github.com/cpuguy83/docker/commit/4e7df4ae4d948789bbd48682ebf1f21b6288fb14##commitcomment-23677689

cpuguy83 · 2017-08-18T00:18:55Z

Mainly it allows us to store the spec that we created the container from (e.g. docker API) directly with the container in containerd rather than having this split up in multiple metadata stores.

Random-Liu · 2017-08-18T00:20:47Z

@cpuguy83 In Kubernetes containerd integration, we also need to put stuff (CRI container config etc.) inside container metadata store. Currently our plan is to serialize them and put them inside a label. :p

Won't that work?

cpuguy83 · 2017-08-18T00:27:17Z

I would like to not lose type information, especially since we may (definitely) want to update this.
Serializing and throwing into a string field doesn't seem very elegant. Honestly I think I'd prefer to manage a separate store in that case.

Random-Liu · 2017-08-18T00:51:02Z

I would like to not lose type information, especially since we may (definitely) want to update this.

@cpuguy83 Do you mean that you'll update it after creating container? Or the spec will be updated across releases?

Serializing and throwing into a string field doesn't seem very elegant. Honestly I think I'd prefer to manage a separate store in that case.

We were planning to do that because we don't have other options at that time. :) Actually this also benefits us.

cpuguy83 · 2017-08-18T00:53:40Z

@cpuguy83 Do you mean that you'll update it after creating container? Or the spec will be updated across version?

Between versions.

We were planning to do that because we don't have other options at that time. :) Actually this also benefits us.

👍 Totally it seems worth while to do something here to reduce the burden for implementers.

cpuguy83 · 2017-08-18T13:39:33Z

Updated to use extensions.
Looks like the test suite timed out, though.

mlaventure · 2017-08-18T14:49:10Z

It looks like there's a deadlock in the linux runtime, those 4 tests are stuck on a call to the task service:

TestContainerExecNoBinaryExists
TestContainerKill
TestUserNamespaces
TestContainerExec

crosbymichael · 2017-08-18T20:52:46Z

Can you rebase on master to pick up the fix?

codecov-io · 2017-08-18T21:02:02Z

Codecov Report

Merging #1378 into master will increase coverage by 0.03%.
The diff coverage is 44.44%.

@@            Coverage Diff             @@
##           master    #1378      +/-   ##
==========================================
+ Coverage   40.82%   40.85%   +0.03%     
==========================================
  Files          23       23              
  Lines        2920     2964      +44     
==========================================
+ Hits         1192     1211      +19     
- Misses       1451     1466      +15     
- Partials      277      287      +10

Impacted Files	Coverage Δ
metadata/buckets.go	`46.66% <ø> (ø)`	⬆️
metadata/containers.go	`46.59% <44.44%> (-0.61%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 52fbc5f...f7d31e2. Read the comment docs.

cpuguy83 · 2017-08-21T13:55:32Z

Would it make sense to have this for Image as well?

mlaventure · 2017-08-21T14:13:17Z

This is missing the code in the metadata snapshotter to actually store the data in extension 😇

cpuguy83 · 2017-08-21T16:50:36Z

My mistake, assumed it was using the proto.
I've updated with I think everything that's needed, added a test for it.

mlaventure · 2017-08-21T17:46:45Z

could you also add a a test where you load the container and check if the data is correctly sent back?

Also, don't forget to clean up after the test, this need a container.Delete() call.

NOTE: just do it within that same test, no need for a new one

stevvooe · 2017-08-31T00:21:50Z

This should be repeated.

stevvooe · 2017-08-31T00:22:02Z

"Extension"

cpuguy83 · 2017-09-07T14:01:55Z

Ok, this is updated.
Please take a look at how I'm encoding/decoding the []types.Any in the store.

crosbymichael · 2017-09-07T14:38:13Z

Could this take an interface and we do the any marshal in the opt?

mlaventure · 2017-09-07T20:22:35Z

This will require the urls to be registered with typeurl.Register unless they are proto messages. May want to specify so in the godoc

This field allows a client to store specialized information in the container metadata rather than having to store this itself and keep the data in sync with containerd. Signed-off-by: Brian Goff <[email protected]>

mlaventure · 2017-09-07T21:10:46Z

+// Use this to decorate the container object with additional data for the client
+// integration.
+//
+// Make sure to register the type of `extension` in the typeurl package via


Sorry, I should have been more clear 😅

If the url was not registered via typeurl.Register and the message doesn't implement protobuf/proto.Message this will error out :)

We should probably be able to support json here, as well.

stevvooe · 2017-09-07T21:44:40Z

I think this design is fine, but I'm worried that we are going to make it hard to manage updates that involve more than one extensions (I know, my fault for suggesting repeated... ;) ). The problem here is that one needs to read all the extensions, then make the modification for their extension, then send back all the extensions. This model will also allow for duplicates.

Would it be better to have a map where one can update only the extension they care about by specifying the key for the fieldpath? For example, you could update the extensions.docker fieldpath without affecting other extensions.

cpuguy83 · 2017-09-07T22:16:25Z

I think a map is a good idea. It also lets us treat the extensions like fields and will be more straight forward to encode in the store.

stevvooe · 2017-09-08T21:54:37Z

Moved this to a map type. PTAL

To allow for updating extensions without collisions, we have moved to using a map type that can be explicitly selected via the field path for updates. This ensures that multiple parties can operate on their extensions without stepping on each other's toes or incurring an inordinate number of round trips. Signed-off-by: Stephen J Day <[email protected]>

crosbymichael · 2017-09-12T18:10:46Z

LGTM

Random-Liu · 2017-09-15T21:43:02Z

@crosbymichael @stevvooe Can we get this in before the end of September (our 1.0.0-alpha.0 release)? We want to switch to Extension before that to avoid backward compatibility issue in the future. :)

mlaventure

LGTM

cpuguy83 · 2017-09-18T17:20:34Z

Sweet, thanks for carrying this through!

stevvooe reviewed Aug 17, 2017

View reviewed changes

cpuguy83 force-pushed the container_store_extra_data branch from 2b014cc to b8e8884 Compare August 18, 2017 00:42

cpuguy83 force-pushed the container_store_extra_data branch from b8e8884 to 84dc48a Compare August 18, 2017 00:51

cpuguy83 force-pushed the container_store_extra_data branch 2 times, most recently from 5fa1f4b to 761841c Compare August 18, 2017 00:56

Random-Liu mentioned this pull request Aug 18, 2017

Checkpoint and restart recovery containerd/cri#120

Closed

cpuguy83 force-pushed the container_store_extra_data branch from 761841c to e73cabb Compare August 18, 2017 13:19

cpuguy83 force-pushed the container_store_extra_data branch from e73cabb to fe2af86 Compare August 18, 2017 20:58

cpuguy83 force-pushed the container_store_extra_data branch from fe2af86 to 870efa3 Compare August 21, 2017 16:49

cpuguy83 force-pushed the container_store_extra_data branch 3 times, most recently from 283f8fc to 8c67de5 Compare August 21, 2017 17:04

mlaventure reviewed Aug 21, 2017

View reviewed changes

stevvooe reviewed Aug 31, 2017

View reviewed changes

Comment thread api/services/containers/v1/containers.proto Outdated

Copy link
Copy Markdown

Member

stevvooe Aug 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be repeated.

stevvooe reviewed Aug 31, 2017

View reviewed changes

Comment thread container_opts.go Outdated

Copy link
Copy Markdown

Member

stevvooe Aug 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Extension"

Random-Liu mentioned this pull request Aug 31, 2017

ctr: drop labels from ctr containers subcommand list #1456

Merged

stevvooe modified the milestones: containerd beta, containerd 1.0.0 Sep 6, 2017

cpuguy83 force-pushed the container_store_extra_data branch 3 times, most recently from 55bd94d to 7e17e0b Compare September 7, 2017 14:00

crosbymichael reviewed Sep 7, 2017

View reviewed changes

cpuguy83 force-pushed the container_store_extra_data branch from 7e17e0b to 94f9575 Compare September 7, 2017 16:07

mlaventure reviewed Sep 7, 2017

View reviewed changes

Add field to Container for client-defined data

3552ce5

This field allows a client to store specialized information in the container metadata rather than having to store this itself and keep the data in sync with containerd. Signed-off-by: Brian Goff <[email protected]>

cpuguy83 force-pushed the container_store_extra_data branch from 94f9575 to 3552ce5 Compare September 7, 2017 20:54

mlaventure reviewed Sep 7, 2017

View reviewed changes

stevvooe force-pushed the container_store_extra_data branch from c35e3c6 to eaf4140 Compare September 9, 2017 02:20

stevvooe force-pushed the container_store_extra_data branch from eaf4140 to f7d31e2 Compare September 9, 2017 02:34

stevvooe mentioned this pull request Sep 14, 2017

limit size of labels #1513

Closed

Random-Liu mentioned this pull request Sep 15, 2017

Switch to containerd extension containerd/cri#251

Closed

mlaventure approved these changes Sep 18, 2017

View reviewed changes

mlaventure merged commit e517952 into containerd:master Sep 18, 2017

cpuguy83 deleted the container_store_extra_data branch September 18, 2017 14:37

Conversation

cpuguy83 commented Aug 17, 2017

Uh oh!

mlaventure commented Aug 17, 2017

Uh oh!

cpuguy83 commented Aug 17, 2017

Uh oh!

stevvooe commented Aug 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Random-Liu commented Aug 17, 2017

Uh oh!

cpuguy83 commented Aug 18, 2017

Uh oh!

cpuguy83 commented Aug 18, 2017

Uh oh!

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

Uh oh!

Uh oh!

cpuguy83 commented Aug 18, 2017

Uh oh!

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

Uh oh!

Uh oh!

cpuguy83 commented Aug 18, 2017

Uh oh!

cpuguy83 commented Aug 18, 2017

Uh oh!

mlaventure commented Aug 18, 2017

Uh oh!

crosbymichael commented Aug 18, 2017

Uh oh!

codecov-io commented Aug 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

cpuguy83 commented Aug 21, 2017

Uh oh!

mlaventure commented Aug 21, 2017

Uh oh!

cpuguy83 commented Aug 21, 2017

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!

cpuguy83 commented Sep 7, 2017

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!

stevvooe commented Sep 7, 2017

Uh oh!

cpuguy83 commented Sep 7, 2017

Uh oh!

stevvooe commented Sep 8, 2017

Uh oh!

crosbymichael commented Sep 12, 2017

Uh oh!

Random-Liu commented Sep 15, 2017

Uh oh!

mlaventure left a comment

Choose a reason for hiding this comment

Uh oh!

cpuguy83 commented Sep 18, 2017

Uh oh!

Random-Liu commented Aug 18, 2017 •

edited

Loading

Random-Liu commented Aug 18, 2017 •

edited

Loading

codecov-io commented Aug 18, 2017 •

edited

Loading