fix(controller): prevent volume/snapshot deletion if clone exists #606
Conversation
Signed-off-by: sinhaashish <[email protected]>
|
Even though i have raised the PR as per the request here but i find that this feature is not supported by csi . This PR fails on CI as the csi tests expects that the snapshot should be deleted even if the clone exists . Refer here This is what is reflected in the ci output. IMHO we should close this request as its not supported by csi. Your inputs please @avishnu @tiagolobocastro @Abhinandan-Purkait |
sinhaashish
requested review from
tiagolobocastro,
Abhinandan-Purkait and
avishnu
There was a problem hiding this comment.
Even though i have raised the PR as per the request here
but i find that this feature is not supported by csi . In our csi test we run the csi test from this repo https://github1s.com/kubernetes-csi/csi-test.
This PR fails on CI as the csi tests expects that the snapshot should be deleted even if the clone exists . Refer here
This is what is reflected in the ci output.
IMHO we should close this request as its not supported by csi.
Your inputs please @avishnu @tiagolobocastro @Abhinandan-Purkait
A Quick Look on the csi spec seems to show returning failed precondition on deletion is allowed - but would be better to check what K8s expects. IIRC if we do that we'll be blasted with delete retries? CC @Abhinandan-Purkait
That being said, we may be able to decouple the K8s resources from the "backend" resources. IIRC we do that on mayastor?
But I'd like some more context here, why do we want to add this validation. Does the ZFS clone break if we delete the volume/snapshot?
| // snapshot delete is not supported if clones exist for this snapshot | ||
| if len(cloneList.Items) != 0 { | ||
| return status.Errorf( | ||
| codes.Internal, |
There was a problem hiding this comment.
Should be FAILED_PRECONDITION
There was a problem hiding this comment.
I think we should first evaluate the issue with what happens if we delete the volume/snapshot.
There was a problem hiding this comment.
verified that if we delete the volume i.e pv and pvc the underneath zfs snapshot get deleted. Even though the zfs snapshot cr is available. To rectify this i have raised the PR #607
Before deleting the pvc
root@node-0-290533:~# zfs list -t all
NAME USED AVAIL REFER MOUNTPOINT
zfspv-pool 186K 9.20G 24K /zfspv-pool
zfspv-pool/pvc-dd7c6d67-8451-43c3-bc56-993e9f41b1b3 24.5K 4.00G 24.5K legacy
zfspv-pool/pvc-dd7c6d67-8451-43c3-bc56-993e9f41b1b3@snapshot-cc55c781-c4d5-4f7b-ac84-1b9582b469c6 0B - 24.5K -
zfspv-pool/pvc-dd7c6d67-8451-43c3-bc56-993e9f41b1b3@snapshot-19b7e7d1-f81f-435f-ae88-e73aa828ced8 0B - 24.5K -
After deleting the pvc
root@node-0-290533:~# zfs list -t all
NAME USED AVAIL REFER MOUNTPOINT
zfspv-pool 141K 9.20G 24K /zfspv-pool
Pull Request template
Please, go through these steps before you submit a PR.
Why is this PR required? What issue does it fix?:
prevent volume/snapshot deletion if clone exists
What this PR does?:
Checks if a clone exists before deleting the snapshot
Does this PR require any upgrade changes?:
If the changes in this PR are manually verified, list down the scenarios covered::
Any additional information for your reviewer? :
Mention if this PR is part of any design or a continuation of previous PRs
Checklist:
<type>(<scope>): <subject>PLEASE REMOVE BELOW INFORMATION BEFORE SUBMITTING
The PR title message must follow convention:
<type>(<scope>): <subject>.Where:
typeis defining if release will be triggering after merging submitted changes, details in CONTRIBUTING.md.Most common types are:
feat- for new features, not a new feature for build scriptfix- for bug fixes or improvements, not a fix for build scriptchore- changes not related to production codedocs- changes related to documentationstyle- formatting, missing semi colons, linting fix etc; no significant production code changestest- adding missing tests, refactoring tests; no production code changerefactor- refactoring production code, eg. renaming a variable or function name, there should not be any significant production code changesscopeis a single word that best describes where the changes fit.Most common scopes are like:
localpv,jiva,cstor)provisioning,backup,restore,exporter)api,webhook,cast,upgrade)tests,bdd)version,build,log,travis)subjectis a single line brief description of the changes made in the pull request.