azure: remove disk locks per vm during attach/detach

[aramase]

What type of PR is this?

/kind bug

What this PR does / why we need it:

• Changes the per VM lock logic for attach/detach operations.

Currently the per VM lock logic uses number of CPUs by default to lock which throttles number of concurrent attach/detach requests. With this updated locking logic, disk attach/detach for different nodes will be run concurrently. Disk attach/detach operations for the same node will be done sequentially.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

azure: update disk lock logic per vm during attach/detach to allow concurrent updates for different nodes.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

NONE

Test result -

Testing on a cluster with 1VMSS - 100 nodes

With the following changes -
Time taken for 10 disks (5 pods on same node + 5 pods on different nodes) - 9min
Time taken to scale from 10 disks to 60 disks - 6m36s
Time taken to detach and delete all 60 disks - 14m28s

Before the changes -
Time taken for 10 disks (5 pods on same node + 5 pods on different nodes) - 16min
Time taken to scale from 10 disks to 60 disks - 49m58s

[aramase]

/area provider/azure
/hold

(for testing repercussions of removing lock and ensuring updates to same VM is not a problem + soak tests)

/cc @khenidak

[aramase]

/assign @andyzhangx

[andyzhangx]

@aramase disk attach/detach lock cannot be removed now, is there a way to replace it with another better lock? This lock uses number of CPUs by default, we may change the default max thread num size.

[aramase]

@andyzhangx I'm going to test the behavior of removing the lock. If there are any issues, then yes we can investigate using the existing lock with different config or a different locking implementation.

[aramase]

/test pull-kubernetes-e2e-gce

[aramase]

/test pull-kubernetes-e2e-gce

[aramase]

/test pull-kubernetes-node-e2e-containerd

[aramase]

/test pull-kubernetes-kubemark-e2e-gce-big

[aramase]

Test result -

Testing on a cluster with 1VMSS - 100 nodes

With the following changes -
Time taken for 10 disks (5 pods on same node + 5 pods on different nodes) - 9min
Time taken to scale from 10 disks to 60 disks - 6m36s
Time taken to detach and delete all 60 disks - 14m28s

Before the changes -
Time taken for 10 disks (5 pods on same node + 5 pods on different nodes) - 16min
Time taken to scale from 10 disks to 60 disks - 49m58s

The tests are done on a cluster with 2 vcpus. This means the length of mutexes is equal to the CPU num (2). In case of AKS, the master has 8vcpus from what @andyzhangx told. This would mean with the old keymutex, there could be 8 concurrent updates (attach/detach) in parallel provided there are no hash collisions. With the changes in this PR, all vm updates happen in parallel which reduces disk attach/detach time for numerous disks.

@andyzhangx PTAL!

/hold
(adding a hold because want to get a approval from @khenidak too)

[andyzhangx]

/lgtm
/approve

[andyzhangx]

/hold cancel
since it's quite tight

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andyzhangx, aramase

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• staging/src/k8s.io/legacy-cloud-providers/azure/OWNERS [andyzhangx]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[andyzhangx]

/priority important-soon
/sig cloud-provider
/area provider/azure

[andyzhangx]

@aramase could you also cherry-pick this PR to old releases? thanks.

[aramase]

@andyzhangx will do!