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

Add docs for KEP 4216: Image pull per runtime class #43541

Closed
wants to merge 1 commit into from

Conversation

kiashok
Copy link
Contributor

@kiashok kiashok commented Oct 17, 2023

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 17, 2023
@netlify
Copy link

netlify bot commented Oct 17, 2023

Pull request preview available for checking

Built without sensitive environment variables

Name Link
🔨 Latest commit 10a984d
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/655cf8bd541aa4000820b25f
😎 Deploy Preview https://deploy-preview-43541--kubernetes-io-main-staging.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@katcosgrove
Copy link
Contributor

Hi, @kiashok! v1.29 Docs Lead here. Please take a look at Documenting for a release - PR Ready for Review to get your PR ready for review before Tuesday 14th November 2023. Thank you!

@katcosgrove
Copy link
Contributor

Hi, @kiashok! Just dropping by to remind you that the deadline for docs PRs to be ready for review is tomorrow, November 14. Is there anything we can help you with to get this PR ready?

@kiashok
Copy link
Contributor Author

kiashok commented Nov 13, 2023

Hi, @kiashok! Just dropping by to remind you that the deadline for docs PRs to be ready for review is tomorrow, November 14. Is there anything we can help you with to get this PR ready?

I'll send one out soon

@kiashok
Copy link
Contributor Author

kiashok commented Nov 14, 2023

Hi, @kiashok! Just dropping by to remind you that the deadline for docs PRs to be ready for review is tomorrow, November 14. Is there anything we can help you with to get this PR ready?

I'll send one out soon

I think it is not a good idea to have a k8s doc change for the alpha changes of this KEP as implementation is pending on container runtime side. Even if the feature gate is on, container runtimes aren't going to be really honoring the runtime handler that kubelet passes down. Hence I think at this stage, a blog post might be more appropriate than a doc change for this KEP.
Created a placeholder PR here for blog post change #43923 . Working on it now and should have it go out early tomorrow.

cc @jsturtevant @marosset @aravindhp @mikebrow

@k8s-ci-robot k8s-ci-robot added the area/blog Issues or PRs related to the Kubernetes Blog subproject label Nov 14, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign kbhawkey for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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 language/en Issues or PRs related to English language sig/docs Categorizes an issue or PR as relevant to SIG Docs. labels Nov 14, 2023
@kiashok
Copy link
Contributor Author

kiashok commented Nov 14, 2023

Hi, @kiashok! Just dropping by to remind you that the deadline for docs PRs to be ready for review is tomorrow, November 14. Is there anything we can help you with to get this PR ready?

I'll send one out soon

I think it is not a good idea to have a k8s doc change for the alpha changes of this KEP as implementation is pending on container runtime side. Even if the feature gate is on, container runtimes aren't going to be really honoring the runtime handler that kubelet passes down. Hence I think at this stage, a blog post might be more appropriate than a doc change for this KEP. Created a placeholder PR here for blog post change #43923 . Working on it now and should have it go out early tomorrow.

cc @jsturtevant @marosset @aravindhp @mikebrow

@katcosgrove discussed with @aravindhp and updated this PR with the changes for the feature gate. It would make more sense to add more details about this feature once support for this is added to containerd. Please let me know if this looks ok.

@katcosgrove
Copy link
Contributor

Thank you!

/retitle Add docs for KEP 4216: Image pull per runtime class

@k8s-ci-robot k8s-ci-robot changed the title [WIP] Add placeholder for KEP 4216: Image pull per runtime class Add docs for KEP 4216: Image pull per runtime class Nov 14, 2023
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 14, 2023
@jsturtevant
Copy link
Contributor

Is there a place in the documentation to briefly mention what it will do and mention runtimes may not have implemented it yet? Is that something that is typically done or is this enough while the runtimes catch up?

@marosset
Copy link
Contributor

Is there a place in the documentation to briefly mention what it will do and mention runtimes may not have implemented it yet? Is that something that is typically done or is this enough while the runtimes catch up?

In the past (for SIG-windows) if there are significant user-facing behaviors we've added documentation to k8s.io explaining the new functionality while also stating that container runtime changes are needed.

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

@kiashok
Copy link
Contributor Author

kiashok commented Nov 14, 2023

Is there a place in the documentation to briefly mention what it will do and mention runtimes may not have implemented it yet? Is that something that is typically done or is this enough while the runtimes catch up?

https://github.com/kubernetes/website/blob/main/content/en/docs/concepts/containers/images.md this could be one of the places that could hold it. But since none of the container runtimes have implemented it and we don't know what feedback we might get after containerd implementation goes in (may or may not change some behavior on kubelet) so I thought it would be good to add in such a wide user facing doc once we are able to test this e2e after containerd changes are checked in.

@kiashok
Copy link
Contributor Author

kiashok commented Nov 14, 2023

Is there a place in the documentation to briefly mention what it will do and mention runtimes may not have implemented it yet? Is that something that is typically done or is this enough while the runtimes catch up?

In the past (for SIG-windows) if there are significant user-facing behaviors we've added documentation to k8s.io explaining the new functionality while also stating that container runtime changes are needed.

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

yeah I agree

@sftim
Copy link
Contributor

sftim commented Nov 14, 2023

/remove-area blog

@k8s-ci-robot k8s-ci-robot removed the area/blog Issues or PRs related to the Kubernetes Blog subproject label Nov 14, 2023
@katcosgrove
Copy link
Contributor

Hi, @kiashok! Kubernetes v1.29 Docs Lead here. Just a reminder that the deadline to have this PR reviewed and merged is Tuesday, 28 November. Let me know if you need any help!

@kiashok
Copy link
Contributor Author

kiashok commented Nov 20, 2023

Hi, @kiashok! Kubernetes v1.29 Docs Lead here. Just a reminder that the deadline to have this PR reviewed and merged is Tuesday, 28 November. Let me know if you need any help!

@katcosgrove As discussed in this PR, these are all the changes needed for this KEP at this stage as there are no significant changes and no container runtime supports the changes as yet. I am waiting for review on this PR. Could you please help assign doc reviewers? Or should it be the folks who signed off on the initial kubelet changes PRs itself?

@sftim
Copy link
Contributor

sftim commented Nov 20, 2023

Placeholder 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

@kiakshok, you might want to change the description to make it clear the PR is reviewable.

Copy link
Contributor

@jsturtevant jsturtevant left a comment

Choose a reason for hiding this comment

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

/lgtm

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

LGTM label has been added.

Git tree hash: 1bce6f89e24106c5d00117fb0aff1621e96103e7

@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 21, 2023
@dipesh-rawat
Copy link
Member

@kiashok Considering these changes are geared towards the upcoming release, we should opt for the dev-1.29 branch as the target branch for this PR instead of main.

/hold
(OK to unhold once the above comment is addressed)

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 21, 2023
Signed-off-by: Kirtana Ashok <kiashok@microsoft.com>
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 21, 2023
@k8s-ci-robot
Copy link
Contributor

New changes are detected. LGTM label has been removed.

@kiashok
Copy link
Contributor Author

kiashok commented Nov 21, 2023

@kiashok Considering these changes are geared towards the upcoming release, we should opt for the dev-1.29 branch as the target branch for this PR instead of main.

/hold (OK to unhold once the above comment is addressed)

@dipesh-rawat Opened #44028 . Should this PR remain open so the changes get checked into the main branch anyway?

@katcosgrove
Copy link
Contributor

Hi @kiashok! If you are now documenting this KEP in #44028 instead of here, please close this PR. All of the docs for v1.29 are merged on release day from our dev branch, so it is correct to point the docs PR at dev-1.29, not main.

@dipesh-rawat
Copy link
Member

Closing this in favour of #44028 that has the same changes.

/close

SIG Node PR Triage automation moved this from Triage to Done Nov 28, 2023
@k8s-ci-robot
Copy link
Contributor

@dipesh-rawat: Closed this PR.

In response to this:

Closing this in favour of #44028 that has the same changes.

/close

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. language/en Issues or PRs related to English language 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/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
Archived in project
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

7 participants