From 9069efe73acf3b3956af8fd4d8e6e8df2c67abc4 Mon Sep 17 00:00:00 2001 From: Jan Safranek Date: Thu, 21 Jun 2018 18:43:22 +0200 Subject: [PATCH] Fix cleanup of volume metadata json file. Create the json file with metadata as the last item, when everything else is ready, so we don't need to clean up the file in all error cases in this function. --- pkg/volume/csi/csi_attacher.go | 54 ++++++++++++++--------------- pkg/volume/csi/csi_attacher_test.go | 4 --- 2 files changed, 27 insertions(+), 31 deletions(-) diff --git a/pkg/volume/csi/csi_attacher.go b/pkg/volume/csi/csi_attacher.go index 7cc5f09d96b..7ebba4ae22e 100644 --- a/pkg/volume/csi/csi_attacher.go +++ b/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,24 +269,34 @@ func (c *csiAttacher) MountDevice(spec *volume.Spec, devicePath string, deviceMo return err } - dataDir := filepath.Dir(deviceMountPath) - if err := os.MkdirAll(dataDir, 0750); err != nil { - glog.Error(log("attacher.MountDevice failed to create dir %#v: %v", dataDir, 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 { + if err = saveVolumeData(dataDir, volDataFileName, data); err != nil { glog.Error(log("failed to save volume info data: %v", err)) - if err := os.RemoveAll(dataDir); err != nil { - glog.Error(log("failed to remove dir after error [%s]: %v", dataDir, err)) - return 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) @@ -298,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()) @@ -317,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 @@ -331,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 { @@ -364,10 +368,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 removeMountDirErr := removeMountDir(c.plugin, deviceMountPath); removeMountDirErr != nil { - glog.Error(log("attacher.MountDevice failed to remove mount dir after a NodeStageVolume() error [%s]: %v", deviceMountPath, removeMountDirErr)) - } return err } diff --git a/pkg/volume/csi/csi_attacher_test.go b/pkg/volume/csi/csi_attacher_test.go index adb2cb2d458..e31cecfa6f0 100644 --- a/pkg/volume/csi/csi_attacher_test.go +++ b/pkg/volume/csi/csi_attacher_test.go @@ -523,10 +523,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 {