From 18162c71bc146fed57afff8e10c1c3319815f65e Mon Sep 17 00:00:00 2001 From: Niels de Vos Date: Thu, 21 Mar 2024 10:48:50 +0100 Subject: [PATCH] cleanup: do not pass an empty snapshot to genSnapFromSnapID() Just like GenVolFromVolID() the genSnapFromSnapID() function can return a snapshot. There is no need to allocated an empty snapshot and pass that to the genSnapFromSnapID() function. Signed-off-by: Niels de Vos --- internal/rbd/controllerserver.go | 11 ++++---- internal/rbd/rbd_util.go | 45 +++++++++++++++++--------------- 2 files changed, 29 insertions(+), 27 deletions(-) diff --git a/internal/rbd/controllerserver.go b/internal/rbd/controllerserver.go index 75197c8bb..5f5e70ecd 100644 --- a/internal/rbd/controllerserver.go +++ b/internal/rbd/controllerserver.go @@ -638,7 +638,6 @@ func (cs *ControllerServer) createVolumeFromSnapshot( rbdVol *rbdVolume, snapshotID string, ) error { - rbdSnap := &rbdSnapshot{} if acquired := cs.SnapshotLocks.TryAcquire(snapshotID); !acquired { log.ErrorLog(ctx, util.SnapshotOperationAlreadyExistsFmt, snapshotID) @@ -646,7 +645,7 @@ func (cs *ControllerServer) createVolumeFromSnapshot( } defer cs.SnapshotLocks.Release(snapshotID) - err := genSnapFromSnapID(ctx, rbdSnap, snapshotID, cr, secrets) + rbdSnap, err := genSnapFromSnapID(ctx, snapshotID, cr, secrets) if err != nil { if errors.Is(err, util.ErrPoolNotFound) { log.ErrorLog(ctx, "failed to get backend snapshot for %s: %v", snapshotID, err) @@ -789,8 +788,8 @@ func checkContentSource( if snapshotID == "" { return nil, nil, status.Errorf(codes.NotFound, "volume Snapshot ID cannot be empty") } - rbdSnap := &rbdSnapshot{} - if err := genSnapFromSnapID(ctx, rbdSnap, snapshotID, cr, req.GetSecrets()); err != nil { + rbdSnap, err := genSnapFromSnapID(ctx, snapshotID, cr, req.GetSecrets()) + if err != nil { log.ErrorLog(ctx, "failed to get backend snapshot for %s: %v", snapshotID, err) if !errors.Is(err, ErrSnapNotFound) { return nil, nil, status.Error(codes.Internal, err.Error()) @@ -1429,8 +1428,8 @@ func (cs *ControllerServer) DeleteSnapshot( } defer cs.OperationLocks.ReleaseDeleteLock(snapshotID) - rbdSnap := &rbdSnapshot{} - if err = genSnapFromSnapID(ctx, rbdSnap, snapshotID, cr, req.GetSecrets()); err != nil { + rbdSnap, err := genSnapFromSnapID(ctx, snapshotID, cr, req.GetSecrets()) + if err != nil { // if error is ErrPoolNotFound, the pool is already deleted we don't // need to worry about deleting snapshot or omap data, return success if errors.Is(err, util.ErrPoolNotFound) { diff --git a/internal/rbd/rbd_util.go b/internal/rbd/rbd_util.go index b318d0f62..13e226bf4 100644 --- a/internal/rbd/rbd_util.go +++ b/internal/rbd/rbd_util.go @@ -949,54 +949,57 @@ func (ri *rbdImage) checkImageChainHasFeature(ctx context.Context, feature uint6 // genSnapFromSnapID generates a rbdSnapshot structure from the provided identifier, updating // the structure with elements from on-disk snapshot metadata as well. +// +// NOTE: The returned rbdSnapshot can be non-nil in case of an error. That +// seems to be required for the DeleteSnapshot procedure, so that OMAP +// attributes can be cleaned-up. func genSnapFromSnapID( ctx context.Context, - rbdSnap *rbdSnapshot, snapshotID string, cr *util.Credentials, secrets map[string]string, -) error { +) (*rbdSnapshot, error) { var vi util.CSIIdentifier - rbdSnap.VolID = snapshotID - - err := vi.DecomposeCSIID(rbdSnap.VolID) + err := vi.DecomposeCSIID(snapshotID) if err != nil { - log.ErrorLog(ctx, "error decoding snapshot ID (%s) (%s)", err, rbdSnap.VolID) + log.ErrorLog(ctx, "error decoding snapshot ID (%s) (%s)", err, snapshotID) - return err + return nil, err } + rbdSnap := &rbdSnapshot{} + rbdSnap.VolID = snapshotID rbdSnap.ClusterID = vi.ClusterID rbdSnap.Monitors, _, err = util.GetMonsAndClusterID(ctx, rbdSnap.ClusterID, false) if err != nil { log.ErrorLog(ctx, "failed getting mons (%s)", err) - return err + return nil, err } rbdSnap.Pool, err = util.GetPoolName(rbdSnap.Monitors, cr, vi.LocationID) if err != nil { - return err + return nil, err } rbdSnap.JournalPool = rbdSnap.Pool rbdSnap.RadosNamespace, err = util.GetRadosNamespace(util.CsiConfigFile, rbdSnap.ClusterID) if err != nil { - return err + return nil, err } j, err := snapJournal.Connect(rbdSnap.Monitors, rbdSnap.RadosNamespace, cr) if err != nil { - return err + return nil, err } defer j.Destroy() imageAttributes, err := j.GetImageAttributes( ctx, rbdSnap.Pool, vi.ObjectUUID, true) if err != nil { - return err + return rbdSnap, err } rbdSnap.ImageID = imageAttributes.ImageID rbdSnap.RequestName = imageAttributes.RequestName @@ -1009,42 +1012,42 @@ func genSnapFromSnapID( rbdSnap.JournalPool, err = util.GetPoolName(rbdSnap.Monitors, cr, imageAttributes.JournalPoolID) if err != nil { // TODO: If pool is not found we may leak the image (as DeleteSnapshot will return success) - return err + return rbdSnap, err } } err = rbdSnap.Connect(cr) + if err != nil { + return rbdSnap, fmt.Errorf("failed to connect to %q: %w", + rbdSnap, err) + } defer func() { if err != nil { rbdSnap.Destroy() } }() - if err != nil { - return fmt.Errorf("failed to connect to %q: %w", - rbdSnap, err) - } if imageAttributes.KmsID != "" && imageAttributes.EncryptionType == util.EncryptionTypeBlock { err = rbdSnap.configureBlockEncryption(imageAttributes.KmsID, secrets) if err != nil { - return fmt.Errorf("failed to configure block encryption for "+ + return rbdSnap, fmt.Errorf("failed to configure block encryption for "+ "%q: %w", rbdSnap, err) } } if imageAttributes.KmsID != "" && imageAttributes.EncryptionType == util.EncryptionTypeFile { err = rbdSnap.configureFileEncryption(ctx, imageAttributes.KmsID, secrets) if err != nil { - return fmt.Errorf("failed to configure file encryption for "+ + return rbdSnap, fmt.Errorf("failed to configure file encryption for "+ "%q: %w", rbdSnap, err) } } err = updateSnapshotDetails(rbdSnap) if err != nil { - return fmt.Errorf("failed to update snapshot details for %q: %w", rbdSnap, err) + return rbdSnap, fmt.Errorf("failed to update snapshot details for %q: %w", rbdSnap, err) } - return err + return rbdSnap, err } // updateSnapshotDetails will copy the details from the rbdVolume to the