Add network-topology-aware plugin and hyperNode score callback #3894

ecosysbin · 2024-12-16T03:16:12Z

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

What this PR does / why we need it:
Add network-topology-aware plugin and hyperNode score callback

Which issue(s) this PR fixes:
#3885

volcano-sh-bot · 2024-12-16T03:16:15Z

Welcome @ecosysbin!

It looks like this is your first PR to volcano-sh/volcano.

Thank you, and welcome to Volcano. 😃

ecosysbin · 2024-12-16T07:41:57Z

/assign @Monokaix

JesseStutler · 2024-12-17T06:54:46Z

pkg/scheduler/api/job_info.go

+
+	// The RootHypernode property of a job is the hypernode that serves as the smallest root in the hypernode tree.
+	// A job has multiple tasks, each belonging to a hypernode. This RootHypernode is the topmost and lowest common ancestor among the hypernodes of all tasks within the job.
+	RootHyperNode string


When is RootHyperNode assigned? I didn't find any codes assign the value to RootHyperNode

when the task of job commit, the hyperNode of the task will assigned to job. it will add the code later.

JesseStutler · 2024-12-17T06:55:33Z

pkg/scheduler/api/types.go

@@ -300,6 +301,9 @@ type NodeReduceFn func(*TaskInfo, k8sframework.NodeScoreList) error
 // NodeOrderMapFn is the func declaration used to get priority score of all plugins for a node for a particular task.
 type NodeOrderMapFn func(*TaskInfo, *NodeInfo) (map[string]float64, float64, error)

+// HyperNodeOrderFn is the func declaration used to score hyperNodes for job.
+type HyperNodeOrderFn func(*JobInfo, map[string][]*NodeInfo, []int, map[int][]string, map[string]sets.Set[string]) (map[string]float64, error)


You may need to rebase this pr and factor your code:https://github.com/volcano-sh/volcano/pull/3874/files, the HyperNodeOrderFn structure has changed.

ok. l will do it later.

JesseStutler · 2024-12-17T06:56:56Z

pkg/scheduler/framework/session.go

+	// hyperNode can gain a better performance, the lower the tier of hyperNode, the better performance.
+	HyperNodesListByTier map[int][]string
+
+	HyperNodesTiers []int


Same as above, there are no so many fields now, may need to rebase and refactor: https://github.com/volcano-sh/volcano/pull/3874/files

ok. According to the discussion, there will have a hyperNode tree here. it will do in another PR.

JesseStutler · 2024-12-17T07:01:19Z

pkg/scheduler/plugins/networktopologyaware/networktopologyaware.go

+
+// Goals:
+// - The tier to which the rootHypernode of a job belongs should be as low as possible.
+func networkTopologyAwareScore(hyperNode string, job *api.JobInfo, hyperNodesTiers []int, hyperNodesListByTier map[int][]string, hyperNodesMap map[string]sets.Set[string]) float64 {


I didn’t get the point for this score func. Does this have a design plan or a picture? After refactoring based on this PR: https://github.com/volcano-sh/volcano/pull/3874/files, could you add more details or description about how to score hypernodes in your pr?

Explanation:
The RootHypernode property of a job is the hypernode that serves as the smallest root in the hypernode
tree.
A job has multiple tasks, each belonging to a hypernode. This RootHypernode is the topmost and lowest common ancestor among the hypernodes of all tasks within the job.

Goals:
The tier to which the rootHypernode of a job belongs should be as low as possible.

I will change the name 'RootHypernode' to 'LCAHyperNode' later.

JesseStutler · 2024-12-27T02:48:18Z

pkg/scheduler/plugins/networktopologyaware/networktopologyaware.go

+// - Tasks under a job should be scheduled to one hyperNode as much as possible.
+func networkTopologyAwareScore(hyperNodeName string, job *api.JobInfo, hyperNodeTree []map[string][]string) float64 {
+	jobHyperNode := job.PodGroup.GetAnnotations()[api.TopologyAllocateLCAHyperNode]
+	// job fist first scheduler.


-> //job is scheduled for the first time

JesseStutler · 2024-12-27T02:59:07Z

pkg/scheduler/plugins/networktopologyaware/networktopologyaware.go

+// HyperNodeTree is the hypernode tree of all hypernodes in the cluster.
+// currentJobLCAHyperNode is the hypernode of the job's LCAHyperNode.
+var (
+	HyperNodeTree []map[string][]string


Do we need to maintain a separate global variable here? And this data structure does not seem to be called HyperNodeTree, it does not reflect hierarchical relationships.

l have move the hyperNodeTree to session, According to the discussion, this variable will be replaced by Gu Peng's HyperNodeTree later.

JesseStutler · 2024-12-27T03:06:37Z

pkg/scheduler/util/scheduler_helper.go

+	return taskCount
+}
+
+// FindOutRootHyperNode find out the root hypernode of the job when the hypernode join the job.


-> //FindLCAHyperNode finds out the common ancestor of the current hypernode and the hypernode where the job is scheduled

JesseStutler · 2024-12-27T03:55:35Z

pkg/scheduler/util/scheduler_helper_test.go

+	}
+}
+
+func TestFindLCAHyperNode(t *testing.T) {


None of the use cases specify currentJobLCAHyperNode?

JesseStutler · 2024-12-27T06:18:58Z

pkg/scheduler/util/scheduler_helper.go

+	}
+
+	hyperNodesMap := make(map[string]sets.Set[string])
+	for i := 0; i < len(revertHyperNodeTree); i++ {


extract a public function, has the same code in FindLCAHyperNode

JesseStutler · 2024-12-27T06:27:08Z

pkg/scheduler/plugins/networktopologyaware/networktopologyaware.go

+	// PluginName indicates name of volcano scheduler plugin.
+	PluginName            = "networktopologyaware"
+	BaseScore             = 100
+	TaskBaseScore         = 10


Is 10 points reasonable? need to discuss /cc @Monokaix

To ensure that the scores for the task num are distributed between 0 and 10, so that the scores for the tier level will be higher than those for the task num.

JesseStutler · 2024-12-27T06:35:00Z

pkg/scheduler/framework/session.go

@@ -127,8 +126,10 @@ func openSession(cache cache.Cache) *Session {

 		TotalResource:  api.EmptyResource(),
 		TotalGuarantee: api.EmptyResource(),
-		podGroupStatus: map[api.JobID]scheduling.PodGroupStatus{},
-
+		PodGroupCache: api.PodGroupCache{


It is better to rename as PodGroupOldState, which is used to record some old PodGroup data.

JesseStutler · 2024-12-27T06:41:43Z

pkg/scheduler/cache/cache.go

 		job.PodGroup = pg
 	}
-
+	sc.Jobs[job.UID] = job


No need to update here

JesseStutler · 2024-12-27T06:44:25Z

pkg/scheduler/cache/cache.go

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


I think there is no need to add lock? session --> UpdateJobStatus is still in the same goroutine, and there is no other goroutine will read or write this map

According to the discussion, locks need to be added for all operations on the cache.

Here is to update podgroup to apiserver. If you just want to update cache, should not be here .

Yes, l need to update podgroup to apiserver.

JesseStutler · 2024-12-27T06:45:08Z

pkg/scheduler/actions/allocate/allocate.go

@@ -245,6 +247,14 @@ func (alloc *Action) allocateResourceForTasksWithTopology(tasks *util.PriorityQu
 			break
 		}
 		for _, hyperNodeName := range ssn.HyperNodesListByTier[tier] {
+			if jobHyperNode == "" {
+				// job first scheduler


-> //job is scheduled for the first time

JesseStutler · 2025-02-10T02:12:34Z

pkg/scheduler/plugins/networktopologyaware/networktopologyaware.go

+		return nodeScores, nil
+	}
+
+	ssn.AddHyperNodeOrederFn(nta.Name(), hyperNodeFn)


AddHyperNodeOrederFn has a typo, it should be AddHyperNodeOrderFn, could you also help to fix it?

I'm glad to do it，done.

JesseStutler · 2025-02-10T02:46:16Z

pkg/scheduler/framework/session.go

@@ -202,6 +204,7 @@ func openSession(cache cache.Cache) *Session {
 	ssn.NodeList = util.GetNodeList(snapshot.Nodes, snapshot.NodeList)
 	ssn.HyperNodes = snapshot.HyperNodes
 	ssn.HyperNodesSetByTier = snapshot.HyperNodesSetByTier
+	parseHyperNodesTiers(ssn)


Would it be better to set parseHyperNodesTiers as one of the session's methods? This seems to be semantically consistent, like ssn.parseHyperNodesTiers()

I agree，done

JesseStutler · 2025-02-10T02:53:55Z

pkg/scheduler/plugins/networktopologyaware/networktopologyaware.go

+		klog.V(5).Infof("Leaving networkTopologyAware plugin ...")
+	}()
+
+	weight := calculateWeight(nta.pluginArguments)


Better to set weight in New method of networktopologyawareplugin, and set weight as one of the fields of networktopologyawareplugin, just as the binpack plugin did:

volcano/pkg/scheduler/plugins/binpack/binpack.go

Lines 89 to 92 in bff75bb

func New(aruguments framework.Arguments) framework.Plugin {

weight := calculateWeight(aruguments)

return &binpackPlugin{weight: weight}

}

Good idea, done.

JesseStutler · 2025-02-10T03:31:14Z

pkg/scheduler/plugins/networktopologyaware/networktopologyaware.go

+	return taskNumScore
+}
+
+func scoreHyperNodeWithTier(tier int, minTier int, maxTier int) float64 {


Why not take this func as one of networkTopologyAwarePlugin's methods?

good idea, I have done it

JesseStutler · 2025-02-10T03:35:39Z

pkg/scheduler/plugins/networktopologyaware/networktopologyaware.go

+	return PluginName
+}
+
+func calculateWeight(args framework.Arguments) int {


Please add an example conf to show users what does the weight means and how to configure the weight, just as binpack did:

volcano/pkg/scheduler/plugins/binpack/binpack.go

Lines 95 to 110 in bff75bb

/*

User Should give priorityWeight in this format(binpack.weight, binpack.cpu, binpack.memory).

Support change the weight about cpu, memory and additional resource by arguments.

actions: "enqueue, reclaim, allocate, backfill, preempt"

tiers:

- plugins:

- name: binpack

arguments:

binpack.weight: 10

binpack.cpu: 5

binpack.memory: 1

binpack.resources: nvidia.com/gpu, example.com/foo

binpack.resources.nvidia.com/gpu: 2

binpack.resources.example.com/foo: 3

*/

JesseStutler · 2025-02-10T09:03:54Z

pkg/scheduler/plugins/networktopologyaware/networktopologyaware.go

+		}
+		// Calculate score based on the number of tasks scheduled for the job when max score of hyperNode has more than one.
+		if maxScore != ZeroScore && len(scoreHyperNode[maxScore]) > 1 {
+			reScoreHyperNodes := scoreHyperNode[maxScore]


The name reScoreHyperNodes is strange, better to rename as candicates or candidateHyperNodes

JesseStutler · 2025-02-10T09:27:44Z

pkg/scheduler/plugins/networktopologyaware/networktopologyaware.go

+		return hyperNodeScores, nil
+	}
+
+	nodeFn := func(task *api.TaskInfo, nodes []*api.NodeInfo) (map[string]float64, error) {


Is this nodeFn added for soft mode? Doesn’t the hard mode count twice? The HyperNode score is counted once, and the Node score is counted again.

Yes, I have modify it. you can check again.

JesseStutler · 2025-02-10T09:30:17Z

pkg/scheduler/plugins/networktopologyaware/networktopologyaware.go

+		}
+		// The job still has remaining tasks to be scheduled, calculate score based on LCAHyperNode tier.
+		maxScore := ZeroScore
+		scoreNodes := map[float64][]string{}


same to rename scoreNodes and reScoreNodes

JesseStutler · 2025-02-10T09:46:54Z

pkg/scheduler/plugins/networktopologyaware/networktopologyaware.go

+		nodeScores := make(map[string]float64)
+		taskJob := ssn.Jobs[task.Job]
+
+		jobAllocatedHyperNode := taskJob.PodGroup.GetAnnotations()[api.JobAllocatedHyperNode]


In soft mode, we still lack the update of this annotation. For example, if a job is scheduled for the first time, then when the for loop traverses each pod in the job, if the pod has been allocated previously, then our subsequent pod scheduling should be scored based on the number of pods in the hypernode.

yes, I have modify it， you can check again.

JesseStutler · 2025-02-10T09:48:34Z

pkg/scheduler/plugins/networktopologyaware/networktopologyaware_test.go

+	}
+}
+
+func TestNetworkTopologyAwareHyperNodeScore(t *testing.T) {


Also need to add testcases of hypernode

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

Copilot

PR Overview

This PR adds a new network-topology-aware scheduling plugin with a hyperNode score callback and integrates it into the Volcano scheduler. Key changes include:

Introduction of the networktopologyaware plugin in pkg/scheduler/plugins/networktopologyaware.
Updates and additions in scheduler utilities and tests to support network topology and hyperNode operations.
Modifications in the allocation and session code to incorporate hyperNode-based scheduling and scoring.

Reviewed Changes

File	Description
pkg/scheduler/plugins/networktopologyaware/networktopologyaware.go	New plugin implementation with network topology scoring functions.
pkg/scheduler/util/scheduler_helper_test.go	New tests for hyperNode matching and task counts.
pkg/scheduler/util/scheduler_helper.go	Consolidation of hyperNode helper functions.
pkg/scheduler/actions/allocate/allocate.go	Updates to use session’s hyperNodesTiers and hyperNode selection logic.
pkg/scheduler/framework/session.go	Added parsing of hyper node tiers into the session object.
pkg/scheduler/conf/scheduler_conf.go	New configuration options for network topology.
pkg/scheduler/api/job_info.go	Added JobAllocatedHyperNode field in job annotations.
pkg/scheduler/framework/session_plugins.go	Fixed a typographical error in the hyperNode order function name.
pkg/scheduler/uthelper/helper.go	Updated test common structure to include RealNodesList.

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Copilot · 2025-03-04T02:57:12Z

pkg/scheduler/actions/allocate/allocate.go

 		bestNode, highestScore := alloc.prioritizeNodes(ssn, task, predicateNodes)
 		if bestNode == nil {
 			continue
 		}
-
+		if hyperNode == "" {


The variable 'hyperNode' is used without being declared or initialized. Please declare it before this block, for example initializing it as an empty string.

The 'hyperNode' is a function parameter.

Monokaix · 2025-03-10T09:56:31Z

pkg/scheduler/framework/session.go

@@ -220,6 +223,20 @@ func openSession(cache cache.Cache) *Session {
 	return ssn
 }

+func (ssn *Session) parseHyperNodesTiers() {


Why move to ssn?

Monokaix · 2025-03-10T09:56:41Z

pkg/scheduler/plugins/networktopologyaware/networktopologyaware.go

@@ -0,0 +1,224 @@
+/*
+Copyright 2019 The Volcano Authors.


Monokaix · 2025-03-10T09:57:38Z

pkg/scheduler/plugins/networktopologyaware/networktopologyaware.go

@@ -0,0 +1,224 @@
+/*


network-topology-aware is better.

volcano-sh-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 16, 2024

volcano-sh-bot requested review from hudson741 and Thor-wl December 16, 2024 03:16

volcano-sh-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 16, 2024

ecosysbin changed the title ~~[WIP]Add network-topology-aware plugin and hyperNode score callback #3885~~ Add network-topology-aware plugin and hyperNode score callback #3885 Dec 16, 2024

volcano-sh-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 16, 2024

volcano-sh-bot assigned Monokaix Dec 16, 2024

ecosysbin force-pushed the network-topology branch 2 times, most recently from 2f15078 to 947107e Compare December 16, 2024 11:41

JesseStutler reviewed Dec 17, 2024

View reviewed changes

volcano-sh-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 18, 2024

ecosysbin changed the title ~~Add network-topology-aware plugin and hyperNode score callback #3885~~ Add network-topology-aware plugin and hyperNode score callback Dec 18, 2024

ecosysbin force-pushed the network-topology branch 6 times, most recently from d1da751 to 414e547 Compare December 25, 2024 02:40

volcano-sh-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 27, 2024

ecosysbin force-pushed the network-topology branch from 414e547 to 5036976 Compare December 27, 2024 05:12

volcano-sh-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 27, 2024

ecosysbin force-pushed the network-topology branch 2 times, most recently from 889afed to b9a291b Compare December 27, 2024 06:49

JesseStutler reviewed Dec 27, 2024

View reviewed changes

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

ecosysbin force-pushed the network-topology branch from 2e8f63b to a82f026 Compare January 17, 2025 07:10

volcano-sh-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 17, 2025

ecosysbin force-pushed the network-topology branch 2 times, most recently from 2f44b62 to 325f845 Compare January 20, 2025 00:26

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

ecosysbin force-pushed the network-topology branch 5 times, most recently from c9c6f67 to 6cd178c Compare January 20, 2025 05:11

ecosysbin force-pushed the network-topology branch from 6cd178c to 2e934b2 Compare February 1, 2025 04:04

Monokaix added this to the v1.12 milestone Feb 5, 2025

JesseStutler reviewed Feb 10, 2025

View reviewed changes

ecosysbin force-pushed the network-topology branch 6 times, most recently from c0711fc to 2fa0ab7 Compare February 14, 2025 03:16

Add network-topology-aware plugin and hyperNode score callback

d1b5a92

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

ecosysbin force-pushed the network-topology branch from 2fa0ab7 to d1b5a92 Compare February 15, 2025 03:57

Monokaix requested a review from Copilot March 4, 2025 02:55

Copilot AI reviewed Mar 4, 2025

View reviewed changes

Monokaix reviewed Mar 10, 2025

View reviewed changes

pkg/scheduler/plugins/networktopologyaware/networktopologyaware.go

@@ -0,0 +1,224 @@

/*

Copy link

Member

Monokaix Mar 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

network-topology-aware is better.

	func New(aruguments framework.Arguments) framework.Plugin {
	weight := calculateWeight(aruguments)
	return &binpackPlugin{weight: weight}
	}

	/*
	User Should give priorityWeight in this format(binpack.weight, binpack.cpu, binpack.memory).
	Support change the weight about cpu, memory and additional resource by arguments.

	actions: "enqueue, reclaim, allocate, backfill, preempt"
	tiers:
	- plugins:
	- name: binpack
	arguments:
	binpack.weight: 10
	binpack.cpu: 5
	binpack.memory: 1
	binpack.resources: nvidia.com/gpu, example.com/foo
	binpack.resources.nvidia.com/gpu: 2
	binpack.resources.example.com/foo: 3
	*/

Add network-topology-aware plugin and hyperNode score callback #3894

Are you sure you want to change the base?

Add network-topology-aware plugin and hyperNode score callback #3894

Conversation

ecosysbin commented Dec 16, 2024 • edited Loading

volcano-sh-bot commented Dec 16, 2024

ecosysbin commented Dec 16, 2024

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

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

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

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

Copilot AI left a comment

Choose a reason for hiding this comment

PR Overview

Reviewed Changes

Copilot AI Mar 4, 2025

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

ecosysbin commented Dec 16, 2024 •

edited

Loading