[Federation] Make the hpa scale time window configurable by irfanurrehman · Pull Request #49583 · kubernetes/kubernetes

irfanurrehman · 2017-07-25T19:20:13Z

This PR is on top of open pr #45993.
Please review only the last commit in this PR.
This adds a config param to controller manager, the value of which gets passed to hpa adapter via sync controller.
This is needed to reduce the overall time limit of the hpa scaling window to much lesser (then the default 2 mins) to get e2e tests run faster. Please see the comment on the newly added parameter.

Special notes for your reviewer:
@kubernetes/sig-federation-pr-reviews
@quinton-hoole
@marun to please validate the mechanism used to pass a parameter from cmd line to adapter.

Release note:

federation-controller-manager gets a new flag --hpa-scale-forbidden-window.
This flag is used to configure the duration used by federation hpa controller to determine if it can move max and/or min replicas 
around (or not), of a cluster local hpa object, by comparing current time with the last scaled time of that cluster local hpa. 
Lower value will result in faster response to scalibility conditions achieved by cluster local hpas on local replicas, but too low 
a value can result in thrashing. Higher values will result in slower response to scalibility conditions on local replicas.

marun · 2017-08-01T17:40:14Z

If an argument is going to be added to the AdaptrerFactory signature, would it make more sense to make it a map of interfaces to allow for future extensibility?

irfanurrehman · 2017-08-01T18:33:16Z

If an argument is going to be added to the AdaptrerFactory signature, would it make more sense to make it a map of interfaces to allow for future extensibility?

I do not clearly understand the need of doing so.
If you mean to point at being able to pass multiple params to a particular adapter, if the need be. Wouldn't it be possible to pass a map (or an array) of params to an interface{} (rather then a map of interfaces), which the adapter then casts back to the needed map or array. Am I missing something?

marun · 2017-08-01T19:36:31Z

My suggestion is to avoid having to special-case instantiation in the controller and instead provide a map of all parameters (of type interface) to all sync controllers.

irfanurrehman · 2017-08-02T10:43:52Z

My suggestion is to avoid having to special-case instantiation in the controller and instead provide a map of all parameters (of type interface) to all sync controllers.

Got it, thanks for the suggestion. Updated!

marun · 2017-08-03T16:07:29Z

federation/pkg/federatedtypes/hpa.go

s/Forbiddne/Forbidden/

Also, why is it desirable to drop the comment?

I guess it's been moved to options.go above. Makes sense.

Actually, I did add quite an elaborate comment on the new argument added to the federation controller manager, thus dropped the one from here; but I see that it makes sense to keep the comment here also. Updated.

marun · 2017-08-03T16:07:33Z

federation/pkg/federatedtypes/hpa_test.go

Consider basing scaledTime on ScaleForbiddenWindow so that any change in that value does not require a change in this test.

ghost · 2017-08-03T22:49:06Z

Looks OK to me. @marun let me know when you're happy and I can approve.

ghost · 2017-08-07T23:37:47Z

/lgtm
/approve no-issue

irfanurrehman · 2017-08-09T09:56:45Z

@madhusudancs @shashidharatd, can one of you please approve this for the flag name updated in "hack".

shashidharatd · 2017-08-09T10:43:28Z

/lgtm

k8s-github-robot · 2017-08-09T10:44:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: irfanurrehman, quinton-hoole, shashidharatd

Associated issue requirement bypassed by: quinton-hoole

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

Details

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

~~federation/OWNERS~~ [quinton-hoole]
~~hack/verify-flags/OWNERS~~ [shashidharatd]
~~test/e2e_federation/OWNERS~~ [quinton-hoole,shashidharatd]
~~test/integration/federation/OWNERS~~ [quinton-hoole]

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

fejta-bot · 2017-08-09T18:22:30Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

k8s-github-robot · 2017-08-09T21:14:26Z

Automatic merge from submit-queue (batch tested with PRs 50016, 49583, 49930, 46254, 50337)

k8s-ci-robot added sig/federation cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 25, 2017

k8s-github-robot assigned madhusudancs and colhom Jul 25, 2017

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

irfanurrehman force-pushed the fed-hpa-configTimeout branch 6 times, most recently from 0e48108 to 7a3e8af Compare July 31, 2017 08:48

ghost assigned ghost and unassigned madhusudancs and colhom Jul 31, 2017

ghost self-requested a review July 31, 2017 16:20

irfanurrehman force-pushed the fed-hpa-configTimeout branch from 7a3e8af to 93d094b Compare August 2, 2017 10:43

marun reviewed Aug 3, 2017

View reviewed changes

[Federation] hpa controller

0bea0ca

irfanurrehman force-pushed the fed-hpa-configTimeout branch from 93d094b to 9e4d3fe Compare August 5, 2017 19:19

irfanurrehman mentioned this pull request Aug 5, 2017

[Federation] HPA controller #45993

Merged

[Federation] Make the hpa scale time window configurable

2be69a5

irfanurrehman force-pushed the fed-hpa-configTimeout branch from 9e4d3fe to 2be69a5 Compare August 5, 2017 19:39

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

k8s-ci-robot assigned shashidharatd Aug 9, 2017

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

k8s-github-robot merged commit 82b3a80 into kubernetes:master Aug 9, 2017

Conversation

irfanurrehman commented Jul 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marun commented Aug 1, 2017

Uh oh!

irfanurrehman commented Aug 1, 2017

Uh oh!

marun commented Aug 1, 2017

Uh oh!

irfanurrehman commented Aug 2, 2017

Uh oh!

marun Aug 3, 2017

Choose a reason for hiding this comment

Uh oh!

ghost Aug 3, 2017

Choose a reason for hiding this comment

Uh oh!

irfanurrehman Aug 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marun Aug 3, 2017

Choose a reason for hiding this comment

Uh oh!

ghost Aug 3, 2017

Choose a reason for hiding this comment

Uh oh!

irfanurrehman Aug 5, 2017

Choose a reason for hiding this comment

Uh oh!

ghost commented Aug 3, 2017

Uh oh!

ghost commented Aug 7, 2017

Uh oh!

irfanurrehman commented Aug 9, 2017

Uh oh!

shashidharatd commented Aug 9, 2017

Uh oh!

k8s-github-robot commented Aug 9, 2017

Uh oh!

fejta-bot commented Aug 9, 2017

Uh oh!

k8s-github-robot commented Aug 9, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

irfanurrehman commented Jul 25, 2017 •

edited

Loading

irfanurrehman Aug 5, 2017 •

edited

Loading