Skip to content

[FG:InPlacePodVerticalScaling] Remove restrictions on subresource flag in kubectl commands #128296

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
Merged
merged 1 commit into from
Nov 6, 2024
Merged

[FG:InPlacePodVerticalScaling] Remove restrictions on subresource flag in kubectl commands #128296

merged 1 commit into from
Nov 6, 2024

Conversation

@AnishShah
Copy link
Contributor

@AnishShah AnishShah commented Oct 23, 2024 •
edited
Loading

What type of PR is this?

/kind feature
/sig cli

What this PR does / why we need it:

We are introducing a new resize subresource in #128266. We would like to add support for this subresource in kubectl. We are doing this by removing restrictions on the subresource flag instead of adding resize to the allowlist.

Which issue(s) this PR fixes:

Fixes #128278

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Removed restrictions on subresource flag in kubectl commands

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

- KEP: https://github.com/kubernetes/enhancements/tree/master/keps/sig-node/1287-in-place-update-pod-resources

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Oct 23, 2024

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

@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. release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. sig/cli Categorizes an issue or PR as relevant to SIG CLI. 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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 23, 2024
@k8s-ci-robot k8s-ci-robot added area/kubectl area/test kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Oct 23, 2024
@AnishShah AnishShah mentioned this pull request Oct 24, 2024
Merged
@AnishShah AnishShah force-pushed the kubectl-resize branch 2 times, most recently from 96b1c68 to 2f3957e Compare October 25, 2024 04:32
jpbetz referenced this pull request Oct 25, 2024
@AnishShah
support for resize subresource in ResourceQuota admission.
05e7cb2
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 31, 2024
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 1, 2024
@AnishShah AnishShah force-pushed the kubectl-resize branch 2 times, most recently from 084f338 to ef30edc Compare November 1, 2024 19:59
@AnishShah
Copy link
Contributor Author

AnishShah commented Nov 1, 2024

lgtm, if we're removing the hard-coded limits on which subresources support these, should AddSubresourceFlags stop taking an allowedSubresources param?

I removed this param.

With the removal of these restrictions, what happens if these commands are used on a subresource that takes a different schema? for example edit a replicaset/scale, or apply a pod/bind?

I got this error:

kubectl patch pod pause -p '{"metadata":{"labels":{"env":"staging"}}}' --subresource=bind  
Error from server (NotFound): the server could not find the requested resource

@liggitt
Copy link
Member

liggitt commented Nov 1, 2024

With the removal of these restrictions, what happens if these commands are used on a subresource that takes a different schema? for example edit a replicaset/scale, or apply a pod/bind?

I got this error:

kubectl patch pod pause -p '{"metadata":{"labels":{"env":"staging"}}}' --subresource=bind  
Error from server (NotFound): the server could not find the requested resource

Right, the operations just attempt to operate on the subresource specified... if the subresource doesn't exist or the submitted operation isn't valid for that subresource, it'll error.

@tallclair
Copy link
Member

tallclair commented Nov 3, 2024

/milestone v1.32

@k8s-ci-robot k8s-ci-robot added this to the v1.32 milestone Nov 3, 2024
@tjons tjons mentioned this pull request Nov 4, 2024
Open
95 tasks
@AnishShah
Copy link
Contributor Author

AnishShah commented Nov 6, 2024

kind ping @ardaguclu @soltysh since we are a few days away from code freeze.

#128266 is approved and ready to be merged. It would be good to get feedback on this PR so that the user can leverage new subresource via kubectl.

ardaguclu
ardaguclu reviewed Nov 6, 2024
View reviewed changes
@AnishShah
kubectl: remove subresource restrictions from all commands
e1ca634 
Removing this restrictions will allow us to use these commands with the
new resize subresource.
@ardaguclu
Copy link
Member

ardaguclu commented Nov 6, 2024

Thank you
/lgtm
/approve

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

k8s-ci-robot commented Nov 6, 2024

LGTM label has been added.

Git tree hash: 3c9834c7a9debbcf891f4466eaee9f0d4710f599

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Nov 6, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AnishShah, ardaguclu

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 Nov 6, 2024
@k8s-ci-robot k8s-ci-robot merged commit f451aec into kubernetes:master Nov 6, 2024
14 checks passed
@soltysh soltysh mentioned this pull request Nov 6, 2024
Closed
@AnishShah AnishShah mentioned this pull request Nov 6, 2024
Closed
@AnishShah AnishShah deleted the kubectl-resize branch November 6, 2024 17:56
@esotsal
Copy link
Contributor

esotsal commented Nov 7, 2024 •
edited
Loading

I think it is imperative to go through the documentation and update parts where patch is mentioned for InPlacePodVerticalScaling and update examples with the new approach. I was hit by this being unaware of the change, i assume others will be as well. posted in slack this, to create awareness, try to gather feedback and accelerate the process of writing equivalents, thanks @AnishShah for letting me know.

@esotsal esotsal mentioned this pull request Nov 7, 2024
Merged
This was referenced Feb 18, 2025
Closed
Merged
@brandond brandond mentioned this pull request Apr 15, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AxeZhan AxeZhan Awaiting requested review from AxeZhan

@brianpursley brianpursley Awaiting requested review from brianpursley

@mpuckett159 mpuckett159 Awaiting requested review from mpuckett159

@seans3 seans3 Awaiting requested review from seans3

@liggitt liggitt Awaiting requested review from liggitt

@soltysh soltysh Awaiting requested review from soltysh

2 more reviewers

@ardaguclu ardaguclu ardaguclu left review comments

@tallclair tallclair tallclair left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@ardaguclu ardaguclu

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubectl cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API 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. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cli Categorizes an issue or PR as relevant to SIG CLI. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Projects

SIG Apps
Archived in project
SIG CLI
Archived in project
SIG Node CI/Test Board
Archived in project
SIG Node: In Place Pod Vertical Scaling
Status: Done
SIG Node: code and documentation PRs
Archived in project
[sig-release] Bug Triage
Archived in project

Milestone

v1.32

Development

Successfully merging this pull request may close these issues.

[FG:InPlacePodVerticalScaling] kubectl subcommand to resize pod resources

7 participants

@AnishShah @k8s-ci-robot @liggitt @k8s-triage-robot @ardaguclu @tallclair @esotsal