longhorn: [BUG] Unable to backup volume after NFS server IP change

I discussed with @derekbit the action items below.

• When ensuring the mount point, check the specific mount point instead of scanning the whole mount table. https://github.com/longhorn/backupstore/pull/129
• There are two places to create a backup target mount point (in replica and longhorn-manager), so when reconciling the backup target setting change, go clean up the old mount points from longhorn-manager. (@ChanYiLin has done the cleaning when the backup target is unset, so we probably can consolidate both) @derekbit will work on this.

This issue is required to fix in 1.4.2.

Yeah, I think the cases are covered. Changing hard to soft mode makes error handling more complicated.

Yeah, I think the cases are covered. Changing hard to soft mode makes error handling more complicated.

But it will solve potential performance issues, so it still deserves it. We just need to ensure the coverage w/o regression.

Yes.

I still want to clarify a bit. When doing unmount, we can just umount it no matter what type of mount, right?

EnsureMountPoint checks if the mount point is valid. If it is invalid, clean up mount point. So, the purpose is to make sure the mountpoint’s type is expected and mountpoint is accessible, so it is not just do blindly unnmout.

Yes, it is my fix. But Don't use filesystem.GetMnt(). Use os.statfs instead is still necessary, because we need to check if the filesystem type is expected.

I see, that makes sense. However, one question, right now we only have one backup target, so if blindly clean up the mount point in this case w/o checking the filesystem type, will be there any side effects?

In longhorn/backupstore@149c9aa3 fix, it’s different from this scenario, because it’s to clean up mount points if the backup target is reset, so it can blindly cleanup w/o any checks.

For v1.4.x, the check of filesystem type is not needed, because it only supports nfs. But v1.5+, it is a must, because nfs and cifs use the same mountpoint.

I will do two improvements

• Don’t use filesystem.GetMnt(). Use os.statfs instead
• Move the cleanup logic to longhorn-manager like [IMPROVEMENT] Clean up backup target if the backup target setting is unset #5655 WDYT?

I mean can we just clean up it directly? since cleanupMount is the best effort?

	// mnt, err := filesystem.GetMount(mountPoint)
	// if err != nil {
	// 	return true, errors.Wrapf(err, "failed to get mount for %v", mountpoint)
	// }

	// if strings.Contains(mnt.FilesystemType, Kind) {
	// 	return true, nil
	// }

	log.Warnf("Cleaning up the mount point %v because the fstype %v is changed to %v", mountPoint, mnt.FilesystemType, Kind)

	if mntErr := cleanupMount(mountPoint, mounter, log); mntErr != nil {
		return true, errors.Wrapf(mntErr, "failed to clean up mount point %v (%v) for %v protocol", mnt.FilesystemType, mountPoint, Kind)
	}

	return false, nil

Yes, it is my fix. But Don't use filesystem.GetMnt(). Use os.statfs instead is still necessary, because we need to check if the filesystem type is expected.

This is due tofilesystem.GetMnt() in https://github.com/longhorn/backupstore/blob/master/util/util.go#L316. GetMnt() iterates and gets information of all mount points in the mount table. If there is any dead mount point, the iteration will hang for a while, so the caller (backup ls) will run into a timeout error in the end.