diff --git a/pkg/volume/azure_dd/BUILD b/pkg/volume/azure_dd/BUILD index fb93b436e29..4833f5344cc 100644 --- a/pkg/volume/azure_dd/BUILD +++ b/pkg/volume/azure_dd/BUILD @@ -60,6 +60,7 @@ filegroup( go_test( name = "go_default_test", srcs = [ + "attacher_test.go", "azure_common_test.go", "azure_dd_block_test.go", "azure_dd_test.go", diff --git a/pkg/volume/azure_dd/attacher.go b/pkg/volume/azure_dd/attacher.go index 001a8bae008..5b559078069 100644 --- a/pkg/volume/azure_dd/attacher.go +++ b/pkg/volume/azure_dd/attacher.go @@ -22,6 +22,7 @@ import ( "path/filepath" "runtime" "strconv" + "strings" "time" "github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2019-03-01/compute" @@ -133,12 +134,14 @@ func (a *azureDiskAttacher) VolumesAreAttached(specs []*volume.Spec, nodeName ty } func (a *azureDiskAttacher) WaitForAttach(spec *volume.Spec, devicePath string, _ *v1.Pod, timeout time.Duration) (string, error) { - volumeSource, _, err := getVolumeSource(spec) - if err != nil { - return "", err + // devicePath could be a LUN number or + // "/dev/disk/azure/scsi1/lunx", "/dev/sdx" on Linux node + // "/dev/diskx" on Windows node + if strings.HasPrefix(devicePath, "/dev/") { + return devicePath, nil } - diskController, err := getDiskController(a.plugin.host) + volumeSource, _, err := getVolumeSource(spec) if err != nil { return "", err } @@ -146,23 +149,9 @@ func (a *azureDiskAttacher) WaitForAttach(spec *volume.Spec, devicePath string, nodeName := types.NodeName(a.plugin.host.GetHostName()) diskName := volumeSource.DiskName - lun := int32(-1) - if runtime.GOOS != "windows" { - // on Linux, usually devicePath is like "/dev/disk/azure/scsi1/lun2", get LUN directly - lun, err = getDiskLUN(devicePath) - if err != nil { - klog.V(2).Infof("azureDisk - WaitForAttach: getDiskLUN(%s) failed with error: %v", devicePath, err) - } - } - - if lun < 0 { - klog.V(2).Infof("azureDisk - WaitForAttach: begin to GetDiskLun by diskName(%s), DataDiskURI(%s), nodeName(%s), devicePath(%s)", - diskName, volumeSource.DataDiskURI, nodeName, devicePath) - lun, err = diskController.GetDiskLun(diskName, volumeSource.DataDiskURI, nodeName) - if err != nil { - return "", err - } - klog.V(2).Infof("azureDisk - WaitForAttach: GetDiskLun succeeded, got lun(%v)", lun) + lun, err := strconv.Atoi(devicePath) + if err != nil { + return "", fmt.Errorf("parse %s failed with error: %v, diskName: %s, nodeName: %s", devicePath, err, diskName, nodeName) } exec := a.plugin.host.GetExec(a.plugin.GetPluginName()) @@ -249,6 +238,14 @@ func (attacher *azureDiskAttacher) MountDevice(spec *volume.Spec, devicePath str if notMnt { diskMounter := util.NewSafeFormatAndMountFromHost(azureDataDiskPluginName, attacher.plugin.host) mountOptions := util.MountOptionFromSpec(spec, options...) + if runtime.GOOS == "windows" { + // only parse devicePath on Windows node + diskNum, err := getDiskNum(devicePath) + if err != nil { + return err + } + devicePath = diskNum + } err = diskMounter.FormatAndMount(devicePath, deviceMountPath, *volumeSource.FSType, mountOptions) if err != nil { if cleanErr := os.Remove(deviceMountPath); cleanErr != nil { diff --git a/pkg/volume/azure_dd/attacher_test.go b/pkg/volume/azure_dd/attacher_test.go new file mode 100644 index 00000000000..50a89f6bfff --- /dev/null +++ b/pkg/volume/azure_dd/attacher_test.go @@ -0,0 +1,73 @@ +/* +Copyright 2019 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package azure_dd + +import ( + "fmt" + "testing" + "time" + + "github.com/stretchr/testify/assert" + + "k8s.io/api/core/v1" + "k8s.io/kubernetes/pkg/volume" +) + +func createVolSpec(name string, readOnly bool) *volume.Spec { + return &volume.Spec{ + Volume: &v1.Volume{ + VolumeSource: v1.VolumeSource{ + AzureDisk: &v1.AzureDiskVolumeSource{ + DiskName: name, + ReadOnly: &readOnly, + }, + }, + }, + } +} +func TestWaitForAttach(t *testing.T) { + tests := []struct { + devicePath string + expected string + expectError bool + }{ + { + devicePath: "/dev/disk/azure/scsi1/lun0", + expected: "/dev/disk/azure/scsi1/lun0", + expectError: false, + }, + { + devicePath: "/dev/sdc", + expected: "/dev/sdc", + expectError: false, + }, + { + devicePath: "/dev/disk0", + expected: "/dev/disk0", + expectError: false, + }, + } + + attacher := azureDiskAttacher{} + spec := createVolSpec("fakedisk", false) + + for _, test := range tests { + result, err := attacher.WaitForAttach(spec, test.devicePath, nil, 3000*time.Millisecond) + assert.Equal(t, result, test.expected) + assert.Equal(t, err != nil, test.expectError, fmt.Sprintf("error msg: %v", err)) + } +} diff --git a/pkg/volume/azure_dd/azure_common.go b/pkg/volume/azure_dd/azure_common.go index ae85a7f9596..a02506b2f00 100644 --- a/pkg/volume/azure_dd/azure_common.go +++ b/pkg/volume/azure_dd/azure_common.go @@ -22,7 +22,6 @@ import ( "os" "path/filepath" "regexp" - "strconv" libstrings "strings" "github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2019-03-01/compute" @@ -62,7 +61,9 @@ var ( string(api.AzureDedicatedBlobDisk), string(api.AzureManagedDisk)) - lunPathRE = regexp.MustCompile(`/dev/disk/azure/scsi(?:.*)/lun(.+)`) + // only for Windows node + winDiskNumRE = regexp.MustCompile(`/dev/disk(.+)`) + winDiskNumFormat = "/dev/disk%d" ) func getPath(uid types.UID, volName string, host volume.VolumeHost) string { @@ -206,24 +207,12 @@ func strFirstLetterToUpper(str string) string { return libstrings.ToUpper(string(str[0])) + str[1:] } -// getDiskLUN : deviceInfo could be a LUN number or a device path, e.g. /dev/disk/azure/scsi1/lun2 -func getDiskLUN(deviceInfo string) (int32, error) { - var diskLUN string - if len(deviceInfo) <= 2 { - diskLUN = deviceInfo - } else { - // extract the LUN num from a device path - matches := lunPathRE.FindStringSubmatch(deviceInfo) - if len(matches) == 2 { - diskLUN = matches[1] - } else { - return -1, fmt.Errorf("cannot parse deviceInfo: %s", deviceInfo) - } +// getDiskNum : extract the disk num from a device path, +// deviceInfo format could be like this: e.g. /dev/disk2 +func getDiskNum(deviceInfo string) (string, error) { + matches := winDiskNumRE.FindStringSubmatch(deviceInfo) + if len(matches) == 2 { + return matches[1], nil } - - lun, err := strconv.Atoi(diskLUN) - if err != nil { - return -1, err - } - return int32(lun), nil + return "", fmt.Errorf("cannot parse deviceInfo: %s, correct format: /dev/disk?", deviceInfo) } diff --git a/pkg/volume/azure_dd/azure_common_test.go b/pkg/volume/azure_dd/azure_common_test.go index ad074923018..0d1ca64ccdb 100644 --- a/pkg/volume/azure_dd/azure_common_test.go +++ b/pkg/volume/azure_dd/azure_common_test.go @@ -183,57 +183,42 @@ func TestNormalizeStorageAccountType(t *testing.T) { } } -func TestGetDiskLUN(t *testing.T) { +func TestGetDiskNum(t *testing.T) { tests := []struct { deviceInfo string - expectedLUN int32 + expectedNum string expectError bool }{ { - deviceInfo: "0", - expectedLUN: 0, + deviceInfo: "/dev/disk0", + expectedNum: "0", expectError: false, }, { - deviceInfo: "10", - expectedLUN: 10, + deviceInfo: "/dev/disk99", + expectedNum: "99", expectError: false, }, { - deviceInfo: "11d", - expectedLUN: -1, + deviceInfo: "", + expectedNum: "", + expectError: true, + }, + { + deviceInfo: "/dev/disk", + expectedNum: "", expectError: true, }, { deviceInfo: "999", - expectedLUN: -1, - expectError: true, - }, - { - deviceInfo: "", - expectedLUN: -1, - expectError: true, - }, - { - deviceInfo: "/dev/disk/azure/scsi1/lun2", - expectedLUN: 2, - expectError: false, - }, - { - deviceInfo: "/dev/disk/azure/scsi0/lun12", - expectedLUN: 12, - expectError: false, - }, - { - deviceInfo: "/dev/disk/by-id/scsi1/lun2", - expectedLUN: -1, + expectedNum: "", expectError: true, }, } for _, test := range tests { - result, err := getDiskLUN(test.deviceInfo) - assert.Equal(t, result, test.expectedLUN) + result, err := getDiskNum(test.deviceInfo) + assert.Equal(t, result, test.expectedNum) assert.Equal(t, err != nil, test.expectError, fmt.Sprintf("error msg: %v", err)) } } diff --git a/pkg/volume/azure_dd/azure_common_windows.go b/pkg/volume/azure_dd/azure_common_windows.go index c48f191f309..c1c390999c1 100644 --- a/pkg/volume/azure_dd/azure_common_windows.go +++ b/pkg/volume/azure_dd/azure_common_windows.go @@ -84,7 +84,7 @@ func findDiskByLun(lun int, iohandler ioHandler, exec mount.Exec) (string, error if d, ok := v["number"]; ok { if diskNum, ok := d.(float64); ok { klog.V(2).Infof("azureDisk Mount: got disk number(%d) by LUN(%d)", int(diskNum), lun) - return strconv.Itoa(int(diskNum)), nil + return fmt.Sprintf(winDiskNumFormat, int(diskNum)), nil } klog.Warningf("LUN(%d) found, but could not get disk number(%q), location: %q", lun, d, location) }