search

kubernetes-sigs / aws-ebs-csi-driver

CSI driver for Amazon EBS https://aws.amazon.com/ebs/
Apache License 2.0
994 stars 797 forks source link

Only bail to AttachVolume if volume is detached #2183

Closed ConnorJC3 closed 1 month ago

ConnorJC3 commented 1 month ago

Is this a bug fix or adding new feature?

Bug fix

What is this PR about? / Why do we need it?

In WaitForAttachmentState, we bail to calling AttachVolume and immediately returning an error if the volume was reported as attached by DescribeInstances (i.e. if alreadyAssigned is true), but the volume returns a state other than attached. This check exists to guard against the case where DescribeInstances is incorrect, bailing early allows us to get the attachment moving and avoid sitting in an infinite loop until a timeout occurs.

However, this is incorrect - we should only perform this action if the volume is detached, not all states other than attached. This is for two reasons:

  1. Making an AttachVolume call when the volume is in a state other than detached will almost always fail.
  2. Due to eventual consistency, it is reasonably common some volumes will (correctly) report as attached via DescribeInstances but still report as attaching via DescribeVolumes for a short period of time. In this case, we repeatedly error out at the earliest possible opportunity, preventing WaitForAttachmentState's exponential backoff from working and peppering the AWS API with spurious AttachVolume calls.

What testing is done?

Added new unit test. Note that although depending on the test name is a horrible practice we should remove from this test, I have not spent the significant effort that would be required in this small PR to fix it.

github-actions[bot] commented 1 month ago

Code Coverage Diff

This PR does not change the code coverage

AndrewSirenko commented 1 month ago

/approve

k8s-ci-robot commented 1 month ago

[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: - ~~[OWNERS](https://github.com/kubernetes-sigs/aws-ebs-csi-driver/blob/master/OWNERS)~~ [AndrewSirenko] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment