Fix volume remount on reboot

[chakri-nelluri]

What this PR does / why we need it:
Check the mount is actually attached & mounted before marking actual state of world of Kubelet reconciler.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #51982

Special notes for your reviewer:
Added explicit check to make sure volumes are attached and are mounted before marking the state in actual state of world.

Release note:

Fix Flexvolume/FC/ISCSI volumes not being mounted after Node reboot.

[chakri-nelluri]

/cc @jingxu97 @saad-ali @rootfs

[jingxu97]

For flex volume, what this will return?

[chakri-nelluri]

It returns the mount path where the volume is expected to be mounted. It is static for a volume.

[mtaufen]

/unassign
/assign @jingxu97 @saad-ali

[jingxu97]

I think we can put the check of mountpoint into reconstructVolume function. If it is not a mountpoint, fail to reconstruct volume directly. This way, no need to add deviceMountPath into the reconstructedVolume data structure

[chakri-nelluri]

Isn't it a odd place for doing a mount check? mount check has nothing to do with reconstruction right?

[jingxu97]

the reason for reconstruction is mainly because when kubelet restarts, we don't have the cached state information. If there are mounts left on the node, we should reconstruct it. But if we checked that the mounts no longer exist, we can ignore it without reconstructing the volume back to actual state, I think.

[chakri-nelluri]

Ok. It is infact then "reconstructActualStateofVolume". It would work, the only minor concern is failing reconstruction because a volume is not mounted. Volume is an object and mount is one of the intermediate states of volume. We are combining both.

[jingxu97]

yeah, the function name is not very clear. I am fine with changing it to reconstuctActualStateofVolume. We have to reconstruct the spec because it is needed in the actual state.

[jingxu97]

@chakri-nelluri After checking the code in other volume plugins for ConstructVolumeSpec, I am thinking we could follow the same way by calling mounter.GetDeviceNameFromMount(), if no device is returned, constructVolumeSpec failed because it shows that the global mount path is no longer here.
Does it make sense?

[chakri-nelluri]

@jingxu97 IMHO it would make sense for this to be part of the infra, rather than having each plugin do the same logic. Also the constructVolumeSpec by the name goes, it just a helper function to reconstruct the spec. Other plugins, my assumption are working by chance as they require to check the mount to construct the volume spec.

@saad-ali @rootfs @childsb any preference?

[ailusazh]

What about just comment out these lines of the code, as follows:

		//if volume.pluginIsAttachable {
		//	err = rc.actualStateOfWorld.MarkDeviceAsMounted(volume.volumeName)
		//	if err != nil {
		//		glog.Errorf("Could not mark device is mounted to actual state of world: %v", err)
		//		continue
		//	}
		//}

In this way, the reconciler in volumemanager will remount all the attachable volumes. And for the situation of just restart kubelet and didn't reboot the node , when reconciler try to remount, the method MountDevice in attacher and SetUp in mounter will check whether the dir is mounted, if already mounted, they will return nil and the mount volume operation will be succeed.

[dixudx]

What about just comment out these lines of the code

@ailusazh They are useful. Just commenting out these lines will not help fix the remount issue, but may bring in other potential ones.

[ailusazh]

chakri-nelluri's fixed way considered more, but this will cause the unmounted volumes reattached, what abount reattach failed?

[ailusazh]

@dixudx, can you think of any specific potential problems?

[msau42]

/cc @kubernetes/sig-storage-pr-reviews

[chakri-nelluri]

@ailusazh Even, if we just comment out that code, it will not to remount the same volume, as attach & remount are always kicked from the same operation.

The initial PR has a check just about this code block to verify whether the volume was actually mounted or not, before marking it mounted. On @jingxu97 initial feedback, we decided to fail the actual reconstructVolume call, in case a volume is not mounted.

[chakri-nelluri]

@ailusazh When attachdetach controller is enabled we would not reattach the volume. When attachdetach controller is disabled, volumes are reattached on node restart, which was always the case. The code does not change the behavior for Kubelet restart.

[jingxu97]

The logic behind reconstructing desired and actual state supposed be to in the following sequence

1. When controller starts, reconstruct the desired state first by listing all the pods and their volumes.
2. After desired state is filled, reconstruct volume in actual state only if the volume is not in desired state, because controller might not have enough information to fully reconstruct the actual state. Only if the volume is no longer in desired state, reconstruct with partial information is ok because volume tear down does not really need the full information. The reconstruction process varies with different volume plugins, some might check mount points because they need this information, some don't.

But because of another race condition issue related to node status update, the current code is not really implemented as above design. In the first round of syncState, even if the volume is in desired state, reconciler still tries to reconstruct the actual state with partial information. see code https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/volumemanager/reconciler/reconciler.go#L394

I feel this is not good and cause the issue you are trying to fix here. I am thinking to use a separate cache to represent what is discovered from the disk instead of directly updating the actual state. I need to think through and have a proposal for it.

@chakri-nelluri Your approach checks the global device mount path, but it is also possible that global device path exists, but bind mount is gone, I think. To fully solve this issue, we might also need to check the bind mount.
For now I am ok with your change, just a small comment. Otherwise lgtm. It would be very nice if we could have an e2e test for it.

[jingxu97]

Could you add a comment for this part of checking?

[jingxu97]

Also move this part above the defination of volumeMounter?

[chakri-nelluri]

Ack

[ailusazh]

Understand, thank you for your explanation.

[k8s-github-robot]

/test all [submit-queue is verifying that this PR is safe to merge]

[k8s-github-robot]

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here..

[chakri-nelluri]

We should cherrypick the fix to 1.6 & 1.7 releases too.

[chakri-nelluri]

@jingxu97 @saad-ali Please target it for 1.6 & 1.7 patch releases too. Thanks.

[saad-ali]

Marked as 1.7 cherry pick candidate. Once that's done we can do the same for 1.6

[saad-ali]

CC @wojtek-t, 1.7.x release manager for approval

[wojtek-t]

Cherrypick approved - cherrypick in #53368

[k8s-cherrypick-bot]

Commit found in the "release-1.7" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

[saad-ali]

Marked as 1.6 cherry pick candidate.

[k8s-cherrypick-bot]

Commit found in the "release-1.6" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.