support node resource reservation #998

lucming · 2023-02-09T02:05:05Z

Ⅰ. Describe what this PR does

support resource reservation from node.annotation.
see this proposal for details: #922

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how to verify it

Ⅳ. Special notes for reviews

V. Checklist

I have written necessary docs and comments
I have added necessary unit tests and integration tests
All checks passed in make test

codecov · 2023-02-09T02:26:00Z

Codecov Report

Patch coverage: 86.82% and project coverage change: -0.19 ⚠️

Comparison is base (f8b4e31) 67.09% compared to head (3273a42) 66.91%.

❗ Current head 3273a42 differs from pull request most recent head d0b7c11. Consider uploading reports for the commit d0b7c11 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #998      +/-   ##
==========================================
- Coverage   67.09%   66.91%   -0.19%     
==========================================
  Files         262      272      +10     
  Lines       28960    29735     +775     
==========================================
+ Hits        19430    19896     +466     
- Misses       8173     8428     +255     
- Partials     1357     1411      +54

Flag	Coverage Δ
unittests	`66.91% <86.82%> (-0.19%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/koordlet/resmanager/cpu_suppress.go	`73.34% <78.94%> (+3.63%)`	⬆️
pkg/util/node.go	`86.04% <84.61%> (-2.19%)`	⬇️
...xt/sharedlisterext/node_reservation_transformer.go	`86.36% <86.36%> (ø)`
...dlet/statesinformer/states_noderesourcetopology.go	`66.12% <86.95%> (+2.20%)`	⬆️
pkg/scheduler/plugins/elasticquota/node_handler.go	`71.23% <100.00%> (+4.04%)`	⬆️
.../plugins/nodenumaresource/topology_eventhandler.go	`57.14% <100.00%> (+1.26%)`	⬆️
...oller/noderesource/plugins/batchresource/plugin.go	`69.27% <100.00%> (-0.73%)`	⬇️

... and 58 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

eahydra

Wow, long awaited PR. I read part of the code first, and I will continue to read it later. BTW docker/koord-runtimeproxy.docker maybe removed?

pkg/scheduler/plugins/nodeReservation/plugin.go

apis/extension/constants.go

apis/extension/node.go

pkg/koordlet/resmanager/cpu_suppress.go

pkg/slo-controller/noderesource/resource_calculator.go

apis/extension/node.go

zwzhang0107 · 2023-02-13T05:01:37Z

pkg/slo-controller/noderesource/resource_calculator.go

+	// if specific cpus reserved by annotation of node, should remove those cpus.
+	cpusReservedByNodeAnno, _ := util.GetCPUsReservedByNodeAnno(node.Annotations)
+	if quantity, ok := result[corev1.ResourceCPU]; ok {
+		quantity.Sub(cpusReservedByNodeAnno[corev1.ResourceCPU])


only for QOSEffected == LSE, since LSR can be shared with BE

zwzhang0107 · 2023-02-13T05:03:45Z

pkg/koordlet/resmanager/cpu_suppress.go

 	var lsrCpus []koordletutil.ProcessorInfo
 	var lsCpus []koordletutil.ProcessorInfo
 	// FIXME: be pods might be starved since lse pods can run out of all cpus
 	for _, processor := range nodeCPUInfo.ProcessorInfos {
+		cpuset := cpuset.NewCPUSet(int(processor.CPUID))
+		if cpuset.IsSubsetOf(reservedByNode) {


should only exclude reservedByNode(qosEffected==LSE).

zwzhang0107 · 2023-02-13T05:13:35Z

pkg/koordlet/resmanager/cpu_suppress.go

+		cpuReservedByNode, _ = util.GetCPUsReservedByNode(reserved)
+	}
+
+	// suppress(BE) := node.Total * SLOPercent - pod(LS).Used - system.Used - reservedByNodeAnno


acturally, CPUSuppress want to ensure that nodeCPUUtilPercent <= SLOPercent.
the system.Used already includes the usage of non-pod-type process.
so the target should be:

for QosEffected == LSE
suppress(BE) := node.Total * SLOPercent - pod(LS-type).Used - max(system.Used, reservedByNodeAnno)

for QosEffected == LSR
suppress(BE) := node.Total * SLOPercent - pod(LS-type).Used - system.Used

apis/extension/node_reservation.go

docker/koord-runtimeproxy.dockerfile

apis/extension/node_reservation.go

pkg/scheduler/plugins/nodenumaresource/topology_eventhandler.go

pkg/scheduler/plugins/nodereservation/plugin.go

eahydra · 2023-02-23T13:50:08Z

pkg/util/node.go

+}
+
+// RemoveNodeReservedCPUs filter out cpus that reserved by annotation of node.
+func RemoveNodeReservedCPUs(cpuSharePools []apiext.CPUSharedPool, cpusReservedByNodeAnno string) []apiext.CPUSharedPool {


I think the function RemoveNodeReservedCPUs maybe just used by koordlet, move the function to the koordlet/util?

pkg/util/node.go

pkg/slo-controller/noderesource/resource_calculator.go

eahydra · 2023-02-23T13:52:38Z

There is another special place that needs to be modified. The code related to elasticQuota will perceive Node.Status.Allocatable as the total amount that can be allocated by Quota. Here we also need to modify it.

saintube · 2023-03-23T02:02:16Z

pkg/slo-controller/noderesource/plugins/batchresource/plugin.go

@@ -133,6 +133,14 @@ func (p *Plugin) Calculate(strategy *extension.ColocationStrategy, node *corev1.
 	nodeUsage := getNodeMetricUsage(nodeMetric.Status.NodeMetric)
 	systemUsed := quotav1.Max(quotav1.Subtract(nodeUsage, podAllUsed), util.NewZeroResourceList())

+	// System.Used = max(System.Used, Node.Anno.Reserved)
+	nodeReserved := util.GetNodeReservationFromAnnotation(node.Annotations)
+	for name, sysUsedQ := range systemUsed {


How about using quotav1.Max()?
There miss some checks, e.g. if the returned nodeReserved == nil, and node.Annotations == nil (mainly for testing).

saintube · 2023-03-23T02:04:31Z

pkg/slo-controller/noderesource/plugins/batchresource/plugin.go

@@ -225,6 +233,11 @@ func getNodeMetricUsage(info *slov1alpha1.NodeMetricInfo) corev1.ResourceList {
 func getNodeAllocatable(node *corev1.Node) corev1.ResourceList {
 	result := node.Status.Allocatable.DeepCopy()
 	result = quotav1.Mask(result, []corev1.ResourceName{corev1.ResourceCPU, corev1.ResourceMemory})
+
+	// if something reserved by node.annotation, should remove those resource.


The node reserved resources would be substracted in the Calculate(). We should not double-subtract the node reserved.

this function is used in Calculate()

Yes, it is used in the Calculate. Please check the formula if the nodeAllocatable was subtracted with the node.anno.reserved.

nodeAllocatable contains node.allocatable[memory] and node.allocatable[cpu], we haven't modified this, so there is no double-subtract node.anno.reserved

We've defined the formula: batch.alloc := node.alloc * ratio - sum(pod(Prod).usage) - max(sys.usage, node.anno.reserved), where node.anno.reserved would be subtracted no more than once. While in the current implementation, we subtract the node.anno.reserved firstly in the getNodeAllocatable and secondly in the Calculate.

node.alloc = node.alloc - node.anno.reserved
batch.alloc := node.alloc * ratio - sum(pod(Prod).usage) - sys.usage
so use this formula to calculate?

node.alloc = node.alloc - node.anno.reserved batch.alloc := node.alloc * ratio - sum(pod(Prod).usage) - sys.usage so use this formula to calculate?

This formula is incorrect as we discussed in #922, where sys.usage also includes the usage of node.anno.reserved.

saintube · 2023-03-23T02:10:40Z

pkg/koordlet/resmanager/cpu_suppress.go

+		return
+	}
+
+	cpusReservedByAnno, _ := apiext.GetReservedCPUs(topo.Annotations)


how to handle the "numReservedCPUs"

noderesourcetopo.annotation will only store the specific cores to be reserved. either by quantity or by specifying the CPU directly, the exact core that is reserved will be calculated, and then written to nodetopo.annotation["node.koordinator.sh/reservation"]

In the current implementation, if we set the NodeReservation.Resources to a non-zero quantity and set the NodeReservation.ReservedCPUs as empty, the reserved resources by quantity will be ignored unexpectedly.

please see this demo, https://github.com/koordinator-sh/koordinator/blob/20cc5a4673362afc8aba1e28556529599dc4a456/docs/proposals/scheduling/20221227-node-resource-reservation.md#example

saintube · 2023-03-23T02:14:59Z

pkg/slo-controller/noderesource/plugins/batchresource/plugin.go

@@ -133,6 +133,14 @@ func (p *Plugin) Calculate(strategy *extension.ColocationStrategy, node *corev1.
 	nodeUsage := getNodeMetricUsage(nodeMetric.Status.NodeMetric)
 	systemUsed := quotav1.Max(quotav1.Subtract(nodeUsage, podAllUsed), util.NewZeroResourceList())

+	// System.Used = max(System.Used, Node.Anno.Reserved)
+	nodeReserved := util.GetNodeReservationFromAnnotation(node.Annotations)


To ensure the calculation is correct, please add a UT case for the node resource reservation later.

pkg/koordlet/resmanager/cpu_suppress.go

pkg/koordlet/statesinformer/states_noderesourcetopology.go

saintube · 2023-03-28T02:54:54Z

pkg/koordlet/resmanager/cpu_suppress.go

+	notPodUsed := systemUsedCPU
+	rl := util.GetNodeReservationFromAnnotation(node.Annotations)
+	if rl != nil {
+		nodeAnnoReserved, _ := rl[corev1.ResourceCPU]


also use quotav1.Max()

saintube · 2023-03-28T02:56:39Z

pkg/slo-controller/noderesource/plugins/batchresource/plugin.go

@@ -138,6 +138,10 @@ func (p *Plugin) Calculate(strategy *extension.ColocationStrategy, node *corev1.
 	nodeUsage := getNodeMetricUsage(nodeMetric.Status.NodeMetric)
 	systemUsed := quotav1.Max(quotav1.Subtract(nodeUsage, podAllUsed), util.NewZeroResourceList())

+	// System.Used = max(System.Used, Node.Anno.Reserved)
+	nodeAnnoReserved := util.GetNodeReservationFromAnnotation(node.Annotations)
+	systemUsed = quotav1.Max(systemUsed, nodeAnnoReserved)


Could you please add a case in UT TestPluginCalculate where the node has annotation reserved resources to verify the calculation?

Signed-off-by: lucming <2876757716@qq.com>

zwzhang0107 · 2023-03-29T02:26:03Z

/lgtm

saintube

/lgtm
Well done. Thanks for your contributions.

zwzhang0107 · 2023-03-29T04:04:04Z

/approve

FillZpp

Nice work!

koordinator-bot · 2023-03-29T04:06:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: eahydra, FillZpp, saintube, zwzhang0107

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:

~~apis/OWNERS~~ [FillZpp]
~~pkg/koordlet/OWNERS~~ [saintube,zwzhang0107]
~~pkg/scheduler/OWNERS~~ [eahydra]
~~pkg/slo-controller/OWNERS~~ [zwzhang0107]
~~pkg/util/OWNERS~~ [FillZpp]

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

koordinator-bot bot requested review from eahydra, FillZpp, jasonliu747, saintube and zwzhang0107 February 9, 2023 02:05

koordinator-bot bot added the size/XL label Feb 9, 2023

lucming force-pushed the static_cpu_allocation2 branch 2 times, most recently from 163f452 to 3a1ea13 Compare February 9, 2023 02:10

lucming force-pushed the static_cpu_allocation2 branch from 3a1ea13 to e54ce35 Compare February 9, 2023 02:48

eahydra changed the title ~~resource reservation~~ support node resource reservation Feb 9, 2023

eahydra requested review from hormes and ZiMengSheng February 9, 2023 03:03

eahydra reviewed Feb 9, 2023

View reviewed changes

pkg/scheduler/plugins/nodeReservation/plugin.go Outdated Show resolved Hide resolved

apis/extension/constants.go Outdated Show resolved Hide resolved

apis/extension/node.go Outdated Show resolved Hide resolved

apis/extension/node.go Outdated Show resolved Hide resolved

zwzhang0107 reviewed Feb 9, 2023

View reviewed changes

pkg/koordlet/resmanager/cpu_suppress.go Outdated Show resolved Hide resolved

pkg/slo-controller/noderesource/resource_calculator.go Outdated Show resolved Hide resolved

saintube reviewed Feb 9, 2023

View reviewed changes

apis/extension/node.go Outdated Show resolved Hide resolved

apis/extension/node.go Outdated Show resolved Hide resolved

apis/extension/node.go Outdated Show resolved Hide resolved

lucming force-pushed the static_cpu_allocation2 branch 2 times, most recently from 48316bb to c9bc526 Compare February 9, 2023 16:06

zwzhang0107 reviewed Feb 13, 2023

View reviewed changes

koordinator-bot bot added size/XXL and removed size/XL labels Feb 14, 2023

lucming force-pushed the static_cpu_allocation2 branch from 8dc5e16 to cc94337 Compare February 14, 2023 04:54

koordinator-bot bot added size/XL and removed size/XXL labels Feb 14, 2023

lucming force-pushed the static_cpu_allocation2 branch 3 times, most recently from 9a62955 to 50ccbe9 Compare February 14, 2023 06:19

hormes reviewed Feb 15, 2023

View reviewed changes

apis/extension/node_reservation.go Outdated Show resolved Hide resolved

eahydra requested changes Feb 23, 2023

View reviewed changes

eahydra requested a review from zwzhang0107 March 22, 2023 12:38

lucming force-pushed the static_cpu_allocation2 branch from 9b76936 to a06c6a9 Compare March 22, 2023 13:36

saintube reviewed Mar 23, 2023

View reviewed changes

zwzhang0107 reviewed Mar 23, 2023

View reviewed changes

pkg/koordlet/resmanager/cpu_suppress.go Show resolved Hide resolved

pkg/koordlet/statesinformer/states_noderesourcetopology.go Show resolved Hide resolved

lucming force-pushed the static_cpu_allocation2 branch 2 times, most recently from 02d3ed6 to 08633e8 Compare March 24, 2023 02:40

koordinator-bot bot added size/XXL and removed size/XL labels Mar 24, 2023

lucming force-pushed the static_cpu_allocation2 branch from 08633e8 to 707d230 Compare March 24, 2023 08:59

jasonliu747 force-pushed the static_cpu_allocation2 branch from 707d230 to 48cd91a Compare March 27, 2023 09:04

eahydra added enhancement New feature or request area/koordlet area/koord-scheduler area/koord-manager labels Mar 27, 2023

saintube reviewed Mar 28, 2023

View reviewed changes

lucming force-pushed the static_cpu_allocation2 branch from 48cd91a to 3273a42 Compare March 29, 2023 01:36

support node resource reservation

d0b7c11

Signed-off-by: lucming <2876757716@qq.com>

lucming force-pushed the static_cpu_allocation2 branch from 3273a42 to d0b7c11 Compare March 29, 2023 01:37

koordinator-bot bot assigned zwzhang0107 Mar 29, 2023

koordinator-bot bot added the lgtm label Mar 29, 2023

saintube approved these changes Mar 29, 2023

View reviewed changes

koordinator-bot bot assigned saintube Mar 29, 2023

eahydra requested review from FillZpp and jasonliu747 March 29, 2023 04:03

FillZpp approved these changes Mar 29, 2023

View reviewed changes

koordinator-bot bot added the approved label Mar 29, 2023

koordinator-bot bot merged commit 76604d6 into koordinator-sh:main Mar 29, 2023
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support node resource reservation #998

support node resource reservation #998

lucming commented Feb 9, 2023

codecov bot commented Feb 9, 2023 •

edited

eahydra left a comment

zwzhang0107 Feb 13, 2023

zwzhang0107 Feb 13, 2023

zwzhang0107 Feb 13, 2023

eahydra Feb 23, 2023

eahydra commented Feb 23, 2023

saintube Mar 23, 2023

saintube Mar 23, 2023

lucming Mar 23, 2023

saintube Mar 23, 2023

lucming Mar 23, 2023

lucming Mar 24, 2023

saintube Mar 24, 2023 •

edited

lucming Mar 24, 2023

saintube Mar 24, 2023

lucming Mar 24, 2023

saintube Mar 23, 2023

lucming Mar 23, 2023

saintube Mar 23, 2023

lucming Mar 23, 2023

saintube Mar 23, 2023

saintube Mar 23, 2023

lucming Mar 24, 2023

saintube Mar 28, 2023

lucming Mar 29, 2023

saintube Mar 28, 2023

lucming Mar 29, 2023

zwzhang0107 commented Mar 29, 2023

saintube left a comment

zwzhang0107 commented Mar 29, 2023

FillZpp left a comment

koordinator-bot bot commented Mar 29, 2023

support node resource reservation #998

support node resource reservation #998

Conversation

lucming commented Feb 9, 2023

Ⅰ. Describe what this PR does

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how to verify it

Ⅳ. Special notes for reviews

V. Checklist

codecov bot commented Feb 9, 2023 • edited

Codecov Report

eahydra 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

eahydra commented Feb 23, 2023

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

saintube Mar 24, 2023 • 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

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

zwzhang0107 commented Mar 29, 2023

saintube left a comment

Choose a reason for hiding this comment

zwzhang0107 commented Mar 29, 2023

FillZpp left a comment

Choose a reason for hiding this comment

koordinator-bot bot commented Mar 29, 2023

codecov bot commented Feb 9, 2023 •

edited

saintube Mar 24, 2023 •

edited