Use container annotations when creating containers

[bart0sh]

Container annotations should be used the same way as pod annotations when creating new containers. They should be also passed down to the OCI runtime.

[k8s-ci-robot]

Hi @bart0sh. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[mikebrow]

/ok-to-test

[jterry75]

@bart0sh - Was this a regression? I thought we already did this?

[mikebrow]

LGTM

hard to believe this was missed and no issues opened :-O

I'll add a test case PR to go along with this one

[mikebrow]

@bart0sh - Was this a regression? I thought we already did this?

So did I... but it looks like we only pass the pod annotations.. see my test case pr.

[mikebrow]

You know what... it sort of depends how you read this:
# pod_annotations is a list of pod annotations passed to both pod
# sandbox as well as container OCI annotations. Pod_annotations also
# supports golang path match pattern - https://golang.org/pkg/path/#Match.
# e.g. ["runc.com."], [".runc.com"], ["runc.com/*"].
#
# For the naming convention of annotation keys, please reference:
# * Kubernetes: https://kubernetes.io/docs/concepts/overview/working-with-objects/annotations/#syntax-and-character-set
# * OCI: https://github.com/opencontainers/image-spec/blob/master/annotations.md

when reading the Kubernetes docs it's obvious or not so obvious that by objects they mean pod and container kinds...

When reading our config doc section above on pod_annotations one could read these are wildcards for pod annotations copied to the pod and container spec.. or one could read it as wild cards for container and pod kind annotations copied to pod and container specs respectively. In hind sight it should just have been passthrough_annotations if the later was the intention.

So maybe we need a new container_annotations field.

[mikebrow]

Needs either a new field for container_annotations or an edit to the config doc to explain pod_annotations is for pod and container kind object annotations from the pod/container specs.

[Random-Liu]

@mikebrow @bart0sh Why do we need to pass down "container annotations"?

There is no "container annotations" in the Kubernetes api, it is purely specified by kubelet. Why does the underlying runtime care about it? https://github.com/kubernetes/kubernetes/blob/4fec22b3e4da6034948fd684f60637171cb55561/pkg/kubelet/kuberuntime/labels.go#L108

[Random-Liu]

Ah, I guess you want the device plugin annotations.

Not sure whether we should use pod_annotations for this purpose, because it is not some annotations specified explicitly in the pod api object. Let me think about it.

I feel like we should have a well-known annotation domain defined either in CRI or device plugin interface for experimental device plugin annotations.

[jterry75]

@mikebrow / @Random-Liu - I vote we add a container_annotations map and clear document pod_annotations is for sandboxes, and container_annotations is for containers. There are many cases were annotations make sense as workarounds to the spec while we work on getting them upstream. For example, Kubernetes has no spec for many windows QoS settings but we can pass them down via annotations while we work on getting upstream KEP's in place.

And to be clear we only forward values: ContainerConfig.Annotations that match container_annotations in the toml.

[mikebrow]

https://kubernetes.io/docs/concepts/overview/working-with-objects/annotations/

[jterry75]

@mikebrow - I get the intended use of annotations and that our use of annotations is against that. But the truth is there is no other way to forward key/value pairs to the OCI spec. And since a sandbox and a container are both eventually an OCI spec for a container it seems reasonable to forward all annotations that match the prefixes defined the config. Kubernetes/Kublet can strictly adhere to what annotations "should" be by setting what is acceptable in the config but other authors can experiment on their own as needed as well without a hack. I think its a win win solution.

[Random-Liu]

I prefer we define well known annotation domain for device plugin annotations in kubernetes like this https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/apis/well_known_annotations_windows.go.

I'm fine with arbitrary annotation match for pod_annotations, because it is user facing, and different runtimes may want to have their own annotation domains.

However, the device plugin annotation is just some underlying integration detail, I think it is possible to standardize it instead of defining arbitrary annotations.

For example, we can have something like alpha.kubernetes.io/device-plugin, and for different devices, they can do things like:

• alpha.kubernetes.io/device-plugin/fpga/ocihook
• alpha.kubernetes.io/device-plugin/nvidia-gpu/ocihook
• ...

@mikebrow @bart0sh does this work for you?

[mikebrow]

I prefer we define well known annotation domain for device plugin annotations in kubernetes like this https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/apis/well_known_annotations_windows.go.

I'm fine with arbitrary annotation match for pod_annotations, because it is user facing, and different runtimes may want to have their own annotation domains.

However, the device plugin annotation is just some underlying integration detail, I think it is possible to standardize it instead of defining arbitrary annotations.

For example, we can have something like alpha.kubernetes.io/device-plugin, and for different devices, they can do things like:

• alpha.kubernetes.io/device-plugin/fpga/ocihook
• alpha.kubernetes.io/device-plugin/nvidia-gpu/ocihook
• ...

@mikebrow @bart0sh does this work for you?

I really like the point of reserving Kubernetes keys, kubernetes/ k8s/ for annotations.

And for device-plugin annotations it also makes very good sense to use some-kube-domain/device-plugin/some-predefined-names/* as a pattern.

[mikebrow]

So yeah I vote we go for the win win.. reserve and support, by default, the passing of k8s & kubernetes annotation key ranges for the non-arbitrary use cases... and add the ability to specify container_annotations that can be passed through for the more arbitrary container user facing annotation use cases (as we already do for pod (object) annotations). Or something similar :)

[bart0sh]

@Random-Liu

Ah, I guess you want the device plugin annotations.

Correct.

as for restricting annotations to some domain I have mixed feelings. Why would we want to do that? We can filter out any unwanted annotations using container_annotations.

I'll add container_annotations usage to this PR.

[kad]

I really like the point of reserving Kubernetes keys, kubernetes/ k8s/ for annotations.

And for device-plugin annotations it also makes very good sense to use some-kube-domain/device-plugin/some-predefined-names/* as a pattern.

The Kubernetes documentation mentions that kubernetes.io and k8s.io domains should be reserved to Kubernetes components. Device plugins are coming from device vendors and they are neither k8s components nor even CNCF projects. So, legally, they should not be using kubernetes.io/k8s.io domains for passing proprietary information.

[mikebrow]

see comments

[Random-Liu]

The Kubernetes documentation mentions that kubernetes.io and k8s.io domains should be reserved to Kubernetes components. Device plugins are coming from device vendors and they are neither k8s components nor even CNCF projects. So, legally, they should not be using kubernetes.io/k8s.io domains for passing proprietary information.

If we go with the well-known device plugin annotation model, the annotation format will be defined as part of device plugin interface, which is a "property" of Kubernetes. Of course the device plugin itself belongs to the vendor.

[kad]

Would it make sense to add filtering of container annotations in containerd only when it will be defined in Device Manager in Kubelet?

[bart0sh]

@Random-Liu thank you for the review! I've updated the PR as you've suggested. Please review and merge if you don't mind.

[mikebrow]

LGTM

[Random-Liu]

LGTM other than the comments.

[bart0sh]

@mikebrow @Random-Liu Thank you for the review! Updated the PR according to the review comments.

[mikebrow]

LGTM

[Random-Liu]

/lgtm