Feature/hpa upscale downscale delay configurable by Dmitry1987 · Pull Request #42101 · kubernetes/kubernetes

Dmitry1987 · 2017-02-25T13:58:44Z

What this PR does / why we need it:
Makes "upscale forbidden window" and "downscale forbidden window" duration configurable in arguments of kube-controller-manager. Those are options of horizontal pod autoscaler.

Special notes for your reviewer:
Please have a look @DirectXMan12 , the PR as discussed in Slack.

Release note:

Make "upscale forbidden window" and "downscale forbidden window"  duration configurable in arguments of kube-controller-manager. Those are options of horizontal pod autoscaler. Right now are hardcoded 3 minutes for upscale, and 5 minutes to downscale.  But sometimes cluster administrator might want to change this for his own needs.

k8s-ci-robot · 2017-02-25T13:58:44Z

Hi @Dmitry1987. 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 @k8s-bot 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.

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

k8s-ci-robot · 2017-02-25T13:58:46Z

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://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

Once you've signed, please reply here (e.g. "I signed it!") and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.

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

k8s-ci-robot · 2017-02-25T13:58:46Z

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://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

Once you've signed, please reply here (e.g. "I signed it!") and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.

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

k8s-reviewable · 2017-02-25T13:58:51Z

This change is

dannyz29 · 2017-02-27T14:58:17Z

1

Dmitry1987 · 2017-02-27T15:03:34Z

Hi @DirectXMan12 , one of the commits was made from ubuntu VM where I was testing. And other commits from windows. So, the commit from ubuntu has an "X", it thinks no CLA signed (the username of commit "Dmitry Shmakov" different from account username "Dmitry1987"...), is it ok and will be possible to merge, or I should fix this somehow?

DirectXMan12 · 2017-02-27T15:43:51Z

is it ok and will be possible to merge, or I should fix this somehow?

You'll need to fix it. You can amend the commit while using the --author flag to do so.

DirectXMan12

A couple of minor changes, otherwise looks good in general. Thanks for doing this! :-)

DirectXMan12 · 2017-02-27T15:44:44Z

pkg/apis/componentconfig/types.go

 	// horizontalPodAutoscalerSyncPeriod is the period for syncing the number of
 	// pods in horizontal pod autoscaler.
 	HorizontalPodAutoscalerSyncPeriod metav1.Duration
+        // horizontalPodAutoscalerUpscaleForbiddenWindow is a period after which next upscale allowed.


you appear to have some indentation issues here

DirectXMan12 · 2017-02-27T15:45:24Z

pkg/controller/podautoscaler/horizontal.go

 		}

-		rescale = shouldScale(hpa, currentReplicas, desiredReplicas, timestamp)
+		rescale = shouldScale(a, hpa, currentReplicas, desiredReplicas, timestamp)


just make this a method of the HPA controller instead of passing the HPA as the first argument.

DirectXMan12 · 2017-02-27T15:47:12Z

cc @jszczepkowski

timothysc · 2017-02-27T16:02:09Z

Punting to @piosz and @DirectXMan12. It doesn't appear that sig-instrumentation has their kubernetes team configured properly.

snirad · 2017-02-27T16:12:18Z

Very nice !

Dmitry1987 · 2017-03-08T16:29:32Z

rebased and added needed params to "legacy_horizontal_test.go" (this file didn't yet exist in my branch), so test shouldn't fail now, i hope =) .

jszczepkowski · 2017-03-09T07:18:22Z

@k8s-bot test this

k8s-ci-robot · 2017-03-09T08:01:42Z

@Dmitry1987: The following test(s) failed:

Test name	Commit	Details	Rerun command
Jenkins GCI GKE smoke e2e	84243f75fca1797283e8d9d66f616d056bcd0048	link	`@k8s-bot gci gke e2e test this`
Jenkins GKE smoke e2e	84243f75fca1797283e8d9d66f616d056bcd0048	link	`@k8s-bot cvm gke e2e test this`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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

Dmitry1987 · 2017-03-09T12:33:24Z

Hi guys, from latest failed tests I found and fixed all "gofmt" needed files with "gofmt -s -w". But why "kops" test failed and where to fix what, I don't understand :( , can someone take a quick look at the log here: https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/42101/pull-kubernetes-e2e-kops-aws/16450/?log#log
there are errors like build/temp.linux-x86_64-2.7/check_libyaml.c:2:18: fatal error: yaml.h: No such file or directory which I have no idea how to fix or if they're related to my changes at all (or did those fail as chain reaction following 'bazel build'?).

The 3d test "/hack/verify-flags-underscore.py" failure I fixed too, added flags like it says to "hack/verify-flags/known-flags.txt"... so I just need help with kops test. Thanks :)

Dmitry1987 · 2017-03-16T13:50:08Z

Hi @jszczepkowski , can you help with the bot? it doesn't run test after my last push... and I don't know what is the failure in 'kops' test, maybe you can help?

jszczepkowski · 2017-03-16T14:52:07Z

@k8s-bot test this

DirectXMan12 · 2017-03-24T18:31:21Z

as long as the tests pass, looks good! 👍

DirectXMan12 · 2017-03-24T19:18:45Z

@k8s-bot ok to test

DirectXMan12 · 2017-03-26T15:43:23Z

/lgtm /approve

Dmitry1987 · 2017-04-03T07:28:25Z

Hi @deads2k, @mikedanese , do you have a minute to have a look at this PR?
It's really small, but we need this feature, and I don't want to rebuild the controller-manager myself with every k8s release we upgrade :P . Thanks in advance! :)

deads2k · 2017-04-11T17:09:49Z

/approve

mtaufen · 2017-04-25T00:10:35Z

/approve
the changes in componentconfig lgtm

k8s-github-robot · 2017-04-25T00:10:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DirectXMan12, Dmitry1987, deads2k, mtaufen

Details

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

~~cmd/kube-controller-manager/OWNERS~~ [deads2k]
~~hack/verify-flags/OWNERS~~ [deads2k]
~~pkg/apis/componentconfig/OWNERS~~ [mtaufen]
~~pkg/controller/OWNERS~~ [deads2k]

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-04-25T02:39:41Z

Automatic merge from submit-queue (batch tested with PRs 44862, 42241, 42101, 43181, 44147)

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Feb 25, 2017

k8s-github-robot assigned timothysc Feb 25, 2017

DirectXMan12 suggested changes Feb 27, 2017

View reviewed changes

timothysc added the sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. label Feb 27, 2017

timothysc assigned DirectXMan12 and piosz and unassigned timothysc Feb 27, 2017

Dmitry1987 force-pushed the feature/hpa-upscale-downscale-delay-configurable branch from 44f0193 to 3306615 Compare February 27, 2017 20:38

k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 27, 2017

Dmitry1987 force-pushed the feature/hpa-upscale-downscale-delay-configurable branch from 3306615 to abe024b Compare February 27, 2017 21:15

k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 27, 2017

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Feb 27, 2017

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 27, 2017

Dmitry1987 force-pushed the feature/hpa-upscale-downscale-delay-configurable branch from b0b2309 to c23fa18 Compare March 9, 2017 13:17

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 Mar 9, 2017

k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 23, 2017

Dmitry1987 force-pushed the feature/hpa-upscale-downscale-delay-configurable branch from 92a56fb to c23fa18 Compare March 24, 2017 17:21

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 24, 2017

Dmitry1987 force-pushed the feature/hpa-upscale-downscale-delay-configurable branch 2 times, most recently from 22d6cd3 to 1f816cf Compare March 24, 2017 17:38

make hpa upscale and downscale delay window configurable

965dab3

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 24, 2017

Dmitry1987 force-pushed the feature/hpa-upscale-downscale-delay-configurable branch from 1f816cf to 965dab3 Compare March 24, 2017 18:01

deads2k added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 11, 2017

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

k8s-github-robot merged commit 6c8cb33 into kubernetes:master Apr 25, 2017

Dmitry1987 deleted the feature/hpa-upscale-downscale-delay-configurable branch April 26, 2017 07:53

Comments

Conversation

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

Uh oh!

Uh oh!

k8s-ci-robot commented Feb 25, 2017

Uh oh!

k8s-ci-robot commented Feb 25, 2017

Uh oh!

k8s-ci-robot commented Feb 25, 2017

Uh oh!

k8s-reviewable commented Feb 25, 2017

Uh oh!

dannyz29 commented Feb 27, 2017

Uh oh!

Dmitry1987 commented Feb 27, 2017

Uh oh!

DirectXMan12 commented Feb 27, 2017

Uh oh!

DirectXMan12 left a comment

Choose a reason for hiding this comment

Uh oh!

DirectXMan12 Feb 27, 2017

Choose a reason for hiding this comment

Uh oh!

DirectXMan12 Feb 27, 2017

Choose a reason for hiding this comment

Uh oh!

DirectXMan12 commented Feb 27, 2017

Uh oh!

timothysc commented Feb 27, 2017

Uh oh!

snirad commented Feb 27, 2017

Uh oh!

Dmitry1987 commented Mar 8, 2017

Uh oh!

jszczepkowski commented Mar 9, 2017

Uh oh!

k8s-ci-robot commented Mar 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Dmitry1987 commented Mar 9, 2017

Uh oh!

Dmitry1987 commented Mar 16, 2017

Uh oh!

jszczepkowski commented Mar 16, 2017

Uh oh!

DirectXMan12 commented Mar 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DirectXMan12 commented Mar 24, 2017

Uh oh!

DirectXMan12 commented Mar 26, 2017

Uh oh!

Dmitry1987 commented Apr 3, 2017

Uh oh!

deads2k commented Apr 11, 2017

Uh oh!

mtaufen commented Apr 25, 2017

Uh oh!

k8s-github-robot commented Apr 25, 2017

Uh oh!

k8s-github-robot commented Apr 25, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

Dmitry1987 commented Feb 25, 2017 •

edited

Loading

k8s-ci-robot commented Mar 9, 2017 •

edited

Loading

DirectXMan12 commented Mar 24, 2017 •

edited

Loading