Support both directory and block device for local volume plugin FileSystem VolumeMode #63011

NickrenREN · 2018-04-23T15:13:50Z

Support both directory and block device for local volume plugin FileSystem VolumeMode

xref: local storage dynamic provisioning design #1914

What this PR does / why we need it:

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 #

Special notes for your reviewer:

Release note:

Support both directory and block device for local volume plugin FileSystem VolumeMode

msau42 · 2018-04-23T15:35:51Z

/assign

lichuqiang · 2018-04-26T11:13:02Z

pkg/apis/core/types.go

+	// Must be a filesystem type supported by the host operating system.
+	// Ex. "ext4", "xfs", "ntfs". Implicitly inferred to be "ext4" if unspecified.
+	// +optional
+	FSType string


should be a pointer, as the field is optional

Why do we need to use pointer for optional field ? I notice that other persistent volume sources are just using string except AzureDiskVolumeSource , see https://github.com/kubernetes/kubernetes/blob/master/pkg/apis/core/types.go#L578
https://github.com/kubernetes/kubernetes/blob/master/pkg/apis/core/types.go#L960
...

New optional fields to existing types need to be pointers for backwards compatibility. See #50121

I think there also needs to be defaulting logic in v1/defaults.go

good to know, thanks

ah sorry, I think we may not be able to default the field, because it would not work on windows.

msau42 · 2018-04-27T21:06:35Z

pkg/volume/local/local.go

+		return devicePath, "", err
+	}
+
+	if string(fileType) == string(mount.FileTypeDirectory) {


this could be a switch statement

msau42 · 2018-04-27T21:12:25Z

pkg/volume/local/local.go

+	}
+	if string(fileType) == string(mount.FileTypeDirectory) {
+		return spec.PersistentVolume.Spec.Local.Path, nil
+	} else if string(fileType) == string(mount.FileTypeBlockDev) {


This is only called when path is a block device. Do we need these checks again?

This is called in MountDevice() and before UnmountDevice(), here, i add FileTypeDirectory check in order to set deviceMountPath to local volume source path if the local PV is fs dir, so, if we want to remove this check, we need to set deviceMountPath to "" if local PV is fs dir ?

msau42 · 2018-04-27T21:15:26Z

pkg/volume/local/local.go

+var _ volume.DeviceUmounter = &deviceMounter{}
+
+func (dm *deviceMounter) UnmountDevice(deviceMountPath string) error {
+	basemountPath := filepath.Join(dm.plugin.host.GetPluginDir(localVolumePluginName), mount.MountsInGlobalPDPath)


Can you just do a Dir() on the deviceMountPath?

here is not try to get deviceMountPath dir, we want to get the expected base mount Dir, and then compare with deviceMountPath to see if we need to unmount device.

if we want to remove fs dir check in GetDeviceMountPath as we pointed out above, we need to add deviceMountPath != ""/ len(...) == 0 check

Can you add a comment here explaining the deviceMountPath formats that can be passed into here? That will help better understand why we can't just call Dir().

msau42 · 2018-04-27T21:16:55Z

pkg/volume/local/local.go

+	}, nil
+}
+
+func (dm *deviceMounter) MountDevice(spec *volume.Spec, devicePath string, _ *v1.Pod) (string, string, error) {


Can you add unit tests for these new functions

msau42 · 2018-04-27T21:18:27Z

pkg/apis/core/types.go

+	// Must be a filesystem type supported by the host operating system.
+	// Ex. "ext4", "xfs", "ntfs". Implicitly inferred to be "ext4" if unspecified.
+	// +optional
+	FSType string


I think there also needs to be defaulting logic in v1/defaults.go

msau42 · 2018-04-30T20:56:00Z

pkg/volume/local/local.go

+
+	switch fileType {
+	case mount.FileTypeBlockDev:
+		return dm.mountLocalBlockDevice(spec, devicePath)


Does SetUp also have to change, to bind mount from this global path instead?

yes. i think so

msau42 · 2018-04-30T20:56:46Z

pkg/volume/local/local.go

+		return dm.mountLocalBlockDevice(spec, devicePath)
+	case mount.FileTypeDirectory:
+		// if the given local volume path is of already filesystem directory, return directly
+		return spec.PersistentVolume.Spec.Local.Path, spec.PersistentVolume.Spec.Local.Path, nil


I wonder, can we return nil path for this case? Could UnmountDevice be simplified if it is only called for blockdev scenario?

I think they are the same. deviceMountPath is set to local pv path If it is filesystem dir. and UnmountPath will not be executed for this case.

msau42 · 2018-04-30T20:59:56Z

pkg/apis/core/types.go

@@ -1442,6 +1442,12 @@ type LocalVolumeSource struct {
 	// Block devices can be represented only by VolumeMode=Block, which also requires the


This comment also needs to be updated because now you can specify a block device with VolumeMode=Filesystem.

Deleted the last three lines

NickrenREN · 2018-09-03T06:47:15Z

/retest

andyzhangx · 2018-09-03T07:24:35Z

@PatrickLang raw block device is not supported on Windows k8s now. Will windows 2019 supports raw block device for windows container? If so, I could enable it in k8s level.
Anyway, this PR deals with linux implementation, I am ok with that.

andyzhangx · 2018-09-03T07:26:23Z

/test pull-kubernetes-cross

thockin · 2018-09-04T15:30:07Z

/lgtm
/approve

k8s-ci-robot · 2018-09-04T15:30:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jsafrane, msau42, NickrenREN, thockin

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:

~~api/OWNERS~~ [thockin]
~~docs/api-reference/OWNERS~~ [thockin]
~~pkg/apis/core/OWNERS~~ [thockin]
~~pkg/util/mount/OWNERS~~ [jsafrane,thockin]
~~pkg/volume/local/OWNERS~~ [jsafrane,msau42,thockin]
~~pkg/volume/testing/OWNERS~~ [jsafrane,thockin]
~~staging/src/k8s.io/api/OWNERS~~ [thockin]
~~test/e2e/storage/OWNERS~~ [jsafrane,msau42,thockin]

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

k8s-github-robot · 2018-09-04T17:24:35Z

Automatic merge from submit-queue (batch tested with PRs 63011, 68089, 67944, 68132). If you want to cherry-pick this change to another branch, please follow the instructions here: https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md.

…root

msau42 · 2018-09-06T17:15:35Z

@NickrenREN can you also investigate if the functionality of local volumes (bind mount/read/write/fsgroup) would have worked before this change if the volume was created with permissions that kubelet did not have?

We need to figure out if this has the potential to be a regression, and if so, we need an appropriate release note with "ACTION REQUIRED"

msau42 · 2018-09-08T01:57:49Z

We discussed, this is only an issue with /tmp directories, which is only a testing issue

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 23, 2018

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

k8s-ci-robot requested review from davidz627, humblec, jeffvance, jingxu97, jsafrane, kerneltime, matchstick, msau42, piosz, rata, roberthbailey, rootfs, saad-ali, screeley44 and sjenning April 23, 2018 15:14

NickrenREN force-pushed the local-plugin-change branch from 2ce304e to 73024ed Compare April 23, 2018 15:25

k8s-ci-robot assigned msau42 Apr 23, 2018

lichuqiang reviewed Apr 26, 2018

View reviewed changes

lichuqiang mentioned this pull request Apr 26, 2018

Local volume dynamic provisioning kubernetes-retired/external-storage#743

Closed

msau42 reviewed Apr 27, 2018

View reviewed changes

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

NickrenREN force-pushed the local-plugin-change branch from 73024ed to 87dc3ac Compare April 28, 2018 03:27

msau42 reviewed Apr 30, 2018

View reviewed changes

NickrenREN force-pushed the local-plugin-change branch from 87dc3ac to e671bb5 Compare May 2, 2018 13:58

NickrenREN added 6 commits September 3, 2018 09:49

add fstype for local volume source

25caf91

auto generated files

159a435

make pathWithinBase public

7157d45

local volume plugin changes

8173ef1

fix UT errors and add new UTs

8d3fdc5

fix and update local volume e2es

5bc823f

NickrenREN force-pushed the local-plugin-change branch from 03caeec to 5bc823f Compare September 3, 2018 05:37

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 4, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 4, 2018

k8s-github-robot merged commit 9c86087 into kubernetes:master Sep 4, 2018

NickrenREN deleted the local-plugin-change branch September 5, 2018 01:36

lichuqiang mentioned this pull request Sep 6, 2018

MountOptions for local volume #68317

Closed

NickrenREN mentioned this pull request Sep 6, 2018

Failing tests : [sig-storage] Multiple on gke-gci-new-gci-master-upgrade-cluster-parallel #68314

Closed

msau42 added a commit to msau42/kubernetes that referenced this pull request Sep 6, 2018

Port e2e test changes from kubernetes#63011 that created symlinks as …

73533a5

…root

msau42 mentioned this pull request Sep 6, 2018

Fix PersistentVolumes-local symlink upgrade tests #68371

Merged

lichuqiang mentioned this pull request Sep 7, 2018

[local-volume] support fs volumes on block kubernetes-retired/external-storage#973

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support both directory and block device for local volume plugin FileSystem VolumeMode #63011

Support both directory and block device for local volume plugin FileSystem VolumeMode #63011

NickrenREN commented Apr 23, 2018 •

edited

msau42 commented Apr 23, 2018

lichuqiang Apr 26, 2018

NickrenREN Apr 26, 2018 •

edited

msau42 Apr 27, 2018

msau42 Apr 27, 2018

NickrenREN Apr 28, 2018

msau42 Apr 30, 2018

msau42 Apr 27, 2018

NickrenREN Apr 28, 2018

msau42 Apr 27, 2018

NickrenREN Apr 28, 2018

msau42 Apr 27, 2018

NickrenREN Apr 28, 2018

NickrenREN Apr 28, 2018

msau42 Aug 16, 2018

NickrenREN Aug 17, 2018

msau42 Apr 27, 2018

NickrenREN Apr 28, 2018

msau42 Apr 27, 2018

msau42 Apr 30, 2018

NickrenREN May 1, 2018

msau42 Apr 30, 2018

NickrenREN May 1, 2018

msau42 Apr 30, 2018

NickrenREN May 2, 2018

NickrenREN commented Sep 3, 2018

andyzhangx commented Sep 3, 2018

andyzhangx commented Sep 3, 2018

thockin commented Sep 4, 2018

k8s-ci-robot commented Sep 4, 2018

k8s-github-robot commented Sep 4, 2018

msau42 commented Sep 6, 2018

msau42 commented Sep 8, 2018

		@@ -1442,6 +1442,12 @@ type LocalVolumeSource struct {
		// Block devices can be represented only by VolumeMode=Block, which also requires the

Support both directory and block device for local volume plugin FileSystem VolumeMode #63011

Support both directory and block device for local volume plugin FileSystem VolumeMode #63011

Conversation

NickrenREN commented Apr 23, 2018 • edited

msau42 commented Apr 23, 2018

Choose a reason for hiding this comment

NickrenREN Apr 26, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickrenREN commented Sep 3, 2018

andyzhangx commented Sep 3, 2018

andyzhangx commented Sep 3, 2018

thockin commented Sep 4, 2018

k8s-ci-robot commented Sep 4, 2018

k8s-github-robot commented Sep 4, 2018

msau42 commented Sep 6, 2018

msau42 commented Sep 8, 2018

NickrenREN commented Apr 23, 2018 •

edited

NickrenREN Apr 26, 2018 •

edited