fix: the volume is not detached after the pod and PVC objects are del…

…eted
kubernetes · Apr 14, 2023 · 67e993d · 67e993d
1 parent aec914e
commit 67e993d
Show file tree

Hide file tree

Showing 3 changed files with 11 additions and 82 deletions.
diff --git a/pkg/volume/csi/csi_attacher.go b/pkg/volume/csi/csi_attacher.go
@@ -584,14 +584,13 @@ func (c *csiAttacher) UnmountDevice(deviceMountPath string) error {
 		driverName = data[volDataKey.driverName]
 		volID = data[volDataKey.volHandle]
 	} else {
-		klog.Error(log("UnmountDevice failed to load volume data file [%s]: %v", dataDir, err))
-
-		// The volume might have been mounted by old CSI volume plugin. Fall back to the old behavior: read PV from API server
-		driverName, volID, err = getDriverAndVolNameFromDeviceMountPath(c.k8s, deviceMountPath)
-		if err != nil {
-			klog.Errorf(log("attacher.UnmountDevice failed to get driver and volume name from device mount path: %v", err))
-			return err
+		if errors.Is(err, os.ErrNotExist) {
+			klog.V(4).Info(log("attacher.UnmountDevice skipped because volume data file [%s] does not exist", dataDir))
+			return nil
 		}
+
+		klog.Errorf(log("attacher.UnmountDevice failed to get driver and volume name from device mount path: %v", err))
+		return err
 	}
 
 	if c.csiClient == nil {
@@ -671,36 +670,6 @@ func makeDeviceMountPath(plugin *csiPlugin, spec *volume.Spec) (string, error) {
 	return filepath.Join(plugin.host.GetPluginDir(plugin.GetPluginName()), driver, volSha, globalMountInGlobalPath), nil
 }
 
-func getDriverAndVolNameFromDeviceMountPath(k8s kubernetes.Interface, deviceMountPath string) (string, string, error) {
-	// deviceMountPath structure: /var/lib/kubelet/plugins/kubernetes.io/csi/pv/{pvname}/globalmount
-	dir := filepath.Dir(deviceMountPath)
-	if file := filepath.Base(deviceMountPath); file != globalMountInGlobalPath {
-		return "", "", errors.New(log("getDriverAndVolNameFromDeviceMountPath failed, path did not end in %s", globalMountInGlobalPath))
-	}
-	// dir is now /var/lib/kubelet/plugins/kubernetes.io/csi/pv/{pvname}
-	pvName := filepath.Base(dir)
-
-	// Get PV and check for errors
-	pv, err := k8s.CoreV1().PersistentVolumes().Get(context.TODO(), pvName, metav1.GetOptions{})
-	if err != nil {
-		return "", "", err
-	}
-	if pv == nil || pv.Spec.CSI == nil {
-		return "", "", errors.New(log("getDriverAndVolNameFromDeviceMountPath could not find CSI Persistent Volume Source for pv: %s", pvName))
-	}
-
-	// Get VolumeHandle and PluginName from pv
-	csiSource := pv.Spec.CSI
-	if csiSource.Driver == "" {
-		return "", "", errors.New(log("getDriverAndVolNameFromDeviceMountPath failed, driver name empty"))
-	}
-	if csiSource.VolumeHandle == "" {
-		return "", "", errors.New(log("getDriverAndVolNameFromDeviceMountPath failed, VolumeHandle empty"))
-	}
-
-	return csiSource.Driver, csiSource.VolumeHandle, nil
-}
-
 func verifyAttachmentStatus(attachment *storage.VolumeAttachment, volumeHandle string) (bool, error) {
 	// when we received a deleted event during attachment, fail fast
 	if attachment == nil {

diff --git a/pkg/volume/csi/csi_attacher_test.go b/pkg/volume/csi/csi_attacher_test.go
@@ -1616,12 +1616,11 @@ func TestAttacherUnmountDevice(t *testing.T) {
 		},
 		// PV agnostic path negative test cases
 		{
-			testName:        "fail: missing json, fail to retrieve driver and volumeID from globalpath",
-			volID:           "project/zone/test-vol1",
+			testName:        "success: json file doesn't exist, unmount device is skipped",
 			deviceMountPath: "plugins/csi/" + generateSha("project/zone/test-vol1") + "/globalmount",
 			jsonFile:        "",
 			stageUnstageSet: true,
-			shouldFail:      true,
+			createPV:        true,
 		},
 		{
 			testName:        "fail: invalid json, fail to retrieve driver and volumeID from globalpath",
@@ -1631,45 +1630,6 @@ func TestAttacherUnmountDevice(t *testing.T) {
 			stageUnstageSet: true,
 			shouldFail:      true,
 		},
-		// Old style PV based path positive test cases
-		{
-			testName:        "success: json file doesn't exist, old style pv based global path -> use PV",
-			volID:           "project/zone/test-vol1",
-			deviceMountPath: "plugins/csi/pv/test-pv-name/globalmount",
-			jsonFile:        "",
-			stageUnstageSet: true,
-			createPV:        true,
-		},
-		{
-			testName:        "success: invalid json file, old style pv based global path -> use PV",
-			volID:           "project/zone/test-vol1",
-			deviceMountPath: "plugins/csi/pv/test-pv-name/globalmount",
-			jsonFile:        `{"driverName"}}`,
-			stageUnstageSet: true,
-			createPV:        true,
-		},
-		{
-			testName:        "stage_unstage not set, PV based path, unmount device is skipped",
-			deviceMountPath: "plugins/csi/pv/test-pv-name/globalmount",
-			jsonFile:        `{"driverName":"test-driver","volumeHandle":"test-vol1"}`,
-			stageUnstageSet: false,
-		},
-		// Old style PV based path negative test cases
-		{
-			testName:        "fail: json file doesn't exist, old style pv based device path, missing PV",
-			volID:           "project/zone/test-vol1",
-			deviceMountPath: "plugins/csi/pv/test-pv-name/globalmount",
-			jsonFile:        "",
-			stageUnstageSet: true,
-			shouldFail:      true,
-		},
-		{
-			testName:        "fail: no json, no PV.volID, old style pv based global path",
-			volID:           "",
-			deviceMountPath: "plugins/csi/pv/test-pv-name/globalmount",
-			jsonFile:        "",
-			shouldFail:      true,
-		},
 	}
 
 	for _, tc := range testCases {
@@ -1733,7 +1693,7 @@ func TestAttacherUnmountDevice(t *testing.T) {
 
 			// Verify call goes through all the way
 			expectedSet := 0
-			if !tc.stageUnstageSet {
+			if !tc.stageUnstageSet || tc.volID == "" {
 				expectedSet = 1
 			}
 			staged := cdc.nodeClient.GetNodeStagedVolumes()
@@ -1742,7 +1702,7 @@ func TestAttacherUnmountDevice(t *testing.T) {
 			}
 
 			_, ok := staged[tc.volID]
-			if ok && tc.stageUnstageSet {
+			if ok && tc.stageUnstageSet && tc.volID != "" {
 				t.Errorf("found unexpected staged volume: %s", tc.volID)
 			} else if !ok && !tc.stageUnstageSet {
 				t.Errorf("could not find expected staged volume: %s", tc.volID)

diff --git a/pkg/volume/csi/csi_util.go b/pkg/volume/csi/csi_util.go
@@ -81,7 +81,7 @@ func loadVolumeData(dir string, fileName string) (map[string]string, error) {
 
 	file, err := os.Open(dataFileName)
 	if err != nil {
-		return nil, errors.New(log("failed to open volume data file [%s]: %v", dataFileName, err))
+		return nil, fmt.Errorf("%s: %w", log("failed to open volume data file [%s]", dataFileName), err)
 	}
 	defer file.Close()
 	data := map[string]string{}