Bugfix/csi default fs type #65499

krunaljain · 2018-06-26T21:01:45Z

This PR address the issue mentioned in the following ticket #65122
The FSType string will now not be defaulted to ext4. Removes defaulting of CSI file system type to ext4. CSI plugins that depended on this default need to be updated as the fsType would remain an empty string if not provided and would not default to ext4. CSI spec allows for an empty fstype string. This is intended for non-block plugins like nfs and gluster where filesystems are not separately created on the volume. But currently the default file system is overridden to ext4 which makes the above case redundant. This commit prevents such an overridding.

ACTION REQUIRED: Removes defaulting of CSI file system type to ext4. All the production drivers listed under https://kubernetes-csi.github.io/docs/Drivers.html were inspected and should not be impacted after this change. If you are using a driver not in that list, please test the drivers on an updated test cluster first. ```

k8s-ci-robot · 2018-06-26T21:01:48Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: [email protected]

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.

msau42 · 2018-06-26T23:32:09Z

/ok-to-test

msau42

A release note should be added that FSType is not defaulted anymore.

Also, I think the comment in pkg/apis/core/types.go and staging/src/k8s.io/api/core/v1/types.go need to be updated (and run make update to regenerate all the documentation)

msau42 · 2018-06-26T23:33:25Z

pkg/volume/csi/csi_mounter_test.go

+
+	//Test the default value of file system type is not overriden
+	if len(csiMounter.spec.PersistentVolume.Spec.CSI.FSType) != 0 {
+		t.Fatalf("default value of file system type was overriden by type %s" , csiMounter.spec.PersistentVolume.Spec.CSI.FSType)


This could just be Errorf instead of Fatalf. Fatalf will exit and not run the remainder of the test.

msau42 · 2018-06-26T23:33:27Z

pkg/volume/csi/csi_mounter.go

@@ -34,7 +34,7 @@ import (
 	"k8s.io/kubernetes/pkg/volume/util"
 )

-const defaultFSType = "ext4"
+const defaultFSType = ""


I would remove this whole variable

msau42 · 2018-06-26T23:35:41Z

Also, it would be good to put the generated files into a separate commit.

jsafrane · 2018-06-27T08:22:10Z

Please squash the commits. And should it include removal of "Implicitly inferred to be "ext4" if unspecified." in types.go?

msau42 · 2018-06-27T17:16:50Z

In addition to updating the comments in types.go, can you also say in the release note "ACTION REQUIRED: Removes defaulting of CSI fsType to ext4. CSI plugins that depended on this default need to be updated"

saad-ali · 2018-06-27T17:20:23Z

/assign @vladimirvivien

msau42 · 2018-06-27T18:02:01Z

You also need to update staging/src/k8s.io/api/core/v1/types.go, and then run make update to regenerate the documentation (please put the autogenerated files in a separate commit)

lpabon · 2018-07-09T18:18:14Z

Hi guys. Just wanted to point out that this change will not affect the Portworx CSI driver. It currently uses the storage class fs value as stated here.

/cc @msau42

msau42 · 2018-07-09T19:43:02Z

/retest

msau42 · 2018-07-09T19:44:27Z

Release note has been updated, and all known production csi drivers have been checked.

msau42 · 2018-07-09T19:44:45Z

Also, email has been sent to kubernetes-sig-storage

vladimirvivien · 2018-07-10T15:21:05Z

@msau42 thanks for doing that research !!

liggitt · 2018-07-10T15:25:38Z

/hold cancel

thanks. this seems worthwhile (and safe, given the research done) to pick back to maintenance branches.

k8s-github-robot · 2018-07-10T17:14:12Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2018-07-10T17:18:05Z

Automatic merge from submit-queue (batch tested with PRs 55023, 65499). If you want to cherry-pick this change to another branch, please follow the instructions here.

liggitt · 2018-07-10T18:16:32Z

@krunaljain thanks, can you pick to 1.11 as well?

k8s-ci-robot · 2018-07-10T18:16:40Z

@krunaljain: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-gce-100-performance	`fb28a1d`	link	`/test pull-kubernetes-e2e-gce-100-performance`

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.

…5499-upstream-release-1.11 Automatic merge from submit-queue. Automated cherry pick of #65499: Removes defaulting of CSI fsType to ext4 Cherry pick of #65499 on release-1.11. #65499: Removes defaulting of CSI fsType to ext4

…5499-upstream-release-1.10 Automatic merge from submit-queue. Automated cherry pick of #65499: Removes defaulting of CSI fsType to ext4 Cherry pick of #65499 on release-1.10. #65499: Removes defaulting of CSI fsType to ext4

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. 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 Jun 26, 2018

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 26, 2018

k8s-ci-robot requested review from jingxu97 and jsafrane June 26, 2018 21:01

krunaljain force-pushed the bugfix/csi_default_fs_type branch from 5b39796 to 03f36bd Compare June 26, 2018 23:25

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jun 26, 2018

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 26, 2018

msau42 reviewed Jun 26, 2018

View reviewed changes

k8s-ci-robot assigned vladimirvivien Jun 27, 2018

k8s-github-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Jun 27, 2018

k8s-ci-robot added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Jun 27, 2018

krunaljain force-pushed the bugfix/csi_default_fs_type branch from b47aa4c to 1b4dba8 Compare June 27, 2018 19:21

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 Jun 27, 2018

krunaljain force-pushed the bugfix/csi_default_fs_type branch from 5aa8bb4 to 73822ce Compare June 27, 2018 21:33

k8s-ci-robot requested a review from msau42 July 9, 2018 18:18

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 10, 2018

k8s-github-robot merged commit 3b269e1 into kubernetes:master Jul 10, 2018

krunaljain mentioned this pull request Jul 10, 2018

Automated cherry pick of #65499: Removes defaulting of CSI fsType to ext4 #66035

Merged

krunaljain mentioned this pull request Jul 10, 2018

Automated cherry pick of #65499: Removes defaulting of CSI fsType to ext4 #66037

Merged

msau42 mentioned this pull request Jul 18, 2018

CSI volume source fstype incompatible with CSI spec #65122

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix/csi default fs type #65499

Bugfix/csi default fs type #65499

krunaljain commented Jun 26, 2018 •

edited

Loading

k8s-ci-robot commented Jun 26, 2018

msau42 commented Jun 26, 2018

msau42 left a comment

msau42 Jun 26, 2018

msau42 Jun 26, 2018

msau42 commented Jun 26, 2018

jsafrane commented Jun 27, 2018

msau42 commented Jun 27, 2018

saad-ali commented Jun 27, 2018

msau42 commented Jun 27, 2018

lpabon commented Jul 9, 2018

msau42 commented Jul 9, 2018

msau42 commented Jul 9, 2018

msau42 commented Jul 9, 2018

vladimirvivien commented Jul 10, 2018

liggitt commented Jul 10, 2018

k8s-github-robot commented Jul 10, 2018

k8s-github-robot commented Jul 10, 2018

liggitt commented Jul 10, 2018

k8s-ci-robot commented Jul 10, 2018 •

edited

Loading

Bugfix/csi default fs type #65499

Bugfix/csi default fs type #65499

Conversation

krunaljain commented Jun 26, 2018 • edited Loading

k8s-ci-robot commented Jun 26, 2018

msau42 commented Jun 26, 2018

msau42 left a comment

Choose a reason for hiding this comment

msau42 Jun 26, 2018

Choose a reason for hiding this comment

msau42 Jun 26, 2018

Choose a reason for hiding this comment

msau42 commented Jun 26, 2018

jsafrane commented Jun 27, 2018

msau42 commented Jun 27, 2018

saad-ali commented Jun 27, 2018

msau42 commented Jun 27, 2018

lpabon commented Jul 9, 2018

msau42 commented Jul 9, 2018

msau42 commented Jul 9, 2018

msau42 commented Jul 9, 2018

vladimirvivien commented Jul 10, 2018

liggitt commented Jul 10, 2018

k8s-github-robot commented Jul 10, 2018

k8s-github-robot commented Jul 10, 2018

liggitt commented Jul 10, 2018

k8s-ci-robot commented Jul 10, 2018 • edited Loading

krunaljain commented Jun 26, 2018 •

edited

Loading

k8s-ci-robot commented Jul 10, 2018 •

edited

Loading