Change break condition and thresholds validation for lowUtilization #285

lixiang233 · 2020-05-15T09:10:08Z

We should stop evicting pods when any of totalCPU/totalMem/totalPods ran out, otherwise we may make underutilized nodes overutilized which is not efficient. To do this, there should be a value of totalCPU/Mem if their targetThreshold is not configured, here I set it to all of the remaining resources
on these nodes.

k8s-ci-robot · 2020-05-15T09:42:23Z

@lixiang233: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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.

ingvagabund

Thanks for noticing insufficiency in the stopping conditions. I agree the conditions need to be stronger. Which on the other hand requires all resources to be set. Also, defaulting cpu/memory to 100 allows a user to execute "I want to balance all nodes wrt. memory" case and yet have confidence cpu resource will be still balanced properly. Which, in the future can be extended with arbitrary set of resources to balance wrt some threshold.

ingvagabund · 2020-05-19T12:34:23Z

pkg/descheduler/strategies/lownodeutilization.go

@@ -249,7 +258,8 @@ func evictPods(
 	taintsOfLowNodes map[string][]v1.Taint,
 	podEvictor *evictions.PodEvictor,
 	node *v1.Node) {
-	if IsNodeAboveTargetUtilization(nodeUsage, targetThresholds) && (*totalPods > 0 || *totalCPU > 0 || *totalMem > 0) {
+	// stop if node utilization drops below target threshold or any of required capacity (cpu, memory, pods) is moved
+	if IsNodeAboveTargetUtilization(nodeUsage, targetThresholds) && *totalPods > 0 && *totalCPU > 0 && *totalMem > 0 {


Valid change: if at least one of the essential resources is zero, no pod can be rescheduled to a different node

ingvagabund · 2020-05-19T12:35:18Z

pkg/descheduler/strategies/lownodeutilization.go

-				// check if node utilization drops below target threshold or required capacity (cpu, memory, pods) is moved
-				if !IsNodeAboveTargetUtilization(nodeUsage, targetThresholds) || (*totalPods <= 0 && *totalCPU <= 0 && *totalMem <= 0) {
+				// check if node utilization drops below target threshold or any required capacity (cpu, memory, pods) is moved
+				if !IsNodeAboveTargetUtilization(nodeUsage, targetThresholds) || *totalPods <= 0 || *totalCPU <= 0 || *totalMem <= 0 {


Valid change: the same condition, just negated.

ingvagabund · 2020-05-19T12:46:51Z

pkg/descheduler/strategies/lownodeutilization.go

@@ -190,12 +190,21 @@ func evictPodsFromTargetNodes(
 		if _, ok := targetThresholds[v1.ResourceCPU]; ok {


I prefer to default both CPU and Memory target thresholds to 100% and fail, if either of thresholds is not set. Rather then checking it for every node. Can you move the change into LowNodeUtilization right after if !validateTargetThresholds(targetThresholds) condition and set the missing target thresholds there?

ingvagabund · 2020-05-19T13:14:44Z

pkg/descheduler/strategies/lownodeutilization_test.go

@@ -141,6 +144,64 @@ func TestLowNodeUtilization(t *testing.T) {
 			},
 			expectedPodsEvicted: 3,
 		},
+		{
+			name: "without priorities stop if any required capacity (cpu, memory, pods) is moved",


without priorities stop when cpu capacity is depleted

pkg/descheduler/strategies/lownodeutilization_test.go

ingvagabund · 2020-05-19T13:21:28Z

@lixiang233 can you also squash both commits into one?

ingvagabund · 2020-05-19T13:26:46Z

/ok-to-test

ingvagabund · 2020-05-19T14:18:34Z

@lixiang233 you will also need to call LowNodeUtilization directly in the TestLowNodeUtilization which will require to refactor TestLowNodeUtilization a bit. Plus, update README.md to mention the target thresholds default to 100% for cpu and memory.

lixiang233 · 2020-05-20T07:16:52Z

done. @ingvagabund

ingvagabund · 2020-05-20T12:12:04Z

README.md

@@ -91,7 +91,8 @@ Currently, pods request resource requirements are considered for computing node
 There is another configurable threshold, `targetThresholds`, that is used to compute those potential nodes
 from where pods could be evicted. Any node, between the thresholds, `thresholds` and `targetThresholds` is
 considered appropriately utilized and is not considered for eviction. The threshold, `targetThresholds`,
-can be configured for cpu, memory, and number of pods too in terms of percentage.
+can be configured for cpu, memory, and number of pods too in terms of percentage. Notice that `pods` must be
+configured and if `cpu` or `memory` not configured, they'll be set to default value `100`.


s/not configured/are not configured/

ingvagabund · 2020-05-20T12:13:10Z

pkg/descheduler/strategies/lownodeutilization.go

@@ -59,6 +59,13 @@ func LowNodeUtilization(ctx context.Context, client clientset.Interface, strateg
 	if !validateTargetThresholds(targetThresholds) {
 		return
 	}
+	// check if CPU/Mem not set in targetThresholds and set it to 100


check if CPU/Mem not set in targetThresholds and set it to 100
check if CPU/Mem are set in targetThresholds, if not, set them to 100

ingvagabund · 2020-05-20T12:14:36Z

pkg/descheduler/strategies/lownodeutilization.go

-			cpuPercentage := targetThresholds[v1.ResourceCPU] - node.usage[v1.ResourceCPU]
-			totalCPU += ((float64(cpuPercentage) * float64(nodeCapacity.Cpu().MilliValue())) / 100)
-		}
+		// CPU and Mem in targetThresholds have already validated in LowNodeUtilization


No need for the comment as it's assumed both targetThresholds are already set.

ingvagabund

Just nits, otherwise lgtm

lixiang233 · 2020-05-20T13:28:21Z

Changed, thanks for your help @ingvagabund

damemi · 2020-05-20T20:25:44Z

README.md

+can be configured for cpu, memory, and number of pods too in terms of percentage. Notice that `pods` must be
+configured and if `cpu` or `memory` are not configured, they'll be set to default value `100`.


why is pods required now, and we should add some validation to ensure that it's set

I only added default value to cpu and mem and didn't change the validate func .Current validateTargetThresholds func only checks wheather pods is configured, if not, LowNodeUtilization will stop, so only pods is required. Should we treatpods as same as cpu and mem, and validateTargetThresholds func returns true if any of these resources configured?

Maybe we can use validateThresholds to check targetThresholds

It looks like you pushed a change to set a default for pods, is that correct? If so please remember to update this readme note

Yes, I'll update it in later commit

lixiang233 · 2020-05-21T04:21:16Z

What about the latest commit? I unified the verification method for targetThresholds and thresholds, and set the allowed range of resource percentage to [0, 100]. @damemi @ingvagabund

ingvagabund · 2020-05-21T09:34:32Z

pkg/descheduler/strategies/lownodeutilization.go

@@ -117,11 +125,20 @@ func validateThresholds(thresholds api.ResourceThresholds) bool {
 	for name := range thresholds {
 		switch name {
 		case v1.ResourceCPU:
-			continue
+			if !validateResourcePercentage(thresholds[v1.ResourceCPU]) {


Can you refactor the function to return fmt.Errorf("cpu threshold not valid") instead of bool? Logging Info/Error and return bool is not practical. Instead, caller of validateThresholds can decide what to do about non-nil error.

ingvagabund · 2020-05-21T09:48:27Z

pkg/descheduler/strategies/lownodeutilization.go

@@ -56,15 +61,18 @@ func LowNodeUtilization(ctx context.Context, client clientset.Interface, strateg
 		return
 	}
 	targetThresholds := strategy.Params.NodeResourceUtilizationThresholds.TargetThresholds
-	if !validateTargetThresholds(targetThresholds) {
+	if !validateThresholds(targetThresholds) {


Better but not sufficient. thresholds has to be less than targetThresholds per a resource type. @lixiang233 Can you add check for that as well?

Also, thresholds and targetThresholds have gave me a lot of headache before learning what they actually mean. I would prefer to rename both of them into lowThreshold and highThreshold at least on the code level. Also given the strategy is configured as:

"LowNodeUtilization": enabled: true params: nodeResourceUtilizationThresholds: thresholds: "cpu" : 20 "memory": 20 "pods": 20 targetThresholds: "cpu" : 50 "memory": 50 "pods": 50

we can drop thresholds and targetThresholds and have:

"LowNodeUtilization": enabled: true params: nodeResourceUtilizationThresholds: low: "cpu" : 20 "memory": 20 "pods": 20 high: "cpu" : 50 "memory": 50 "pods": 50

@damemi @seanmalloy anything against?

Should we allow thresholds and targetThresholds configuring different resources? For instance, thresholds configured cpu and mem while targetThresholds only configured pods.

Good point. Assuming low threshold has a cpu set but high does not, you mark a node as underutilized wrt. cpu. You then find over-utilized node(s) wrt. memory. In the process of moving workload from over-utilized (since you want to decrease memory consumption), you move some pods to the underutilized nodes with a hope there's at least some memory left on those nodes. So, in practice this scenario is possible though you are removing any guarantee the list of underutilized nodes is selected effectively. I.e. the nodes have sufficient memory left. Since there's no low threshold defaulting for memory, you might consume all the memory left and end up with nodes which are memory over utilized. Also, selecting under-utilized nodes based on cpu becomes irrelevant since you then may pick any subset of nodes since you still have no guarantee there's sufficient memory left.

So, the answer is no.

ingvagabund · 2020-05-21T09:50:19Z

pkg/descheduler/strategies/lownodeutilization.go

-		klog.V(1).Infof("no target resource threshold for pods is configured")
-		return false
+// validateResourcePercentage checks if resource percentage is in the range [0, 100]
+func validateResourcePercentage(percent api.Percentage) bool {


I don't think validateResourcePercentage is the right function to unit test given its length and complexity. Can you move the distinct cases into TestValidateThresholds and drop TestValidateResourcePercentage completely?

ingvagabund · 2020-05-21T13:43:26Z

pkg/descheduler/strategies/lownodeutilization_test.go

@@ -453,7 +543,7 @@ func TestValidateThresholds(t *testing.T) {
 	}

 	for _, test := range tests {
-		isValid := validateThresholds(test.input)
+		isValid := (validateThresholds(test.input) == nil)

 		if isValid != test.succeed {


Please replace succeed bool with error err and set it to nil or a specific error instead given validateThresholds now returns err.

Got it. I'll commit later.

lixiang233 · 2020-05-22T03:40:32Z

One more question, after I added new validation check, TestWithTaints failed because strategy's TargetThresholds is changed in LowNodeUtilization while thresholds not. Should I set thresholds to 100 while setting TargetThresholds to make sure strategy is always valid or just change the testcase? I prefer the first one. @ingvagabund

lixiang233 · 2020-05-22T08:37:57Z

Updated README and validation checks for the two thresholds @damemi @ingvagabund

pkg/descheduler/strategies/lownodeutilization.go

ingvagabund · 2020-05-22T15:21:42Z

@lixiang233 that's the last nit I found. Sorry for the review taking so long. Thanks for asking the right questions!!!

lixiang233 · 2020-05-23T07:46:20Z

Simplified validateThresholds and squashed all commits into one. @ingvagabund @damemi

ingvagabund

Just some documentation nits. Otherwise, lgtm.

ingvagabund · 2020-05-25T10:17:25Z

README.md

@@ -89,7 +89,8 @@ usage is below threshold for all (cpu, memory, and number of pods), the node is
 Currently, pods request resource requirements are considered for computing node resource utilization.

 There is another configurable threshold, `targetThresholds`, that is used to compute those potential nodes
-from where pods could be evicted. Any node, between the thresholds, `thresholds` and `targetThresholds` is
+from where pods could be evicted. If a node's usage is above targetThreshold for any (cpu, memory, or number of pods),
+the node is considered over utilized. Any node, between the thresholds, `thresholds` and `targetThresholds` is


Any node between the thresholds thresholds and targetThresholds is ... (no need for the comas)

ingvagabund · 2020-05-25T10:17:49Z

README.md

@@ -114,6 +115,15 @@ strategies:
           "pods": 50
 ```

+Policy should pass the following validation checks:
+* Only three types of resource are supported: `cpu`, `memory` and `pods`.


s/resource/resources

ingvagabund · 2020-05-25T10:27:00Z

README.md

+* The valid range of the resource's percentage value is \[0, 100\]
+* Percentage value of `thresholds` can not be greater than `targetThresholds` for the same resource.
+
+You can configure any number of these three resources, if one resource is not configured, it will be set to default


If any of the resource types is not specified, all its thresholds default to 100% to avoid nodes going from underutilized to overutilized.

All these modified

1.Set default CPU/Mem/Pods percentage of thresholds to 100 2.Stop evicting pods if any resource ran out 3.Add thresholds verification method and limit resource percentage within [0, 100] 4.Change testcases and readme

ingvagabund · 2020-05-25T18:11:20Z

/lgtm

damemi

/approve
Thanks for the fix! I think this is one of the more confusing strategies, so I appreciate the help fixing bugs like this.

damemi · 2020-05-28T14:20:00Z

pkg/descheduler/strategies/lownodeutilization.go

+	// check if Pods/CPU/Mem are set, if not, set them to 100
+	if _, ok := thresholds[v1.ResourcePods]; !ok {
+		thresholds[v1.ResourcePods] = MaxResourcePercentage
+		targetThresholds[v1.ResourcePods] = MaxResourcePercentage
+	}
+	if _, ok := thresholds[v1.ResourceCPU]; !ok {
+		thresholds[v1.ResourceCPU] = MaxResourcePercentage
+		targetThresholds[v1.ResourceCPU] = MaxResourcePercentage
+	}
+	if _, ok := thresholds[v1.ResourceMemory]; !ok {
+		thresholds[v1.ResourceMemory] = MaxResourcePercentage
+		targetThresholds[v1.ResourceMemory] = MaxResourcePercentage
+	}


nit: I feel like this section could probably be moved to validateStrategyConfig but I won't block on it

k8s-ci-robot · 2020-05-28T14:34:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: damemi, lixiang233

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:

~~OWNERS~~ [damemi]

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

seanmalloy · 2020-05-29T05:59:15Z

/kind bug

kubernetes-sigs#285 got merged before re-running the unit tests.

Based on https://github.com/kubernetes/community/blob/master/community-membership.md#requirements-1: The following apply to the part of codebase for which one would be a reviewer in an OWNERS file (for repos using the bot). > member for at least 3 months For a couple of years now > Primary reviewer for at least 5 PRs to the codebase kubernetes-sigs#285 kubernetes-sigs#275 kubernetes-sigs#267 kubernetes-sigs#254 kubernetes-sigs#181 > Reviewed or merged at least 20 substantial PRs to the codebase https://github.com/kubernetes-sigs/descheduler/pulls?q=is%3Apr+is%3Aclosed+assignee%3Aingvagabund > Knowledgeable about the codebase yes > Sponsored by a subproject approver > With no objections from other approvers > Done through PR to update the OWNERS file this PR > May either self-nominate, be nominated by an approver in this subproject, or be nominated by a robot self-nominating

…tilization_break Change break condition and thresholds validation for lowUtilization

kubernetes-sigs#285 got merged before re-running the unit tests.

Based on https://github.com/kubernetes/community/blob/master/community-membership.md#requirements-1: The following apply to the part of codebase for which one would be a reviewer in an OWNERS file (for repos using the bot). > member for at least 3 months For a couple of years now > Primary reviewer for at least 5 PRs to the codebase kubernetes-sigs#285 kubernetes-sigs#275 kubernetes-sigs#267 kubernetes-sigs#254 kubernetes-sigs#181 > Reviewed or merged at least 20 substantial PRs to the codebase https://github.com/kubernetes-sigs/descheduler/pulls?q=is%3Apr+is%3Aclosed+assignee%3Aingvagabund > Knowledgeable about the codebase yes > Sponsored by a subproject approver > With no objections from other approvers > Done through PR to update the OWNERS file this PR > May either self-nominate, be nominated by an approver in this subproject, or be nominated by a robot self-nominating

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 15, 2020

k8s-ci-robot requested review from damemi and k82cn May 15, 2020 09:10

ingvagabund requested changes May 19, 2020

View reviewed changes

k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label May 19, 2020

lixiang233 force-pushed the Ft_change_lowUtilization_break branch from 1d61242 to 92fc350 Compare May 20, 2020 06:58

k8s-ci-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 May 20, 2020

lixiang233 force-pushed the Ft_change_lowUtilization_break branch from 92fc350 to 6c34ad2 Compare May 20, 2020 07:08

lixiang233 changed the title ~~Change break confition of lowUtilization~~ Change break condition for lowUtilization May 20, 2020

ingvagabund reviewed May 20, 2020

View reviewed changes

ingvagabund requested changes May 20, 2020

View reviewed changes

lixiang233 force-pushed the Ft_change_lowUtilization_break branch from 6c34ad2 to 564f628 Compare May 20, 2020 13:04

lixiang233 closed this May 20, 2020

lixiang233 reopened this May 20, 2020

damemi reviewed May 20, 2020

View reviewed changes

ingvagabund requested changes May 21, 2020

View reviewed changes

ingvagabund reviewed May 21, 2020

View reviewed changes

lixiang233 changed the title ~~Change break condition for lowUtilization~~ Change break condition and thresholds validation for lowUtilization May 22, 2020

ingvagabund requested changes May 22, 2020

View reviewed changes

pkg/descheduler/strategies/lownodeutilization.go Show resolved Hide resolved

lixiang233 force-pushed the Ft_change_lowUtilization_break branch from e86db6d to 11663c2 Compare May 23, 2020 07:31

ingvagabund reviewed May 25, 2020

View reviewed changes

Stop condition and config validation change for lowUtilization

2a8dc69

1.Set default CPU/Mem/Pods percentage of thresholds to 100 2.Stop evicting pods if any resource ran out 3.Add thresholds verification method and limit resource percentage within [0, 100] 4.Change testcases and readme

lixiang233 force-pushed the Ft_change_lowUtilization_break branch from 11663c2 to 2a8dc69 Compare May 25, 2020 11:04

k8s-ci-robot assigned ingvagabund May 25, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 25, 2020

damemi approved these changes May 28, 2020

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 28, 2020

k8s-ci-robot merged commit 616a9b5 into kubernetes-sigs:master May 28, 2020

k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label May 29, 2020

lixiang233 deleted the Ft_change_lowUtilization_break branch May 29, 2020 08:15

ingvagabund mentioned this pull request May 29, 2020

Sync with upstream openshift/descheduler#32

Closed

ingvagabund added a commit to ingvagabund/descheduler that referenced this pull request May 29, 2020

Add missing address of api.StrategyParameters

abdf794

kubernetes-sigs#285 got merged before re-running the unit tests.

ingvagabund mentioned this pull request May 29, 2020

Add missing address of api.StrategyParameters #306

Merged

ingvagabund mentioned this pull request Jun 4, 2020

Add myself to project reviewers #317

Merged

lixiang233 mentioned this pull request Sep 10, 2020

Elements of nodeResourceUtilizationThresholds should be optional #212

Closed

ingvagabund mentioned this pull request Nov 12, 2020

Support extended resources in LowNodeUtilization #434

Merged

lixiang233 mentioned this pull request Feb 22, 2021

Add a parameter for making the thresholds of the LowNodeUtilization strategy relative to average values #473

Closed

briend pushed a commit to briend/descheduler that referenced this pull request Feb 11, 2022

Merge pull request kubernetes-sigs#285 from lixiang233/Ft_change_lowU…

8986629

…tilization_break Change break condition and thresholds validation for lowUtilization

briend pushed a commit to briend/descheduler that referenced this pull request Feb 11, 2022

Add missing address of api.StrategyParameters

35c8abd

kubernetes-sigs#285 got merged before re-running the unit tests.

		@@ -190,12 +190,21 @@ func evictPodsFromTargetNodes(
		if _, ok := targetThresholds[v1.ResourceCPU]; ok {

		can be configured for cpu, memory, and number of pods too in terms of percentage. Notice that `pods` must be
		configured and if `cpu` or `memory` are not configured, they'll be set to default value `100`.

Change break condition and thresholds validation for lowUtilization #285

Change break condition and thresholds validation for lowUtilization #285

Conversation

lixiang233 commented May 15, 2020

k8s-ci-robot commented May 15, 2020

ingvagabund 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

ingvagabund commented May 19, 2020

ingvagabund commented May 19, 2020

ingvagabund commented May 19, 2020 • edited

lixiang233 commented May 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ingvagabund left a comment

Choose a reason for hiding this comment

lixiang233 commented May 20, 2020

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

lixiang233 commented May 21, 2020

ingvagabund May 21, 2020 • edited

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

lixiang233 commented May 22, 2020

lixiang233 commented May 22, 2020

ingvagabund commented May 22, 2020

lixiang233 commented May 23, 2020 • edited

ingvagabund 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

ingvagabund commented May 25, 2020

damemi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented May 28, 2020

seanmalloy commented May 29, 2020

ingvagabund commented May 19, 2020 •

edited

ingvagabund May 21, 2020 •

edited

lixiang233 commented May 23, 2020 •

edited