This does not seem correct, so if NodeUnpublish fails then subsequent calls to NodeUnpublish won't even start because directory and json files has already been removed.

I do not think fixing it here is the right solution but we need to think it from perspective of how cleanup should work if drivers are not yet available or volume is not yet mounted. IMO - the fix has to be made in reconstruction code, not here.

Actually, csi mount directory will be removed anyway by housekeeping handler in removeOrphanedPodVolumeDirs, this function will even remove entire pod volume dir, which is /var/lib/kubelet/pods/<POD_UID>/volumes, the reason why the directory was not deleted sucessfully and left over is that there is a file called vol_data.json in it, and this function can only delete directory but when any file exists, it returns error. So here I put removeMountDir before calling out csi driver to make sure vol_data.json is cleaned up no matter NodeUnpublish sucess or not, in this way, the orphan pod volume will be cleaned up by housekeeping handler eventually.

Anyway, the goal is to clean up orphan Pod volume, if we can make it by this simple way, there is no need to make things complicated.

I agree with @gnufied , this doesn't seem like the right place to fix it.
What happens when the directory is still mounted? Removal of the directory would fail then but with this change it means TearDownAt would return an error before calling NodeUnpublishVolume to unmount the volume, and it would never get unmounted. So I think this breaks the main use case (unmount volume and then remove the mountpoint).

Actually, when the directory is still mounted, removeMountDir won't return error. It is still unmounted by csi driver first. Every cleaning up action in kubelet should have considered the mount state in the first place to ensure the safety.

Considering that csi driver is responsible for unmounting and cleaning the mount directory, which also mentioned in the comment, maybe we just only remove vol_data.json here, which should be done by kubelet, rather than remove the mount dir.

@gnufied @dobsonj I agree that we should not break the original logic of TearDownAt. And I update the PR. This time I put the deletion of the volume data file in kubelet housekeeping process, which is responsible to clean up the orphan Pod volume. Could you PTAL?

This change actually re-introduces a rather serious race condition where vol_data.json may be deleted by removeOrphanedPodVolumeDirs before NodeUnstageVolume has a chance to run, which causes unmount to fail and kubelet ends up not being able to clean the mountpoints on the node: #101911
So this is not the right place to remove vol_data.json, it will result in unmounts failing in some scenarios.
You may want to take a look at the volume reconstruction code in volumemanager. This is a tough bug in a complex part of the code, and the fix is not obvious to me, but when I saw it before syncStates failed here because the /mount subdir did not exist. Then it calls cleanupMounts but that failed because the volume failed to be reconstructed and it couldn't create the unmounter at that point. If you're able to reproduce the problem, you can check if you're seeing the same failure scenario in kubelet.log.
IMO, we either need to be able to reconstruct the volume to unmount it in this scenario, or at least find some way to create the unmounter so that cleanupMounts can remove vol_data.json even when the /mount subdir does not exist. Sorry if this direction is still a little vague, but that's as far as I've been able to dig into this.

@dobsonj Thank you so much for the explanation. I am excited that there are still so many things I didn't know😄, and I will dig into it again to find a right solution.

@dobsonj @gnufied Actually, not only the volume directory /mount will be left over, the /globalmount directory will be left over either. Because in this scenario, the volume manager did neither NodeUnstageVolume nor NodeUnpublishVolume:

When reconstructing, the volume directory /mount is not mountpoint, so volume manager tries to do cleanup by calling NodeUnpublishVolume and cleanup the /mount directory and vol_data.json, but the csi driver did not register yet, which makes cleanup fails. Actually, no matter it succeeds or fails, NodeUnstageVolume won't be called, so /globalmount will be left over.

I think there may be two solutions:

1. Let the reconstruction success no matter /mount is mountpoint or not. After adding the volume to ASW, the volume manager's reconcile process will do NodeUnstageVolume and NodeUnpublishVolume later. However, I wonder if there is special reason for making reconstruction failing when mountpoint does not exist.
2. When the unmounter executes unmounting, clean up the directory and vol_data.json without calling NodeUnpublishVolume and NodeUnstageVolume if the directory is already not a mountpoint.

WDYT? Which one is better or any other idea?