feat(provisioning): extra format options (mkfs) added

[mhkarimi1383]

Pull Request template

Please, go through these steps before you submit a PR.

Why is this PR required? What issue does it fix?:

adding extra format (mkfs) options to newly created Volumes

also switched from deprecated utils/monut to mount-utils to be able to add this feature and moving from an old package to a maintained one

What this PR does?:

Refactor to new mount utils package and change format and mount function

Does this PR require any upgrade changes?:

No, But requires document updates for StorageClasses

If the changes in this PR are manually verified, list down the scenarios covered::

Any additional information for your reviewer? :
Mention if this PR is part of any design or a continuation of previous PRs

Checklist:

• Fixes Extra mkfs options #296
• PR Title follows the convention of <type>(<scope>): <subject>
• Has the change log section been updated?
• Commit has unit tests
• Commit has integration tests
• (Optional) Are upgrade changes included in this PR? If not, mention the issue/PR to track:
• (Optional) If documentation changes are required, which issue on https://github.com/openebs/openebs-docs is used to track them:

[dsharma-dc]

How is the new formatOptions going to be set by user? via storageclass? There is no mkfsOptions unlike mountOptions as provided by storageclass.
Please write a test as well to show how this is used.

[tiagolobocastro]

I think formatOptions need to be parsed from storage class at GetVolAndMountInfo ?

[mhkarimi1383]

Hi

formatOptions is added to MountInfo just like mountOptions

So, It will work if you add it to storageclass

[mhkarimi1383]

Also there is no test for the mounter itself

[dsharma-dc]

I think formatOptions need to be parsed from storage class at GetVolAndMountInfo ?

GetVolAndMountInfo is parsing and building mountinfo from csi node publish request, but I don't see anything for format options in csi request. So I wonder how that's going to work. The storageclass parsing is being done in NewVolumeParams that's part of controller plugin.
cc: @abhilashshetty04

[dsharma-dc]

Also there is no test for the mounter itself

You can run this manually and document here the steps to set formatOptions.

[mhkarimi1383]

I think formatOptions need to be parsed from storage class at GetVolAndMountInfo ?

GetVolAndMountInfo is parsing and building mountinfo from csi node publish request, but I don't see anything for format options in csi request. So I wonder how that's going to work. The storageclass parsing is being done in NewVolumeParams that's part of controller plugin.
cc: @abhilashshetty04

It's my first time that I'm contributing to this project so I just looked for where mountOptions is handled to handle formatOptions too

[mhkarimi1383]

Hello again

I'm using NixOS, and I can't run make, waiting on #351

[mhkarimi1383]

I think formatOptions need to be parsed from storage class at GetVolAndMountInfo ?

I checked out more and I saw that we have to put this on where we are mounting volume for the first time, So there is no need to add this into GetVolAndMountInfo, we have to handle that just like mountOptions

[mhkarimi1383]

Not able to test due to #353 and #352

[dsharma-dc]

@mhkarimi1383
Also, please refer to the change https://github.com/openebs/zfs-localpv/pull/542/files . Something similar is likely required here also for these formatOptions to be usable.
cc: @abhilashshetty04 @Abhinandan-Purkait

[mhkarimi1383]

@dsharma-dc

golint-ci returns error

pkg/driver/controller.go:338:6: declared and not used: vol (typecheck)
	var vol *lvmapi.LVMVolume
	   ^
pkg/lvm/mount.go:31:2: could not import k8s.io/mount-utils (-: cannot compile Go 1.22 code) (typecheck)
	"k8s.io/mount-utils"
	^
pkg/lvm/mount.go:73:14: undefined: mount (typecheck)
	mounter := &mount.SafeFormatAndMount{Interface: mount.New(""), Exec: utilexec.New()}
	           ^
pkg/lvm/mount.go:97:14: undefined: mount (typecheck)
	mounter := &mount.SafeFormatAndMount{Interface: mount.New(""), Exec: utilexec.New()}
	           ^
pkg/lvm/mount.go:99:19: undefined: mount (typecheck)
	dev, ref, err := mount.GetDeviceNameFromMount(mounter, targetPath)
	                ^
make: *** [Makefile:263: golint] Error 1

I think to update to k8s.io/mount-utils we have to update go version

[dsharma-dc]

@dsharma-dc

golint-ci returns error

pkg/driver/controller.go:338:6: declared and not used: vol (typecheck)
	var vol *lvmapi.LVMVolume
	   ^
pkg/lvm/mount.go:31:2: could not import k8s.io/mount-utils (-: cannot compile Go 1.22 code) (typecheck)
	"k8s.io/mount-utils"
	^
pkg/lvm/mount.go:73:14: undefined: mount (typecheck)
	mounter := &mount.SafeFormatAndMount{Interface: mount.New(""), Exec: utilexec.New()}
	           ^
pkg/lvm/mount.go:97:14: undefined: mount (typecheck)
	mounter := &mount.SafeFormatAndMount{Interface: mount.New(""), Exec: utilexec.New()}
	           ^
pkg/lvm/mount.go:99:19: undefined: mount (typecheck)
	dev, ref, err := mount.GetDeviceNameFromMount(mounter, targetPath)
	                ^
make: *** [Makefile:263: golint] Error 1

I think to update to k8s.io/mount-utils we have to update go version

Would you like to do that as part of this PR? or instead try using a mount-utils that might work with go 1.19.

[mhkarimi1383]

I can do both, whatever is Ok :)

[dsharma-dc]

I can do both, whatever is Ok :)

Thanks, I'd say then try using a compatible mount-utils version(I hope no other issues pop up), and Go upgrade can be taken up as a new PR.

[mhkarimi1383]

@dsharma-dc

Ok, I will revert my changes to go.mod, etc. And will change the package again

[mhkarimi1383]

I have fixed package versions and switched to a correct version of mount utils

And I was able to build project successfully and tests passed

[mhkarimi1383]

@abhilashshetty04, @tiagolobocastro

I have done with tests ;)

[mhkarimi1383]

Also it's good to make tests to run in parallel in CI I think

[abhilashshetty04]

Also it's good to make tests to run in parallel in CI I think

Lets do a separate PR for that. We dont have bors or similar utility inplace for that right now in localpv repo.

[mhkarimi1383]

Also it's good to make tests to run in parallel in CI I think

Lets do a separate PR for that. We dont have bors or similar utility inplace for that right now in localpv repo.

Yeah we should check if we don't have any naming conflicts, etc in resources that are creating in K8s but ginkgo has support for parallel tests

[abhilashshetty04]

Also it's good to make tests to run in parallel in CI I think

Lets do a separate PR for that. We dont have bors or similar utility inplace for that right now in localpv repo.

Yeah we should check if we don't have any naming conflicts, etc in resources that are creating in K8s but ginkgo has support for parallel tests

Got it. We definately check it out.

[abhilashshetty04]

Thanks, Please squash commits before merging @mhkarimi1383

[mhkarimi1383]

Thanks, Please squash commits before merging @mhkarimi1383

I will

[mhkarimi1383]

@abhilashshetty04
Squshed.

I had to recreate branch and apply diff of the PR, Because of some merge commits, etc.

[dsharma-dc]

anything pending before merge here, @mhkarimi1383 @abhilashshetty04 ?

[mhkarimi1383]

anything pending before merge here, @mhkarimi1383 @abhilashshetty04 ?

I'm Done with this PR, LGTM

[mhkarimi1383]

I can Also add a Test case for xfs using it's block size option, But IDK if xfs_info is available to the pods or not

[abhilashshetty04]

Can you please rebase @mhkarimi1383

[mhkarimi1383]

@abhilashshetty04 Rebased

[dsharma-dc]

@mhkarimi1383 Could you please update the documentation here: https://github.com/openebs/lvm-localpv/blob/develop/docs/storageclasses.md , based on your changes?

[mhkarimi1383]

@dsharma-dc I will

[mhkarimi1383]

@dsharma-dc

Covered in
#396