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.

Sign up for GitHub

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

Jump to bottom

Update CRI-API Capabilities to include a field that allows us to set … #104620

Merged
merged 1 commit into from
Jan 25, 2022
Merged

Update CRI-API Capabilities to include a field that allows us to set … #104620

merged 1 commit into from
Jan 25, 2022

Conversation

vinayakankugoyal
Copy link
Contributor

@vinayakankugoyal vinayakankugoyal commented Aug 26, 2021 •
edited
Loading

…ambient capabilities.

What type of PR is this?

/kind feature

What this PR does / why we need it:

It adds a field to the Capabilities message that allows us to add ambient capabilities to a container. CRI-O and containerd will need to be updated to respect the existence of this field and configure the container accordingly.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

go files generated using hack/update-generated-runtime.sh

Does this PR introduce a user-facing change?

Added field add_ambient_capabilities to the Capabilities message in the CRI-API.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

- [KEP]: https://github.com/kubernetes/enhancements/pull/2757

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Aug 26, 2021
@vinayakankugoyal
Copy link
Contributor Author

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Aug 26, 2021
@k8s-ci-robot k8s-ci-robot added area/kubelet sig/node Categorizes an issue or PR as relevant to SIG Node. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Aug 26, 2021
@vinayakankugoyal
Copy link
Contributor Author

/cc @tallclair

@k8s-ci-robot k8s-ci-robot requested a review from tallclair August 26, 2021 23:39
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 26, 2021
tallclair
tallclair reviewed Aug 30, 2021
View reviewed changes
Copy link
Member

@tallclair tallclair left a comment

Choose a reason for hiding this comment

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

This looks good to me, but you'll need signoff from sig-node

staging/src/k8s.io/cri-api/pkg/apis/runtime/v1/api.proto Show resolved Hide resolved
@qiutongs
Copy link
Contributor

qiutongs commented Sep 7, 2021

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 7, 2021
@ehashman
Copy link
Member

/assign @mrunalp

SergeyKanzhelev
SergeyKanzhelev reviewed Jan 18, 2022
View reviewed changes
staging/src/k8s.io/cri-api/pkg/apis/runtime/v1alpha2/api.proto
message Capability {
// List of capabilities to add.
repeated string add_capabilities = 1;
// List of capabilities to drop.
repeated string drop_capabilities = 2;
// List of ambient capabilities to add.
repeated string add_ambient_capabilities = 3;
Copy link
Member

Choose a reason for hiding this comment

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

do we have to keep updating the v1alpha2? Do we expect this to be implemented on Containerd 1.5, for example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I only expect this feature to be implemented in the latest version of containerd. I believe that would be 1.5.x wouldn't it? Sorry I am not very familiar with how CRI-API versions translate to containerd verisons.

Copy link
Member

Choose a reason for hiding this comment

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

containerd 1.6 is about to be released. it supports CRI v1, which means there is no need to fall back to v1alpha2.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack. I'll remove the v1alpha2 changes. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! PTAL. Thanks for all your help!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SergeyKanzhelev - looks like removing the changes from v1alpha2 causes TestV1alpha2ContainerConfig test to fail. I think we will need to make the changes in v1 and v1alpha2. WDYT?

@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 18, 2022
@SergeyKanzhelev
Copy link
Member

/test pull-kubernetes-unit

This raises an interesting question. On one hand, there is no reason and it confusing to update v1alpha2. On other, we are testing the absolute match between v1 and v1alpha2 as we need to ensure there is a way to convert one to another. It comes back to the policy as well: #107190 as easiest way just to keep everything at the latest version while the reality is that runtime may not implement the latest version.

Specifically for this PR, I think keeping versions in sync is better. sorry that you need to re-revet it back. I didn't realize this when was commenting above

@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 18, 2022
@vinayakankugoyal
Copy link
Contributor Author

/test pull-kubernetes-unit

This raises an interesting question. On one hand, there is no reason and it confusing to update v1alpha2. On other, we are testing the absolute match between v1 and v1alpha2 as we need to ensure there is a way to convert one to another. It comes back to the policy as well: #107190 as easiest way just to keep everything at the latest version while the reality is that runtime may not implement the latest version.

Specifically for this PR, I think keeping versions in sync is better. sorry that you need to re-revet it back. I didn't realize this when was commenting above

Thanks added the v1alpha2 changes back!

@vinayakankugoyal
Copy link
Contributor Author

/test pull-kubernetes-unit

@vinayakankugoyal
Copy link
Contributor Author

/test pull-kubernetes-e2e-kind-ipv6

@SergeyKanzhelev
Copy link
Member

/lgtm
/retest

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mrunalp, qiutongs, vinayakankugoyal

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 24, 2022
@vinayakankugoyal
Copy link
Contributor Author

/test pull-kubernetes-e2e-kind-ipv6

@SergeyKanzhelev
Copy link
Member

/retest

@vinayakankugoyal
Copy link
Contributor Author

/retest-required

@k8s-ci-robot k8s-ci-robot merged commit df21ff8 into kubernetes:master Jan 25, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.24 milestone Jan 25, 2022
@pacoxu pacoxu mentioned this pull request Aug 26, 2022
Open
6 tasks
@pacoxu pacoxu mentioned this pull request Mar 7, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tallclair tallclair tallclair left review comments

@SergeyKanzhelev SergeyKanzhelev SergeyKanzhelev left review comments

@qiutongs qiutongs qiutongs approved these changes

@mrunalp mrunalp mrunalp approved these changes

@bobbypage bobbypage Awaiting requested review from bobbypage

@vishh vishh Awaiting requested review from vishh

Assignees

@mrunalp mrunalp

@SergeyKanzhelev SergeyKanzhelev

@yujuhong yujuhong

@qiutongs qiutongs

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubelet cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/node Categorizes an issue or PR as relevant to SIG Node. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
SIG Node: code and documentation PRs
Archived in project
Milestone
v1.24
Development

Successfully merging this pull request may close these issues.
9 participants
@vinayakankugoyal @qiutongs @mmiranda96 @ehashman @SergeyKanzhelev @k8s-ci-robot @mrunalp @tallclair @yujuhong