-
Notifications
You must be signed in to change notification settings - Fork 778
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove DeleteDisk call in CreateDisk path #2009
Remove DeleteDisk call in CreateDisk path #2009
Conversation
Signed-off-by: Connor Catlett <conncatl@amazon.com>
1acde90
to
1129a0c
Compare
Code Coverage DiffThis PR does not change the code coverage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with the changes made in this PR and specifically the point which states that this logic handles behavior that is the CO's responsibility. My understanding is that having a DeleteDisk
call in the CreateVolume
path makes is a violation of the CSI spec.
Also worth highlighting that the external provisioner takes care of cleaning up orphaned volumes:
err = cleanupVolume(ctx, p, delReq, provisionerCredentials)
if err != nil {
capErr = fmt.Errorf("%v. Cleanup of volume %s failed, volume is orphaned: %v", capErr, pvName, err)
}
I'm not sure if this is true for EBS CSI Driver today, due to Kubernetes/External-Provisioner not knowing the volume-id of the orphaned volume (and therefore not knowing what EBS volume to delete). Some other CSI Drivers store volume-id in PVC, because they can pass in a volume ID to their CreateVolume equivalent in their backend, which lets Kubernetes know the volume-id so K8s can be sure to delete volume if volume was orphaned. Alas, because Kubernetes and EBS CSI Driver only know volume's Idempotency token (hash of PVC name), and we cannot call EC2 DeleteVolume with just an idempotency token today, we do risk leaking/orphaning a volume (as an edge case). Can draw out a diagram later. Either way I agree with the PR because this edge case mentioned above is NOT solved via the code @ConnorJC3 is deleting anyway, due to the context typically being cancelled by the time we reach this DeleteVolume call. Furthermore this code today can lead to stuck pods IF sidecar timeout is very large, createVolume succeeds, but waitForVolume times out (due to today's 1 min limit), so I agree it is safer to delete this code today and think of something better. I do agree with first half of @torredil's statement though. |
@AndrewSirenko Sorry that last bit was poorly phrased and confusing. Also worth highlighting that the external provisioner takes care of cleaning up volumes is a bit clearer.
Can you clarify this point - what is the specific scenario in which the driver needs to call EC2's The correct behavior for cleaning up volumes is as follows:
In practice, this means failing the request and letting the provisioner retry as opposed to attempting to delete the volume, which is what this PR steers us towards - I think we all agree on that point. Once Kubernetes (the provisioner) has retrieved the volume ID from a
Agree 👍 I would like to learn more about this edge case and look forward to the diagram. |
Yep, I agree with this phrasing. I only disagreed with the external provisioner cleaning up orphaned volumes via the code from the
There is no scenario. We all agree, the driver should not call EC2 DeleteVolume in CreateVolume RPC. My sentence was not arguing against that. I'm saying that until EBS provides a way to call EC2 DeleteVolume with the idempotency token used for EC2 CreateVolume (instead of the volume-id), there may still be an edge case of volume leaks because the Sidenote: Looking more closely at |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: AndrewSirenko 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 |
Is this a bug fix or adding new feature?
Bug fix
What is this PR about? / Why do we need it?
Today, when we time out waiting for the volume to create, we delete it. This is broken for several reasons:
CreateVolume
Also takes this opportunity to cleanup the extremely unclear error message.
What testing is done?
N/A