Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RBD: Flatten group snapshot #4973

Open
wants to merge 5 commits into
base: devel
Choose a base branch
from
+139 −100
Open

RBD: Flatten group snapshot #4973

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3d5f06d
e2e: test creation of additional groupSnaps to test minSnapLimit
Rakshith-R Nov 22, 2024
2bcdc34
rbd: consolidate snapshot flatten logic in PrepareVolumeForSnapshot()
Rakshith-R Nov 13, 2024
9857641
rbd: make use of both listSnapshots and listChildren
Rakshith-R Nov 22, 2024
8c99b2f
rbd: add layering & deep flattenfeatures for groupsnapshot image
Rakshith-R Dec 5, 2024
c420bf5
cleanup: pass volumeGroupSnapshotterBase by pointer
Rakshith-R Dec 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
rbd: consolidate snapshot flatten logic in PrepareVolumeForSnapshot() 
This commit consolidates flatten logic checks for cloneDepth
and snapshotLimit in PrepareVolumeForSnapshot. This allows
the function to be called for both CreateSnapshot and
CreateVolumeGroupSnapshot.
Clone Depth check and flattening of grand parent image
now occurs before creation of snapshot starts.
This aligns better with how PVC-PVC clone and
PVC-restore process occurs currently.
Flattening the grandparent image once prevents
flattening of every newly created snapshot.
Snapshot in above para refers to k8s VolumeSnapshot
(which is backed by a rbd image).

Signed-off-by: Rakshith R <[email protected]>
  • Loading branch information
@Rakshith-R
Rakshith-R committed Dec 16, 2024
commit 2bcdc34946eafe9880c6040ec5f1b33694b3c016
7 changes: 1 addition & 6 deletions internal/rbd/controllerserver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1157,7 +1157,7 @@ func (cs *ControllerServer) CreateSnapshot(
return cloneFromSnapshot(ctx, rbdVol, rbdSnap, cr, req.GetParameters())
}

err = flattenTemporaryClonedImages(ctx, rbdVol, cr)
err = rbdVol.PrepareVolumeForSnapshot(ctx, cr)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -1376,11 +1376,6 @@ func (cs *ControllerServer) doSnapshotClone(
return cloneRbd, err
}

err = cloneRbd.flattenRbdImage(ctx, false, rbdHardMaxCloneDepth, rbdSoftMaxCloneDepth)
if err != nil {
return cloneRbd, err
}

return cloneRbd, nil
}

Expand Down
23 changes: 23 additions & 0 deletions internal/rbd/group_controllerserver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ import (
// group snapshot and remove all images from the group again. This leaves the
// group and its snapshot around, the group snapshot can be inspected to list
// the snapshots of the images.
//
//nolint:gocyclo,cyclop // TODO: reduce complexity.
func (cs *ControllerServer) CreateVolumeGroupSnapshot(
ctx context.Context,
req *csi.CreateVolumeGroupSnapshotRequest,
Expand Down Expand Up @@ -130,6 +132,27 @@ func (cs *ControllerServer) CreateVolumeGroupSnapshot(
"failed to get existing one with name %q: %v", vgsName, err)
}

creds, err := util.NewUserCredentials(req.GetSecrets())
if err != nil {
return nil, status.Error(codes.Internal, err.Error())
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

defer cr.DeleteCredentials() is missing here?

defer creds.DeleteCredentials()

errList := make([]error, 0)
for _, volume := range volumes {
err = volume.PrepareVolumeForSnapshot(ctx, creds)
if err != nil {
errList = append(errList, err)
}
}
if len(errList) > 0 {
// FIXME: we should probably choose a error code that has more priority.
return nil, status.Errorf(
Madhu-1 marked this conversation as resolved.
Show resolved Hide resolved
status.Code(errList[0]),
"failed to prepare volumes for snapshot: %v",
errList)
}

// create a temporary VolumeGroup with a different name
vg, err = mgr.CreateVolumeGroup(ctx, vgName)
if err != nil {
Expand Down
25 changes: 25 additions & 0 deletions internal/rbd/rbd_util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2277,3 +2277,28 @@ func (ri *rbdImage) GetClusterID(ctx context.Context) (string, error) {

return ri.ClusterID, nil
}

func (rv *rbdVolume) PrepareVolumeForSnapshot(ctx context.Context, cr *util.Credentials) error {
Madhu-1 marked this conversation as resolved.
Show resolved Hide resolved
hardLimit := rbdHardMaxCloneDepth
softLimit := rbdSoftMaxCloneDepth
err := flattenTemporaryClonedImages(ctx, rv, cr)
if err != nil {
return err
}

// choosing 2, since snapshot adds one depth and we'll be flattening the parent.
const depthToAvoidFlatten = 2
if rbdHardMaxCloneDepth > depthToAvoidFlatten {
hardLimit = rbdHardMaxCloneDepth - depthToAvoidFlatten
}
if rbdSoftMaxCloneDepth > depthToAvoidFlatten {
softLimit = rbdSoftMaxCloneDepth - depthToAvoidFlatten
}

err = rv.flattenParent(ctx, hardLimit, softLimit)
if err != nil {
return getGRPCErrorForCreateVolume(err)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why its returning GRPC error that to for CreateVolume? ideally internal functions are expected to return plain error message and the caller should pick the right GRPC message.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ceph-csi/internal/rbd/controllerserver.go

Line 301 in a32ba13

func getGRPCErrorForCreateVolume(err error) error {

It's used in the same form at multiple places.
We have other internal functions that add grpc error codes itself too

ceph-csi/internal/rbd/controllerserver.go

Line 575 in a32ba13

func flattenTemporaryClonedImages(ctx context.Context, rbdVol *rbdVolume, cr *util.Credentials) error {

Let's handle it separately

}

return nil
}
4 changes: 4 additions & 0 deletions internal/rbd/types/volume.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,10 @@ type Volume interface {
// if the parent image is in trash, it returns an error.
// if the parent image exists and is not enabled for mirroring, it returns an error.
HandleParentImageExistence(ctx context.Context, flattenMode FlattenMode) error
// PrepareVolumeForSnapshot prepares the volume for snapshot by
// checking snapshots limit and clone depth limit and flatten it
// if required.
PrepareVolumeForSnapshot(ctx context.Context, cr *util.Credentials) error

// ToMirror converts the Volume to a Mirror.
ToMirror() (Mirror, error)
Expand Down