Make CreateVolume idempotent

[chrishenzie]

Is this a bug fix or adding new feature?
Addresses #165, but the full fix requires an EBS API change.

What is this PR about? / Why do we need it?
This PR works around idempotency issues with the CreateVolume API. It does so by using the InFlight struct to mark a request as "in progress". If an identical CreateVolumeRequest is received while another is in progress, we return an error to the caller.

What testing is done?
Unit tests and smoke tests on a k8s cluster on AWS.

This issue can be repeated by the following command (similar to the one in the GitHub issue, however it uses & to background the first call):

csc controller --endpoint <socket or address> create-volume --cap SINGLE_NODE_WRITER,block <volume name> & \
csc controller --endpoint <socket or address> create-volume --cap SINGLE_NODE_WRITER,block <volume name>

When running against master I observed:

$ csc controller --endpoint <socket> create-volume --cap SINGLE_NODE_WRITER,block chrishenzie-test & \
> csc controller --endpoint <socket> --cap SINGLE_NODE_WRITER,block chrishenzie-test
[1] 30225
"vol-0e313cca607416c77" 107374182400
"vol-02d575a8de114bbf3" 107374182400
[1]+  Done                    csc controller --endpoint <socket> create-volume --cap SINGLE_NODE_WRITER,block chrishenzie-test

This created two volumes with tags Key:CSIVolumeName, Value:chrishenzie-test (per the two volume IDs that are returned).

When running against this branch I observed:

$ csc controller --endpoint <socket> create-volume --cap SINGLE_NODE_WRITER,block chrishenzie-test & \
> csc controller --endpoint <socket> create-volume --cap SINGLE_NODE_WRITER,block chrishenzie-test
[1] 464
Create volume request for chrishenzie-test is already in progress

Please use -h,--help for more information

"vol-0ee90da47751cc6dd" 107374182400
[1]+  Exit 13                 csc controller --endpoint <socket> create-volume --cap SINGLE_NODE_WRITER,block chrishenzie-test

Which created only one volume.

Open questions

• At what point do we want to consider requests "identical"?
  • The thing we are depending on to establish uniqueness (at the very least) is the volume name, should we just be using that?
• What error code should be returned?
  • It should be Aborted based on the CSI spec

[coveralls]

Pull Request Test Coverage Report for Build 1604

• 8 of 8 (100.0%) changed or added relevant lines in 2 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.2%) to 81.49%

---

Totals
Change from base Build 1601:	0.2%
Covered Lines:	1717
Relevant Lines:	2107

---

💛 - Coveralls

[chrishenzie]

/assign @leakingtapan

@msau42

[msau42]

/assign @bertinatto @wongma7

[msau42]

CSI spec says use "ABORTED" for an operation in progress: https://github.com/container-storage-interface/spec/blame/master/spec.md#L470

[wongma7]

should we just use the req.Name as the key? I dont think it will ever matter in practice but spec says name is sufficient: https://github.com/container-storage-interface/spec/blob/master/spec.md#controller-service-rpc

[chrishenzie]

One issue with this approach is that this inFlight field is shared amongst the entire controller. By limiting the key to just the name we may encounter issues once other operations start depending on this field (assuming they also use only the name as the key).

[wongma7]

good point, it would need to categorize operations within the inflight map ... lgtm in current state then.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chrishenzie, wongma7

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:

• OWNERS [wongma7]

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

[leakingtapan]

It's okay to workaround the issue through inflight check, but I won't mark the original issue as resolved. Since the driver could still crash at the middle of processing the duplicate CreateVolume request, and the restarted driver won't have the original inFlight map as it is only held in memory if nothing changed too much recently. The real fix should be let EBS to take the client token during CreateVolume, which they admitted it's an easy change.

[chrishenzie]

@leakingtapan Yes this would still be an issue should the driver crash. Does the real fix you described require an EBS API change that is not currently available? I just want to make sure we're taking the best approach given the current limitations.

[leakingtapan]

Does the real fix you described require an EBS API change that is not currently available?

yep. And the change is to add a new field to the existing request struct which is totally doable.

@wongma7 do you wanna chat with EBS again for their progress on this? As I talked to them earlier, this is ready in the backend, and it just needs to be surfaced up.

@chrishenzie feel free to merge ur change in at the meantime. I just don’t want to close the original issue accidentally

[chrishenzie]

I don't think I can LGTM my own PR, would you be able to? I can re-open the issue if it closes or open a separate one that links to it.

[msau42]

Instead of "Fixes X", you can use a different keyword (like Addresses) so that github doesn't automatically close the issue.

[chrishenzie]

Updated description to use "Addresses" instead of "Fixes".

[ayberk]

/lgtm

[gnufied]

@AndyXiangLi I think we need a similar issue for CreateSnapshot unless we want to allow multiple CreateSnapshot calls for same volume.

[AndyXiangLi]

@gnufied We definitely don't want multiple CreateSnapshot call in flight for same volume, I will take a look and follow up with my team regarding this! Do we have a ticket created for this?