Skip to content
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

Block Volume Support: Local Volume Plugin update #59303

Merged
merged 2 commits into from Feb 7, 2018

Conversation

@dhirajh
Copy link
Contributor

dhirajh commented Feb 3, 2018

What this PR does / why we need it:

Introduce block volume support to local volumes plugin.

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 #59500

Special notes for your reviewer:
@msau42 @mtanino @ianchakeres

Adding support for block volumes as per kubernetes/enhancements#121

Other related PRs:
(#50457) API Change
(#53385) VolumeMode PV-PVC Binding change
(#51494) Container runtime interface change, volumemanager changes, operationexecutor changes

Release note:

Added support for Block Volume type to local-volume plugin.
@ianchakeres

This comment has been minimized.

Copy link
Member

ianchakeres commented Feb 3, 2018

/ok-to-test

@ianchakeres

This comment has been minimized.

Copy link
Member

ianchakeres commented Feb 3, 2018

/sig storage

@dhirajh

This comment has been minimized.

Copy link
Contributor Author

dhirajh commented Feb 3, 2018

/retest

@@ -34,6 +34,8 @@ import (
"k8s.io/kubernetes/pkg/volume"
"k8s.io/kubernetes/pkg/volume/util"
"k8s.io/kubernetes/pkg/volume/validation"
"path"
"path/filepath"

This comment has been minimized.

@mlmhl

mlmhl Feb 3, 2018

Contributor

We need gofmt these two pkgs.

This comment has been minimized.

@dhirajh

dhirajh Feb 5, 2018

Author Contributor

Fixed goimports.


var _ volume.BlockVolumeMapper = &localVolumeMapper{}

// SetUpDevice provides physical device path for the local PV

This comment has been minimized.

@ianchakeres

ianchakeres Feb 3, 2018

Member

nit: periods at the end of comments.

This comment has been minimized.

@dhirajh

dhirajh Feb 5, 2018

Author Contributor

Fixed.

@@ -307,3 +359,43 @@ func (u *localVolumeUnmounter) TearDownAt(dir string) error {
glog.V(4).Infof("Unmounting volume %q at path %q\n", u.volName, dir)
return util.UnmountMountPoint(dir, u.mounter, true) /* extensiveMountPointCheck = true */
}

// Block Volume

This comment has been minimized.

@ianchakeres

ianchakeres Feb 3, 2018

Member

nit: This comment isn't adding much value. I recommend removing or expanding it.

This comment has been minimized.

@dhirajh

dhirajh Feb 5, 2018

Author Contributor

Fixed.

return filepath.EvalSymlinks(m.globalPath)
}

type localVolumeUnMapper struct {

This comment has been minimized.

@ianchakeres

ianchakeres Feb 3, 2018

Member

/UnMapper/Unmapper/

This comment has been minimized.

@dhirajh

dhirajh Feb 5, 2018

Author Contributor

Fixed.

@@ -197,8 +222,63 @@ func TestMountUnmount(t *testing.T) {
}
}

func TestMapUnmap(t *testing.T) {

This comment has been minimized.

@ianchakeres

ianchakeres Feb 3, 2018

Member

We should mention that this method is testing block.

This comment has been minimized.

@dhirajh

dhirajh Feb 5, 2018

Author Contributor

Fixed.

defer os.RemoveAll(tmpDir)

pod := &v1.Pod{ObjectMeta: metav1.ObjectMeta{UID: types.UID("poduid")}}
volSpec := getTestVolume(false, tmpDir, true)

This comment has been minimized.

@ianchakeres

ianchakeres Feb 3, 2018

Member

getTestVolume(false, tmpDir, true) => getTestVolume(false, tmpDir, true /*isBlock*/)

This comment has been minimized.

@dhirajh

dhirajh Feb 5, 2018

Author Contributor

Fixed.

}
devPath, err := mapper.SetUpDevice()
if err != nil {
t.Errorf("Expected success, got: %v", err)

This comment has been minimized.

@ianchakeres

ianchakeres Feb 3, 2018

Member

"Failed to SetUpDevice, err:"

This comment has been minimized.

@dhirajh

dhirajh Feb 5, 2018

Author Contributor

Fixed.

}

if err := unmapper.TearDownDevice(globalPath, devPath); err != nil {
t.Errorf("Expected success, got: %v", err)

This comment has been minimized.

@ianchakeres

ianchakeres Feb 3, 2018

Member

"TearDownDevice failed, err:"

This comment has been minimized.

@dhirajh

dhirajh Feb 5, 2018

Author Contributor

Fixed.

Spec: v1.PersistentVolumeSpec{
PersistentVolumeSource: v1.PersistentVolumeSource{
Local: &v1.LocalVolumeSource{
Path: "",

This comment has been minimized.

@mtanino

mtanino Feb 3, 2018

Member

Seems both ConstructBlockVolumeSpec() and ConstructVolumeSpec() can't construct proper volume spec. Any reason for this?

This comment has been minimized.

@dhirajh

dhirajh Feb 5, 2018

Author Contributor

Fixed ConstructBlockVolumeSpec() to specify the device path. Regarding ConstructVolumeSpec(), @msau42 mentioned that it was not done because the device path was not needed.

This comment has been minimized.

@mtanino

mtanino Feb 5, 2018

Member

@dhirajh

ConstructVolumeSpec(), @msau42 mentioned that it was not done because the device path was not needed.

Could you let me know the detail? I couldn't find her comment.

This comment has been minimized.

@dhirajh

dhirajh Feb 5, 2018

Author Contributor

It was from a chat, that I had a week back. @msau42 please do correct me if I missed something. The path is not needed for unmounting. The only thing the local volume plugin does is unmount the kubelet's pod volume bind mount. It leaves the real path unchanged.

This comment has been minimized.

@msau42

msau42 Feb 5, 2018

Member

@mtanino local volumes are very simple and don't need the underlying path to do teardown.

For filesystem volumes: Setup will bind mount the path to the kubelet's volume directory. Teardown only needs the kubelet's volume directory to unmount it.

For block volumes: I think it should be similar. Setup returns the path. Kubelet will create the appropriate mappings for it. For teardown, the plugin doesn't need to do anything, just relies on kubelet to unmap everything. Maybe one problem here is if the reconstruction also calls setup? Then we do not have the underlying path to return. We should probably fail or return an empty path. Does the new reconstruction design that only does teardown help with this?

This comment has been minimized.

@mtanino

mtanino Feb 5, 2018

Member

@msau42

local volumes are very simple and don't need the underlying path to do teardown.

I see. So during reconstruction, local volume plugin doesn't need to return the Path into reconstruct volume spec for both filesystem and block, right?

Maybe one problem here is if the reconstruction also calls setup? Then we do not have the >>underlying path to return. We should probably fail or return an empty path. Does the new
reconstruction design that only does teardown help with this?

IIUC, volume reconstruction doesn't call setUp(). It calls following methods during reconstruction.

  • plugin's constructVolumeSpec()
  • kubelet's GenerateUnmountVolumeFunc()
  • kubelet's GenerateUnmountDeviceFunc()
    => plugin's TearDown()

(*) block volume case is similar to above steps.

In the new reconstruction design, @jingxu97 is proposing two clean up methods based on reconstruction method support by plugin.

  1. reconciler updates volume information using reconstruction into the states and reconciler takes care of volume cleanup
  2. reconciler cleans up the mounts directly using tearDown() without reconstruction

Either way, setUp() isn't called. Which way would you prefer for local volume plugin?

This comment has been minimized.

@msau42

msau42 Feb 5, 2018

Member

Yeah so if only TearDown is called, then local volume is fine with a blank spec. Only volume name needs to be correctly populated. I would prefer having the cleanup down by reconciler so that it can retry if it fails the first time.

This comment has been minimized.

@mtanino

mtanino Feb 5, 2018

Member

yes, agree. Added comment on reconstruction design doc.

@msau42

This comment has been minimized.

Copy link
Member

msau42 commented Feb 3, 2018

/assign

plug, err := plugMgr.FindMapperPluginByName(localVolumePluginName)
if err != nil {
os.RemoveAll(tmpDir)
t.Fatalf("Can't find the plugin by name")

This comment has been minimized.

@ianchakeres

ianchakeres Feb 5, 2018

Member

nit: Can you please print the name of the plugin, that failed to be found?

This comment has been minimized.

@dhirajh

dhirajh Feb 6, 2018

Author Contributor

Fixed.


// SetUpDevice provides physical device path for the local PV.
func (m *localVolumeMapper) SetUpDevice() (string, error) {
return filepath.EvalSymlinks(m.globalPath)

This comment has been minimized.

@msau42

msau42 Feb 5, 2018

Member

Why do we need to eval symlinks?

This comment has been minimized.

@dhirajh

dhirajh Feb 5, 2018

Author Contributor

@msau42 The primary consumer of this function seems to be this line: https://github.com/kubernetes/kubernetes/blob/master/pkg/volume/util/operationexecutor/operation_generator.go#L855

The returned path is used as the source for creating symbolic links at lines 874 and 887. I thought it would be better that they got created using the true device as source, rather than symbolic link to symbolic link. Let me know if this is not ideal.

This comment has been minimized.

@msau42

msau42 Feb 5, 2018

Member

My only concern is would it make debugging slightly harder? Because if you try searching for the path in the spec, you won't see it if it got resolved to something else.

This comment has been minimized.

@dhirajh

dhirajh Feb 6, 2018

Author Contributor

Fixed.

}

// GetGlobalMapPath returns global map path and error.
// path: plugins/kubernetes.io/kubernetes.io~local-volume/volumeDevices/{volumeName}

This comment has been minimized.

@mtanino

mtanino Feb 6, 2018

Member

I guess correct path is,

plugins/kubernetes.io/local-volume/volumeDevices/{volumeName}

This comment has been minimized.

@dhirajh

dhirajh Feb 6, 2018

Author Contributor

Fixed.

testMountPath = "pods/poduid/volumes/kubernetes.io~local-volume/pvA"
testGlobalPath = "plugins/kubernetes.io~local-volume/volumeDevices/pvA"
testPodPath = "pods/poduid/volumeDevices/kubernetes.io~local-volume"
testDev = "fakedev"

This comment has been minimized.

@rootfs

rootfs Feb 6, 2018

Member

nit; fakeDev

This comment has been minimized.

@dhirajh

dhirajh Feb 6, 2018

Author Contributor

Fixed.

t.Fatalf("PersistentVolume object nil")
}

if *spec.PersistentVolume.Spec.VolumeMode != v1.PersistentVolumeBlock {

This comment has been minimized.

@rootfs

rootfs Feb 6, 2018

Member

spec.PersistentVolume.Spec.VolumeMode == nil || *spec.PersistentVolume.Spec.VolumeMode != v1.PersistentVolumeBlock

This comment has been minimized.

@dhirajh

dhirajh Feb 6, 2018

Author Contributor

Fixed.

}

if *spec.PersistentVolume.Spec.VolumeMode != v1.PersistentVolumeBlock {
t.Errorf("Unexected volume mode %q", *spec.PersistentVolume.Spec.VolumeMode)

This comment has been minimized.

@rootfs

rootfs Feb 6, 2018

Member

validate pointer here too

This comment has been minimized.

@dhirajh

dhirajh Feb 6, 2018

Author Contributor

Fixed as part of above change.


unmapper, err := plug.NewBlockVolumeUnmapper(testPVName, pod.UID)
if err != nil {
t.Errorf("Failed to make a new Unmapper: %v", err)

This comment has been minimized.

@rootfs

rootfs Feb 6, 2018

Member

Fatalf (though checking unmapper == nil below can guard that too)

This comment has been minimized.

@dhirajh

dhirajh Feb 6, 2018

Author Contributor

Fixed.

Initial local PV block device plugin checkin.
Unit tests for block.

Add docs/test

Fix gofmt issues.

Address code review comments.

Remove evalsymlink fro setupDevice()

Address review comments.

@dhirajh dhirajh force-pushed the dhirajh:localblock branch from 1175a3a to 8b8b856 Feb 6, 2018

if err != nil {
t.Fatalf("Cannot close fakedev: %v", err)
}
podPath := path.Join(tmpDir, testPodPath)

This comment has been minimized.

@msau42

msau42 Feb 6, 2018

Member

Do you actually need to create/symlink the pod path here? The plugin itself doesn't actually do anything except pass through the device paths.

This comment has been minimized.

@dhirajh

dhirajh Feb 7, 2018

Author Contributor

Removed it.

@ianchakeres

This comment has been minimized.

Copy link
Member

ianchakeres commented Feb 7, 2018

/test pull-kubernetes-bazel-test

@msau42

This comment has been minimized.

Copy link
Member

msau42 commented Feb 7, 2018

/lgtm

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

@msau42

This comment has been minimized.

Copy link
Member

msau42 commented Feb 7, 2018

Can you fix the release note formatting so the bot will recognize it?

@msau42

This comment has been minimized.

Copy link
Member

msau42 commented Feb 7, 2018

/assign @rootfs

@msau42

This comment has been minimized.

Copy link
Member

msau42 commented Feb 7, 2018

Can you also create an issue for this and reference it in the initial comment?

@dhirajh

This comment has been minimized.

Copy link
Contributor Author

dhirajh commented Feb 7, 2018

@msau42 created issue #59500 and updated formatting of release notes.

@rootfs

This comment has been minimized.

Copy link
Member

rootfs commented Feb 7, 2018

/approve

@rootfs

This comment has been minimized.

Copy link
Member

rootfs commented Feb 7, 2018

/approve no-issue

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Feb 7, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dhirajh, msau42, rootfs

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 OWNERS Files:

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

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Feb 7, 2018

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

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Feb 7, 2018

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

@k8s-github-robot k8s-github-robot merged commit c37b5d7 into kubernetes:master Feb 7, 2018

12 of 13 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-e2e-gce
Details
cla/linuxfoundation dhirajh 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
You can’t perform that action at this time.