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

cri-api policies #566

Merged
merged 2 commits into from
Jan 26, 2025
Merged

Conversation

SergeyKanzhelev
Copy link
Member

Moving from kubernetes/website#49455

Policies of CRI API development.

The consensus was recieved via google doc: https://docs.google.com/document/d/1y42XrUPrm-6DZby1RQjexYYoNn822IRR6igsOiy_62c/edit?tab=t.0#heading=h.nzjfadot7nt7 and presented on both - SIG Node and SIG Atchitecture.

I am following the example of https://kubernetes.io/docs/reference/using-api/deprecation-policy/ placing it under the "Reference" section of docs.

/sig node

@k8s-ci-robot k8s-ci-robot added sig/node Categorizes an issue or PR as relevant to SIG Node. 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 Jan 23, 2025
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 23, 2025
@SergeyKanzhelev
Copy link
Member Author

/cc @sftim

@k8s-ci-robot k8s-ci-robot requested a review from sftim January 23, 2025 17:32
@SergeyKanzhelev SergeyKanzhelev changed the title [WIP] cri-api policies cri-api policies Jan 23, 2025
@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 Jan 23, 2025
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.

/hold

as there's feedback you could incorporate - if you'd like to. @SergeyKanzhelev this is OK to unhold; we expect that (and are confident that) you will follow our code of conduct and good practice.

/lgtm
/approve

content/en/docs/code/cri-api-policies.md Outdated Show resolved Hide resolved
content/en/docs/code/cri-api-policies.md Outdated Show resolved Hide resolved
content/en/docs/code/cri-api-policies.md Outdated Show resolved Hide resolved
@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 Jan 23, 2025
@k8s-ci-robot 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 23, 2025
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 23, 2025
@BenTheElder
Copy link
Member

BenTheElder commented Jan 24, 2025

I feel like either we should explain what leads us to these two runtimes in particular in a little detail and indirect through some name that refers to both (so the diff isn't huge if we add a third or something later, and so it's clear what the criteria is to be in that list) maybe "supported CRI implementations" or something like that? This doc winds up saying "cri-o and containerd" many times.

alternatively just say "must be in two implementations"?

@SergeyKanzhelev
Copy link
Member Author

I feel like either we should explain what leads us to these two runtimes in particular in a little detail and indirect through some name that refers to both (so the diff isn't huge if we add a third or something later, and so it's clear what the criteria is to be in that list) maybe "supported CRI implementations" or something like that? This doc winds up saying "cri-o and containerd" many times.

alternatively just say "must be in two implementations"?

We intentionally keeping it specific. It was the very first feedback from early drafts of the document. I put a notice on top and a whole section on two participating runtimes.

@SergeyKanzhelev SergeyKanzhelev force-pushed the cri-api-policy branch 2 times, most recently from c6a9e42 to d3c2bf6 Compare January 24, 2025 07:25
@sftim
Copy link
Contributor

sftim commented Jan 24, 2025

If we can merge this and iterate, let's do that.

@sftim
Copy link
Contributor

sftim commented Jan 24, 2025

@BenTheElder if we want to make it easy to add a third later, we can try to use the exact same string each occurrence, so that a replace operation is all that's needed.

@sftim
Copy link
Contributor

sftim commented Jan 24, 2025

/hold cancel

No current LGTM
/approve cancel

@k8s-ci-robot 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 24, 2025
@k8s-ci-robot k8s-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 24, 2025
@BenTheElder
Copy link
Member

We intentionally keeping it specific. It was the very first feedback from early drafts of the document. I put a notice on top and a whole section on two participating runtimes.

that's fine, I think we should be clear about what criteria would permit a third being included or cause one to be removed. That still doesn't seem to be addressed in much detail.

@SergeyKanzhelev SergeyKanzhelev force-pushed the cri-api-policy branch 2 times, most recently from 113147d to c58ceef Compare January 24, 2025 19:39
@@ -0,0 +1,5 @@
aliases:
sig-architecture-approvers:
Copy link
Member

Choose a reason for hiding this comment

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

thank you!

LGTM

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @dims - this helps me review!

weight: 51
---

CRI is a plugin interface which enables the kubelet to use a wide variety of container runtimes,
Copy link
Member Author

Choose a reason for hiding this comment

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

this file is mostly copied from kubernetes/kubernetes#129789

Copy link
Member Author

Choose a reason for hiding this comment

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

so it was approved for a long time, just moving it in the proper place

Copy link
Member

@samuelkarp samuelkarp left a comment

Choose a reason for hiding this comment

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

Largely looks good, but I have a few minor suggestions for wording

content/en/docs/code/cri-api-dev-policies.md Outdated Show resolved Hide resolved
content/en/docs/code/cri-api-dev-policies.md Outdated Show resolved Hide resolved
content/en/docs/code/cri-api-dev-policies.md Outdated Show resolved Hide resolved
content/en/docs/code/cri-api-dev-policies.md Outdated Show resolved Hide resolved
release of containerd and CRI-O implementing APIs needed for the feature in
those container runtimes. And those APIs **must not** be marked as experimental
features ([containerd experimental features](https://containerd.io/releases/#experimental-features)).
For the beta, neither container runtime has a notion of beta feature or release and
Copy link
Member

Choose a reason for hiding this comment

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

Speaking with my containerd hat on:

I don't think this needs to change the general policy for features in Kubernetes, but we may want to clarify that the notion of "beta" is not equivalent between Kubernetes and containerd.

Copy link
Contributor

Choose a reason for hiding this comment

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

In general, we could emphasize that external projects have their own ideas and jargon concerning stability (or not).

content/en/docs/code/cri-api-dev-policies.md Outdated Show resolved Hide resolved
content/en/docs/code/cri-api-dev-policies.md Outdated Show resolved Hide resolved
Copy link
Member

@samuelkarp samuelkarp 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 Jan 24, 2025
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.

/approve

@@ -0,0 +1,5 @@
aliases:
sig-architecture-approvers:
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @dims - this helps me review!

version'd kubelet and older versioned crictl. This is a supported scenario within
the version skew policy.

### Version Skew Policy for CRI API
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: should use sentence case

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: samuelkarp, SergeyKanzhelev, sftim

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 Jan 26, 2025
@k8s-ci-robot k8s-ci-robot merged commit 148e293 into kubernetes:master Jan 26, 2025
6 checks passed
@SergeyKanzhelev SergeyKanzhelev deleted the cri-api-policy branch January 27, 2025 17:55
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. lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/node Categorizes an issue or PR as relevant to SIG Node. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants