VolumeAttachment not marked as detached causes problems when the Node is deleted.

[msau42]

In #184, we had decided that instead of marking the VolumeAttachment as detached, we would just requeue the volume to have the workqueue process it again.

However, this doesn't work in the case where the Node is deleted. In that scenario:

1. ListVolumes() shows that volume is not attached to the node anymore
2. ReconcileVA() sets force sync
3. syncAttach() just tries to reattach the volume again and fails because node is gone
4. In k/k AD controller, we try to attach to new node, but it fails on the multi-attach check because volume is still attached in asw.

What should happen is:

1. ListVolumes() shows that volume is not attached to the node anymore
2. We actually mark VolumeAttachment.status.attached as detached
3. In k/k AD controller, VerifyVolumesAttached() sees that VolumeAttachment is detached, updates asw
4. AD reconciler allows new Attach on new node to proceed.

I'm not sure the best way to fix step 2). Some suggestions I have in order of preference:

1. We go back to actually updating VolumeAttachment in ReconcileVA() like the original PR did. But we call markAsDetached to make sure we update everything properly.
2. We pass some more state to syncVA() so that it can markAsDetached if csiAttach failed on the force sync.

[msau42]

cc @jsafrane

[msau42]

/kind bug

[jsafrane]

What should happen is:

1. ListVolumes() shows that volume is not attached to the node anymore
2. We actually mark VolumeAttachment.status.attached as detached

The volume might be already attaching. A/D controller does not know about that.

3. In k/k AD controller, VerifyVolumesAttached() sees that VolumeAttachment is detached, updates asw

It's not possible to distinguish attaching from attached there.

4. AD reconciler allows new Attach on new node to proceed.

IMO, A/D controller (or csi plugin) should see the Node does not exist and delete VolumeAttachment.

[msau42]

Do you see a problem if we mark VolumeAttachment as detached while it's still attaching? How is this different from the normal case when we are first attaching a volume and it's VolumeAttachment.status.attached = false?

[jsafrane]

From external provisioner POV, it should be safe. In the worst case when both regular sync and ListVolume sync race, it marks just attached volume as detached. This causes VolumeAttachment to be synced again, ControllerPublish will be called and it will fix the VolumeAttachment status.

Still, something in the A/D controller / CSI plugin there must check the destination node is gone and delete VolumeAttachment and update ASW.

[msau42]

When a node is deleted, the combination of pods getting deleted and AD.verifyvolumesattached causes the detach

[msau42]

I forgot to mention in the initial comment, the Pod does get deleted, which should trigger detach, however AD controller doesn't because it still sees the volume mounted in asw and says it's not safe to detach. This also starts the 6 minute force detach timer.

It depends on VerifyVolumesAttached to override asw.

[jsafrane]

It starts making sense now.

I'm thinking if it can break anything in A/D controller. If A/D VerifyVolumesAreAttached finds a volume that is detached and the pod does not exist, it will remove the pod from ASW. Who is going to delete the VolumeAttachment then? CSI plugin.Detach is not called... Should A/D controller call it?

[msau42]

Looks like VolumesAreAttached calls DeleteVolumeNode, which completely removes it from asw, so you're right, Detach won't get called and VolumeAttachment will be leaked. One possibility is to not completely remove it from asw, but mark some special state.

Another possibility is we add VolumeAttachment GC to the AD: kubernetes/kubernetes#77324

[msau42]

Another thought, should VolumeAttachment have ownerref to Node object so when the Node object gets deleted, we automatically trigger Detach?

[jsafrane]

I'd prefer to have a proper fix instead of piling up hacks in A/D controller & external-attacher.

Node object is deleted. PodGCController deletes pods from the node. Should it delete VolumeAttachments too? That looks too CSI specific... Or should we add loop to A/D controller to force-detach from really deleted nodes sooner that in 6 minutes? This will work for all volume plugins and we don't need ListVolumes / VerifyVolumesAreAttached at all.

How probable is it that the deleted node comes back and how quickly? If someone accidentally deletes a Node object (keeping kubelet running), how quickly is it re-created? PodGCController has 40s "quarantine".

[msau42]

VerifyVolumesAreAttached is still needed for other scenarios where node still exists but volume got detached out of band, or node gets recreated with the same name.

The other thing I realized is that this problem only occurs if the Node is ungracefully deleted (without a drain).

If someone deleted the Node object, kubelet only recreates it if it restarts.

I think we should still let Pod GC invoke detach. Right now it is guarded by the existence of node, which is guarded by this check. I'm not sure what problems may happen if we delete the node from cache while we think pods are still attached to it. The other problem is that nodeDelete is only called via informer. If it fails that one time, then we never retry removing the node from cache again.

[msau42]

I discussed a bit with @saad-ali and he has concerns about using the volumeattachment.status.attached field to indicate that a volume is detached because it could also mean attach or detach could be in progress. If we want to fix this, we may need to think about having a new field that can actually indicate something is detached with nothing in progress.

Also, we probably should revisit the logic in csi.VolumesAreAttached that uses the status.attached field, and also revisit other cases where we return attached = false. That causes the volume to be removed from asw. Could that be problematic if it's actually still attached?

[jsafrane]

concerns about using the volumeattachment.status.attached field to indicate that a volume is detached because it could also mean attach or detach could be in progress

The only signal that a volume is fully detached is that VolumeAttachment is deleted / missing and IMO only this should be used by A/D controller to check that the volume is fully detached. When VolumeAttachment exists, the volume may be attaching / detaching / fully attached.

We could add "detached and can't attach because node is gone" to VolumeAttachment, but that is safe and race free only until the node appears again.