HyperNode supports select Nodes By labels #4068

ecosysbin · 2025-03-08T15:13:32Z

What this PR does / why we need it:

HyperNode supports select Nodes By labels.

Which issue(s) this PR fixes:
#4007

ecosysbin · 2025-03-08T15:14:08Z

JesseStutler · 2025-03-17T02:01:31Z

go.mod

 	sigs.k8s.io/yaml v1.4.0
 	stathat.com/c/consistent v1.0.0
-	volcano.sh/apis v1.11.0-network-topology-preview.0
+	volcano.sh/apis v0.0.0-20250306023628-7264f8fe811c


This version number looks a bit strange. It shouldn't start with v0.0.0. At least should start with v1.11.0-network-topology-preview, maybe ask @Monokaix how to generate it

JesseStutler · 2025-03-17T02:34:44Z

pkg/scheduler/api/hyper_node_info.go

+	if selector.LabelMatch != nil {
+		labelSelector, err := metav1.LabelSelectorAsSelector(selector.LabelMatch)
+		if err != nil {
+			klog.ErrorS(err, "Failed to construct labelSelector as labelMatch", "LabelMatch", selector.LabelMatch)


There is a problem with the order of subject and object here. It's "Failed to convert labelMatch to labelSelector"

JesseStutler · 2025-03-17T02:49:15Z

pkg/webhooks/admission/hypernodes/validate/admit_hypernode.go

-	if selector.RegexMatch == nil && selector.ExactMatch == nil {
-		err := field.Invalid(fldPath, selector,
-			"member selector must have one of regexMatch or exactMatch")
+	if memberType != hypernodev1alpha1.MemberTypeHyperNode && memberType != hypernodev1alpha1.MemberTypeNode {


It is best to extract a function that validate memberType separately, otherwise the responsibility of the func validateHyperNodeMemberSelector does not match

JesseStutler · 2025-03-17T02:51:28Z

pkg/webhooks/admission/hypernodes/validate/admit_hypernode.go

-			"member selector cannot have both regexMatch and exactMatch")
-		errs = append(errs, err)
-		return errs
+	if memberType == hypernodev1alpha1.MemberTypeHyperNode {


The validation using kubebuilder is not consistent with here. MemberType is also verified here. I think we should keep consistent? @Monokaix

Ok, I have delete it. Just ignore it when the MemberType is invalid.

I mean, do we need to distinguish MemberTypeHyperNode from MemberTypeNode? Currently, our kubebuilder rules do not distinguish between them. We should keep consistent

I looked at the cel validation in kubebuilder and it also supports referencing other fields. Perhaps if type validation is needed here, when memberType is HyperNode, it can only use ExactMatch. You should also update in apis repo to distinguish memberType is HyperNode and Node to keep consistent

JesseStutler · 2025-03-17T03:15:06Z

pkg/webhooks/admission/hypernodes/validate/admit_hypernode.go

+			return errs
+		}
+	} else {
+		if (selector.LabelMatch != nil && (selector.RegexMatch != nil || selector.ExactMatch != nil)) ||


This is a bit too complext to understand, making it difficult for others to read. Esstentially, we want to verify that there can not be two or more matches. Perhaps we can use a counting method to do the same thing.

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

JesseStutler · 2025-03-17T03:27:30Z

pkg/webhooks/admission/hypernodes/validate/admit_hypernode.go

 		}
 	}
+	if selector.LabelMatch != nil {
+		if (selector.LabelMatch.MatchExpressions == nil || len(selector.LabelMatch.MatchExpressions) == 1) &&


What are these codes for? If matchExpressions and matchLabels are both be empty, it means matching all nodes. And we didn't add validation in kubebuilder validation format, I don't think we need these codes

We can skip the verification here. Not configuring the label matching rule will match all Nodes, which seems really strange. Wouldn't it be more appropriate to match the empty nodes? @Monokaix , what's your opinion?

JesseStutler · 2025-03-17T03:30:09Z

Please fix the CI :), and I think we should also add tese cases for the new webhook validation codes.

JesseStutler · 2025-03-17T04:20:11Z

Also need to execute make generate-yaml to update CRD yaml

JesseStutler · 2025-03-17T04:22:25Z

Besides, what if node label updates? We need to handle these cases

JesseStutler · 2025-04-07T07:50:42Z

volcano.sh/apis/pkg/client/batch/v1alpha1/job.go

Why do we generate so many new files, shouldn't only config/crd/volcano/bases/topology.volcano.sh_hypernodes.yaml change?

I have modified it, you can check again.

JesseStutler · 2025-04-07T07:58:05Z

pkg/webhooks/admission/hypernodes/validate/admit_hypernode.go

Also add test cases into admit_hypernode_test.go

ecosysbin · 2025-04-17T01:07:47Z

And UpdateNode should also add the update event.

Done

JesseStutler · 2025-04-17T02:04:56Z

pkg/scheduler/api/hyper_node_info.go


-// NodeRegexMatchLeafHyperNode checks if a given node regex matches the MemberSelector of a HyperNode.
-func (hni *HyperNodesInfo) NodeRegexMatchLeafHyperNode(nodeName string, hyperNodeName string) (bool, error) {
+// NodeMatchLeafHyperNode checks if a given node regex matches the MemberSelector of a HyperNode.


Comments should also be adapted and modified

JesseStutler · 2025-04-17T02:05:18Z

pkg/scheduler/api/hyper_node_info.go

 	return false, nil
 }

 // nodeMatchRegexSelector checks if a node matches a MemberSelector.


Comments should also be adapted and modified

JesseStutler · 2025-04-17T02:18:14Z

pkg/scheduler/cache/event_handlers.go

 		return
 	}
 	sc.nodeQueue.Add(newNode.Name)
+	if len(oldNode.GetLabels()) != len(newNode.GetLabels()) {


I think use reflect.DeepEqual is better?

if !reflect.DeepEqual(oldNode.GetLabels(), newNode.GetLabels()) { sc.hyperNodesQueue.Add(string(hyperNodeEventSourceNode) + "/" + newNode.Name) }

Good idea, Done

JesseStutler · 2025-04-17T02:23:15Z

pkg/scheduler/api/hyper_node_info_test.go

 		assert.NoError(t, err)
 	}
+	for _, node := range initialNodes {
+		// err := nodeInformer.Informer().GetIndexer().Add(node)


delete useless comment

JesseStutler · 2025-04-17T02:25:46Z

Please make CI happy :)

JesseStutler · 2025-04-17T08:34:49Z

/lgtm

cc @Monokaix

Monokaix · 2025-04-18T01:47:37Z

pkg/scheduler/cache/event_handlers.go

 			continue
 		}
-		match, err := sc.HyperNodesInfo.NodeRegexMatchLeafHyperNode(name, hn.Name)
+		match, err := sc.HyperNodesInfo.NodeMatchLeafHyperNode(name, hn.Name)


line 726 GetRegexSelectorLeafHyperNodes should also consider label selector.

Monokaix · 2025-04-18T01:51:19Z

pkg/scheduler/api/hyper_node_info.go

-// NodeRegexMatchLeafHyperNode checks if a given node regex matches the MemberSelector of a HyperNode.
-func (hni *HyperNodesInfo) NodeRegexMatchLeafHyperNode(nodeName string, hyperNodeName string) (bool, error) {
+// NodeMatchLeafHyperNode checks if a given node matches the MemberSelector of a HyperNode.
+func (hni *HyperNodesInfo) NodeMatchLeafHyperNode(nodeName string, hyperNodeName string) (bool, error) {


Suggested change

func (hni *HyperNodesInfo) NodeMatchLeafHyperNode(nodeName string, hyperNodeName string) (bool, error) {

func (hni *HyperNodesInfo) NodeRegexOrSelectorMatchLeafHyperNode(nodeName string, hyperNodeName string) (bool, error) {

Copilot

Pull Request Overview

This PR introduces support for selecting Nodes by labels in addition to the previously supported regex-based selection. The changes update the MemberConfig and BuildHyperNode APIs, adjust node matching logic in HyperNodesInfo, and update related tests and utilities accordingly.

Updated test files to use the new BuildHyperNode signature with an extra LabelSelector parameter.
Modified node update logic to trigger hyper-node events upon label changes.
Refactored node matching functions to support both regex‐ and label-based matching.

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
pkg/scheduler/cache/event_handlers_test.go	Updated BuildHyperNode calls to pass nil for the new LabelSelector parameter.
pkg/scheduler/cache/event_handlers.go	Extended UpdateNode to check for label changes and updated node matching to use the new NodeMatchLeafHyperNode function.
pkg/scheduler/api/test_utils_test.go	Adjusted BuildHyperNode calls to include the new nil parameter.
pkg/scheduler/api/test_utils.go	Modified BuildNode and BuildHyperNode to support a LabelSelector and added the "label" case.
pkg/scheduler/api/node_info_test.go	Updated BuildNode calls to account for the new BuildNode signature.
pkg/scheduler/api/hyper_node_info.go	Updated node matching logic to support label selectors and renamed the node matching function accordingly.

Monokaix · 2025-04-18T03:18:17Z

/approve

volcano-sh-bot · 2025-04-18T03:18:25Z

[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

Signed-off-by: wangbin <[email protected]>

JesseStutler · 2025-04-18T07:22:04Z

/lgtm

MondayCha · 2025-05-26T07:58:17Z

In the finalized merged code, I noticed that the pkg/webhooks/admission/hypernodes/validate/admit_hypernode.go file remains unmodified. Consequently, in the current Preview 3 version of Network Topology, the LabelSelector cannot be used for node selection.

Is this behavior intentional? If not, I would be glad to submit a PR to update the admission webhook.

Monokaix · 2025-05-26T08:03:34Z

In the finalized merged code, I noticed that the pkg/webhooks/admission/hypernodes/validate/admit_hypernode.go file remains unmodified. Consequently, in the current Preview 3 version of Network Topology, the LabelSelector cannot be used for node selection.

Is this behavior intentional? If not, I would be glad to submit a PR to update the admission webhook.

Yeah there is no check in webhook for label selector, but the scheduler should work well.

MondayCha · 2025-05-26T08:21:21Z

Hi, when we attempting to create a HyperNode using a LabelSelector, the admission webhook returns a validation error indicating that either regexMatch or exactMatch must be provided.

We are currently using the Helm chart version:

volcano-1.11.0-network-topology-preview.3

Example YAML:

apiVersion: hypernode.volcano.sh/v1alpha1
kind: HyperNode
metadata:
  name: example-hypernode
spec:
  members:
    selector:
      labelMatch:
        matchLabels:
          node-group: workers

Admission webhook denies the request with the following error:

admission webhook "validatehypernodes.volcano.sh" denied the request: 
[].spec.members.selector: Invalid value: ...: member selector must have one of regexMatch or exactMatch

Looking into the webhook validation logic, it appears that support for labelMatch is not yet implemented or checked:

func validateHyperNodeMemberSelector(selector hypernodev1alpha1.MemberSelector, fldPath *field.Path) field.ErrorList {
	errs := field.ErrorList{}

	if selector.RegexMatch == nil && selector.ExactMatch == nil {
		err := field.Invalid(fldPath, selector,
			"member selector must have one of regexMatch or exactMatch")
		errs = append(errs, err)
		return errs
	}
}

This validation does not account for the presence of LabelMatch, even though the schema allows it.

Monokaix · 2025-05-26T08:25:50Z

Hi, when we attempting to create a HyperNode using a LabelSelector, the admission webhook returns a validation error indicating that either regexMatch or exactMatch must be provided.

We are currently using the Helm chart version:
volcano-1.11.0-network-topology-preview.3
Example YAML:
apiVersion: hypernode.volcano.sh/v1alpha1
kind: HyperNode
metadata:
  name: example-hypernode
spec:
  members:
    selector:
      labelMatch:
        matchLabels:
          node-group: workers
Admission webhook denies the request with the following error:
admission webhook "validatehypernodes.volcano.sh" denied the request: 
[].spec.members.selector: Invalid value: ...: member selector must have one of regexMatch or exactMatch
Looking into the webhook validation logic, it appears that support for labelMatch is not yet implemented or checked:
func validateHyperNodeMemberSelector(selector hypernodev1alpha1.MemberSelector, fldPath *field.Path) field.ErrorList {
	errs := field.ErrorList{}

	if selector.RegexMatch == nil && selector.ExactMatch == nil {
		err := field.Invalid(fldPath, selector,
			"member selector must have one of regexMatch or exactMatch")
		errs = append(errs, err)
		return errs
	}
}
This validation does not account for the presence of LabelMatch, even though the schema allows it.

Please feel free to fix it.

volcano-sh-bot requested review from Thor-wl and merryzhou March 8, 2025 15:13

volcano-sh-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 8, 2025

volcano-sh-bot assigned Monokaix Mar 8, 2025

ecosysbin force-pushed the rescheduler branch from 31fef6b to b7385e5 Compare March 8, 2025 15:56

volcano-sh-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 8, 2025

ecosysbin force-pushed the rescheduler branch 3 times, most recently from ab01534 to e5489c1 Compare March 10, 2025 15:38

JesseStutler reviewed Mar 17, 2025

View reviewed changes

ecosysbin force-pushed the rescheduler branch 2 times, most recently from 4707244 to 8a2319a Compare March 17, 2025 09:51

ecosysbin force-pushed the rescheduler branch from 8a2319a to 89730fa Compare March 31, 2025 01:39

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 Mar 31, 2025

ecosysbin force-pushed the rescheduler branch from 89730fa to 6ef898b Compare April 1, 2025 02:41

JesseStutler reviewed Apr 7, 2025

View reviewed changes

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

ecosysbin force-pushed the rescheduler branch from 6ef898b to c6f938c Compare April 12, 2025 14:10

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

ecosysbin force-pushed the rescheduler branch from c6f938c to 6ef898b Compare April 15, 2025 01:20

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 Apr 15, 2025

ecosysbin closed this Apr 17, 2025

ecosysbin reopened this Apr 17, 2025

JesseStutler reviewed Apr 17, 2025

View reviewed changes

ecosysbin force-pushed the rescheduler branch 3 times, most recently from abe1d3c to f5059fa Compare April 17, 2025 08:04

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

Monokaix reviewed Apr 18, 2025

View reviewed changes

Monokaix requested a review from Copilot April 18, 2025 01:53

Copilot AI reviewed Apr 18, 2025

View reviewed changes

ecosysbin force-pushed the rescheduler branch from f5059fa to f64ca30 Compare April 18, 2025 02:13

volcano-sh-bot removed the lgtm Indicates that a PR is ready to be merged. label Apr 18, 2025

ecosysbin force-pushed the rescheduler branch 2 times, most recently from 6776c4f to daa3d0d Compare April 18, 2025 02:32

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

HyperNode supports select Nodes By labels

6dac24f

Signed-off-by: wangbin <[email protected]>

ecosysbin force-pushed the rescheduler branch from daa3d0d to 6dac24f Compare April 18, 2025 03:25

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

volcano-sh-bot merged commit 074478f into volcano-sh:network-topology Apr 18, 2025
10 of 11 checks passed

	func (hni *HyperNodesInfo) NodeMatchLeafHyperNode(nodeName string, hyperNodeName string) (bool, error) {
	func (hni *HyperNodesInfo) NodeRegexOrSelectorMatchLeafHyperNode(nodeName string, hyperNodeName string) (bool, error) {

HyperNode supports select Nodes By labels #4068

HyperNode supports select Nodes By labels #4068

Uh oh!

Conversation

ecosysbin commented Mar 8, 2025

Uh oh!

ecosysbin commented Mar 8, 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!

JesseStutler Apr 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

JesseStutler commented Mar 17, 2025

Uh oh!

JesseStutler commented Mar 17, 2025

Uh oh!

JesseStutler commented Mar 17, 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!

ecosysbin commented Apr 17, 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!

JesseStutler commented Apr 17, 2025

Uh oh!

JesseStutler commented Apr 17, 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!

Copilot AI left a comment

Choose a reason for hiding this comment

JesseStutler Apr 7, 2025 •

edited

Loading