New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update GCE plugin for block support #58710

Merged
merged 1 commit into from Feb 26, 2018

Conversation

@screeley44
Contributor

screeley44 commented Jan 23, 2018

update GCE plugin for block volume support

cc @gnufied @mtanino @jsafrane

GCE PD volume plugin got block volume support
@screeley44

This comment has been minimized.

Contributor

screeley44 commented Jan 23, 2018

/sig storage

@screeley44

This comment has been minimized.

Contributor

screeley44 commented Jan 24, 2018

/test pull-kubernetes-unit

@screeley44

This comment has been minimized.

Contributor

screeley44 commented Jan 26, 2018

/test pull-kubernetes-unit

@saad-ali saad-ali requested a review from msau42 Feb 15, 2018

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

This comment has been minimized.

@msau42

msau42 Feb 16, 2018

Member

year needs to be updated

This comment has been minimized.

@screeley44

screeley44 Feb 20, 2018

Contributor

done

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

This comment has been minimized.

@msau42

msau42 Feb 16, 2018

Member

Why 1?

This comment has been minimized.

@screeley44

screeley44 Feb 20, 2018

Contributor

well if the path is empty Dir returns "."

This comment has been minimized.

@msau42

msau42 Feb 23, 2018

Member

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

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

This comment has been minimized.

@msau42

msau42 Feb 16, 2018

Member

Again, why 1?

This comment has been minimized.

@screeley44

screeley44 Feb 20, 2018

Contributor

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

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

This comment has been minimized.

@msau42

msau42 Feb 16, 2018

Member

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.

This comment has been minimized.

@msau42

msau42 Feb 16, 2018

Member

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

This comment has been minimized.

@screeley44

screeley44 Feb 20, 2018

Contributor

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

This comment has been minimized.

@mtanino

mtanino Feb 22, 2018

Member

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.

This comment has been minimized.

@msau42

msau42 Feb 22, 2018

Member

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?

This comment has been minimized.

@mtanino

mtanino Feb 22, 2018

Member

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.

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

This comment has been minimized.

@msau42

msau42 Feb 16, 2018

Member

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.

This comment has been minimized.

@screeley44

screeley44 Feb 20, 2018

Contributor

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

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

This comment has been minimized.

@msau42

msau42 Feb 16, 2018

Member

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

This comment has been minimized.

@screeley44

screeley44 Feb 21, 2018

Contributor

carry over from FS - will remove it

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

This comment has been minimized.

@msau42

msau42 Feb 16, 2018

Member

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

This comment has been minimized.

@screeley44

screeley44 Feb 21, 2018

Contributor

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

This comment has been minimized.

@msau42

msau42 Feb 21, 2018

Member

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.

This comment has been minimized.

@screeley44

screeley44 Feb 21, 2018

Contributor

@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.

This comment has been minimized.

@msau42

msau42 Feb 21, 2018

Member

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.

This comment has been minimized.

@screeley44

screeley44 Feb 21, 2018

Contributor

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

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

This comment has been minimized.

@msau42

msau42 Feb 21, 2018

Member

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?

This comment has been minimized.

@msau42

msau42 Feb 21, 2018

Member

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

This comment has been minimized.

@screeley44

screeley44 Feb 21, 2018

Contributor

it is used in the attacher

This comment has been minimized.

@mtanino

mtanino Feb 22, 2018

Member

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.

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

This comment has been minimized.

@msau42

msau42 Feb 21, 2018

Member

Do you want to validate the fields in this spec?

This comment has been minimized.

@screeley44

screeley44 Feb 21, 2018

Contributor

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

This comment has been minimized.

@msau42

msau42 Feb 21, 2018

Member

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

This comment has been minimized.

@screeley44

screeley44 Feb 22, 2018

Contributor

of course, my bad

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

This comment has been minimized.

@msau42

msau42 Feb 21, 2018

Member

Test case for this function?

This comment has been minimized.

@screeley44

screeley44 Feb 21, 2018

Contributor

I'll add one

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

This comment has been minimized.

@msau42

msau42 Feb 21, 2018

Member

Test case?

This comment has been minimized.

@screeley44

screeley44 Feb 21, 2018

Contributor

I'll add one

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

This comment has been minimized.

@msau42

msau42 Feb 21, 2018

Member

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

@msau42

This comment has been minimized.

Member

msau42 commented Feb 22, 2018

Also any plans to write an e2e test?

@screeley44

This comment has been minimized.

Contributor

screeley44 commented Feb 22, 2018

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

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

This comment has been minimized.

@msau42

This comment has been minimized.

@screeley44

screeley44 Feb 22, 2018

Contributor

ack

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)

This comment has been minimized.

@msau42

msau42 Feb 22, 2018

Member

maybe just Errorf instead of Fatalf here

This comment has been minimized.

@screeley44

screeley44 Feb 22, 2018

Contributor

ack

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

This comment has been minimized.

@msau42

msau42 Feb 22, 2018

Member

Same errorF. Also check for nil

This comment has been minimized.

@screeley44

screeley44 Feb 22, 2018

Contributor

ack

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

This comment has been minimized.

@msau42

msau42 Feb 22, 2018

Member

Also a test case with invalid global map path

This comment has been minimized.

@screeley44

screeley44 Feb 22, 2018

Contributor

ack

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

This comment has been minimized.

@msau42

msau42 Feb 23, 2018

Member

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

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

This comment has been minimized.

@msau42

msau42 Feb 23, 2018

Member

Errorf is fine here

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)

This comment has been minimized.

@msau42

msau42 Feb 23, 2018

Member

Errorf

This comment has been minimized.

@screeley44

screeley44 Feb 23, 2018

Contributor

ack to all

@msau42

This comment has been minimized.

Member

msau42 commented Feb 23, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Feb 23, 2018

@jsafrane

This comment has been minimized.

Member

jsafrane commented Feb 26, 2018

/approve no-issue

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Feb 26, 2018

[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:

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

@jsafrane

This comment has been minimized.

Member

jsafrane commented Feb 26, 2018

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

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Feb 26, 2018

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

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Feb 26, 2018

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

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

12 of 13 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-e2e-gce
Details
cla/linuxfoundation screeley44 authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-unit Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment