Add FSType for CSI volume source #58209
Conversation
k8s-ci-robot
added
release-note
k8s-github-robot
added
the
kind/api-change
|
cc @kubernetes/sig-storage-api-reviews |
k8s-ci-robot
added
the
sig/storage
|
/approve |
pkg/volume/csi/csi_mounter.go
Outdated
| @@ -197,7 +197,7 @@ func (c *csiMountMgr) SetUpAt(dir string, fsGroup *int64) error { | |||
| accessMode, | |||
| c.volumeInfo, | |||
| attribs, | |||
| "ext4", //TODO needs to be sourced from PV or somewhere else | |||
| csiSource.FSType, | |||
There was a problem hiding this comment.
Comment above says that Implicitly inferred to be "ext4" if unspecified. We need some code for that if csiSource.FSType is empty.
There was a problem hiding this comment.
I saw that in drivers repo, we would defaults FSType to ext4 in function formatAndMount if it was not set. Should we deal with this here ?
btw, when this gets merged, i will update the external provisioner to add FSType for newly created PV if it is set in StorageClass parameters.
There was a problem hiding this comment.
We don't have control about all CSI drivers in the wold to fall back to ext4 when nothing is specified. Other drivers may choose a different default, so it's Kubernetes who should enforce ext4 here.
There was a problem hiding this comment.
done, thanks, PTAL
NickrenREN
force-pushed
the
csi-fstype
branch
from
373e00f to
37d4287
Compare
k8s-ci-robot
added
size/M
|
/assign @jsafrane |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/assign @thockin |
|
@NickrenREN last time I had to manually make type changes in staging/src/k8s.io/api/core/v1/types.go . You will need to make FSType change there as well. |
|
@vladimirvivien i have already changed that in my first commit |
|
/lgtm One more thing to consider for API review: Does this aligns with how we plan to expose block storage long term? Having the |
|
During the block design, the idea came up that it could be cleaner if fstype was in the PVC instead of per volume type, so that we could standardize fs handling across all volume types instead of having each plugin handle it. |
|
IIUC,
Do you mean
However, I believe changing this affects big(?) impact to all plugins, also we need keep backward compatibility, therefore I think we don't have a plan to change this. @erinboyd @screeley44 |
|
Yeah I meant putting the fsType in the higher level PV/PVC, instead of as a parameter in each volume-specific source. Then fs formatting and supporting scenarios like fs-on-block could be done by Kubernetes instead of every single plugin. But yes, this is a major change and would be messy when also considering backwards compatibility, so we just stayed with the current way of specifying fsType in every volume source spec. |
|
@saad-ali |
|
Sounds good to me. Will need to clean this up for Kubernetes 2.0! @thockin for API approval |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jsafrane, NickrenREN, saad-ali, thockin Associated issue: #58183 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
k8s-ci-robot
added
the
approved
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here. |
What this PR does / why we need it:
Add FSType for CSI volume source to specify filesystems (alpha defaults to
ext4)Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #58183
Special notes for your reviewer:
Release note:
/assign @saad-ali
cc @vladimirvivien