util: simplify error handling

The sentinel error code had additional fields in the errors, that are
used nowhere.  This leads to unneccesarily complicated code.  This
change replaces the sentinel errors in utils with standard errors
created with errors.New() and adds a simple JoinErrors() function to
be able to combine sentinel errors from different code tiers.

Related: #1203

Signed-off-by: Sven Anderson <sven@redhat.com>

internal/cephfs/cephfs_util.go

@@ -84,7 +84,7 @@ func getMetadataPool(ctx context.Context, monitors string, cr *util.Credentials,
 		}
 	}
 
-	return "", util.ErrPoolNotFound{Pool: fsName, Err: fmt.Errorf("fsName (%s) not found in Ceph cluster", fsName)}
+	return "", fmt.Errorf("%w: fsName (%s) not found in Ceph cluster", util.ErrPoolNotFound, fsName)
 }
 
 // CephFilesystemDump is a representation of the main json structure returned by 'ceph fs dump'.
@@ -114,5 +114,5 @@ func getFsName(ctx context.Context, monitors string, cr *util.Credentials, fscID
 		}
 	}
 
-	return "", util.ErrPoolNotFound{Pool: string(fscID), Err: fmt.Errorf("fscID (%d) not found in Ceph cluster", fscID)}
+	return "", fmt.Errorf("%w: fscID (%d) not found in Ceph cluster", util.ErrPoolNotFound, fscID)
 }

internal/cephfs/controllerserver.go

@@ -172,16 +172,14 @@ func (cs *ControllerServer) DeleteVolume(ctx context.Context, req *csi.DeleteVol
 	if err != nil {
 		// if error is ErrPoolNotFound, the pool is already deleted we dont
 		// need to worry about deleting subvolume or omap data, return success
-		var epnf util.ErrPoolNotFound
+		if errors.Is(err, util.ErrPoolNotFound) {
-		if errors.As(err, &epnf) {
 			klog.Warningf(util.Log(ctx, "failed to get backend volume for %s: %v"), string(volID), err)
 			return &csi.DeleteVolumeResponse{}, nil
 		}
 		// if error is ErrKeyNotFound, then a previous attempt at deletion was complete
 		// or partially complete (subvolume and imageOMap are garbage collected already), hence
 		// return success as deletion is complete
-		var eknf util.ErrKeyNotFound
+		if errors.Is(err, util.ErrKeyNotFound) {
-		if errors.As(err, &eknf) {
 			return &csi.DeleteVolumeResponse{}, nil
 		}
 

internal/journal/omap.go

@@ -37,7 +37,7 @@ func getOMapValues(
 	// fetch and configure the rados ioctx
 	ioctx, err := conn.conn.GetIoctx(poolName)
 	if err != nil {
-		return nil, omapPoolError(poolName, err)
+		return nil, omapPoolError(err)
 	}
 	defer ioctx.Destroy()
 
@@ -66,7 +66,7 @@ func getOMapValues(
 			klog.Errorf(
 				util.Log(ctx, "omap not found (pool=%q, namespace=%q, name=%q): %v"),
 				poolName, namespace, oid, err)
-			return nil, util.NewErrKeyNotFound(oid, err)
+			return nil, util.JoinErrors(util.ErrKeyNotFound, err)
 		}
 		return nil, err
 	}
@@ -83,7 +83,7 @@ func removeMapKeys(
 	// fetch and configure the rados ioctx
 	ioctx, err := conn.conn.GetIoctx(poolName)
 	if err != nil {
-		return omapPoolError(poolName, err)
+		return omapPoolError(err)
 	}
 	defer ioctx.Destroy()
 
@@ -118,7 +118,7 @@ func setOMapKeys(
 	// fetch and configure the rados ioctx
 	ioctx, err := conn.conn.GetIoctx(poolName)
 	if err != nil {
-		return omapPoolError(poolName, err)
+		return omapPoolError(err)
 	}
 	defer ioctx.Destroy()
 
@@ -142,9 +142,9 @@ func setOMapKeys(
 	return nil
 }
 
-func omapPoolError(poolName string, err error) error {
+func omapPoolError(err error) error {
 	if errors.Is(err, rados.ErrNotFound) {
-		return util.NewErrPoolNotFound(poolName, err)
+		return util.JoinErrors(util.ErrPoolNotFound, err)
 	}
 	return err
 }

internal/journal/voljournal.go

@@ -282,9 +282,7 @@ func (conn *Connection) CheckReservation(ctx context.Context,
 		ctx, conn, journalPool, cj.namespace, cj.csiDirectory,
 		cj.commonPrefix, fetchKeys)
 	if err != nil {
-		var eknf util.ErrKeyNotFound
+		if errors.Is(err, util.ErrKeyNotFound) || errors.Is(err, util.ErrPoolNotFound) {
-		var epnf util.ErrPoolNotFound
-		if errors.As(err, &eknf) || errors.As(err, &epnf) {
 			// pool or omap (oid) was not present
 			// stop processing but without an error for no reservation exists
 			return nil, nil
@@ -316,8 +314,7 @@ func (conn *Connection) CheckReservation(ctx context.Context,
 
 		savedImagePool, err = util.GetPoolName(conn.monitors, conn.cr, savedImagePoolID)
 		if err != nil {
-			var epnf util.ErrPoolNotFound
+			if errors.Is(err, util.ErrPoolNotFound) {
-			if errors.As(err, &epnf) {
 				err = conn.UndoReservation(ctx, journalPool, "", "", reqName)
 			}
 			return nil, err
@@ -329,8 +326,7 @@ func (conn *Connection) CheckReservation(ctx context.Context,
 	if err != nil {
 		// error should specifically be not found, for image to be absent, any other error
 		// is not conclusive, and we should not proceed
-		var eknf util.ErrKeyNotFound
+		if errors.Is(err, util.ErrKeyNotFound) {
-		if errors.As(err, &eknf) {
 			err = conn.UndoReservation(ctx, journalPool, savedImagePool,
 				cj.GetNameForUUID(namePrefix, objUUID, snapSource), reqName)
 		}
@@ -363,10 +359,10 @@ func (conn *Connection) CheckReservation(ctx context.Context,
 		if savedImageAttributes.SourceName != parentName {
 			// NOTE: This can happen if there is a snapname conflict, and we already have a snapshot
 			// with the same name pointing to a different UUID as the source
-			err = fmt.Errorf("snapname points to different volume, request name (%s)"+
+			err = fmt.Errorf("%w: snapname points to different volume, request name (%s)"+
-				" source name (%s) saved source name (%s)",
+				" source name (%s) saved source name (%s)", util.ErrSnapNameConflict,
 				reqName, parentName, savedImageAttributes.SourceName)
-			return nil, util.NewErrSnapNameConflict(reqName, err)
+			return nil, err
 		}
 	}
 
@@ -412,8 +408,7 @@ func (conn *Connection) UndoReservation(ctx context.Context,
 
 		err := util.RemoveObject(ctx, conn.monitors, conn.cr, volJournalPool, cj.namespace, cj.cephUUIDDirectoryPrefix+imageUUID)
 		if err != nil {
-			var eonf util.ErrObjectNotFound
+			if !errors.Is(err, util.ErrObjectNotFound) {
-			if !errors.As(err, &eonf) {
 				klog.Errorf(util.Log(ctx, "failed removing oMap %s (%s)"), cj.cephUUIDDirectoryPrefix+imageUUID, err)
 				return err
 			}
@@ -445,8 +440,7 @@ func reserveOMapName(ctx context.Context, monitors string, cr *util.Credentials,
 
 		err := util.CreateObject(ctx, monitors, cr, pool, namespace, oMapNamePrefix+iterUUID)
 		if err != nil {
-			var eoe util.ErrObjectExists
+			if errors.Is(err, util.ErrObjectExists) {
-			if errors.As(err, &eoe) {
 				attempt++
 				// try again with a different uuid, for maxAttempts tries
 				util.DebugLog(ctx, "uuid (%s) conflict detected, retrying (attempt %d of %d)",
@@ -616,9 +610,7 @@ func (conn *Connection) GetImageAttributes(ctx context.Context, pool, objectUUID
 		ctx, conn, pool, cj.namespace, cj.cephUUIDDirectoryPrefix+objectUUID,
 		cj.commonPrefix, fetchKeys)
 	if err != nil {
-		var eknf util.ErrKeyNotFound
+		if !errors.Is(err, util.ErrKeyNotFound) && !errors.Is(err, util.ErrPoolNotFound) {
-		var epnf util.ErrPoolNotFound
-		if !errors.As(err, &eknf) && !errors.As(err, &epnf) {
 			return nil, err
 		}
 		klog.Warningf(util.Log(ctx, "unable to read omap keys: pool or key missing: %v"), err)
@@ -656,9 +648,8 @@ func (conn *Connection) GetImageAttributes(ctx context.Context, pool, objectUUID
 	if snapSource {
 		imageAttributes.SourceName, found = values[cj.cephSnapSourceKey]
 		if !found {
-			return nil, util.NewErrKeyNotFound(
+			return nil, fmt.Errorf("%w: no snap source in omap for %q",
-				cj.cephSnapSourceKey,
+				util.ErrKeyNotFound, cj.cephUUIDDirectoryPrefix+objectUUID)
-				fmt.Errorf("no snap source in omap for %q", cj.cephUUIDDirectoryPrefix+objectUUID))
 		}
 	}
 

internal/rbd/controllerserver.go

@@ -391,8 +391,7 @@ func (cs *ControllerServer) createVolumeFromSnapshot(ctx context.Context, cr *ut
 
 	err := genSnapFromSnapID(ctx, rbdSnap, snapshotID, cr)
 	if err != nil {
-		var epnf util.ErrPoolNotFound
+		if errors.Is(err, util.ErrPoolNotFound) {
-		if errors.As(err, &epnf) {
 			klog.Errorf(util.Log(ctx, "failed to get backend snapshot for %s: %v"), snapshotID, err)
 			return status.Error(codes.InvalidArgument, err.Error())
 		}
@@ -581,8 +580,7 @@ func (cs *ControllerServer) DeleteVolume(ctx context.Context, req *csi.DeleteVol
 
 	rbdVol, err = genVolFromVolID(ctx, volumeID, cr, req.GetSecrets())
 	if err != nil {
-		var epnf util.ErrPoolNotFound
+		if errors.Is(err, util.ErrPoolNotFound) {
-		if errors.As(err, &epnf) {
 			klog.Warningf(util.Log(ctx, "failed to get backend volume for %s: %v"), volumeID, err)
 			return &csi.DeleteVolumeResponse{}, nil
 		}
@@ -590,8 +588,7 @@ func (cs *ControllerServer) DeleteVolume(ctx context.Context, req *csi.DeleteVol
 		// 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
-		var eknf util.ErrKeyNotFound
+		if errors.Is(err, util.ErrKeyNotFound) {
-		if errors.As(err, &eknf) {
 			klog.Warningf(util.Log(ctx, "Failed to volume options for %s: %v"), volumeID, err)
 			return &csi.DeleteVolumeResponse{}, nil
 		}
@@ -717,11 +714,10 @@ func (cs *ControllerServer) CreateSnapshot(ctx context.Context, req *csi.CreateS
 	rbdVol, err = genVolFromVolID(ctx, req.GetSourceVolumeId(), cr, req.GetSecrets())
 	if err != nil {
 		var einf ErrImageNotFound
-		var epnf util.ErrPoolNotFound
 		// nolint:gocritic // this ifElseChain can not be rewritten to a switch statement
 		if errors.As(err, &einf) {
 			err = status.Errorf(codes.NotFound, "source Volume ID %s not found", req.GetSourceVolumeId())
-		} else if errors.As(err, &epnf) {
+		} else if errors.Is(err, util.ErrPoolNotFound) {
 			klog.Errorf(util.Log(ctx, "failed to get backend volume for %s: %v"), req.GetSourceVolumeId(), err)
 			err = status.Errorf(codes.NotFound, err.Error())
 		} else {
@@ -765,8 +761,7 @@ func (cs *ControllerServer) CreateSnapshot(ctx context.Context, req *csi.CreateS
 	// check for the requested source volume id and already allocated source volume id
 	found, err := checkSnapCloneExists(ctx, rbdVol, rbdSnap, cr)
 	if err != nil {
-		var esnc util.ErrSnapNameConflict
+		if errors.Is(err, util.ErrSnapNameConflict) {
-		if errors.As(err, &esnc) {
 			return nil, status.Error(codes.AlreadyExists, err.Error())
 		}
 		return nil, status.Errorf(codes.Internal, err.Error())
@@ -983,8 +978,7 @@ func (cs *ControllerServer) DeleteSnapshot(ctx context.Context, req *csi.DeleteS
 	if err = genSnapFromSnapID(ctx, rbdSnap, snapshotID, cr); err != nil {
 		// if error is ErrPoolNotFound, the pool is already deleted we dont
 		// need to worry about deleting snapshot or omap data, return success
-		var epnf util.ErrPoolNotFound
+		if errors.Is(err, util.ErrPoolNotFound) {
-		if errors.As(err, &epnf) {
 			klog.Warningf(util.Log(ctx, "failed to get backend snapshot for %s: %v"), snapshotID, err)
 			return &csi.DeleteSnapshotResponse{}, nil
 		}
@@ -992,8 +986,7 @@ func (cs *ControllerServer) DeleteSnapshot(ctx context.Context, req *csi.DeleteS
 		// if error is ErrKeyNotFound, then a previous attempt at deletion was complete
 		// or partially complete (snap and snapOMap are garbage collected already), hence return
 		// success as deletion is complete
-		var eknf util.ErrKeyNotFound
+		if errors.Is(err, util.ErrKeyNotFound) {
-		if errors.As(err, &eknf) {
 			return &csi.DeleteSnapshotResponse{}, nil
 		}
 
@@ -1089,11 +1082,10 @@ func (cs *ControllerServer) ControllerExpandVolume(ctx context.Context, req *csi
 	rbdVol, err = genVolFromVolID(ctx, volID, cr, req.GetSecrets())
 	if err != nil {
 		var einf ErrImageNotFound
-		var epnf util.ErrPoolNotFound
 		// nolint:gocritic // this ifElseChain can not be rewritten to a switch statement
 		if errors.As(err, &einf) {
 			err = status.Errorf(codes.NotFound, "volume ID %s not found", volID)
-		} else if errors.As(err, &epnf) {
+		} else if errors.Is(err, util.ErrPoolNotFound) {
 			klog.Errorf(util.Log(ctx, "failed to get backend volume for %s: %v"), volID, err)
 			err = status.Errorf(codes.NotFound, err.Error())
 		} else {

internal/util/cephcmds.go

@@ -61,7 +61,8 @@ func GetPoolID(monitors string, cr *Credentials, poolName string) (int64, error)
 
 	id, err := conn.GetPoolByName(poolName)
 	if errors.Is(err, rados.ErrNotFound) {
-		return InvalidPoolID, ErrPoolNotFound{poolName, fmt.Errorf("pool (%s) not found in Ceph cluster", poolName)}
+		return InvalidPoolID, fmt.Errorf("%w: pool (%s) not found in Ceph cluster",
+			ErrPoolNotFound, poolName)
 	} else if err != nil {
 		return InvalidPoolID, err
 	}
@@ -80,7 +81,8 @@ func GetPoolName(monitors string, cr *Credentials, poolID int64) (string, error)
 
 	name, err := conn.GetPoolByID(poolID)
 	if err != nil {
-		return "", ErrPoolNotFound{string(poolID), fmt.Errorf("pool ID (%d) not found in Ceph cluster", poolID)}
+		return "", fmt.Errorf("%w: pool ID (%d) not found in Ceph cluster",
+			ErrPoolNotFound, poolID)
 	}
 	return name, nil
 }
@@ -117,9 +119,8 @@ func CreateObject(ctx context.Context, monitors string, cr *Credentials, poolNam
 
 	ioctx, err := conn.GetIoctx(poolName)
 	if err != nil {
-		var epnf ErrPoolNotFound
+		if errors.Is(err, ErrPoolNotFound) {
-		if errors.As(err, &epnf) {
+			err = JoinErrors(ErrObjectNotFound, err)
-			err = ErrObjectNotFound{poolName, err}
 		}
 		return err
 	}
@@ -131,7 +132,7 @@ func CreateObject(ctx context.Context, monitors string, cr *Credentials, poolNam
 
 	err = ioctx.Create(objectName, rados.CreateExclusive)
 	if errors.Is(err, rados.ErrObjectExists) {
-		return ErrObjectExists{objectName, err}
+		return JoinErrors(ErrObjectExists, err)
 	} else if err != nil {
 		klog.Errorf(Log(ctx, "failed creating omap (%s) in pool (%s): (%v)"), objectName, poolName, err)
 		return err
@@ -152,9 +153,8 @@ func RemoveObject(ctx context.Context, monitors string, cr *Credentials, poolNam
 
 	ioctx, err := conn.GetIoctx(poolName)
 	if err != nil {
-		var epnf ErrPoolNotFound
+		if errors.Is(err, ErrPoolNotFound) {
-		if errors.As(err, &epnf) {
+			err = JoinErrors(ErrObjectNotFound, err)
-			err = ErrObjectNotFound{poolName, err}
 		}
 		return err
 	}
@@ -166,7 +166,7 @@ func RemoveObject(ctx context.Context, monitors string, cr *Credentials, poolNam
 
 	err = ioctx.Delete(oMapName)
 	if errors.Is(err, rados.ErrNotFound) {
-		return ErrObjectNotFound{oMapName, err}
+		return JoinErrors(ErrObjectNotFound, err)
 	} else if err != nil {
 		klog.Errorf(Log(ctx, "failed removing omap (%s) in pool (%s): (%v)"), oMapName, poolName, err)
 		return err

internal/util/connection.go

@@ -74,7 +74,7 @@ func (cc *ClusterConnection) GetIoctx(pool string) (*rados.IOContext, error) {
 	if err != nil {
 		// ErrNotFound indicates the Pool was not found
 		if errors.Is(err, rados.ErrNotFound) {
-			err = ErrPoolNotFound{pool, err}
+			err = JoinErrors(ErrPoolNotFound, err)
 		} else {
 			err = fmt.Errorf("failed to open IOContext for pool %s: %w", pool, err)
 		}

internal/util/errors.go

@@ -16,98 +16,46 @@ limitations under the License.
 
 package util
 
-// ErrKeyNotFound is returned when requested key in omap is not found.
+import (
-type ErrKeyNotFound struct {
+	"errors"
-	keyName string
+	"fmt"
-	err     error
+)
+
+var (
+	// ErrKeyNotFound is returned when requested key in omap is not found.
+	ErrKeyNotFound = errors.New("key not found")
+	// ErrObjectExists is returned when named omap is already present in rados.
+	ErrObjectExists = errors.New("object exists")
+	// ErrObjectNotFound is returned when named omap is not found in rados.
+	ErrObjectNotFound = errors.New("object not found")
+	// ErrSnapNameConflict is generated when a requested CSI snap name already exists on RBD but with
+	// different properties, and hence is in conflict with the passed in CSI volume name.
+	ErrSnapNameConflict = errors.New("snapshot name conflict")
+	// ErrPoolNotFound is returned when pool is not found.
+	ErrPoolNotFound = errors.New("pool not found")
+)
+
+type errorPair struct {
+	first  error
+	second error
 }
 
-// NewErrKeyNotFound returns a new ErrKeyNotFound error.
+func (e errorPair) Error() string {
-func NewErrKeyNotFound(keyName string, err error) ErrKeyNotFound {
+	return fmt.Sprintf("%v: %v", e.first, e.second)
-	return ErrKeyNotFound{keyName, err}
 }
 
-// Error returns the error string for ErrKeyNotFound.
+// Is checks if target error is wrapped in the first error.
-func (e ErrKeyNotFound) Error() string {
+func (e errorPair) Is(target error) bool {
-	return e.err.Error()
+	return errors.Is(e.first, target)
 }
 
-// Unwrap returns the encapsulated error.
+// Unwrap returns the second error.
-func (e ErrKeyNotFound) Unwrap() error {
+func (e errorPair) Unwrap() error {
-	return e.err
+	return e.second
 }
 
-// ErrObjectExists is returned when named omap is already present in rados.
+// JoinErrors combines two errors. Of the returned error, Is() follows the first
-type ErrObjectExists struct {
+// branch, Unwrap() folllows the second branch.
-	objectName string
+func JoinErrors(e1, e2 error) error {
-	err        error
+	return errorPair{e1, e2}
-}
-
-// Error returns the error string for ErrObjectExists.
-func (e ErrObjectExists) Error() string {
-	return e.err.Error()
-}
-
-// Unwrap returns the encapsulated error.
-func (e ErrObjectExists) Unwrap() error {
-	return e.err
-}
-
-// ErrObjectNotFound is returned when named omap is not found in rados.
-type ErrObjectNotFound struct {
-	oMapName string
-	err      error
-}
-
-// Error returns the error string for ErrObjectNotFound.
-func (e ErrObjectNotFound) Error() string {
-	return e.err.Error()
-}
-
-// Unwrap returns the encapsulated error.
-func (e ErrObjectNotFound) Unwrap() error {
-	return e.err
-}
-
-// ErrSnapNameConflict is generated when a requested CSI snap name already exists on RBD but with
-// different properties, and hence is in conflict with the passed in CSI volume name.
-type ErrSnapNameConflict struct {
-	requestName string
-	err         error
-}
-
-// Error returns the error string for ErrSnapNameConflict.
-func (e ErrSnapNameConflict) Error() string {
-	return e.err.Error()
-}
-
-// Unwrap returns the encapsulated error.
-func (e ErrSnapNameConflict) Unwrap() error {
-	return e.err
-}
-
-// NewErrSnapNameConflict returns a ErrSnapNameConflict error when CSI snap name already exists.
-func NewErrSnapNameConflict(name string, err error) ErrSnapNameConflict {
-	return ErrSnapNameConflict{name, err}
-}
-
-// ErrPoolNotFound is returned when pool is not found.
-type ErrPoolNotFound struct {
-	Pool string
-	Err  error
-}
-
-// Error returns the error string for ErrPoolNotFound.
-func (e ErrPoolNotFound) Error() string {
-	return e.Err.Error()
-}
-
-// Unwrap returns the encapsulated error.
-func (e ErrPoolNotFound) Unwrap() error {
-	return e.Err
-}
-
-// NewErrPoolNotFound returns a new ErrPoolNotFound error.
-func NewErrPoolNotFound(pool string, err error) ErrPoolNotFound {
-	return ErrPoolNotFound{pool, err}
 }