From b9afd2d34ad90bddb6e7ebfff161b9c7f8b9a348 Mon Sep 17 00:00:00 2001 From: fankangbest Date: Wed, 25 Mar 2020 14:00:56 +0800 Subject: [PATCH 1/3] do not remove volume dir when saveVolumeData fails --- pkg/volume/csi/csi_attacher.go | 8 +++----- pkg/volume/csi/csi_plugin.go | 11 ++++++++--- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/pkg/volume/csi/csi_attacher.go b/pkg/volume/csi/csi_attacher.go index fbeebdb40f71..9aecfdd2ae75 100644 --- a/pkg/volume/csi/csi_attacher.go +++ b/pkg/volume/csi/csi_attacher.go @@ -297,11 +297,9 @@ func (c *csiAttacher) MountDevice(spec *volume.Spec, devicePath string, deviceMo volDataKey.driverName: csiSource.Driver, } if err = saveVolumeData(dataDir, volDataFileName, data); err != nil { - klog.Error(log("failed to save volume info data: %v", err)) - if cleanErr := os.RemoveAll(dataDir); cleanErr != nil { - klog.Error(log("failed to remove dir after error [%s]: %v", dataDir, cleanErr)) - } - return err + errMsg := log("failed to save volume info data: %v", err) + klog.Error(errMsg) + return errors.New(errMsg) } defer func() { // Only if there was an error and volume operation was considered diff --git a/pkg/volume/csi/csi_plugin.go b/pkg/volume/csi/csi_plugin.go index 5291a729e7da..aaae2f7bdf24 100644 --- a/pkg/volume/csi/csi_plugin.go +++ b/pkg/volume/csi/csi_plugin.go @@ -457,10 +457,15 @@ func (p *csiPlugin) NewMounter( volData[volDataKey.attachmentID] = attachID if err := saveVolumeData(dataDir, volDataFileName, volData); err != nil { - if removeErr := os.RemoveAll(dataDir); removeErr != nil { - klog.Error(log("failed to remove dir after error [%s]: %v", dataDir, removeErr)) + errorMsg := log("csi.NewMounter failed to save volume info data: %v", err) + klog.Error(errorMsg) + + // attempt to cleanup volume mount dir. + if removeMountDirErr := removeMountDir(p, dir); removeMountDirErr != nil { + klog.Error(log("csi.NewMounter failed to remove mount dir [%s]: %v", dir, removeMountDirErr)) } - return nil, errors.New(log("failed to save volume info data: %v", err)) + + return nil, errors.New(errorMsg) } klog.V(4).Info(log("mounter created successfully")) From df3119e815bf28966ecde53bcc45eda1864b43b1 Mon Sep 17 00:00:00 2001 From: Christian Huffman Date: Thu, 29 Oct 2020 16:00:59 -0400 Subject: [PATCH 2/3] Adjust defer to correctly call --- pkg/volume/csi/csi_attacher.go | 13 +++++++----- pkg/volume/csi/csi_plugin.go | 38 +++++++++++++++++++++++++--------- 2 files changed, 36 insertions(+), 15 deletions(-) diff --git a/pkg/volume/csi/csi_attacher.go b/pkg/volume/csi/csi_attacher.go index 9aecfdd2ae75..0b668ec8fcff 100644 --- a/pkg/volume/csi/csi_attacher.go +++ b/pkg/volume/csi/csi_attacher.go @@ -296,11 +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 { - errMsg := log("failed to save volume info data: %v", err) - klog.Error(errMsg) - return errors.New(errMsg) - } + + err = saveVolumeData(dataDir, volDataFileName, data) defer func() { // Only if there was an error and volume operation was considered // finished, we should remove the directory. @@ -313,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. diff --git a/pkg/volume/csi/csi_plugin.go b/pkg/volume/csi/csi_plugin.go index aaae2f7bdf24..a01de25f3606 100644 --- a/pkg/volume/csi/csi_plugin.go +++ b/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,15 +457,22 @@ 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) + defer func() { + // 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)) + } + } + }() + + if err != nil { errorMsg := log("csi.NewMounter failed to save volume info data: %v", err) klog.Error(errorMsg) - // attempt to cleanup volume mount dir. - if removeMountDirErr := removeMountDir(p, dir); removeMountDirErr != nil { - klog.Error(log("csi.NewMounter failed to remove mount dir [%s]: %v", dir, removeMountDirErr)) - } - return nil, errors.New(errorMsg) } @@ -706,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 { - if removeErr := os.RemoveAll(dataDir); removeErr != nil { - klog.Error(log("failed to remove dir after error [%s]: %v", dataDir, removeErr)) + err = saveVolumeData(dataDir, volDataFileName, volData) + defer func() { + // 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 From 15da65d9ae97f068756966be623e01d7377d63c1 Mon Sep 17 00:00:00 2001 From: Christian Huffman Date: Mon, 30 Nov 2020 11:51:01 -0500 Subject: [PATCH 3/3] Include unit test --- pkg/volume/csi/csi_attacher_test.go | 70 +++++++++++++++++++++++++---- 1 file changed, 61 insertions(+), 9 deletions(-) diff --git a/pkg/volume/csi/csi_attacher_test.go b/pkg/volume/csi/csi_attacher_test.go index 35c25223bf78..bb096dffbda0 100644 --- a/pkg/volume/csi/csi_attacher_test.go +++ b/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 - volName string - devicePath string - deviceMountPath string - stageUnstageSet bool - shouldFail bool - createAttachment bool - exitError error - spec *volume.Spec + testName string + volName string + devicePath string + deviceMountPath string + stageUnstageSet bool + shouldFail bool + createAttachment bool + populateDeviceMountPath bool + 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 {