Controller: Return appropriate error code in case of failure

DeleteVolume should return FailedPrecondition if volume is in use.

Author: avalluri

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

@@ -299,7 +299,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

@@ -12,7 +12,9 @@ import (
 	"fmt"
 	"strconv"
 	"sync"
+	"syscall"
 
+	"github.com/pkg/errors"
 	"golang.org/x/net/context"
 	"google.golang.org/grpc"
 	"google.golang.org/grpc/codes"
@@ -273,6 +275,9 @@ func (cs *nodeControllerServer) DeleteVolume(ctx context.Context, req *csi.Delet
 	}
 
 	if err := cs.dm.DeleteDevice(req.VolumeId, eraseafter); err != nil {
+		if errors.Cause(err) == syscall.EBUSY {
+			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-util.go

@@ -7,8 +7,9 @@ import (
 	"time"
 
 	pmemexec "github.com/intel/pmem-csi/pkg/pmem-exec"
+	"github.com/pkg/errors"
+	"golang.org/x/sys/unix"
 	"k8s.io/klog"
-	"k8s.io/kubernetes/pkg/volume/util/hostutil"
 )
 
 const (
@@ -33,17 +34,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 errors.Wrapf(err, "%s is in use", dev.Path)
 	}
+
 	if blocks == 0 {
 		klog.V(5).Infof("Wiping entire device: %s", dev.Path)
 		// use one iteration instead of shred's default=3 for speed

pkg/pmem-device-manager/pmme-lvm_test.go

@@ -10,12 +10,14 @@ import (
 	"io/ioutil"
 	"math/rand"
 	"os"
+	"syscall"
 	"testing"
 
 	pmdmanager "github.com/intel/pmem-csi/pkg/pmem-device-manager"
 	pmemexec "github.com/intel/pmem-csi/pkg/pmem-exec"
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
+	"github.com/pkg/errors"
 	losetup "gopkg.in/freddierice/go-losetup.v1"
 )
 
@@ -58,7 +60,7 @@ var _ = Describe("DeviceManage LVM", func() {
 
 			dev, err := dm.GetDevice(name)
 			Expect(err).Should(BeNil(), "Failed to retrieve device info")
-			Expect(dev.Name).Should(Equal(name), "Name mismatch")
+			Expect(dev.VolumeId).Should(Equal(name), "Name mismatch")
 			Expect(dev.Size >= size).Should(BeTrue(), "Size mismatch")
 			Expect(dev.Path).ShouldNot(BeNil(), "Null device path")
 		})
@@ -89,8 +91,8 @@ var _ = Describe("DeviceManage LVM", func() {
 			Expect(err).Should(BeNil(), "Failed to list devices")
 			Expect(len(list)).Should(BeEquivalentTo(max_devices), "count mismatch")
 			for _, dev := range list {
-				size, ok := sizes[dev.Name]
-				Expect(ok).Should(BeTrue(), "Unexpected device name:"+dev.Name)
+				size, ok := sizes[dev.VolumeId]
+				Expect(ok).Should(BeTrue(), "Unexpected device name:"+dev.VolumeId)
 				Expect(dev.Size >= size).Should(BeTrue(), "Device size mismatch")
 			}
 
@@ -106,8 +108,8 @@ var _ = Describe("DeviceManage LVM", func() {
 			Expect(err).Should(BeNil(), "Failed to list devices")
 			Expect(len(list)).Should(BeEquivalentTo(max_devices-max_deletes), "count mismatch")
 			for _, dev := range list {
-				size, ok := sizes[dev.Name]
-				Expect(ok).Should(BeTrue(), "Unexpected device name:"+dev.Name)
+				size, ok := sizes[dev.VolumeId]
+				Expect(ok).Should(BeTrue(), "Unexpected device name:"+dev.VolumeId)
 				Expect(dev.Size >= size).Should(BeTrue(), "Device size mismatch")
 			}
 		})
@@ -120,10 +122,27 @@ var _ = Describe("DeviceManage LVM", func() {
 
 			dev, err := dm.GetDevice(name)
 			Expect(err).Should(BeNil(), "Failed to retrieve device info")
-			Expect(dev.Name).Should(Equal(name), "Name mismatch")
+			Expect(dev.VolumeId).Should(Equal(name), "Name mismatch")
 			Expect(dev.Size >= size).Should(BeTrue(), "Size mismatch")
 			Expect(dev.Path).ShouldNot(BeNil(), "Null device path")
 
+			mountPath, err := mountDevice(&dev)
+			Expect(err).Should(BeNil(), "Failed to create mount path: %s", mountPath)
+
+			defer unmount(mountPath)
+
+			out, _ := pmemexec.RunCommand("df")
+			fmt.Println(out)
+
+			// Delete should fail as the device is in use
+			err = dm.DeleteDevice(name, true)
+			Expect(err).ShouldNot(BeNil(), "Error expected when deleting device in use: %s", dev.VolumeId)
+			Expect(errors.Cause(err)).Should(BeEquivalentTo(syscall.EBUSY), "Expected device busy error: %s", dev.VolumeId)
+
+			err = unmount(mountPath)
+			Expect(err).Should(BeNil(), "Failed to unmount the device: %s", dev.VolumeId)
+
+			// Delete should succeed
 			err = dm.DeleteDevice(name, true)
 			Expect(err).Should(BeNil(), "Failed to delete device")
 
@@ -132,9 +151,12 @@ var _ = Describe("DeviceManage LVM", func() {
 			//Expect(os.IsNotExist(errors.Cause())).Should(BeTrue(), "expected error is os.ErrNotExist")
 			//Expect(dev).Should(BeNil(), "returned device should be nil")
 
+			// Delete call should not return any error on non-existing device
 			err = dm.DeleteDevice(name, true)
 			Expect(err).Should(BeNil(), "DeleteDevice() is not idempotent")
+
 		})
+
 	})
 })
 
@@ -227,3 +249,36 @@ func (vg *testVGS) Clean() error {
 
 	return nil
 }
+
+func mountDevice(device *pmdmanager.PmemDeviceInfo) (string, error) {
+	targetPath, err := ioutil.TempDir("/tmp", "lmv-mnt-path-")
+	if err != nil {
+		return "", err
+	}
+
+	cmd := "mkfs.ext4"
+	args := []string{"-b 4096", "-F", device.Path}
+
+	if _, err := pmemexec.RunCommand(cmd, args...); err != nil {
+		os.Remove(targetPath)
+		return "", err
+	}
+
+	cmd = "mount"
+	args = []string{"-c", device.Path, targetPath}
+
+	if _, err := pmemexec.RunCommand(cmd, args...); err != nil {
+		os.Remove(targetPath)
+		return "", err
+	}
+
+	return targetPath, nil
+}
+
+func unmount(path string) error {
+	args := []string{path}
+	if _, err := pmemexec.RunCommand("umount", args...); err != nil {
+		return err
+	}
+	return os.Remove(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"
@@ -43,9 +45,9 @@ import (
 	clientset "k8s.io/client-go/kubernetes"
 	clientexec "k8s.io/client-go/util/exec"
 	"k8s.io/kubernetes/test/e2e/framework"
-	testutils "k8s.io/kubernetes/test/utils"
 	e2epod "k8s.io/kubernetes/test/e2e/framework/pod"
 	e2essh "k8s.io/kubernetes/test/e2e/framework/ssh"
+	testutils "k8s.io/kubernetes/test/utils"
 
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
@@ -67,7 +69,7 @@ var _ = Describe("sanity", func() {
 		// and deletes all extra entries that it does not know about.
 		TargetPath:  "/var/lib/kubelet/plugins/kubernetes.io/csi/pv/pmem-sanity-target.XXXXXX",
 		StagingPath: "/var/lib/kubelet/plugins/kubernetes.io/csi/pv/pmem-sanity-staging.XXXXXX",
-		IDGen: &sanity.DefaultIDGenerator{},
+		IDGen:       &sanity.DefaultIDGenerator{},
 	}
 
 	f := framework.NewDefaultFramework("pmem")
@@ -352,6 +354,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",