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(snapshot): add support for thin snapshot for lvm volumes #102

Merged
merged 3 commits into from
Jun 21, 2021
Merged

feat(snapshot): add support for thin snapshot for lvm volumes #102

merged 3 commits into from
Jun 21, 2021

Conversation

prateekpandey14
Copy link
Member

@prateekpandey14 prateekpandey14 commented Jun 17, 2021 •
edited
Loading

Signed-off-by: prateekpandey14 [email protected]

Pull Request template

Why is this PR required? What issue does it fix?:
To support thin snapshot for lvm volumes

What this PR does?:
Add support for thin snapshot for lvm volumes based on the SnapSize configured in SnapshotClass.

So there are two scenarios here :

1. Snapshot class snapsize parameter.

In this case whether volume is thin provisioned or thick provisioned, we should create a thick snapshot with size as snapsize mentioned in the snapshotclass.

kind: VolumeSnapshotClass
apiVersion: snapshot.storage.k8s.io/v1
metadata:
  name: lvmpv-snapclass
  annotations:
    snapshot.storage.kubernetes.io/is-default-class: "true"
parameters:
  snapsize: 1Gi
driver: local.csi.openebs.io
deletionPolicy: Delete

 $ sudo lvs
  LV                                       VG    Attr       LSize Pool           Origin                                   Data%  Meta%  Move Log Cpy%Sync Convert
  7ba91701-8663-47a9-898e-40970ca137b8     lvmvg sri-a-s--- 1.00g                pvc-699c1ee1-c0c0-4bb1-a353-f63e8be5f190 0.01                                   
  c1c4264f-2216-4e33-9145-dbf7a357d4b5     lvmvg sri-a-s--- 1.00g                pvc-c367aad2-885f-4eb1-82b4-17df56285a8f 0.01                                   
  lvmvg_thinpool                           lvmvg twi-aotz-- 2.00g                                                         4.76   12.30                           
  pvc-699c1ee1-c0c0-4bb1-a353-f63e8be5f190 lvmvg owi-aos--- 4.00g                                                                                                
  pvc-c367aad2-885f-4eb1-82b4-17df56285a8f lvmvg owi-aotz-- 2.00g lvmvg_thinpool                                          4.76

2. Snapshot class does not have snapsize parameter
In this case, for thin volumes, we should create a thin snapshot and for thick volume, we should create a thick snapshot.
While creating the thick snapshot for thick volumes, since snapsize parameter is not provided, driver should reserve size equal to the volume for snapshots.

kind: VolumeSnapshotClass
apiVersion: snapshot.storage.k8s.io/v1
metadata:
  name: lvmpv-snapclass
  annotations:
    snapshot.storage.kubernetes.io/is-default-class: "true"
driver: local.csi.openebs.io
deletionPolicy: Delete
$ sudo lvs
  LV                                       VG    Attr       LSize Pool           Origin                                   Data%  Meta%  Move Log Cpy%Sync Convert
  c6a3e548-8614-4c9f-86a3-774de6eaaa73     lvmvg Vri---tz-k 2.00g lvmvg_thinpool pvc-c367aad2-885f-4eb1-82b4-17df56285a8f                                        
  f2e18137-1aa2-4302-a562-17ba9f088bc4     lvmvg sri-a-s--- 4.00g                pvc-699c1ee1-c0c0-4bb1-a353-f63e8be5f190 0.01                                   
  lvmvg_thinpool                           lvmvg twi-aotz-- 2.00g                                                         4.77   12.50                           
  pvc-699c1ee1-c0c0-4bb1-a353-f63e8be5f190 lvmvg owi-aos--- 4.00g                                                                                                
  pvc-c367aad2-885f-4eb1-82b4-17df56285a8f lvmvg Vwi-aotz-- 2.00g lvmvg_thinpool                                          4.76

Does this PR require any upgrade changes?:
No

Checklist:

akhilerm
akhilerm reviewed Jun 17, 2021
View reviewed changes
pkg/driver/controller.go Outdated Show resolved Hide resolved
akhilerm
akhilerm suggested changes Jun 17, 2021
View reviewed changes
Copy link
Member

@akhilerm akhilerm left a comment

Choose a reason for hiding this comment

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

@prateekpandey14 suggested a few changes.

prateekpandey14
prateekpandey14 commented Jun 21, 2021
View reviewed changes
pkg/lvm/lvm_util.go
@@ -518,6 +523,17 @@ func lvThinExists(vg string, name string) bool {
return name == strings.TrimSpace(string(out))
}

// snapshotExists checks if a snapshot volume exists given the name of the volume.
func isSnapshotExists(vg, snapVolumeName string) (bool, error) {
cmd := exec.Command("lvs", vg+"/"+snapVolumeName, "--noheadings", "-o", "lv_name")
Copy link
Member Author

@prateekpandey14 prateekpandey14 Jun 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This change is required , since thin-snapshot can not be found in /dev/vol-group/snapshot-id path using os.State().

@prateekpandey14
feat(snap): add support for thin snapshot for thin volumes
d285676 
thin snapshot will be created only for thin volumes

Signed-off-by: prateekpandey14 <[email protected]>
@prateekpandey14 prateekpandey14 changed the title feat(snap): add support for thin snapshot for thin volumes feat(snapshot): add support for thin snapshot for lvm volumes Jun 21, 2021
@pawanpraka1 pawanpraka1 added the enhancement New feature or request label Jun 21, 2021
@pawanpraka1 pawanpraka1 added this to the v0.7 milestone Jun 21, 2021
@prateekpandey14
fix review comment
01c9483 
Signed-off-by: prateekpandey14 <[email protected]>
pawanpraka1
pawanpraka1 reviewed Jun 21, 2021
View reviewed changes
pkg/lvm/lvm_util.go Outdated Show resolved Hide resolved
shubham14bajpai
shubham14bajpai approved these changes Jun 21, 2021
View reviewed changes
Copy link
Member

@shubham14bajpai shubham14bajpai left a comment

Choose a reason for hiding this comment

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

LGTM

@prateekpandey14
fix the code comment
6b04147 
Signed-off-by: prateekpandey14 <[email protected]>
pawanpraka1
pawanpraka1 approved these changes Jun 21, 2021
View reviewed changes
Copy link
Contributor

@pawanpraka1 pawanpraka1 left a comment

Choose a reason for hiding this comment

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

looks good.

@pawanpraka1 pawanpraka1 merged commit 971b9d6 into openebs:master Jun 21, 2021
@pawanpraka1
Copy link
Contributor

pawanpraka1 commented Jun 21, 2021 •
edited
Loading

@prateekpandey14 can we update the doc with this paramater and its behavior?

@prateekpandey14 prateekpandey14 deleted the thin-snapshot branch June 22, 2021 04:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shubham14bajpai shubham14bajpai shubham14bajpai approved these changes

@pawanpraka1 pawanpraka1 pawanpraka1 approved these changes

@akhilerm akhilerm Awaiting requested review from akhilerm

@praveengt praveengt Awaiting requested review from praveengt

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
v0.7
Development

Successfully merging this pull request may close these issues.

add support to create thin snapshots
4 participants
@prateekpandey14 @pawanpraka1 @akhilerm @shubham14bajpai