From 72f17602a59f7e90cdf8c0cd76f64128bb23d454 Mon Sep 17 00:00:00 2001 From: Amarnath Valluri Date: Mon, 23 Sep 2019 15:15:22 +0300 Subject: [PATCH] DeviceManager: Return no error if the device not found while deleting Discussion on issue #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. --- go.mod | 2 +- .../controllerserver-master.go | 2 +- pkg/pmem-csi-driver/controllerserver-node.go | 4 ++++ pkg/pmem-device-manager/pmd-lvm.go | 15 +++++++++++-- pkg/pmem-device-manager/pmd-ndctl.go | 6 +++++ pkg/pmem-device-manager/pmd-util.go | 17 ++++++++------ test/e2e/storage/sanity.go | 22 +++++++++++++++++++ 7 files changed, 57 insertions(+), 11 deletions(-) diff --git a/go.mod b/go.mod index 25e659ac1f..8e5fbd3927 100644 --- a/go.mod +++ b/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 diff --git a/pkg/pmem-csi-driver/controllerserver-master.go b/pkg/pmem-csi-driver/controllerserver-master.go index f5bedc8b05..40511a204c 100644 --- a/pkg/pmem-csi-driver/controllerserver-master.go +++ b/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() diff --git a/pkg/pmem-csi-driver/controllerserver-node.go b/pkg/pmem-csi-driver/controllerserver-node.go index 03bb43a460..1c9dadc9a8 100644 --- a/pkg/pmem-csi-driver/controllerserver-node.go +++ b/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 { diff --git a/pkg/pmem-device-manager/pmd-lvm.go b/pkg/pmem-device-manager/pmd-lvm.go index a8c8b880e3..6f3cad2285 100644 --- a/pkg/pmem-device-manager/pmd-lvm.go +++ b/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 } diff --git a/pkg/pmem-device-manager/pmd-ndctl.go b/pkg/pmem-device-manager/pmd-ndctl.go index 32a22c2890..50397cd1d2 100644 --- a/pkg/pmem-device-manager/pmd-ndctl.go +++ b/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) diff --git a/pkg/pmem-device-manager/pmd-util.go b/pkg/pmem-device-manager/pmd-util.go index 7ac4fb6d70..b747d87857 100644 --- a/pkg/pmem-device-manager/pmd-util.go +++ b/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: %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("%s: %w", dev.Path, ErrDeviceNotReady) } diff --git a/test/e2e/storage/sanity.go b/test/e2e/storage/sanity.go index 640c36552d..88c1a6a249 100644 --- a/test/e2e/storage/sanity.go +++ b/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",