update GCE plugin for block support #58710

screeley44 · 2018-01-23T19:48:52Z

update GCE plugin for block volume support

GCE PD volume plugin got block volume support

screeley44 · 2018-01-23T19:53:07Z

/sig storage

screeley44 · 2018-01-24T21:05:54Z

/test pull-kubernetes-unit

screeley44 · 2018-01-26T17:52:56Z

/test pull-kubernetes-unit

msau42 · 2018-02-15T21:21:05Z

pkg/volume/gce_pd/gce_pd_block.go

@@ -0,0 +1,173 @@
+/*
+Copyright 2014 The Kubernetes Authors.


year needs to be updated

msau42 · 2018-02-16T22:07:51Z

pkg/volume/gce_pd/gce_pd_block.go

+	glog.V(5).Infof("globalMapPathUUID: %v, err: %v", globalMapPathUUID, err)
+
+	globalMapPath := filepath.Dir(globalMapPathUUID)
+	if len(globalMapPath) == 1 {


well if the path is empty Dir returns "."

Should this be <= 1, like the check below? Can you also add a comment

msau42 · 2018-02-16T22:09:13Z

pkg/volume/gce_pd/gce_pd_block.go

+	//   plugins/kubernetes.io/{PluginName}/{DefaultKubeletVolumeDevicesDirName}/{volumeID}
+	//   plugins/kubernetes.io/gce-pd/volumeDevices/vol-XXXXXX
+	pdName := filepath.Base(globalMapPath)
+	if len(pdName) <= 1 {


Again, why 1?

Same, Base returns "." for an empty path so would indicate an error condition

msau42 · 2018-02-16T22:11:41Z

pkg/volume/gce_pd/gce_pd_block.go

+		Spec: v1.PersistentVolumeSpec{
+			PersistentVolumeSource: v1.PersistentVolumeSource{
+				GCEPersistentDisk: &v1.GCEPersistentDiskVolumeSource{
+					PDName: pdName,


Hmmm partition is not being reconstructed (same with filesystem). I wonder if that could be a problem. I need to think about that a little more.

When we unmap, do we need to know the actual device path? Or do we just cleanup the symlinks that only use volume name?

I think we need the devicePath, Unmap will clean up the sym links and the loopback devices as far as I can tell.

yes, In order to remove loopback device, devicePath like /dev/sdc is needed. If the block volume is created for partitioned device like /dev/sdc1, then kubelet needs devicePath + partition number to remove loopback device.

So for regular scenario, the device path for partitions should be what attacher returns. For reconstruction, do we need to correctly reconstruct the partition number?

So for regular scenario, the device path for partitions should be what attacher returns.

Correct.

For reconstruction, do we need to correctly reconstruct the partition number?

I suppose we don't need to create full spec during reconstruction but at least, reconstructed spec must have members to create block volume mapper successfully. If block volume is created by partitioned devicePath in previous, symlink is connected to the devicePath including partition number. Therefore kubelet can retrieve devicePath from symlink instead of using value of partition .

We need this logic in @jingxu97 's next reconstruction update PR.

msau42 · 2018-02-16T22:13:48Z

pkg/volume/gce_pd/gce_pd_block.go

+			manager:         manager,
+			mounter:         mounter,
+			plugin:          plugin,
+			MetricsProvider: volume.NewMetricsStatFS(getPath(podUID, spec.Name(), plugin.host)),


I think StatFS is not going to work for block since there's no filesystem on it. Do usage metrics even make sense for block volumes? You can't really tell how much is used on a raw block device.

yeah, was thinking the same thing, I can remove this I think

msau42 · 2018-02-16T22:14:38Z

pkg/volume/gce_pd/gce_pd_block.go

+			MetricsProvider: volume.NewMetricsStatFS(getPath(podUID, spec.Name(), plugin.host)),
+		},
+		readOnly:    readOnly,
+		diskMounter: volumehelper.NewSafeFormatAndMountFromHost(plugin.GetPluginName(), plugin.host)}, nil


Do we need this? We shouldn't be formatting or mounting anything for block volumes

carry over from FS - will remove it

msau42 · 2018-02-16T22:26:21Z

pkg/volume/gce_pd/gce_pd_block.go

+			volName:         spec.Name(),
+			podUID:          podUID,
+			pdName:          pdName,
+			partition:       partition,


Where does partition get used to return the correct device path?

I don't think partition gets used for block that I can tell, it was probably carry over from the FS struct, I will remove it

I think it's still legit to pass in a partition, although it's only supported with static PVs at the moment. I think you need to use the partition information when returning the device path.

@msau42 - when I tested it after commenting it out like this, I was able to get devicePath with no issues and the plugin worked correctly when in happy path. I also couldn't find where it is being used for block (maybe missed it), I see that is a volume partition like /dev/sda1 the partition would be 1 but just not sure where it is being called for block?. Anyway, I can add back in if you think we really do need it.

If a user creates a PV with partition = 1, then we'll want the block device path to be /dev/sda1 (or whatever the gce pd device path format is). If it's partition = 2, then sda2, etc. Each partition should still be treated as a separate block device.

ah, I see now, I'll add back in

msau42 · 2018-02-21T20:03:43Z

pkg/volume/gce_pd/gce_pd_block.go

+			volName:   spec.Name(),
+			podUID:    podUID,
+			pdName:    pdName,
+			partition: partition,


You need to use this partition number when returning the global device path for this volume. @mtanino is it SetUp() that returns the global device path?

Or because this is attachable, is it returned in the attach method?

it is used in the attacher

Or because this is attachable, is it returned in the attach method?

yes, gce plugins is attachable and cloud provider supports remote attach for the plugin.
So the devicePath is returned during Attach() operation.

msau42 · 2018-02-21T20:06:38Z

pkg/volume/gce_pd/gce_pd_block_test.go

+	if err1a != nil {
+		t.Fatalf("can't make a temp dir: %v", err1a)
+	}
+	spec, err := getVolumeSpecFromGlobalMapPath(path1a)


Do you want to validate the fields in this spec?

Is that necessary since that is already done within the api/validation and the existing unit tests there?

I mean, validate that this function return an appropriate spec with the correct pdName based on the given path.

of course, my bad

msau42 · 2018-02-21T20:07:07Z

pkg/volume/gce_pd/gce_pd_block.go

+
+// GetGlobalMapPath returns global map path and error
+// path: plugins/kubernetes.io/{PluginName}/volumeDevices/pdName
+func (pd *gcePersistentDisk) GetGlobalMapPath(spec *volume.Spec) (string, error) {


Test case for this function?

I'll add one

msau42 · 2018-02-21T20:07:14Z

pkg/volume/gce_pd/gce_pd_block.go

+
+// GetPodDeviceMapPath returns pod device map path and volume name
+// path: pods/{podUid}/volumeDevices/kubernetes.io~aws
+func (pd *gcePersistentDisk) GetPodDeviceMapPath() (string, string) {


I'll add one

msau42 · 2018-02-21T20:07:53Z

pkg/volume/gce_pd/gce_pd_block.go

+			volName:   spec.Name(),
+			podUID:    podUID,
+			pdName:    pdName,
+			partition: partition,


Or because this is attachable, is it returned in the attach method?

msau42 · 2018-02-22T01:48:06Z

Also any plans to write an e2e test?

screeley44 · 2018-02-22T03:09:39Z

yes, we def plan on adding e2e tests but not sure we will make this release, I need to sync up with Erin and @mtanino

msau42 · 2018-02-22T19:32:20Z

pkg/volume/gce_pd/gce_pd_block_test.go

@@ -0,0 +1,135 @@
+/*
+Copyright 2014 The Kubernetes Authors.


msau42 · 2018-02-22T19:33:18Z

pkg/volume/gce_pd/gce_pd_block_test.go

+		t.Fatalf("Failed to get spec from GlobalMapPath: %v", err)
+	}
+	if spec.PersistentVolume.Spec.GCEPersistentDisk.PDName != testPdName {
+		t.Fatalf("Invalid pdName retrieved from GlobalMapPath spec: %s", spec.PersistentVolume.Spec.GCEPersistentDisk.PDName)


maybe just Errorf instead of Fatalf here

msau42 · 2018-02-22T19:33:38Z

pkg/volume/gce_pd/gce_pd_block_test.go

+	block := v1.PersistentVolumeBlock
+	specMode := spec.PersistentVolume.Spec.VolumeMode
+	if *specMode != block {
+		t.Fatalf("Invalid volumeMode retrieved from GlobalMapPath spec: %v - %v", *specMode, block)


Same errorF. Also check for nil

msau42 · 2018-02-22T19:34:50Z

pkg/volume/gce_pd/gce_pd_block_test.go

+
+	expectedGlobalPath := path.Join(tmpVDir, testGlobalPath)
+
+	spec, err := getVolumeSpecFromGlobalMapPath(expectedGlobalPath)


Also a test case with invalid global map path

msau42 · 2018-02-23T18:57:35Z

pkg/volume/gce_pd/gce_pd_block.go

+	glog.V(5).Infof("globalMapPathUUID: %v, err: %v", globalMapPathUUID, err)
+
+	globalMapPath := filepath.Dir(globalMapPathUUID)
+	if len(globalMapPath) == 1 {


Should this be <= 1, like the check below? Can you also add a comment

msau42 · 2018-02-23T18:58:07Z

pkg/volume/gce_pd/gce_pd_block_test.go

+	//Bad Path
+	badspec, err := getVolumeSpecFromGlobalMapPath("")
+	if badspec != nil || err == nil {
+		t.Fatalf("Expected not to get spec from GlobalMapPath but did")


Errorf is fine here

msau42 · 2018-02-23T18:58:44Z

pkg/volume/gce_pd/gce_pd_block_test.go

+		t.Fatalf("Invalid GlobalMapPath from spec: %s", spec.PersistentVolume.Spec.GCEPersistentDisk.PDName)
+	}
+	if gMapPath != expectedGlobalPath {
+		t.Fatalf("Failed to get GlobalMapPath: %s %s", gMapPath, expectedGlobalPath)


msau42 · 2018-02-23T23:01:38Z

/lgtm

jsafrane · 2018-02-26T11:55:42Z

/approve no-issue

k8s-ci-robot · 2018-02-26T11:55:49Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~pkg/volume/gce_pd/OWNERS~~ [jsafrane]

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

jsafrane · 2018-02-26T11:56:47Z

@screeley44, I added release note to the first comment, please review.

k8s-github-robot · 2018-02-26T11:57:06Z

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

k8s-github-robot · 2018-02-26T12:39:12Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot requested review from davidz627 and jingxu97 January 23, 2018 19:49

k8s-ci-robot added the sig/storage Categorizes an issue or PR as relevant to SIG Storage. label Jan 23, 2018

screeley44 force-pushed the gce-block-update2 branch 2 times, most recently from 00a6c37 to 616f0ce Compare January 25, 2018 20:09

mtanino mentioned this pull request Jan 25, 2018

Raw block device using persistent volume source kubernetes/enhancements#351

Closed

screeley44 force-pushed the gce-block-update2 branch 2 times, most recently from 802bb95 to 3bffe56 Compare January 26, 2018 01:41

screeley44 force-pushed the gce-block-update2 branch from 3bffe56 to 16bc2ac Compare February 1, 2018 15:45

screeley44 force-pushed the gce-block-update2 branch from 16bc2ac to 210f5d9 Compare February 14, 2018 14:59

saad-ali assigned msau42 Feb 15, 2018

saad-ali requested a review from msau42 February 15, 2018 07:13

msau42 reviewed Feb 16, 2018

View reviewed changes

screeley44 force-pushed the gce-block-update2 branch 6 times, most recently from 3105fb9 to be4b24d Compare February 21, 2018 19:26

msau42 reviewed Feb 21, 2018

View reviewed changes

screeley44 force-pushed the gce-block-update2 branch from be4b24d to be56e28 Compare February 22, 2018 03:05

msau42 reviewed Feb 22, 2018

View reviewed changes

screeley44 force-pushed the gce-block-update2 branch 4 times, most recently from b07f43f to 974bcbd Compare February 23, 2018 17:14

msau42 reviewed Feb 23, 2018

View reviewed changes

update GCE plugin for block support

2c0129c

screeley44 force-pushed the gce-block-update2 branch from 974bcbd to 2c0129c Compare February 23, 2018 19:16

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

jsafrane added this to the v1.10 milestone Feb 26, 2018

k8s-github-robot merged commit 2aed8d7 into kubernetes:master Feb 26, 2018

jsafrane mentioned this pull request Feb 26, 2018

refactor volume util files #59310

Merged


		expectedGlobalPath := path.Join(tmpVDir, testGlobalPath)

		spec, err := getVolumeSpecFromGlobalMapPath(expectedGlobalPath)

update GCE plugin for block support #58710

update GCE plugin for block support #58710

Conversation

screeley44 commented Jan 23, 2018 • edited by jsafrane Loading

screeley44 commented Jan 23, 2018

screeley44 commented Jan 24, 2018

screeley44 commented Jan 26, 2018

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

screeley44 Feb 21, 2018 • edited Loading

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

msau42 commented Feb 22, 2018

screeley44 commented Feb 22, 2018

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

msau42 commented Feb 23, 2018

jsafrane commented Feb 26, 2018

k8s-ci-robot commented Feb 26, 2018

jsafrane commented Feb 26, 2018

k8s-github-robot commented Feb 26, 2018

k8s-github-robot commented Feb 26, 2018

screeley44 commented Jan 23, 2018 •

edited by jsafrane

Loading

screeley44 Feb 21, 2018 •

edited

Loading