Merge pull request #65529 from…

pkg/volume/csi/csi_attacher.go

@@ -245,7 +245,7 @@ func (c *csiAttacher) GetDeviceMountPath(spec *volume.Spec) (string, error) {
 	return deviceMountPath, nil
 }
 
-func (c *csiAttacher) MountDevice(spec *volume.Spec, devicePath string, deviceMountPath string) error {
+func (c *csiAttacher) MountDevice(spec *volume.Spec, devicePath string, deviceMountPath string) (err error) {
 	glog.V(4).Infof(log("attacher.MountDevice(%s, %s)", devicePath, deviceMountPath))
 
 	mounted, err := isDirMounted(c.plugin, deviceMountPath)
@@ -269,6 +269,35 @@ func (c *csiAttacher) MountDevice(spec *volume.Spec, devicePath string, deviceMo
 		return err
 	}
 
+	// Store volume metadata for UnmountDevice. Keep it around even if the
+	// driver does not support NodeStage, UnmountDevice still needs it.
+	if err = os.MkdirAll(deviceMountPath, 0750); err != nil {
+		glog.Error(log("attacher.MountDevice failed to create dir %#v:  %v", deviceMountPath, err))
+		return err
+	}
+	glog.V(4).Info(log("created target path successfully [%s]", deviceMountPath))
+	dataDir := filepath.Dir(deviceMountPath)
+	data := map[string]string{
+		volDataKey.volHandle:  csiSource.VolumeHandle,
+		volDataKey.driverName: csiSource.Driver,
+	}
+	if err = saveVolumeData(dataDir, volDataFileName, data); err != nil {
+		glog.Error(log("failed to save volume info data: %v", err))
+		if cleanerr := os.RemoveAll(dataDir); err != nil {
+			glog.Error(log("failed to remove dir after error [%s]: %v", dataDir, cleanerr))
+		}
+		return err
+	}
+	defer func() {
+		if err != nil {
+			// clean up metadata
+			glog.Errorf(log("attacher.MountDevice failed: %v", err))
+			if err := removeMountDir(c.plugin, deviceMountPath); err != nil {
+				glog.Error(log("attacher.MountDevice failed to remove mount dir after errir [%s]: %v", deviceMountPath, err))
+			}
+		}
+	}()
+
 	if c.csiClient == nil {
 		c.csiClient = newCsiDriverClient(csiSource.Driver)
 	}
@@ -279,17 +308,18 @@ func (c *csiAttacher) MountDevice(spec *volume.Spec, devicePath string, deviceMo
 	// Check whether "STAGE_UNSTAGE_VOLUME" is set
 	stageUnstageSet, err := hasStageUnstageCapability(ctx, csi)
 	if err != nil {
-		glog.Error(log("attacher.MountDevice failed to check STAGE_UNSTAGE_VOLUME: %v", err))
 		return err
 	}
 	if !stageUnstageSet {
 		glog.Infof(log("attacher.MountDevice STAGE_UNSTAGE_VOLUME capability not set. Skipping MountDevice..."))
+		// defer does *not* remove the metadata file and it's correct - UnmountDevice needs it there.
 		return nil
 	}
 
 	// Start MountDevice
 	if deviceMountPath == "" {
-		return fmt.Errorf("attacher.MountDevice failed, deviceMountPath is empty")
+		err = fmt.Errorf("attacher.MountDevice failed, deviceMountPath is empty")
+		return err
 	}
 
 	nodeName := string(c.plugin.host.GetNodeName())
@@ -298,13 +328,12 @@ func (c *csiAttacher) MountDevice(spec *volume.Spec, devicePath string, deviceMo
 	// search for attachment by VolumeAttachment.Spec.Source.PersistentVolumeName
 	attachment, err := c.k8s.StorageV1beta1().VolumeAttachments().Get(attachID, meta.GetOptions{})
 	if err != nil {
-		glog.Error(log("attacher.MountDevice failed while getting volume attachment [id=%v]: %v", attachID, err))
-		return err
+		return err // This err already has enough context ("VolumeAttachment xyz not found")
 	}
 
 	if attachment == nil {
-		glog.Error(log("unable to find VolumeAttachment [id=%s]", attachID))
-		return errors.New("no existing VolumeAttachment found")
+		err = errors.New("no existing VolumeAttachment found")
+		return err
 	}
 	publishVolumeInfo := attachment.Status.AttachmentMetadata
 
@@ -312,18 +341,12 @@ func (c *csiAttacher) MountDevice(spec *volume.Spec, devicePath string, deviceMo
 	if csiSource.NodeStageSecretRef != nil {
 		nodeStageSecrets, err = getCredentialsFromSecret(c.k8s, csiSource.NodeStageSecretRef)
 		if err != nil {
-			return fmt.Errorf("fetching NodeStageSecretRef %s/%s failed: %v",
+			err = fmt.Errorf("fetching NodeStageSecretRef %s/%s failed: %v",
 				csiSource.NodeStageSecretRef.Namespace, csiSource.NodeStageSecretRef.Name, err)
+			return err
 		}
 	}
 
-	// create target_dir before call to NodeStageVolume
-	if err := os.MkdirAll(deviceMountPath, 0750); err != nil {
-		glog.Error(log("attacher.MountDevice failed to create dir %#v:  %v", deviceMountPath, err))
-		return err
-	}
-	glog.V(4).Info(log("created target path successfully [%s]", deviceMountPath))
-
 	//TODO (vladimirvivien) implement better AccessModes mapping between k8s and CSI
 	accessMode := v1.ReadWriteOnce
 	if spec.PersistentVolume.Spec.AccessModes != nil {
@@ -341,11 +364,6 @@ func (c *csiAttacher) MountDevice(spec *volume.Spec, devicePath string, deviceMo
 		csiSource.VolumeAttributes)
 
 	if err != nil {
-		glog.Errorf(log("attacher.MountDevice failed: %v", err))
-		if err := removeMountDir(c.plugin, deviceMountPath); err != nil {
-			glog.Error(log("attacher.MountDevice failed to remove mount dir after a NodeStageVolume() error [%s]: %v", deviceMountPath, err))
-			return err
-		}
 		return err
 	}
 
@@ -458,10 +476,21 @@ func (c *csiAttacher) UnmountDevice(deviceMountPath string) error {
 	glog.V(4).Info(log("attacher.UnmountDevice(%s)", deviceMountPath))
 
 	// Setup
-	driverName, volID, err := getDriverAndVolNameFromDeviceMountPath(c.k8s, deviceMountPath)
-	if err != nil {
-		glog.Errorf(log("attacher.UnmountDevice failed to get driver and volume name from device mount path: %v", err))
-		return err
+	var driverName, volID string
+	dataDir := filepath.Dir(deviceMountPath)
+	data, err := loadVolumeData(dataDir, volDataFileName)
+	if err == nil {
+		driverName = data[volDataKey.driverName]
+		volID = data[volDataKey.volHandle]
+	} else {
+		glog.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 {
+			glog.Errorf(log("attacher.UnmountDevice failed to get driver and volume name from device mount path: %v", err))
+			return err
+		}
 	}
 
 	if c.csiClient == nil {
@@ -479,6 +508,11 @@ func (c *csiAttacher) UnmountDevice(deviceMountPath string) error {
 	}
 	if !stageUnstageSet {
 		glog.Infof(log("attacher.UnmountDevice STAGE_UNSTAGE_VOLUME capability not set. Skipping UnmountDevice..."))
+		// Just	delete the global directory + json file
+		if err := removeMountDir(c.plugin, deviceMountPath); err != nil {
+			return fmt.Errorf("failed to clean up gloubal mount %s: %s", dataDir, err)
+		}
+
 		return nil
 	}
 
@@ -492,6 +526,11 @@ func (c *csiAttacher) UnmountDevice(deviceMountPath string) error {
 		return err
 	}
 
+	// Delete the global directory + json file
+	if err := removeMountDir(c.plugin, deviceMountPath); err != nil {
+		return fmt.Errorf("failed to clean up gloubal mount %s: %s", dataDir, err)
+	}
+
 	glog.V(4).Infof(log("attacher.UnmountDevice successfully requested NodeStageVolume [%s]", deviceMountPath))
 	return nil
 }

pkg/volume/csi/csi_attacher_test.go

@@ -18,6 +18,7 @@ package csi
 
 import (
 	"fmt"
+	"io/ioutil"
 	"os"
 	"path/filepath"
 	"testing"
@@ -528,10 +529,6 @@ func TestAttacherMountDevice(t *testing.T) {
 			deviceMountPath: "path2",
 			stageUnstageSet: false,
 		},
-		{
-			testName:        "stage_unstage not set no vars should not fail",
-			stageUnstageSet: false,
-		},
 	}
 
 	for _, tc := range testCases {
@@ -610,45 +607,50 @@ func TestAttacherUnmountDevice(t *testing.T) {
 		testName        string
 		volID           string
 		deviceMountPath string
+		jsonFile        string
+		createPV        bool
 		stageUnstageSet bool
 		shouldFail      bool
 	}{
 		{
-			testName:        "normal",
+			testName:        "normal, json file exists",
 			volID:           "project/zone/test-vol1",
-			deviceMountPath: "/tmp/csi-test049507108/plugins/csi/pv/test-pv-name/globalmount",
+			deviceMountPath: "plugins/csi/pv/test-pv-name/globalmount",
+			jsonFile:        `{"driverName": "csi", "volumeHandle":"project/zone/test-vol1"}`,
+			createPV:        false,
 			stageUnstageSet: true,
 		},
 		{
-			testName:        "no device mount path",
+			testName:        "normal, json file doesn't exist -> use PV",
 			volID:           "project/zone/test-vol1",
-			deviceMountPath: "",
-			stageUnstageSet: true,
-			shouldFail:      true,
+			deviceMountPath: "plugins/csi/pv/test-pv-name/globalmount",
+			jsonFile:        "",
+			createPV:        true,
 		},
 		{
-			testName:        "missing part of device mount path",
+			testName:        "invalid json ->  use PV",
 			volID:           "project/zone/test-vol1",
-			deviceMountPath: "/tmp/csi-test049507108/plugins/csi/pv/test-pv-name/globalmount",
+			deviceMountPath: "plugins/csi/pv/test-pv-name/globalmount",
+			jsonFile:        `{"driverName"}}`,
+			createPV:        true,
 			stageUnstageSet: true,
-			shouldFail:      true,
 		},
 		{
-			testName:        "test volume name mismatch",
-			volID:           "project/zone/test-vol1",
-			deviceMountPath: "/tmp/csi-test049507108/plugins/csi/pv/test-pv-name/globalmount",
-			stageUnstageSet: true,
+			testName:        "no json, no PV.volID",
+			volID:           "",
+			deviceMountPath: "plugins/csi/pv/test-pv-name/globalmount",
+			jsonFile:        "",
+			createPV:        true,
 			shouldFail:      true,
 		},
 		{
-			testName:        "stage_unstage not set",
+			testName:        "no json, no PV",
 			volID:           "project/zone/test-vol1",
-			deviceMountPath: "/tmp/csi-test049507108/plugins/csi/pv/test-pv-name/globalmount",
-			stageUnstageSet: false,
-		},
-		{
-			testName:        "stage_unstage not set no vars should not fail",
-			stageUnstageSet: false,
+			deviceMountPath: "plugins/csi/pv/test-pv-name/globalmount",
+			jsonFile:        "",
+			createPV:        false,
+			stageUnstageSet: true,
+			shouldFail:      true,
 		},
 	}
 
@@ -665,29 +667,45 @@ func TestAttacherUnmountDevice(t *testing.T) {
 		csiAttacher := attacher.(*csiAttacher)
 		csiAttacher.csiClient = setupClient(t, tc.stageUnstageSet)
 
+		if tc.deviceMountPath != "" {
+			tc.deviceMountPath = filepath.Join(tmpDir, tc.deviceMountPath)
+		}
+
 		// Add the volume to NodeStagedVolumes
 		cdc := csiAttacher.csiClient.(*fakeCsiDriverClient)
 		cdc.nodeClient.AddNodeStagedVolume(tc.volID, tc.deviceMountPath)
 
-		// Make the PV for this object
+		// Make JSON for this object
+		if tc.deviceMountPath != "" {
+			if err := os.MkdirAll(tc.deviceMountPath, 0755); err != nil {
+				t.Fatalf("error creating directory %s: %s", tc.deviceMountPath, err)
+			}
+		}
 		dir := filepath.Dir(tc.deviceMountPath)
-		// dir is now /var/lib/kubelet/plugins/kubernetes.io/csi/pv/{pvname}
-		pvName := filepath.Base(dir)
-		pv := makeTestPV(pvName, 5, "csi", tc.volID)
-		_, err := csiAttacher.k8s.CoreV1().PersistentVolumes().Create(pv)
-		if err != nil && !tc.shouldFail {
-			t.Fatalf("Failed to create PV: %v", err)
+		if tc.jsonFile != "" {
+			dataPath := filepath.Join(dir, volDataFileName)
+			if err := ioutil.WriteFile(dataPath, []byte(tc.jsonFile), 0644); err != nil {
+				t.Fatalf("error creating %s: %s", dataPath, err)
+			}
+		}
+		if tc.createPV {
+			// Make the PV for this object
+			pvName := filepath.Base(dir)
+			pv := makeTestPV(pvName, 5, "csi", tc.volID)
+			_, err := csiAttacher.k8s.CoreV1().PersistentVolumes().Create(pv)
+			if err != nil && !tc.shouldFail {
+				t.Fatalf("Failed to create PV: %v", err)
+			}
 		}
 
 		// Run
-		err = csiAttacher.UnmountDevice(tc.deviceMountPath)
-
+		err := csiAttacher.UnmountDevice(tc.deviceMountPath)
 		// Verify
 		if err != nil {
 			if !tc.shouldFail {
 				t.Errorf("test should not fail, but error occurred: %v", err)
 			}
-			return
+			continue
 		}
 		if err == nil && tc.shouldFail {
 			t.Errorf("test should fail, but no error occurred")
@@ -710,6 +728,18 @@ func TestAttacherUnmountDevice(t *testing.T) {
 			t.Errorf("could not find expected staged volume: %s", tc.volID)
 		}
 
+		if tc.jsonFile != "" && !tc.shouldFail {
+			dataPath := filepath.Join(dir, volDataFileName)
+			if _, err := os.Stat(dataPath); !os.IsNotExist(err) {
+				if err != nil {
+					t.Errorf("error checking file %s: %s", dataPath, err)
+				} else {
+					t.Errorf("json file %s should not exists, but it does", dataPath)
+				}
+			} else {
+				t.Logf("json file %s was correctly removed", dataPath)
+			}
+		}
 	}
 }