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

Refactor `kubectl scale` to patch scale subresource #81342

Merged
merged 2 commits into from Aug 26, 2019

Conversation

@knight42
Copy link
Contributor

commented Aug 13, 2019

What type of PR is this?
/kind feature

What this PR does / why we need it:
Currently kubectl scale would send a PUT request without ResourceVersion, which is introduced in #75210, and because of that custom resource cannot be scaled since custom resource could not be updated unconditionally.

Considered that PUT without resource version is not a best practice and we are not willing to do a GET before the request, so I choose to update kubectl to use PATCH instead of PUT.

Which issue(s) this PR fixes:
Part of #80515

Special notes for your reviewer:
Although I have created another PR to fix the problem, which requires changes on apiserver, the apiserver would not be updated as frequently as kubectl in reality, so it may be best to update both apiserver(make old kubectls work) and kubectl.

Does this PR introduce a user-facing change?:

kubectl could scale custom resource again

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


@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2019

Hi @knight42. 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/test-infra repository.

@k8s-ci-robot k8s-ci-robot requested review from dims and soltysh Aug 13, 2019
@knight42

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2019

/assign @seans3

cc @liggitt

@seans3

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

/ok-to-test

@knight42

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2019

/assign @smarterclayton

@knight42

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2019

@seans3 @smarterclayton all tests passed now, PTAL

@xmudrii

This comment has been minimized.

Copy link
Member

commented Aug 20, 2019

@seans3 @liggitt Ping. Can anyone help with reviewing this PR?

@dims

This comment has been minimized.

Copy link
Member

commented Aug 20, 2019

/uncc

@k8s-ci-robot k8s-ci-robot removed the request for review from dims Aug 20, 2019
@seans3

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2019

/cc @apelisse

for more context

@k8s-ci-robot k8s-ci-robot requested a review from apelisse Aug 21, 2019
@knight42 knight42 force-pushed the knight42:fix/kubectl-patch-scale branch 2 times, most recently from 5147a8b to 66d8056 Aug 22, 2019
@knight42

This comment has been minimized.

Copy link
Contributor Author

commented Aug 22, 2019

/retest

Copy link
Member

left a comment

Few style nits, but otherwise looks good. Feel free to fix or not
/lgtm
/approve
/hold

staging/src/k8s.io/kubectl/pkg/scale/scale.go Outdated Show resolved Hide resolved
@knight42 knight42 force-pushed the knight42:fix/kubectl-patch-scale branch from 66d8056 to 92c4c2e Aug 23, 2019
@k8s-ci-robot k8s-ci-robot removed the lgtm label Aug 23, 2019
@knight42

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2019

@apelisse all fixed now

@apelisse

This comment has been minimized.

Copy link
Member

commented Aug 23, 2019

Thanks, I think it looks much better this way,
/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label Aug 23, 2019
@smarterclayton

This comment has been minimized.

Copy link
Contributor

commented Aug 26, 2019

/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 26, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: apelisse, knight42, smarterclayton

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

@knight42

This comment has been minimized.

Copy link
Contributor Author

commented Aug 26, 2019

@apelisse @smarterclayton would you mind unholding this PR?

@apelisse

This comment has been minimized.

Copy link
Member

commented Aug 26, 2019

/hold cancel

@k8s-ci-robot k8s-ci-robot merged commit f81ab5a into kubernetes:master Aug 26, 2019
23 checks passed
23 checks passed
cla/linuxfoundation knight42 authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-iscsi Skipped.
pull-kubernetes-e2e-gce-iscsi-serial Skipped.
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
@k8s-ci-robot k8s-ci-robot added this to the v1.16 milestone Aug 26, 2019
@knight42 knight42 deleted the knight42:fix/kubectl-patch-scale branch Sep 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.