Refactor inFlight key to add lock per volumeId #702
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
Hi @AndyXiangLi. 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
needs-ok-to-test
k8s-ci-robot
added
the
size/L
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
Pull Request Test Coverage Report for Build 1612
💛 - Coveralls |
AndyXiangLi
force-pushed
the
node-concurrent-issue
branch
from
0a50dd2 to
5da84f0
Compare
AndyXiangLi
changed the title
pkg/driver/internal/inflight.go
Outdated
| @@ -45,26 +46,25 @@ func NewInFlight() *InFlight { | |||
|
|
|||
| // Insert inserts the entry to the current list of inflight requests. | |||
There was a problem hiding this comment.
Can we update the comment here?
pkg/driver/internal/inflight.go
Outdated
| db.mux.Lock() | ||
| defer db.mux.Unlock() | ||
|
|
||
| delete(db.inFlight, h.String()) | ||
| delete(db.inFlight, volumeId) |
There was a problem hiding this comment.
so h.String() is always equal to volumeId?
There was a problem hiding this comment.
No, previously the key in this cache is the hash of entire request( including volumeId and other fields like targetPath) so the driver has chance to process multiple requests for one volume at same time (Like same volumeId with different targetPath), which should not happen per spec
That's why I changed the key to volumeId.
There was a problem hiding this comment.
I guess my question is will the delete function work properly with the volumeId instead?
|
Mostly LGTM, but ideally we should have @gnufied take a look as well since he opened one of the issues. |
AndyXiangLi
force-pushed
the
node-concurrent-issue
branch
from
5da84f0 to
22b25f6
Compare
|
/test pull-aws-ebs-csi-driver-e2e-external-test |
|
@AndyXiangLi: The specified target(s) for
Use
In response to this:
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. |
|
/test pull-aws-ebs-csi-driver-external-test-latest |
|
/test pull-aws-ebs-csi-driver-e2e-single-az |
|
/test pull-aws-ebs-csi-driver-e2e-multi-az |
|
/test pull-aws-ebs-csi-driver-e2e-single-az |
AndyXiangLi
force-pushed
the
node-concurrent-issue
branch
from
22b25f6 to
c4c854d
Compare
AndyXiangLi
force-pushed
the
node-concurrent-issue
branch
from
c4c854d to
cb3f336
Compare
AndyXiangLi
force-pushed
the
node-concurrent-issue
branch
from
cb3f336 to
8155fe8
Compare
|
@gnufied How's this looks to you and can we merge this change? |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: AndyXiangLi, ayberk 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
Is this a bug fix or adding new feature?
Fixes #307
Fixes #370
What is this PR about? / Why do we need it?
Handle concurrency for node operations (stage/unstage publish/unpublish)
According to https://github.com/container-storage-interface/spec/blob/master/spec.md#concurrency driver's InFlight cache should track in-flight request per volumeId instead of per request to ensure no more than one request per volume at a given time.