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
|
When the snapshot is deleted for which the clone exist . The VS and zfssnap gets deleted but the underneath snapshot and volume are intact . When the last clone is deleted then the volume and snapshot at zfs level gets deleted , thus cleaning up all the related resource and leaving no orphaned volumes. Delete the pvc and then the vs Once the clone is deleted , every thing gets deleted at zfs level So its works as expected. |
|
Closing the PR as this feature in not supported by csi |
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## develop #606 +/- ##
==========================================
- Coverage 95.99% 0 -96.00%
==========================================
Files 1 0 -1
Lines 574 0 -574
==========================================
- Hits 551 0 -551
+ Misses 19 0 -19
+ Partials 4 0 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.