Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fsType check during NodePublishVolume will cause issues for volumes created with storageclass set to "" fstype #1025

Closed
jingxu97 opened this issue Jun 5, 2023 · 7 comments
Labels
kind/bug Categorizes issue or PR as related to a bug.

Comments

@jingxu97
Copy link
Contributor

jingxu97 commented Jun 5, 2023

/kind bug

What happened?

If useing a StorageClass without the parameter fstype, then the component external-provisioner will use the default one set through the flag defaultFsType which is ext4 [1], later the PV ends up setting pv.Spec.PersistentVolumeSource.CSI.FSType = ext4 in [2].

Because of [3] there's now a validation in the controller/node CSI calls to check that the req.VolumeCapabilities is setting a valid fstype which for the EFS CSI Driver is now defined as either ("efs" or "") in [3] supportedFSTypes = []string{"efs", ""}, therefore because the PV fstype is ext4 it fails the validation.

[1] https://github.com/kubernetes-csi/external-provisioner/blob/master/pkg/controller/controller.go#L626
[2] https://github.com/kubernetes-csi/external-provisioner/blob/master/pkg/controller/controller.go#L920
[3] #612

What you expected to happen?

Volume create with storageclass using empty fstype should not fail on mount

How to reproduce it (as minimally and precisely as possible)?

Create a storageclass with empty fstype, and create volume and a pod to use the volume

Anything else we need to know?:

Environment

  • Kubernetes version (use kubectl version):
  • Driver version:

Please also attach debug logs to help us better diagnose

  • Instructions to gather debug logs can be found here
@k8s-ci-robot k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Jun 5, 2023
@jingxu97
Copy link
Contributor Author

jingxu97 commented Jun 5, 2023

A possible solution to avoid checking the fstype during NodePublishVolume, and only check it during createVolume

@jingxu97
Copy link
Contributor Author

jingxu97 commented Jun 5, 2023

/cc @msau42 @wongma7 @animeshsingh

@msau42
Copy link

msau42 commented Jun 5, 2023

cc @jsafrane @RomanBednar

@wongma7
Copy link
Contributor

wongma7 commented Jun 5, 2023

I don't see where ext4 is set, https://github.com/kubernetes-csi/external-provisioner/blob/7eed97a6c45d9542f097adaed0c7c38becd7f0db/cmd/csi-provisioner/csi-provisioner.go#L101, and we don't set default-fstype in the template we ship https://github.com/kubernetes-sigs/aws-efs-csi-driver/blob/master/deploy/kubernetes/base/controller-deployment.yaml#L67, so could it be specific to your efs-csi-controller Deployment, could you double check?

@jingxu97
Copy link
Contributor Author

jingxu97 commented Jun 7, 2023

right our csi provisioner has a default setting using ext4 before, so all the existing PVs will carry this. To solve this issue, we are thinking to make this validation only work on createVolume phase. Is that a sufficient checking here?

@wongma7
Copy link
Contributor

wongma7 commented Jun 7, 2023

👍 I would be OK with that... it should fail open, efs driver should ignore fsType since it can assume every PV with driver=efs has efs filesystem, it's not like EBS which must check fsType and format volume accordingly.

(that said I cant do much to help implement/ review such a change )

@jingxu97
Copy link
Contributor Author

jingxu97 commented Jun 7, 2023

#1027

Thank you @wongma7 , I opened a PR, who do you think can help review? Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
Development

No branches or pull requests

4 participants