feat: controller only expansion

[jakobmoellerdev]

This change is prototyping how using the native LVM device paths reported by device mapper could work together with lvm controlled filesystem resizing in order to run controller-only online/offline volume expansion.

The main benefit of this is significantly faster resizing due to not having to wait for kubelet expansion. Additionally we no longer have to maintain our own devices in /dev/topolvm. (fix #902)

Note that the current design breaks with existing TopoLVM because it switches from using the TopoLVM device directory to using the lvm owned device paths (so TopoLVM no longer renames the devices). Unmounting via lvm2 provided device paths always works even if legacy paths exist.

I want to drive a POC here so that I can further debug and showcase how much more efficient we could be regarding dealing with device paths, lvm compatibility and the CSI spec. I think this approach is feasible and I would like to open it for reviews

Summary of Changes:

• Change all ControllerExpandVolumeResponses to NodeExpansionRequired: false
• Extend lvmd with resize behavior instead of NodeExpansion
• Allow lvmd to temporarily do a mount for resizing of PVCs under /tmp when there is currently no mount for the filesystem, otherwise reuse the mount present.

[toshipp]

Since I've not concluded avoiding NodeExpandVolume yet, I'd like to separate this PR into making expansion faster and not using /dev/topolvm.

[jakobmoellerdev]

How would you be able to achieve that? I am combining these since you need a path in the Controller and I would have to migrate the entirety of /dev/topolvm device logic into the controller first otherwise

[toshipp]

IIUC, all logic on node.go you wrote is valid if lvmd tells us lv device paths. So we could review the decommission of /dev/topolvm first.

[jakobmoellerdev]

Understand what you mean now, will work on separating this out, might take a bit

[github-actions]

This pull request has been automatically marked as stale because it has not had any activity for 30 days. It will be closed in a week if no further activity occurs. Thank you for your contributions.

[jakobmoellerdev]

@toshipp @peng225 I have now extracted the device changes out into the recently merged PR and rebased on main. The only changes left are now the controller based expansion. PTAL and let me know what you think of it.

[toshipp]

@jakobmoellerdev
Thank you for your update. We'll dig into the problem, but it will take time, IMO. Please wait for a while.

[toshipp]

@jakobmoellerdev
I am investigating the slow expansion issue and suspect kubelet's reconcile mechanism.
I have found a way to speed up e2e testing. Would you like to speed up e2e tests? Or the real environment?

[jakobmoellerdev]

The slow expand is because the kubelets desired state of world populator, but my issue is that we should not need to wait for the node if we already interact with lvmd, hence my PR.

So my desire is to speed up both. Real life expands take as much as testing

[toshipp]

Thanks.
I think it is not a good idea to abuse ControllerExpandVolume to resize a filesystem. I found that mutating a pod makes kubelet work faster, it might be a better solution. Maybe this should be solved by the kubernetes project. I am looking into it more.

[jakobmoellerdev]

I think your concern is valid. Let me know if you need any input from me to proceed.

[toshipp]

@jakobmoellerdev
I tried to solve this problem by kubelet watching PVC, but the attempt failed because the authors who implemented online resizing of PV had decided not to watch PV/PVC directly, but to use the existing pod resync loop. see kubernetes/community#1535 (comment).
So now I agree with the idea to solve it in topolvm. But as I mentioned before, I think the PR is not good because it abuses ControllerExpandVolume where CSI spec does not expect to resize a filesystem.
I tested another solution to notify kubelet by mutating pods on #956, it worked. I'd like to proceed with this, do you have any comments?

[jakobmoellerdev]

But as I mentioned before, I think the PR is not good because it abuses ControllerExpandVolume where CSI spec does not expect to resize a filesystem.

I think this assumption is not quite correct. ControllerExpandVolume is abstracted from filesystem management. Thats why one can set NodeVolumeExpansionRequired to true.

Nevertheless I think for code simplicity I agree, we can move forward with "nudging" the pod to do what we want, as it maintains our code paths better. I suggest we add some documentation to that pr and close this one out

[jakobmoellerdev]

Closing in favor of #956