AWS: Fix detaching volumes of deleted pods

[jsafrane]

When attachDiskAndVerify gives up waiting for a volume to be attached, AWS may be still trying to attach the volume.

If the pod that wants the volume attached still exists, kubelet will retry attaching the volume on next kubelet sync (and everything is allright).

However, if the pod is deleted, AWS is still trying to attach the volume and it may eventually succeed. We end up with a volume attached to a node and no pod for it. Nobody will detach the volume.

As fix, just fire DetachVolume to AWS when attachDiskAndVerify gives up, without waiting for result. If associated pod still exists, it will try to attach the volume soon. If the pod is deleted, AWS will detach the volume (or stop attaching it).

Fixes: #24807

There are several tiny patches, the last one bringing everything together. I tested it really fixes the referenced issue, still I am open for better suggestions how to fix it in a better way.

@justinsb @kubernetes/sig-storage

[saad-ali]

From my experience on the GCE side of things, I'd be caution against doing this. The existing code has some ugly races between attach and detach. This means rapid pod creation and deletion can result in funky behavior like two attaches getting triggered back to back followed by a detach. Basically, this whole house of cards is held together with duct tape and moving one little piece to fix one bug may result in unintentionally (and non-deterministically) breaking a bunch of other scenarios.

That said, if you're confident this will fix the issue and not break anything, go for it. Otherwise you may want to wait for v1.3's implementation of #20262 that should address this issue more robustly.

I'll let @justinsb review this.

[ghost]

I don't follow this warning.. what are the possible values of attachmentStatus ?

[jsafrane]

I just refactored part of waitForAttachmentStatus into standalone function, I admit I don't understand all the details here. I've never seen this warning printed in my logs.

[justinsb]

Yeah it is probably superfluous with the len(info.Attachments) check, but EBS seems so flaky at times that I wanted to program really defensively. This will only fire if somehow a volume got attached twice (which should be impossible)

[ghost]

i am okay with this change overall. Lets see what @justinsb says.
How much of an impact did it have ?

[jsafrane]

Fixed unit tests and pushed.

[jsafrane]

Basically, this whole house of cards is held together with duct tape and moving one little piece to fix one bug may result in unintentionally (and non-deterministically) breaking a bunch of other scenarios.

+1000. I know it's really fragile and I hope my changes will be replaced by rock stable attach controller soon. Unfortunately, we do leak attached volumes now and we should fix it even before 1.3.

[jsafrane]

@k8s-bot test this issue: #24937

[k8s-bot]

GCE e2e build/test passed for commit ecd38ad.

• Build Log
• Test Artifacts
• Internal Jenkins Results

[justinsb]

Have we actually seen those leaks in the wild? Do we have logs we can review?

I am also super-wary of changing the aws ebs volumes code, as @saad-ali said. I did a lot of work in 1.2 to get back in sync with the GCE volumes code, and I am wary of breaking sync with it again. If we do this on AWS, why don't we do this on GCE also?

That said, there is a loophole in my logic... If you think this is worse than leaking the volume, then maybe we could put the Detach fire into AttachDisk? Similar to this code:

// attachEnded is set to true if the attach operation completed
    // (successfully or not)
    attachEnded := false
    defer func() {
        if attachEnded {
            awsInstance.endAttaching(disk.awsID, mountDevice)
        }
    }()

We would essentially fire your detach on exit from AttachDisk where we otherwise would leak the attach.

I'm also not entirely sure that a Detach actually cancels an EBS Attach, if it is in fact stuck.

What is the status of a the new sync logic @saad-ali ?

[saad-ali]

What is the status of a the new sync logic @saad-ali ?

The new attach detach controller should be in for v1.3 Code Complete May 20th.

[saad-ali]

This PR will be obsolete with #25888 and #26801. Recommend closing this and waiting for the suggested fix: adding a mechanism to attach detach controller to query storage providers for orphaned attached volumes.

[jsafrane]

@saad-ali is right, I'm closing this PR