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

fixed the container runtime documentation #48398

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

thisisharrsh
Copy link
Contributor

Fixes : #48257

@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. labels Oct 17, 2024
@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 salaxander 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 size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 17, 2024
Copy link

netlify bot commented Oct 17, 2024

Pull request preview available for checking

Built without sensitive environment variables

Name Link
🔨 Latest commit fe88f18
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/671b6342f7850600085bbbe1
😎 Deploy Preview https://deploy-preview-48398--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.

@thisisharrsh thisisharrsh marked this pull request as ready for review October 17, 2024 05:33
@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 Oct 17, 2024
@@ -171,7 +171,7 @@ follow [configuring a cgroup driver](/docs/tasks/administer-cluster/kubeadm/conf

## CRI version support {#cri-versions}

Your container runtime must support at least v1alpha2 of the container runtime interface.
Your container runtime must support at least v1 of the container runtime interface.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Your container runtime must support at least v1 of the container runtime interface.
Your container runtime must support v1 of the container runtime interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted! will apply those chnages.

@thisisharrsh
Copy link
Contributor Author

I have made the suggested changes, PTAL.

@SergeyKanzhelev
Copy link
Member

Please address this comment as well: #48398 (comment)

@thisisharrsh
Copy link
Contributor Author

thisisharrsh commented Oct 24, 2024

Please address this comment as well: #48398 (comment)

The v{{< skew currentVersion >}} kubelet can also negotiate CRI `v1alpha2`, but
this version is considered as deprecated.

I am changing the doc here as considered as deprecated and removed.
WDYT?

@SergeyKanzhelev
Copy link
Member

This statement is invalid and need to be changed to simply say that no other versions are supported by the kubelet now:

For Kubernetes v{{< skew currentVersion >}}, the kubelet prefers to use CRI `v1`.
If a container runtime does not support `v1` of the CRI, then the kubelet tries to
negotiate any older supported version.

@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 Oct 25, 2024
@thisisharrsh
Copy link
Contributor Author

Incorporated the suggested changes. 
PTAL

@thisisharrsh
Copy link
Contributor Author

Comment on lines 31 to +33
For Kubernetes v{{< skew currentVersion >}}, the kubelet prefers to use CRI `v1`.
If a container runtime does not support `v1` of the CRI, then the kubelet tries to
negotiate any older supported version.
The v{{< skew currentVersion >}} kubelet can also negotiate CRI `v1alpha2`, but
this version is considered as deprecated.
If the kubelet cannot negotiate a supported CRI version, the kubelet gives up
and doesn't register as a node.
If a container runtime does not support `v1` of the CRI, then the kubelet cannot register as a node.
The v{{< skew currentVersion >}} kubelet only supports CRI `v1`. The deprecated `v1alpha2` API is no longer supported. If the kubelet cannot connect to a container runtime using CRI `v1`, it will give up and will not register as a node.
Copy link
Member

Choose a reason for hiding this comment

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

now these two paragraphs are saying the same thing twice. Also instead of saying "kubelet gives up" please clarify. I think it still registers the node. It just never removes the Not Ready status.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @SergeyKanzhelev, It would be great if you could suggest a possible change in the documentation. So that i can directly push those changes.

Copy link
Member

Choose a reason for hiding this comment

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

This is basically the task - review docs and update them. Sorry, I only have time to review changes and give a quick feedback. Maybe after code freeze I can find more time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no problem!
I'll do the needful.

@sftim
Copy link
Contributor

sftim commented Dec 12, 2024

@thisisharrsh - would you like to update the PR to incorporate pending feedback?

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. language/en Issues or PRs related to English language size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CRI API reference at Container Runtime page is stale
4 participants