Change fsType to vhdFsType. Remove fsType in parameter support #486
Conversation
k8s-ci-robot
added
kind/cleanup
|
/cc @msau42 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Jiawei0227 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
This might only be compatible with csi-provisioner:v2.0 because you can specify default-fsType there... |
|
This is more of a POC. I think we need to at least rename it before GA |
| @@ -90,8 +90,8 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest) | |||
| fileShareName = v | |||
| case diskNameField: | |||
| diskName = v | |||
| case fsTypeField: | |||
| fsType = v | |||
| case vhdFsTypeField: | |||
There was a problem hiding this comment.
we may still need to support the original fstype parameter if backwards compatibility is important. Will let @andyzhangx decide.
There was a problem hiding this comment.
Yeah, I think we need to get rid of them before it goes to 1.0, but yeah... let's see
There was a problem hiding this comment.
I think we still need to make backwards compatibility, is it possible to keep this fsType, e.g. we have two places to set this field: 1) csi.k8s.io/fstype 2) fstype: ext4
if `csi.k8s.io/fstype` != "" {
fsType = `csi.k8s.io/fstype` setting
} esle {
fsType = `fstype` setting
}
Azure disk driver also has same issue, it's better not rename or remove existing parameters.
There was a problem hiding this comment.
Hmm, so I think at least one thing important is that we should read fsType from VolumeCapability instead of parameters.
And I feel like use fsType to decide when to use VHD over normal Azure file can be confusing. Would it possible to add a field vhdFsType and put that as the recommended field to set? And we can support fsType at the same time. But remove it once the driver wants to promote GA
There was a problem hiding this comment.
And another reference:
Here is how gce-pd-csi-driver parses the fsType:
https://github.com/kubernetes-sigs/gcp-compute-persistent-disk-csi-driver/blob/2f5ea47b054d091c9c97362f2e48721a7f8de698/pkg/gce-pd-csi-driver/node.go#L306
There was a problem hiding this comment.
looks like we have 3 parameters now:
parametes.fsTypeparametes.vhdFsTypecsi.k8s.io/fstype
if we want to use parametes.vhdFsType, then had better not use csi.k8s.io/fstype since the naming is different, another option is only use:
parametes.fsTypecsi.k8s.io/fstype
It's still open to discuss. Thanks.
There was a problem hiding this comment.
Unfortunately because of kubernetes-csi/external-provisioner#328, any PV's fstype is going to default to ext4 even if nothing was specified in the StorageClass.
That means existing PVs are going to have fstype ext4 by default, even if they were using cifs. So I think to retain backwards compatibility, we cannot use 'csi.k8s.io/fstype' to determine if something should be vhd or not. We need a specific parameter for vhd which then determines how we interpret the 'csi.k8s.io/fstype' field.
There was a problem hiding this comment.
Hmm I think my point is that regardless how fsType are passing from parameters. The fsType should only be decided by VolumeCapabilities. And that is the way moving forward for CSI spec compatibility. Currently Azuredisk and Azurefile does not seem to read from VolumeCapabilities to get fsType.
There was a problem hiding this comment.
it's better that only add vhdFsType in this PR, and keep fsType as compatible, and due to this issue(kubernetes-csi/external-provisioner#328), csi.k8s.io/fstype could be useless now since it's always ext4 by default.
There was a problem hiding this comment.
The external provisioner issue has been fixed in 2.0. However existing PVs are going to have PV.fstype set to ext4
| @@ -117,16 +121,13 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest) | |||
| enableHTTPSTrafficOnly := true | |||
| shareProtocol := storage.SMB | |||
| var vnetResourceIDs []string | |||
| if fsType == nfs || protocol == nfs { | |||
| protocol = nfs | |||
| if protocol == nfs { | |||
There was a problem hiding this comment.
just for my own learning, when should nfs be used vs cifs?
There was a problem hiding this comment.
@andyzhangx I also want to learn :)
There was a problem hiding this comment.
Azure file provides both SMB(by default) and NFS protocols
There was a problem hiding this comment.
Yeah, but is there a recommendation on when to use NFS over cifs? Normally how should user decides which one to use?
There was a problem hiding this comment.
currently cifs is the default protocol
|
@Jiawei0227: PR needs rebase. 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. |
k8s-ci-robot
added
the
needs-rebase
|
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-contributor-experience at kubernetes/community. |
k8s-ci-robot
added
the
lifecycle/stale
|
@Jiawei0227: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
|
Stale issues rot after 30d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-contributor-experience at kubernetes/community. |
k8s-ci-robot
added
lifecycle/rotten
|
Rotten issues close after 30d of inactivity. Send feedback to sig-contributor-experience at kubernetes/community. |
|
@k8s-triage-robot: Closed this PR. In response to this:
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. |
What type of PR is this?
/kind cleanup
/kind api-change
What this PR does / why we need it:
This PR remove fsType in the parameter supports as we also have 'csi.k8s.io/fstype' field in storageclass which can be confusing to users.
Instead, we rename it to vhdFsType.
Also, this PR remove
fsType: nfssupports so whoever wants to use it need to set protocol instead of fsType.Add mfsymlinks as the default mountOptions
Which issue(s) this PR fixes:
Fixes # #485 #484
Special notes for your reviewer:
This is a breaking change so we might need to be more cautious about this! fsType in parameters will not be supported anymore.
Release note: