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

Add comprehensive horizontal pod autoscaling documentation #3942

Merged
merged 7 commits into from
Mar 6, 2018
Merged

Add comprehensive horizontal pod autoscaling documentation #3942

merged 7 commits into from
Mar 6, 2018

Conversation

itskingori
Copy link
Member

As requested by @chrislovecnm in #3939 (review).

Please note that this is a first pass. I'm currently trying to set up HPA and these are the things I've had to set. I'm yet to actually roll out the changes to the cluster and so I might update these docs with new discoveries.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 28, 2017
zacblazic
zacblazic reviewed Nov 28, 2017
View reviewed changes
docs/horizontal_pod_autoscaling.md Outdated
--proxy-client-cert-file=<path to aggregator proxy cert>
--proxy-client-key-file=<path to aggregator proxy key>
```
* [kubernetes/kops#3679][4] - add config option to set `--horizontal-pod-autoscaler-use-rest-clients` kube-controller-manager flag required to enable custom metrics
Copy link
Contributor

Choose a reason for hiding this comment

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

The link name kubernetes/kops#3679 should be kubernetes/kops#3939.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in 7625898.

zacblazic
zacblazic reviewed Nov 28, 2017
View reviewed changes
docs/horizontal_pod_autoscaling.md Outdated
Kops can assist in setting up HPA and recommends Kubernetes `>= 1.7` and Kops
`>=1.8`. Relevant reading you will need to go through:

* [Configure The Aggregation Layer][1].
Copy link
Contributor

Choose a reason for hiding this comment

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

No periods at the end looks nicer IMO.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in 7772c44.

@mikesplain
Copy link
Contributor

If we're breaking out HPA docs, can we link to it from here: https://github.com/kubernetes/kops/blob/master/docs/cluster_spec.md#kubecontrollermanager

@chrislovecnm
Copy link
Contributor

/ok-to-test
/lgtm

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 28, 2017
@itskingori
Copy link
Member Author

@mikesplain yeah, there's some duplication 😓 ... both docs makes sense. I think your suggestion makes sense.

@chrislovecnm
Copy link
Contributor

/lgtm cancel

@k8s-ci-robot k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 28, 2017
@chrislovecnm
Copy link
Contributor

Thinking about something. Does this feature use the api aggregation server? I know we have some nasty bugs in 1.8 that are being worked on

@itskingori
Copy link
Member Author

itskingori commented Nov 28, 2017 •
edited
Loading

If we're breaking out HPA docs, can we link to it ...

@mikesplain addressed in c38aa8b.

Thinking about something. Does this feature use the api aggregation server? I know we have some nasty bugs in 1.8 that are being worked on

@chrislovecnm yes it does. It's no problem. We can keep this open for discussion until everything has been resolved.

@chrislovecnm
Copy link
Contributor

@itskingori should we merge with a note about the bug, or just keep this open? thoughts? I am leaning toward keeping this open.

@chrislovecnm
Copy link
Contributor

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 28, 2017
@itskingori
Copy link
Member Author

@chrislovecnm we can keep it open. I'm currently setting up HPA on 1.7 and can keep updating this as I roll it out).

@chrislovecnm chrislovecnm mentioned this pull request Nov 29, 2017
Open
@23doors
Copy link

23doors commented Jan 7, 2018

Any progress on this? What about custom metrics aggregator https://github.com/kubernetes/metrics/blob/master/IMPLEMENTATIONS.md ? Guess it still needs to be deployed.

@mikesplain
Copy link
Contributor

@chrislovecnm Any updates on this? I think it'd be good to get these docs in eventually. I believe the 1.8 bugs have since been resolved and this could be a good starting place for future HPA docs. Thoughts?

@mikesplain
Copy link
Contributor

@itskingori any other recommended updates since you last touched this?

/assign

@itskingori
Copy link
Member Author

@mikesplain Oh my, forgot about this. Learnt quite a bit, there's quite a bit to add. Will make some time tomorrow.

@justinsb justinsb added this to the 1.9 milestone Feb 21, 2018
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 5, 2018
@mikesplain
Copy link
Contributor

Ping @itskingori :) Would love to get the docs updated with anything you've learned for 1.9!

@mikesplain
Copy link
Contributor

Oh that's why I got pinged, nevermind!

@itskingori
Copy link
Member Author

itskingori commented Mar 5, 2018 •
edited
Loading

@mikesplain yeah 😅 ... added some notes. I'm kinda torn in the sense that I don't want to regurgitate what's in the docs. So I've made mention of everything that's needed i.e. all the necessary configuration (from an implementation point of view), the cluster config that one needs and the metrics servers (resource and custom) that need to the registered (needs kubectl apply only).

I'm wondering if having the manifest files for the metrics servers in kops as addons is a good idea or just send people to the relevant repos/docs. 🤔

And sorry for the delay, I promised this sooner.

@itskingori itskingori mentioned this pull request Mar 5, 2018
Merged
@itskingori
Copy link
Member Author

I'm wondering if having the manifest files for the metrics servers in kops as addons is a good idea or just send people to the relevant repos/docs. 🤔

@mikesplain about this ... decided to create a separate PR, see #4581.

@mikesplain
Copy link
Contributor

This is great @itskingori! Thanks for all the detail! I agree that breaking that into a separate PR is a good call.

cc: @chrislovecnm I think this looks good now if you want to take another look and consider removing the hold?

/area documentation
/lgtm

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. area/documentation labels Mar 5, 2018
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 5, 2018
@itskingori
Copy link
Member Author

@chrislovecnm I never quite responded to your suggestion in #3942 (comment). I've addressed it in 929f299. @mikesplain added one last commit 🙈 post-LGTM.

@23doors about #3942 (comment), addressed in 472604c. See also #4581.

@mikesplain
Copy link
Contributor

Awesome, good fix!

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 5, 2018
@chrislovecnm chrislovecnm removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 5, 2018
@chrislovecnm
Copy link
Contributor

/lgtm

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chrislovecnm, itskingori, mikesplain

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 Mar 5, 2018
@k8s-ci-robot k8s-ci-robot merged commit 0f4c4e7 into kubernetes:master Mar 6, 2018
@itskingori itskingori deleted the add_hpa_docs branch March 6, 2018 06:34
@itskingori itskingori mentioned this pull request Mar 12, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zacblazic zacblazic zacblazic left review comments

@chrislovecnm chrislovecnm chrislovecnm approved these changes

Assignees

@chrislovecnm chrislovecnm

@mikesplain mikesplain

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/documentation cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
1.9.0
Development

Successfully merging this pull request may close these issues.
7 participants
@itskingori @mikesplain @chrislovecnm @23doors @k8s-ci-robot @zacblazic @justinsb