KEP-3619: Add NodeStatus.Features.SupplementalGroupsPolicy API and e2e #125470

everpeace · 2024-06-12T13:39:46Z

Depended by:

What this PR does / why we need it:

KEP: kubernetes/enhancements#3619
KEP PR for the API changes: kubernetes/enhancements#4728

Add NodeFeatures.SupplementGroupsPolicy in kubernetes api
Add RuntimeFeatures.SupplementalGroupsPolicy in cri-api
- this can also help to merge my working cri-tool's PR: KEP-3619: Fine-grained SupplementalGroups control kubernetes-sigs/cri-tools#1438 before container runtimes releases
- I will update the cri-api dependency once the PR got merged
Implements e2e test for this KEP
- the test will skip when the feature is not supported in Node.Status.Features.SupplementalGroupsPolicy

Special notes for your reviewer:

#117842 implemented API/CRI and k/k logics for this KEP. But, it didn't include E2E test. Moreover, I also realized cri-tools's my working PR(kubernetes-sigs/cri-tools#1438) is difficult to merge because critest's CI depends on containerd's previous versions.

Because the core logic for this KEP is implemented in container runtimes, E2E should detect whether the feature is supported or not at CRI level　so that we can implement safe E2E which can gracefully skip it when the runtime does not support this feature. To do this, this PR added feature fields in RuntimeHandlerFeature both in CRI and kubernetes API.

Once the container runtime in E2E gets bumped and it supports this feature, the E2E for this KEP will be automatically activated.

Does this PR introduce a user-facing change?

Added Node.Status.Features.SupplementalGroupsPolicy field which is set to true when the feature is implemented in the CRI implementation (KEP-3619)

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

- [KEP]: https://github.com/kubernetes/enhancements/tree/master/keps/sig-node/3619-supplemental-groups-policy

k8s-ci-robot · 2024-06-12T13:39:50Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

mrunalp · 2024-07-15T21:51:03Z

pkg/apis/core/types.go

@@ -4914,6 +4916,15 @@ type NodeRuntimeHandler struct {
 	Features *NodeRuntimeHandlerFeatures
 }

+// NodeFeatures describes the set of features implemented by the CRI implementation.
+// The features contained in the NodeFeatures should depend on only cri implementation, be independent of runtime handlers,
+// (i.e. it should not require inspecting any information from the OCI runtime-spec's features).


Same as my previous comment in the API description.

Fixed in

K8s API: 5d75660

CRI: 38e4c6b

mrunalp · 2024-07-15T21:51:56Z

pkg/kubelet/container/runtime.go

@@ -556,6 +556,8 @@ type RuntimeStatus struct {
 	Conditions []RuntimeCondition
 	// Handlers is an array of current available handlers
 	Handlers []RuntimeHandler
+	// Features is the set of features implmented by the runtime


typo: implemented

Thanks! Fixed in f46ecf5

mrunalp · 2024-07-15T21:52:19Z

pkg/kubelet/container/runtime.go

+	SupplementalGroupsPolicy bool
+}
+
+// String formats the runtime condition into human readable string.


into a human

fixed in f46ecf5

mrunalp · 2024-07-15T21:54:26Z

staging/src/k8s.io/api/core/v1/types.go

-// NodeRuntimeHandlerFeatures is a set of runtime features.
+// NodeRuntimeHandlerFeatures is a set of features implemented by the runtime handler.
+// The features contained in the NodeRuntimeHandlerFeatures should depend on the runtime handlers,
+// (i.e. dependent to the information exposed by the OCI runtime-spec's features).


Same comment as above.

Fixed in

K8s API: 5d75660

CRI: 38e4c6b

mrunalp · 2024-07-15T21:54:34Z

staging/src/k8s.io/api/core/v1/types.go

@@ -5891,6 +5893,15 @@ type NodeRuntimeHandler struct {
 	Features *NodeRuntimeHandlerFeatures `json:"features,omitempty" protobuf:"bytes,2,opt,name=features"`
 }

+// NodeFeatures describes the set of features implemented by the CRI implementation.
+// The features contained in the NodeFeatures should depend on only cri implementation, be independent of runtime handlers,
+// (i.e. it should not require inspecting any information from the OCI runtime-spec's features).


Same comment as above.

Fixed in

K8s API: 5d75660

CRI: 38e4c6b

…Response KEP-3619: don't capitalize comment in CRI KEP-3619: fix typos and grammatical ones in CRI KEP-3619: rephrase RuntimeFeatures, RuntimeHandlerFeatures comment in CRI

KEP-3619: don't capitalize comment in K8S API KEP-3619: fix typos and grammatical ones in K8s API KEP-3619: rephrase NodeFeatures, NodeHandlerFeatures in K8s API

…) to NodeFeatures.SupplementalGroupsPolicy(API) KEP-3619: fix typos in pkg/kubelet/container/runtime.go

…ropDisabledFields for node

everpeace

@mrunalp Thanks for your review! I addressed all your comments. And, I cleaned up my commits and rebased to the latest master.

PTAL🙇

mrunalp

/lgtm

k8s-ci-robot · 2024-07-16T18:49:38Z

LGTM label has been added.

Git tree hash: f26619eb4b2ebf9f17cbb87ffb35410f12d81053

thockin · 2024-07-16T19:30:17Z

/approve

k8s-ci-robot · 2024-07-16T19:30:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: everpeace, mrunalp, thockin

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:

~~api/OWNERS~~ [thockin]
~~pkg/apis/OWNERS~~ [thockin]
~~pkg/generated/openapi/OWNERS~~ [thockin]
~~pkg/kubelet/OWNERS~~ [mrunalp,thockin]
~~pkg/registry/core/node/OWNERS~~ [mrunalp,thockin]
~~staging/src/k8s.io/api/OWNERS~~ [thockin]
~~staging/src/k8s.io/client-go/applyconfigurations/OWNERS~~ [thockin]
~~staging/src/k8s.io/cri-api/OWNERS~~ [mrunalp,thockin]
~~test/e2e/feature/OWNERS~~ [mrunalp,thockin]
~~test/e2e/node/OWNERS~~ [mrunalp,thockin]

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 requested review from andrewsykim and bart0sh June 12, 2024 13:40

This comment was marked as off-topic.

Sign in to view

This comment has been minimized.

Sign in to view

everpeace marked this pull request as ready for review June 13, 2024 07:24

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 Jun 13, 2024

This comment was marked as outdated.

Sign in to view

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jun 13, 2024

k8s-ci-robot requested review from caesarxuchao and feiskyer June 13, 2024 07:24

mrunalp reviewed Jul 15, 2024

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 16, 2024

everpeace force-pushed the kep-3619-SupplementalGroupsPolicy-e2e branch from b4006ce to 775f05d Compare July 16, 2024 03:11

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 16, 2024

k8s-ci-robot requested a review from haircommander July 16, 2024 03:11

everpeace added 5 commits July 16, 2024 12:11

KEP-3619: CRI: add RuntimeFeatures.SupplementalGroupsPolicy in Status…

38e4c6b

…Response KEP-3619: don't capitalize comment in CRI KEP-3619: fix typos and grammatical ones in CRI KEP-3619: rephrase RuntimeFeatures, RuntimeHandlerFeatures comment in CRI

KEP-3619: API: add NodeFeatures.SupplementalGroupsPolicy in NodeStatus

5d75660

KEP-3619: don't capitalize comment in K8S API KEP-3619: fix typos and grammatical ones in K8s API KEP-3619: rephrase NodeFeatures, NodeHandlerFeatures in K8s API

KEP-3619: Wiring up from RuntimeFeatures.SupplementalGroupsPolicy(CRI…

f46ecf5

…) to NodeFeatures.SupplementalGroupsPolicy(API) KEP-3619: fix typos in pkg/kubelet/container/runtime.go

KEP-3619: implement e2e test for SupplementalGroupsPolicy

a89bb01

KEP-3619: should check supplementalGroupsPolicyInUse for oldNode in d…

1663223

…ropDisabledFields for node

everpeace force-pushed the kep-3619-SupplementalGroupsPolicy-e2e branch from 775f05d to 1663223 Compare July 16, 2024 03:36

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 16, 2024

everpeace commented Jul 16, 2024

View reviewed changes

mrunalp approved these changes Jul 16, 2024

View reviewed changes

k8s-ci-robot assigned mrunalp Jul 16, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 16, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 16, 2024

k8s-ci-robot merged commit fc3abda into kubernetes:master Jul 16, 2024
16 checks passed

k8s-ci-robot added this to the v1.31 milestone Jul 16, 2024

everpeace mentioned this pull request Jul 17, 2024

KEP-3619: implement RuntimeStatus.features.supplemental_groups_policy cri-o/cri-o#8386

Merged

everpeace deleted the kep-3619-SupplementalGroupsPolicy-e2e branch July 17, 2024 13:54

everpeace mentioned this pull request Oct 7, 2024

KEP-3619: updated Production Readiness Review Questionnaire for beta release kubernetes/enhancements#4895

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KEP-3619: Add NodeStatus.Features.SupplementalGroupsPolicy API and e2e #125470

KEP-3619: Add NodeStatus.Features.SupplementalGroupsPolicy API and e2e #125470

everpeace commented Jun 12, 2024 •

edited

Loading

k8s-ci-robot commented Jun 12, 2024

This comment was marked as off-topic.

This comment has been minimized.

This comment was marked as outdated.

mrunalp Jul 15, 2024

everpeace Jul 16, 2024

mrunalp Jul 15, 2024

everpeace Jul 16, 2024

mrunalp Jul 15, 2024

everpeace Jul 16, 2024

mrunalp Jul 15, 2024

everpeace Jul 16, 2024

mrunalp Jul 15, 2024

everpeace Jul 16, 2024

everpeace left a comment

mrunalp left a comment

k8s-ci-robot commented Jul 16, 2024

thockin commented Jul 16, 2024

k8s-ci-robot commented Jul 16, 2024

KEP-3619: Add NodeStatus.Features.SupplementalGroupsPolicy API and e2e #125470

KEP-3619: Add NodeStatus.Features.SupplementalGroupsPolicy API and e2e #125470

Conversation

everpeace commented Jun 12, 2024 • edited Loading

What this PR does / why we need it:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

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

k8s-ci-robot commented Jun 12, 2024

This comment was marked as off-topic.

This comment has been minimized.

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

everpeace left a comment

Choose a reason for hiding this comment

mrunalp left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 16, 2024

thockin commented Jul 16, 2024

k8s-ci-robot commented Jul 16, 2024

everpeace commented Jun 12, 2024 •

edited

Loading