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.

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

fix(controller): prevent volume deletion if snapshot exists #350

Open
wants to merge 11 commits into
base: develop
Choose a base branch
from

Conversation

akhilerm
Copy link
Member

@akhilerm akhilerm commented Jun 22, 2021

Why is this PR required? What issue does it fix?:

  • Volume deletion should be stopped if a clone/snapshot exists
  • Snapshot deletion should be stopped if a clone exists

What this PR does?:

  • add a check in DeleteVolume() call in controller side to validate that no snapshot/clone exists before proceeding to volume deletion
  • add a check in DeleteSnapshot() call in controller side to validate that no clone exists before proceeding to snapshot deletion

Does this PR require any upgrade changes?:
No

If the changes in this PR are manually verified, list down the scenarios covered::

  1. Create a PVC, csi-zfspv
  2. Create a snapshot zfs-snap from the above pvc
  3. Create a clone zfs-snap-clone from the above snapshot
  4. Create a clone zfs-vol-clone from the pvc csi-zfspv
  5. Delete the volume csi-zfspv. Deletion fails with the error
E0624 07:47:30.582195       1 grpc.go:79] GRPC error: rpc error: code = Internal desc = failed to handle delete volume request for {pvc-53e5ce41-5755-4833-b6c4-bb48188b9293} with 1 snapshots
  1. Delete the snapshot zfs-snap. The deletion fails with the error in controller
E0624 08:20:14.395489       1 grpc.go:79] GRPC error: rpc error: code = Internal desc = failed to handle delete snapshot request for {pvc-53e5ce41-5755-4833-b6c4-bb48188b9293@snapshot-2ca86a72-6153-423a-9604-78594aa03517} with 1 clones
  1. Delete the clone zfs-snap-clone and the snapshot will be deleted.
  2. But now the delete volume csi-zfspv will be failing with error
E0624 08:21:16.433858      1 grpc.go:79] GRPC error: rpc error: code = Internal desc = failed to handle delete volume request for {pvc-53e5ce41-5755-4833-b6c4-bb48188b9293} with 1 clones

since a clone also exists for this volume
8. Delete the clone zfs-vol-clone and the volume gets deleted

Any additional information for your reviewer? :
NOTE
Currently to find the clones of a snapshot, all volumes are listed and then searched for a matching snapname. This process can be made easier if a label is added to the clone volumes created from a snapshot

Checklist:

@akhilerm akhilerm added this to Pre-commits and Designs - Due: June 30 2021 in 2.11 Release Tracker - Due July 15th. Jun 22, 2021
@pawanpraka1 pawanpraka1 added this to the v1.9.0 milestone Jun 22, 2021
@pawanpraka1 pawanpraka1 added the enhancement Add new functionality to existing feature label Jun 22, 2021
pkg/zfs/volume.go Outdated Show resolved Hide resolved
Signed-off-by: Akhil Mohan <[email protected]>
Signed-off-by: Akhil Mohan <[email protected]>
@pawanpraka1
Copy link
Contributor

@akhilerm csi-sanity is failing. we need to have waitforSnapDestroy function similar to volume destroy

func waitForVolDestroy(volname string) error {
. We should wait in the DeleteSnapshot call

	if _, ok := parameters["wait"]; ok {
		if err := waitForSnapDestroy(snapName); err != nil {
			return nil, err
		}
	}

@akhilerm
Copy link
Member Author

@pawanpraka1 But parameters are not present in the DeleteSnapshot() call. How should I get the parameters?

@akhilerm
Copy link
Member Author

@pawanpraka1 Updated with the changes for waitForSnapDestroy

Signed-off-by: Akhil Mohan <[email protected]>
Signed-off-by: Akhil Mohan <[email protected]>
Signed-off-by: Akhil Mohan <[email protected]>
Signed-off-by: Akhil Mohan <[email protected]>
@akhilerm akhilerm added the pr/hold-merge hold the merge. label Jun 29, 2021
@prateekpandey14 prateekpandey14 moved this from Pre-commits and Designs - Due: June 30 2021 to Pushed to Next release due to WIP in 2.11 Release Tracker - Due July 15th. Jul 1, 2021
@pawanpraka1 pawanpraka1 modified the milestones: v1.9.0, v1.10.0 Jul 26, 2021
@niladrih niladrih self-assigned this Sep 20, 2023
@plevart
Copy link

plevart commented Sep 25, 2023

May I suggest there to be an exception to the constraint that a volume can't be deleted if there is a snapshot created for that volume. Let me explain on the ZFS level...

Suppose we have a ZFS volume vol1 an create 3 snapshots for it: vol1@snap1, vol1@snap2, vol1@snap3.
Suppose we take the oldest of those snapshots: vol1@snap3 and create a volume clone from it: vol2. Now we have these dependencies:

vol1 <- vol1@snap1
vol1 <- vol1@snap2
vol1 <- vol1@snap3 <- vol2

ZFS can turn these dependencise upside-down with "zfs promote vol2" command (https://openzfs.github.io/openzfs-docs/man/master/8/zfs-promote.8.html) and create this:

vol2 <- vol2@snap1
vol2 <- vol2@snap2
vol2 <- vol2@snap3 <- vol1

Now after that promotion of vol2, vol1 may be deleted as nothing is dependent on it.

So what I suggest is the following exception to the constraint you are adding with this pull request:

  • if the volume being deleted has several snapshots and the oldest of them has a single cloned volume created, allow deletion of the volume but 1st promote the clone.

Perhaps even a better alternative would be to have a special CRD that would allow execution of "zfs promote" action. Or maybe an attribute on a PVC that would trigger the "zfs promote" action... In that case this pull request can do one thing only.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Add new functionality to existing feature pr/hold-merge hold the merge.
Projects
2.11 Release Tracker - Due July 15th.
  
Pushed to Next release due to WIP
Development

Successfully merging this pull request may close these issues.

do not attempt to delete the ZFSVolume CR if there is a snapshot holding it.
4 participants