From f5642bbe88d2702a8a3981dac4648a134177d72f Mon Sep 17 00:00:00 2001 From: Yecheng Fu Date: Fri, 30 Nov 2018 17:03:56 +0800 Subject: [PATCH 1/3] Fix device mountable volume names in DSW --- .../volumemanager/cache/desired_state_of_world.go | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/pkg/kubelet/volumemanager/cache/desired_state_of_world.go b/pkg/kubelet/volumemanager/cache/desired_state_of_world.go index d4745ee0b1b..cb15d19f75e 100644 --- a/pkg/kubelet/volumemanager/cache/desired_state_of_world.go +++ b/pkg/kubelet/volumemanager/cache/desired_state_of_world.go @@ -203,11 +203,12 @@ func (dsw *desiredStateOfWorld) AddPodToVolume( var volumeName v1.UniqueVolumeName - // The unique volume name used depends on whether the volume is attachable + // The unique volume name used depends on whether the volume is attachable/device-mountable // or not. attachable := dsw.isAttachableVolume(volumeSpec) - if attachable { - // For attachable volumes, use the unique volume name as reported by + deviceMountable := dsw.isDeviceMountableVolume(volumeSpec) + if attachable || deviceMountable { + // For attachable/device-mountable volumes, use the unique volume name as reported by // the plugin. volumeName, err = util.GetUniqueVolumeNameFromSpec(volumePlugin, volumeSpec) @@ -219,13 +220,11 @@ func (dsw *desiredStateOfWorld) AddPodToVolume( err) } } else { - // For non-attachable volumes, generate a unique name based on the pod + // For non-attachable and non-device-mountable volumes, generate a unique name based on the pod // namespace and name and the name of the volume within the pod. volumeName = util.GetUniqueVolumeNameForNonAttachableVolume(podName, volumePlugin, volumeSpec) } - deviceMountable := dsw.isDeviceMountableVolume(volumeSpec) - if _, volumeExists := dsw.volumesToMount[volumeName]; !volumeExists { dsw.volumesToMount[volumeName] = volumeToMount{ volumeName: volumeName, From 5ada29ac169b2157f28af78f8b1b03cba893c4c7 Mon Sep 17 00:00:00 2001 From: Yecheng Fu Date: Fri, 30 Nov 2018 17:58:14 +0800 Subject: [PATCH 2/3] Rename GetUniqueVolumeNameForNonAttachableVolume to GetUniqueVolumeNameFromSpecWithPod --- pkg/kubelet/volumemanager/cache/desired_state_of_world.go | 2 +- pkg/kubelet/volumemanager/reconciler/reconciler.go | 8 ++++++-- pkg/volume/util/util.go | 7 ++++--- 3 files changed, 11 insertions(+), 6 deletions(-) diff --git a/pkg/kubelet/volumemanager/cache/desired_state_of_world.go b/pkg/kubelet/volumemanager/cache/desired_state_of_world.go index cb15d19f75e..1c748505a26 100644 --- a/pkg/kubelet/volumemanager/cache/desired_state_of_world.go +++ b/pkg/kubelet/volumemanager/cache/desired_state_of_world.go @@ -222,7 +222,7 @@ func (dsw *desiredStateOfWorld) AddPodToVolume( } else { // For non-attachable and non-device-mountable volumes, generate a unique name based on the pod // namespace and name and the name of the volume within the pod. - volumeName = util.GetUniqueVolumeNameForNonAttachableVolume(podName, volumePlugin, volumeSpec) + volumeName = util.GetUniqueVolumeNameFromSpecWithPod(podName, volumePlugin, volumeSpec) } if _, volumeExists := dsw.volumesToMount[volumeName]; !volumeExists { diff --git a/pkg/kubelet/volumemanager/reconciler/reconciler.go b/pkg/kubelet/volumemanager/reconciler/reconciler.go index 1b6bbbfbe77..f57d6bf26e9 100644 --- a/pkg/kubelet/volumemanager/reconciler/reconciler.go +++ b/pkg/kubelet/volumemanager/reconciler/reconciler.go @@ -455,6 +455,10 @@ func (rc *reconciler) reconstructVolume(volume podVolume) (*reconstructedVolume, if err != nil { return nil, err } + deviceMountablePlugin, err := rc.volumePluginMgr.FindDeviceMountablePluginByName(volume.pluginName) + if err != nil { + return nil, err + } // Create pod object pod := &v1.Pod{ @@ -480,13 +484,13 @@ func (rc *reconciler) reconstructVolume(volume podVolume) (*reconstructedVolume, } var uniqueVolumeName v1.UniqueVolumeName - if attachablePlugin != nil { + if attachablePlugin != nil || deviceMountablePlugin != nil { uniqueVolumeName, err = util.GetUniqueVolumeNameFromSpec(plugin, volumeSpec) if err != nil { return nil, err } } else { - uniqueVolumeName = util.GetUniqueVolumeNameForNonAttachableVolume(volume.podName, plugin, volumeSpec) + uniqueVolumeName = util.GetUniqueVolumeNameFromSpecWithPod(volume.podName, plugin, volumeSpec) } // Check existence of mount point for filesystem volume or symbolic link for block volume isExist, checkErr := rc.operationExecutor.CheckVolumeExistenceOperation(volumeSpec, volume.mountPath, volumeSpec.Name(), rc.mounter, uniqueVolumeName, volume.podName, pod.UID, attachablePlugin) diff --git a/pkg/volume/util/util.go b/pkg/volume/util/util.go index 070961c2822..73bc7638a58 100644 --- a/pkg/volume/util/util.go +++ b/pkg/volume/util/util.go @@ -825,9 +825,10 @@ func GetUniqueVolumeName(pluginName, volumeName string) v1.UniqueVolumeName { return v1.UniqueVolumeName(fmt.Sprintf("%s/%s", pluginName, volumeName)) } -// GetUniqueVolumeNameForNonAttachableVolume returns the unique volume name -// for a non-attachable volume. -func GetUniqueVolumeNameForNonAttachableVolume( +// GetUniqueVolumeNameFromSpecWithPod returns a unique volume name with pod +// name included. This is useful to generate different names for different pods +// on same volume. +func GetUniqueVolumeNameFromSpecWithPod( podName types.UniquePodName, volumePlugin volume.VolumePlugin, volumeSpec *volume.Spec) v1.UniqueVolumeName { return v1.UniqueVolumeName( fmt.Sprintf("%s/%v-%s", volumePlugin.GetPluginName(), podName, volumeSpec.Name())) From 67552a8f6e0133829b9ef0c34f784b177f5c8153 Mon Sep 17 00:00:00 2001 From: Yecheng Fu Date: Tue, 11 Dec 2018 13:02:52 +0800 Subject: [PATCH 3/3] Add unit test to verify generated volume names. --- .../cache/desired_state_of_world_test.go | 162 ++++++++++++++++++ pkg/volume/testing/testing.go | 88 ++++++++++ 2 files changed, 250 insertions(+) diff --git a/pkg/kubelet/volumemanager/cache/desired_state_of_world_test.go b/pkg/kubelet/volumemanager/cache/desired_state_of_world_test.go index 42707169d92..b7df006316e 100644 --- a/pkg/kubelet/volumemanager/cache/desired_state_of_world_test.go +++ b/pkg/kubelet/volumemanager/cache/desired_state_of_world_test.go @@ -117,6 +117,168 @@ func Test_AddPodToVolume_Positive_ExistingPodExistingVolume(t *testing.T) { verifyVolumeExistsWithSpecNameInVolumeDsw(t, podName, volumeSpec.Name(), dsw) } +// Call AddPodToVolume() on different pods for different kinds of volumes +// Verities generated names are same for different pods if volume is device mountable or attachable +// Verities generated names are different for different pods if volume is not device mountble and attachable +func Test_AddPodToVolume_Positive_NamesForDifferentPodsAndDifferentVolumes(t *testing.T) { + // Arrange + fakeVolumeHost := volumetesting.NewFakeVolumeHost( + "", /* rootDir */ + nil, /* kubeClient */ + nil, /* plugins */ + ) + plugins := []volume.VolumePlugin{ + &volumetesting.FakeBasicVolumePlugin{ + Plugin: volumetesting.FakeVolumePlugin{ + PluginName: "basic", + }, + }, + &volumetesting.FakeDeviceMountableVolumePlugin{ + FakeBasicVolumePlugin: volumetesting.FakeBasicVolumePlugin{ + Plugin: volumetesting.FakeVolumePlugin{ + PluginName: "device-mountable", + }, + }, + }, + &volumetesting.FakeAttachableVolumePlugin{ + FakeDeviceMountableVolumePlugin: volumetesting.FakeDeviceMountableVolumePlugin{ + FakeBasicVolumePlugin: volumetesting.FakeBasicVolumePlugin{ + Plugin: volumetesting.FakeVolumePlugin{ + PluginName: "attachable", + }, + }, + }, + }, + } + volumePluginMgr := volume.VolumePluginMgr{} + volumePluginMgr.InitPlugins(plugins, nil /* prober */, fakeVolumeHost) + dsw := NewDesiredStateOfWorld(&volumePluginMgr) + + testcases := map[string]struct { + pod1 *v1.Pod + pod2 *v1.Pod + same bool + }{ + "basic": { + &v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pod1", + UID: "pod1uid", + }, + Spec: v1.PodSpec{ + Volumes: []v1.Volume{ + { + Name: "basic", + VolumeSource: v1.VolumeSource{}, + }, + }, + }, + }, + &v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pod2", + UID: "pod2uid", + }, + Spec: v1.PodSpec{ + Volumes: []v1.Volume{ + { + Name: "basic", + VolumeSource: v1.VolumeSource{}, + }, + }, + }, + }, + false, + }, + "device-mountable": { + &v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pod1", + UID: "pod1uid", + }, + Spec: v1.PodSpec{ + Volumes: []v1.Volume{ + { + Name: "device-mountable", + VolumeSource: v1.VolumeSource{}, + }, + }, + }, + }, + &v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pod2", + UID: "pod2uid", + }, + Spec: v1.PodSpec{ + Volumes: []v1.Volume{ + { + Name: "device-mountable", + VolumeSource: v1.VolumeSource{}, + }, + }, + }, + }, + true, + }, + "attachable": { + &v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pod1", + UID: "pod1uid", + }, + Spec: v1.PodSpec{ + Volumes: []v1.Volume{ + { + Name: "attachable", + VolumeSource: v1.VolumeSource{}, + }, + }, + }, + }, + &v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pod2", + UID: "pod2uid", + }, + Spec: v1.PodSpec{ + Volumes: []v1.Volume{ + { + Name: "attachable", + VolumeSource: v1.VolumeSource{}, + }, + }, + }, + }, + true, + }, + } + + // Act & Assert + for name, v := range testcases { + volumeSpec1 := &volume.Spec{Volume: &v.pod1.Spec.Volumes[0]} + volumeSpec2 := &volume.Spec{Volume: &v.pod2.Spec.Volumes[0]} + generatedVolumeName1, err1 := dsw.AddPodToVolume(util.GetUniquePodName(v.pod1), v.pod1, volumeSpec1, volumeSpec1.Name(), "") + generatedVolumeName2, err2 := dsw.AddPodToVolume(util.GetUniquePodName(v.pod2), v.pod2, volumeSpec2, volumeSpec2.Name(), "") + if err1 != nil { + t.Fatalf("test %q: AddPodToVolume failed. Expected: Actual: <%v>", name, err1) + } + if err2 != nil { + t.Fatalf("test %q: AddPodToVolume failed. Expected: Actual: <%v>", name, err2) + } + if v.same { + if generatedVolumeName1 != generatedVolumeName2 { + t.Fatalf("test %q: AddPodToVolume should generate same names, but got %q != %q", name, generatedVolumeName1, generatedVolumeName2) + } + } else { + if generatedVolumeName1 == generatedVolumeName2 { + t.Fatalf("test %q: AddPodToVolume should generate different names, but got %q == %q", name, generatedVolumeName1, generatedVolumeName2) + } + } + } + +} + // Populates data struct with a new volume/pod // Calls DeletePodFromVolume() to removes the pod // Verifies newly added pod/volume are deleted diff --git a/pkg/volume/testing/testing.go b/pkg/volume/testing/testing.go index 4c116aa5934..2791d9bd910 100644 --- a/pkg/volume/testing/testing.go +++ b/pkg/volume/testing/testing.go @@ -504,6 +504,94 @@ func (plugin *FakeVolumePlugin) VolumeLimitKey(spec *Spec) string { return plugin.LimitKey } +// FakeBasicVolumePlugin implements a basic volume plugin. It wrappers on +// FakeVolumePlugin but implements VolumePlugin interface only. +// It is useful to test logic involving plugin interfaces. +type FakeBasicVolumePlugin struct { + Plugin FakeVolumePlugin +} + +func (f *FakeBasicVolumePlugin) GetPluginName() string { + return f.Plugin.GetPluginName() +} + +func (f *FakeBasicVolumePlugin) GetVolumeName(spec *Spec) (string, error) { + return f.Plugin.GetVolumeName(spec) +} + +// CanSupport tests whether the plugin supports a given volume specification by +// testing volume spec name begins with plugin name or not. +// This is useful to choose plugin by volume in testing. +func (f *FakeBasicVolumePlugin) CanSupport(spec *Spec) bool { + return strings.HasPrefix(spec.Name(), f.GetPluginName()) +} + +func (f *FakeBasicVolumePlugin) ConstructVolumeSpec(ame, mountPath string) (*Spec, error) { + return f.Plugin.ConstructVolumeSpec(ame, mountPath) +} + +func (f *FakeBasicVolumePlugin) Init(ost VolumeHost) error { + return f.Plugin.Init(ost) +} + +func (f *FakeBasicVolumePlugin) NewMounter(spec *Spec, pod *v1.Pod, opts VolumeOptions) (Mounter, error) { + return f.Plugin.NewMounter(spec, pod, opts) +} + +func (f *FakeBasicVolumePlugin) NewUnmounter(volName string, podUID types.UID) (Unmounter, error) { + return f.Plugin.NewUnmounter(volName, podUID) +} + +func (f *FakeBasicVolumePlugin) RequiresRemount() bool { + return f.Plugin.RequiresRemount() +} + +func (f *FakeBasicVolumePlugin) SupportsBulkVolumeVerification() bool { + return f.Plugin.SupportsBulkVolumeVerification() +} + +func (f *FakeBasicVolumePlugin) SupportsMountOption() bool { + return f.Plugin.SupportsMountOption() +} + +var _ VolumePlugin = &FakeBasicVolumePlugin{} + +// FakeDeviceMountableVolumePlugin implements an device mountable plugin based on FakeBasicVolumePlugin. +type FakeDeviceMountableVolumePlugin struct { + FakeBasicVolumePlugin +} + +func (f *FakeDeviceMountableVolumePlugin) NewDeviceMounter() (DeviceMounter, error) { + return f.Plugin.NewDeviceMounter() +} + +func (f *FakeDeviceMountableVolumePlugin) NewDeviceUnmounter() (DeviceUnmounter, error) { + return f.Plugin.NewDeviceUnmounter() +} + +func (f *FakeDeviceMountableVolumePlugin) GetDeviceMountRefs(deviceMountPath string) ([]string, error) { + return f.Plugin.GetDeviceMountRefs(deviceMountPath) +} + +var _ VolumePlugin = &FakeDeviceMountableVolumePlugin{} +var _ DeviceMountableVolumePlugin = &FakeDeviceMountableVolumePlugin{} + +// FakeAttachableVolumePlugin implements an attachable plugin based on FakeDeviceMountableVolumePlugin. +type FakeAttachableVolumePlugin struct { + FakeDeviceMountableVolumePlugin +} + +func (f *FakeAttachableVolumePlugin) NewAttacher() (Attacher, error) { + return f.Plugin.NewAttacher() +} + +func (f *FakeAttachableVolumePlugin) NewDetacher() (Detacher, error) { + return f.Plugin.NewDetacher() +} + +var _ VolumePlugin = &FakeAttachableVolumePlugin{} +var _ AttachableVolumePlugin = &FakeAttachableVolumePlugin{} + type FakeFileVolumePlugin struct { }