hpa: Prevent scaling below MinReplicas if desiredReplicas is zero #48997

johanneswuerbach · 2017-07-16T19:43:56Z

What this PR does / why we need it:
Prevent a HPA scaling below minReplicas if desiredReplicas is calculated as 0.

Example events of a HPA continuously scaling between 1 and MinReplicas:

2h        59s        22    horizontal-pod-autoscaler            Normal        SuccessfulRescale    New size: 1; reason: All metrics below target
2h        29s        22    horizontal-pod-autoscaler            Normal        SuccessfulRescale    New size: 15; reason: Current number of replicas below Spec.MinReplicas

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #49028

Special notes for your reviewer:

Release note:

hpa: Prevent scaling below MinReplicas if desiredReplicas is zero

k8s-ci-robot · 2017-07-16T19:44:03Z

Hi @johanneswuerbach. 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.

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

johanneswuerbach · 2017-07-16T19:51:45Z

pkg/controller/podautoscaler/horizontal.go

+				desiredReplicas = *hpa.Spec.MinReplicas
+			} else {
+				desiredReplicas = 1
+			}


Not sure whether fallthrough would be more in-line with the coding style, happy to change if preferred.

How about as this:

case desiredReplicas == 0: if hpa.Spec.MinReplicas != nil { // never scale down to 0, reserved for disabling autoscaling setCondition(hpa, autoscalingv2.ScalingLimited, v1.ConditionTrue, "TooFewReplicas", "the desired replica count was zero and less than the minimum replica count") desiredReplicas = *hpa.Spec.MinReplicas } else { // never scale down to 0, reserved for disabling autoscaling setCondition(hpa, autoscalingv2.ScalingLimited, v1.ConditionTrue, "TooFewReplicas", "the desired replica count was zero") desiredReplicas = 1 }

Instead of duplicating the condition I now aligned the order of cases to

kubernetes/pkg/controller/podautoscaler/horizontal.go

Lines 413 to 418 in e8eb858

} else if hpa.Spec.MinReplicas != nil && currentReplicas < *hpa.Spec.MinReplicas {

rescaleReason = "Current number of replicas below Spec.MinReplicas"

desiredReplicas = *hpa.Spec.MinReplicas

} else if currentReplicas == 0 {

rescaleReason = "Current number of replicas must be greater than 0"

desiredReplicas = 1

, which also prevents the broken behaviour. WDYT?

+1 for this, thanks @johanneswuerbach

gyliu513 · 2017-07-16T21:46:38Z

pkg/controller/podautoscaler/horizontal.go

-			desiredReplicas = 1
+			// make sure we aren't below our minimum if one is defined
+			if hpa.Spec.MinReplicas != nil {
+				desiredReplicas = *hpa.Spec.MinReplicas


How about logging a warning message here instead of setting the desiredReplicas silently?

Sorry, I'm not sure I understand. In case of near zero load (which causes the desired replicas to be calculated as 0), the hpa should still try to downscale to a possible minimum.

This PR changes this behaviour only to scale to the defined minimum instead of hardcoded 1, which causes all except one running pod to be killed and then HPA scaling to the defined min during the next iteration with Current number of replicas below Spec.MinReplicas, which causes some unnecessary pod churn and increased load on the cluster.

Sorry, I mean both adding a warning message and setting desiredReplicas as your current PR here, make sense?

Not sure, isn't this desired behaviour that low-usage defaults to scaling to min replicas?

If I read https://kubernetes.io/docs/api-reference/v1.7/#horizontalpodautoscalerspec-v1-autoscaling correctly, minReplicas will default to 1 and can't be unset. Maybe the desiredReplicas == 0 case could be completely dropped and just be ensured that minReplicas is set. In that case the case hpa.Spec.MinReplicas != nil && desiredReplicas < *hpa.Spec.MinReplicas: should catch also the 0 case and handle it correctly.

MaciekPytel · 2017-07-17T09:54:04Z

/ok-to-test

johanneswuerbach · 2017-07-17T11:55:31Z

Semi-unrelated question: Is this something that would be back-ported to 1.7 if approved or is there anything I would need to do for that?

MaciekPytel · 2017-07-17T12:18:10Z

@johanneswuerbach I am strongly convinced we need to cherry-pick it on 1.7 and make sure it gets included in the next patch release, as it is a serious regression in hpa. Let's wait for @DirectXMan12 (I think it's still pretty early in his timezone) to review this and than we can try to get it cherrypicked.

DirectXMan12 · 2017-07-17T14:47:36Z

/lgtm

this is supposed to have an associated issue. Please create one, add it to the PR body.

am strongly convinced we need to cherry-pick it on 1.7 and make sure it gets included in the next patch release, as it is a serious regression in hpa

Agreed, please do cherry-pick this to any appropriate versions.

johanneswuerbach · 2017-07-17T15:13:59Z

Created and referenced #49028

piosz · 2017-07-17T15:37:13Z

/approve no-issue

mikedanese · 2017-07-17T16:48:36Z

/lgtm

mikedanese · 2017-07-17T16:48:43Z

/approve no-issue

k8s-github-robot · 2017-07-17T16:48:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DirectXMan12, johanneswuerbach, mikedanese, piosz

Associated issue: 49028

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

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

~~pkg/controller/OWNERS~~ [mikedanese]

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 · 2017-07-17T20:23:46Z

Automatic merge from submit-queue (batch tested with PRs 48997, 48595, 48898, 48711, 48972)

…48997-upstream-release-1.7 Automatic merge from submit-queue Automated cherry pick of #48997 Cherry pick of #48997 on release-1.7. #48997: hpa: Prevent scaling below MinReplicas if desiredReplicas is

k8s-cherrypick-bot · 2017-07-18T07:17:04Z

Commit found in the "release-1.7" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 16, 2017

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 16, 2017

k8s-github-robot assigned jszczepkowski and piosz Jul 16, 2017

k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jul 16, 2017

johanneswuerbach commented Jul 16, 2017

View reviewed changes

gyliu513 reviewed Jul 16, 2017

View reviewed changes

piosz assigned DirectXMan12 and unassigned piosz Jul 17, 2017

hpa: Prevent scaling below MinReplicas if desiredReplicas is zero

a99d988

johanneswuerbach force-pushed the hpa-min branch from 66b347a to a99d988 Compare July 17, 2017 07:33

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 17, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 17, 2017

k8s-ci-robot assigned mikedanese Jul 17, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 17, 2017

MaciekPytel added this to the v1.7 milestone Jul 17, 2017

MaciekPytel added the cherrypick-candidate label Jul 17, 2017

k8s-github-robot merged commit aed912b into kubernetes:master Jul 17, 2017

MaciekPytel mentioned this pull request Jul 17, 2017

Automated cherry pick of #48997 #49047

Merged

wojtek-t added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Jul 18, 2017

k8s-cherrypick-bot removed the cherrypick-candidate label Jul 18, 2017

johanneswuerbach deleted the hpa-min branch July 18, 2017 07:43

MaciekPytel mentioned this pull request Aug 22, 2017

autoscaler scales down below minReplicas when "All metrics below target" #51111

Closed

DirectXMan12 mentioned this pull request Oct 3, 2017

UPSTREAM: 52168: Fix incorrect status msg in podautoscaler openshift/origin#16664

Merged

This was referenced Oct 11, 2017

Fix hpa scaling above max replicas w/ scaleUpLimit #53690

Merged

Clean-up reconcileAutoscaler and improve it's unittest coverage #53728

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

hpa: Prevent scaling below MinReplicas if desiredReplicas is zero #48997

hpa: Prevent scaling below MinReplicas if desiredReplicas is zero #48997

johanneswuerbach commented Jul 16, 2017 •

edited

Loading

k8s-ci-robot commented Jul 16, 2017

johanneswuerbach Jul 16, 2017

gyliu513 Jul 16, 2017

johanneswuerbach Jul 17, 2017

gyliu513 Jul 17, 2017

gyliu513 Jul 16, 2017

johanneswuerbach Jul 16, 2017 •

edited

Loading

gyliu513 Jul 16, 2017

johanneswuerbach Jul 16, 2017 •

edited

Loading

MaciekPytel commented Jul 17, 2017

johanneswuerbach commented Jul 17, 2017

MaciekPytel commented Jul 17, 2017

DirectXMan12 commented Jul 17, 2017

johanneswuerbach commented Jul 17, 2017

piosz commented Jul 17, 2017

mikedanese commented Jul 17, 2017

mikedanese commented Jul 17, 2017

k8s-github-robot commented Jul 17, 2017

k8s-github-robot commented Jul 17, 2017

k8s-cherrypick-bot commented Jul 18, 2017

	} else if hpa.Spec.MinReplicas != nil && currentReplicas < *hpa.Spec.MinReplicas {
	rescaleReason = "Current number of replicas below Spec.MinReplicas"
	desiredReplicas = *hpa.Spec.MinReplicas
	} else if currentReplicas == 0 {
	rescaleReason = "Current number of replicas must be greater than 0"
	desiredReplicas = 1

hpa: Prevent scaling below MinReplicas if desiredReplicas is zero #48997

hpa: Prevent scaling below MinReplicas if desiredReplicas is zero #48997

Conversation

johanneswuerbach commented Jul 16, 2017 • edited Loading

k8s-ci-robot commented Jul 16, 2017

johanneswuerbach Jul 16, 2017

Choose a reason for hiding this comment

gyliu513 Jul 16, 2017

Choose a reason for hiding this comment

johanneswuerbach Jul 17, 2017

Choose a reason for hiding this comment

gyliu513 Jul 17, 2017

Choose a reason for hiding this comment

gyliu513 Jul 16, 2017

Choose a reason for hiding this comment

johanneswuerbach Jul 16, 2017 • edited Loading

Choose a reason for hiding this comment

gyliu513 Jul 16, 2017

Choose a reason for hiding this comment

johanneswuerbach Jul 16, 2017 • edited Loading

Choose a reason for hiding this comment

MaciekPytel commented Jul 17, 2017

johanneswuerbach commented Jul 17, 2017

MaciekPytel commented Jul 17, 2017

DirectXMan12 commented Jul 17, 2017

johanneswuerbach commented Jul 17, 2017

piosz commented Jul 17, 2017

mikedanese commented Jul 17, 2017

mikedanese commented Jul 17, 2017

k8s-github-robot commented Jul 17, 2017

k8s-github-robot commented Jul 17, 2017

k8s-cherrypick-bot commented Jul 18, 2017

johanneswuerbach commented Jul 16, 2017 •

edited

Loading

johanneswuerbach Jul 16, 2017 •

edited

Loading

johanneswuerbach Jul 16, 2017 •

edited

Loading