From 6068a19ca8346100403ff039cb7b511f7ae0517a 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. --- Gopkg.lock | 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 | 24 ++++++++++++++++++- 7 files changed, 58 insertions(+), 12 deletions(-) diff --git a/Gopkg.lock b/Gopkg.lock index a17da496a2..34c3a0adc2 100644 --- a/Gopkg.lock +++ b/Gopkg.lock @@ -1281,6 +1281,7 @@ "github.com/onsi/gomega", "github.com/pkg/errors", "golang.org/x/net/context", + "golang.org/x/sys/unix", "google.golang.org/grpc", "google.golang.org/grpc/codes", "google.golang.org/grpc/connectivity", @@ -1306,7 +1307,6 @@ "k8s.io/klog", "k8s.io/kubernetes/pkg/util/mount", "k8s.io/kubernetes/pkg/version", - "k8s.io/kubernetes/pkg/volume/util/hostutil", "k8s.io/kubernetes/test/e2e/framework", "k8s.io/kubernetes/test/e2e/framework/config", "k8s.io/kubernetes/test/e2e/framework/ginkgowrapper", 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 03e8bc3b79..f695a2aa04 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 918b415d68..728a9f2348 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..a2a539c23b 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 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) } diff --git a/test/e2e/storage/sanity.go b/test/e2e/storage/sanity.go index 6b5ecc100d..0c8065cd40 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" @@ -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" @@ -351,6 +353,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",