diff --git a/e2e/utils.go b/e2e/utils.go index c02fc9d1c99..d0319aa2f63 100644 --- a/e2e/utils.go +++ b/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 { diff --git a/e2e/volumegroupsnapshot.go b/e2e/volumegroupsnapshot.go index 9ec6f70ec16..513f6d60717 100644 --- a/e2e/volumegroupsnapshot.go +++ b/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 } diff --git a/internal/rbd/controllerserver.go b/internal/rbd/controllerserver.go index 3ec067c3ecf..eb37a806356 100644 --- a/internal/rbd/controllerserver.go +++ b/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()) } diff --git a/internal/rbd/group/util.go b/internal/rbd/group/util.go index 39583edf027..6dcf28e4445 100644 --- a/internal/rbd/group/util.go +++ b/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) } diff --git a/internal/rbd/rbd_util.go b/internal/rbd/rbd_util.go index fbae7f4181d..343d6370895 100644 --- a/internal/rbd/rbd_util.go +++ b/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 diff --git a/internal/rbd/snapshot.go b/internal/rbd/snapshot.go index 6a06cefc288..ba6f640ab5d 100644 --- a/internal/rbd/snapshot.go +++ b/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,