Add hyperNode controller framework and provider #4014

Monokaix · 2025-02-17T07:50:15Z

What type of PR is this?

/kind feature
/area controllers

What this PR does / why we need it:

Add hyperNode controller provider and vendors can register plugins to add/update/delete hyperNodes by auto discovering network topology of their own datacenters.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Vendors should register their own plugins and integrate with vc controller to reconcile hyperNodes

volcano-sh-bot · 2025-02-18T10:48:17Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign monokaix
You can assign the PR to them by writing /assign @monokaix 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:

OWNERS

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

pkg/controllers/hypernode/provider/provider.go

Signed-off-by: Monokaix <changxuzheng@huawei.com>

Monokaix · 2025-02-25T06:57:29Z

cc @lowang-bh @hwdef @JesseStutler

hwdef · 2025-03-03T07:40:40Z

please import copilot :)

Copilot

PR Overview

This PR introduces the hyperNode controller framework and provider to support the auto-discovery of network topology for hyperNodes, enabling vendors to seamlessly integrate their plugins with the Volcano controller.

Added a new hyperNode controller implementation with basic lifecycle methods.
Introduced a plugin-based hyperNode provider with dynamic loading and event handling.
Updated related tests, installer YAMLs, and command-line options to integrate the new framework.

Reviewed Changes

File	Description
example/hypernode-provider/README.md	Added documentation for writing and building a hyperNode provider
pkg/controllers/hypernode/hypernode_controller.go	Introduced the core hyperNode controller functions
pkg/controllers/hypernode/hypernode_controller_test.go	Added tests for the hyperNode controller run logic
example/hypernode-provider/example_provider.go	Provided an example implementation of a hyperNode provider
pkg/controllers/hypernode/provider/provider_test.go	Added unit tests covering add, update, and delete event scenarios
pkg/controllers/hypernode/provider/interface.go	Defined the provider plugin interface
pkg/controllers/hypernode/provider/provider.go	Implemented the provider loading, event handling, and retry logic
installer/volcano-development.yaml and installer/helm/chart/volcano/templates/controllers.yaml	Updated RBAC roles to include permissions for hyperNodes
pkg/controllers/framework/interface.go, cmd/controller-manager/app/options/options.go, cmd/controller-manager/main.go, cmd/controller-manager/app/server.go	Updated server options and integration to support the new provider

Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

pkg/controllers/hypernode/provider/provider.go:220

[nitpick] Function name 'handleNodeDeleted' is inconsistent with the naming of 'handleHyperNodeAdd' and 'handleHyperNodeUpdate'. Consider renaming it to 'handleHyperNodeDelete' for consistency.

func (p *provider) handleNodeDeleted(event Event) {

Copilot · 2025-03-03T07:45:13Z

pkg/controllers/hypernode/provider/provider.go

+	err := retry.OnError(
+		backoff,
+		func(err error) bool {
+			return true


In handleHyperNodeUpdate, the unconditional retry condition may lead to infinite retries on non-transient errors. Consider refining the retry condition to check for specific error types.

Suggested change

return true

return !apierrors.IsConflict(err) && !apierrors.IsInvalid(err)

Copilot · 2025-03-03T07:45:13Z

pkg/controllers/hypernode/provider/provider.go

+}
+
+func (p *provider) loadProvider(dir string) error {
+	pluginPaths, _ := filepath.Glob(fmt.Sprintf("%s/*.so", dir))


The error returned by filepath.Glob is ignored. Handling this error could prevent issues during plugin loading if globbing fails.

Suggested change

pluginPaths, _ := filepath.Glob(fmt.Sprintf("%s/*.so", dir))

pluginPaths, err := filepath.Glob(fmt.Sprintf("%s/*.so", dir))

if err != nil {

return fmt.Errorf("failed to glob plugins: %v", err)

}

hwdef

Code quality is high as always, with only a few minor issues

hwdef · 2025-03-03T07:30:46Z

example/hypernode-provider/Dockerfile

+
+# Install musl
+RUN apt-get update && \
+    apt-get install -y sudo


Is sudo necessary? Is the default user in the container root?

hwdef · 2025-03-03T07:33:18Z

example/hypernode-provider/Dockerfile

+RUN apt-get update && \
+    apt-get install -y sudo
+
+RUN wget http://musl.libc.org/releases/musl-1.2.1.tar.gz && \


Suggested change

RUN wget http://musl.libc.org/releases/musl-1.2.1.tar.gz && \

RUN wget http://musl.libc.org/releases/musl-latest.tar.gz && \

hwdef · 2025-03-03T07:55:11Z

pkg/controllers/hypernode/provider/provider.go

+}
+
+func (p *provider) loadProvider(dir string) error {
+	pluginPaths, _ := filepath.Glob(fmt.Sprintf("%s/*.so", dir))


Why ignore the error?

hwdef · 2025-03-03T07:58:05Z

pkg/controllers/hypernode/provider/provider.go

+func (p *provider) loadProvider(dir string) error {
+	pluginPaths, _ := filepath.Glob(fmt.Sprintf("%s/*.so", dir))
+	for _, pluginPath := range pluginPaths {
+		klog.InfoS("Loading provider plugin...", "", "path", pluginPath)


Infos is used in some places, will this lead to too many logs by default?

hwdef · 2025-03-03T07:58:44Z

pkg/controllers/hypernode/provider/provider.go

+			return err
+		}
+		plugin := pb()
+		//pluginName := getPluginName(pluginPath)


Delete this if not used

volcano-sh-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/feature Categorizes issue or PR as related to a new feature. labels Feb 17, 2025

volcano-sh-bot requested review from hwdef and hzxuzhonghu February 17, 2025 07:50

volcano-sh-bot added area/controllers size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 17, 2025

Monokaix force-pushed the network-topo-controller branch 2 times, most recently from e3671f8 to 5c3c66d Compare February 18, 2025 10:48

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 Feb 18, 2025

Monokaix force-pushed the network-topo-controller branch 19 times, most recently from 77df7d9 to 2896479 Compare February 20, 2025 09:04

Monokaix force-pushed the network-topo-controller branch 3 times, most recently from bb42d94 to df2089d Compare February 20, 2025 09:33

fishingfly reviewed Feb 20, 2025

View reviewed changes

pkg/controllers/hypernode/provider/provider.go Outdated Show resolved Hide resolved

Monokaix force-pushed the network-topo-controller branch 3 times, most recently from faa2068 to d2fd538 Compare February 21, 2025 06:15

Monokaix changed the title ~~[WIP]Add hyperNode controller framework and provider~~ Add hyperNode controller framework and provider Feb 21, 2025

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 Feb 21, 2025

Add hyperNode controller framework and provider

59d50f6

Signed-off-by: Monokaix <changxuzheng@huawei.com>

Monokaix force-pushed the network-topo-controller branch from d2fd538 to 59d50f6 Compare February 21, 2025 06:23

Monokaix requested a review from Copilot March 3, 2025 07:43

Copilot AI reviewed Mar 3, 2025

View reviewed changes

hwdef reviewed Mar 3, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add hyperNode controller framework and provider #4014

Add hyperNode controller framework and provider #4014

Monokaix commented Feb 17, 2025

volcano-sh-bot commented Feb 18, 2025

Monokaix commented Feb 25, 2025

hwdef commented Mar 3, 2025

Copilot AI left a comment

Copilot AI Mar 3, 2025

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Copilot AI Mar 3, 2025

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

hwdef left a comment

hwdef Mar 3, 2025

hwdef Mar 3, 2025

hwdef Mar 3, 2025

hwdef Mar 3, 2025

hwdef Mar 3, 2025

	return true
	return !apierrors.IsConflict(err) && !apierrors.IsInvalid(err)

-	pluginPaths, _ := filepath.Glob(fmt.Sprintf("%s/*.so", dir))
+	pluginPaths, err := filepath.Glob(fmt.Sprintf("%s/*.so", dir))
+	if err != nil {
+		return fmt.Errorf("failed to glob plugins: %v", err)
+	}

	RUN wget http://musl.libc.org/releases/musl-1.2.1.tar.gz && \
	RUN wget http://musl.libc.org/releases/musl-latest.tar.gz && \

Add hyperNode controller framework and provider #4014

Are you sure you want to change the base?

Add hyperNode controller framework and provider #4014

Conversation

Monokaix commented Feb 17, 2025

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

volcano-sh-bot commented Feb 18, 2025

Monokaix commented Feb 25, 2025

hwdef commented Mar 3, 2025

Copilot AI left a comment

Choose a reason for hiding this comment

PR Overview

Reviewed Changes

Copilot AI Mar 3, 2025

Choose a reason for hiding this comment

Copilot AI Mar 3, 2025

Choose a reason for hiding this comment

hwdef left a comment

Choose a reason for hiding this comment

hwdef Mar 3, 2025

Choose a reason for hiding this comment

hwdef Mar 3, 2025

Choose a reason for hiding this comment

hwdef Mar 3, 2025

Choose a reason for hiding this comment

hwdef Mar 3, 2025

Choose a reason for hiding this comment

hwdef Mar 3, 2025

Choose a reason for hiding this comment