Merge pull request #71509 from cofyc/fix71438

Fix device mountable volume names in DSW

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 {
+	deviceMountable := dsw.isDeviceMountableVolume(volumeSpec)
-		// For attachable volumes, use the unique volume name as reported by
+	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)
+		volumeName = util.GetUniqueVolumeNameFromSpecWithPod(podName, volumePlugin, volumeSpec)
 	}
 
-	deviceMountable := dsw.isDeviceMountableVolume(volumeSpec)
-
 	if _, volumeExists := dsw.volumesToMount[volumeName]; !volumeExists {
 		dsw.volumesToMount[volumeName] = volumeToMount{
 			volumeName:              volumeName,

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: <no error> Actual: <%v>", name, err1)
+		}
+		if err2 != nil {
+			t.Fatalf("test %q: AddPodToVolume failed. Expected: <no error> 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

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)

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 {
 }
 

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
+// GetUniqueVolumeNameFromSpecWithPod returns a unique volume name with pod
-// for a non-attachable volume.
+// name included. This is useful to generate different names for different pods
-func GetUniqueVolumeNameForNonAttachableVolume(
+// 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()))