PowerStore CSI driver Support for the multiple interface for iSCSI discovery + Increased UT Coverage #434

karthikk92 · 2025-02-07T06:34:57Z

Description

Bug Fixes:

Improved iSCSI discovery to support multiple interfaces, enhancing the node package.
Added ReachableEndPoint checks for better portal reachability during target discovery.
Updated go.mod and go.sum to use the latest version of goiscsi.

Refactoring and Unit Tests:

Refactored main.go for better readability and modularity.
Added main_test.go for core functionality tests.
Added tests for node.go
Introduced core_test.go and semver_test.go to validate core package variables and semantic versioning.

Concurrency and Safety:

Implemented mutex locks for IPToArray in the array package to ensure thread-safe updates and avoid race conditions.

GitHub Issues

List the GitHub issues impacted by this PR:

GitHub Issue #
dell/csm#1714

Checklist:

I have performed a self-review of my own code to ensure there are no formatting, vetting, linting, or security issues
I have verified that new and existing unit tests pass locally with my changes
I have not allowed coverage numbers to degenerate
I have maintained at least 90% code coverage
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
Backward compatibility is not broken

How Has This Been Tested?

Installed the powerstore driver both environment(i.e multiple interface and default interface) and installation working fine and iscsi discovery working as expected.
cert-csi scale testing executed and results are passing

pkg/node/node.go

pkg/node/node_connectivity_checker.go

adarsh-dell · 2025-02-11T13:03:02Z

pkg/node/node.go

 						if err != nil {
 							log.Error("couldn't discover targets")
 							continue
 						}
 						break
 					}
-					log.Debugf("Portal %s is not rechable from the node", address.Portal)
+					log.Debugf("Portal is not rechable from the node")


Any reason to not print the portal in the log?

ipAddress will not have any value populated at this line.

@karthikk92 Could you print 'address.Portal' here.? It will be helpful in debugging.

donatwork

Please add some more to the description regarding what was changed. A summary of changes perhaps?

donatwork · 2025-02-18T11:16:55Z

cmd/csi-powerstore/main.go

+		Controller:                controllerService,
+		Identity:                  identityService,
+		Node:                      nodeService,
+		Interceptors:              interList,


interList is not a good name, inter could be expanded into many things; interceptor, interface for example.

donatwork · 2025-02-18T11:23:04Z

pkg/array/array.go

@@ -109,7 +119,7 @@ func (s *Locker) UpdateArrays(configPath string, fs fs.Interface) error {
 		return fmt.Errorf("can't get config for arrays: %s", err.Error())
 	}
 	s.SetArrays(arrays)
-	IPToArray = matcher
+	updateIPToArray(matcher)


You are not really updating, you are setting so should start the function with set*.

donatwork · 2025-02-18T11:23:58Z

pkg/node/node.go

 				var iscsiTargets []goiscsi.ISCSITarget
 				for _, address := range infoList {
 					// first check if this portal is reachable from this machine or not
 					if ReachableEndPoint(address.Portal) {
+						ipAddressList := strings.Split(address.Portal, ":")


We may not yet support IPv6 but what's the impact here? is using colons the right choice of separator?

donatwork · 2025-02-18T11:25:50Z

pkg/node/node_connectivity_checker.go

 		var iscsiTargets []goiscsi.ISCSITarget
 		for _, address := range infoList {
 			// first check if this portal is reachable from this machine or not
 			if ReachableEndPoint(address.Portal) {
+				ipAddressList := strings.Split(address.Portal, ":")


Same here for use of colons. You could encapsulate this in a common utility function.

santhoshatdell · 2025-02-18T16:10:11Z

pkg/node/node_connectivity_checker.go

@@ -356,7 +363,7 @@ func (s *Service) populateTargetsInCache(array *array.PowerStoreArray) {
 				}
 				break
 			}
-			log.Debugf("Portal %s is not rechable from the node", address.Portal)
+			log.Debugf("Portal is not rechable from the node")


@karthikk92 Could you print 'address.Portal' here.? It will be helpful in debugging.

karthikk92 added 11 commits February 3, 2025 13:02

Discover multiple ifaces

838764c

Discover multiple ifaces

5da23d1

Discover multiple ifaces

743267f

Discover multiple ifaces

aca91a9

Discover multiple ifaces

4f1e1dd

Updated portal with IP

8aa206e

Updated portal with IP

920e90e

Updated portal with IP

bf8702d

Updated portal with IP

56e158a

Updated portal with IP

1ceac75

Updated portal with IP

608fbed

karthikk92 requested review from abhi16394, adarsh-dell, AkshaySainiDell, donatwork and santhoshatdell as code owners February 7, 2025 06:34

karthikk92 added 4 commits February 7, 2025 12:33

Resolved go fmt and lint issue

6d25e8c

Resolved go fmt and lint issue

2b8d13c

Resolved go fmt and lint issue

580b36a

Resolved go fmt and lint issue

7994da0

santhoshatdell reviewed Feb 7, 2025

View reviewed changes

pkg/node/node.go Outdated Show resolved Hide resolved

pkg/node/node_connectivity_checker.go Outdated Show resolved Hide resolved

karthikk92 and others added 7 commits February 11, 2025 09:08

addressed review comments and UT fialure fix

0c3a4aa

addressed review comments and UT fialure fix

6c7696d

Lint issue

4af7e0e

Update node_test.go to increase UT coverage

946b0e9

added test for cmd/main.go

bd12dce

added header for cmd/main_test.go

6fce2b8

Merge branch 'main' into discover-multiple-interface

563d45f

adarsh-dell reviewed Feb 11, 2025

View reviewed changes

Update UT coverage for semver.go

866c55e

WilsonRadadia20 and others added 17 commits February 13, 2025 13:09

Update UT coverage for semver.go

1e9fdaa

Add UT main.go

6894c2b

Updated UTs for core variables :

2926582

Add UT main.go

6c45165

Add UT main.go

2a37d04

Add UT main.go

4904a9e

Add UT main.go

795fc1f

Add UT main.go

5b99b27

Increased UT coverage of csi-powerstore.go

b5f4ab5

Remove unnecesary changes in semver.go

90c6a2b

Fix for semver failure

deed68e

Reverting the changes

38b3a19

Fix for failure in semver.go

172f9d0

remove linter errors

c706390

fixed race condition

e5ec09e

fixed race condition

8f83349

fixed linters errors

9752905

rishabhatdell previously approved these changes Feb 18, 2025

View reviewed changes

suryagupta4 previously approved these changes Feb 18, 2025

View reviewed changes

rishabhatdell changed the title ~~PowerStore CSI driver Support for the multiple interface for iSCSI discovery~~ PowerStore CSI driver Support for the multiple interface for iSCSI discovery + Increased UT Coverage Feb 18, 2025

donatwork previously approved these changes Feb 18, 2025

View reviewed changes

fixed don review comments

30e2882

rishabhatdell dismissed stale reviews from donatwork, suryagupta4, and themself via 30e2882 February 18, 2025 11:41

donatwork approved these changes Feb 18, 2025

View reviewed changes

rishabhatdell approved these changes Feb 18, 2025

View reviewed changes

rishabhatdell merged commit ce5fb75 into main Feb 18, 2025
7 checks passed

rishabhatdell deleted the discover-multiple-interface branch February 18, 2025 11:53

santhoshatdell reviewed Feb 18, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PowerStore CSI driver Support for the multiple interface for iSCSI discovery + Increased UT Coverage #434

PowerStore CSI driver Support for the multiple interface for iSCSI discovery + Increased UT Coverage #434

karthikk92 commented Feb 7, 2025 •

edited by rishabhatdell

Loading

adarsh-dell Feb 11, 2025

karthikk92 Feb 13, 2025

santhoshatdell Feb 18, 2025

donatwork left a comment

donatwork Feb 18, 2025

donatwork Feb 18, 2025

donatwork Feb 18, 2025

donatwork Feb 18, 2025

santhoshatdell Feb 18, 2025

PowerStore CSI driver Support for the multiple interface for iSCSI discovery + Increased UT Coverage #434

PowerStore CSI driver Support for the multiple interface for iSCSI discovery + Increased UT Coverage #434

Conversation

karthikk92 commented Feb 7, 2025 • edited by rishabhatdell Loading

Description

GitHub Issues

Checklist:

How Has This Been Tested?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donatwork left a 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

karthikk92 commented Feb 7, 2025 •

edited by rishabhatdell

Loading