DeviceManager: Return no error if the device not found while deleting

Discussion on issue intel#413, raised a requirement that PMEM-CSI should handle the
DeleteVolume() call for volumes that the underlined device already deleted
manually by the admin.  So for such a volume we just ignore and return no error
in DeviceManager.DeleteVolume(). This also makes DeleteVolume() idempotent.

Author: avalluri

go.mod

@@ -33,7 +33,7 @@ require (
 	go.uber.org/zap v1.11.0 // indirect
 	golang.org/x/crypto v0.0.0-20191011191535-87dc89f01550 // indirect
 	golang.org/x/net v0.0.0-20191027233614-53de4c7853b5
-	golang.org/x/sys v0.0.0-20191105231009-c1f44814a5cd // indirect
+	golang.org/x/sys v0.0.0-20191105231009-c1f44814a5cd
 	golang.org/x/time v0.0.0-20191024005414-555d28b269f0 // indirect
 	google.golang.org/appengine v1.6.5 // indirect
 	google.golang.org/genproto v0.0.0-20191009194640-548a555dbc03 // indirect

pkg/pmem-csi-driver/controllerserver-master.go

@@ -305,7 +305,7 @@ func (cs *masterController) DeleteVolume(ctx context.Context, req *csi.DeleteVol
 			defer conn.Close() // nolint:errcheck
 			klog.V(4).Infof("Asking node %s to delete volume name:%s id:%s", node, vol.name, vol.id)
 			if _, err := csi.NewControllerClient(conn).DeleteVolume(ctx, req); err != nil {
-				return nil, status.Error(codes.Internal, "Failed to delete volume name:"+vol.name+" id:"+vol.id+" on "+node+": "+err.Error())
+				return nil, err
 			}
 		}
 		cs.mutex.Lock()

pkg/pmem-csi-driver/controllerserver-node.go

@@ -9,6 +9,7 @@ package pmemcsidriver
 import (
 	"crypto/sha1"
 	"encoding/hex"
+	"errors"
 	"fmt"
 	"strconv"
 	"sync"
@@ -273,6 +274,9 @@ func (cs *nodeControllerServer) DeleteVolume(ctx context.Context, req *csi.Delet
 	}
 
 	if err := cs.dm.DeleteDevice(req.VolumeId, eraseafter); err != nil {
+		if errors.Is(err, pmdmanager.ErrDeviceInUse) {
+			return nil, status.Errorf(codes.FailedPrecondition, err.Error())
+		}
 		return nil, status.Errorf(codes.Internal, "Failed to delete volume: %s", err.Error())
 	}
 	if cs.sm != nil {

pkg/pmem-device-manager/pmd-lvm.go

@@ -1,6 +1,7 @@
 package pmdmanager
 
 import (
+	"errors"
 	"fmt"
 	"strconv"
 	"strings"
@@ -148,11 +149,21 @@ func (lvm *pmemLvm) DeleteDevice(volumeId string, flush bool) error {
 	lvmMutex.Lock()
 	defer lvmMutex.Unlock()
 
-	device, err := lvm.getDevice(volumeId)
-	if err != nil {
+	var err error
+	var device *PmemDeviceInfo
+
+	if device, err = lvm.getDevice(volumeId); err != nil {
+		if errors.Is(err, ErrDeviceNotFound) {
+			return nil
+		}
 		return err
 	}
 	if err := clearDevice(device, flush); err != nil {
+		if errors.Is(err, ErrDeviceNotFound) {
+			// Remove device from cache
+			delete(lvm.devices, volumeId)
+			return nil
+		}
 		return err
 	}
 

pkg/pmem-device-manager/pmd-ndctl.go

@@ -137,9 +137,15 @@ func (pmem *pmemNdctl) DeleteDevice(volumeId string, flush bool) error {
 
 	device, err := pmem.getDevice(volumeId)
 	if err != nil {
+		if errors.Is(err, ErrDeviceNotFound) {
+			return nil
+		}
 		return err
 	}
 	if err := clearDevice(device, flush); err != nil {
+		if errors.Is(err, ErrDeviceNotFound) {
+			return nil
+		}
 		return err
 	}
 	return pmem.ctx.DestroyNamespaceByName(volumeId)

pkg/pmem-device-manager/pmd-util.go

@@ -7,8 +7,8 @@ import (
 	"time"
 
 	pmemexec "github.com/intel/pmem-csi/pkg/pmem-exec"
+	"golang.org/x/sys/unix"
 	"k8s.io/klog"
-	"k8s.io/kubernetes/pkg/volume/util/hostutil"
 )
 
 const (
@@ -33,17 +33,20 @@ func clearDevice(dev *PmemDeviceInfo, flush bool) error {
 		klog.Errorf("clearDevice: %s does not exist", dev.Path)
 		return err
 	}
+
+	// Check if device
 	if (fileinfo.Mode() & os.ModeDevice) == 0 {
 		klog.Errorf("clearDevice: %s is not device", dev.Path)
 		return fmt.Errorf("%s is not device", dev.Path)
 	}
-	devOpen, err := hostutil.NewHostUtil().DeviceOpened(dev.Path)
+
+	fd, err := unix.Open(dev.Path, unix.O_RDONLY|unix.O_EXCL|unix.O_CLOEXEC, 0)
+	defer unix.Close(fd)
+
 	if err != nil {
-		return err
-	}
-	if devOpen {
-		return fmt.Errorf("%s is in use", dev.Path)
+		return fmt.Errorf("failed to clear device %q as %w", dev.Path, ErrDeviceInUse)
 	}
+
 	if blocks == 0 {
 		klog.V(5).Infof("Wiping entire device: %s", dev.Path)
 		// use one iteration instead of shred's default=3 for speed
@@ -75,5 +78,5 @@ func waitDeviceAppears(dev *PmemDeviceInfo) error {
 			i, dev.Path, retryStatTimeout)
 		time.Sleep(retryStatTimeout)
 	}
-	return fmt.Errorf("device %s did not appear after multiple retries", dev.Path)
+	return fmt.Errorf("%w(%s)", ErrDeviceNotReady, dev.Path)
 }

test/e2e/storage/sanity.go

@@ -34,6 +34,8 @@ import (
 	"github.com/kubernetes-csi/csi-test/pkg/sanity"
 	sanityutils "github.com/kubernetes-csi/csi-test/utils"
 	"google.golang.org/grpc"
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
 	appsv1 "k8s.io/api/apps/v1"
 	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/resource"
@@ -353,6 +355,26 @@ var _ = Describe("sanity", func() {
 			Expect(resp.AvailableCapacity).To(Equal(nodeCapacity), "capacity mismatch")
 		})
 
+		It("delete volume should fail with appropriate error", func() {
+			v.namePrefix = "delete-volume"
+
+			name, vol := v.create(2*1024*1024, nodeID)
+			// Publish for the second time.
+			nodeID := v.publish(name, vol)
+
+			_, err := v.cc.DeleteVolume(v.ctx, &csi.DeleteVolumeRequest{
+				VolumeId: vol.GetVolumeId(),
+			})
+			Expect(err).ShouldNot(BeNil(), fmt.Sprintf("Volume(%s) in use cannot be deleted", name))
+			s, ok := status.FromError(err)
+			Expect(ok).Should(BeTrue(), "Expected a status error")
+			Expect(s.Code()).Should(BeEquivalentTo(codes.FailedPrecondition), "Expected device busy error")
+
+			v.unpublish(vol, nodeID)
+
+			v.remove(vol, name)
+		})
+
 		var (
 			numWorkers = flag.Int("pmem.sanity.workers", 10, "number of worker creating volumes in parallel and thus also the maximum number of volumes at any time")
 			numVolumes = flag.Int("pmem.sanity.volumes",