Fix reconstruction of FC volumes

pkg/volume/fc/fc.go

@@ -19,6 +19,7 @@ package fc
 import (
 	"fmt"
 	"os"
+	"path/filepath"
 	"strconv"
 	"strings"
 
@@ -257,40 +258,20 @@ func (plugin *fcPlugin) ConstructVolumeSpec(volumeName, mountPath string) (*volu
 	if len(globalPDPath) == 0 {
 		return nil, fmt.Errorf("couldn't fetch globalPDPath. failed to obtain volume spec")
 	}
-	arr := strings.Split(globalPDPath, "/")
-	if len(arr) < 1 {
-		return nil, fmt.Errorf("failed to retrieve volume plugin information from globalPDPath: %v", globalPDPath)
+
+	wwns, lun, wwids, err := parsePDName(globalPDPath)
+	if err != nil {
+		return nil, fmt.Errorf("failed to retrieve volume plugin information from globalPDPath: %s", err)
 	}
-	volumeInfo := arr[len(arr)-1]
 	// Create volume from wwn+lun or wwid
-	var fcVolume *v1.Volume
-	if strings.Contains(volumeInfo, "-lun-") {
-		wwnLun := strings.Split(volumeInfo, "-lun-")
-		if len(wwnLun) < 2 {
-			return nil, fmt.Errorf("failed to retrieve TargetWWN and Lun. volumeInfo is invalid: %v", volumeInfo)
-		}
-		lun, err := strconv.Atoi(wwnLun[1])
-		if err != nil {
-			return nil, err
-		}
-		lun32 := int32(lun)
-		fcVolume = &v1.Volume{
-			Name: volumeName,
-			VolumeSource: v1.VolumeSource{
-				FC: &v1.FCVolumeSource{TargetWWNs: []string{wwnLun[0]}, Lun: &lun32},
-			},
-		}
-		klog.V(5).Infof("ConstructVolumeSpec: TargetWWNs: %v, Lun: %v",
-			fcVolume.VolumeSource.FC.TargetWWNs, *fcVolume.VolumeSource.FC.Lun)
-	} else {
-		fcVolume = &v1.Volume{
-			Name: volumeName,
-			VolumeSource: v1.VolumeSource{
-				FC: &v1.FCVolumeSource{WWIDs: []string{volumeInfo}},
-			},
-		}
-		klog.V(5).Infof("ConstructVolumeSpec: WWIDs: %v", fcVolume.VolumeSource.FC.WWIDs)
+	fcVolume := &v1.Volume{
+		Name: volumeName,
+		VolumeSource: v1.VolumeSource{
+			FC: &v1.FCVolumeSource{WWIDs: wwids, Lun: &lun, TargetWWNs: wwns},
+		},
 	}
+	klog.V(5).Infof("ConstructVolumeSpec: TargetWWNs: %v, Lun: %v, WWIDs: %v",
+		fcVolume.VolumeSource.FC.TargetWWNs, *fcVolume.VolumeSource.FC.Lun, fcVolume.VolumeSource.FC.WWIDs)
 	return volume.NewSpecFromVolume(fcVolume), nil
 }
 
@@ -310,36 +291,23 @@ func (plugin *fcPlugin) ConstructBlockVolumeSpec(podUID types.UID, volumeName, m
 	}
 	klog.V(5).Infof("globalMapPathUUID: %v, err: %v", globalMapPathUUID, err)
 
-	// Retrieve volumePluginDependentPath from globalMapPathUUID
+	// Retrieve globalPDPath from globalMapPathUUID
 	// globalMapPathUUID examples:
 	//   wwn+lun: plugins/kubernetes.io/fc/volumeDevices/50060e801049cfd1-lun-0/{pod uuid}
 	//   wwid: plugins/kubernetes.io/fc/volumeDevices/3600508b400105e210000900000490000/{pod uuid}
-	arr := strings.Split(globalMapPathUUID, "/")
-	if len(arr) < 2 {
-		return nil, fmt.Errorf("Fail to retrieve volume plugin information from globalMapPathUUID: %v", globalMapPathUUID)
-	}
-	l := len(arr) - 2
-	volumeInfo := arr[l]
-
+	globalPDPath := filepath.Dir(globalMapPathUUID)
 	// Create volume from wwn+lun or wwid
-	var fcPV *v1.PersistentVolume
-	if strings.Contains(volumeInfo, "-lun-") {
-		wwnLun := strings.Split(volumeInfo, "-lun-")
-		lun, err := strconv.Atoi(wwnLun[1])
-		if err != nil {
-			return nil, err
-		}
-		lun32 := int32(lun)
-		fcPV = createPersistentVolumeFromFCVolumeSource(volumeName,
-			v1.FCVolumeSource{TargetWWNs: []string{wwnLun[0]}, Lun: &lun32})
-		klog.V(5).Infof("ConstructBlockVolumeSpec: TargetWWNs: %v, Lun: %v",
-			fcPV.Spec.PersistentVolumeSource.FC.TargetWWNs,
-			*fcPV.Spec.PersistentVolumeSource.FC.Lun)
-	} else {
-		fcPV = createPersistentVolumeFromFCVolumeSource(volumeName,
-			v1.FCVolumeSource{WWIDs: []string{volumeInfo}})
-		klog.V(5).Infof("ConstructBlockVolumeSpec: WWIDs: %v", fcPV.Spec.PersistentVolumeSource.FC.WWIDs)
+	wwns, lun, wwids, err := parsePDName(globalPDPath)
+	if err != nil {
+		return nil, fmt.Errorf("failed to retrieve volume plugin information from globalPDPath: %s", err)
 	}
+	fcPV := createPersistentVolumeFromFCVolumeSource(volumeName,
+		v1.FCVolumeSource{TargetWWNs: wwns, Lun: &lun, WWIDs: wwids})
+	klog.V(5).Infof("ConstructBlockVolumeSpec: TargetWWNs: %v, Lun: %v, WWIDs: %v",
+		fcPV.Spec.PersistentVolumeSource.FC.TargetWWNs,
+		*fcPV.Spec.PersistentVolumeSource.FC.Lun,
+		fcPV.Spec.PersistentVolumeSource.FC.WWIDs)
+
 	return volume.NewSpecFromPersistentVolume(fcPV, false), nil
 }
 

pkg/volume/fc/fc_util.go

@@ -22,6 +22,7 @@ import (
 	"os"
 	"path"
 	"path/filepath"
+	"strconv"
 	"strings"
 
 	"k8s.io/api/core/v1"
@@ -131,20 +132,47 @@ func scsiHostRescan(io ioHandler) {
 	}
 }
 
-// make a directory like /var/lib/kubelet/plugins/kubernetes.io/fc/target-lun-0
+// make a directory like /var/lib/kubelet/plugins/kubernetes.io/fc/target1-target2-lun-0
 func makePDNameInternal(host volume.VolumeHost, wwns []string, lun string, wwids []string) string {
 	if len(wwns) != 0 {
-		return path.Join(host.GetPluginDir(fcPluginName), wwns[0]+"-lun-"+lun)
+		w := strings.Join(wwns, "-")
+		return path.Join(host.GetPluginDir(fcPluginName), w+"-lun-"+lun)
 	}
-	return path.Join(host.GetPluginDir(fcPluginName), wwids[0])
+	return path.Join(host.GetPluginDir(fcPluginName), strings.Join(wwids, "-"))
 }
 
 // make a directory like /var/lib/kubelet/plugins/kubernetes.io/fc/volumeDevices/target-lun-0
 func makeVDPDNameInternal(host volume.VolumeHost, wwns []string, lun string, wwids []string) string {
 	if len(wwns) != 0 {
-		return path.Join(host.GetVolumeDevicePluginDir(fcPluginName), wwns[0]+"-lun-"+lun)
+		w := strings.Join(wwns, "-")
+		return path.Join(host.GetVolumeDevicePluginDir(fcPluginName), w+"-lun-"+lun)
 	}
-	return path.Join(host.GetVolumeDevicePluginDir(fcPluginName), wwids[0])
+	return path.Join(host.GetVolumeDevicePluginDir(fcPluginName), strings.Join(wwids, "-"))
+}
+
+func parsePDName(path string) (wwns []string, lun int32, wwids []string, err error) {
+	// parse directory name created by makePDNameInternal or makeVDPDNameInternal
+	dirname := filepath.Base(path)
+	components := strings.Split(dirname, "-")
+	l := len(components)
+	if l == 1 {
+		// No '-', it must be single WWID
+		return nil, 0, components, nil
+	}
+	if components[l-2] == "lun" {
+		// it has -lun-, it's list of WWNs + lun number as the last component
+		if l == 2 {
+			return nil, 0, nil, fmt.Errorf("no wwn in: %s", dirname)
+		}
+		lun, err := strconv.Atoi(components[l-1])
+		if err != nil {
+			return nil, 0, nil, err
+		}
+
+		return components[:l-2], int32(lun), nil, nil
+	}
+	// no -lun-, it's just list of WWIDs
+	return nil, 0, components, nil
 }
 
 type fcUtil struct{}

pkg/volume/fc/fc_util_test.go

@@ -18,6 +18,7 @@ package fc
 
 import (
 	"os"
+	"reflect"
 	"testing"
 	"time"
 
@@ -116,3 +117,68 @@ func TestSearchDiskWWID(t *testing.T) {
 		t.Errorf("no fc disk found")
 	}
 }
+
+func TestParsePDName(t *testing.T) {
+	tests := []struct {
+		name        string
+		path        string
+		wwns        []string
+		lun         int32
+		wwids       []string
+		expectError bool
+	}{
+		{
+			name:  "single WWID",
+			path:  "/var/lib/kubelet/plugins/kubernetes.io/fc/60050763008084e6e0000000000001ae",
+			wwids: []string{"60050763008084e6e0000000000001ae"},
+		},
+		{
+			name:  "multiple WWID",
+			path:  "/var/lib/kubelet/plugins/kubernetes.io/fc/60050763008084e6e0000000000001ae-60050763008084e6e0000000000001af",
+			wwids: []string{"60050763008084e6e0000000000001ae", "60050763008084e6e0000000000001af"},
+		},
+		{
+			name: "single WWN",
+			path: "/var/lib/kubelet/plugins/kubernetes.io/fc/50050768030539b6-lun-0",
+			wwns: []string{"50050768030539b6"},
+			lun:  0,
+		},
+		{
+			name: "multiple WWNs",
+			path: "/var/lib/kubelet/plugins/kubernetes.io/fc/50050768030539b6-50050768030539b7-lun-0",
+			wwns: []string{"50050768030539b6", "50050768030539b7"},
+			lun:  0,
+		},
+		{
+			name:        "no WWNs",
+			path:        "/var/lib/kubelet/plugins/kubernetes.io/fc/lun-0",
+			expectError: true,
+		},
+		{
+			name:        "invalid lun",
+			path:        "/var/lib/kubelet/plugins/kubernetes.io/fc/50050768030539b6-lun-x",
+			expectError: true,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			wwns, lun, wwids, err := parsePDName(test.path)
+			if test.expectError && err == nil {
+				t.Errorf("expected error but got none")
+			}
+			if !test.expectError && err != nil {
+				t.Errorf("got unexpected error: %s", err)
+			}
+			if !reflect.DeepEqual(wwns, test.wwns) {
+				t.Errorf("expected WWNs %+v, got %+v", test.wwns, wwns)
+			}
+			if lun != test.lun {
+				t.Errorf("expected lun %d, got %d", test.lun, lun)
+			}
+			if !reflect.DeepEqual(wwids, test.wwids) {
+				t.Errorf("expected WWIDs %+v, got %+v", test.wwids, wwids)
+			}
+		})
+	}
+}