(feat: scale) limitranger allow pod update #124887

mouuii · 2024-05-15T13:07:52Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

The LimitRanger admission plugin does not currently handle pod updates, but needs to be updated to account for InPlacePodVerticalScaling:

Which issue(s) this PR fixes:

Fixes #124855

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 · 2024-05-15T13:08:01Z

Hi @mouuii. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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-sigs/prow repository.

mouuii · 2024-05-15T13:24:40Z

/sig node

mouuii · 2024-05-15T13:28:16Z

/cc tallclair

iholder101 · 2024-05-15T14:54:29Z

/release-note-none
/ok-to-test

Looks good overall.
Would you mind adding an e2e test?

iholder101 · 2024-05-15T14:54:44Z

/cc

mouuii · 2024-05-15T15:35:28Z

/release-note-none /ok-to-test

Looks good overall. Would you mind adding an e2e test?

I'm not familiar with writing e2e and I think this change will only affect performance ？

ffromani · 2024-05-15T16:34:09Z

should the changes done here be gated by the InPlaceVerticalAutoscaling featuregate, like we do e.g. in the kubelet?

iholder101 · 2024-05-15T16:36:34Z

should the changes done here be gated by the InPlaceVerticalAutoscaling featuregate, like we do e.g. in the kubelet?

Good point. +1

iholder101 · 2024-09-05T09:24:10Z

@mouuii Are you still willing to work on this PR?
If so, I'll be happy to help move this forward. If not, I'll gladly pick it up. Let me know! :)

mouuii · 2024-09-08T01:09:33Z

@mouuii Are you still willing to work on this PR? If so, I'll be happy to help move this forward. If not, I'll gladly pick it up. Let me know! :)

yes @iholder101

googs1025 · 2024-09-20T09:47:33Z

@mouuii Are you still willing to work on this PR? If so, I'll be happy to help move this forward. If not, I'll gladly pick it up. Let me know! :)

yes @iholder101

Are you still willing to work on this PR?
If you are not in charge of this work, is it possible for someone else to be involved?
It looks like we need to add e2e tests or integration tests (if needed).
cc @iholder101

googs1025 · 2024-09-20T09:50:25Z

plugin/pkg/admission/limitranger/admission_test.go

@@ -743,8 +743,8 @@ func TestLimitRangerIgnoresSubresource(t *testing.T) {
 		t.Errorf("Expected an error since the pod did not specify resource limits in its create call")
 	}
 	err = handler.Validate(context.TODO(), admission.NewAttributesRecord(&testPod, nil, api.Kind("Pod").WithVersion("version"), limitRange.Namespace, "testPod", api.Resource("pods").WithVersion("version"), "", admission.Update, &metav1.UpdateOptions{}, false, nil), nil)
-	if err != nil {
-		t.Errorf("Expected not to call limitranger actions on pod updates")
+	if err == nil {


why err == nil but print t.Errorf(

Test cases are needed to verify whether the code we modified complies with the logic. It is not just for passing the test.

to allow pod update , previous we do not allow update.
what kind of test cases ? can you show me how to write?
@googs1025

First, you need to modify the unit test code itself, not just to pass the tests.... Second, since you've made changes, you should incrementally cover the logic of your changes to ensure the code's rationality.

If you encounter difficulties during the process, perhaps you could consider handing over this pr to someone else.

k8s-ci-robot · 2024-09-21T08:37:21Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mouuii
Once this PR has been reviewed and has the lgtm label, please assign jpbetz 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:

plugin/pkg/admission/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

mouuii · 2024-09-23T00:21:36Z

/restest

mouuii · 2024-09-24T01:25:46Z

/retest

mouuii · 2024-09-24T02:58:45Z

/test pull-kubernetes-e2e-gce

mouuii · 2024-09-24T05:23:52Z

/test pull-kubernetes-e2e-gce

mouuii · 2024-09-25T01:02:49Z

This is ready. PTAL.@iholder101 @tallclair

googs1025 · 2024-09-25T09:08:22Z

/release-note-none /ok-to-test

Looks good overall. Would you mind adding an e2e test?

According to this comment, we need to add e2e or integration tests to ensure that the changes are reasonable. :)

tallclair · 2024-10-14T23:45:44Z

/assign

AnishShah · 2024-10-19T07:07:29Z

Can we add some e2e tests for this? For eg, we have pod resize e2e tests with ResourceQuota here

AnishShah · 2024-10-24T21:04:25Z

plugin/pkg/admission/limitranger/admission.go

-	// to mutate and validate limitrange on pod updates. Trying to mutate containers or initContainers on a pod
-	// update request will always fail pod validation because those fields are immutable once the object is created.
-	if a.GetKind().GroupKind() == api.Kind("Pod") && a.GetOperation() == admission.Update {
+	if a.GetKind().GroupKind() == api.Kind("Pod") && a.GetOperation() == admission.Update && !feature.DefaultFeatureGate.Enabled(features.InPlacePodVerticalScaling) {


FYI, This won't work after #128266 as we add resize subresource to do pod resize.

Do we need this logic after using resize subresource ? @AnishShah

Yes, we still need the logic. But instead of pod update, we need it during resize subresource. I provisionally added the logic in my PR

so this pr can close after your pr merged ? and e2e test can be writen in your pr ? @AnishShah

Yes, I added e2e tests in 1b93d0f

tallclair · 2024-11-04T19:12:21Z

This fix has been rolled into #128266

/close

k8s-ci-robot · 2024-11-04T19:12:27Z

@tallclair: Closed this PR.

In response to this:

This fix has been rolled into #128266

/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-sigs/prow repository.

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels May 15, 2024

k8s-ci-robot requested review from derekwaynecarr and yue9944882 May 15, 2024 13:08

mouuii force-pushed the limit-range-allow-pod-update branch from 84d2679 to afb96b0 Compare May 15, 2024 13:22

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 May 15, 2024

k8s-ci-robot added 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 May 15, 2024

k8s-ci-robot requested a review from tallclair May 15, 2024 13:28

k8s-ci-robot requested a review from iholder101 May 15, 2024 14:54

mouuii changed the title ~~limit range allow pod update~~ limitranger allow pod update May 15, 2024

k8s-ci-robot removed 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 May 20, 2024

googs1025 reviewed Sep 20, 2024

View reviewed changes

k8s-ci-robot added the do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. label Sep 21, 2024

mouuii changed the title ~~limitranger allow pod update~~ [wip]limitranger allow pod update Sep 24, 2024

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 24, 2024

mouuii changed the title ~~[wip]limitranger allow pod update~~ limitranger allow pod update Sep 24, 2024

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 Sep 24, 2024

allow limitranger limit

9ee7511

mouuii force-pushed the limit-range-allow-pod-update branch from 57857f2 to 9ee7511 Compare September 24, 2024 05:22

k8s-ci-robot removed the do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. label Sep 24, 2024

mouuii changed the title ~~limitranger allow pod update~~ (feat: scale) limitranger allow pod update Oct 11, 2024

k8s-ci-robot assigned tallclair Oct 14, 2024

tallclair mentioned this pull request Oct 24, 2024

[FG:InPlacePodVerticalScaling] Introduce /resize subresource to request pod resource resizing #128266

Merged

AnishShah reviewed Oct 24, 2024

View reviewed changes

k8s-ci-robot closed this Nov 4, 2024

(feat: scale) limitranger allow pod update #124887

(feat: scale) limitranger allow pod update #124887

Uh oh!

Conversation

mouuii commented May 15, 2024

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

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

Uh oh!

k8s-ci-robot commented May 15, 2024

Uh oh!

mouuii commented May 15, 2024

Uh oh!

mouuii commented May 15, 2024

Uh oh!

iholder101 commented May 15, 2024

Uh oh!

iholder101 commented May 15, 2024

Uh oh!

mouuii commented May 15, 2024

Uh oh!

ffromani commented May 15, 2024

Uh oh!

iholder101 commented May 15, 2024

Uh oh!

iholder101 commented Sep 5, 2024

Uh oh!

mouuii commented Sep 8, 2024

Uh oh!

googs1025 commented Sep 20, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

googs1025 Sep 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

googs1025 Sep 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Sep 21, 2024

Uh oh!

mouuii commented Sep 23, 2024

Uh oh!

mouuii commented Sep 24, 2024

Uh oh!

mouuii commented Sep 24, 2024

Uh oh!

mouuii commented Sep 24, 2024

Uh oh!

mouuii commented Sep 25, 2024

Uh oh!

googs1025 commented Sep 25, 2024

Uh oh!

tallclair commented Oct 14, 2024

Uh oh!

AnishShah commented Oct 19, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tallclair commented Nov 4, 2024

Uh oh!

googs1025 Sep 20, 2024 •

edited

Loading

googs1025 Sep 21, 2024 •

edited

Loading