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

HPA: only send updates when the status has changed #47078

Merged

Conversation

DirectXMan12
Copy link
Contributor

This commit only sends updates if the status has actually changed.
Since the HPA runs at a regular interval, this should reduce the volume
of writes, especially on short HPA intervals with relatively constant
metrics.

Fixes #47077

Release note:

The HorizontalPodAutoscaler controller will now only send updates when it has new status information, reducing the number of writes caused by the controller.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 6, 2017
@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jun 6, 2017
@DirectXMan12
Copy link
Contributor Author

cc @kubernetes/sig-autoscaling-pr-reviews @derekwaynecarr @liggitt @dak1n1

@k8s-ci-robot k8s-ci-robot added the sig/autoscaling Categorizes an issue or PR as relevant to SIG Autoscaling. label Jun 6, 2017
@DirectXMan12 DirectXMan12 force-pushed the bug/only-update-when-needed branch from 5c9df1f to 5c46438 Compare June 6, 2017 20:17
@derekwaynecarr
Copy link
Member

To elaborate, the writes cause other side effects (like frequent snapshotting of etcd), which can have other side effects when trying to run and restore HA etcd deployments. Minimizing the write frequency is a priority.

@luxas
Copy link
Member

luxas commented Jun 6, 2017

Does this bugfix target v1.7?

@fgrzadkowski
Copy link
Contributor

@DirectXMan12 Aren't we always updating observedGeneration?

@derekwaynecarr derekwaynecarr added this to the v1.7 milestone Jun 7, 2017
@derekwaynecarr derekwaynecarr added the kind/bug Categorizes issue or PR as related to a bug. label Jun 7, 2017
@derekwaynecarr
Copy link
Member

@luxas -- yes, i updated milestone.

@DirectXMan12 DirectXMan12 force-pushed the bug/only-update-when-needed branch from 5c46438 to 00a19f1 Compare June 7, 2017 14:57
This commit only sends updates if the status has actually changed.
Since the HPA runs at a regular interval, this should reduce the volume
of writes, especially on short HPA intervals with relatively constant
metrics.
@DirectXMan12
Copy link
Contributor Author

@DirectXMan12 Aren't we always updating observedGeneration?

We actually never update observed generation, AFAICT.

@derekwaynecarr
Copy link
Member

/lgtm

with this change, what amount of traffic should i expect per HPA in a steady state cluster environment?

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 7, 2017
@derekwaynecarr
Copy link
Member

@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DirectXMan12, derekwaynecarr

Associated issue: 47077

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 7, 2017
@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 46979, 47078, 47138, 46916)

@k8s-github-robot k8s-github-robot merged commit 6e8d6ac into kubernetes:master Jun 8, 2017
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Jun 8, 2017

@DirectXMan12: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-unit 00a19f1 link @k8s-bot pull-kubernetes-unit test this
pull-kubernetes-kubemark-e2e-gce 00a19f1 link @k8s-bot pull-kubernetes-kubemark-e2e-gce test this

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@DirectXMan12 DirectXMan12 deleted the bug/only-update-when-needed branch June 8, 2017 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. 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. sig/autoscaling Categorizes an issue or PR as relevant to SIG Autoscaling. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants