Add support for scale up/down delay #2598

44past4 · 2019-11-28T17:19:52Z

Fix for #2597 for 1.8

k8s-ci-robot · 2019-11-28T17:20:00Z

Welcome @44past4!

It looks like this is your first PR to kubernetes/autoscaler 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/autoscaler has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

44past4 · 2019-11-28T17:22:18Z

/assign @wojtek-t

wojtek-t · 2019-12-02T13:05:24Z

addon-resizer/nanny/estimator.go

@@ -98,6 +98,8 @@ func calculateResources(numNodes uint64, resources []Resource) *corev1.ResourceR
 		overhead := computeResourceOverheadValueString(numNodes, r)
 		newRes := r.Base
 		newRes.Add(resource.MustParse(overhead))
+		// Force initialization of newRes.s so that it could be logged
+		newRes.String()


Seriously? You really need?
How this is logged?

Without this call this value can be logged as:
{i:{value:0 scale:0} d:{Dec:0xc4202b5d70} s: Format:DecimalSI}
WIth this call this becomes:
{i:{value:0 scale:0} d:{Dec:0xc4202b5d70} s:455m Format:DecimalSI}

How do you print it? If you would log it with:
klog.("%s", <your var>)
if would work fine (as this is calling String() method.

And it seems like a better way to print it - things like d:{Dec:0xc4202b5d70} aren't particularly useful :)

This value is not printed directly but instead it is nested in k8s.io/api/core/v1/ResourceRequirements which is printed with %+v.
I will change it in a way that I will serialize ResourceRequirements to json before displaying it.

bskiba · 2019-12-03T09:11:33Z

addon-resizer/nanny/nanny_lib.go

+			log.V(3).Infof("Resources are not within the expected limits, Actual: %+v, accepted range: %+v. Skipping resource update because of scale up/down delay", *resources, *expResources)
+			continue
+		}
+
 		log.Infof("Resources are not within the expected limits, updating the deployment. Actual: %+v Expected: %+v", *resources, *expResources)


nit: can you make this log consistent with the one above? Also could you add a log verbosity level?

Log message has been updated. I have not added log verbosity because I believe that this message will be logged only when actual resize will be performed and therefore it is ok to log this message without a verbosity level being set.

bskiba · 2019-12-03T09:15:53Z

addon-resizer/nanny/nanny_lib.go

+	scaleDown operation = iota
+	scaleUp   operation = iota
+)
+
 // checkResource determines whether a specific resource needs to be over-written.


Can you update the comment?

mwielgus

Same comments as on #2599

Please provide more information on the E2E tests performed.

mwielgus · 2019-12-06T04:40:17Z

addon-resizer/nanny/nanny_lib.go

 			log.V(4).Infof("Resources are within the expected limits. Actual: %+v Expected: %+v", *resources, *expResources)
 			continue
 		}

+		now := time.Now()
+		if (op == scaleDown && now.Before(lastChange.Add(scaleDownDelay))) ||


Can you point me to the specific line in test where this thing is properly unit-tested. I don't see a line where we check what happens if last change is slightly before or slightly after scale****Delay. But maybe I'm missing something.

I have extracted logic for checking resources for deployment and updating them to a separate method and added unit tests for this.
When it comes E2E tests I have build an image with addon-resizer, deployed it to a Kubernetes cluster first without any delay being set and then with scale up and scale down being set and I have scaled cluster from 3 to 10 nodes and then back to 3 nodes.

wojtek-t · 2019-12-17T11:59:01Z

addon-resizer/nanny/nanny_lib.go

+		override, op = checkResource(threshold, reqs, expReqs, resourceType)
+		if override {
+			return true, op
+		}


This silently assumes that we don't go up in some resource and down on the other resource.

This is probably fine, but it requires an explicit comment.

I have added a comment about the operation type to the function description.

wojtek-t · 2019-12-17T13:54:52Z

addon-resizer/nanny/nanny_lib.go

+	if (op == scaleDown && now.Before(lastChange.Add(scaleDownDelay))) ||
+		(op == scaleUp && now.Before(lastChange.Add(scaleUpDelay))) {
+		if log.V(3) {
+			log.V(3).Infof("Resources are not within the expected limits, Actual: %+v, accepted range: %+v. Skipping resource update because of scale up/down delay", jsonOrValue(*resources), jsonOrValue(*expResources))


I actually think this should be logged unconditionally - this is important log and lack of it may introduce a lot of misunderstanding - please change to simply log.Infof

Logging this unconditionally could spam logs with hundreds of the same messages were only the first one is really useful for the user. Therefore I have decided to change the implementation in a way that I will log this message unconditionally but only for the first time we when we are skipping the the override.

wojtek-t · 2019-12-18T08:49:15Z

addon-resizer/nanny/nanny_lib.go

+// updateResources counts the number of nodes, estimates the expected
+// ResourceRequirements, compares them to the actual ResourceRequirements, and
+// updates the deployment with the expected ResourceRequirements if necessary.
+// It returns true if deployment has been updated.


no longer true

Updated the information about the returned values.

wojtek-t · 2019-12-18T08:50:33Z

addon-resizer/nanny/nanny_lib.go

-			log.V(4).Infof("Resources are within the expected limits. Actual: %+v Expected: %+v", *resources, *expResources)
-			continue
+	// If there's a difference, go ahead and set the new values.
+	overwrite, op := shouldOverwriteResources(int64(threshold), resources.Limits, resources.Requests, expResources.Limits, expResources.Requests)


nit: consistency - you're using overwrite in some places and override in other
Can you please unify it?

I have changed all occurrences of override to overwrite.

wojtek-t · 2019-12-18T09:20:24Z

/lgtm
/approve

k8s-ci-robot · 2019-12-18T09:20:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wojtek-t

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:

~~addon-resizer/OWNERS~~ [wojtek-t]

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

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

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 28, 2019

k8s-ci-robot requested review from bskiba and wojtek-t November 28, 2019 17:20

k8s-ci-robot assigned wojtek-t Nov 28, 2019

44past4 force-pushed the cooldown-1.8 branch from cd4b744 to ec7a0f3 Compare November 29, 2019 12:07

wojtek-t reviewed Dec 2, 2019

View reviewed changes

bskiba reviewed Dec 3, 2019

View reviewed changes

mwielgus reviewed Dec 6, 2019

View reviewed changes

44past4 force-pushed the cooldown-1.8 branch 3 times, most recently from 5077ba6 to 893093b Compare December 17, 2019 07:18

wojtek-t reviewed Dec 17, 2019

View reviewed changes

44past4 force-pushed the cooldown-1.8 branch 2 times, most recently from 7701275 to a007438 Compare December 18, 2019 08:23

wojtek-t reviewed Dec 18, 2019

View reviewed changes

44past4 force-pushed the cooldown-1.8 branch 2 times, most recently from 251e7ca to 5c35e71 Compare December 18, 2019 09:10

Add support for scale up/down delay

e158b6b

44past4 force-pushed the cooldown-1.8 branch from 5c35e71 to e158b6b Compare December 18, 2019 09:13

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 18, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 18, 2019

k8s-ci-robot merged commit 0aa7321 into kubernetes:addon-resizer-release-1.8 Dec 18, 2019

44past4 mentioned this pull request Dec 18, 2019

Add support for scale up/down delay #2599

Merged

44past4 mentioned this pull request Feb 28, 2020

REQUEST: New membership for 44past4 kubernetes/org#1672

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for scale up/down delay #2598

Add support for scale up/down delay #2598

44past4 commented Nov 28, 2019

k8s-ci-robot commented Nov 28, 2019

44past4 commented Nov 28, 2019

wojtek-t Dec 2, 2019

44past4 Dec 2, 2019

wojtek-t Dec 2, 2019

wojtek-t Dec 2, 2019

44past4 Dec 11, 2019

bskiba Dec 3, 2019

44past4 Dec 11, 2019

bskiba Dec 3, 2019

44past4 Dec 11, 2019

mwielgus left a comment

mwielgus Dec 6, 2019

44past4 Dec 11, 2019

wojtek-t Dec 17, 2019

44past4 Dec 18, 2019

wojtek-t Dec 17, 2019

44past4 Dec 18, 2019

wojtek-t Dec 18, 2019

44past4 Dec 18, 2019

wojtek-t Dec 18, 2019

44past4 Dec 18, 2019

wojtek-t commented Dec 18, 2019

k8s-ci-robot commented Dec 18, 2019

Add support for scale up/down delay #2598

Add support for scale up/down delay #2598

Conversation

44past4 commented Nov 28, 2019

k8s-ci-robot commented Nov 28, 2019

44past4 commented Nov 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwielgus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wojtek-t commented Dec 18, 2019

k8s-ci-robot commented Dec 18, 2019