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.

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

Jump to bottom

Fix reconstruction of FC volumes #74023

Merged
merged 1 commit into from
Feb 19, 2019
Merged

Fix reconstruction of FC volumes #74023

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
80 changes: 24 additions & 56 deletions pkg/volume/fc/fc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package fc
import (
"fmt"
"os"
"path/filepath"
"strconv"
"strings"

Expand Down Expand Up @@ -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
}

Expand All @@ -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
}

Expand Down
38 changes: 33 additions & 5 deletions pkg/volume/fc/fc_util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"os"
"path"
"path/filepath"
"strconv"
"strings"

"k8s.io/api/core/v1"
Expand Down Expand Up @@ -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 {
Copy link
Member

Choose a reason for hiding this comment

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

How will this work for existing FC mounts? I assume this only fixes new mounts right? Existing mounts that did not store all information will still be broken?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, only the first WWN / WWID will be restored from mounts created by old kubelet. That's equal to behaviour before this PR, i.e. it won't re-introduce any regression (tested on my FC machine).

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{}
Expand Down
66 changes: 66 additions & 0 deletions pkg/volume/fc/fc_util_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package fc

import (
"os"
"reflect"
"testing"
"time"

Expand Down Expand Up @@ -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)
}
})
}
}