Add runtime handler as annotations to ImageSpec #1448

kkmsft · 2020-01-14T09:05:05Z

No description provided.

michmike

/lgtm
/approve

Random-Liu · 2020-01-14T18:12:48Z

/lgtm

This looks good to me.

PatrickLang · 2020-01-14T18:13:09Z

/assign @mikebrow
/assign @Random-Liu

Random-Liu · 2020-01-14T18:13:15Z

keps/sig-windows/windows-runtimeclass-support.md

+ 
+The runtimeHandler annotation will be based on the Runtime Class specified by the user:
+```
+“runtimehandler”: “<corresponding values>”


Can we also add kubernetes.io domain?

as in kubernetes.io/runtimehandler ?

Suggest adding a note with a link to the oci annotations for the oci image spec... https://github.com/opencontainers/image-spec/blob/master/annotations.md#annotations given that oci reserves org.opencontainers.image domain

Yeah, I think kubernetes.io/runtimehandler sounds good to me. :)

those annotations are bit different scope though - they're stored in the container manifest. We're talking about changing CRI, not OCI

right just a mention regarding another reserved annotation domain for these annotations.

jterry75 · 2020-01-14T21:10:10Z

keps/sig-windows/windows-runtimeclass-support.md

+“kubernetes.io/os”: ”linux”
+```
+
+Note that these are currently arrived from GOARCH and GOOS at runtime, which does not reflect the image which needs to be pulled but instead corresponds to the system on which kubelet is running. The runtime handler specified in the annotation will provide more accurate indication of the user intend to the runtime.


'these are currently derived'

s/intend/intent/

I thought someone would jut make new runtime handler configs to indicate if they wanted a particular arch/os/variant.. Seems odd to have runtime handler be a first class string then switches on the handler be a suggested annotation evaluated by container runtimes... Maybe just mention that this is a temporary soln to use annotations for this arch/os/variant selection pattern..

another example of use is where we need per container runtime container image cache storage policies passed to the integration. For example: kubernetes.io/cache_image = "node", "pod", "container", "userid", "groupid", ...
In this example "node" is the default where images pulled on the node are cacheable... "pod" would mean one cache per pod no sharing of images across pods... etc... TBD for details based on container image policy kep WIP.

jterry75 · 2020-01-14T21:16:03Z

@Random-Liu - Is there any expectation at all that on ImageSpec return values from *Stats* and List* style functions that there be annotations? IE: Are the annotations only for *Request input to various functions and not used for *Response

jterry75 · 2020-01-14T21:16:44Z

keps/sig-windows/windows-runtimeclass-support.md

+We could potentially also add the kubernetes specification annotations for consideration of the runtime:
+```
+“kubernetes.io/arch”: “amd64”
+“kubernetes.io/os”: ”linux”


Should we define these in the kubernetes.io/image/* namespace?

And have:
.../arch
.../os
.../variant
as known values?

Ah good point. I see why this is confusing. Today we already have well-known labels applied to the node by the kubelet doc

kubernetes.io/os

kubernetes.io/arch

Those are intended to be used for scheduling as nodeSelectors, not for determining what image to pull.

In this KEP I propose to encode a user's desired os/arch as part of the RuntimeClass. This may be different from the node labels above, so it would be good to use different names. RuntimeClass is the only thing passed in the pod sandbox config. I think for image/* to be useful, we'd need matching ones in the pod sandbox config too.

Ah I see. Ok then yes we will just have one known annotation kubernetes.io/runtimehandler that is passed in the ImageSpec.Annotations. Thanks

Should we define these in the kubernetes.io/image/* namespace?

Only one / is allowed for Kubernetes annotation, we may want to follow the same rule.
https://kubernetes.io/docs/concepts/overview/working-with-objects/annotations/#syntax-and-character-set

Random-Liu · 2020-01-14T21:43:48Z

Is there any expectation at all that on ImageSpec return values from Stats and List* style functions that there be annotations? IE: Are the annotations only for *Request input to various functions and not used for *Response

I think we should return the annotation in all Responses.

We may want to make that clear in the doc.

mikebrow

See comments.

mikebrow · 2020-01-21T18:08:48Z

keps/sig-windows/windows-runtimeclass-support.md


 #### Adding handler to CRI pull API

+> As per discussions in sig-node meetings based on the PR https://github.com/kubernetes/kubernetes/pull/84486, this approach will no longer be considered. An alternate approach to add annotations to the ImageSpec will be path taken.


I think we can just delete this section since it's no longer being considered and delete other lines discussing handler.

Sure, will remove

mikebrow · 2020-01-21T18:11:30Z

keps/sig-windows/windows-runtimeclass-support.md

+ 
+The runtimeHandler annotation will be based on the Runtime Class specified by the user:
+```
+“runtimehandler”: “<corresponding values>”


Suggest adding a note with a link to the oci annotations for the oci image spec... https://github.com/opencontainers/image-spec/blob/master/annotations.md#annotations given that oci reserves org.opencontainers.image domain

mikebrow · 2020-01-21T18:21:27Z

keps/sig-windows/windows-runtimeclass-support.md

+“kubernetes.io/os”: ”linux”
+```
+
+Note that these are currently arrived from GOARCH and GOOS at runtime, which does not reflect the image which needs to be pulled but instead corresponds to the system on which kubelet is running. The runtime handler specified in the annotation will provide more accurate indication of the user intend to the runtime.


s/intend/intent/

mikebrow · 2020-01-21T18:30:18Z

keps/sig-windows/windows-runtimeclass-support.md

+“kubernetes.io/os”: ”linux”
+```
+
+Note that these are currently arrived from GOARCH and GOOS at runtime, which does not reflect the image which needs to be pulled but instead corresponds to the system on which kubelet is running. The runtime handler specified in the annotation will provide more accurate indication of the user intend to the runtime.


I thought someone would jut make new runtime handler configs to indicate if they wanted a particular arch/os/variant.. Seems odd to have runtime handler be a first class string then switches on the handler be a suggested annotation evaluated by container runtimes... Maybe just mention that this is a temporary soln to use annotations for this arch/os/variant selection pattern..

mikebrow · 2020-01-21T18:37:06Z

keps/sig-windows/windows-runtimeclass-support.md

+“kubernetes.io/os”: ”linux”
+```
+
+Note that these are currently arrived from GOARCH and GOOS at runtime, which does not reflect the image which needs to be pulled but instead corresponds to the system on which kubelet is running. The runtime handler specified in the annotation will provide more accurate indication of the user intend to the runtime.


another example of use is where we need per container runtime container image cache storage policies passed to the integration. For example: kubernetes.io/cache_image = "node", "pod", "container", "userid", "groupid", ...
In this example "node" is the default where images pulled on the node are cacheable... "pod" would mean one cache per pod no sharing of images across pods... etc... TBD for details based on container image policy kep WIP.

mikebrow · 2020-01-21T18:39:52Z

keps/sig-windows/windows-runtimeclass-support.md

 This will be reviewed with contributors from both Kubernetes and ContainerD and tested together while Windows support in CRI-ContainerD is still in development. As this is an optional field today in `RunPodSandboxRequest`, there's no risk if someone wants to not specify it in `RuntimeClass`. In that case ContainerD will use the default configuration for pull, create sandbox, and so on.

+#### Adding annotations to ImageSpec
+These annotation will be an optional parameter and default configuration is used by `ContainerD`(or any other runtime involved) when no annotations are specified. 


s/annotation/annotations/

Support for annotations will be on a container runtime implementation basis. A table should be built by each container runtime mentioning which annotations are supported by CR version.

I don't think we can "force" annotation support.... unless we want to have an api querying for a list of supported annotations from the CR.

Have reframed the words to reflect this.

kkmsft · 2020-01-28T19:47:05Z

@Random-Liu @PatrickLang - Have update the ImageSpec usage in List. PTAL.

PatrickLang · 2020-01-28T20:25:14Z

/LGTM
/hold
Can remove hold once @Random-Liu has reviewed

Random-Liu · 2020-01-28T22:04:30Z

keps/sig-windows/windows-runtimeclass-support.md

+    // the local storage. It returns ("", nil) if the image isn't in the local storage.
+    GetImageRef(image ImageSpec) (string, error)
+    // Gets all images currently on the machine.
+    ListImages() ([]ImageSpec, error)


ListImages should continue return Images, we just need to add ImageSpec information into Image.

ok - can we work out the specifics of that during phase 2 PR? We don't have the background on why Image and ImageSpec are different

Thanks you @Random-Liu . I have modified the struct Image to include the ImageSpec in the updated PR. Please have a look - @Random-Liu @PatrickLang

oh ok - that makes sense

/lgtm
/hold
(remove once @Random-Liu ok with final update)

Random-Liu · 2020-01-28T22:58:45Z

/lgtm

dchen1107 · 2020-01-28T22:59:26Z

/lgtm
/approve

PatrickLang · 2020-01-28T23:00:08Z

/cancel hold

PatrickLang · 2020-01-28T23:00:42Z

/lgtm
/hold cancel

k8s-ci-robot · 2020-01-28T23:01:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dchen1107, kkmsft, michmike, PatrickLang, Random-Liu

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~keps/sig-windows/OWNERS~~ [PatrickLang,michmike]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

this-week: 2023-07-28

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 14, 2020

k8s-ci-robot requested review from PatrickLang and michmike January 14, 2020 09:05

k8s-ci-robot added kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/windows Categorizes an issue or PR as relevant to SIG Windows. labels Jan 14, 2020

michmike approved these changes Jan 14, 2020

View reviewed changes

k8s-ci-robot assigned michmike Jan 14, 2020

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 14, 2020

k8s-ci-robot assigned Random-Liu Jan 14, 2020

k8s-ci-robot assigned mikebrow Jan 14, 2020

Random-Liu reviewed Jan 14, 2020

View reviewed changes

jterry75 reviewed Jan 14, 2020

View reviewed changes

mikebrow suggested changes Jan 21, 2020

View reviewed changes

kkmsft force-pushed the kkmsft-patch-1 branch 2 times, most recently from e5bdec4 to 427fc40 Compare January 28, 2020 19:40

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 28, 2020

kkmsft force-pushed the kkmsft-patch-1 branch from edd74b9 to 993b8f7 Compare January 28, 2020 19:45

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 28, 2020

k8s-ci-robot assigned PatrickLang Jan 28, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 28, 2020

kkmsft requested a review from Random-Liu January 28, 2020 21:57

Random-Liu reviewed Jan 28, 2020

View reviewed changes

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 28, 2020

kkmsft force-pushed the kkmsft-patch-1 branch from 4218da0 to 61263eb Compare January 28, 2020 22:32

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 28, 2020

Add runtime handler as annotations to ImageSpec

1f185c9

kkmsft force-pushed the kkmsft-patch-1 branch from 61263eb to 1f185c9 Compare January 28, 2020 22:48

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 28, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 28, 2020

k8s-ci-robot assigned dchen1107 Jan 28, 2020

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 28, 2020

k8s-ci-robot merged commit fe9b215 into kubernetes:master Jan 28, 2020

k8s-ci-robot added this to the v1.18 milestone Jan 28, 2020

kkmsft deleted the kkmsft-patch-1 branch April 16, 2020 19:03

ingvagabund pushed a commit to ingvagabund/enhancements that referenced this pull request Feb 26, 2025

Merge pull request kubernetes#1448 from dhellmann/twie-2023-07-28

708f182

this-week: 2023-07-28


		#### Adding handler to CRI pull API

		> As per discussions in sig-node meetings based on the PR https://github.com/kubernetes/kubernetes/pull/84486, this approach will no longer be considered. An alternate approach to add annotations to the ImageSpec will be path taken.

Add runtime handler as annotations to ImageSpec #1448

Add runtime handler as annotations to ImageSpec #1448

Uh oh!

Conversation

kkmsft commented Jan 14, 2020

Uh oh!

michmike left a comment

Choose a reason for hiding this comment

Uh oh!

Random-Liu commented Jan 14, 2020

Uh oh!

PatrickLang commented Jan 14, 2020

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!

jterry75 Jan 14, 2020 • 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!

jterry75 commented Jan 14, 2020

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 Jan 14, 2020

Uh oh!

mikebrow left a comment

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!

kkmsft commented Jan 28, 2020

Uh oh!

PatrickLang commented Jan 28, 2020

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!

jterry75 Jan 14, 2020 •

edited

Loading