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

Jump to bottom

Cleanup snapshots before removing rbd image #3456

Closed
wants to merge 1 commit into from
Closed

Cleanup snapshots before removing rbd image #3456

wants to merge 1 commit into from

Conversation

ruslanloman
Copy link

Hello!

Describe what this PR does

Ceph csi couldn't remove pvc with snapshots that was created directly in
ceph using rbd or other clients. As a result, the rbd image remains permanently in the trash.
As solution we can cleanup snapshots before removing rbd image.

Related issues

Fixes: #3416

@ruslanloman
Cleanup snapshots before removing rbd image
df98fe7 
  Ceph csi couldn't remove pvc with snapshots that was created directly in
  ceph using rbd or other clients. As a result, the rbd image remains
  permanently in the trash.

  Fixes: ceph#3416

Signed-off-by: ruslanloman <ruslanloman@gmail.com>
@nixpanic nixpanic added the component/rbd Issues related to RBD label Oct 19, 2022
nixpanic
nixpanic reviewed Oct 19, 2022
View reviewed changes
Copy link
Member

@nixpanic nixpanic left a comment

Choose a reason for hiding this comment

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

I am not sure this is what all users want. Some users might still want to use the snapshot, even when someone/thing else deleted the volume. Having the snapshot available, and the parent rbd image in the trash makes it possible to undo unintentional deletions (for example).

internal/rbd/rbd_util.go

snaps, err := ri.listSnapshots()
if err != nil {
log.DebugLog(ctx, "rbd: failed to get list of snapshots %v", err)
Copy link
Member

Choose a reason for hiding this comment

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

return the error?

@Madhu-1
Copy link
Collaborator

Madhu-1 commented Oct 19, 2022

ceph using rbd or other clients. As a result, the rbd image remains permanently in the trash.

why cephcsi need to handle the user created snapshots? if user is creating the snapshot its user responsibility to delete it also #3416, having this feature can also lead to similar changes for others also like Replication, cephfs subvolume deletion etc (just an example).

Rakshith-R
Rakshith-R requested changes Oct 19, 2022
View reviewed changes
Copy link
Contributor

@Rakshith-R Rakshith-R left a comment

Choose a reason for hiding this comment

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

We still had discussions going on the issue thread?

why does cephcsi need to handle the user created snapshots? if user is creating the snapshot its user responsibility to delete it also

+1
We cannot guarantee such behavior, when another external entity acts on cephcsi managed images.

We cannot purge snapshots of a pvc having k8s snapshot and clone. These preserve the link between parent and child

@ruslanloman
Copy link
Author

ruslanloman commented Oct 19, 2022 •
edited
Loading

Hi, thank you for your comments!

With this PR i'm trying to find some solution for this issue. Because right now cephcsi couldn't remove the ceph rdb image with snapshots and we can consider it as a bug.
There are 3 solutions that i have in my mind.

  1. Purge snapshots before image removing (this approach is in PR)
  2. In case we are trying to remove pvc with rbd snapshots then we just return an error and let the client clean it up manually.
  3. Same as item 1 but we can add a config option to allow snapshot cleanup.

@nixpanic
Copy link
Member

With this PR i'm trying to find some solution for this issue. Because right now cephcsi couldn't remove the ceph rdb image with snapshots and we can consider it as a bug. There are 3 solutions that i have in my mind.

I don't think this is a bug, I would probably call it intentional behaviour. It might be unexpected for some users, but Ceph-CSI should not, and does not delete volumes or snapshots that were created by others.

1. Purge snapshots before image removing (this approach is  in PR)

2. In case we are trying to remove pvc with rbd snapshots then we just return an error and let the client clean it up manually.

3. Same as item 1 but we can add a config option to allow snapshot cleanup.

I think option 2 is the most reasonable. Ceph-CSI should not (never) be able to get blamed for data-loss, and wrongly configuring a "force snapshot removal" option invites that (even if it is an admin/user error).

Option 2 makes sure that the user/admin notices their unbalanced usage of Ceph-CSI and other tools. The user/admin should then decide to correct the cooperation of the tools/procedures.

@ruslanloman
Copy link
Author

Hi @nixpanic I got your point. I'm going to close this PR as invalid.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nixpanic nixpanic nixpanic left review comments

@Rakshith-R Rakshith-R Rakshith-R requested changes

Assignees
No one assigned
Labels
component/rbd Issues related to RBD
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Delete snapshots before deleting RBD image
4 participants
@ruslanloman @Madhu-1 @nixpanic @Rakshith-R