Volcano scheduler: Supports network topology aware scheduling when pods rescheduled #3971

ecosysbin · 2025-01-14T14:43:05Z

What type of PR is this?
/kind feature
/area scheduling

What this PR does / why we need it:
Volcano scheduler: Supports network topology aware scheduling when pods rescheduled

Which issue(s) this PR fixes:
#3877

JesseStutler · 2025-01-15T02:46:11Z

pkg/scheduler/actions/allocate/allocate.go

+			if jobHyperNode != "" {
+				LCAHyperNode := ssn.HyperNodes.GetLCAHyperNode(hyperNodeName, jobHyperNode)
+				hyperNodeInfo := ssn.HyperNodes[LCAHyperNode]
+				if hyperNodeInfo.Tier()+1 > highestAllowedTier {


Shouldn't add 1 here, for example, s0 and s1 both are tier1 hypernode and they are sibling hypernodes, and current jobHyperNode is s1. Then LCAHyperNode is s4, which are s0 and s1's parent, tier 2 > highestAllowedTier, there is no need to add 1

ok， I have done it

JesseStutler · 2025-01-15T02:50:25Z

pkg/scheduler/actions/allocate/allocate.go

+				LCAHyperNode := ssn.HyperNodes.GetLCAHyperNode(hyperNodeName, jobHyperNode)
+				hyperNodeInfo := ssn.HyperNodes[LCAHyperNode]
+				if hyperNodeInfo.Tier()+1 > highestAllowedTier {
+					klog.V(4).InfoS("Skip search for higher tier cause highest allowed tier reached", "jobName", job.UID, "highestAllowedTier", highestAllowedTier, "tier", index+1)


Can the log here be distinguished from the previous? It could be written like this: Current tier of LCAHyperNode is larger than highestAllowedTier, skipping

index+1? Should be hypernodeInfo.Tier()

ok, I have done it.

ecosysbin · 2025-01-15T03:12:13Z

/assign @Monokaix

JesseStutler · 2025-01-15T03:02:04Z

pkg/scheduler/cache/event_handlers.go

@@ -819,7 +819,10 @@ func (sc *SchedulerCache) AddPodGroupV1beta1(obj interface{}) {
 		klog.Errorf("Failed to convert podgroup from %T to %T", ss, podgroup)
 		return
 	}
-
+	if podgroup.GetAnnotations() == nil {
+		klog.V(3).Infof("Add PodGroup(%s) into cache, spec(%#v)", ss.Name, ss.Spec)


Is it to solve the nil map problem? Why to add a log to print the whole podgroup in if here...

Is it to solve the nil map problem? yes, I have clear the log.

JesseStutler · 2025-01-15T06:29:08Z

pkg/scheduler/actions/allocate/allocate.go

@@ -290,6 +302,10 @@ func (alloc *Action) allocateResourceForTasksWithTopology(tasks *util.PriorityQu
 		klog.V(4).InfoS("Find available hyperNodes for job", "jobName", job.UID, "tier", selectedTier, "hyperNodes", hyperNodes)
 	}
 	stmt, hyperNode := alloc.selectBestHyperNode(jobStmtsByTier[selectedTier], job)
+	jobNewHyperNode := LCAHyperNodeMap[hyperNode]
+	if jobNewHyperNode != jobHyperNode {
+		job.PodGroup.GetAnnotations()[api.TopologyAllocateLCAHyperNode] = jobHyperNode


Should be jobNewHyperNode?

yes, thanks, l will modify it

JesseStutler · 2025-01-15T06:53:37Z

pkg/scheduler/api/well_known_labels.go

@@ -42,4 +42,7 @@ const (

 	// topologyDecisionAnnotation is the key of topology decision about pod request resource
 	topologyDecisionAnnotation = "volcano.sh/topology-decision"
+
+	// TopologyAllocateLCAHyperNode is the key to the lowest common ancestor of the network topology to which the tasks assigned to a job belong.


Better to rewrite as TopologyAllocateLCAHyperNode is the key recording the lowest common ancestor hypernode of job

All right. I have done it

JesseStutler · 2025-01-15T07:01:09Z

pkg/scheduler/cache/cache.go

@@ -1564,6 +1565,9 @@ func (sc *SchedulerCache) UpdateJobStatus(job *schedulingapi.JobInfo, updatePG b
 		if err != nil {
 			return nil, err
 		}
+		sc.Mutex.Lock()


Can we wrap it as a func?

that’s ok. I have done it

JesseStutler · 2025-01-15T07:19:56Z

pkg/scheduler/actions/allocate/allocate.go

@@ -290,6 +302,10 @@ func (alloc *Action) allocateResourceForTasksWithTopology(tasks *util.PriorityQu
 		klog.V(4).InfoS("Find available hyperNodes for job", "jobName", job.UID, "tier", selectedTier, "hyperNodes", hyperNodes)
 	}
 	stmt, hyperNode := alloc.selectBestHyperNode(jobStmtsByTier[selectedTier], job)
+	jobNewHyperNode := LCAHyperNodeMap[hyperNode]


selectBestHyperNode may fail to return a true hypernode, then it will return empty string "", so we may exclude BestHyperNode is an empty string

ok， I have modify it.

JesseStutler · 2025-01-15T08:27:57Z

pkg/scheduler/actions/allocate/allocate.go

+			if jobHyperNode != "" {
+				LCAHyperNode := ssn.HyperNodes.GetLCAHyperNode(hyperNodeName, jobHyperNode)
+				hyperNodeInfo := ssn.HyperNodes[LCAHyperNode]
+				if hyperNodeInfo.Tier() > highestAllowedTier {


I tested a case where the two hypernodes are isolated from each other, they don't have LCAHyperNode, which should also be skipped at this point, at this point fetching from ssn.HyperNodes will be nil and calling Tier() will be panic. We should also add UT to cover this

yes ,have done

pkg/scheduler/actions/allocate/allocate_test.go

Monokaix · 2025-01-16T01:17:52Z

pkg/scheduler/actions/allocate/allocate_test.go

+			MinimalBindCheck: true,
+		},
+		{
+			Name: "hard network topology constrain and job rescheduler, can not allocate job when LCAHyperNode is empty",


JesseStutler · 2025-01-16T03:46:17Z

pkg/scheduler/api/node_info.go

@@ -90,6 +91,15 @@ type NodeInfo struct {
 	ImageStates map[string]*k8sframework.ImageStateSummary
 }

+// PodGroupOldState recored podgroup old state
+type PodGroupOldState struct {
+	// Status recored podgroup status during schedule


typo recored, records is enough

…ds rescheduled Signed-off-by: ecosysbin <14729934+ecosysbin@user.noreply.gitee.com>

Monokaix · 2025-01-17T01:10:19Z

/approve

volcano-sh-bot · 2025-01-17T01:10:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Monokaix

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:

~~pkg/scheduler/OWNERS~~ [Monokaix]

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

hwdef

/lgtm

hwdef · 2025-01-17T02:20:02Z

By the way, PodGroupOldState is a weird name

volcano-sh-bot requested review from alcorj-mizar and k82cn January 14, 2025 14:43

volcano-sh-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 14, 2025

ecosysbin force-pushed the rescheduler branch from 95a93df to d715c8a Compare January 14, 2025 15:02

JesseStutler reviewed Jan 15, 2025

View reviewed changes

ecosysbin force-pushed the rescheduler branch from d715c8a to 4522e7f Compare January 15, 2025 02:57

volcano-sh-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 15, 2025

ecosysbin force-pushed the rescheduler branch from 4522e7f to 196b72b Compare January 15, 2025 03:10

volcano-sh-bot assigned Monokaix Jan 15, 2025

ecosysbin force-pushed the rescheduler branch 2 times, most recently from bff6064 to d1fd45c Compare January 15, 2025 04:51

JesseStutler reviewed Jan 15, 2025

View reviewed changes

ecosysbin force-pushed the rescheduler branch from d1fd45c to 5760f43 Compare January 15, 2025 07:59

volcano-sh-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 15, 2025

ecosysbin force-pushed the rescheduler branch 2 times, most recently from ff29b61 to 9ad3afc Compare January 15, 2025 08:25

JesseStutler reviewed Jan 15, 2025

View reviewed changes

ecosysbin force-pushed the rescheduler branch 2 times, most recently from 45e4b1a to 8fb1eae Compare January 15, 2025 09:36

volcano-sh-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 15, 2025

ecosysbin force-pushed the rescheduler branch 2 times, most recently from 9068dfa to 54188de Compare January 15, 2025 09:54

volcano-sh-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 15, 2025

ecosysbin force-pushed the rescheduler branch 3 times, most recently from 44bee2a to 283be11 Compare January 15, 2025 15:23

Monokaix reviewed Jan 16, 2025

View reviewed changes

pkg/scheduler/actions/allocate/allocate_test.go Outdated Show resolved Hide resolved

Monokaix reviewed Jan 16, 2025

View reviewed changes

ecosysbin force-pushed the rescheduler branch 2 times, most recently from 1fb58f1 to a06eb75 Compare January 16, 2025 03:03

JesseStutler reviewed Jan 16, 2025

View reviewed changes

ecosysbin force-pushed the rescheduler branch 10 times, most recently from 10b47d1 to 7b97ae6 Compare January 16, 2025 15:38

Volcano scheduler: Supports network topology aware scheduling when po…

67957a1

…ds rescheduled Signed-off-by: ecosysbin <14729934+ecosysbin@user.noreply.gitee.com>

ecosysbin force-pushed the rescheduler branch from 7b97ae6 to 67957a1 Compare January 16, 2025 15:57

volcano-sh-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 17, 2025

hwdef reviewed Jan 17, 2025

View reviewed changes

volcano-sh-bot assigned hwdef Jan 17, 2025

volcano-sh-bot added the lgtm Indicates that a PR is ready to be merged. label Jan 17, 2025

volcano-sh-bot merged commit 4c100b1 into volcano-sh:network-topology Jan 17, 2025
10 checks passed

volcano-sh-bot added kind/feature Categorizes issue or PR as related to a new feature. area/scheduling labels Jan 17, 2025

Volcano scheduler: Supports network topology aware scheduling when pods rescheduled #3971

Volcano scheduler: Supports network topology aware scheduling when pods rescheduled #3971

Uh oh!

Conversation

ecosysbin commented Jan 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ecosysbin commented Jan 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Monokaix commented Jan 17, 2025

Uh oh!

volcano-sh-bot commented Jan 17, 2025

Uh oh!

hwdef left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hwdef commented Jan 17, 2025

Uh oh!

Uh oh!

ecosysbin commented Jan 14, 2025 •

edited

Loading