From c7ea8c927d639ac873518675bf65f57d09a45183 Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Mon, 2 Nov 2020 08:38:32 +0100 Subject: [PATCH] pmem-csi-driver: fix rescheduling after CreateVolume failure If CreateVolume fails for a volume with late binding, the Kubernetes scheduler is supposed to be told that it needs to reschedule the pod. This didn't work with PMEM-CSI because it returned an "internal" error code instead of the "resource exhausted" one that the external-provisioner knows about. Instead of having different well-defined errors at different levels of the PMEM-CSI software stack, a common error package is used. These errors get wrapped when it is useful to add more information. We still add the gRPC error code, though. That could be avoided by making the internal errors based on gRPC but it is uncertain whether we want to go that far. (cherry picked from commit 34323ff8df9bbce5328551e2cc0694395475ae12) --- pkg/errors/errors.go | 32 ++++++++++++++++++ pkg/ndctl/ndctl.go | 9 ++--- pkg/ndctl/region.go | 4 ++- .../controllerserver-master.go | 2 +- pkg/pmem-csi-driver/controllerserver-node.go | 11 +++++-- pkg/pmem-csi-driver/nodeserver.go | 9 ++--- pkg/pmem-device-manager/pmd-lvm.go | 13 ++++---- pkg/pmem-device-manager/pmd-manager.go | 30 ++--------------- pkg/pmem-device-manager/pmd-manager_test.go | 7 ++-- pkg/pmem-device-manager/pmd-ndctl.go | 12 +++---- pkg/pmem-device-manager/pmd-util.go | 5 +-- test/e2e/storage/sanity.go | 33 +++++++++++++++---- 12 files changed, 99 insertions(+), 68 deletions(-) create mode 100644 pkg/errors/errors.go diff --git a/pkg/errors/errors.go b/pkg/errors/errors.go new file mode 100644 index 0000000000..45b406f4aa --- /dev/null +++ b/pkg/errors/errors.go @@ -0,0 +1,32 @@ +/* +Copyright 2020 Intel Coporation. + +SPDX-License-Identifier: Apache-2.0 +*/ + +// Package errors contain some well-defined errors that may have to be +// passed up from low-level layers in the PMEM-CSI stack up to the +// gRPC interface. +// +// These errors must be wrapped (for example, with %w) so that the +// upper layers can use errors.Is to recognize these special errors if +// needed. +package errors + +import ( + "errors" +) + +var ( + // DeviceExists device with given id already exists + DeviceExists = errors.New("device exists") + + // ErrDeviceNotFound device does not exists + DeviceNotFound = errors.New("device not found") + + // ErrDeviceInUse device is in use + DeviceInUse = errors.New("device in use") + + // ErrNotEnoughSpace no space to create the device + NotEnoughSpace = errors.New("not enough space") +) diff --git a/pkg/ndctl/ndctl.go b/pkg/ndctl/ndctl.go index 4c3cecf273..a074a51113 100644 --- a/pkg/ndctl/ndctl.go +++ b/pkg/ndctl/ndctl.go @@ -8,10 +8,11 @@ package ndctl import "C" import ( - "errors" "fmt" "k8s.io/klog/v2" + + pmemerr "github.com/intel/pmem-csi/pkg/errors" ) const ( @@ -23,10 +24,6 @@ const ( tib uint64 = gib * 1024 ) -var ( - ErrNotExist = errors.New("namespace not found") -) - //CreateNamespaceOpts options to create a namespace type CreateNamespaceOpts struct { Name string @@ -109,7 +106,7 @@ func (ctx *Context) GetNamespaceByName(name string) (*Namespace, error) { } } } - return nil, ErrNotExist + return nil, pmemerr.DeviceNotFound } //GetActiveNamespaces returns list of all active namespaces in all regions diff --git a/pkg/ndctl/region.go b/pkg/ndctl/region.go index b42df5305f..e47d6686bf 100644 --- a/pkg/ndctl/region.go +++ b/pkg/ndctl/region.go @@ -12,6 +12,8 @@ import ( "github.com/google/uuid" "k8s.io/klog/v2" + + pmemerr "github.com/intel/pmem-csi/pkg/errors" ) type RegionType string @@ -170,7 +172,7 @@ func (r *Region) CreateNamespace(opts CreateNamespaceOpts) (*Namespace, error) { available = r.AvailableSize() } if opts.Size > available { - return nil, fmt.Errorf("Not enough space to create namespace with size %v", opts.Size) + return nil, fmt.Errorf("create namespace with size %v: %w", opts.Size, pmemerr.NotEnoughSpace) } } diff --git a/pkg/pmem-csi-driver/controllerserver-master.go b/pkg/pmem-csi-driver/controllerserver-master.go index e37dba084d..121b72921f 100644 --- a/pkg/pmem-csi-driver/controllerserver-master.go +++ b/pkg/pmem-csi-driver/controllerserver-master.go @@ -250,7 +250,7 @@ func (cs *masterController) CreateVolume(ctx context.Context, req *csi.CreateVol } if len(chosenNodes) == 0 { - return nil, status.Error(codes.Unavailable, fmt.Sprintf("No node found with %v capacity", asked)) + return nil, status.Error(codes.ResourceExhausted, fmt.Sprintf("No node found with %v capacity", asked)) } klog.V(3).Infof("Chosen nodes: %v", chosenNodes) diff --git a/pkg/pmem-csi-driver/controllerserver-node.go b/pkg/pmem-csi-driver/controllerserver-node.go index cf9d83ffcb..5115083557 100644 --- a/pkg/pmem-csi-driver/controllerserver-node.go +++ b/pkg/pmem-csi-driver/controllerserver-node.go @@ -19,6 +19,7 @@ import ( "github.com/container-storage-interface/spec/lib/go/csi" + pmemerr "github.com/intel/pmem-csi/pkg/errors" grpcserver "github.com/intel/pmem-csi/pkg/grpc-server" "github.com/intel/pmem-csi/pkg/pmem-csi-driver/parameters" pmdmanager "github.com/intel/pmem-csi/pkg/pmem-device-manager" @@ -97,7 +98,7 @@ func NewNodeControllerServer(nodeID string, dm pmdmanager.PmemDeviceManager, sm if _, err := dm.GetDevice(id); err == nil { found = true - } else if !errors.Is(err, pmdmanager.ErrDeviceNotFound) { + } else if !errors.Is(err, pmemerr.DeviceNotFound) { klog.Warningf("Failed to fetch device for state volume '%s'(volume mode: '%s'): %v", id, v.GetDeviceMode(), err) // Let's ignore this volume continue @@ -263,7 +264,11 @@ func (cs *nodeControllerServer) createVolumeInternal(ctx context.Context, asked = 1 } if err := cs.dm.CreateDevice(volumeID, uint64(asked)); err != nil { - statusErr = status.Errorf(codes.Internal, "Node CreateVolume: device creation failed: %v", err) + code := codes.Internal + if errors.Is(err, pmemerr.NotEnoughSpace) { + code = codes.ResourceExhausted + } + statusErr = status.Errorf(code, "Node CreateVolume: device creation failed: %v", err) return } // TODO(?): determine and return actual size here? @@ -316,7 +321,7 @@ func (cs *nodeControllerServer) DeleteVolume(ctx context.Context, req *csi.Delet } if err := dm.DeleteDevice(req.VolumeId, p.GetEraseAfter()); err != nil { - if errors.Is(err, pmdmanager.ErrDeviceInUse) { + if errors.Is(err, pmemerr.DeviceInUse) { return nil, status.Errorf(codes.FailedPrecondition, err.Error()) } return nil, status.Errorf(codes.Internal, "Failed to delete volume: %s", err.Error()) diff --git a/pkg/pmem-csi-driver/nodeserver.go b/pkg/pmem-csi-driver/nodeserver.go index c075722ffc..c9800859b1 100644 --- a/pkg/pmem-csi-driver/nodeserver.go +++ b/pkg/pmem-csi-driver/nodeserver.go @@ -22,6 +22,7 @@ import ( "k8s.io/klog/v2" "k8s.io/utils/mount" + pmemerr "github.com/intel/pmem-csi/pkg/errors" pmemexec "github.com/intel/pmem-csi/pkg/exec" grpcserver "github.com/intel/pmem-csi/pkg/grpc-server" "github.com/intel/pmem-csi/pkg/imagefile" @@ -150,7 +151,7 @@ func (ns *nodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePublis // 1) No Volume found with given volume id // 2) No provisioner info found in VolumeContext "storage.kubernetes.io/csiProvisionerIdentity" // 3) No StagingPath in the request - if device, err = ns.cs.dm.GetDevice(req.VolumeId); err != nil && !errors.Is(err, pmdmanager.ErrDeviceNotFound) { + if device, err = ns.cs.dm.GetDevice(req.VolumeId); err != nil && !errors.Is(err, pmemerr.DeviceNotFound) { return nil, status.Errorf(codes.Internal, "failed to get device details for volume id '%s': %v", req.VolumeId, err) } _, ok := req.GetVolumeContext()[volumeProvisionerIdentity] @@ -181,7 +182,7 @@ func (ns *nodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePublis volumeParameters = v if device, err = ns.cs.dm.GetDevice(req.VolumeId); err != nil { - if errors.Is(err, pmdmanager.ErrDeviceNotFound) { + if errors.Is(err, pmemerr.DeviceNotFound) { return nil, status.Errorf(codes.NotFound, "no device found with volume id %q: %v", req.VolumeId, err) } return nil, status.Errorf(codes.Internal, "failed to get device details for volume id %q: %v", req.VolumeId, err) @@ -487,7 +488,7 @@ func (ns *nodeServer) NodeStageVolume(ctx context.Context, req *csi.NodeStageVol device, err := ns.cs.dm.GetDevice(req.VolumeId) if err != nil { - if errors.Is(err, pmdmanager.ErrDeviceNotFound) { + if errors.Is(err, pmemerr.DeviceNotFound) { return nil, status.Errorf(codes.NotFound, "no device found with volume id %q: %v", req.VolumeId, err) } return nil, status.Errorf(codes.Internal, "failed to get device details for volume id %q: %v", req.VolumeId, err) @@ -544,7 +545,7 @@ func (ns *nodeServer) NodeUnstageVolume(ctx context.Context, req *csi.NodeUnstag // so we look up the current device by volumeID and see is that device // mounted on staging target path if _, err := ns.cs.dm.GetDevice(req.VolumeId); err != nil { - if errors.Is(err, pmdmanager.ErrDeviceNotFound) { + if errors.Is(err, pmemerr.DeviceNotFound) { return nil, status.Errorf(codes.NotFound, "no device found with volume id '%s': %s", req.VolumeId, err.Error()) } return nil, status.Errorf(codes.Internal, "failed to get device details for volume id '%s': %s", req.VolumeId, err.Error()) diff --git a/pkg/pmem-device-manager/pmd-lvm.go b/pkg/pmem-device-manager/pmd-lvm.go index 5df752d0d4..ccf190eb86 100644 --- a/pkg/pmem-device-manager/pmd-lvm.go +++ b/pkg/pmem-device-manager/pmd-lvm.go @@ -8,6 +8,7 @@ import ( "sync" api "github.com/intel/pmem-csi/pkg/apis/pmemcsi/v1alpha1" + pmemerr "github.com/intel/pmem-csi/pkg/errors" pmemexec "github.com/intel/pmem-csi/pkg/exec" "github.com/intel/pmem-csi/pkg/ndctl" pmemcommon "github.com/intel/pmem-csi/pkg/pmem-common" @@ -132,7 +133,7 @@ func (lvm *pmemLvm) CreateDevice(volumeId string, size uint64) error { // Avoid device filling with garbage entries by returning error. // Overall, no point having more than one namespace with same volumeId. if _, err := lvm.getDevice(volumeId); err == nil { - return ErrDeviceExists + return pmemerr.DeviceExists } vgs, err := getVolumeGroups(lvm.volumeGroups) if err != nil { @@ -177,7 +178,7 @@ func (lvm *pmemLvm) CreateDevice(volumeId string, size uint64) error { } } } - return ErrNotEnoughSpace + return pmemerr.NotEnoughSpace } func (lvm *pmemLvm) DeleteDevice(volumeId string, flush bool) error { @@ -188,13 +189,13 @@ func (lvm *pmemLvm) DeleteDevice(volumeId string, flush bool) error { var device *PmemDeviceInfo if device, err = lvm.getDevice(volumeId); err != nil { - if errors.Is(err, ErrDeviceNotFound) { + if errors.Is(err, pmemerr.DeviceNotFound) { return nil } return err } if err := clearDevice(device, flush); err != nil { - if errors.Is(err, ErrDeviceNotFound) { + if errors.Is(err, pmemerr.DeviceNotFound) { // Remove device from cache delete(lvm.devices, volumeId) return nil @@ -236,7 +237,7 @@ func (lvm *pmemLvm) getDevice(volumeId string) (*PmemDeviceInfo, error) { return dev, nil } - return nil, ErrDeviceNotFound + return nil, pmemerr.DeviceNotFound } func getUncachedDevice(volumeId string, volumeGroup string) (*PmemDeviceInfo, error) { @@ -249,7 +250,7 @@ func getUncachedDevice(volumeId string, volumeGroup string) (*PmemDeviceInfo, er return dev, nil } - return nil, ErrDeviceNotFound + return nil, pmemerr.DeviceNotFound } // listDevices Lists available logical devices in given volume groups diff --git a/pkg/pmem-device-manager/pmd-manager.go b/pkg/pmem-device-manager/pmd-manager.go index 1e5ef5cda4..f5882fc1c7 100644 --- a/pkg/pmem-device-manager/pmd-manager.go +++ b/pkg/pmem-device-manager/pmd-manager.go @@ -1,35 +1,9 @@ package pmdmanager import ( - "errors" - "os" - api "github.com/intel/pmem-csi/pkg/apis/pmemcsi/v1alpha1" ) -var ( - // ErrInvalid invalid argument passed - ErrInvalid = os.ErrInvalid - - // ErrPermission no permission to complete the task - ErrPermission = os.ErrPermission - - // ErrDeviceExists device with given id already exists - ErrDeviceExists = errors.New("device exists") - - // ErrDeviceNotFound device does not exists - ErrDeviceNotFound = errors.New("device not found") - - // ErrDeviceInUse device is in use - ErrDeviceInUse = errors.New("device in use") - - // ErrDeviceNotReady device not ready yet - ErrDeviceNotReady = errors.New("device not ready") - - // ErrNotEnoughSpace no space to create the device - ErrNotEnoughSpace = errors.New("not enough space") -) - //PmemDeviceInfo represents a block device type PmemDeviceInfo struct { //VolumeId is name of the block device @@ -64,7 +38,7 @@ type PmemDeviceManager interface { GetCapacity() (Capacity, error) // CreateDevice creates a new block device with give name, size and namespace mode - // Possible errors: ErrNotEnoughSpace, ErrInvalid, ErrDeviceExists + // Possible errors: ErrNotEnoughSpace, ErrDeviceExists CreateDevice(name string, size uint64) error // GetDevice returns the block device information for given name @@ -73,7 +47,7 @@ type PmemDeviceManager interface { // DeleteDevice deletes an existing block device with give name. // If 'flush' is 'true', then the device data is zeroed before deleting the device - // Possible errors: ErrDeviceInUse, ErrPermission + // Possible errors: ErrDeviceInUse DeleteDevice(name string, flush bool) error // ListDevices returns all the block devices information that was created by this device manager diff --git a/pkg/pmem-device-manager/pmd-manager_test.go b/pkg/pmem-device-manager/pmd-manager_test.go index fb195c96a4..b6376772a6 100644 --- a/pkg/pmem-device-manager/pmd-manager_test.go +++ b/pkg/pmem-device-manager/pmd-manager_test.go @@ -14,6 +14,7 @@ import ( "strings" "testing" + pmemerr "github.com/intel/pmem-csi/pkg/errors" pmemexec "github.com/intel/pmem-csi/pkg/exec" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" @@ -125,7 +126,7 @@ func runTests(mode string) { It("Should fail to retrieve non-existent device", func() { dev, err := dm.GetDevice("unknown") Expect(err).ShouldNot(BeNil(), "Error expected") - Expect(errors.Is(err, ErrDeviceNotFound)).Should(BeTrue(), "expected error is device not found error") + Expect(errors.Is(err, pmemerr.DeviceNotFound)).Should(BeTrue(), "expected error is device not found error") Expect(dev).Should(BeNil(), "returned device should be nil") }) @@ -206,7 +207,7 @@ func runTests(mode string) { // 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.Is(err, ErrDeviceInUse)).Should(BeTrue(), "Expected device busy error: %s", dev.VolumeId) + Expect(errors.Is(err, pmemerr.DeviceInUse)).Should(BeTrue(), "Expected device busy error: %s", dev.VolumeId) cleanupList[name] = false err = unmount(mountPath) @@ -218,7 +219,7 @@ func runTests(mode string) { dev, err = dm.GetDevice(name) Expect(err).ShouldNot(BeNil(), "GetDevice() should fail on deleted device") - Expect(errors.Is(err, ErrDeviceNotFound)).Should(BeTrue(), "expected error is os.ErrNotExist") + Expect(errors.Is(err, pmemerr.DeviceNotFound)).Should(BeTrue(), "expected error is DeviceNodeFound") Expect(dev).Should(BeNil(), "returned device should be nil") // Delete call should not return any error on non-existing device diff --git a/pkg/pmem-device-manager/pmd-ndctl.go b/pkg/pmem-device-manager/pmd-ndctl.go index 0f4da2527a..4e107dbdfd 100644 --- a/pkg/pmem-device-manager/pmd-ndctl.go +++ b/pkg/pmem-device-manager/pmd-ndctl.go @@ -6,6 +6,7 @@ import ( "sync" api "github.com/intel/pmem-csi/pkg/apis/pmemcsi/v1alpha1" + pmemerr "github.com/intel/pmem-csi/pkg/errors" "github.com/intel/pmem-csi/pkg/ndctl" "k8s.io/klog/v2" "k8s.io/utils/mount" @@ -112,7 +113,7 @@ func (pmem *pmemNdctl) CreateDevice(volumeId string, size uint64) error { // Avoid device filling with garbage entries by returning error. // Overall, no point having more than one namespace with same name. if _, err := getDevice(ndctx, volumeId); err == nil { - return ErrDeviceExists + return pmemerr.DeviceExists } // libndctl needs to store meta data and will use some of the allocated @@ -158,13 +159,13 @@ func (pmem *pmemNdctl) DeleteDevice(volumeId string, flush bool) error { device, err := getDevice(ndctx, volumeId) if err != nil { - if errors.Is(err, ErrDeviceNotFound) { + if errors.Is(err, pmemerr.DeviceNotFound) { return nil } return err } if err := clearDevice(device, flush); err != nil { - if errors.Is(err, ErrDeviceNotFound) { + if errors.Is(err, pmemerr.DeviceNotFound) { return nil } return err @@ -205,10 +206,7 @@ func (pmem *pmemNdctl) ListDevices() ([]*PmemDeviceInfo, error) { func getDevice(ndctx *ndctl.Context, volumeId string) (*PmemDeviceInfo, error) { ns, err := ndctx.GetNamespaceByName(volumeId) if err != nil { - if errors.Is(err, ndctl.ErrNotExist) { - return nil, ErrDeviceNotFound - } - return nil, fmt.Errorf("error getting device %q: %v", volumeId, err) + return nil, fmt.Errorf("error getting device %q: %w", volumeId, err) } return namespaceToPmemInfo(ns), nil diff --git a/pkg/pmem-device-manager/pmd-util.go b/pkg/pmem-device-manager/pmd-util.go index 16adbbf80a..620be6ccdb 100644 --- a/pkg/pmem-device-manager/pmd-util.go +++ b/pkg/pmem-device-manager/pmd-util.go @@ -6,6 +6,7 @@ import ( "strconv" "time" + pmemerr "github.com/intel/pmem-csi/pkg/errors" pmemexec "github.com/intel/pmem-csi/pkg/exec" "golang.org/x/sys/unix" "k8s.io/klog/v2" @@ -44,7 +45,7 @@ func clearDevice(dev *PmemDeviceInfo, flush bool) error { defer unix.Close(fd) if err != nil { - return fmt.Errorf("failed to clear device %q: %w", dev.Path, ErrDeviceInUse) + return fmt.Errorf("failed to clear device %q: %w", dev.Path, pmemerr.DeviceInUse) } if blocks == 0 { @@ -78,5 +79,5 @@ func waitDeviceAppears(dev *PmemDeviceInfo) error { i, dev.Path, retryStatTimeout) time.Sleep(retryStatTimeout) } - return fmt.Errorf("%s: %w", dev.Path, ErrDeviceNotReady) + return fmt.Errorf("%s: device not ready", dev.Path) } diff --git a/test/e2e/storage/sanity.go b/test/e2e/storage/sanity.go index b9a2c1cb95..265131d2e7 100644 --- a/test/e2e/storage/sanity.go +++ b/test/e2e/storage/sanity.go @@ -484,6 +484,12 @@ var _ = deploy.DescribeForSome("sanity", func(d *deploy.Deployment) bool { v.remove(vol, name) }) + It("CreateVolume should return ResourceExhausted", func() { + v.namePrefix = "resource-exhausted" + + v.create(1024*1024*1024*1024*1024, nodeID, codes.ResourceExhausted) + }) + It("stress test", func() { // The load here consists of n workers which // create and test volumes in parallel until @@ -858,7 +864,7 @@ func (v volume) getTargetPath() string { return v.sc.TargetPath } -func (v volume) create(sizeInBytes int64, nodeID string) (string, *csi.Volume) { +func (v volume) create(sizeInBytes int64, nodeID string, expectedStatus ...codes.Code) (string, *csi.Volume) { var err error name := sanity.UniqueString(v.namePrefix) @@ -901,13 +907,26 @@ func (v volume) create(sizeInBytes int64, nodeID string) (string, *csi.Volume) { } } var vol *csi.CreateVolumeResponse - err = v.retry(func() error { - vol, err = v.cc.CreateVolume( - v.ctx, req, - ) - return err - }, "CreateVolume") + if len(expectedStatus) > 0 { + // Expected to fail, no retries. + vol, err = v.cc.CreateVolume(v.ctx, req) + } else { + // With retries. + err = v.retry(func() error { + vol, err = v.cc.CreateVolume( + v.ctx, req, + ) + return err + }, "CreateVolume") + } v.cl.MaybeRegisterVolume(name, vol, err) + if len(expectedStatus) > 0 { + framework.ExpectError(err, create) + status, ok := status.FromError(err) + Expect(ok).To(BeTrue(), "have gRPC status error") + Expect(status.Code()).To(Equal(expectedStatus[0]), "expected gRPC status code") + return name, nil + } framework.ExpectNoError(err, create) Expect(vol).NotTo(BeNil()) Expect(vol.GetVolume()).NotTo(BeNil())