Cleanup RADOS namespace with forced deletion annotation #14052

sp98 · 2024-04-10T11:47:15Z

If RadosNamespace CR is deleted with rook.io/force-deletion annotation then start a clean up job that will delete the images and snapshots on the radosnamespace.
Steps followed for cleanup:

List images in pool
List snapshots for each image.
Delete snapshots for each image.
Move Image to trash
Start a task to delete the trash.

Operator logs after deleting radosNamespace CR with annotation
operator-logs.txt

Checklist:

Commit Message Formatting: Commit titles and messages follow guidelines in the developer guide.
Reviewed the developer guide on Submitting a Pull Request
Pending release notes updated with breaking and/or notable changes for the next minor release.
Documentation has been updated, if necessary.
Unit tests have been added, if necessary.
Integration tests have been added, if necessary.

travisn

some small suggestions

travisn · 2024-04-10T13:49:42Z

pkg/daemon/ceph/cleanup/radosnamespace.go

+			logger.Error(retErr)
+		}
+
+		// TODO: rather than deleting add tasks to trash the image?


What does this mean to add tasks to trash it? Since force deleting, seems like we should directly delete all the images as you have here.

Removed the TODO for now.
There was one suggestion by Madhu to use following command instead of deleting the images

rbd trash mv <RBD image image> ceph rbd task trash remove <RBD image>

travisn · 2024-04-10T13:52:38Z

pkg/daemon/ceph/client/image.go

@@ -41,8 +41,11 @@ type CephBlockImage struct {
 	InfoName string `json:"name"`
 }

-func ListImages(context *clusterd.Context, clusterInfo *ClusterInfo, poolName string) ([]CephBlockImage, error) {
+func ListImages(context *clusterd.Context, clusterInfo *ClusterInfo, poolName, namespace string) ([]CephBlockImage, error) {


For code readability, how about creating two methods? The latter will have exactly your implementation, and the first one just calls the second one.

Suggested change

func ListImages(context *clusterd.Context, clusterInfo *ClusterInfo, poolName, namespace string) ([]CephBlockImage, error) {

func ListImagesInPool(context *clusterd.Context, clusterInfo *ClusterInfo, poolName string) ([]CephBlockImage, error) {

return ListImagesInRadosNamespace(context, clusterInfo, poolName, "")

}

func ListImagesInRadosNamespace(context *clusterd.Context, clusterInfo *ClusterInfo, poolName, namespace string) ([]CephBlockImage, error) {

travisn · 2024-04-10T13:55:10Z

pkg/daemon/ceph/client/image.go

@@ -116,10 +133,13 @@ func CreateImage(context *clusterd.Context, clusterInfo *ClusterInfo, name, pool
 	return &CephBlockImage{Name: name, Size: newSizeBytes}, nil
 }

-func DeleteImage(context *clusterd.Context, clusterInfo *ClusterInfo, name, poolName string) error {
+func DeleteImage(context *clusterd.Context, clusterInfo *ClusterInfo, name, poolName, namespace string) error {


Similarly, I'd suggest DeleteImageInPool() and DeleteImageInRadosNamespace()

travisn · 2024-04-10T16:27:30Z

Documentation/Storage-Configuration/ceph-teardown.md

+| Custom Resource                      | Ceph Resources to be cleaned up |
+| --------                             | ------- |
+| CephFilesystemSubVolumeGroup         | CSI stored RADOS OMAP details for pvc/volumesnapshots, subvolume snapshots, subvolume clones, subvolumes |
+| CephBlockPoolRadosNamespace          | Images and snapshots in the rados namespace|


Madhu-1 · 2024-04-10T17:57:27Z

pkg/daemon/ceph/cleanup/radosnamespace.go

+)
+
+func RadosNamespaceCleanup(context *clusterd.Context, clusterInfo *client.ClusterInfo, poolName, radosNamespace string) error {
+	logger.Infof("starting clean up of CephBlockPoolRadosNamespace %q resources in blockpool %q", radosNamespace, poolName)


update blockpool to cephblockpool ro match the name in all other places?

Madhu-1 · 2024-04-10T18:00:22Z

pkg/daemon/ceph/client/image.go

+	return ListImagesInRadosNamespace(context, clusterInfo, poolName, "")
+}
+
+func ListImagesInRadosNamespace(context *clusterd.Context, clusterInfo *ClusterInfo, poolName, namespace string) ([]CephBlockImage, error) {
 	args := []string{"ls", "-l", poolName}


not related to this PR we could have used rbd ls -p=replicapool instead of -l

Yep. Just used the the existing method we had for list images.

Madhu-1 · 2024-04-10T18:16:41Z

pkg/daemon/ceph/cleanup/radosnamespace.go

+		err = cephclient.DeleteImageInRadosNamespace(context, clusterInfo, image.Name, poolName, radosNamespace)
+		if err != nil {
+			retErr = errors.Wrapf(err, "failed to delete image %q in pool %q.", image.Name, poolName)
+			logger.Error(retErr)
+		}


AFAIK rhis doesn't work when we have clones created from the snapshot, when we try to delete the clones on the parent image the parent will fail saying there are clones created from the parent image

sh-4.4$ rbd clone --rbd-default-clone-format 2 replicapool/test@test replicapool/test2 sh-4.4$ sh-4.4$ rbd ls replicapool test test2 sh-4.4$ rbd snap purge test rbd: error opening default pool 'rbd' Ensure that the default pool has been created or specify an alternate pool name. sh-4.4$ rbd snap purge replicapool/test Removing all snapshots: 100% complete...done. sh-4.4$ rbd rm replicapool/test 2024-04-10T18:04:15.724+0000 7f406b33b580 -1 librbd::api::Image: remove: image has snapshots - not removing Removing image: 0% complete...failed. rbd: image has snapshots with linked clones - these must be deleted or flattened before the image can be removed. sh-4.4$ sh-4.4$ ceph rbd task add trash remove replicapool/911d3a445d20 {"sequence": 1, "id": "2fc6afa4-1239-45f9-98e6-2a4298d8839c", "message": "Removing image replicapool/911d3a445d20 from trash", "refs": {"action": "trash remove", "pool_name": "replicapool", "pool_namespace": "", "image_id": "911d3a445d20"}} sh-4.4$ rbd trash mv replicapool/test2 sh-4.4$ ceph rbd task add trash remove replicapool/ee91d79d6186 {"sequence": 2, "id": "fed70961-a430-4b6d-8594-f8e18f824905", "message": "Removing image replicapool/ee91d79d6186 from trash", "refs": {"action": "trash remove", "pool_name": "replicapool", "pool_namespace": "", "image_id": "ee91d79d6186"}} sh-4.4$ ceph rbd task add trash remove replicapool/ee91d79d6186 {"sequence": 2, "id": "fed70961-a430-4b6d-8594-f8e18f824905", "message": "Removing image replicapool/ee91d79d6186 from trash", "refs": {"action": "trash remove", "pool_name": "replicapool", "pool_namespace": "", "image_id": "ee91d79d6186"}}

please test it out and see all the images are deleted from pool/namespace and also from trash.

travisn

LGTM with the latest changes

Madhu-1

some nits, LGTM

Madhu-1 · 2024-04-12T15:49:09Z

pkg/daemon/ceph/cleanup/radosnamespace.go

+		err = cephclient.MoveImageToTrashInRadosNamespace(context, clusterInfo, poolName, image.Name, radosNamespace)
+		if err != nil {
+			retErr = errors.Wrapf(err, "failed to delete image %q in cephblockpool %q.", image.Name, poolName)
+			logger.Error(retErr)
+		}
+		err = cephclient.DeleteImageFromTrashInRadosNamespace(context, clusterInfo, poolName, image.ID, radosNamespace)
+		if err != nil {
+			retErr = errors.Wrapf(err, "failed to delete image %q in cephblockpool %q.", image.Name, poolName)
+			logger.Error(retErr)
+		}


above we already have a for loop for image and we can move this check to above for loop.

Madhu-1 · 2024-04-12T15:50:07Z

pkg/daemon/ceph/cleanup/radosnamespace.go

+	for _, image := range images {
+		err = cephclient.MoveImageToTrashInRadosNamespace(context, clusterInfo, poolName, image.Name, radosNamespace)
+		if err != nil {
+			retErr = errors.Wrapf(err, "failed to delete image %q in cephblockpool %q.", image.Name, poolName)


failed to move image to trash add radosnamespace name to the error message (please take care of it in all the places

Madhu-1 · 2024-04-12T15:50:21Z

pkg/daemon/ceph/cleanup/radosnamespace.go

+		}
+		err = cephclient.DeleteImageFromTrashInRadosNamespace(context, clusterInfo, poolName, image.ID, radosNamespace)
+		if err != nil {
+			retErr = errors.Wrapf(err, "failed to delete image %q in cephblockpool %q.", image.Name, poolName)


failed to add task to remove image from trash add radosnamespace name to the error message

Madhu-1 · 2024-04-12T15:51:49Z

pkg/daemon/ceph/client/image.go

+func ListImagesInPool(context *clusterd.Context, clusterInfo *ClusterInfo, poolName string) ([]CephBlockImage, error) {
+	return ListImagesInRadosNamespace(context, clusterInfo, poolName, "")
+}
+
+func ListImagesInRadosNamespace(context *clusterd.Context, clusterInfo *ClusterInfo, poolName, namespace string) ([]CephBlockImage, error) {


Add comments for these exported functions?

Madhu-1 · 2024-04-12T15:52:24Z

pkg/daemon/ceph/client/image.go

+	cmd.JsonOutput = true
+	buf, err := cmd.Run()
+	if err != nil {
+		return snapshots, errors.Wrapf(err, "failed to list snapshots of image %q in cephblockpool %q", imageName, poolName)


add radosnamespace name to the error message?

Madhu-1 · 2024-04-12T15:52:57Z

pkg/daemon/ceph/client/image.go

+	cmd := NewRBDCommand(context, clusterInfo, args)
+	_, err := cmd.Run()
+	if err != nil {
+		return errors.Wrapf(err, "failed to delete snapshot %q of image %q in cephblockpool %q", snapshot, imageName, poolName)


add radosnamespace name to the error message?

Madhu-1 · 2024-04-12T15:54:43Z

pkg/daemon/ceph/client/image.go

+}
+
+// DeleteSnapshotInRadosNamespace deletes a image snapshot created in block pool in a given rados namespace
+func DeleteSnapshotInRadosNamespace(context *clusterd.Context, clusterInfo *ClusterInfo, poolName, imageName, snapshot, namespace string) error {


can we remove below if check as we are deleting snapshots in a rados namespace, same can be done for all other functions as well.

if namespace != "" { args = append(args, "--namespace", namespace) }

applicable for all other functions

Madhu-1 · 2024-04-12T15:56:36Z

pkg/daemon/ceph/client/image.go

@@ -196,3 +273,10 @@ func UnMapImage(context *clusterd.Context, clusterInfo *ClusterInfo, imageName,
 func getImageSpec(name, poolName string) string {
 	return fmt.Sprintf("%s/%s", poolName, name)
 }
+
+func getImageSpecInRadosNamespace(name, poolName, namespace string) string {


name, poolName, namespace string to poolName, namespace,image string lets keep it in order

Madhu-1 · 2024-04-12T15:56:47Z

pkg/daemon/ceph/client/image.go

+func getImageSpecInRadosNamespace(name, poolName, namespace string) string {
+	return fmt.Sprintf("%s/%s/%s", poolName, namespace, name)
+}
+func getImageSnapshotSpec(name, poolName, snapshot string) string {


same here as well

Clean up pool images and snapshots in the radosnamespace Signed-off-by: sp98 <sapillai@redhat.com>

Feedback addressed, or else let's follow up in a separate PR

Cleanup RADOS namespace with forced deletion annotation (backport #14052)

sp98 force-pushed the cleanup-radosnamespace branch 3 times, most recently from d6f684b to 92c96c1 Compare April 10, 2024 13:38

travisn requested changes Apr 10, 2024

View reviewed changes

sp98 force-pushed the cleanup-radosnamespace branch from 92c96c1 to 663fc94 Compare April 10, 2024 14:50

sp98 requested a review from travisn April 10, 2024 14:53

sp98 force-pushed the cleanup-radosnamespace branch 2 times, most recently from 806567a to 6581c50 Compare April 10, 2024 15:05

sp98 requested a review from Madhu-1 April 10, 2024 15:05

sp98 force-pushed the cleanup-radosnamespace branch 2 times, most recently from 2a33156 to 499bee1 Compare April 10, 2024 16:10

travisn approved these changes Apr 10, 2024

View reviewed changes

sp98 force-pushed the cleanup-radosnamespace branch from 499bee1 to 0b8517a Compare April 10, 2024 16:32

travisn changed the title ~~Cleanup radosnamespace~~ Cleanup RADOS namespace with forced deletion annotation Apr 10, 2024

Madhu-1 previously requested changes Apr 10, 2024

View reviewed changes

sp98 force-pushed the cleanup-radosnamespace branch 2 times, most recently from 2f8ede6 to 57b46b2 Compare April 12, 2024 14:23

sp98 requested a review from Madhu-1 April 12, 2024 14:24

travisn reviewed Apr 12, 2024

View reviewed changes

Madhu-1 reviewed Apr 12, 2024

View reviewed changes

core: cephblockpoolRadosNamespace cleanup

f6b1449

Clean up pool images and snapshots in the radosnamespace Signed-off-by: sp98 <sapillai@redhat.com>

sp98 force-pushed the cleanup-radosnamespace branch from 57b46b2 to f6b1449 Compare April 12, 2024 16:19

sp98 requested a review from Madhu-1 April 12, 2024 16:36

travisn added the backport-release-1.14 label Apr 12, 2024

travisn merged commit 499a09e into rook:master Apr 12, 2024
53 checks passed

mergify bot mentioned this pull request Apr 12, 2024

Cleanup RADOS namespace with forced deletion annotation (backport #14052) #14068

Merged

6 tasks

travisn added a commit that referenced this pull request Apr 12, 2024

Merge pull request #14068 from rook/mergify/bp/release-1.14/pr-14052

fb661a5

Cleanup RADOS namespace with forced deletion annotation (backport #14052)

travisn mentioned this pull request Apr 16, 2024

Allow force deletion of resources that have specific cleanup annotation #13869

Closed

sp98 mentioned this pull request Apr 24, 2024

Unit test are failing with cleanup job #14054

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup RADOS namespace with forced deletion annotation #14052

Cleanup RADOS namespace with forced deletion annotation #14052

sp98 commented Apr 10, 2024 •

edited

travisn left a comment

travisn Apr 10, 2024

sp98 Apr 10, 2024

travisn Apr 10, 2024

travisn Apr 10, 2024

travisn Apr 10, 2024

Madhu-1 Apr 10, 2024

Madhu-1 Apr 10, 2024

sp98 Apr 11, 2024

Madhu-1 Apr 10, 2024

travisn left a comment

Madhu-1 left a comment

Madhu-1 Apr 12, 2024

Madhu-1 Apr 12, 2024

Madhu-1 Apr 12, 2024

Madhu-1 Apr 12, 2024

Madhu-1 Apr 12, 2024

Madhu-1 Apr 12, 2024

Madhu-1 Apr 12, 2024

Madhu-1 Apr 12, 2024

Madhu-1 Apr 12, 2024

Madhu-1 Apr 12, 2024

	\| CephBlockPoolRadosNamespace \| Images and snapshots in the rados namespace\|
	\| CephBlockPoolRadosNamespace \| Images and snapshots in the RADOS namespace\|

Cleanup RADOS namespace with forced deletion annotation #14052

Cleanup RADOS namespace with forced deletion annotation #14052

Conversation

sp98 commented Apr 10, 2024 • edited

travisn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

travisn left a comment

Choose a reason for hiding this comment

Madhu-1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sp98 commented Apr 10, 2024 •

edited