Refactor low node utilization #384

ingvagabund · 2020-08-21T16:01:16Z

Each time a pod's resource consumption is translated into a fraction (number in (0; 1) interval), some precision is lost. Instead, summing all resource consumption absolutely allows to compute the resource usage precisely (up to rounding losses when using Quantity type).

Storing resources consumption into a map of Quantities also allows to simplify arguments of functions used in the code.

ingvagabund · 2020-08-24T11:46:01Z

@seanmalloy @damemi @lixiang233 PTAL

changing the utilization computation from fractions (in percentages) to resources that are left instead
refactoring the code to make it more generic (so it's more reusable when implementing HighNodeUtilization strategy)

pkg/descheduler/strategies/lownodeutilization.go

seanmalloy · 2020-08-25T03:29:10Z

I don't see anything wrong with these changes. @lixiang233 please review again when you have some time. Thanks!

seanmalloy · 2020-08-26T03:43:36Z

/lgtm

@lixiang233 please review one more time when you get a chance. Thanks!

lixiang233 · 2020-08-26T06:08:51Z

@seanmalloy this change looks pretty good to me.

seanmalloy · 2020-08-26T14:13:02Z

/assign @damemi

seanmalloy · 2020-09-02T03:57:41Z

/kind cleanup

pkg/descheduler/strategies/lownodeutilization.go

ingvagabund · 2020-09-09T14:30:55Z

@seanmalloy done PTAL

seanmalloy · 2020-09-09T14:54:10Z

/lgtm
/assign @damemi

damemi

@ingvagabund could you add some more description to the commits/PR on what the goal is with this refactor, for future reference?

ingvagabund · 2020-09-10T13:59:08Z

@ingvagabund could you add some more description to the commits/PR on what the goal is with this refactor, for future reference?

Description updated. Lemme know whether it's sufficient.

damemi

This is definitely one of the more complex strategies and I'm thankful for the improvements to quality and readability @ingvagabund. Just a couple comments to make sure my understanding is correct

damemi · 2020-09-10T19:44:29Z

pkg/descheduler/strategies/lownodeutilization.go

+		func(node *v1.Node, usage NodeUsage) bool {
+			if nodeutil.IsNodeUnschedulable(node) {
+				klog.V(2).InfoS("Node is unschedulable, thus not considered as underutilized", "node", klog.KObj(node))
+				return false
+			}
+			return isNodeWithLowUtilization(usage)
+		},
+		func(node *v1.Node, usage NodeUsage) bool {
+			return isNodeAboveTargetUtilization(usage)
+		},


is it necessary to pass functions here? The checks seem simple enough that they could just be hardcoded into classifyNodes, unless something else is going to be re-using it with different filters

It's a preparation for HighNodeUtilization strategy where the upper/lower threshold conditions are different

pkg/descheduler/strategies/lownodeutilization.go

damemi · 2020-09-10T20:08:26Z

pkg/descheduler/strategies/lownodeutilization.go

-				tj += value
-			}
-		}
+		ti := nodes[i].usage[v1.ResourceMemory].Value() + nodes[i].usage[v1.ResourceCPU].MilliValue() + nodes[i].usage[v1.ResourcePods].Value()


do these still make sense to add together now that you're storing absolute values instead of percentages? Like if I have a node with 500 pods, 1 mem, 1 cpu that will be ranked higher usage than 10 pods, 100 mem, 100 cpu right? But idk if that's a reasonable comparison

I kept the original computation. Though, it might be better to rather carry addition over relative units. E.g. pods/totalpods + cpu/totalcpu + memory/totalmemory. So nodes with higher percentage of consumed resources have higher priority over those with lower percentage. As a feature we might allow to set weight for each resource. E.g. in case memory is more expensive than cpu.

…rutilized

seanmalloy · 2020-09-12T05:07:10Z

@damemi please take a look when you have some time. I believe @ingvagabund has addressed all of your review feedback.

/lgtm

damemi

/approve
Thanks @ingvagabund !

k8s-ci-robot · 2020-09-14T01:41:44Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

…node-utilization Refactor low node utilization

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 21, 2020

k8s-ci-robot requested review from damemi and k82cn August 21, 2020 16:01

k8s-ci-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 Aug 21, 2020

ingvagabund force-pushed the refactor-low-node-utilization branch from cf94218 to 7cf77a4 Compare August 24, 2020 11:15

ingvagabund changed the title ~~wip: Refactor low node utilization~~ Refactor low node utilization Aug 24, 2020

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 24, 2020

ingvagabund force-pushed the refactor-low-node-utilization branch 3 times, most recently from 041d3ce to e9c1975 Compare August 24, 2020 11:21

ingvagabund mentioned this pull request Aug 24, 2020

HighNodeUtilisation strategy #341

Closed

lixiang233 reviewed Aug 24, 2020

View reviewed changes

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

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

ingvagabund force-pushed the refactor-low-node-utilization branch from e9c1975 to a3d47fc Compare August 24, 2020 14:29

k8s-ci-robot assigned seanmalloy Aug 26, 2020

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

ingvagabund mentioned this pull request Aug 26, 2020

Change klog to use structured logging #376

Merged

k8s-ci-robot assigned damemi Aug 26, 2020

k8s-ci-robot added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Sep 2, 2020

ingvagabund force-pushed the refactor-low-node-utilization branch from a06ae4b to acf8857 Compare September 2, 2020 18:45

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 2, 2020

seanmalloy requested changes Sep 3, 2020

View reviewed changes

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

ingvagabund force-pushed the refactor-low-node-utilization branch from acf8857 to 432b674 Compare September 3, 2020 08:32

seanmalloy reviewed Sep 3, 2020

View reviewed changes

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

Remove createNodePodsMap

dc41e6a

ingvagabund force-pushed the refactor-low-node-utilization branch from 432b674 to f4a55cb Compare September 9, 2020 14:30

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

damemi reviewed Sep 9, 2020

View reviewed changes

damemi reviewed Sep 10, 2020

View reviewed changes

ingvagabund added 2 commits September 11, 2020 12:57

Compute utilization absolutely, not relatively

63039fc

Be verbose about unschedulable nodes which are not considered as unde…

9b4f781

…rutilized

ingvagabund force-pushed the refactor-low-node-utilization branch from f4a55cb to 9b4f781 Compare September 11, 2020 10:57

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 11, 2020

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

damemi approved these changes Sep 14, 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 Sep 14, 2020

k8s-ci-robot merged commit c9cfeb3 into kubernetes-sigs:master Sep 14, 2020

ingvagabund deleted the refactor-low-node-utilization branch September 14, 2020 08:27

damemi mentioned this pull request Sep 15, 2020

Node not indentified as underutilized #381

Closed

bytetwin mentioned this pull request Mar 31, 2021

Add HighNodeUtilization strategy #196

Closed

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

Merge pull request kubernetes-sigs#384 from ingvagabund/refactor-low-…

82cba73

…node-utilization Refactor low node utilization

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor low node utilization #384

Refactor low node utilization #384

ingvagabund commented Aug 21, 2020 •

edited

Loading

ingvagabund commented Aug 24, 2020

seanmalloy commented Aug 25, 2020

seanmalloy commented Aug 26, 2020

lixiang233 commented Aug 26, 2020

seanmalloy commented Aug 26, 2020

seanmalloy commented Sep 2, 2020

ingvagabund commented Sep 9, 2020

seanmalloy commented Sep 9, 2020

damemi left a comment

ingvagabund commented Sep 10, 2020

damemi left a comment

damemi Sep 10, 2020

ingvagabund Sep 11, 2020

damemi Sep 10, 2020

ingvagabund Sep 11, 2020

seanmalloy commented Sep 12, 2020

damemi left a comment

k8s-ci-robot commented Sep 14, 2020

Refactor low node utilization #384

Refactor low node utilization #384

Conversation

ingvagabund commented Aug 21, 2020 • edited Loading

ingvagabund commented Aug 24, 2020

seanmalloy commented Aug 25, 2020

seanmalloy commented Aug 26, 2020

lixiang233 commented Aug 26, 2020

seanmalloy commented Aug 26, 2020

seanmalloy commented Sep 2, 2020

ingvagabund commented Sep 9, 2020

seanmalloy commented Sep 9, 2020

damemi left a comment

Choose a reason for hiding this comment

ingvagabund commented Sep 10, 2020

damemi left a comment

Choose a reason for hiding this comment

damemi Sep 10, 2020

Choose a reason for hiding this comment

ingvagabund Sep 11, 2020

Choose a reason for hiding this comment

damemi Sep 10, 2020

Choose a reason for hiding this comment

ingvagabund Sep 11, 2020

Choose a reason for hiding this comment

seanmalloy commented Sep 12, 2020

damemi left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 14, 2020

ingvagabund commented Aug 21, 2020 •

edited

Loading