Skip to content

Sumit/csi 5841 #846

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
sumitguptaibm wants to merge 9 commits into
base: release-1.13.1
Choose a base branch
from
Open

Sumit/csi 5841 #846

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
e2391a6
Fix: NVMe over FC bug in NodeUnstageVolume.
sumitguptaibm Feb 6, 2026
1f0fd1b
undo unecessary change in func WaitForDmToExist
sumitguptaibm Feb 9, 2026
cd44bb6
Update IsNvmeBaseDevice func
sumitguptaibm Feb 9, 2026
d664aa5
Optimizing Fixes
sumitguptaibm Feb 9, 2026
013ca59
csi-5997
uriziv1 Feb 8, 2026
b9abbcf
Merge branch 'release-1.13.1' of github.com:IBM/ibm-block-csi-driver …
sumitguptaibm Feb 13, 2026
c706a20
Fix for NodeExpand Volume in case of NVMe oF.
sumitguptaibm Feb 13, 2026
ec6491d
Handle native & non-native NVMe checks in NodeUnStage + NodeExpand
sumitguptaibm Mar 5, 2026
a8ac5c7
Adding connection logic for nvme
sumitguptaibm Mar 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 27 additions & 17 deletions node/pkg/driver/device_connectivity/device_connectivity_helper_scsigeneric.go
View file
Open in desktop

@shlomitn shlomitn Feb 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I see the logic was: Run multipathd, if it fails (returns with error) - exit immediately with the error. Otherwise: check if the output contains contains the volumeIdVariation. If it does - exit. If not - sleep for intervalSeconds and retry - for maxRetries.
The logic after your change is: run multipathd. If it fails - sleep and retry. Otherwise - same as above, return if found the volumeIdVariation or else retry.
That's a change of the logic. Why?... Who says we want to retry in case of multipathd error?

Original file line number Diff line number Diff line change
Expand Up @@ -750,24 +750,34 @@ func convertScsiIdToNguid(scsiId string) string {

func (o GetDmsPathHelperGeneric) WaitForDmToExist(volumeIdVariations []string, maxRetries int, intervalSeconds int,
multipathdCommandFormatArgs []string) (string, error) {
formatTemplate := strings.Join(multipathdCommandFormatArgs, mpathdSeparator)
formatTemplate := strings.Join(multipathdCommandFormatArgs, mpathdSeparator)
args := []string{"show", "maps", "raw", "format", "\"", formatTemplate, "\""}
logger.Debugf("Waiting for dm to exist")
for i := 0; i < maxRetries; i++ {
out, err := o.executer.ExecuteWithTimeout(TimeOutMultipathdCmd, multipathdCmd, args)
if err != nil {
return "", err
}
dms := string(out)
for _, volumeIdVariation := range volumeIdVariations {
if strings.Contains(dms, volumeIdVariation) {
return dms, nil
}
}

time.Sleep(time.Second * time.Duration(intervalSeconds))
}
return "", &MultipathDeviceNotFoundForVolumeError{volumeIdVariations[0]}
logger.Debugf("Waiting for dm to exist")
var lastErr error
for i := 0; i < maxRetries; i++ {
out, err := o.executer.ExecuteWithTimeout(TimeOutMultipathdCmd, multipathdCmd, args)
if err != nil {
lastErr = err
logger.Warningf("multipathd show maps failed (attempt %d/%d), retrying: %v", i+1, maxRetries, err)
time.Sleep(time.Second * time.Duration(intervalSeconds))
continue
}
lastErr = nil
dms := string(out)
for _, volumeIdVariation := range volumeIdVariations {
if strings.Contains(dms, volumeIdVariation) {
return dms, nil
}
}

time.Sleep(time.Second * time.Duration(intervalSeconds))
}

if lastErr != nil {
return "", lastErr
}

return "", &MultipathDeviceNotFoundForVolumeError{volumeIdVariations[0]}
}

func (o GetDmsPathHelperGeneric) ExtractDmFieldValues(dmFilterValues []string, mpathdOutput string) map[string]bool {
Expand Down
19 changes: 14 additions & 5 deletions node/pkg/driver/node.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -389,13 +389,22 @@ func (d *NodeService) NodeUnstageVolume(ctx context.Context, req *csi.NodeUnstag
return nil, status.Error(codes.Internal, err.Error())
}

err = d.OsDeviceConnectivityHelper.FlushMultipathDevice(baseDevice)
isNvme, err := d.NodeUtils.IsNvmeBaseDevice(baseDevice)
if err != nil {
return nil, status.Errorf(codes.Internal, "Multipath -f command failed with error: %v", err)
logger.Warningf("Failed to check if device %s is NVMe, assuming non-NVMe: %v", baseDevice, err)

@shlomitn shlomitn Feb 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this is... a problem. But I'm not sure there's a better way.
What would happen here? Either we're not NVMe, and then it's ok, or we may be NVMe and still try multipath -f, and it will fail, and so we won't clean the device.
What does actually clean the device eventually for NVMe? The function ClearStageInfoFile? Or what?...

isNvme = false
}
err = d.OsDeviceConnectivityHelper.RemovePhysicalDevice(sysDevices)
if err != nil {
return nil, status.Errorf(codes.Internal, "Remove scsi device failed with error: %v", err)
if !isNvme {
err = d.OsDeviceConnectivityHelper.FlushMultipathDevice(baseDevice)
if err != nil {
return nil, status.Errorf(codes.Internal, "Multipath -f command failed with error: %v", err)
}
err = d.OsDeviceConnectivityHelper.RemovePhysicalDevice(sysDevices)
if err != nil {
return nil, status.Errorf(codes.Internal, "Remove scsi device failed with error: %v", err)
}
} else {
logger.Infof("Device %s is NVMe: skipping multipath -f and SCSI device cleanup", baseDevice)
}

stageInfoPath := path.Join(stagingTargetPath, StageInfoFilename)
Expand Down
14 changes: 14 additions & 0 deletions node/pkg/driver/node_utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ type NodeUtilsInterface interface {
GetVolumeUuid(volumeId string) string
ReadNvmeNqn() (string, error)
DevicesAreNvme(sysDevices []string) (bool, error)
IsNvmeBaseDevice(device string) (bool, error)
ParseFCPorts() ([]string, error)
ParseIscsiInitiators() (string, error)
GetInfoFromPublishContext(publishContext map[string]string) (string, int, map[string][]string, error)
Expand Down Expand Up @@ -215,6 +216,19 @@ func (n NodeUtils) DevicesAreNvme(sysDevices []string) (bool, error) {
return false, nil
}

func (n NodeUtils) IsNvmeBaseDevice(device string) (bool, error) {
subsysNqnPath := path.Join("/sys/block", device, "device/subsysnqn")

if _, err := os.Stat(subsysNqnPath); err == nil {
return true, nil
} else if os.IsNotExist(err) {
return false, nil
} else {
logger.Errorf("Error while checking if device %s is NVMe: %v", device, err)
return false, err
}
}

func getRelevantLines(rawContent *os.File) ([]string, error) {
scanner := bufio.NewScanner(rawContent)
var relevantLines []string
Expand Down