Configurable scale velocity for HPA by gliush · Pull Request #883 · kubernetes/enhancements

gliush · 2019-03-07T17:20:18Z

No description provided.

k8s-ci-robot · 2019-03-07T17:20:28Z

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

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

jdumars · 2019-03-07T18:30:31Z

/ok-to-test

keps/sig-autoscaling/20190307-configurable-scale-velocity-for-hpa.md

mwielgus · 2019-03-11T22:02:04Z

@thockin Could you review the api change here?

keps/sig-autoscaling/20190307-configurable-scale-velocity-for-hpa.md

* Adds "delaySeconds" field into the HPA constraints * Makes sure that it could be configured by the "stabilization window" command line argument * Adds user story * Fixes typos

keps/sig-autoscaling/20190307-configurable-scale-velocity-for-hpa.md

Fixes typos Use another default values Some additional explanations added

keps/sig-autoscaling/20190307-configurable-scale-velocity-for-hpa.md

Also adds new section for the delay option

josephburnett · 2019-04-29T12:07:47Z

/lgtm
@thockin @mwielgus I think this is good-to-go. Could you take a look?

k8s-ci-robot · 2019-04-29T12:07:55Z

@josephburnett: changing LGTM is restricted to assignees, and only kubernetes/enhancements repo collaborators may be assigned issues.

Details

In response to this:

/lgtm
@thockin @mwielgus I think this is good-to-go. Could you take a look?

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.

keps/sig-autoscaling/20190307-configurable-scale-velocity-for-hpa.md

gliush · 2019-04-29T19:01:07Z

@josephburnett:
We have a KEP freeze tomorrow, Apr 30 for k8s-1.15 (#853 (comment))
Is it possible to have this KEP approved so that I can merge it and start working on the implementation to make it part of 1.15?

josephburnett · 2019-04-30T10:52:51Z

@gliush it's not up to me whether it's approved or not. I've already pinged @thockin and @mwielgus that it's ready for review. 🤞

mwielgus · 2019-05-01T00:01:56Z

/lgtm

k8s-ci-robot · 2019-05-01T00:02:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gliush, josephburnett, mwielgus

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~keps/sig-autoscaling/OWNERS~~ [mwielgus]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

liggitt · 2019-05-01T00:29:08Z

a couple notes for clarity:

this merged still in provisional state, and the expectation is that things will be in implementable by feature freeze
I didn't see an ack from @thockin on the updates after his review (that doesn't necessarily have to happen by feature freeze, but an API review ack is still needed before an implementation merges)

thockin · 2019-05-01T18:29:00Z

A KEP should not be a full API review, so it seems appropriate that this merge without that final approval. It would be bad if the ultimate API was radically different than what is in the KEP, but the reality is that APIs almost always evolve as they are implemented :)

Re-reading now.

thockin

Mostly API comments to sort through as you finalize the UX. This is a complicated API, so I'll encourage you to seek out simplifications, even at the cost of some flexibility. We can always add stuff, but removing is very hard.

thockin · 2019-05-01T18:34:15Z

keps/sig-autoscaling/20190307-configurable-scale-velocity-for-hpa.md

+
+- `constraints`:
+  - `scaleUp`:
+    - `percent = 900`    (i.e., to increase the number of pods 10 times per minute is ok).


For final API we may want to try a few variants to see what UX works best and is least confusing. E.g. if I want to allow 10x growth, does it make sense to say "grow by 900%" or "grow to 1000%" ?

For examples like "grow by 100%" it seems pretty obvious, just less so at larger numbers :)

I would prefer "grow to 1000%". It makes the math more obvious: maxAllowed = current * (percent / 100)

thockin · 2019-05-01T18:40:17Z

keps/sig-autoscaling/20190307-configurable-scale-velocity-for-hpa.md

+type HPAScaleConstraintRateValue struct {
+    Pods          *int32
+    Percent       *int32
+    PeriodSeconds *int32


Is this smoothed over the period or done at edges? E.g. 3 pods per minute could be 1 pod per 20 seconds or 3 pods all at once after 60 seconds. Just specify the behavior.

Is it ok for pods and percent to share a period? e.g. do I need to be able to specify "3 pods per minute or 100% per 20 seconds" ?

thockin · 2019-05-01T18:41:29Z

keps/sig-autoscaling/20190307-configurable-scale-velocity-for-hpa.md

+Create an HPA with the following constraints:
+
+- `constraints`:
+  - `scaleDown`:


This is a little clunky -- if there are ever new modes here (I don't have even a hypothetical example, but pretend :), then any YAML that carries this payload will not have that new field.

There are other API conventions being strengthened around one-of sets, which want a discriminator field, so maybe this looks like:

constraints: scaleDown: # 3 pods per 10 minutes policy: Pods value: 3 periodSeconds: 600

constraints: scaleDown: # no scale-down allowed policy: Disabled

Or if pods and percent are not mutually exclusive (?) then something like:

constraints: scaleDown: {} # no scale-down allowed

This changes the defaults from being on the leaf fields to being on the struct. If scaleDown is not specified, the default value is { percent: 100, pods: 4 }, but if it is specified, the fields inside default to 0.

Something like that.

@gliush this is worth considering for kubernetes/kubernetes#74525. Just wanted to make sure you didn't miss this feedback.

thockin · 2019-05-01T18:43:28Z

keps/sig-autoscaling/20190307-configurable-scale-velocity-for-hpa.md

+- `constraints`:
+  - `scaleDown`:
+    - `pods = 5`
+  - `delaySeconds = 600`


is delay specific to down-scaling or also for up? The YAML listed here is confusing

thockin · 2019-05-01T18:52:16Z

keps/sig-autoscaling/20190307-configurable-scale-velocity-for-hpa.md

+
+```golang
+type HPAScaleConstraintValue struct {
+    Rate         *HPAScaleConstraintRateValue


what does it mean if this is not specified? Make sure to document and think through all optionality

thockin · 2019-05-01T18:53:31Z

keps/sig-autoscaling/20190307-configurable-scale-velocity-for-hpa.md

+    MinReplicas    *int32
+    MaxReplicas    int32
+    Metrics        []MetricSpec
+    Constraints    *HPAScaleConstraints


FWIW I still dislike "constraints" as a term here. "policy" or somethng makes more sense to me, but I probably won't fight very hard on this

thockin · 2019-05-01T18:55:03Z

keps/sig-autoscaling/20190307-configurable-scale-velocity-for-hpa.md

+```golang
+type HPAScaleConstraintValue struct {
+    Rate         *HPAScaleConstraintRateValue
+    DelaySeconds *int32


if I understand, this is not a "delay" as much as a "window" ? Please think about the name that best captures the semantics.

Also document bounds. Can I set a delay of 14 days? How much buffer are we willing to allocate (and lose if the controller crashes) ?

thockin · 2019-05-01T18:56:48Z

keps/sig-autoscaling/20190307-configurable-scale-velocity-for-hpa.md

+
+Example for `CurReplicas = 10` and HPA controller cycle once per a minute:
+
+- First 9 minutes the algorithm will do nothing except gathering recommendations.


where are these stored? If the controller goes down, is it all lost? That should feedback into our API limits so we don't promise to store too much, and then cause a huge problem when the controller dies for whatever reason.

new kep: configurable scale velocity for hpa

3312764

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 7, 2019

k8s-ci-robot requested review from jdumars and mwielgus March 7, 2019 17:20

This was referenced Mar 7, 2019

Configurable scale velocity for HPA #853

Closed

Configurable HorizontalPodAutoscaler kubernetes/kubernetes#74525

Merged

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 7, 2019

mwielgus suggested changes Mar 11, 2019

View reviewed changes

keps/sig-autoscaling/20190307-configurable-scale-velocity-for-hpa.md Outdated Show resolved Hide resolved

thockin reviewed Mar 11, 2019

View reviewed changes

keps/sig-autoscaling/20190307-configurable-scale-velocity-for-hpa.md Show resolved Hide resolved

keps/sig-autoscaling/20190307-configurable-scale-velocity-for-hpa.md Outdated Show resolved Hide resolved

Ivan Glushkov added 2 commits April 1, 2019 18:22

Fix typos, style, rephrase explanations

c3d70a3

Rework API to separate constraints for Up and Down directions

d6a61af

gliush mentioned this pull request Apr 2, 2019

Having programable, the waiting time before the "autoscale-up/down" is effective kubernetes/kubernetes#56335

Closed

josephburnett reviewed Apr 15, 2019

View reviewed changes

keps/sig-autoscaling/20190307-configurable-scale-velocity-for-hpa.md Outdated Show resolved Hide resolved

josephburnett reviewed Apr 16, 2019

View reviewed changes

keps/sig-autoscaling/20190307-configurable-scale-velocity-for-hpa.md Outdated Show resolved Hide resolved

josephburnett reviewed Apr 17, 2019

View reviewed changes

keps/sig-autoscaling/20190307-configurable-scale-velocity-for-hpa.md Show resolved Hide resolved

Use pointer instead of negative values

8fe45e5

liggitt assigned thockin Apr 17, 2019

Adds Hysteresis to the HPA constraints

fe36d8a

* Adds "delaySeconds" field into the HPA constraints * Makes sure that it could be configured by the "stabilization window" command line argument * Adds user story * Fixes typos

josephburnett reviewed Apr 23, 2019

View reviewed changes

keps/sig-autoscaling/20190307-configurable-scale-velocity-for-hpa.md Outdated Show resolved Hide resolved

josephburnett reviewed Apr 23, 2019

View reviewed changes

keps/sig-autoscaling/20190307-configurable-scale-velocity-for-hpa.md Outdated Show resolved Hide resolved

Fixes golang code alignment

0c6f599

josephburnett reviewed Apr 23, 2019

View reviewed changes

keps/sig-autoscaling/20190307-configurable-scale-velocity-for-hpa.md Outdated Show resolved Hide resolved

josephburnett reviewed Apr 29, 2019

View reviewed changes