scheduler: add new pod estimate with loadaware plugin #1992

zwForrest · 2024-04-08T12:42:05Z

Ⅰ. Describe what this PR does

When scheduling a new pod, it is necessary to consider the resource utilization of the new pod with the loadaware plugin, to prevent the node utilization from becoming too high after the new pod is scheduled.

This can prevent nodes with excessive utilization entering score phase.

The resource utilization for the new pod here is calculated using the original estimated method.

Ⅱ. 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

koordinator-bot · 2024-04-08T12:42:15Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign eahydra after the PR has been reviewed.
You can assign the PR to them by writing /assign @eahydra in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

pkg/scheduler/OWNERS

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

codecov · 2024-04-08T12:48:05Z

Codecov Report

Attention: Patch coverage is 78.26087% with 10 lines in your changes are missing coverage. Please review.

Project coverage is 68.75%. Comparing base (2dc8735) to head (8943b2d).
Report is 44 commits behind head on main.

Files	Patch %	Lines
pkg/scheduler/plugins/loadaware/load_aware.go	78.26%	5 Missing and 5 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1992      +/-   ##
==========================================
+ Coverage   67.50%   68.75%   +1.25%     
==========================================
  Files         421      423       +2     
  Lines       47100    39174    -7926     
==========================================
- Hits        31795    26936    -4859     
+ Misses      12990     9909    -3081     
- Partials     2315     2329      +14

Flag	Coverage Δ
unittests	`68.75% <78.26%> (+1.25%)`	⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pkg/scheduler/plugins/loadaware/load_aware.go

hormes · 2024-04-28T07:31:00Z

What is the status of this issue?

zwForrest · 2024-04-30T08:57:14Z

What is the status of this issue?

The corresponding changes have been updated.

pkg/scheduler/plugins/loadaware/load_aware.go

eahydra · 2024-05-07T11:42:45Z

I'm sorry I've been away from X for a while.

I reviewed this PR and reviewed the implementation of the loadaware plugin. I noticed that there are many areas that need to be improved in the code I implemented in the past. Especially in this PR, you can see that the logic of filter and score are very similar. We should calculate the usage first, and then decide whether to score or filter. When calculating usage, we can decide whether to use estimated.

These codes should be wrapped as a function that can be used in filter and score stage.

koordinator/pkg/scheduler/plugins/loadaware/load_aware.go

Lines 291 to 327 in 038308b

    
           prodPod := extension.GetPodPriorityClassWithDefault(pod) == extension.PriorityProd && p.args.ScoreAccordingProdUsage 
        
           podMetrics := buildPodMetricMap(p.podLister, nodeMetric, prodPod) 
        
           estimatedUsed, err := p.estimator.EstimatePod(pod) 
        
           if err != nil { 
        
           	return 0, nil 
        
           } 
        
           assignedPodEstimatedUsed, estimatedPods := p.estimatedAssignedPodUsed(nodeName, nodeMetric, podMetrics, prodPod) 
        
           for resourceName, value := range assignedPodEstimatedUsed { 
        
           	estimatedUsed[resourceName] += value 
        
           } 
        
           podActualUsages, estimatedPodActualUsages := sumPodUsages(podMetrics, estimatedPods) 
        
           if prodPod { 
        
           	for resourceName, quantity := range podActualUsages { 
        
           		estimatedUsed[resourceName] += getResourceValue(resourceName, quantity) 
        
           	} 
        
           } else { 
        
           	if nodeMetric.Status.NodeMetric != nil { 
        
           		var nodeUsage *slov1alpha1.ResourceMap 
        
           		if scoreWithAggregation(p.args.Aggregated) { 
        
           			nodeUsage = getTargetAggregatedUsage(nodeMetric, &p.args.Aggregated.ScoreAggregatedDuration, p.args.Aggregated.ScoreAggregationType) 
        
           		} else { 
        
           			nodeUsage = &nodeMetric.Status.NodeMetric.NodeUsage 
        
           		} 
        
           		if nodeUsage != nil { 
        
           			for resourceName, quantity := range nodeUsage.ResourceList { 
        
           				if q := estimatedPodActualUsages[resourceName]; !q.IsZero() { 
        
           					quantity = quantity.DeepCopy() 
        
           					if quantity.Cmp(q) >= 0 { 
        
           						quantity.Sub(q) 
        
           					} 
        
           				} 
        
           				estimatedUsed[resourceName] += getResourceValue(resourceName, quantity) 
        
           			} 
        
           		} 
        
           	} 
        
           }

zwForrest · 2024-05-08T13:14:51Z

I'm sorry I've been away from X for a while.

I reviewed this PR and reviewed the implementation of the loadaware plugin. I noticed that there are many areas that need to be improved in the code I implemented in the past. Especially in this PR, you can see that the logic of filter and score are very similar. We should calculate the usage first, and then decide whether to score or filter. When calculating usage, we can decide whether to use estimated.

These codes should be wrapped as a function that can be used in filter and score stage.

koordinator/pkg/scheduler/plugins/loadaware/load_aware.go

Lines 291 to 327 in 038308b

prodPod := extension.GetPodPriorityClassWithDefault(pod) == extension.PriorityProd && p.args.ScoreAccordingProdUsage

podMetrics := buildPodMetricMap(p.podLister, nodeMetric, prodPod)

estimatedUsed, err := p.estimator.EstimatePod(pod)

if err != nil {

return 0, nil

}

assignedPodEstimatedUsed, estimatedPods := p.estimatedAssignedPodUsed(nodeName, nodeMetric, podMetrics, prodPod)

for resourceName, value := range assignedPodEstimatedUsed {

estimatedUsed[resourceName] += value

}

podActualUsages, estimatedPodActualUsages := sumPodUsages(podMetrics, estimatedPods)

if prodPod {

for resourceName, quantity := range podActualUsages {

estimatedUsed[resourceName] += getResourceValue(resourceName, quantity)

}

} else {

if nodeMetric.Status.NodeMetric != nil {

var nodeUsage *slov1alpha1.ResourceMap

if scoreWithAggregation(p.args.Aggregated) {

nodeUsage = getTargetAggregatedUsage(nodeMetric, &p.args.Aggregated.ScoreAggregatedDuration, p.args.Aggregated.ScoreAggregationType)

} else {

nodeUsage = &nodeMetric.Status.NodeMetric.NodeUsage

}

if nodeUsage != nil {

for resourceName, quantity := range nodeUsage.ResourceList {

if q := estimatedPodActualUsages[resourceName]; !q.IsZero() {

quantity = quantity.DeepCopy()

if quantity.Cmp(q) >= 0 {

quantity.Sub(q)

}

}

estimatedUsed[resourceName] += getResourceValue(resourceName, quantity)

}

}

}

}

Yes, I noticed this issue while making the changes. I will further modify this part.

saintube · 2024-05-23T01:52:59Z

@zwForrest Thanks for your contributions! Please resolve the conflicts and we will continue the review.

codecov-commenter · 2024-05-24T01:49:10Z

Codecov Report

Attention: Patch coverage is 78.26087% with 10 lines in your changes are missing coverage. Please review.

Project coverage is 68.99%. Comparing base (2dc8735) to head (0a6249c).
Report is 56 commits behind head on main.

❗ Current head 0a6249c differs from pull request most recent head c77ed01

Please upload reports for the commit c77ed01 to get more accurate results.

Files	Patch %	Lines
pkg/scheduler/plugins/loadaware/load_aware.go	78.26%	5 Missing and 5 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1992      +/-   ##
==========================================
+ Coverage   67.50%   68.99%   +1.49%     
==========================================
  Files         421      430       +9     
  Lines       47100    40048    -7052     
==========================================
- Hits        31795    27633    -4162     
+ Misses      12990    10056    -2934     
- Partials     2315     2359      +44

Flag	Coverage Δ
unittests	`68.99% <78.26%> (+1.49%)`	⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

zwForrest · 2024-05-27T09:30:51Z

@zwForrest Thanks for your contributions! Please resolve the conflicts and we will continue the review.

Done, The ci error is caused by other code. @saintube

pkg/scheduler/plugins/loadaware/load_aware.go

Signed-off-by: zwForrest <756495135@qq.com>

saintube · 2024-06-07T03:07:51Z

@zwForrest Please check your logic to pass the UTs.

saintube · 2024-06-07T03:09:01Z

pkg/scheduler/plugins/loadaware/load_aware.go

@@ -140,117 +140,47 @@ func (p *Plugin) Filter(ctx context.Context, state *framework.CycleState, pod *c
 		}
 		return framework.NewStatus(framework.Error, err.Error())
 	}
+	if nodeMetric.Status.NodeMetric == nil {
+		klog.Warningf("nodeMetrics(%s) should not be nil.\n", node.Name)


klog / glog does not need a tailing \n.
Same below.

saintube · 2024-06-07T03:13:28Z

pkg/scheduler/plugins/loadaware/load_aware.go

+			continue
+		}
+		usage := int64(math.Round(float64(estimatedUsed[resourceName]) / float64(total) * 100))
+		if usage > value {


to reduce the cyclomatic complexity and improve the readability, consider this pattern:

if usage <= value { continue } // ...

koordinator-bot bot requested review from buptcozy and eahydra April 8, 2024 12:42

koordinator-bot bot added the size/L label Apr 8, 2024

zwForrest force-pushed the feat/updateloadaware branch 2 times, most recently from 84026c9 to 8e7711f Compare April 8, 2024 13:02

eahydra reviewed Apr 10, 2024

View reviewed changes

pkg/scheduler/plugins/loadaware/load_aware.go Outdated Show resolved Hide resolved

eahydra reviewed Apr 10, 2024

View reviewed changes

pkg/scheduler/plugins/loadaware/load_aware.go Outdated Show resolved Hide resolved

zwForrest force-pushed the feat/updateloadaware branch from 52e117b to dcb76fe Compare April 12, 2024 03:16

eahydra reviewed Apr 12, 2024

View reviewed changes

pkg/scheduler/plugins/loadaware/load_aware.go Outdated Show resolved Hide resolved

pkg/scheduler/plugins/loadaware/load_aware.go Outdated Show resolved Hide resolved

zwForrest force-pushed the feat/updateloadaware branch from 27b00fa to 5b1bba3 Compare April 29, 2024 09:44

eahydra reviewed May 7, 2024

View reviewed changes

pkg/scheduler/plugins/loadaware/load_aware.go Outdated Show resolved Hide resolved

pkg/scheduler/plugins/loadaware/load_aware.go Outdated Show resolved Hide resolved

pkg/scheduler/plugins/loadaware/load_aware.go Outdated Show resolved Hide resolved

koordinator-bot bot added size/XL and removed size/L labels May 9, 2024

zwForrest force-pushed the feat/updateloadaware branch from 4304b54 to 8943b2d Compare May 9, 2024 08:27

zwForrest force-pushed the feat/updateloadaware branch from b2d4a43 to 0a6249c Compare May 24, 2024 01:49

zwForrest force-pushed the feat/updateloadaware branch from bcaa7dc to 0a6249c Compare May 27, 2024 11:26

saintube reviewed May 28, 2024

View reviewed changes

scheduler: add new pod estimate in loadaware plugin

c77ed01

Signed-off-by: zwForrest <756495135@qq.com>

zwForrest force-pushed the feat/updateloadaware branch from ad20f2c to c77ed01 Compare May 30, 2024 12:59

saintube reviewed Jun 7, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

scheduler: add new pod estimate with loadaware plugin #1992

scheduler: add new pod estimate with loadaware plugin #1992

zwForrest commented Apr 8, 2024

koordinator-bot bot commented Apr 8, 2024

codecov bot commented Apr 8, 2024 •

edited

hormes commented Apr 28, 2024

zwForrest commented Apr 30, 2024

eahydra commented May 7, 2024

zwForrest commented May 8, 2024

saintube commented May 23, 2024

codecov-commenter commented May 24, 2024 •

edited by codecov bot

zwForrest commented May 27, 2024 •

edited

saintube commented Jun 7, 2024 •

edited

saintube Jun 7, 2024

saintube Jun 7, 2024

scheduler: add new pod estimate with loadaware plugin #1992

Are you sure you want to change the base?

scheduler: add new pod estimate with loadaware plugin #1992

Conversation

zwForrest commented Apr 8, 2024

Ⅰ. Describe what this PR does

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how to verify it

Ⅳ. Special notes for reviews

V. Checklist

koordinator-bot bot commented Apr 8, 2024

codecov bot commented Apr 8, 2024 • edited

Codecov Report

hormes commented Apr 28, 2024

zwForrest commented Apr 30, 2024

eahydra commented May 7, 2024

zwForrest commented May 8, 2024

saintube commented May 23, 2024

codecov-commenter commented May 24, 2024 • edited by codecov bot

Codecov Report

zwForrest commented May 27, 2024 • edited

saintube commented Jun 7, 2024 • edited

saintube Jun 7, 2024

Choose a reason for hiding this comment

saintube Jun 7, 2024

Choose a reason for hiding this comment

codecov bot commented Apr 8, 2024 •

edited

codecov-commenter commented May 24, 2024 •

edited by codecov bot

zwForrest commented May 27, 2024 •

edited

saintube commented Jun 7, 2024 •

edited