Merge pull request #96021 from huffmanca/dont-remove-volume

Dont remove volumes when saveVolumeData fails

pkg/volume/csi/csi_attacher.go

@@ -296,13 +296,8 @@ func (c *csiAttacher) MountDevice(spec *volume.Spec, devicePath string, deviceMo
 		volDataKey.volHandle:  csiSource.VolumeHandle,
 		volDataKey.driverName: csiSource.Driver,
 	}
-	if err = saveVolumeData(dataDir, volDataFileName, data); err != nil {
+
-		klog.Error(log("failed to save volume info data: %v", err))
+	err = saveVolumeData(dataDir, volDataFileName, data)
-		if cleanErr := os.RemoveAll(dataDir); cleanErr != nil {
-			klog.Error(log("failed to remove dir after error [%s]: %v", dataDir, cleanErr))
-		}
-		return err
-	}
 	defer func() {
 		// Only if there was an error and volume operation was considered
 		// finished, we should remove the directory.
@@ -315,6 +310,12 @@ func (c *csiAttacher) MountDevice(spec *volume.Spec, devicePath string, deviceMo
 		}
 	}()
 
+	if err != nil {
+		errMsg := log("failed to save volume info data: %v", err)
+		klog.Error(errMsg)
+		return errors.New(errMsg)
+	}
+
 	if !stageUnstageSet {
 		klog.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.

pkg/volume/csi/csi_attacher_test.go

@@ -21,6 +21,7 @@ import (
 	"fmt"
 	"io/ioutil"
 	"os"
+	"os/user"
 	"path/filepath"
 	"reflect"
 	"sync"
@@ -1112,15 +1113,16 @@ func TestAttacherMountDevice(t *testing.T) {
 	transientError := volumetypes.NewTransientOperationFailure("")
 
 	testCases := []struct {
-		testName         string
+		testName                string
-		volName          string
+		volName                 string
-		devicePath       string
+		devicePath              string
-		deviceMountPath  string
+		deviceMountPath         string
-		stageUnstageSet  bool
+		stageUnstageSet         bool
-		shouldFail       bool
+		shouldFail              bool
-		createAttachment bool
+		createAttachment        bool
-		exitError        error
+		populateDeviceMountPath bool
-		spec             *volume.Spec
+		exitError               error
+		spec                    *volume.Spec
 	}{
 		{
 			testName:         "normal PV",
@@ -1210,9 +1212,24 @@ func TestAttacherMountDevice(t *testing.T) {
 			exitError:        nonFinalError,
 			shouldFail:       true,
 		},
+		{
+			testName:                "failure PV with existing data",
+			volName:                 "test-vol1",
+			devicePath:              "path1",
+			deviceMountPath:         "path2",
+			stageUnstageSet:         true,
+			createAttachment:        true,
+			populateDeviceMountPath: true,
+			shouldFail:              true,
+			spec:                    volume.NewSpecFromPersistentVolume(makeTestPV(pvName, 10, testDriver, "test-vol1"), true),
+		},
 	}
 
 	for _, tc := range testCases {
+		user, _ := user.Current()
+		if tc.populateDeviceMountPath && user.Uid == "0" {
+			t.Skipf("Skipping intentional failure on existing data when running as root.")
+		}
 		t.Run(tc.testName, func(t *testing.T) {
 			t.Logf("Running test case: %s", tc.testName)
 
@@ -1254,6 +1271,25 @@ func TestAttacherMountDevice(t *testing.T) {
 				}()
 			}
 
+			parent := filepath.Dir(tc.deviceMountPath)
+			filePath := filepath.Join(parent, "newfile")
+			if tc.populateDeviceMountPath {
+				// We need to create the deviceMountPath before we Mount,
+				// so that we can correctly create the file without errors.
+				err := os.MkdirAll(tc.deviceMountPath, 0750)
+				if err != nil {
+					t.Errorf("error attempting to create the directory")
+				}
+				_, err = os.Create(filePath)
+				if err != nil {
+					t.Errorf("error attempting to populate file on parent path: %v", err)
+				}
+				err = os.Chmod(parent, 0555)
+				if err != nil {
+					t.Errorf("error attempting to modify directory permissions: %v", err)
+				}
+			}
+
 			// Run
 			err := csiAttacher.MountDevice(tc.spec, tc.devicePath, tc.deviceMountPath)
 
@@ -1262,6 +1298,22 @@ func TestAttacherMountDevice(t *testing.T) {
 				if !tc.shouldFail {
 					t.Errorf("test should not fail, but error occurred: %v", err)
 				}
+				if tc.populateDeviceMountPath {
+					// We're expecting saveVolumeData to fail, which is responsible
+					// for creating this file. It shouldn't exist.
+					_, err := os.Stat(parent + "/" + volDataFileName)
+					if !os.IsNotExist(err) {
+						t.Errorf("vol_data.json should not exist: %v", err)
+					}
+					_, err = os.Stat(filePath)
+					if os.IsNotExist(err) {
+						t.Errorf("expecting file to exist after err received: %v", err)
+					}
+					err = os.Chmod(parent, 0777)
+					if err != nil {
+						t.Errorf("failed to modify permissions after test: %v", err)
+					}
+				}
 				return
 			}
 			if err == nil && tc.shouldFail {

pkg/volume/csi/csi_plugin.go

@@ -43,6 +43,7 @@ import (
 	"k8s.io/kubernetes/pkg/features"
 	"k8s.io/kubernetes/pkg/volume"
 	"k8s.io/kubernetes/pkg/volume/csi/nodeinfomanager"
+	volumetypes "k8s.io/kubernetes/pkg/volume/util/types"
 )
 
 const (
@@ -456,11 +457,23 @@ func (p *csiPlugin) NewMounter(
 	attachID := getAttachmentName(volumeHandle, driverName, node)
 	volData[volDataKey.attachmentID] = attachID
 
-	if err := saveVolumeData(dataDir, volDataFileName, volData); err != nil {
+	err = saveVolumeData(dataDir, volDataFileName, volData)
-		if removeErr := os.RemoveAll(dataDir); removeErr != nil {
+	defer func() {
-			klog.Error(log("failed to remove dir after error [%s]: %v", dataDir, removeErr))
+		// Only if there was an error and volume operation was considered
+		// finished, we should remove the directory.
+		if err != nil && volumetypes.IsOperationFinishedError(err) {
+			// attempt to cleanup volume mount dir.
+			if err = removeMountDir(p, dir); err != nil {
+				klog.Error(log("attacher.MountDevice failed to remove mount dir after error [%s]: %v", dir, err))
+			}
 		}
-		return nil, errors.New(log("failed to save volume info data: %v", err))
+	}()
+
+	if err != nil {
+		errorMsg := log("csi.NewMounter failed to save volume info data: %v", err)
+		klog.Error(errorMsg)
+
+		return nil, errors.New(errorMsg)
 	}
 
 	klog.V(4).Info(log("mounter created successfully"))
@@ -701,11 +714,21 @@ func (p *csiPlugin) NewBlockVolumeMapper(spec *volume.Spec, podRef *api.Pod, opt
 		volDataKey.attachmentID: attachID,
 	}
 
-	if err := saveVolumeData(dataDir, volDataFileName, volData); err != nil {
+	err = saveVolumeData(dataDir, volDataFileName, volData)
-		if removeErr := os.RemoveAll(dataDir); removeErr != nil {
+	defer func() {
-			klog.Error(log("failed to remove dir after error [%s]: %v", dataDir, removeErr))
+		// Only if there was an error and volume operation was considered
+		// finished, we should remove the directory.
+		if err != nil && volumetypes.IsOperationFinishedError(err) {
+			// attempt to cleanup volume mount dir.
+			if err = removeMountDir(p, dataDir); err != nil {
+				klog.Error(log("attacher.MountDevice failed to remove mount dir after error [%s]: %v", dataDir, err))
+			}
 		}
-		return nil, errors.New(log("failed to save volume info data: %v", err))
+	}()
+	if err != nil {
+		errorMsg := log("csi.NewBlockVolumeMapper failed to save volume info data: %v", err)
+		klog.Error(errorMsg)
+		return nil, errors.New(errorMsg)
 	}
 
 	return mapper, nil