Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[dev-1.29] Add docs for KEP 4216: Image pull per runtime class #44028

Merged
merged 1 commit into from
Nov 27, 2023

Conversation

kiashok
Copy link
Contributor

@kiashok kiashok commented Nov 21, 2023

Add docs for KEP 4216: https://github.com/kubernetes/enhancements/tree/master/keps/sig-node/4216-image-pull-per-runtime-class which has been accepted for k8s 1.29

P.S. : Opened this PR against dev-1.29 branch based on review comment here #43541 (comment)

@k8s-ci-robot k8s-ci-robot added this to the 1.29 milestone Nov 21, 2023
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 21, 2023
Copy link

netlify bot commented Nov 21, 2023

👷 Deploy Preview for kubernetes-io-vnext-staging processing.

Name Link
🔨 Latest commit edddb55
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-vnext-staging/deploys/6563c1438c059d0008157c58

@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Nov 21, 2023
@k8s-ci-robot k8s-ci-robot added the language/en Issues or PRs related to English language label Nov 21, 2023
@k8s-ci-robot k8s-ci-robot requested review from sftim and tengqm November 21, 2023 18:48
@k8s-ci-robot k8s-ci-robot added the sig/docs Categorizes an issue or PR as relevant to SIG Docs. label Nov 21, 2023
@kiashok
Copy link
Contributor Author

kiashok commented Nov 21, 2023

Copy link
Contributor

@aravindhp aravindhp left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

Thanks.

Where's the rest of the documentation? I'd expect a mention within https://kubernetes.io/docs/concepts/containers/images/ and maybe also https://kubernetes.io/docs/concepts/containers/runtime-class/

@kiashok
Copy link
Contributor Author

kiashok commented Nov 21, 2023

Thanks.

Where's the rest of the documentation? I'd expect a mention within https://kubernetes.io/docs/concepts/containers/images/ and maybe also https://kubernetes.io/docs/concepts/containers/runtime-class/

@sftim there was a conversation related to this in the PR I have linked in the description #43541 (comment)
#43541 (comment)

@kiashok kiashok force-pushed the docs-kep4216-dev1.29 branch 2 times, most recently from 02a3463 to d7cd60f Compare November 22, 2023 00:00
@dipesh-rawat
Copy link
Member

/sig windows
/sig node

@k8s-ci-robot k8s-ci-robot added sig/windows Categorizes an issue or PR as relevant to SIG Windows. sig/node Categorizes an issue or PR as relevant to SIG Node. labels Nov 22, 2023
@kiashok kiashok force-pushed the docs-kep4216-dev1.29 branch from d7cd60f to 9847709 Compare November 22, 2023 01:31
@sftim
Copy link
Contributor

sftim commented Nov 22, 2023

Quoting #43541 (comment):

In this case I don't think the user-facing behavior is significant enough to add such documentation while the feature in in alpha but we should definitely add some documentation before going to beta

Thanks, but (writing as a tech lead for SIG Docs), we disagree. Even if the ecosystem doesn't have this support today, that could change before v1.29 goes out of support.

When we add functionality to Kubernetes that a user could use, we want to document it. Hence, please document the feature. Don't assume that readers will go the feature gates page or the source code to find out how Kubernetes works in this regard.

For alpha, a few lines of documentation may be enough. For beta, the docs should be close to GA quality.

@kiashok
Copy link
Contributor Author

kiashok commented Nov 22, 2023

Quoting #43541 (comment):

In this case I don't think the user-facing behavior is significant enough to add such documentation while the feature in in alpha but we should definitely add some documentation before going to beta

Thanks, but (writing as a tech lead for SIG Docs), we disagree. Even if the ecosystem doesn't have this support today, that could change before v1.29 goes out of support.

When we add functionality to Kubernetes that a user could use, we want to document it. Hence, please document the feature. Don't assume that readers will go the feature gates page or the source code to find out how Kubernetes works in this regard.

For alpha, a few lines of documentation may be enough. For beta, the docs should be close to GA quality.

As mentioned in the conversations on the other PR, there is no support from any container runtime and there is no working e2e functionality for this featue yet. The alpha changes for kubelet is only helping to plumb the newly introduced 'RuntimeHandler' ImageSpec CRI field to container runtimes and the runtimes that would like to support this feature can use it. Specifically I am working on adding support in containerd Runtime and when we go beta in 1.29 is when there will be something significant for the user to use. I think there is no value in adding any documentation for the feature in alpha at all at this point.

Cc @marosset @jsturtevant @aravindhp @mikebrow

Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

(reiterating the request for documentation, ahead of shipping a change that's exposed outside of Kubernetes itself)

@kiashok kiashok force-pushed the docs-kep4216-dev1.29 branch from 9847709 to 1f6fde7 Compare November 22, 2023 18:41
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 22, 2023
@kiashok
Copy link
Contributor Author

kiashok commented Nov 22, 2023

(reiterating the request for documentation, ahead of shipping a change that's exposed outside of Kubernetes itself)

added generic documentation for this feature.

Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

Thanks. Here's some suggestions around documentation style.

Comment on lines 49 to 55
In K8s v1.29.0-alpha.3, CRI changes needed to support image pulls based on pod runtime class
has been introduced. It is under feature gate `RuntimeClassInImageCriApi` and off by default.

When the image pull per runtime class is GA, container images will be referenced as a tuple of
(imageName, runtimeHandler) instead of just the imageName or image digest. This will be helpful
in pulling suitable images for running VM based containers.
Container runtimes may or may not add support for this feature.
Copy link
Contributor

@sftim sftim Nov 22, 2023

Choose a reason for hiding this comment

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

Suggested change
In K8s v1.29.0-alpha.3, CRI changes needed to support image pulls based on pod runtime class
has been introduced. It is under feature gate `RuntimeClassInImageCriApi` and off by default.
When the image pull per runtime class is GA, container images will be referenced as a tuple of
(imageName, runtimeHandler) instead of just the imageName or image digest. This will be helpful
in pulling suitable images for running VM based containers.
Container runtimes may or may not add support for this feature.
{{< feature-state for_k8s_version="v1.29" state="alpha" >}}
Kubernetes includes alpha support for performing image pulls based on the RuntimeClass of a Pod.
If you enable the `RuntimeClassInImageCriApi` [feature gate](/docs/reference/command-line-tools-reference/feature-gates/),
the kubelet references container images by a tuple of (image name, runtime handler) rather than just the
image name or digest. Your {{< glossary_tooltip text="container runtime" term_id="container-runtime" >}}
may adapt its behavior based on the selected runtime handler. Provided your container runtime does include
support, this extra information passed via {{< glossary_tooltip text="CRI" term_id="cri" >}} is useful
if you mix low level runtime based on virtualization with runtimes based on Linux (cgroup and namespace) process isolation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sftim I am not sure I understand what you mean by "if you mix low level runtime based on virtualization with runtimes based on Linux (cgroup and namespace) process isolation.".

Copy link
Contributor Author

@kiashok kiashok Nov 26, 2023

Choose a reason for hiding this comment

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

Changed to:

{{< feature-state for_k8s_version="v1.29" state="alpha" >}}
Kubernetes includes alpha support for performing image pulls based on the RuntimeClass of a Pod.

If you enable the RuntimeClassInImageCriApi feature gate,
the kubelet references container images by a tuple of (image name, runtime handler) rather than just the
image name or digest. Your {{< glossary_tooltip text="container runtime" term_id="container-runtime" >}}
may adapt its behavior based on the selected runtime handler.
Pulling images based on runtime class will be helpful for VM based containers like windows hyperV containers.

Copy link
Contributor

Choose a reason for hiding this comment

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

The feature is only really relevant if you use different ways to run containers, on the same node. For example, if everything is gVisor on a node, you don't need this feature. Or if everything is Kata + Firecracker.

@@ -44,6 +44,16 @@ There are additional rules about where you can place the separator
characters (`_`, `-`, and `.`) inside an image tag.
If you don't specify a tag, Kubernetes assumes you mean the tag `latest`.

## Image pull per runtime class
Copy link
Contributor

Choose a reason for hiding this comment

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

I would move this to just before the heading ## Serial and parallel image pulls, and make it a 3rd level (### heading).

Copy link
Contributor

Choose a reason for hiding this comment

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

(please make that change)

@katcosgrove
Copy link
Contributor

Hello @kiashok! The deadline to have this PR merged is Tuesday, November 28. Enhancements that do not include required documentation may not be included in the release. Is there anything I can do to help get this ready to merge? Thanks!

Signed-off-by: Kirtana Ashok <[email protected]>
(cherry picked from commit 10a984d)
Signed-off-by: Kirtana Ashok <[email protected]>
@kiashok kiashok force-pushed the docs-kep4216-dev1.29 branch from 1f6fde7 to edddb55 Compare November 26, 2023 22:05
@sftim
Copy link
Contributor

sftim commented Nov 27, 2023

See #44028 (comment) for some pending feedback.

/lgtm
even with the missing piece

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 27, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 40a31711b1fb04c6d0114e7bc57de5ae46b847d1

@katcosgrove
Copy link
Contributor

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: katcosgrove

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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 27, 2023
@k8s-ci-robot k8s-ci-robot merged commit cb04844 into kubernetes:dev-1.29 Nov 27, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/en Issues or PRs related to English language lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/docs Categorizes an issue or PR as relevant to SIG Docs. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/windows Categorizes an issue or PR as relevant to SIG Windows. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants