Pass the label of docker container to the annotation of OCI runtime spec by miaoyq · Pull Request #36181 · moby/moby

miaoyq · 2018-02-02T08:55:58Z

Signed-off-by: Yanqiang Miao miao.yanqiang@zte.com.cn

- What I did
Currectly, docker save the label data in container struct of docker, but not in config.json defined by OCI.
This pr pass the label of docker container to the annotation of OCI runtime spec, so that the label data which is set by kubernetes can be used by the VM-based runtime (like clear-containers) .
For example:

	containerTypeLabelKey       = "io.kubernetes.docker.type"
	containerTypeLabelSandbox   = "podsandbox"
	containerTypeLabelContainer = "container"
	containerLogPathLabelKey    = "io.kubernetes.container.logpath"
	sandboxIDLabelKey           = "io.kubernetes.sandbox.id"

/cc @sameo @plutoinmii

- How I did it

- How to verify it

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

AkihiroSuda · 2018-02-03T15:46:17Z

daemon/oci_linux.go

Is s.Annotations guaranteed to be empty here?

Even if so, I think the code should be as follows:

for k, v := range c.Config.Labels{ vv, ok := s.Annotations[k] if ok { return nil, errors.Errorf("label %q conflicts, old=%q, new=%q", vv, v) } s.Annotations[k] = v }

@AkihiroSuda Thanks, will do.

@AkihiroSuda If a key already exists in s.Annotations we could print a warning msg instead of return directly, wdyt?

I think ok.

AkihiroSuda · 2018-02-05T02:20:29Z

daemon/oci_linux.go

: -> =?

thaJeztah · 2018-02-12T09:09:39Z

daemon/oci_linux.go

nit: oldValue and ok are only used inside the if, so;

if oldValue, ok := s.Annotations[k]; ok { logrus.Warningf("Key %q already exists in Annotations, old value=%q, new value=%q", k, oldValue, v) continue }

The warning should also mention that we're ignoring the new value

I'm not sure what would be best though:

under what situations would an existing label exist in the annotations?

if an existing annotation is there, is it correct to ignore the new value (even with a warning)? Or should it produce an error; instructing the user to correct the conflicting option?

In general, I'm in favour of being strict, and error out. If at some point in future we think it's ok to change an error into a warning, we can do so without breaking backward compatibility, whereas the opposite is not possible.

ping @mlaventure @cpuguy83 PTAL

I'm not sure in which scenario that would happen, but I'd vote to start with an error too,

@thaJeztah @mlaventure Makes sense. Will change to error.

thaJeztah · 2018-02-13T12:02:16Z

daemon/oci_linux.go

oh; meant this to be returning the error (thus "fail");

return nil, errors.Errorf(".......

@thaJeztah I thought that this should not terminate the container starting, just give an error message, since the Annotations does not seem to be used directly by docker.
But returning the error is also reasonable here. Have done. :-)

thaJeztah

~~LGTM~~

@AkihiroSuda PTAL

thaJeztah · 2018-02-14T01:31:47Z

Actually; one more thought: do labels that are set on a docker container have to be namespaced when passing through? (e.g. a org.moby.label.<label name> prefix)? IOW; is there a need to distinguish labels from other kind of annotations?

miaoyq · 2018-02-14T02:00:53Z

Is there a need to distinguish labels from other kind of annotations?

@thaJeztah I think we should keep it as it is, make sure OCI runtime can get the data that has not be changed.
Containerd also doesn't care the Annotations as far as I know. :-)

justincormack · 2018-02-15T11:20:40Z

I really don't like just passing all labels through, people use labels for all sorts of things, and these may not be intended to be passed on.

Also the OCI spec says "Keys SHOULD be named using a reverse domain notation - e.g. com.example.myKey." and "Keys using the org.opencontainers namespace are reserved and MUST NOT be used by subsequent specifications." so I think just adding arbitrary labels is not the intended use case.

(containerd also has annotations, but I don't think we want to expose those either necessrily).

this needs more discussion

miaoyq · 2018-02-22T07:16:40Z

@justincormack Could we filter the labels whose key format like io.kubernetes.docker.type and copy them into annotations fields?

WeiZhang555 · 2018-04-12T08:40:42Z

I think this feature can be useful, currently there's no way to pass Annotations into OCI runtime from docker, though OCI spec defines Annotations field.

How about we make some rules, for example, if the label key string starts with "annotation.", we pass it to runtime annotation, otherwise we do nothing.

label "annotation.io.kubernetes.docker.type=container", then we pass an Annotation["io.kubernetes.docker.type"]=container
label "io.docker.xxx=xxx", we do NOT pass it to OCI runtime.

miaoyq · 2018-08-03T03:47:15Z

Any conclusion for this feature?
Docker still the main and default container runtime engine for k8s so far, so I think this feature is very useful for the people of using secure containers. :-)
@thaJeztah @justincormack

cpuguy83 · 2018-11-26T23:43:16Z

What about just a list of label keys as an allow-list for what gets passed to the runtime?

olljanat · 2018-12-21T18:18:48Z

@miaoyq this one needs to be rebased

GordonTheTurtle · 2018-12-24T00:39:48Z

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "label-to-oci-spec-annotation" git@github.com:miaoyq/docker.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842354427496
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

Signed-off-by: Yanqiang Miao <miao.yanqiang@zte.com.cn>

codecov · 2018-12-24T01:04:36Z

Codecov Report

❗ No coverage uploaded for pull request base (master@d147fe0). Click here to learn what that means.
The diff coverage is 33.33%.

@@            Coverage Diff            @@
##             master   #36181   +/-   ##
=========================================
  Coverage          ?   36.56%           
=========================================
  Files             ?      608           
  Lines             ?    45042           
  Branches          ?        0           
=========================================
  Hits              ?    16470           
  Misses            ?    26293           
  Partials          ?     2279

thaJeztah · 2023-02-24T16:45:58Z

Implemented (but slightly different) in #45025

GordonTheTurtle added the status/0-triage label Feb 2, 2018

AkihiroSuda reviewed Feb 3, 2018

View reviewed changes

miaoyq force-pushed the label-to-oci-spec-annotation branch from b58f4f6 to 01212c5 Compare February 5, 2018 02:07

AkihiroSuda reviewed Feb 5, 2018

View reviewed changes

miaoyq force-pushed the label-to-oci-spec-annotation branch 3 times, most recently from 1ad04bd to 514713f Compare February 5, 2018 06:29

This was referenced Feb 6, 2018

How to make dockershim support clear containers? clearcontainers/runtime#987

Open

Add support for dockershim pod annotations containers/virtcontainers#607

Merged

thaJeztah requested changes Feb 12, 2018

View reviewed changes

thaJeztah added area/runtime Runtime status/1-design-review and removed status/0-triage labels Feb 12, 2018

miaoyq force-pushed the label-to-oci-spec-annotation branch from 514713f to 472010f Compare February 13, 2018 01:21

thaJeztah reviewed Feb 13, 2018

View reviewed changes

miaoyq force-pushed the label-to-oci-spec-annotation branch from 472010f to 084986e Compare February 14, 2018 00:44

thaJeztah previously approved these changes Feb 14, 2018

View reviewed changes

yongtang added status/2-code-review and removed status/1-design-review labels Feb 14, 2018

vdemeester added status/1-design-review and removed status/2-code-review labels Feb 15, 2018

GordonTheTurtle assigned tonistiigi Feb 17, 2018

jshachm mentioned this pull request May 7, 2018

Do we need a native way to start up a pod by docker cli kata-containers/runtime#285

Closed

jshachm mentioned this pull request Jun 11, 2018

Pass through specific annotation to OCI runtime spec #37262

Closed

miaoyq mentioned this pull request Aug 6, 2018

Make dockershim support kata-containers kubernetes/kubernetes#67007

Closed

derek bot added the status/failing-ci Indicates that the PR in its current state fails the test suite label Dec 22, 2018

GordonTheTurtle added the dco/no Automatically set by a bot when one of the commits lacks proper signature label Dec 24, 2018

miaoyq force-pushed the label-to-oci-spec-annotation branch from d3f80f7 to 634a38c Compare December 24, 2018 00:51

GordonTheTurtle removed the dco/no Automatically set by a bot when one of the commits lacks proper signature label Dec 24, 2018

Pass the label of docker container to the annotation of OCI runtime spec

4b0fc96

Signed-off-by: Yanqiang Miao <miao.yanqiang@zte.com.cn>

miaoyq force-pushed the label-to-oci-spec-annotation branch from 634a38c to 4b0fc96 Compare December 24, 2018 01:04

corhere mentioned this pull request Feb 17, 2023

daemon: allow setting OCI annotations on containers #45025

Merged

cpuguy83 closed this in #45025 Feb 24, 2023

Conversation

miaoyq commented Feb 2, 2018

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!

thaJeztah left a comment • 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!

thaJeztah commented Feb 14, 2018

Uh oh!

miaoyq commented Feb 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justincormack commented Feb 15, 2018

Uh oh!

miaoyq commented Feb 22, 2018

Uh oh!

WeiZhang555 commented Apr 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miaoyq commented Aug 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cpuguy83 commented Nov 26, 2018

Uh oh!

olljanat commented Dec 21, 2018

Uh oh!

GordonTheTurtle commented Dec 24, 2018

Uh oh!

codecov bot commented Dec 24, 2018

Codecov Report

Uh oh!

thaJeztah commented Feb 24, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

thaJeztah left a comment •

edited

Loading

miaoyq commented Feb 14, 2018 •

edited

Loading

WeiZhang555 commented Apr 12, 2018 •

edited

Loading

miaoyq commented Aug 3, 2018 •

edited

Loading