From 401e8446a18b8b232bebee43acc1bc49f134982f Mon Sep 17 00:00:00 2001 From: weizhichen Date: Tue, 28 Feb 2023 17:14:00 +0000 Subject: [PATCH] fix: the volume is not detached after the pod and PVC objects are deleted --- pkg/volume/csi/csi_attacher.go | 43 ++++---------------------- pkg/volume/csi/csi_attacher_test.go | 48 +++-------------------------- pkg/volume/csi/csi_util.go | 2 +- 3 files changed, 11 insertions(+), 82 deletions(-) diff --git a/pkg/volume/csi/csi_attacher.go b/pkg/volume/csi/csi_attacher.go index 8ffb3acf49cf..ef3c98258ac8 100644 --- a/pkg/volume/csi/csi_attacher.go +++ b/pkg/volume/csi/csi_attacher.go @@ -595,14 +595,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 { @@ -682,36 +681,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 index 32231ec5aa98..1494316eac02 100644 --- a/pkg/volume/csi/csi_attacher_test.go +++ b/pkg/volume/csi/csi_attacher_test.go @@ -1596,12 +1596,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", @@ -1611,45 +1610,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 { @@ -1713,7 +1673,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() @@ -1722,7 +1682,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 index ee2bdc193b32..bb4d799ff3c8 100644 --- a/pkg/volume/csi/csi_util.go +++ b/pkg/volume/csi/csi_util.go @@ -79,7 +79,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{}