From 1d94c12cd6c4ff8be3d44556f0dbcce81c499dd4 Mon Sep 17 00:00:00 2001
From: Humble Chirammal <hchiramm@redhat.com>
Date: Thu, 2 Sep 2021 09:31:55 +0530
Subject: [PATCH] cleanup: add checkErrAndUndoReserve() for error
 check,unreserve omap

all the error check scenarios of genVolFromVolID() and unreserving
omap entries based on the error made deleteVolume method complex,
this patch create a new function which handle the error check and
unrerving omap entries accordingly and finally return the response
to deletevolume/caller.

Signed-off-by: Humble Chirammal <hchiramm@redhat.com>
---
 internal/rbd/controllerserver.go | 87 ++++++++++++++++++--------------
 1 file changed, 50 insertions(+), 37 deletions(-)

diff --git a/internal/rbd/controllerserver.go b/internal/rbd/controllerserver.go
index 2f5b8e6c4..121215103 100644
--- a/internal/rbd/controllerserver.go
+++ b/internal/rbd/controllerserver.go
@@ -743,9 +743,56 @@ func checkContentSource(
 	return nil, nil, status.Errorf(codes.InvalidArgument, "not a proper volume source")
 }
 
+// checkErrAndUndoReserve work on error from genVolFromVolID() and undo omap reserve.
+// Even-though volumeID is part of rbdVolume struct we take it as an arg here, the main reason
+// being, the volume id is getting filled from `genVolFromVolID->generateVolumeFromVolumeID` call path,
+// and this function is operating on the error case/scenario of above call chain, so we can not rely
+// on the 'rbdvol->rbdimage->voldID' field.
+
+func (cs *ControllerServer) checkErrAndUndoReserve(
+	ctx context.Context,
+	err error,
+	volumeID string,
+	rbdVol *rbdVolume, cr *util.Credentials) (*csi.DeleteVolumeResponse, error) {
+	if errors.Is(err, util.ErrPoolNotFound) {
+		log.WarningLog(ctx, "failed to get backend volume for %s: %v", volumeID, err)
+
+		return &csi.DeleteVolumeResponse{}, nil
+	}
+
+	// if error is ErrKeyNotFound, then a previous attempt at deletion was complete
+	// or partially complete (image and imageOMap are garbage collected already), hence return
+	// success as deletion is complete
+	if errors.Is(err, util.ErrKeyNotFound) {
+		log.WarningLog(ctx, "failed to volume options for %s: %v", volumeID, err)
+
+		return &csi.DeleteVolumeResponse{}, nil
+	}
+
+	// All errors other than ErrImageNotFound should return an error back to the caller
+	if !errors.Is(err, ErrImageNotFound) {
+		return nil, status.Error(codes.Internal, err.Error())
+	}
+
+	// If error is ErrImageNotFound then we failed to find the image, but found the imageOMap
+	// to lead us to the image, hence the imageOMap needs to be garbage collected, by calling
+	// unreserve for the same
+	if acquired := cs.VolumeLocks.TryAcquire(rbdVol.RequestName); !acquired {
+		log.ErrorLog(ctx, util.VolumeOperationAlreadyExistsFmt, rbdVol.RequestName)
+
+		return nil, status.Errorf(codes.Aborted, util.VolumeOperationAlreadyExistsFmt, rbdVol.RequestName)
+	}
+	defer cs.VolumeLocks.Release(rbdVol.RequestName)
+
+	if err = undoVolReservation(ctx, rbdVol, cr); err != nil {
+		return nil, status.Error(codes.Internal, err.Error())
+	}
+
+	return &csi.DeleteVolumeResponse{}, nil
+}
+
 // DeleteVolume deletes the volume in backend and removes the volume metadata
-// from store
-// TODO: make this function less complex.
+// from store.
 func (cs *ControllerServer) DeleteVolume(
 	ctx context.Context,
 	req *csi.DeleteVolumeRequest) (*csi.DeleteVolumeResponse, error) {
@@ -786,41 +833,7 @@ func (cs *ControllerServer) DeleteVolume(
 	rbdVol, err := genVolFromVolID(ctx, volumeID, cr, req.GetSecrets())
 	defer rbdVol.Destroy()
 	if err != nil {
-		if errors.Is(err, util.ErrPoolNotFound) {
-			log.WarningLog(ctx, "failed to get backend volume for %s: %v", volumeID, err)
-
-			return &csi.DeleteVolumeResponse{}, nil
-		}
-
-		// if error is ErrKeyNotFound, then a previous attempt at deletion was complete
-		// or partially complete (image and imageOMap are garbage collected already), hence return
-		// success as deletion is complete
-		if errors.Is(err, util.ErrKeyNotFound) {
-			log.WarningLog(ctx, "Failed to volume options for %s: %v", volumeID, err)
-
-			return &csi.DeleteVolumeResponse{}, nil
-		}
-
-		// All errors other than ErrImageNotFound should return an error back to the caller
-		if !errors.Is(err, ErrImageNotFound) {
-			return nil, status.Error(codes.Internal, err.Error())
-		}
-
-		// If error is ErrImageNotFound then we failed to find the image, but found the imageOMap
-		// to lead us to the image, hence the imageOMap needs to be garbage collected, by calling
-		// unreserve for the same
-		if acquired := cs.VolumeLocks.TryAcquire(rbdVol.RequestName); !acquired {
-			log.ErrorLog(ctx, util.VolumeOperationAlreadyExistsFmt, rbdVol.RequestName)
-
-			return nil, status.Errorf(codes.Aborted, util.VolumeOperationAlreadyExistsFmt, rbdVol.RequestName)
-		}
-		defer cs.VolumeLocks.Release(rbdVol.RequestName)
-
-		if err = undoVolReservation(ctx, rbdVol, cr); err != nil {
-			return nil, status.Error(codes.Internal, err.Error())
-		}
-
-		return &csi.DeleteVolumeResponse{}, nil
+		return cs.checkErrAndUndoReserve(ctx, err, volumeID, rbdVol, cr)
 	}
 
 	// lock out parallel create requests against the same volume name as we