Make fstype configurable while starting the external-provisioner

[Madhu-1]

currently in external-provisioner fstype is set to ext4 if nothing is passed in storage-class, we have a requirement to make it configurable so that the SP who is using the external-provisioner can choose the default fstype which they want to have it. This will avoid always mentioning of the fstype in the storage-class.

in rbd csi plugin, we have seen better performance with xfs type, as external-provisioner is defaulting to ext4, in rbd csi we are asking users to mention the fstype as xfs in storage class. if fstype can be configured in external-provisioner user don't need to bother about the default fstype used.

CC @msau42

[Madhu-1]

@humblec FYI

[humblec]

/assign @msau42
/assign @jsafrane

[msau42]

I don't think fstype should be defaulted at all. There are filer types like nfs that expect a "" as the fstype

[humblec]

@msau42 As @Madhu-1 mentioned, fstype is set to ext4 in sidecar container if not passed in SC param https://github.com/kubernetes-csi/external-provisioner/blob/master/pkg/controller/controller.go#L437 , so the only way to get around this is via SC parameter which is actually on overhead as all SC has to set this field.

[msau42]

That doesn't seem right. Filer types like nfs/gluster/ceph should be failing when they validate fstype

[msau42]

See kubernetes/kubernetes#65122

[humblec]

Have to go through in detail, but at glance , CSI pv source has been corrected. This has to be corrected here too. Isnt it ?

[msau42]

Yes I don't think external provisioner should default. Now the question is will we break drivers that expect a non empty value? Need to double check CSI spec wording in this

[humblec]

Yes I don't think external provisioner should default. Now the question is will we break drivers that expect a non empty value? Need to double check CSI spec wording in this>

Yeah, we have to cross check on this, and also ( may be ) on backward compatibility of sidecar ?

[msau42]

/kind bug

[msau42]

I spoke with @jsafrane and he suggested we add a --default-fs flag, which defaults to ext4 and then immediately deprecate it. When we have a 2.0, we can remove the defaulting behavior.

Does this sound reasonable to you? Would you have time to work on this?

[jsafrane]

Well, we could have --default-fs flag, with "ext4" as default and then change the default to "" after a long enough "deprecation" period - the flag would not be removed, just its default would change.

[msau42]

I prefer having the external provisioner not default anything at all and leave it entirely up to the plugin

[humblec]

I spoke with @jsafrane and he suggested we add a --default-fs flag, which defaults to ext4 and then immediately deprecate it. When we have a 2.0, we can remove the defaulting behavior.

Sure.

Does this sound reasonable to you? Would you have time to work on this?

Sure, will work on this. thanks!

[msau42]

@humblec do you still have time to work on this?

[msau42]

/help

[k8s-ci-robot]

@msau42:
This request has been marked as needing help from a contributor.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

/help

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.

[humblec]

@humblec do you still have time to work on this?

@msau42 sure, let me put this in my todo list .

[SandeepPissay]

I agree with @msau42 that there should be no default FS type in external provisioner at all. The CSI driver can choose the default FS type based on various other factors like access mode and external provisioner should just pass fstype as empty string in create volume request.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[bswartz]

/remove-lifecycle stale

[msau42]

Fixed in #400
/close

[k8s-ci-robot]

@msau42: Closing this issue.

In response to this:

Fixed in #400
/close

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.

[RaunakShah]

@msau42 @humblec I have a question about this bug. Basically what we're saying is that the storage provider should have the capability of choosing their own fsType (based on their own criteria) and Kubernetes will honor that.

I have a bug where no fsType was provided (in the SC or provisioner) and our CSI driver defaulted to ext4. During NodeStageVolume, the volume is formatted - i can view the mount from the k8s node on which this pod was scheduled.

However, Kubernetes' internal state assumes that fsType is empty (presumably because provisioner didn't set anything while making the CreateVolume request). The result is that the securityContext provided in the Pod spec is not honored.

When i add the --default-fsType flag, this gets fixed.

I've provided more details (logs, ref code) here - kubernetes-sigs/vsphere-csi-driver#370 (comment)

Is that the expected behavior? I'm thinking there should be some mechanism for the CSI driver to communicate back to Kubernetes what fsType (if any) was used to format the volume.

[msau42]

Good find. That is indeed a problem for plugins that need fsgroup support. For now, I think the best approach is to set the --default-fstype flag. In 1.20, we're targeting promoting the csidriver fsgroup feature to beta, which lets drivers explicitly opt in to supporting fsgroup instead of using this rwo + fstype heuristic.

[humblec]

Agree with both of you :) .. In short if SC and Provisioner does not specify fsType with this version of this sidecar, we have to set the default-fstype flag.

[RaunakShah]

Thanks for the info! I'll keep a look out for the fsgroup feature and make a change to our YAMLs for now

[bswartz]

None of the solutions work particularly well for meta-CSI plugins like Trident, which support multiple protocols and therefore multiple default FS types. The ideal solution from our perspective would be to allow the SP to tell the CO which fstype it defaulted to when the CO didn't specify one. That requires a CSI spec change though and I don't see it happening soon if ever.

The workaround we're going with is to tell our users to not leave the fstype blank in their storage classes.

[msau42]

Yes, I think we should look into adding something to CSI. Windows has a similar issue, in that you may want to default to a different filesystem depending on which node the pod gets scheduled to. @jingxu97 is looking into it

[SandeepPissay]

We have the same issue that vSphere CSI supports multiple protocols and we default to a protocol based on the create volume request. We definitely need to enhance CSI spec to communicate back the fstype that was used by CSI driver back to CO.