Support nic lingjun node type by mowangdk · Pull Request #1585 · kubernetes-sigs/alibaba-cloud-csi-driver

mowangdk · 2025-12-15T06:59:38Z

What type of PR is this?

What this PR does / why we need it:

Support nic type lingjun node

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2025-12-15T06:59:40Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

mowangdk · 2025-12-15T08:37:22Z

/test all

mowangdk · 2025-12-15T13:09:09Z

/test all

huww98 · 2025-12-17T04:20:14Z

 	}
+	data, err := os.ReadFile(LingjunConfigFile)
+	if err != nil {
+		klog.V(1).InfoS("can't read lingjun metadata file, use env metadata only", "file", LingjunConfigFile, "error", err)


Maybe we should log "no such file" error at V(3), and other errors with klog.Error

huww98 · 2025-12-17T07:48:23Z

+	if data, exists := LingjunTestCases[caseName]; exists {
+		return []byte(data)
+	}
+	// Return default case if not found


Why so complex.. Just don't make it not found.

huww98 · 2025-12-17T07:53:46Z

+	NimitzInterface   []string `json:"NimitzInterface"`
+}
+
+func NewLingJunMetadata(data []byte) (*LingjunMetaData, error) {


Can we hide all the error handling in this func? e.g. just pass in a path, log and return nil on error. Then we just need this in NewMetadata:

if lm := NewLingJunMetadata(path); lm != nil { defaultMetadata.providers = append(defaultMetadata.providers, newImmutableProvider(lm, "lingjun")) }

So that lingjun.go is more self-contained.

huww98 · 2025-12-18T02:52:15Z

+
+func TestNewLingJunMetadata(t *testing.T) {
+	// Create a temporary directory for test files
+	tempDir := t.TempDir()


Move this to each test case, to avoid any possibility of interference between different cases.

huww98 · 2025-12-18T02:54:25Z

+		LingjunConfigFile = filepath.Join(tempDir, "lingjun_config_standard")
+		defer func() {
+			LingjunConfigFile = originalConfigFile
+		}()


How about just pass LingjunConfigFile into NewLingJunMetadata? Replacing global variable should only be used as last resort, as it prevents parallel test.

huww98 · 2025-12-18T02:56:08Z

+	if err != nil {
+		if errors.Is(err, os.ErrNotExist) {
+			klog.V(3).InfoS("lingjun metadata file does not exist, use env metadata only", "file", LingjunConfigFile)
+		} else {
+			klog.ErrorS(err, "Failed to read lingjun metadatafile", "file", LingjunConfigFile)
+		}
+		return defaultMetadata
+	}


OK, but I'd prefer these code to also go into NewLingJunMetadata, so that we can have test coverage over it.

huww98 · 2025-12-18T02:58:05Z

-		return getLingjunAvailableDiskCount(efloC, lingjunNodeId)
+		count, err := getLingjunAvailableDiskCount(efloC, lingjunNodeId)
+		if err != nil {
+			klog.ErrorS(err, "getAvailableDiskCount: failed to get lingjun available disk count", "lingjunID", lingjunNodeId)


Do we want to ignore all errors? Can we be more specific here?

huww98 · 2025-12-18T03:00:25Z

+			name:      "empty_node_type",
+			lingjunID: "node-123",
+			setupMock: func() {
+				// 模拟返回的 NodeType 为空字符串


Not sure about this case. Do you expect it to return success?

Also, please avoid Chinese in this repo.

huww98 · 2025-12-18T03:09:19Z

+	EniMac            string   `json:"EniMac"`
+	AckNicName        string   `json:"AckNicName"`
+	NimitzNetworkMode string   `json:"NimitzNetworkMode"`
+	NimitzInterface   []string `json:"NimitzInterface"`


How about remove fields that we don't use? This way, we:

reduce the possibility of being breaked by changes of this file.

slightly improve the performance as we don't need to deserialize these extra fields.

huww98 · 2025-12-22T12:30:38Z

+	nodeType := "ebmgvn"
+	lingjunNodeResp := &eflo.DescribeNodeResponse{
+		Body: &eflo.DescribeNodeResponseBody{
+			NodeType: &nodeType,


Suggested change

NodeType: &nodeType,

NodeType: ptr.To("ebmgvn"),

Also avoid Chinese.

huww98 · 2025-12-22T12:35:27Z

+	ctrl := gomock.NewController(t)
+	defer ctrl.Finish()
+
+	mockEFLOClient := cloud.NewMockEFLOInterface(ctrl)


We'd better create a new ctrl/mockEFLOClient for each case, to avoid any interference.

Besides, Finish() is not nessesary:

New in go1.14+, if you are passing a *testing.T into NewController function you no longer need to call ctrl.Finish() in your test methods.

huww98 · 2025-12-22T12:37:53Z

+			volumeHandle:   "fs-12345+",
+			expectedFsId:   "fs-12345",
+			expectedFsetId: "",
+		},


OK, but I'm not sure if you add this by mistake? This is unrelated to other changes.

k8s-ci-robot · 2025-12-22T13:15:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mowangdk

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~pkg/cloud/OWNERS~~ [mowangdk]
~~pkg/disk/OWNERS~~ [mowangdk]

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

huww98 · 2025-12-23T09:03:53Z

/lgtm

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 15, 2025

k8s-ci-robot requested review from huww98 and iltyty December 15, 2025 06:59

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 15, 2025

mowangdk force-pushed the feature/support_nic_lingjun branch from 3998323 to 7939ef8 Compare December 15, 2025 08:34

k8s-ci-robot 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 Dec 15, 2025

mowangdk force-pushed the feature/support_nic_lingjun branch from 7939ef8 to d829b22 Compare December 15, 2025 08:37

mowangdk force-pushed the feature/support_nic_lingjun branch from a7043fc to 1275539 Compare December 15, 2025 13:08

mowangdk force-pushed the feature/support_nic_lingjun branch from 1275539 to cc011d4 Compare December 15, 2025 13:12

mowangdk marked this pull request as ready for review December 15, 2025 13:13

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 15, 2025

huww98 reviewed Dec 17, 2025

View reviewed changes

Comment thread pkg/cloud/metadata/lingjun.go Outdated

huww98 reviewed Dec 17, 2025

View reviewed changes

mowangdk force-pushed the feature/support_nic_lingjun branch 3 times, most recently from 0d98d7c to 1680d9b Compare December 17, 2025 07:35

huww98 reviewed Dec 17, 2025

View reviewed changes

mowangdk force-pushed the feature/support_nic_lingjun branch 5 times, most recently from 52d808a to 4f73aa0 Compare December 17, 2025 09:52

huww98 reviewed Dec 18, 2025

View reviewed changes

mowangdk force-pushed the feature/support_nic_lingjun branch 2 times, most recently from 93566a2 to 804a2be Compare December 18, 2025 14:51

k8s-ci-robot 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 Dec 18, 2025

mowangdk force-pushed the feature/support_nic_lingjun branch 4 times, most recently from 5d900f7 to 1bf2f25 Compare December 22, 2025 12:21

huww98 reviewed Dec 22, 2025

View reviewed changes

mowangdk force-pushed the feature/support_nic_lingjun branch 2 times, most recently from 307b5a5 to d0a937c Compare December 22, 2025 13:15

Support nic & vpd lingjun node type

e3aec60

mowangdk force-pushed the feature/support_nic_lingjun branch from d0a937c to e3aec60 Compare December 23, 2025 08:50

k8s-ci-robot assigned huww98 Dec 23, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 23, 2025

k8s-ci-robot merged commit becf3b9 into kubernetes-sigs:master Dec 23, 2025
9 checks passed

Conversation

mowangdk commented Dec 15, 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?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

Uh oh!

k8s-ci-robot commented Dec 15, 2025

Uh oh!

mowangdk commented Dec 15, 2025

Uh oh!

mowangdk commented Dec 15, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

huww98 Dec 17, 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!

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!

huww98 Dec 22, 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!

k8s-ci-robot commented Dec 22, 2025

Uh oh!

huww98 commented Dec 23, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

huww98 Dec 17, 2025 •

edited

Loading

huww98 Dec 22, 2025 •

edited

Loading