remove VM API call dep in azure disk WaitForAttach

add comment

add unit test for WaitForAttach fnc

add unit test for WaitForAttach Func

Author: andyzhangx

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",

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,36 +134,24 @@ 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
 	}
 
 	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 {

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))
+	}
+}

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)
-		}
-	}
-
-	lun, err := strconv.Atoi(diskLUN)
-	if err != nil {
-		return -1, err
+// 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
 	}
-	return int32(lun), nil
+	return "", fmt.Errorf("cannot parse deviceInfo: %s, correct format: /dev/disk?", deviceInfo)
 }

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,
-			expectError: true,
-		},
-		{
-			deviceInfo:  "999",
-			expectedLUN: -1,
-			expectError: true,
-		},
 		{
 			deviceInfo:  "",
-			expectedLUN: -1,
+			expectedNum: "",
 			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",
+			expectedNum: "",
+			expectError: true,
 		},
 		{
-			deviceInfo:  "/dev/disk/by-id/scsi1/lun2",
-			expectedLUN: -1,
+			deviceInfo:  "999",
+			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))
 	}
 }

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)
 					}