disk: wait to serialize attach & detach #930
Conversation
k8s-ci-robot
added
kind/feature
|
Hi @huww98. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
k8s-ci-robot
added
the
size/L
pkg/disk/attachdetach_slot.go
Outdated
| select { | ||
| case s.detachSlot <- struct{}{}: | ||
| return nil | ||
| case s.normalSlot <- struct{}{}: |
There was a problem hiding this comment.
The test cases should meet the following scenarios on the same node
- Delete the application pod after restarting the controller pod, triggering an uninstallation.
- After restarting controller pod, delete/create different pods at the same time, trigger mounting/unmounting at the same time.
- after restarting the controller pod, create three pods at the same time, two mount and one unmount, and the unmounting is completed first.
There was a problem hiding this comment.
About "restarting the controller". The invariant here is the slot buffer is filled if and only if an attach or detach is in progress. This holds naturally when the process restarted.
I've run this through the official k/k external storage e2e test, with 25 cases in parallel. It works smoothly.
pkg/disk/attachdetach_slot.go
Outdated
| } | ||
|
|
||
| type serialSlot struct { | ||
| detachSlot chan struct{} |
There was a problem hiding this comment.
slot & priority slot would be better
There was a problem hiding this comment.
OK, I've updated the naming, and added some comments.
Avoid backoff caused by InvalidOperation.Conflict thus speed up attaching. Prioritize detach over attach to avoid InstanceDiskLimitExceeded error with best effort set DISK_SERIAL_ATTACH env to opt-in this behaviour in controller
|
I replaced sync.Map with map + sync.RWMutex, which is faster and avoids unnecessary allocations. |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: huww98, mowangdk 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:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
What type of PR is this?
/kind feature
What this PR does / why we need it:
Avoid backoff caused by InvalidOperation.Conflict thus speed up attaching.
Prioritize detach over attach to avoid InstanceDiskLimitExceeded error with best effort
set DISK_SERIAL_ATTACH env to opt-in this behaviour in controller
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
This is necessary to pass the external-storage test in k/k repository. Or we will almost certainly get timeout for volume not attached in 5 minutes.
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: