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

[WIP] Configurable HorizontalPodAutoscaler #74525

Open
wants to merge 6 commits into
base: master
from

Conversation

@gliush
Copy link

commented Feb 25, 2019

For now, only API changes to be reviewed, the functionality to be added later.
KEP PR: kubernetes/enhancements#883

What type of PR is this?
/kind feature

What this PR does / why we need it:
Different applications may have different business values, different logic and may require different scaling behaviours.
At the moment, there’s only one cluster-level configuration parameter that influence how fast the cluster is scaled down:
--horizontal-pod-autoscaler-downscale-stabilization-window (default to 5 min)

And a couple of hard-coded constants that specify how fast the cluster can scale up:

This PR adds algorithm-agnostic HPA object configuration parameters to specify scale velocity in number of pods or percentages.

You can read more detailed motivation in the RFC

Which issue(s) this PR fixes:

Fixes #39090
Fixes #65097
Fixes #69428

It covers partly #56335

Special notes for your reviewer:
As I add only optional parameter, it seems we don't need to change API version.

Does this PR introduce a user-facing change?:

TBA
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2019

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


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.

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2019

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.

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.

@gliush

This comment has been minimized.

Copy link
Author

commented Feb 25, 2019

/sig autoscaling

@gliush

This comment has been minimized.

Copy link
Author

commented Feb 25, 2019

@thockin: Could you have a look? @mwielgus says that you're the most familiar with the matter.
I can't do it by myself, sorry.

@thockin

This comment has been minimized.

Copy link
Member

commented Mar 1, 2019

Is there a KEP for this? Generally we want to see the design work independent of the code.

I am happy to take up the API review, but can't really do that until I know the domain experts (e.g. @mwielgus) are happy with the design.

@thockin

This comment has been minimized.

Copy link
Member

commented Mar 1, 2019

I see there's a doc but that's not a KEP

@@ -69,6 +69,9 @@ type HorizontalPodAutoscalerSpec struct {
// If not set, the default metric will be set to 80% average CPU utilization.
// +optional
Metrics []MetricSpec `json:"metrics,omitempty" protobuf:"bytes,4,rep,name=metrics"`
// constraints contain HPA object configuration parameters
// that configure each particular HPA scaling behaviour.
Constraints HorizontalPodAutoscalerScaleConstraints `json:"constraints,omitempty" protobuf:"bytes,5,opt,name=constraints"`

This comment has been minimized.

Copy link
@thockin

thockin Mar 1, 2019

Member

I am not sure that 'constraints' is the best word here. It's OK, but I want to ask if we considered an y others

This comment has been minimized.

Copy link
@mwielgus

mwielgus Mar 1, 2019

Contributor

That is the best we have :-|, but we will happily consider an alternative.

Show resolved Hide resolved staging/src/k8s.io/api/autoscaling/v2beta2/types.go Outdated
@thockin

This comment has been minimized.

Copy link
Member

commented Mar 1, 2019

@mwielgus

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2019

@thockin - I have worked with @gliush on this for a while. We agreed on the rough idea and the configuration parameters, so we are proceeding with the api.

This functionality has been requested multiple times already so I'm glad that we finally have someone who is working on it :).

@mwielgus

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2019

@thockin Then we will double the field count or create a shared denominator. Not sure what is prettier.

@mwielgus

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2019

@gliush Please also submit the docs as kep for this feature, as agreed.

@hongshibao

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

When is this feature planned to release?
In our internal usage of K8S, we also customize the kube-controller-manager to support similar logic with scaleDownPercent and scaleDownPods. But we put them as flags of kube-controller-manager, which means they are global settings for all HPAs.

@gliush

This comment has been minimized.

Copy link
Author

commented Mar 7, 2019

Here's a KEP (first version, need to be reviewed):
kubernetes/enhancements#883

@gliush

This comment has been minimized.

Copy link
Author

commented Mar 7, 2019

When is this feature planned to release?
In our internal usage of K8S, we also customize the kube-controller-manager to support similar logic with scaleDownPercent and scaleDownPods. But we put them as flags of kube-controller-manager, which means they are global settings for all HPAs.

Well, we can combine these approaches, as in my KEP I have a "default parameters" if the User doesn't configure them explicitly.

@hongshibao

This comment has been minimized.

Copy link
Contributor

commented Mar 8, 2019

When is this feature planned to release?
In our internal usage of K8S, we also customize the kube-controller-manager to support similar logic with scaleDownPercent and scaleDownPods. But we put them as flags of kube-controller-manager, which means they are global settings for all HPAs.

Well, we can combine these approaches, as in my KEP I have a "default parameters" if the User doesn't configure them explicitly.

Yes. Your approach is a more general solution. Looking forward to using this new feature : )
Btw, any release plan for this feature?

@thockin

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

I'm not concerned with field count. and while a VERY MUCH appreciate contributors, we must be cautious with API.

I'll go read the kep, but we should come up with a convention for rational values. I think I'd rather see something like:

{
    ScaleUp HPAConstraintValue
    ScaleDown HPAConstraintValue
}

type HPAConstraintValue struct {
  // One of the following must be set, but for APi compat reasons it's possible that none may be set, which is interpreted as <somethign, you tell me...>
  Pods int32
  Percent int32

  PeriodSeconds int32
}

so you can say:

constraints:
  - scaleUp:
      pods: 3
      periodSeconds: 1
  - scaleDown:
      pods: 1
      periodSeconds: 3

@k8s-ci-robot k8s-ci-robot added size/M and removed size/S labels Apr 1, 2019

@gliush

This comment has been minimized.

Copy link
Author

commented Apr 3, 2019

@thockin: I like your idea; your configuration is more clear.
I fixed it, could you check?

@liggitt liggitt added this to Assigned in API Reviews Apr 17, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Apr 18, 2019

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: gliush
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: thockin

If they are not already assigned, you can assign the PR to them by writing /assign @thockin in a comment when ready.

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

@gliush

This comment has been minimized.

Copy link
Author

commented Apr 19, 2019

Just an update.
According to the discussion in the KEP kubernetes/enhancements#883 I've updated the API.
I've added DelaySeconds parameter to the constraints. It makes the StabilizationWindow concept useless. And the command line parameter --horizontal-pod-autoscaler-downscale-stabilization-window from now on will specify constraints.scaleDown.delaySeconds default value.
Check the last commit 8d5a09b and the discussion in the KEP PR.

@mathewmoon

This comment has been minimized.

Copy link

commented Apr 26, 2019

@gliush
Good work man. This is desperately IMO. Sorry if I should be able to infer this from what is already posted, but will this be a minor version update or will it not be merged into a release until later? I am needing this functionality as soon as possible.

@gliush

This comment has been minimized.

Copy link
Author

commented Apr 27, 2019

@mathewmoon: sorry, I'm not familiar with how versioning works in k8s.
Most likely, it will be merged to the minor version release after it is fully implemented and tested.

@gliush

This comment has been minimized.

Copy link
Author

commented Apr 29, 2019

@thockin: Hi, could you approve my API change so that I could start working on the implementation?
I've received an approve for my KEP PR: kubernetes/enhancements#883

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.