add nodegroup plugin #3132

wuyueandrew · 2023-09-20T03:10:58Z

#3131 add nodegroup plugin with ut

volcano-sh-bot · 2023-09-20T03:11:01Z

Welcome @wuyueandrew!

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

Thank you, and welcome to Volcano. 😃

wuyueandrew · 2023-09-25T02:50:27Z

/assign @Thor-wl

william-wang · 2023-09-25T05:07:24Z

Thanks for your contribution and testing. We will review it as soon as possible.

wuyueandrew · 2023-09-26T11:29:18Z

Thanks for your contribution and testing. We will review it as soon as possible.

lint err fixed plz restart ci pipeline

Monokaix · 2023-10-07T03:14:28Z

Thanks for your contribution! Please add a user-guide for this plugin refer to #3149.

wuyueandrew · 2023-10-08T02:28:02Z

Thanks for your contribution! Please add a user-guide for this plugin refer to #3149.
Doc of node-group was added before, link:
https://github.com/volcano-sh/volcano/blob/master/docs/design/node-group.md

Monokaix · 2023-10-25T02:48:56Z

Hi, please take a look at review and squash your commits : )

william-wang · 2023-10-27T06:09:03Z

pkg/scheduler/plugins/nodegroup/nodegroup.go

@@ -0,0 +1,229 @@
+/*
+Copyright 2019 The Kubernetes Authors.


please update the copyright to be the right one.

Monokaix · 2023-10-07T02:10:37Z

pkg/scheduler/plugins/nodegroup/nodegroup.go

@@ -0,0 +1,229 @@
+/*
+Copyright 2019 The Kubernetes Authors.


please change 2019 to 2023.

Monokaix · 2023-10-07T03:40:58Z

pkg/scheduler/plugins/nodegroup/nodegroup.go

+//    - name: predicates
+//    - name: proportion
+//    - name: nodegroup
+// 		enablePredicate: true


This arg enablePredicate seems not used but exists in example.

i will remove this argument

Monokaix · 2023-10-07T06:47:20Z

pkg/scheduler/plugins/nodegroup/nodegroup.go

+		queueGroupAffinityPreferred:     make(map[string][]string, 0),
+	}
+}
+func (q queueGroupAffinity) predicate(queue, group string) error {


If node has no nodeGroupLabel, predicate will not fit, is this expected?

consider of our scenery, nodeGroupLabel is strict required. if there is a node without nodeGroupLabel, pod cannot assigned to this node.

Monokaix · 2023-10-07T07:15:29Z

pkg/scheduler/plugins/nodegroup/nodegroup.go

+	}
+	if q.queueGroupAffinityPreferred != nil {
+		if groups, ok := q.queueGroupAffinityPreferred[queue]; ok {
+			if !contains(groups, group) {


Here should be contains.

Monokaix · 2023-10-07T07:16:22Z

pkg/scheduler/plugins/nodegroup/nodegroup.go

+	if q.queueGroupAffinityPreferred != nil {
+		if groups, ok := q.queueGroupAffinityPreferred[queue]; ok {
+			if !contains(groups, group) {
+				return 1.0


AntiAffinity and Affinity should be both considered when scoring, and compute a weighted value finally.

Monokaix · 2023-10-07T07:19:24Z

pkg/scheduler/plugins/nodegroup/nodegroup.go

+	return &nodeGroupPlugin{pluginArguments: arguments}
+}
+
+func (pp *nodeGroupPlugin) Name() string {


Better rename this to np or something to be consistent with other receiver.

np is better

Monokaix · 2023-10-07T07:32:40Z

pkg/scheduler/plugins/nodegroup/nodegroup_test.go

+	"volcano.sh/volcano/pkg/scheduler/util"
+)
+
+func TestScore(t *testing.T) {


This ut tests score function, but it seems only predicate is tested because there is only RequiredDuringSchedulingIgnoredDuringExecution item in queue.spec. Please test both score and predict function.

william-wang · 2023-11-03T06:15:41Z

@wuyueandrew Please handle the confliction in the PR and the ci failure:)

Monokaix · 2023-12-14T02:06:29Z

pkg/scheduler/plugins/nodegroup/nodegroup.go

+	queueGroupAffinity := NewQueueGroupAffinity()
+	for _, queue := range ssn.Queues {
+		affinity := queue.Queue.Spec.Affinity
+		if nil == affinity {


To uniform code style, please move nil to front.

Monokaix · 2023-12-14T02:11:13Z

pkg/scheduler/plugins/nodegroup/nodegroup.go

+}
+
+type queueGroupAffinity struct {
+	queueGroupAntiAffinityRequired  map[string][]string


value is a array type is not so good, if user mis-config a lots of duplicated label, it's unacceptable, and sets is better, and it can de-duplicate and is more efficient.

Monokaix · 2023-12-14T02:19:08Z

pkg/scheduler/plugins/nodegroup/nodegroup.go

+	if q.queueGroupAffinityRequired != nil {
+		if groups, ok := q.queueGroupAffinityRequired[queue]; ok {
+			if contains(groups, group) {
+				flag = flag || true


|| is necessary here, and preferred items should not be considered in predict.

Monokaix · 2023-12-14T02:41:09Z

pkg/scheduler/plugins/nodegroup/nodegroup.go

+		}
+	}
+
+	if q.queueGroupAntiAffinityRequired != nil {


AntiAffinityRequired should be filtered in predict, so it should not exists in score

Monokaix · 2023-12-14T02:41:22Z

pkg/scheduler/plugins/nodegroup/nodegroup.go

+	if q.queueGroupAntiAffinityPreferred != nil {
+		if groups, ok := q.queueGroupAntiAffinityPreferred[queue]; ok {
+			if contains(groups, group) {
+				nodeScore = 0.25


This value should be -1

Monokaix · 2023-12-18T11:19:03Z

pkg/scheduler/plugins/nodegroup/nodegroup.go

+
+func NewQueueGroupAffinity() queueGroupAffinity {
+	return queueGroupAffinity{
+		queueGroupAntiAffinityRequired:  make(map[string]sets.Set[string], 0),


0 can be omitted.

Monokaix · 2023-12-20T07:47:24Z

pkg/scheduler/plugins/nodegroup/nodegroup.go

+	if q.queueGroupAffinityPreferred != nil {
+		if groups, ok := q.queueGroupAffinityPreferred[queue]; ok {
+			if groups.Has(group) {
+				nodeScore = 100


Set a base constant value 100.

Monokaix · 2023-12-20T07:48:41Z

pkg/scheduler/plugins/nodegroup/nodegroup.go

+		}
+	}
+	if q.queueGroupAffinityPreferred != nil {
+		if groups, ok := q.queueGroupAffinityPreferred[queue]; ok {


Prefer node get higher score than required?

Monokaix · 2023-12-21T03:19:36Z

docs/design/node-group.md

@@ -39,7 +39,7 @@ case2: recommend queue can use private cloud nodes or public cloud nodes, but tt

 affinity configure:
 1. affinity.nodeGroupAffinity.requiredDuringSchedulingIgnoredDuringExecution, hard constraints, such as `nlp = nodegroup1,nodegroup2`, it means that task in queue=nlp can ony run on the nodes in nodegroup1 or nodegroup2.
-2. affinity.nodeGroupAffinity.preferredDuringSchedulingIgnoredDuringExecution, soft constraints, such as `nlp = nodegroup1`, it means that task in queue=nlp runs on nodegroup1 first, but if the resources of nodegroup1 is insufficient, it can also run on other nodegroups.
+2. affinity.nodeGroupAffinity.preferredDuringSchedulingIgnoredDuringExecution, soft constraints, such as `nlp = nodegroup1`, it means that task in queue=nlp runs on other nodegroups first, but if the resources is insufficient, it can also run on nodegroup1.


other nodegroups is misunderstanding, other nodesgroups specified in affinity.nodeGroupAffinity.requiredDuringSchedulingIgnoredDuringExecution is better.

Monokaix · 2023-12-21T03:20:59Z

docs/user-guide/how_to_use_nodegroup_plugin.md

+
+## Introduction
+
+**Nodegroup plugin** is designed to isolate resources by assigning labels to nodes. 


should also add: and set node label affinty on Queue.

Monokaix · 2023-12-21T03:21:31Z

docs/user-guide/how_to_use_nodegroup_plugin.md

+           preferredDuringSchedulingIgnoredDuringExecution:
+           - groupname3
+   ```
+   This implies that tasks in the "default" queue will be executed on "groupname1" and "groupname2", with a preference for "groupname1" to run first. Tasks are restricted from running on "groupname3" and "groupname4". However, if the resources in other node groups are insufficient, the task can run on "nodegroup3".


executed -> scheduled

Monokaix · 2023-12-21T03:24:46Z

docs/user-guide/how_to_use_nodegroup_plugin.md

+
+The nodegroup design document provides the most detailed information about the node group. There are some tips to help avoid certain issues.
+
+1. Soft constraints are a subset of hard constraints, including both affinity and anti-affinity. Consider a queue setup as follows:


The following steps are both about queue's configuration, should also add other steps, like set node lable, and submit jobs to a queue and verfiy that jobs are scheduled to affinity nodes and schedule failed on antiaffinity nodes.

Monokaix · 2023-12-21T03:26:34Z

pkg/scheduler/plugins/nodegroup/nodegroup_test.go

+						}
+
+						status, _ := ssn.PredicateFn(task, node)
+						// if err != nil {


Please remove commented codes.

Monokaix · 2023-12-21T04:31:34Z

pkg/scheduler/plugins/nodegroup/nodegroup_test.go

+	"volcano.sh/volcano/pkg/scheduler/util"
+)
+
+func TestNormal(t *testing.T) {


Please replace it with a more descriptive name.

Monokaix · 2023-12-21T04:31:55Z

pkg/scheduler/plugins/nodegroup/nodegroup_test.go

+		expectedStatus map[string]map[string]int
+	}{
+		{
+			name: "normal queue test",


Same above.

Monokaix · 2023-12-21T04:32:11Z

pkg/scheduler/plugins/nodegroup/nodegroup_test.go

+			},
+		},
+		{
+			name: "special queue job",


Same above.

Monokaix · 2023-12-27T07:42:55Z

docs/user-guide/how_to_use_nodegroup_plugin.md

+
+### validate queue affinity and antiAffinity rules is effected
+
+Query pod information and verify whether the pod has been assigned to the correct node.


Please illustrate which node is correct and which node is filtered.

Monokaix · 2023-12-28T01:30:25Z

pkg/scheduler/plugins/nodegroup/nodegroup.go

+		group := node.Node.Labels[NodeGroupNameKey]
+		queue := GetPodQueue(task)
+		score := queueGroupAffinity.score(queue, group)
+		klog.V(3).Infof("task <%s>/<%s> queue %s on node %s of nodegroup %s, score %v", task.Namespace, task.Name, queue, node.Name, group, score)


Signed-off-by: wuyue <wuyue_andrew@outlook.com>

how to use nodegroup doc how to use nodegroup doc Signed-off-by: wuyue <wuyue_andrew@outlook.com> how to use nodegroup doc how to use nodegroup doc Signed-off-by: wuyue <wuyue_andrew@outlook.com> how to use nodegroup doc Signed-off-by: wuyue <wuyue_andrew@outlook.com> how to use nodegroup doc Signed-off-by: wuyue <wuyue_andrew@outlook.com>

william-wang

/approve

volcano-sh-bot · 2024-01-17T11:03:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: william-wang

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~~ [william-wang]

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

volcano-sh-bot requested review from hudson741 and Thor-wl September 20, 2023 03:11

volcano-sh-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 20, 2023

wuyueandrew force-pushed the nodegroup-issue3131 branch from 5651c3f to 366e9cf Compare September 20, 2023 03:16

volcano-sh-bot assigned Thor-wl Sep 25, 2023

william-wang reviewed Oct 27, 2023

View reviewed changes

Monokaix reviewed Nov 3, 2023

View reviewed changes

wuyueandrew force-pushed the nodegroup-issue3131 branch from f4681c3 to f161062 Compare November 8, 2023 06:14

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 Nov 8, 2023

wuyueandrew force-pushed the nodegroup-issue3131 branch 2 times, most recently from b8c5755 to 7635571 Compare November 8, 2023 06:29

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 Nov 8, 2023

wuyueandrew force-pushed the nodegroup-issue3131 branch from 5b5c450 to 51320d2 Compare November 8, 2023 06:44

william-wang mentioned this pull request Nov 15, 2023

support nodegroup as a plugin #2352

Closed

Monokaix reviewed Dec 14, 2023

View reviewed changes

Monokaix reviewed Dec 18, 2023

View reviewed changes

Monokaix reviewed Dec 20, 2023

View reviewed changes

wuyueandrew force-pushed the nodegroup-issue3131 branch from 090bbc3 to 18123be Compare December 20, 2023 09:24

Monokaix reviewed Dec 21, 2023

View reviewed changes

pkg/scheduler/plugins/nodegroup/nodegroup_test.go Outdated

},

},

{

name: "special queue job",

Copy link

Member

Monokaix Dec 21, 2023

Choose a reason for hiding this comment

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

Same above.

wuyueandrew force-pushed the nodegroup-issue3131 branch 4 times, most recently from 988b0c5 to a1e11b4 Compare December 27, 2023 06:36

Monokaix reviewed Dec 27, 2023

View reviewed changes

wuyueandrew force-pushed the nodegroup-issue3131 branch from 3c19f47 to cbaf82d Compare December 27, 2023 10:41

Monokaix reviewed Dec 28, 2023

View reviewed changes

wuyueandrew force-pushed the nodegroup-issue3131 branch 2 times, most recently from ec37291 to 89c1cbb Compare December 28, 2023 02:41

wuyueandrew added 3 commits December 28, 2023 10:42

nodegroup implement

2d293e3

Signed-off-by: wuyue <wuyue_andrew@outlook.com>

nodegroup design doc update

1a2a135

Signed-off-by: wuyue <wuyue_andrew@outlook.com>

wuyueandrew force-pushed the nodegroup-issue3131 branch from 89c1cbb to a0f257e Compare December 28, 2023 02:42

Monokaix mentioned this pull request Jan 17, 2024

Encounter "CRD exists" error while Installing multi volcano scheduler #3302

Open

william-wang added this to the v1.9 milestone Jan 17, 2024

william-wang approved these changes Jan 17, 2024

View reviewed changes

volcano-sh-bot assigned william-wang Jan 17, 2024

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

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

volcano-sh-bot merged commit 2110527 into volcano-sh:master Jan 17, 2024
13 checks passed


		## Introduction

		Nodegroup plugin is designed to isolate resources by assigning labels to nodes.


		The nodegroup design document provides the most detailed information about the node group. There are some tips to help avoid certain issues.

		1. Soft constraints are a subset of hard constraints, including both affinity and anti-affinity. Consider a queue setup as follows:


		### validate queue affinity and antiAffinity rules is effected

		Query pod information and verify whether the pod has been assigned to the correct node.

add nodegroup plugin #3132

add nodegroup plugin #3132

Conversation

wuyueandrew commented Sep 20, 2023

volcano-sh-bot commented Sep 20, 2023

wuyueandrew commented Sep 25, 2023

william-wang commented Sep 25, 2023

wuyueandrew commented Sep 26, 2023

Monokaix commented Oct 7, 2023

wuyueandrew commented Oct 8, 2023

Monokaix commented Oct 25, 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

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

william-wang commented Nov 3, 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

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

Monokaix Dec 21, 2023 • edited Loading

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

william-wang left a comment

Choose a reason for hiding this comment

volcano-sh-bot commented Jan 17, 2024

Monokaix Dec 21, 2023 •

edited

Loading