Merge pull request #57823 from mlmhl/rbd_device_mount_path

Automatic merge from submit-queue (batch tested with PRs 57823, 58091, 58093, 58096, 57020). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

fix rbd ConstructVolumeSpec bug

**What this PR does / why we need it**:

Fix rbd `ConstructVolumeSpec` bug as its global device mount path is not canonical. Full details are in #57744 .

**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 #57744 

**Release note**:

```release-note
NONE
```

/sig storage
/kind bug

pkg/volume/rbd/rbd.go

@@ -356,9 +356,28 @@ func (plugin *rbdPlugin) ConstructVolumeSpec(volumeName, mountPath string) (*vol
 		return nil, err
 	}
 	s := dstrings.Split(sourceName, "-image-")
+	if len(s) != 2 {
+		// The mountPath parameter is the volume mount path for a specific pod, its format
+		// is /var/lib/kubelet/pods/{podUID}/volumes/{volumePluginName}/{volumeName}.
+		// mounter.GetDeviceNameFromMount will find the device path(such as /dev/rbd0) by
+		// mountPath first, and then try to find the global device mount path from the mounted
+		// path list of this device. sourceName is extracted from this global device mount path.
+		// mounter.GetDeviceNameFromMount expects the global device mount path conforms to canonical
+		// format: /var/lib/kubelet/plugins/kubernetes.io/rbd/mounts/{pool}-image-{image}.
+		// If this assertion failed, it means that the global device mount path is created by
+		// the deprecated format: /var/lib/kubelet/plugins/kubernetes.io/rbd/rbd/{pool}-image-{image}.
+		// So we will try to check whether this old style global device mount path exist or not.
+		// If existed, extract the sourceName from this old style path, otherwise return an error.
+		glog.V(3).Infof("SourceName %s wrong, fallback to old format", sourceName)
+		sourceName, err = plugin.getDeviceNameFromOldMountPath(mounter, mountPath)
+		if err != nil {
+			return nil, err
+		}
+		s = dstrings.Split(sourceName, "-image-")
 		if len(s) != 2 {
 			return nil, fmt.Errorf("sourceName %s wrong, should be pool+\"-image-\"+imageName", sourceName)
 		}
+	}
 	rbdVolume := &v1.Volume{
 		Name: volumeName,
 		VolumeSource: v1.VolumeSource{
@@ -492,6 +511,22 @@ func (plugin *rbdPlugin) newUnmapperInternal(volName string, podUID types.UID, m
 	}, nil
 }
 
+func (plugin *rbdPlugin) getDeviceNameFromOldMountPath(mounter mount.Interface, mountPath string) (string, error) {
+	refs, err := mount.GetMountRefsByDev(mounter, mountPath)
+	if err != nil {
+		return "", err
+	}
+	// baseMountPath is the prefix of deprecated device global mounted path,
+	// such as: /var/lib/kubelet/plugins/kubernetes.io/rbd/rbd
+	baseMountPath := filepath.Join(plugin.host.GetPluginDir(rbdPluginName), "rbd")
+	for _, ref := range refs {
+		if dstrings.HasPrefix(ref, baseMountPath) {
+			return filepath.Rel(baseMountPath, ref)
+		}
+	}
+	return "", fmt.Errorf("can't find source name from mounted path: %s", mountPath)
+}
+
 func (plugin *rbdPlugin) NewDeleter(spec *volume.Spec) (volume.Deleter, error) {
 	if spec.PersistentVolume != nil && spec.PersistentVolume.Spec.RBD == nil {
 		return nil, fmt.Errorf("spec.PersistentVolumeSource.Spec.RBD is nil")

pkg/volume/rbd/rbd_test.go

@@ -325,7 +325,7 @@ func TestPlugin(t *testing.T) {
 			},
 		},
 		expectedDevicePath:      "/dev/rbd1",
-		expectedDeviceMountPath: fmt.Sprintf("%s/plugins/kubernetes.io/rbd/rbd/pool1-image-image1", tmpDir),
+		expectedDeviceMountPath: fmt.Sprintf("%s/plugins/kubernetes.io/rbd/mounts/pool1-image-image1", tmpDir),
 		expectedPodMountPath:    fmt.Sprintf("%s/pods/%s/volumes/kubernetes.io~rbd/vol1", tmpDir, podUID),
 	})
 	cases = append(cases, &testcase{
@@ -353,7 +353,7 @@ func TestPlugin(t *testing.T) {
 			},
 		},
 		expectedDevicePath:      "/dev/rbd1",
-		expectedDeviceMountPath: fmt.Sprintf("%s/plugins/kubernetes.io/rbd/rbd/pool2-image-image2", tmpDir),
+		expectedDeviceMountPath: fmt.Sprintf("%s/plugins/kubernetes.io/rbd/mounts/pool2-image-image2", tmpDir),
 		expectedPodMountPath:    fmt.Sprintf("%s/pods/%s/volumes/kubernetes.io~rbd/vol2", tmpDir, podUID),
 	})
 
@@ -450,3 +450,127 @@ func TestGetSecretNameAndNamespace(t *testing.T) {
 		t.Errorf("getSecretNameAndNamespace returned incorrect values, expected %s and %s but got %s and %s", secretName, secretNamespace, foundSecretName, foundSecretNamespace)
 	}
 }
+
+// https://github.com/kubernetes/kubernetes/issues/57744
+func TestGetDeviceMountPath(t *testing.T) {
+	tmpDir, err := utiltesting.MkTmpdir("rbd_test")
+	if err != nil {
+		t.Fatalf("error creating temp dir: %v", err)
+	}
+	defer os.RemoveAll(tmpDir)
+
+	fakeVolumeHost := volumetest.NewFakeVolumeHost(tmpDir, nil, nil)
+	plugMgr := volume.VolumePluginMgr{}
+	plugMgr.InitPlugins(ProbeVolumePlugins(), nil /* prober */, fakeVolumeHost)
+	plug, err := plugMgr.FindPluginByName("kubernetes.io/rbd")
+	if err != nil {
+		t.Errorf("Can't find the plugin by name")
+	}
+	fdm := NewFakeDiskManager()
+
+	// attacher
+	attacher, err := plug.(*rbdPlugin).newAttacherInternal(fdm)
+	if err != nil {
+		t.Errorf("Failed to make a new Attacher: %v", err)
+	}
+
+	pool, image := "pool", "image"
+	spec := volume.NewSpecFromVolume(&v1.Volume{
+		Name: "vol",
+		VolumeSource: v1.VolumeSource{
+			RBD: &v1.RBDVolumeSource{
+				CephMonitors: []string{"a", "b"},
+				RBDPool:      pool,
+				RBDImage:     image,
+				FSType:       "ext4",
+			},
+		},
+	})
+
+	deprecatedDir := fmt.Sprintf("%s/plugins/kubernetes.io/rbd/rbd/%s-image-%s", tmpDir, pool, image)
+	canonicalDir := fmt.Sprintf("%s/plugins/kubernetes.io/rbd/mounts/%s-image-%s", tmpDir, pool, image)
+
+	type testCase struct {
+		deprecated bool
+		targetPath string
+	}
+	for _, c := range []testCase{
+		{false, canonicalDir},
+		{true, deprecatedDir},
+	} {
+		if c.deprecated {
+			// This is a deprecated device mount path, we create it,
+			// and hope attacher.GetDeviceMountPath return c.targetPath.
+			if err := os.MkdirAll(c.targetPath, 0700); err != nil {
+				t.Fatalf("Create deprecated mount path failed: %v", err)
+			}
+		}
+		mountPath, err := attacher.GetDeviceMountPath(spec)
+		if err != nil {
+			t.Fatalf("GetDeviceMountPath failed: %v", err)
+		}
+		if mountPath != c.targetPath {
+			t.Errorf("Mismatch device mount path: wanted %s, got %s", c.targetPath, mountPath)
+		}
+	}
+}
+
+// https://github.com/kubernetes/kubernetes/issues/57744
+func TestConstructVolumeSpec(t *testing.T) {
+	tmpDir, err := utiltesting.MkTmpdir("rbd_test")
+	if err != nil {
+		t.Fatalf("error creating temp dir: %v", err)
+	}
+	defer os.RemoveAll(tmpDir)
+
+	fakeVolumeHost := volumetest.NewFakeVolumeHost(tmpDir, nil, nil)
+	plugMgr := volume.VolumePluginMgr{}
+	plugMgr.InitPlugins(ProbeVolumePlugins(), nil /* prober */, fakeVolumeHost)
+	plug, err := plugMgr.FindPluginByName("kubernetes.io/rbd")
+	if err != nil {
+		t.Errorf("Can't find the plugin by name")
+	}
+	fakeMounter := fakeVolumeHost.GetMounter(plug.GetPluginName()).(*mount.FakeMounter)
+
+	pool, image, volumeName := "pool", "image", "vol"
+	podMountPath := fmt.Sprintf("%s/pods/pod123/volumes/kubernetes.io~rbd/%s", tmpDir, volumeName)
+	deprecatedDir := fmt.Sprintf("%s/plugins/kubernetes.io/rbd/rbd/%s-image-%s", tmpDir, pool, image)
+	canonicalDir := fmt.Sprintf("%s/plugins/kubernetes.io/rbd/mounts/%s-image-%s", tmpDir, pool, image)
+
+	type testCase struct {
+		volumeName string
+		targetPath string
+	}
+
+	for _, c := range []testCase{
+		{"vol", canonicalDir},
+		{"vol", deprecatedDir},
+	} {
+		if err := os.MkdirAll(c.targetPath, 0700); err != nil {
+			t.Fatalf("Create mount path %s failed: %v", c.targetPath, err)
+		}
+		if err = fakeMounter.Mount("/dev/rbd0", c.targetPath, "fake", nil); err != nil {
+			t.Fatalf("Mount %s to %s failed: %v", c.targetPath, podMountPath, err)
+		}
+		if err = fakeMounter.Mount(c.targetPath, podMountPath, "fake", []string{"bind"}); err != nil {
+			t.Fatalf("Mount %s to %s failed: %v", c.targetPath, podMountPath, err)
+		}
+		spec, err := plug.ConstructVolumeSpec(c.volumeName, podMountPath)
+		if err != nil {
+			t.Errorf("ConstructVolumeSpec failed: %v", err)
+		} else {
+			if spec.Volume.RBD.RBDPool != pool {
+				t.Errorf("Mismatch rbd pool: wanted %s, got %s", pool, spec.Volume.RBD.RBDPool)
+			}
+			if spec.Volume.RBD.RBDImage != image {
+				t.Fatalf("Mismatch rbd image: wanted %s, got %s", image, spec.Volume.RBD.RBDImage)
+			}
+		}
+		if err = fakeMounter.Unmount(podMountPath); err != nil {
+			t.Fatalf("Unmount pod path %s failed: %v", podMountPath, err)
+		}
+		if err = fakeMounter.Unmount(c.targetPath); err != nil {
+			t.Fatalf("Unmount device path %s failed: %v", c.targetPath, err)
+		}
+	}
+}

pkg/volume/rbd/rbd_util.go

@@ -37,6 +37,7 @@ import (
 	"k8s.io/apimachinery/pkg/api/resource"
 	"k8s.io/apimachinery/pkg/util/wait"
 	fileutil "k8s.io/kubernetes/pkg/util/file"
+	"k8s.io/kubernetes/pkg/util/mount"
 	"k8s.io/kubernetes/pkg/util/node"
 	"k8s.io/kubernetes/pkg/volume"
 	volutil "k8s.io/kubernetes/pkg/volume/util"
@@ -110,9 +111,18 @@ func waitForPath(pool, image string, maxRetries int) (string, bool) {
 	return "", false
 }
 
-// make a directory like /var/lib/kubelet/plugins/kubernetes.io/pod/rbd/pool-image-image
+// make a directory like /var/lib/kubelet/plugins/kubernetes.io/rbd/mounts/pool-image-image
 func makePDNameInternal(host volume.VolumeHost, pool string, image string) string {
-	return path.Join(host.GetPluginDir(rbdPluginName), "rbd", pool+"-image-"+image)
+	// Backward compatibility for the deprecated format: /var/lib/kubelet/plugins/kubernetes.io/rbd/rbd/pool-image-image
+	deprecatedDir := path.Join(host.GetPluginDir(rbdPluginName), "rbd", pool+"-image-"+image)
+	info, err := os.Stat(deprecatedDir)
+	if err == nil && info.IsDir() {
+		// The device mount path has already been created with the deprecated format, return it.
+		glog.V(5).Infof("Deprecated format path %s found", deprecatedDir)
+		return deprecatedDir
+	}
+	// Return the canonical format path.
+	return path.Join(host.GetPluginDir(rbdPluginName), mount.MountsInGlobalPDPath, pool+"-image-"+image)
 }
 
 // make a directory like /var/lib/kubelet/plugins/kubernetes.io/rbd/volumeDevices/pool-image-image