Add explicit AttachVolume call in WaitForAttachmentState

[torredil]

Is this a bug fix or adding new feature?

Bug Fix

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

Today, the driver relies on the DescribeInstances API to determine the attachment state of a volume to an EC2 instance. The API may incorrectly report the volume as attached due to EC2's eventual consistency model, as a result the driver will skip making an expected AttachVolume API call and proceed to directly poll the attachment state of the volume.

This PR adds an explicit AttachVolume API call in the WaitForAttachmentState function when there is a mismatch between the expected attachment state and the actual attachment state reported for a volume. If the AttachVolume call fails, it is logged for debugging purposes but the overall behavior of returning an error and triggering a retry of the AttachDisk operation remains unchanged.

What testing is done?

• make test && make verify
• CI

[github-actions]

Code Coverage Diff

This PR does not change the code coverage

[ConnorJC3]

/lgtm

[ConnorJC3]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ConnorJC3

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 [ConnorJC3]

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

[stefanc18]

Hi @torredil. We're experiencing frequent attachment failures with the log "expected device to be attached but was detached". By looking on EC2, it looks like the volume is bound to the node, but the volume's state is still "Available", and the attachment status doesn't say "Attached". It looks like your change here would fix this as you're triggering a new AttachVolume API call if it gets into this mismatched state, but why still return the error, even if the API call returns no error? Maybe I'm misunderstanding the code, but it doesn't look like it will retry in this case

[torredil]

Hey @stefanc18

An error is intentionally returned here to short-circuit the WaitForAttachmentState polling.

but it doesn't look like it will retry in this case

Once WaitForAttachmentState returns an error, it will get bubbled up the call stack and returned by ControllerPublishVolume which will cause the external attacher sidecar to redrive a request. On the next call, the polling process will proceed as expected since the attach call has already been made.