Repository: kubernetes-sigs/azuredisk-csi-driver
Pull request #1183 (2022-02-17 14:29:58)

feat: disable async attach by default
<!--  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://github.com/kubernetes/community/blob/master/contributors/devel/sig-release/release.md#issue-kind-label
3. Ensure you have added or ran the appropriate tests for your PR: https://github.com/kubernetes/community/blob/master/contributors/devel/sig-testing/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**:
feat: disable async attach by default

**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 #

**Requirements**:
- [ ] uses [conventional commit messages](https://www.conventionalcommits.org/)
  <!-- Common commit types:
        build: Build 🏭
        chore: Maintenance 🔧
        ci: Continuous Integration 💜
        docs: Documentation 📘
        feat: Features 🌈
        fix: Bug Fixes 🐞
        perf: Performance Improvements 🚀
        refactor: Code Refactoring 💎
        revert: Revert Change ◀️
        style: Code Style 🎶
        security: Security Fix 🛡️
        test: Testing 💚 -->
- [ ] includes documentation
- [ ] adds unit tests
- [ ] tested upgrade from previous version

**Special notes for your reviewer**:


**Release note**:
```
chore: disable async attach by default
```


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

This pull-request has been approved by: *<a href="https://github.com/kubernetes-sigs/azuredisk-csi-driver/pull/1183#" 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%2Fazuredisk-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/azuredisk-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":[]} -->

Patch:
diff --git a/docs/driver-parameters.md b/docs/driver-parameters.md
index 2209226fc..4e156dfa0 100644
--- a/docs/driver-parameters.md
+++ b/docs/driver-parameters.md
@@ -27,7 +27,7 @@ networkAccessPolicy | NetworkAccessPolicy property to prevent anybody from gener
 diskAccessID | ARM id of the DiskAccess resource for using private endpoints on disks | | No  | ``
 enableBursting | [enable on-demand bursting](https://docs.microsoft.com/en-us/azure/virtual-machines/disk-bursting) beyond the provisioned performance target of the disk. On-demand bursting only be applied to Premium disk, disk size > 512GB, Ultra & shared disk is not supported. Bursting is disabled by default. | `true`, `false` | No | `false`
 useragent | User agent used for [customer usage attribution](https://docs.microsoft.com/en-us/azure/marketplace/azure-partner-customer-usage-attribution)| | No  | Generated Useragent formatted `driverName/driverVersion compiler/version (OS-ARCH)`
-enableAsyncAttach | allow multiple disk attach operations (in batch) on one node in parallel, this could speed up disk attachment while may hit Azure API throttling when there are large number of volume attachments | `true`, `false` | No | `true`
+enableAsyncAttach | allow multiple disk attach operations (in batch) on one node in parallel, this could speed up disk attachment while may hit Azure API throttling when there are large number of volume attachments | `true`, `false` | No | `false`
 subscriptionID | specify Azure subscription ID in which Azure disk will be created  | Azure subscription ID | No | if not empty, `resourceGroup` must be provided
 
 - disk created by dynamic provisioning
diff --git a/pkg/azuredisk/azuredisk.go b/pkg/azuredisk/azuredisk.go
index 9a6918281..f52a1a3ed 100644
--- a/pkg/azuredisk/azuredisk.go
+++ b/pkg/azuredisk/azuredisk.go
@@ -58,6 +58,7 @@ type DriverOptions struct {
 	UseCSIProxyGAInterface     bool
 	EnableDiskOnlineResize     bool
 	AllowEmptyCloudConfig      bool
+	EnableAsyncAttach          bool
 }
 
 // CSIDriver defines the interface for a CSI driver.
@@ -91,6 +92,7 @@ type DriverCore struct {
 	useCSIProxyGAInterface     bool
 	enableDiskOnlineResize     bool
 	allowEmptyCloudConfig      bool
+	enableAsyncAttach          bool
 }
 
 // Driver is the v1 implementation of the Azure Disk CSI Driver.
@@ -117,6 +119,7 @@ func newDriverV1(options *DriverOptions) *Driver {
 	driver.useCSIProxyGAInterface = options.UseCSIProxyGAInterface
 	driver.enableDiskOnlineResize = options.EnableDiskOnlineResize
 	driver.allowEmptyCloudConfig = options.AllowEmptyCloudConfig
+	driver.enableAsyncAttach = options.EnableAsyncAttach
 	driver.volumeLocks = volumehelper.NewVolumeLocks()
 	driver.ioHandler = azureutils.NewOSIOHandler()
 	driver.hostUtil = hostutil.NewHostUtil()
diff --git a/pkg/azuredisk/controllerserver.go b/pkg/azuredisk/controllerserver.go
index 27d8d3a11..43d916623 100644
--- a/pkg/azuredisk/controllerserver.go
+++ b/pkg/azuredisk/controllerserver.go
@@ -372,10 +372,7 @@ func (d *Driver) ControllerPublishVolume(ctx context.Context, req *csi.Controlle
 		}
 		klog.V(2).Infof("Trying to attach volume %q to node %q", diskURI, nodeName)
 
-		asyncAttach := true
-		if volumeContext[consts.EnableAsyncAttachField] == consts.FalseValue {
-			asyncAttach = false
-		}
+		asyncAttach := isAsyncAttachEnabled(d.enableAsyncAttach, volumeContext)
 		lun, err = d.cloud.AttachDisk(ctx, asyncAttach, diskName, diskURI, nodeName, cachingMode, disk)
 		if err == nil {
 			klog.V(2).Infof("Attach operation successful: volume %q attached to node %q.", diskURI, nodeName)
@@ -990,3 +987,18 @@ func (d *Driver) getSnapshotInfo(snapshotID string) (snapshotName, resourceGroup
 	}
 	return snapshotName, resourceGroup, subsID, err
 }
+
+func isAsyncAttachEnabled(defaultValue bool, volumeContext map[string]string) bool {
+	for k, v := range volumeContext {
+		switch strings.ToLower(k) {
+		case consts.EnableAsyncAttachField:
+			if strings.EqualFold(v, consts.TrueValue) {
+				return true
+			}
+			if strings.EqualFold(v, consts.FalseValue) {
+				return false
+			}
+		}
+	}
+	return defaultValue
+}
diff --git a/pkg/azurediskplugin/main.go b/pkg/azurediskplugin/main.go
index a54413d19..3cf288961 100644
--- a/pkg/azurediskplugin/main.go
+++ b/pkg/azurediskplugin/main.go
@@ -53,6 +53,7 @@ var (
 	useCSIProxyGAInterface     = flag.Bool("use-csiproxy-ga-interface", true, "boolean flag to enable csi-proxy GA interface on Windows")
 	enableDiskOnlineResize     = flag.Bool("enable-disk-online-resize", true, "boolean flag to enable disk online resize")
 	allowEmptyCloudConfig      = flag.Bool("allow-empty-cloud-config", true, "Whether allow running driver without cloud config")
+	enableAsyncAttach          = flag.Bool("enable-async-attach", false, "boolean flag to enable async attach")
 )
 
 func main() {
@@ -89,6 +90,7 @@ func handle() {
 		UseCSIProxyGAInterface:     *useCSIProxyGAInterface,
 		EnableDiskOnlineResize:     *enableDiskOnlineResize,
 		AllowEmptyCloudConfig:      *allowEmptyCloudConfig,
+		EnableAsyncAttach:          *enableAsyncAttach,
 	}
 	driver := azuredisk.NewDriver(&driverOptions)
 	if driver == nil {


Test patch:
diff --git a/pkg/azuredisk/controllerserver_test.go b/pkg/azuredisk/controllerserver_test.go
index 7dd585cda..fa3e64b43 100644
--- a/pkg/azuredisk/controllerserver_test.go
+++ b/pkg/azuredisk/controllerserver_test.go
@@ -22,19 +22,16 @@ import (
 	"reflect"
 	"testing"
 
+	"github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2021-07-01/compute"
 	"github.com/Azure/go-autorest/autorest/date"
 	"github.com/Azure/go-autorest/autorest/to"
-	"github.com/stretchr/testify/assert"
-	v1 "k8s.io/api/core/v1"
-
-	"github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2021-07-01/compute"
 	"github.com/container-storage-interface/spec/lib/go/csi"
 	"github.com/golang/mock/gomock"
-
+	"github.com/stretchr/testify/assert"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
 	"google.golang.org/protobuf/types/known/timestamppb"
-
+	v1 "k8s.io/api/core/v1"
 	consts "sigs.k8s.io/azuredisk-csi-driver/pkg/azureconstants"
 	"sigs.k8s.io/azuredisk-csi-driver/pkg/azuredisk/mockcorev1"
 	"sigs.k8s.io/azuredisk-csi-driver/pkg/azuredisk/mockkubeclient"
@@ -2007,3 +2004,42 @@ func getFakeDriverWithKubeClient(t *testing.T) FakeDriver {
 	d.getCloud().KubeClient.CoreV1().(*mockcorev1.MockInterface).EXPECT().PersistentVolumes().Return(persistentvolume).AnyTimes()
 	return d
 }
+
+func TestIsAsyncAttachEnabled(t *testing.T) {
+	tests := []struct {
+		name          string
+		defaultValue  bool
+		volumeContext map[string]string
+		expected      bool
+	}{
+		{
+			name:         "nil volumeContext",
+			defaultValue: false,
+			expected:     false,
+		},
+		{
+			name:          "empty volumeContext",
+			defaultValue:  true,
+			volumeContext: map[string]string{},
+			expected:      true,
+		},
+		{
+			name:          "false value in volumeContext",
+			defaultValue:  true,
+			volumeContext: map[string]string{"enableAsyncAttach": "false"},
+			expected:      false,
+		},
+		{
+			name:          "trie value in volumeContext",
+			defaultValue:  false,
+			volumeContext: map[string]string{"enableasyncattach": "true"},
+			expected:      true,
+		},
+	}
+	for _, test := range tests {
+		result := isAsyncAttachEnabled(test.defaultValue, test.volumeContext)
+		if result != test.expected {
+			t.Errorf("test(%s): result(%v) != expected result(%v)", test.name, result, test.expected)
+		}
+	}
+}