Merge of #5031

e2e/utils.go

@@ -214,6 +214,14 @@ func validateOmapCount(f *framework.Framework, count int, driver, pool, mode str
 			radosLsKeysCmd:       "rados listomapkeys csi.groups.default " + cephfsOptions(pool),
 			radosLsKeysCmdFilter: fmt.Sprintf("rados listomapkeys csi.groups.default %s | wc -l", cephfsOptions(pool)),
 		},
+		{
+			volumeMode:           groupSnapsType,
+			driverType:           rbdType,
+			radosLsCmd:           "rados ls" + rbdOptions(pool),
+			radosLsCmdFilter:     fmt.Sprintf("rados ls %s | grep -v default | grep -c ^csi.volume.group.", rbdOptions(pool)),
+			radosLsKeysCmd:       "rados listomapkeys csi.groups.default " + rbdOptions(pool),
+			radosLsKeysCmdFilter: fmt.Sprintf("rados listomapkeys csi.groups.default %s | wc -l", rbdOptions(pool)),
+		},
 	}
 
 	for _, cmds := range radosListCommands {

e2e/volumegroupsnapshot.go

@@ -201,6 +201,14 @@ func (rvgs *rbdVolumeGroupSnapshot) ValidateResourcesForCreate(vgs *groupsnapapi
 
 func (rvgs *rbdVolumeGroupSnapshot) ValidateResourcesForDelete() error {
 	validateOmapCount(rvgs.framework, 0, rbdType, defaultRBDPool, volumesType)
+	validateOmapCount(rvgs.framework, 0, rbdType, defaultRBDPool, snapsType)
+	validateOmapCount(rvgs.framework, 0, rbdType, defaultRBDPool, groupSnapsType)
+	validateRBDImageCount(rvgs.framework, 0, defaultRBDPool)
+
+	err := waitToRemoveImagesFromTrash(rvgs.framework, defaultRBDPool, deployTimeout)
+	if err != nil {
+		return err
+	}
 
 	return nil
 }

internal/rbd/controllerserver.go

@@ -1471,27 +1471,9 @@ func (cs *ControllerServer) DeleteSnapshot(
 	// Deleting snapshot and cloned volume
 	log.DebugLog(ctx, "deleting cloned rbd volume %s", rbdSnap.RbdSnapName)
 
-	rbdVol := rbdSnap.toVolume()
-
-	err = rbdVol.Connect(cr)
-	if err != nil {
-		return nil, status.Error(codes.Internal, err.Error())
-	}
-	defer rbdVol.Destroy(ctx)
-
-	rbdVol.ImageID = rbdSnap.ImageID
-	// update parent name to delete the snapshot
-	rbdSnap.RbdImageName = rbdVol.RbdImageName
-	err = cleanUpSnapshot(ctx, rbdVol, rbdSnap, rbdVol)
-	if err != nil {
-		log.ErrorLog(ctx, "failed to delete image: %v", err)
-
-		return nil, status.Error(codes.Internal, err.Error())
-	}
-	err = undoSnapReservation(ctx, rbdSnap, cr)
+	err = rbdSnap.Delete(ctx)
 	if err != nil {
-		log.ErrorLog(ctx, "failed to remove reservation for snapname (%s) with backing snap (%s) on image (%s) (%s)",
-			rbdSnap.RequestName, rbdSnap.RbdSnapName, rbdSnap.RbdImageName, err)
+		log.ErrorLog(ctx, "failed to delete rbd snapshot: %s with error: %v", rbdSnap, err)
 
 		return nil, status.Error(codes.Internal, err.Error())
 	}

internal/rbd/group/util.go

@@ -183,6 +183,15 @@ func (cvg *commonVolumeGroup) GetName(ctx context.Context) (string, error) {
 	return cvg.name, nil
 }
 
+// GetRequestName returns the requestName of the VolumeGroup.
+func (cvg *commonVolumeGroup) GetRequestName(ctx context.Context) (string, error) {
+	if cvg.requestName == "" {
+		return "", errors.New("BUG: requestName is not set")
+	}
+
+	return cvg.requestName, nil
+}
+
 // GetPool returns the name of the pool that holds the VolumeGroup.
 func (cvg *commonVolumeGroup) GetPool(ctx context.Context) (string, error) {
 	if cvg.pool == "" {
@@ -285,9 +294,9 @@ func (cvg *commonVolumeGroup) Delete(ctx context.Context) error {
 		return fmt.Errorf("failed to get name for volume group %q: %w", cvg, err)
 	}
 
-	csiID, err := cvg.GetID(ctx)
+	reqName, err := cvg.GetRequestName(ctx)
 	if err != nil {
-		return fmt.Errorf("failed to get id for volume group %q: %w", cvg, err)
+		return fmt.Errorf("failed to get request name for volume group %q: %w", cvg, err)
 	}
 
 	pool, err := cvg.GetPool(ctx)
@@ -300,7 +309,7 @@ func (cvg *commonVolumeGroup) Delete(ctx context.Context) error {
 		return err
 	}
 
-	err = j.UndoReservation(ctx, pool, name, csiID)
+	err = j.UndoReservation(ctx, pool, name, reqName)
 	if err != nil /* TODO? !errors.Is(..., err) */ {
 		return fmt.Errorf("failed to undo the reservation for volume group %q: %w", cvg, err)
 	}

internal/rbd/rbd_util.go

@@ -680,6 +680,10 @@ func (ri *rbdImage) Delete(ctx context.Context) error {
 	rbdImage := librbd.GetImage(ri.ioctx, image)
 	err = rbdImage.Trash(0)
 	if err != nil {
+		if errors.Is(err, librbd.ErrNotFound) {
+			return fmt.Errorf("Failed as %w (internal %w)", ErrImageNotFound, err)
+		}
+
 		log.ErrorLog(ctx, "failed to delete rbd image: %s, error: %v", ri, err)
 
 		return err

internal/rbd/snapshot.go

@@ -82,7 +82,7 @@ func cleanUpSnapshot(
 ) error {
 	err := parentVol.deleteSnapshot(ctx, rbdSnap)
 	if err != nil {
-		if !errors.Is(err, ErrSnapNotFound) {
+		if !errors.Is(err, ErrImageNotFound) && !errors.Is(err, ErrSnapNotFound) {
 			log.ErrorLog(ctx, "failed to delete snapshot %q: %v", rbdSnap, err)
 
 			return err
@@ -162,6 +162,42 @@ func (rbdSnap *rbdSnapshot) ToCSI(ctx context.Context) (*csi.Snapshot, error) {
 	}, nil
 }
 
+// Delete removes the snapshot from the RBD image and then
+// the RBD image itself. If the backing RBD snapshot and image is removed
+// successfully, the reservation for the snapshot is removed from the journal.
+//
+// NOTE: As the function manipulates omaps, it should be called with a lock against the request name
+// held, to prevent parallel operations from modifying the state of the omaps for this request name.
+func (rbdSnap *rbdSnapshot) Delete(ctx context.Context) error {
+	rbdVol := rbdSnap.toVolume()
+
+	err := rbdVol.Connect(rbdSnap.conn.Creds)
+	if err != nil {
+		return err
+	}
+	defer rbdVol.Destroy(ctx)
+
+	rbdVol.ImageID = rbdSnap.ImageID
+	// update parent name to delete the snapshot
+	rbdSnap.RbdImageName = rbdVol.RbdImageName
+	err = cleanUpSnapshot(ctx, rbdVol, rbdSnap, rbdVol)
+	if err != nil {
+		log.ErrorLog(ctx, "failed to cleanup image %s and snapshot %s: %v", rbdVol, rbdSnap, err)
+
+		return err
+	}
+
+	err = undoSnapReservation(ctx, rbdSnap, rbdSnap.conn.Creds)
+	if err != nil {
+		log.ErrorLog(ctx, "failed to remove reservation for snapname (%s) with backing snap (%s) on image (%s) (%s)",
+			rbdSnap.RequestName, rbdSnap.RbdSnapName, rbdSnap.RbdImageName, err)
+
+		return err
+	}
+
+	return nil
+}
+
 func undoSnapshotCloning(
 	ctx context.Context,
 	parentVol *rbdVolume,