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

HPA: fixed wrong count for target replicas calculations. #34955

Merged
merged 1 commit into from
Oct 18, 2016
Merged

HPA: fixed wrong count for target replicas calculations. #34955

merged 1 commit into from
Oct 18, 2016

Conversation

jszczepkowski
Copy link
Contributor

@jszczepkowski jszczepkowski commented Oct 17, 2016 •
edited by jessfraz
Loading 

HPA: fixed wrong count for target replicas calculations (#34821).

HPA: fixed wrong count for target replicas calculations (#34821).

This change is Reviewable

@jszczepkowski jszczepkowski added the sig/autoscaling Categorizes an issue or PR as relevant to SIG Autoscaling. label Oct 17, 2016
@jszczepkowski jszczepkowski added this to the v1.4 milestone Oct 17, 2016
@jszczepkowski
Copy link
Contributor Author

CC @DirectXMan12
This is a short fix for #34821 which should be cherry-picked to 1.4. Complex fix, together with cleanups, should be implemented as a part of #33593.

@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Oct 17, 2016
@k8s-ci-robot
Copy link
Contributor

Jenkins unit/integration failed for commit 47451cb. Full PR test history.

The magic incantation to run this job again is @k8s-bot unit test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

DirectXMan12
DirectXMan12 suggested changes Oct 17, 2016
View reviewed changes
Copy link
Contributor

@DirectXMan12 DirectXMan12 left a comment

Choose a reason for hiding this comment

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

One small request on a comment (it should reference a bug number so that future readers can look up the problem), but otherwise LGTM 👍

pkg/controller/podautoscaler/horizontal.go
@@ -329,6 +334,12 @@ func (a *HorizontalController) reconcileAutoscaler(hpa *autoscaling.HorizontalPo
if desiredReplicas > hpa.Spec.MaxReplicas {
desiredReplicas = hpa.Spec.MaxReplicas
}

// Do not upscale too much to prevent incorrect rapid increase of the number of master replicas caused by
// bogus CPU usage report from heapster/kubelet.
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add a bug number here so that future readers of this recognize what we're talking about here (I'm assuming this is to avoid the persistent issues with overflow/astronomically high CPU)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

DirectXMan12
DirectXMan12 suggested changes Oct 17, 2016
View reviewed changes
pkg/controller/podautoscaler/horizontal.go
}

return currentReplicas, &utilization, timestamp, nil
desiredReplicas := math.Ceil(usageRatio * float64(numRunningPods))
Copy link
Contributor

Choose a reason for hiding this comment

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

this could effectively cause a scale down, even if it looks like a scale up (if desiredReplicas * numRunningPods < (numRunningPods + numPendingPods))? Should we deal with that, or at least log or note it somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's fine: not running pods are not consuming CPU. We are logging the number of running pods in DesiredReplicasComputed event bellow, so, it is clear what we are doing.

@jessfraz jessfraz self-assigned this Oct 17, 2016
@jessfraz jessfraz added release-note Denotes a PR that will be considered when it comes time to generate release notes. cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. and removed release-note-label-needed labels Oct 17, 2016
mwielgus
mwielgus reviewed Oct 18, 2016
View reviewed changes
pkg/controller/podautoscaler/horizontal.go
desiredReplicas := math.Ceil(usageRatio * float64(numRunningPods))

a.eventRecorder.Eventf(hpa, api.EventTypeNormal, "DesiredReplicasComputed",
"Computed the desired num of replicas: %d, on a base of %d reports (avgCPUutil: %d)", int32(desiredReplicas), numRunningPods, utilization)
Copy link
Contributor

Choose a reason for hiding this comment

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

Also add the current number of created (possibly not started) replicas.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

pkg/controller/podautoscaler/horizontal.go

// Do not upscale too much to prevent incorrect rapid increase of the number of master replicas caused by
// bogus CPU usage report from heapster/kubelet.
if desiredReplicas > int32(math.Max(2.0*float64(currentReplicas), 4.0)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Make 2.0 ad 4.0 a flag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done (made consts like other HPA parameters)

pkg/controller/podautoscaler/horizontal.go

// Do not upscale too much to prevent incorrect rapid increase of the number of master replicas caused by
// bogus CPU usage report from heapster/kubelet.
if desiredReplicas > int32(math.Max(2.0*float64(currentReplicas), 4.0)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Also put the formula to variable - no need to calculate it 2 times (thats a room for mistakes).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@k8s-github-robot k8s-github-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 Oct 18, 2016
@jszczepkowski
Copy link
Contributor Author

Comments applied, PTAL

mwielgus
mwielgus reviewed Oct 18, 2016
View reviewed changes
pkg/controller/podautoscaler/horizontal.go
@@ -48,8 +48,15 @@ const (

HpaCustomMetricsTargetAnnotationName = "alpha/target.custom-metrics.podautoscaler.kubernetes.io"
HpaCustomMetricsStatusAnnotationName = "alpha/status.custom-metrics.podautoscaler.kubernetes.io"

scaleUpLimitFactor = 2
scaleUpLimitMinimum = 1
Copy link
Contributor

Choose a reason for hiding this comment

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

In the previous version we had 4 here. 1 seems to little. With this setup if 1 replica is present we could only scale it up to 2. We should be able to get to 3-4 immediately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry, a typo :)

@jszczepkowski
HPA: fixed wrong count for target replicas calculations.
f495e73 
HPA: fixed wrong count for target replicas calculations (kubernetes#34821).
@mwielgus
Copy link
Contributor

LGTM

@mwielgus mwielgus added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 18, 2016
@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 16fa327 into kubernetes:master Oct 18, 2016
@jessfraz jessfraz mentioned this pull request Oct 18, 2016
Merged
k8s-github-robot pushed a commit that referenced this pull request Oct 18, 2016
Merge pull request #35046 from jessfraz/automated-cherry-pick-of-#349…
c47ba9e 
…55-origin-release-1.4

Automatic merge from submit-queue

Automated cherry pick of #34955

Cherry pick of #34955 on release-1.4.

#34955: HPA: fixed wrong count for target replicas calculations.
@k8s-cherrypick-bot
Copy link

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

shyamjvs pushed a commit to shyamjvs/kubernetes that referenced this pull request Dec 1, 2016
Merge pull request kubernetes#35046 from jessfraz/automated-cherry-pi…
8d1790e 
…ck-of-#34955-origin-release-1.4

Automatic merge from submit-queue

Automated cherry pick of kubernetes#34955

Cherry pick of kubernetes#34955 on release-1.4.

kubernetes#34955: HPA: fixed wrong count for target replicas calculations.
@mattjmcnaughton mattjmcnaughton mentioned this pull request Sep 9, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mwielgus mwielgus mwielgus left review comments

@DirectXMan12 DirectXMan12 DirectXMan12 requested changes

Assignees

@jessfraz jessfraz

@mwielgus mwielgus

Labels
cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. 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
Milestone
v1.4
Development

Successfully merging this pull request may close these issues.
8 participants
@jszczepkowski @k8s-ci-robot @mwielgus @k8s-github-robot @k8s-cherrypick-bot @DirectXMan12 @jessfraz @googlebot