Repository: kubernetes-sigs/azurefile-csi-driver
Pull request #189 (2020-03-08 09:06:43)

chore: change volumeid format to include diskname
<!--  Thanks for sending a pull request!  Here are some tips for you:

1. If this is your first time, please read our contributor guidelines: https://git.k8s.io/community/contributors/guide#your-first-contribution and developer guide https://git.k8s.io/community/contributors/devel/development.md#development-guide
2. Please label this pull request according to what type of issue you are addressing, especially if this is a release targeted pull request. For reference on required PR/issue labels, read here:
https://git.k8s.io/community/contributors/devel/release.md#issue-kind-label
3. Ensure you have added or ran the appropriate tests for your PR: https://git.k8s.io/community/contributors/devel/testing.md
4. If you want *faster* PR reviews, read how: https://git.k8s.io/community/contributors/guide/pull-requests.md#best-practices-for-faster-reviews
5. Follow the instructions for writing a release note: https://git.k8s.io/community/contributors/guide/release-notes.md
6. If the PR is unfinished, see how to mark it: https://git.k8s.io/community/contributors/guide/pull-requests.md#marking-unfinished-pull-requests
-->

**What type of PR is this?**
/kind feature

**What this PR does / why we need it**:
chore: change volumeid format to include diskname

**Which issue(s) this PR fixes**:
<!-- 
*Automatically closes linked issue when PR is merged.
Usage: `Fixes #<issue number>`, or `Fixes (paste link of issue)`.
_If PR is about `failing-tests or flakes`, please post the related issues/tests in a comment and do not use `Fixes`_*
-->
Fixes #

**Special notes for your reviewer**:


**Release note**:
```
none
```


k8s-ci-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubernetes-sigs/azurefile-csi-driver/pull/189#" title="Author self-approved">andyzhangx</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubernetes-sigs%2Fazurefile-csi-driver).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/kubernetes-sigs/azurefile-csi-driver/blob/master/OWNERS)~~ [andyzhangx]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
k8s-ci-robot: @andyzhangx: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-azurefile-csi-driver-sanity | 08fcc882102351a9a36d60fab54d2f699f831307 | [link](https://prow.k8s.io/view/gcs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_azurefile-csi-driver/189/pull-azurefile-csi-driver-sanity/1236579247905050624) | `/test pull-azurefile-csi-driver-sanity`

[Full PR test history](https://prow.k8s.io/pr-history?org=kubernetes-sigs&repo=azurefile-csi-driver&pr=189). [Your PR dashboard](https://gubernator.k8s.io/pr/andyzhangx). Please help us cut down on flakes by [linking to](https://git.k8s.io/community/contributors/devel/sig-testing/flaky-tests.md#filing-issues-for-flaky-tests) an [open issue](https://github.com/kubernetes-sigs/azurefile-csi-driver/issues?q=is:issue+is:open) when you hit one in your PR.

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->

Patch:
diff --git a/pkg/azurefile/azurefile.go b/pkg/azurefile/azurefile.go
index ba35f5897..3c5d51ef8 100644
--- a/pkg/azurefile/azurefile.go
+++ b/pkg/azurefile/azurefile.go
@@ -44,7 +44,7 @@ import (
 const (
 	DriverName         = "file.csi.azure.com"
 	separator          = "#"
-	volumeIDTemplate   = "%s#%s#%s"
+	volumeIDTemplate   = "%s#%s#%s#%s"
 	serviceURLTemplate = "https://%s.file.%s"
 	fileURLTemplate    = "https://%s.file.%s/%s/%s"
 	fileMode           = "file_mode"
@@ -176,14 +176,14 @@ func (d *Driver) getFileSvcClient(accountName, accountKey string) (*azs.FileServ
 }
 
 // get file share info according to volume id, e.g.
-// input: "rg#f5713de20cde511e8ba4900#pvc-file-dynamic-17e43f84-f474-11e8-acd0-000d3a00df41"
-// output: rg, f5713de20cde511e8ba4900, pvc-file-dynamic-17e43f84-f474-11e8-acd0-000d3a00df41
-func getFileShareInfo(id string) (string, string, string, error) {
+// input: "rg#f5713de20cde511e8ba4900#pvc-file-dynamic-17e43f84-f474-11e8-acd0-000d3a00df41#diskname.vhd"
+// output: rg, f5713de20cde511e8ba4900, pvc-file-dynamic-17e43f84-f474-11e8-acd0-000d3a00df41, diskname.vhd
+func getFileShareInfo(id string) (string, string, string, string, error) {
 	segments := strings.Split(id, separator)
-	if len(segments) < 3 {
-		return "", "", "", fmt.Errorf("error parsing volume id: %q, should at least contain two #", id)
+	if len(segments) < 4 {
+		return "", "", "", "", fmt.Errorf("error parsing volume id: %q, should at least contain three #", id)
 	}
-	return segments[0], segments[1], segments[2], nil
+	return segments[0], segments[1], segments[2], segments[3], nil
 }
 
 // check whether mountOptions contains file_mode, dir_mode, vers, if not, append default mode
@@ -288,14 +288,14 @@ func checkShareNameBeginAndEnd(fileShareName string) bool {
 }
 
 // get snapshot name according to snapshot id, e.g.
-// input: "rg#f5713de20cde511e8ba4900#csivolumename#2019-08-22T07:17:53.0000000Z"
+// input: "rg#f5713de20cde511e8ba4900#csivolumename#diskname#2019-08-22T07:17:53.0000000Z"
 // output: 2019-08-22T07:17:53.0000000Z
 func getSnapshot(id string) (string, error) {
 	segments := strings.Split(id, separator)
-	if len(segments) != 4 {
-		return "", fmt.Errorf("error parsing volume id: %q, should at least contain three #", id)
+	if len(segments) < 5 {
+		return "", fmt.Errorf("error parsing volume id: %q, should at least contain four #", id)
 	}
-	return segments[3], nil
+	return segments[4], nil
 }
 
 func (d *Driver) expandVolume(ctx context.Context, volumeID string, capacityBytes int64) (int64, error) {
@@ -321,7 +321,7 @@ func (d *Driver) expandVolume(ctx context.Context, volumeID string, capacityByte
 	return volumehelper.GiBToBytes(int64(resp.Quota())), nil
 }
 
-func (d *Driver) createDisk(ctx context.Context, volumeID, accountName, accountKey, fileShareName, diskName string, diskSizeBytes int64) error {
+func (d *Driver) createDisk(ctx context.Context, accountName, accountKey, fileShareName, diskName string, diskSizeBytes int64) error {
 	vhdHeader := vhd.CreateFixedHeader(uint64(diskSizeBytes), &vhd.VHDOptions{})
 	buf := new(bytes.Buffer)
 	if err := binary.Write(buf, binary.BigEndian, vhdHeader); nil != err {
diff --git a/pkg/azurefile/controllerserver.go b/pkg/azurefile/controllerserver.go
index 940a47d57..da89306ea 100644
--- a/pkg/azurefile/controllerserver.go
+++ b/pkg/azurefile/controllerserver.go
@@ -59,7 +59,7 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest)
 	}
 
 	parameters := req.GetParameters()
-	var sku, resourceGroup, location, account, fileShareName, volumeID, diskName, fsType string
+	var sku, resourceGroup, location, account, fileShareName, diskName, fsType string
 
 	// Apply ProvisionerParameters (case-insensitive). We leave validation of
 	// the values to the cloud provider.
@@ -110,14 +110,6 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest)
 	if err != nil {
 		return nil, fmt.Errorf("failed to create file share(%s) on account(%s) type(%s) rg(%s) location(%s) size(%d), error: %v", fileShareName, account, sku, resourceGroup, location, fileShareSize, err)
 	}
-	volumeID = fmt.Sprintf(volumeIDTemplate, resourceGroup, retAccount, fileShareName)
-	/* todo: snapshot support
-	if req.GetVolumeContentSource() != nil {
-		contentSource := req.GetVolumeContentSource()
-		if contentSource.GetSnapshot() != nil {
-		}
-	}
-	*/
 	klog.V(2).Infof("create file share %s on storage account %s successfully", fileShareName, retAccount)
 
 	if err := d.checkFileShareCapacity(retAccount, retAccountKey, fileShareName, fileShareSize); err != nil {
@@ -129,7 +121,7 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest)
 		diskSizeBytes := volumehelper.GiBToBytes(requestGiB)
 		klog.V(2).Infof("begin to create vhd file(%s) size(%d) on share(%s) on account(%s) type(%s) rg(%s) location(%s)",
 			diskName, diskSizeBytes, fileShareName, account, sku, resourceGroup, location)
-		if err := d.createDisk(ctx, volumeID, retAccount, retAccountKey, fileShareName, diskName, diskSizeBytes); err != nil {
+		if err := d.createDisk(ctx, retAccount, retAccountKey, fileShareName, diskName, diskSizeBytes); err != nil {
 			return nil, status.Error(codes.Internal, fmt.Sprintf("failed to create VHD disk: %v", err))
 		}
 		klog.V(2).Infof("create vhd file(%s) size(%d) on share(%s) on account(%s) type(%s) rg(%s) location(%s) successfully",
@@ -137,6 +129,7 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest)
 		parameters[diskNameField] = diskName
 	}
 
+	volumeID := fmt.Sprintf(volumeIDTemplate, resourceGroup, retAccount, fileShareName, diskName)
 	return &csi.CreateVolumeResponse{
 		Volume: &csi.Volume{
 			VolumeId:      volumeID,
@@ -164,7 +157,7 @@ func (d *Driver) DeleteVolume(ctx context.Context, req *csi.DeleteVolumeRequest)
 		klog.V(4).Infof("failed to get share url with (%s): %v, returning with success", volumeID, err)
 		return &csi.DeleteVolumeResponse{}, nil
 	}
-	resourceGroupName, accountName, fileShareName, err := getFileShareInfo(volumeID)
+	resourceGroupName, accountName, fileShareName, _, err := getFileShareInfo(volumeID)
 	if err != nil {
 		klog.Errorf("getFileShareInfo(%s) in DeleteVolume failed with error: %v", volumeID, err)
 		return &csi.DeleteVolumeResponse{}, nil
@@ -188,7 +181,7 @@ func (d *Driver) ValidateVolumeCapabilities(ctx context.Context, req *csi.Valida
 	}
 
 	volumeID := req.VolumeId
-	resourceGroupName, accountName, fileShareName, err := getFileShareInfo(volumeID)
+	resourceGroupName, accountName, fileShareName, _, err := getFileShareInfo(volumeID)
 	if err != nil {
 		return nil, status.Errorf(codes.NotFound, "error getting volume(%s) info: %v", volumeID, err)
 	}
@@ -379,7 +372,7 @@ func (d *Driver) getShareURL(sourceVolumeID string) (azfile.ShareURL, error) {
 }
 
 func (d *Driver) getServiceURL(sourceVolumeID string) (azfile.ServiceURL, string, error) {
-	resourceGroupName, accountName, fileShareName, err := getFileShareInfo(sourceVolumeID)
+	resourceGroupName, accountName, fileShareName, _, err := getFileShareInfo(sourceVolumeID)
 	if err != nil {
 		klog.Errorf("getFileShareInfo(%s) failed with error: %v", sourceVolumeID, err)
 		return azfile.ServiceURL{}, "", err
diff --git a/pkg/azurefile/nodeserver.go b/pkg/azurefile/nodeserver.go
index d0d23cb7d..200285ba3 100644
--- a/pkg/azurefile/nodeserver.go
+++ b/pkg/azurefile/nodeserver.go
@@ -134,7 +134,7 @@ func (d *Driver) NodeStageVolume(ctx context.Context, req *csi.NodeStageVolumeRe
 	secrets := req.GetSecrets()
 	if len(secrets) == 0 {
 		var resourceGroupName string
-		resourceGroupName, accountName, fileShareName, err = getFileShareInfo(volumeID)
+		resourceGroupName, accountName, fileShareName, _, err = getFileShareInfo(volumeID)
 		if err != nil {
 			return nil, err
 		}
@@ -230,13 +230,14 @@ func (d *Driver) NodeStageVolume(ctx context.Context, req *csi.NodeStageVolumeRe
 		diskPath := filepath.Join(cifsMountPath, diskName)
 		options := util.JoinMountOptions(mountFlags, []string{"loop"})
 		if strings.HasPrefix(fsType, "ext") {
+			// following mount options are only valid for ext2/ext3/ext4 file systems
 			options = util.JoinMountOptions(options, []string{"noatime", "barrier=0", "errors=remount-ro"})
 		}
-		// FormatAndMount will format only if needed
+
 		klog.V(2).Infof("NodeStageVolume: formatting %s and mounting at %s with mount options(%s)", targetPath, diskPath, options)
+		// FormatAndMount will format only if needed
 		if err := d.mounter.FormatAndMount(diskPath, targetPath, fsType, options); err != nil {
-			msg := fmt.Sprintf("could not format %q and mount it at %q", targetPath, diskPath)
-			return nil, status.Error(codes.Internal, msg)
+			return nil, status.Error(codes.Internal, fmt.Sprintf("could not format %q and mount it at %q", targetPath, diskPath))
 		}
 		klog.V(2).Infof("NodeStageVolume: format %s and mounting at %s successfully.", targetPath, diskPath)
 	}


Test patch:
diff --git a/pkg/azurefile/azurefile_test.go b/pkg/azurefile/azurefile_test.go
index 1b481e79c..beb8fe055 100644
--- a/pkg/azurefile/azurefile_test.go
+++ b/pkg/azurefile/azurefile_test.go
@@ -22,6 +22,7 @@ import (
 	"os"
 	"path/filepath"
 	"reflect"
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -73,48 +74,63 @@ func TestAppendDefaultMountOptions(t *testing.T) {
 
 func TestGetFileShareInfo(t *testing.T) {
 	tests := []struct {
-		options   string
-		expected1 string
-		expected2 string
-		expected3 string
-		expected4 error
+		id                string
+		resourceGroupName string
+		accountName       string
+		fileShareName     string
+		diskName          string
+		expectedError     error
 	}{
 		{
-			options:   "rg#f5713de20cde511e8ba4900#pvc-file-dynamic-17e43f84-f474-11e8-acd0-000d3a00df41",
-			expected1: "rg",
-			expected2: "f5713de20cde511e8ba4900",
-			expected3: "pvc-file-dynamic-17e43f84-f474-11e8-acd0-000d3a00df41",
-			expected4: nil,
+			id:                "rg#f5713de20cde511e8ba4900#pvc-file-dynamic-17e43f84-f474-11e8-acd0-000d3a00df41#diskname.vhd",
+			resourceGroupName: "rg",
+			accountName:       "f5713de20cde511e8ba4900",
+			fileShareName:     "pvc-file-dynamic-17e43f84-f474-11e8-acd0-000d3a00df41",
+			diskName:          "diskname.vhd",
+			expectedError:     nil,
 		},
 		{
-			options:   "rg#f5713de20cde511e8ba4900",
-			expected1: "",
-			expected2: "",
-			expected3: "",
-			expected4: fmt.Errorf("error parsing volume id: \"rg#f5713de20cde511e8ba4900\", should at least contain two #"),
+			id:                "rg#f5713de20cde511e8ba4900",
+			resourceGroupName: "",
+			accountName:       "",
+			fileShareName:     "",
+			diskName:          "",
+			expectedError:     fmt.Errorf("error parsing volume id: \"rg#f5713de20cde511e8ba4900\", should at least contain three #"),
 		},
 		{
-			options:   "rg",
-			expected1: "",
-			expected2: "",
-			expected3: "",
-			expected4: fmt.Errorf("error parsing volume id: \"rg\", should at least contain two #"),
+			id:                "rg",
+			resourceGroupName: "",
+			accountName:       "",
+			fileShareName:     "",
+			diskName:          "",
+			expectedError:     fmt.Errorf("error parsing volume id: \"rg\", should at least contain three #"),
 		},
 		{
-			options:   "",
-			expected1: "",
-			expected2: "",
-			expected3: "",
-			expected4: fmt.Errorf("error parsing volume id: \"\", should at least contain two #"),
+			id:                "",
+			resourceGroupName: "",
+			accountName:       "",
+			fileShareName:     "",
+			diskName:          "",
+			expectedError:     fmt.Errorf("error parsing volume id: \"\", should at least contain three #"),
 		},
 	}
 
 	for _, test := range tests {
-		result1, result2, result3, result4 := getFileShareInfo(test.options)
-		if !reflect.DeepEqual(result1, test.expected1) || !reflect.DeepEqual(result2, test.expected2) ||
-			!reflect.DeepEqual(result3, test.expected3) || !reflect.DeepEqual(result4, test.expected4) {
-			t.Errorf("input: %q, getFileShareInfo result1: %q, expected1: %q, result2: %q, expected2: %q, result3: %q, expected3: %q, result4: %q, expected4: %q", test.options, result1, test.expected1, result2, test.expected2,
-				result3, test.expected3, result4, test.expected4)
+		resourceGroupName, accountName, fileShareName, diskName, expectedError := getFileShareInfo(test.id)
+		if !strings.EqualFold(resourceGroupName, test.resourceGroupName) {
+			t.Errorf("getFileShareInfo(%q) returned with: %q, expected: %q", test.id, resourceGroupName, test.resourceGroupName)
+		}
+		if !strings.EqualFold(accountName, test.accountName) {
+			t.Errorf("getFileShareInfo(%q) returned with: %q, expected: %q", test.id, accountName, test.accountName)
+		}
+		if !strings.EqualFold(fileShareName, test.fileShareName) {
+			t.Errorf("getFileShareInfo(%q) returned with: %q, expected: %q", test.id, fileShareName, test.fileShareName)
+		}
+		if !strings.EqualFold(diskName, test.diskName) {
+			t.Errorf("getFileShareInfo(%q) returned with: %q, expected: %q", test.id, diskName, test.diskName)
+		}
+		if !reflect.DeepEqual(expectedError, test.expectedError) {
+			t.Errorf("getFileShareInfo(%q) returned with: %v, expected: %v", test.id, expectedError, test.expectedError)
 		}
 	}
 }
@@ -299,29 +315,29 @@ func TestGetSnapshot(t *testing.T) {
 		expected2 error
 	}{
 		{
-			options:   "rg#f123#csivolumename#2019-08-22T07:17:53.0000000Z",
+			options:   "rg#f123#csivolumename#diskname#2019-08-22T07:17:53.0000000Z",
 			expected1: "2019-08-22T07:17:53.0000000Z",
 			expected2: nil,
 		},
 		{
 			options:   "rg#f123#csivolumename",
 			expected1: "",
-			expected2: fmt.Errorf("error parsing volume id: \"rg#f123#csivolumename\", should at least contain three #"),
+			expected2: fmt.Errorf("error parsing volume id: \"rg#f123#csivolumename\", should at least contain four #"),
 		},
 		{
 			options:   "rg#f123",
 			expected1: "",
-			expected2: fmt.Errorf("error parsing volume id: \"rg#f123\", should at least contain three #"),
+			expected2: fmt.Errorf("error parsing volume id: \"rg#f123\", should at least contain four #"),
 		},
 		{
 			options:   "rg",
 			expected1: "",
-			expected2: fmt.Errorf("error parsing volume id: \"rg\", should at least contain three #"),
+			expected2: fmt.Errorf("error parsing volume id: \"rg\", should at least contain four #"),
 		},
 		{
 			options:   "",
 			expected1: "",
-			expected2: fmt.Errorf("error parsing volume id: \"\", should at least contain three #"),
+			expected2: fmt.Errorf("error parsing volume id: \"\", should at least contain four #"),
 		},
 	}