unmountKubeletDirectory should return err instead of nil if unmounting fails

[SituationNormal]

https://github.com/kubernetes/kubernetes/blob/9e644106593f3f4aa98f8a84b23db5fa378900bd/cmd/kubeadm/app/cmd/phases/reset/unmount_linux.go#L52

unmountKubeletDirectory currently returns nil if it encounters an error unmounting. However, runCleanupNode relies on this code to return an error if there's a problem so it doesn't wipe out the kubelet run directory. In this case, if there's an error unmounting since unmountKubeletDirectory returns nil, runCleanupNode will wipe out the kubelet run directory, including any mounted data that may be pointing to an NFS/persistent storage or something from the kubelet/pods directory.

/sig k8s-infra

[k8s-ci-robot]

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[SituationNormal]

/sig k8s-infra

[SituationNormal]

/sig node

[neolit123]

/remove-sig node k8s-infra
/transfer kubeadm

[neolit123]

https://github.com/kubernetes/kubernetes/blob/9e644106593f3f4aa98f8a84b23db5fa378900bd/cmd/kubeadm/app/cmd/phases/reset/unmount_linux.go#L52

unmountKubeletDirectory currently returns nil if it encounters an error unmounting. However, runCleanupNode relies on this code to return an error if there's a problem so it doesn't wipe out the kubelet run directory. In this case, if there's an error unmounting since unmountKubeletDirectory returns nil, runCleanupNode will wipe out the kubelet run directory, including any mounted data that may be pointing to an NFS/persistent storage or something from the kubelet/pods directory.

i think you have a point.
if we are going to refactor this we have to try to keep reset as a "best practice". thatt is the principle it tries to follow.

please send a PR if you want. also let's see if others have an opinion.

[SataQiu]

unmountKubeletDirectory is a NOOP on all but linux

So unmountKubeletDirectory is just best-effort, and users performing kubeadm reset usually means that they want to clean up the existing data on the node, right?
If users want to keep the data, I think they need to make a backup before executing kubeadm reset.

[huww98]

So unmountKubeletDirectory is just best-effort, and users performing kubeadm reset usually means that they want to clean up the existing data on the node, right? If users want to keep the data, I think they need to make a backup before executing kubeadm reset.

kubeadm reset can be a routine process of removing a node from cluster. It can also be part of an automated process. I think the user would not expect their (network attached) persistent volume to be wiped just because it happens mounted on this node.

[pacoxu]

unmountKubeletDirectory is a NOOP on all but linux

So unmountKubeletDirectory is just best-effort, and users performing kubeadm reset usually means that they want to clean up the existing data on the node, right? If users want to keep the data, I think they need to make a backup before executing kubeadm reset.

+1 for this point.

However, the change in kubernetes/kubernetes#120377 will make the reset safer, which is acceptable to me.

[chendave]

I think the user would not expect their (network attached) persistent volume to be wiped just because it happens mounted on this node.

+1, user might not aware how the PV is mounted in a pods, just wipe it might not appropriate.