Make fstype configurable in external provisioner #400

humblec · 2020-02-04T10:32:33Z

At present the fstype is set to ext4 if nothing is passed in storage-class.
However a SP could prefer to have different fstype for many reasons for their
driver/volumes. This patch enables SP who is using the external-provisioner
to choose the fstype which they want to have it.

Fix# #328

Signed-off-by: Humble Chirammal [email protected]

/kind bug

What this PR does / why we need it:

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

ACTION REQUIRED: The fstype on provisioned PVs no longer defaults to "ext4". A `defaultFStype` arg is added to the provisioner. Admins can also specify this `fstype` via storage class parameter. If `fstype` is set in storage class parameter, it will be used. The sidecar arg is only checked if fstype is not set in the SC param.

k8s-ci-robot · 2020-02-04T10:32:41Z

Hi @humblec. Thanks for your PR.

I'm waiting for a kubernetes-csi member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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 · 2020-02-04T10:35:28Z

@jsafrane @msau42 ptal .

humblec · 2020-02-04T10:35:59Z

/release-note-none

humblec · 2020-02-04T10:50:26Z

/assign @jsafrane
/assign @msau42

humblec · 2020-02-04T13:39:55Z

/ok-to-test

k8s-ci-robot · 2020-02-04T13:40:10Z

@humblec: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/ok-to-test

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.

cmd/csi-provisioner/csi-provisioner.go

jsafrane · 2020-02-05T11:51:05Z

/ok-to-test

cmd/csi-provisioner/csi-provisioner.go

pkg/controller/controller.go

xing-yang · 2020-06-11T15:42:15Z

/lgtm

cmd/csi-provisioner/csi-provisioner.go

msau42 · 2020-06-11T16:52:05Z

pkg/controller/controller.go

@@ -518,8 +519,8 @@ func (p *csiProvisioner) ProvisionExt(options controller.ProvisionOptions) (*v1.
 	if fsTypesFound > 1 {
 		return nil, controller.ProvisioningFinished, fmt.Errorf("fstype specified in parameters with both \"fstype\" and \"%s\" keys", prefixedFsTypeKey)
 	}
-	if len(fsType) == 0 {
-		fsType = defaultFSType
+	if fsType == "" && p.defaultFSType != "" {


Can you add unit tests that test the various combinations?

StorageClass set + default set

StorageClass unset + default set

StorageClass set + default unset

StorageClass unset + default unset

Added the test cases ! ..

humblec · 2020-06-14T09:37:07Z

/retest

humblec · 2020-06-14T10:18:19Z

The newly added test TestFSTypeProvision pass:


/usr/local/go/bin/go test -timeout 30s github.com/kubernetes-csi/external-provisioner/pkg/controller -run ^(TestFSTypeProvision)$

ok  	github.com/kubernetes-csi/external-provisioner/pkg/controller	(cached)

Even when I run individual test on TestProvision it pass:

 /usr/local/go/bin/go test -timeout 30s github.com/kubernetes-csi/external-provisioner/pkg/controller -run ^(TestProvision)$

ok  	github.com/kubernetes-csi/external-provisioner/pkg/controller

But when I run the controller package level test it fails: The failure says CSIPersistentVolumeSource is nil where it expected to have ext4 in the CSI PV source based on existing tests.

 --- FAIL: TestProvision (0.08s)
    controller_test.go:1828: Running test: provision with any volume data source
    controller_test.go:1828: Running test: provision with default secrets
    controller_test.go:1921: test "provision with default secrets": expected PV: &CSIPersistentVolumeSource{Driver:test-driver,VolumeHandle:test-volume-id,ReadOnly:false,FSType:ext4,VolumeAttributes:map[string]string{storage.kubernetes.io/csiProvisionerIdentity: test-provisioner,},ControllerPublishSecretRef:&SecretReference{Name:default-secret,Namespace:default-ns,},NodeStageSecretRef:&SecretReference{Name:default-secret,Namespace:default-ns,},NodePublishSecretRef:&SecretReference{Name:default-secret,Namespace:default-ns,},ControllerExpandSecretRef:&SecretReference{Name:default-secret,Namespace:default-ns,},}, got: &CSIPersistentVolumeSource{Driver:test-driver,VolumeHandle:test-volume-id,ReadOnly:false,FSType:,VolumeAttributes:map[string]string{storage.kubernetes.io/csiProvisionerIdentity: test-provisioner,},ControllerPublishSecretRef:&SecretReference{Name:default-secret,Namespace:default-ns,},NodeStageSecretRef:&SecretReference{Name:default-secret,Namespace:default-ns,},NodePublishSecretRef:&SecretReference{Name:default-secret,Namespace:default-ns,},ControllerExpandSecretRef:&SecretReference{Name:default-secret,Namespace:default-ns,},}
    controller_test.go:1828: Running test: provision with volume mode(Filesystem)
    controller_test.go:1921: test "provision with volume mode(Filesystem)": expected PV: &CSIPersistentVolumeSource{Driver:test-driver,VolumeHandle:test-volume-id,ReadOnly:false,FSType:ext4,VolumeAttributes:map[string]string{storage.kubernetes.io/csiProvisionerIdentity: test-provisioner,},ControllerPublishSecretRef:nil,NodeStageSecretRef:nil,NodePublishSecretRef:nil,ControllerExpandSecretRef:nil,}, got: &CSIPersistentVolumeSource{Driver:test-driver,VolumeHandle:test-volume-id,ReadOnly:false,FSType:,VolumeAttributes:map[string]string{storage.kubernetes.io/csiProvisionerIdentity: test-provisioner,},ControllerPublishSecretRef:nil,NodeStageSecretRef:nil,NodePublishSecretRef:nil,ControllerExpandSecretRef:nil,}

humblec · 2020-06-15T10:10:39Z

@msau42 @jsafrane @xing-yang any thoughts on why few of the existing tests ( For ex: TestProvision) are failing randomly when package-level test is triggered ?

pkg/controller/controller_test.go

There are 4 combinations added here: StorageClass set + default set StorageClass unset + default set StorageClass set + default unset StorageClass unset + default unset Signed-off-by: Humble Chirammal <[email protected]>

humblec · 2020-06-17T09:27:10Z

@msau42 I believe this is good to go in!

msau42 · 2020-06-17T16:43:02Z

pkg/controller/controller_test.go

+	}
+
+	if tc.expectedPVSpec != nil {
+		if pv.Name != tc.expectedPVSpec.Name {


I think the test logic would be cleaner if we did a deepequal of the entire pv spec, instead of matching individual fields one by one. But we can consider cleaning that up separately.

Sure, I will do that in a follow-up PR. There are some more places it need to be done in this file anyway.

msau42 · 2020-06-17T16:43:49Z

/lgtm
/approve

k8s-ci-robot · 2020-06-17T16:43:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: humblec, msau42

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [msau42]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 4, 2020

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 4, 2020

k8s-ci-robot requested review from davidz627 and sbezverk February 4, 2020 10:32

k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Feb 4, 2020

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Feb 4, 2020

k8s-ci-robot assigned jsafrane and msau42 Feb 4, 2020

davidz627 reviewed Feb 5, 2020

View reviewed changes

cmd/csi-provisioner/csi-provisioner.go Outdated Show resolved Hide resolved

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 5, 2020

humblec force-pushed the defaultfstype branch from 2203dd2 to 6ccc9e5 Compare February 17, 2020 06:42

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 17, 2020

xing-yang reviewed Feb 17, 2020

View reviewed changes

cmd/csi-provisioner/csi-provisioner.go Outdated Show resolved Hide resolved

pohly reviewed Feb 18, 2020

View reviewed changes

pkg/controller/controller.go Show resolved Hide resolved

humblec force-pushed the defaultfstype branch from 6ccc9e5 to 3c96181 Compare February 18, 2020 11:22

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 18, 2020

humblec force-pushed the defaultfstype branch from 3c96181 to c67f85c Compare February 18, 2020 11:55

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 27, 2020

k8s-ci-robot assigned xing-yang Jun 11, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 11, 2020

msau42 reviewed Jun 11, 2020

View reviewed changes

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 14, 2020

humblec force-pushed the defaultfstype branch from 55bbdf0 to bbeab57 Compare June 14, 2020 10:40

msau42 reviewed Jun 17, 2020

View reviewed changes

pkg/controller/controller_test.go Outdated Show resolved Hide resolved

humblec force-pushed the defaultfstype branch from 7aebb7b to 864d5cb Compare June 17, 2020 08:56

Add testcase for defaultfstype provisioner conditions:

4515622

There are 4 combinations added here: StorageClass set + default set StorageClass unset + default set StorageClass set + default unset StorageClass unset + default unset Signed-off-by: Humble Chirammal <[email protected]>

humblec force-pushed the defaultfstype branch from 864d5cb to 4515622 Compare June 17, 2020 09:03

msau42 reviewed Jun 17, 2020

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 17, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 17, 2020

k8s-ci-robot merged commit c1de898 into kubernetes-csi:master Jun 17, 2020

msau42 mentioned this pull request Jun 19, 2020

Make fstype configurable while starting the external-provisioner #328

Closed

k8s-ci-robot added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jul 31, 2020

ialidzhikov mentioned this pull request Sep 11, 2020

Fix volume fsType handling for CSI volumes gardener/gardener-extension-provider-openstack#142

Merged

humblec mentioned this pull request Sep 14, 2020

make use of configurable fsType in the templates. ceph/ceph-csi#1473

Closed

saikat-royc mentioned this pull request Sep 28, 2020

Use external provisioner 2.0.x which provides a default-fstype flag kubernetes-sigs/gcp-filestore-csi-driver#60

Closed

spielkind mentioned this pull request Jan 11, 2021

[cinder-csi-plugin] Cinder CSI Plugin doesn't honor anymore securityContext/fsGroup kubernetes/cloud-provider-openstack#1362

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make fstype configurable in external provisioner #400

Make fstype configurable in external provisioner #400

humblec commented Feb 4, 2020 •

edited by msau42

Loading

k8s-ci-robot commented Feb 4, 2020

humblec commented Feb 4, 2020

humblec commented Feb 4, 2020

humblec commented Feb 4, 2020

humblec commented Feb 4, 2020

k8s-ci-robot commented Feb 4, 2020

jsafrane commented Feb 5, 2020

xing-yang commented Jun 11, 2020

msau42 Jun 11, 2020

humblec Jun 17, 2020

humblec commented Jun 14, 2020

humblec commented Jun 14, 2020 •

edited

Loading

humblec commented Jun 15, 2020

humblec commented Jun 17, 2020

msau42 Jun 17, 2020

humblec Jun 17, 2020

msau42 commented Jun 17, 2020

k8s-ci-robot commented Jun 17, 2020

Make fstype configurable in external provisioner #400

Make fstype configurable in external provisioner #400

Conversation

humblec commented Feb 4, 2020 • edited by msau42 Loading

k8s-ci-robot commented Feb 4, 2020

humblec commented Feb 4, 2020

humblec commented Feb 4, 2020

humblec commented Feb 4, 2020

humblec commented Feb 4, 2020

k8s-ci-robot commented Feb 4, 2020

jsafrane commented Feb 5, 2020

xing-yang commented Jun 11, 2020

msau42 Jun 11, 2020

Choose a reason for hiding this comment

humblec Jun 17, 2020

Choose a reason for hiding this comment

humblec commented Jun 14, 2020

humblec commented Jun 14, 2020 • edited Loading

humblec commented Jun 15, 2020

humblec commented Jun 17, 2020

msau42 Jun 17, 2020

Choose a reason for hiding this comment

humblec Jun 17, 2020

Choose a reason for hiding this comment

msau42 commented Jun 17, 2020

k8s-ci-robot commented Jun 17, 2020

humblec commented Feb 4, 2020 •

edited by msau42

Loading

humblec commented Jun 14, 2020 •

edited

Loading