Skip to content
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.

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

Fix Node Driver Crash Caused by Auto-Select Protocol #421

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open
4 changes: 2 additions & 2 deletions samples/configmap/powermax-array-config.yaml
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# Copyright © 2024 Dell Inc. or its subsidiaries. All Rights Reserved.
# Copyright © 2025 Dell Inc. or its subsidiaries. All Rights Reserved.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
Expand All @@ -17,7 +17,7 @@ metadata:
namespace: powermax
data:
powermax-array-config.yaml: |
# List of comma-separated port groups (ISCSI only). Example: PortGroup1, portGroup2 Required for iSCSI only
# List of comma-separated port groups (ISCSI and NVMe/TCP only). Example: PortGroup1, portGroup2 Required for iSCSI and NVMe/TCP only
X_CSI_POWERMAX_PORTGROUPS: ""
# Choose which transport protocol to use (ISCSI, FC, NVMETCP or auto) defaults to auto if nothing is specified
X_CSI_TRANSPORT_PROTOCOL: ""
Expand Down
4 changes: 2 additions & 2 deletions service/controller.go
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
Copyright © 2021 Dell Inc. or its subsidiaries. All Rights Reserved.
Copyright © 2021-2025 Dell Inc. or its subsidiaries. All Rights Reserved.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -2223,7 +2223,7 @@ func getMVLockKey(symID, tgtMaskingViewID string) string {
// on array is NVMe or not
func (s *service) IsNodeNVMe(ctx context.Context, symID, nodeID string, pmaxClient pmax.Pmax) (bool, error) {
nvmeTCPHostID, _, nvmeTCPMaskingViewID := s.GetNVMETCPHostSGAndMVIDFromNodeID(nodeID)
if s.opts.TransportProtocol == NvmeTCPTransportProtocol {
if s.opts.TransportProtocol == NvmeTCPTransportProtocol || s.useNVMeTCP {
log.Debug("Preferred transport protocol is set to NVME/TCP")
// Check if NVME MV exists
_, nvmeMvErr := pmaxClient.GetMaskingViewByID(ctx, symID, nvmeTCPMaskingViewID)
Expand Down
25 changes: 24 additions & 1 deletion service/features/service.feature
Original file line number Diff line number Diff line change
Expand Up @@ -496,10 +496,13 @@ Feature: PowerMax CSI interface
| transport | induced | errormsg | mode |
| "ISCSI" | "none" | "none" | "node" |
| "FC" | "none" | "none" | "node" |
| "NVME" | "none" | "none" | "node" |
| "ISCSI" | "GetInitiatorError" | "Error retrieving Initiator(s)" | "node" |
| "ISCSI" | "none" | "none" | "node" |
| "FC" | "GetInitiatorError" | "Error retrieving Initiator(s)" | "node" |
| "FC" | "none" | "none" | "node" |
| "NVME" | "GetInitiatorError" | "Error retrieving Initiator(s)" | "node" |
| "NVME" | "none" | "none" | "node" |

@v1.0.0
Scenario: Validate nodeHostSetup with temporary failure
Expand Down Expand Up @@ -735,7 +738,7 @@ Feature: PowerMax CSI interface
Then 0 nvmetcp targets are returned

@v1.4.0
Scenario: Validate nodeHostSetup with temporary failure
Scenario: Validate nodeHostSetup with temporary FC failure
Given a PowerMax service
And I set transport protocol to "FC"
And I induce error "GetHostError"
Expand All @@ -744,6 +747,26 @@ Feature: PowerMax CSI interface
When I invoke nodeHostSetup with a "node" service
Then no error was received

@v2.13.0
Scenario: Validate nodeHostSetup with temporary NVMe failure
Given a PowerMax service
And I set transport protocol to "NVME"

Choose a reason for hiding this comment

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

NVMe and iSCSI ?

Choose a reason for hiding this comment

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

Applicable in all other files as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Switch case has this:

case "ISCSI":

And I induce error "GetHostError"
And I induce error "CreateHostError"
And I have a Node "Node1" with MaskingView
When I invoke nodeHostSetup with a "node" service
Then no error was received

@v2.13.0
Scenario: Validate nodeHostSetup with temporary iSCSI failure
Given a PowerMax service
And I set transport protocol to "ISCSI"
And I induce error "GetHostError"
And I induce error "CreateHostError"
And I have a Node "Node1" with MaskingView
When I invoke nodeHostSetup with a "node" service
Then no error was received

@v2.2.0
Scenario Outline: Call NodeGetVolumeStats
Given a PowerMax service
Expand Down
60 changes: 37 additions & 23 deletions service/node.go
Original file line number Diff line number Diff line change
Expand Up @@ -1787,20 +1787,20 @@ func (s *service) nodeHostSetup(ctx context.Context, portWWNs []string, IQNs []s

nodeChroot, _ := csictx.LookupEnv(context.Background(), EnvNodeChroot)
if s.useNVMeTCP {
// resetOtherProtocols
s.useFC = false
s.useIscsi = false
// check nvme module availability on the host
err = s.setupArrayForNVMeTCP(ctx, symID, validNVMeTCPs, pmaxClient)
if err != nil {
log.Errorf("Failed to do the NVMe setup for the Array(%s). Error - %s", symID, err.Error())
} else {
s.initNVMeTCPConnector(nodeChroot)
s.arrayTransportProtocolMap[symID] = NvmeTCPTransportProtocol
// resetOtherProtocols
s.useFC = false
s.useIscsi = false
}
s.initNVMeTCPConnector(nodeChroot)
s.arrayTransportProtocolMap[symID] = NvmeTCPTransportProtocol
} else if s.useFC {
// resetOtherProtocols
s.useNVMeTCP = false
s.useIscsi = false

}
if s.useFC {
formattedFCs := make([]string, 0)
for _, initiatorID := range validFCs {
elems := strings.Split(initiatorID, ":")
Expand All @@ -1809,24 +1809,32 @@ func (s *service) nodeHostSetup(ctx context.Context, portWWNs []string, IQNs []s
err := s.setupArrayForFC(ctx, symID, formattedFCs, pmaxClient)
if err != nil {
log.Errorf("Failed to do the FC setup the Array(%s). Error - %s", symID, err.Error())
} else {
s.initFCConnector(nodeChroot)
s.arrayTransportProtocolMap[symID] = FcTransportProtocol
isSymConnFC[symID] = true
// resetOtherProtocols
s.useNVMeTCP = false
s.useIscsi = false
}
s.initFCConnector(nodeChroot)
s.arrayTransportProtocolMap[symID] = FcTransportProtocol
isSymConnFC[symID] = true
} else if s.useIscsi {
// resetOtherProtocols
s.useNVMeTCP = false
s.useNFS = false

}
if s.useIscsi {
err := s.ensureISCSIDaemonStarted()
if err != nil {
log.Errorf("Failed to start the ISCSI Daemon. Error - %s", err.Error())
}
err = s.setupArrayForIscsi(ctx, symID, validIscsis, pmaxClient)
if err != nil {
log.Errorf("Failed to do the ISCSI setup for the Array(%s). Error - %s", symID, err.Error())
} else {
s.initISCSIConnector(nodeChroot)
s.arrayTransportProtocolMap[symID] = IscsiTransportProtocol
// resetOtherProtocols
s.useNVMeTCP = false
s.useNFS = false
}
s.initISCSIConnector(nodeChroot)
s.arrayTransportProtocolMap[symID] = IscsiTransportProtocol

}
}

Expand Down Expand Up @@ -1866,7 +1874,11 @@ func (s *service) setupArrayForIscsi(ctx context.Context, array string, IQNs []s
return err
}
_, err = s.getAndConfigureMaskingViewTargets(ctx, array, mvName, IQNs, pmaxClient)
return err
if err != nil && !(strings.Contains(err.Error(), "Masking View") && strings.Contains(err.Error(), "cannot be found")) {
return err
}
log.Warning(err)
return nil
}

// setupArrayForIscsi is called to set up a node for iscsi operation.
Expand Down Expand Up @@ -1894,7 +1906,11 @@ func (s *service) setupArrayForNVMeTCP(ctx context.Context, array string, NQNs [

// Create or update the NVMe Host and Initiators
_, err = s.getAndConfigureMaskingViewTargetsNVMeTCP(ctx, array, mvName, pmaxClient)
return err
if err != nil && !(strings.Contains(err.Error(), "Masking View") && strings.Contains(err.Error(), "cannot be found")) {
return err
}
log.Warning(err)
return nil
}

func (s *service) updateNQNWithHostID(ctx context.Context, symID string, NQNs []string, pmaxClient pmax.Pmax) ([]string, error) {
Expand Down Expand Up @@ -2002,10 +2018,8 @@ func (s *service) setupNVMeTCPTargetDiscovery(ctx context.Context, array string,
}

if len(ipInterfaces) == 0 {
log.Errorf("Couldn't find any ip interfaces on any of the port-groups")
return err
return fmt.Errorf("couldn't find any ip interfaces on any of the port-groups %s", s.opts.PortGroups)
}

for _, ip := range ipInterfaces {
// Attempt target discovery from host
log.Debugf("Discovering NVMe targets on %s", ip)
Expand Down
38 changes: 16 additions & 22 deletions service/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -728,28 +728,22 @@ func (s *service) getProxySettingsFromEnv() (string, string, bool) {
}

func (s *service) getTransportProtocolFromEnv() string {
transportProtocol := ""
if tp, ok := csictx.LookupEnv(context.Background(), EnvPreferredTransportProtocol); ok {
tp = strings.ToUpper(tp)
switch tp {
case "FIBRE":
tp = "FC"
break
case "FC":
break
case "ISCSI":
break
case "NVMETCP":
break
case "":
break
default:
log.Errorf("Invalid transport protocol: %s, valid values FC, ISCSI or NVMETCP", tp)
return ""
}
transportProtocol = tp
}
return transportProtocol
tp, ok := csictx.LookupEnv(context.Background(), EnvPreferredTransportProtocol)
if !ok {
return ""
}
tp = strings.ToUpper(tp)
switch tp {
case "FIBRE":
return "FC"
case "FC", "ISCSI", "NVMETCP", "":

Choose a reason for hiding this comment

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

This is nice.

return tp
case "AUTO":
return ""
default:
log.Errorf("Invalid transport protocol: %s, valid values AUTO, FC, ISCSI or NVMETCP", tp)
return ""
}
}

// get the amount of time to retry pmax calls
Expand Down
Loading