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

Added hpa/v1 generator to kubectl autoscale #26775

Merged
merged 1 commit into from
Jun 11, 2016
Merged

Added hpa/v1 generator to kubectl autoscale #26775

merged 1 commit into from
Jun 11, 2016
+48 −17

Conversation

piosz
Copy link
Member

@piosz piosz commented Jun 3, 2016 •
edited
Loading

ref #21577

New default horizontalpodautoscaler/v1 generator for kubectl autoscale.
Use autoscaling/v1 in kubectl by default.

Analytics

@piosz piosz added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jun 3, 2016
@piosz piosz added this to the v1.3 milestone Jun 3, 2016
@piosz piosz mentioned this pull request Jun 3, 2016
Closed
4 tasks
@piosz
Copy link
Member Author

piosz commented Jun 3, 2016

cc @jszczepkowski @mwielgus @kubernetes/autoscaling

@k8s-github-robot k8s-github-robot added kind/old-docs size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 3, 2016
@piosz piosz changed the title Added hpa/v1 generator to kubectl autoscale [WIP] Added hpa/v1 generator to kubectl autoscale Jun 3, 2016
@jszczepkowski
Copy link
Contributor

LGTM for HPA part.

@piosz piosz changed the title [WIP] Added hpa/v1 generator to kubectl autoscale Added hpa/v1 generator to kubectl autoscale Jun 3, 2016
@piosz
Copy link
Member Author

piosz commented Jun 3, 2016

This also change kubectl to use autoscaling/v1 by default.
cc @lavalamp @kubernetes/sig-api-machinery

@piosz piosz mentioned this pull request Jun 3, 2016
Closed
janetkuo
janetkuo reviewed Jun 6, 2016
View reviewed changes
docs/user-guide/kubectl/kubectl_autoscale.md
@@ -64,7 +64,7 @@ kubectl autoscale rc foo --max=5 --cpu-percent=80
--cpu-percent=-1: The target average CPU utilization (represented as a percent of requested CPU) over all the pods. If it's not specified or negative, the server will apply a default value.
--dry-run[=false]: If true, only print the object that would be sent, without sending it.
-f, --filename=[]: Filename, directory, or URL to a file identifying the resource to autoscale.
--generator="horizontalpodautoscaler/v1beta1": The name of the API generator to use. Currently there is only 1 generator.
--generator="horizontalpodautoscaler/v1": The name of the API generator to use. Currently there are supported 2 generators: horizontalpodautoscaler/v1beta1 and horizontalpodautoscaler/v1.
Copy link
Member

Choose a reason for hiding this comment

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

nit: "2 supported generators"

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@piosz
Copy link
Member Author

piosz commented Jun 6, 2016

@janetkuo comments applied. PTAL

janetkuo
janetkuo reviewed Jun 6, 2016
View reviewed changes
hack/test-cmd.sh
# autoscale 1~2 pods, CPU utilization 70%, rc specified by file, using old generator
kubectl autoscale -f hack/testdata/frontend-controller.yaml "${kube_flags[@]}" --max=2 --cpu-percent=70 --generator=horizontalpodautoscaler/v1beta1
kube::test::get_object_assert 'hpa frontend' "{{$hpa_min_field}} {{$hpa_max_field}} {{$hpa_cpu_field}}" '1 2 70'
kubectl delete hpa frontend "${kube_flags[@]}"
# autoscale 2~3 pods, default CPU utilization (80%), rc specified by name
Copy link
Member

Choose a reason for hiding this comment

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

fix this comment also

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@janetkuo
Copy link
Member

janetkuo commented Jun 6, 2016

only nits

@piosz
Copy link
Member Author

piosz commented Jun 7, 2016

@k8s-bot e2e test this issue: #IGNORE

@k8s-github-robot
Copy link

@piosz
You must link to the test flake issue which caused you to request this manual re-test.
Re-test requests should be in the form of: k8s-bot test this issue: #<number>
Here is the list of open test flakes.

1 similar comment
@k8s-github-robot
Copy link

@piosz
You must link to the test flake issue which caused you to request this manual re-test.
Re-test requests should be in the form of: k8s-bot test this issue: #<number>
Here is the list of open test flakes.

@piosz
Copy link
Member Author

piosz commented Jun 7, 2016

@k8s-bot test this issue: #26431

@janetkuo
Copy link
Member

janetkuo commented Jun 7, 2016

After looking at it closer I found that both generators (v1 and v1beta1) are generating exactly the same autoscaling.HorizontalPodAutoscaler object.

The versioning of generator and the versioning of the API is actually unrelated. There's no need to add the new generator horizontalpodautoscaler/v1, unless we want to make it look more GA.

I'm curious whether this'd work against clusters that don't have extensions/v1beta1 or autoscaling/v1 hpa. Perhaps we should add service discovery (like what we had in pkg/kubectl/cmd/run.go) before using the generator in autoscale.

@kubernetes/kubectl

@piosz
Copy link
Member Author

piosz commented Jun 8, 2016

@janetkuo are you ok with just renaming the existing generator v1beta1 to just v1? In Kubernetes 1.3 both extensions/v1beta1 and autoscaling/v1 are supported though autoscaling is default option.

@janetkuo
Copy link
Member

janetkuo commented Jun 8, 2016

@piosz just renaming sounds good. To make it backward-compatible, we can create a v1 generator for hpa which is the same as v1beta1 generator, but hide the v1beta1 one in help text.

@piosz
Copy link
Member Author

piosz commented Jun 9, 2016

I updated flag description. PTAL

@janetkuo janetkuo added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 9, 2016
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 10, 2016
@piosz piosz added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jun 10, 2016
@k8s-bot
Copy link

k8s-bot commented Jun 11, 2016

GCE e2e build/test passed for commit 1818b5b.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 0f24d00 into kubernetes:master Jun 11, 2016
@piosz piosz deleted the generator branch June 11, 2016 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@janetkuo janetkuo

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.3
Development

Successfully merging this pull request may close these issues.
6 participants
@piosz @jszczepkowski @janetkuo @k8s-github-robot @k8s-bot @googlebot