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

feat(clone): add support for creating the Clone from volume as datasource #234

Merged
merged 2 commits into from
Nov 11, 2020
Merged

feat(clone): add support for creating the Clone from volume as datasource #234

merged 2 commits into from
Nov 11, 2020

Conversation

pawanpraka1
Copy link
Contributor

@pawanpraka1 pawanpraka1 commented Nov 10, 2020 •
edited

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

add support for creating the Clone from volume as datasource

What this PR does?:

We can now create the Clone from pvc directly

kind: PersistentVolumeClaim
apiVersion: v1
metadata:
  name: pvc-clone
spec:
  storageClassName: openebs-clone
  dataSource:
    name: pvc-vol
    kind: PersistentVolumeClaim
  accessModes:
    - ReadWriteOnce
  resources:
    requests:
      storage: 4Gi

The ZFS_LocalPV driver will create one internal snapshot of the name
same as the new volume name and will create a clone out of it. Also,
while destroying the volume the driver will take care of deleting
the created snapshot for the clone.

Checklist:

  • Fixes #
  • PR Title follows the convention of <type>(<scope>): <subject>
  • Has the change log section been updated?
  • Commit has unit tests
  • Commit has integration tests
  • (Optional) Are upgrade changes included in this PR? If not, mention the issue/PR to track:
  • (Optional) If documentation changes are required, which issue on https://github.com/openebs/openebs-docs is used to track them:

@pawanpraka1 pawanpraka1 added this to the v1.1.0 milestone Nov 10, 2020
@codecov-io
Copy link

codecov-io commented Nov 10, 2020 •
edited

Codecov Report

Merging #234 (b9981cf) into master (64bc7cb) will decrease coverage by 0.23%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #234      +/-   ##
=========================================
- Coverage    8.50%   8.27%   -0.24%     
=========================================
  Files          20      20              
  Lines        1046    1076      +30     
=========================================
  Hits           89      89              
- Misses        956     986      +30     
  Partials        1       1
Impacted Files Coverage Δ
pkg/driver/controller.go 0.83% <0.00%> (-0.08%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 64bc7cb...3916521. Read the comment docs.

@pawanpraka1 pawanpraka1 removed the pr/hold-review hold the review. label Nov 10, 2020
@pawanpraka1 pawanpraka1 added this to RC2 - Due: Nov 10 2020 in 2.3 Release Tracker - Due Nov 15th. Nov 10, 2020
@pawanpraka1 pawanpraka1 added this to In progress in ZFS Local PV Nov 10, 2020
@pawanpraka1
feat(clone): add support for creating the Clone from volume as dataso…
767f912 
…urce

We can now create the Clone from pvc directly

```
kind: PersistentVolumeClaim
apiVersion: v1
metadata:
  name: pvc-clone
spec:
  storageClassName: openebs-snap
  dataSource:
    name: pvc-snap
    kind: PersistentVolumeClaim
  accessModes:
    - ReadWriteOnce
  resources:
    requests:
      storage: 4Gi
```
The ZFS_LocalPV driver will create one internal snapshot of the name
same as the new volume name and will create a clone out of it. Also,
while destroying the volume the driver will take care of deleting
the created snapshot for the clone.

Signed-off-by: Pawan <pawan@mayadata.io>
@kmova
Copy link
Member

kmova commented Nov 10, 2020

@pawanpraka1 - what happens in the following case:

  • create pvc1
  • create pvc2 ( clone of pvc1)
  • delete pvc1

Will pvc2 (and its PV) still exist?

@pawanpraka1
Copy link
Contributor Author

Yes @kmova, pvc2 and it's pv will be there. Also zfs volume created for pvc1 will also be there and it will be cleaned up automatically when we delete pvc2.

kmova
kmova reviewed Nov 11, 2020
View reviewed changes
pkg/driver/controller.go Outdated

selected := vol.Spec.OwnerNodeID

labels := map[string]string{zfs.ZFSVolKey: vol.Name}
Copy link
Member

Choose a reason for hiding this comment

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

Can we make this a more explicit key - like "source-zfs-volume"?

kmova
kmova reviewed Nov 11, 2020
View reviewed changes
pkg/zfs/zfs_util.go

klog.Infof("destroying snapshot %s@%s for the clone %s", srcVol, snap.Name, volume)

err := DestroySnapshot(snap)
Copy link
Member

Choose a reason for hiding this comment

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

Here is where the source volume needs to be cleaned up, in case this was the last clone/snapshot being deleted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, not for the last clone/snapshot being deleted, every clone will have a corresponsing snapshot. So when we are deleting the clone volume, we should delete the corresponding snapshot here.

@pawanpraka1
review comment: change the key to ZFSSrcVolKey
3916521 
Signed-off-by: Pawan <pawan@mayadata.io>
kmova
kmova approved these changes Nov 11, 2020
View reviewed changes
Copy link
Member

@kmova kmova left a comment

Choose a reason for hiding this comment

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

this PR will be followed up documentation updates and better log management in case of error cases.

@kmova kmova merged commit fb6f100 into openebs:master Nov 11, 2020
ZFS Local PV automation moved this from In progress to Done Nov 11, 2020
2.3 Release Tracker - Due Nov 15th. automation moved this from RC2 - Due: Nov 10 2020 to Done Nov 11, 2020
@pawanpraka1
Copy link
Contributor Author

@kmova here is the issue #236

@pawanpraka1 pawanpraka1 deleted the vclone branch November 11, 2020 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kmova kmova kmova approved these changes

Assignees
No one assigned
Labels
None yet
Projects
ZFS Local PV
  
Done
2.3 Release Tracker - Due Nov 15th.
  
Done
Milestone
v1.1.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@pawanpraka1 @codecov-io @kmova