misc improvement #5

Vicente-Cheng · 2024-07-10T11:47:14Z

Mainly changes for this PR as below:

support vgName from SC
remove vgName/devicePattern
drop csi ephemeral volume support
support generic ephemeral volume
remove redundant code
use new executor
fix the volume expand
support snapshot create/delete
support clone
support dm-thin
enable CGO, move to Debian for cross-compile (binary part)
Update ReadME and CRDs

- Also, remove unused variables Signed-off-by: Vicente Cheng <[email protected]>

Signed-off-by: Vicente Cheng <[email protected]>

- We need to use the command executor from go-common Signed-off-by: Vicente Cheng <[email protected]>

- use the executor to get an accurate command output - Improve the delete lv that can delete lv w/o vg name Signed-off-by: Vicente Cheng <[email protected]>

- Now we use the user-define volume group on the StorageClass. That will give more flexibility for creating lv with different vg Signed-off-by: Vicente Cheng <[email protected]>

- After we support user-defined vg name, we could drop the vgname and device pattern. - Now we support generic ephemeral volume and drop the csi ephemeral volume support. Signed-off-by: Vicente Cheng <[email protected]>

Signed-off-by: Vicente Cheng <[email protected]>

- Execute lvextend multiple times will get EIO if size not change. We need to add pre-check for avoiding that. Signed-off-by: Vicente Cheng <[email protected]>

Signed-off-by: Vicente Cheng <[email protected]>

Vicente-Cheng · 2024-07-23T07:53:53Z

Hi @tserong, @bk201,
Please refer to the new README on this PR to know how to install. Then, update the lvm.conf as OS2 PR (harvester/os2#121)

You also need to build the image from this PR to test manually.

Vicente-Cheng · 2024-07-25T03:57:01Z

~~Just a reminder, we need to wait harvester/go-common#17 to merge and update the go.mod~~
updated

bk201

LGTM, function works great.
Please help coordinate with the GUI team to fix the "undefined" string in the storage class.

pkg/lvm/controllerserver.go

examples/pvc-clone.yaml

WebberHuang1118

LGTM, thanks for the PR, there is one point I want to double-confirm:

https://github.com/Vicente-Cheng/csi-driver-lvm/blob/b226de6de5f2d6a66ec4cd25a4f6075cc8acb18b/cmd/provisioner/clonelv.go#L102-L104

As creating a new LV from a snapshot, the actual logic here is to create a device and copy the data from the snapshot LV to it.

However, for dm-thin, there is a way to make the original device and new device (which is created from a snapshot) share underlying blocks until one of them receives write, and the break-sharing will be triggered, this introduces better space efficiency. We can have this with dmsetup:

//creating a new source dm-thin device
sudo dmsetup message /dev/mapper/thin-pool 0 "create_thin 0"
sudo dmsetup create thin-device --table "0 20971520 thin /dev/mapper/thin-pool 0"

//taking a snapshot for the source device
sudo dmsetup message /dev/mapper/thin-pool 0 "create_snap 0 1"

//creating a new dm-thin device from the snapshot
sudo dmsetup create thin-device-snap --table "0 20971520 thin /dev/mapper/thin-pool 1"

After some searching on LVM, it seems LVM doesn't provide such a mechanism, only dmsetup does. Do you have any thoughts on this?

Update: we can use LVM to have this

//creating thin-pool
sudo lvcreate --size 10G --thinpool vg_thin/pool_thin

// creating dm-thin LV
sudo lvcreate --thin --name thin_lv1 --virtualsize 5G vg_thin/pool_thin

//taking snapshot for the LV
sudo lvcreate --snapshot --name thin_lv1_snap /dev/vg_thin/thin_lv1

//activating a dm-thin LV from the snapshot
sudo lvchange -ay -K vg_thin/thin_lv1_snap

tserong

LGTM in general, just a couple of questions

tserong · 2024-09-13T04:39:05Z

Dockerfile.dapper

@@ -1,11 +1,11 @@

-FROM registry.suse.com/bci/golang:1.22
+FROM golang:1.22-bookworm


Why change this?

bci/golang:1.22 image lacks some cross-platform packages. I need these packages to build the cross-platform binary.

So I changed to Bookworm, that is, to have all cross-platform packages to build.

tserong · 2024-09-13T04:41:32Z

pkg/lvm/controllerserver.go

@@ -116,7 +116,7 @@ func (cs *controllerServer) CreateVolume(ctx context.Context, req *csi.CreateVol
 	}

 	lvmType := req.GetParameters()["type"]
-	if !(lvmType == "linear" || lvmType == "mirror" || lvmType == "striped") {


Why drop the "linear" and "mirror" types?

Because we thought the linear was similar to dm-thin and the mirror would not use too much, we removed these two and focused on the striped and dm-thin.

REF: https://github.com/harvester/harvester/pull/5956/files#diff-f23f9e440c95647f9799f80493d04ea4e5c2ecc2a2165945f52a390d105288dbR63

Got it, thanks.

WebberHuang1118 · 2024-09-16T03:11:27Z

Hi @Vicente-Cheng
Even though we already discussed about how to handle dm-thin-pool expansion, can you explain here to have a record, or have the related comments somewhere?

Vicente-Cheng · 2024-09-17T16:22:44Z

LGTM, thanks for the PR, there is one point I want to double-confirm:

https://github.com/Vicente-Cheng/csi-driver-lvm/blob/b226de6de5f2d6a66ec4cd25a4f6075cc8acb18b/cmd/provisioner/clonelv.go#L102-L104

As creating a new LV from a snapshot, the actual logic here is to create a device and copy the data from the snapshot LV to it.

However, for dm-thin, there is a way to make the original device and new device (which is created from a snapshot) share underlying blocks until one of them receives write, and the break-sharing will be triggered, this introduces better space efficiency. We can have this with dmsetup:
//creating a new source dm-thin device
sudo dmsetup message /dev/mapper/thin-pool 0 "create_thin 0"
sudo dmsetup create thin-device --table "0 20971520 thin /dev/mapper/thin-pool 0"

//taking a snapshot for the source device
sudo dmsetup message /dev/mapper/thin-pool 0 "create_snap 0 1"

//creating a new dm-thin device from the snapshot
sudo dmsetup create thin-device-snap --table "0 20971520 thin /dev/mapper/thin-pool 1"
After some searching on LVM, it seems LVM doesn't provide such a mechanism, only dmsetup does. Do you have any thoughts on this?

Update: we can use LVM to have this
//creating thin-pool
sudo lvcreate --size 10G --thinpool vg_thin/pool_thin

// creating dm-thin LV
sudo lvcreate --thin --name thin_lv1 --virtualsize 5G vg_thin/pool_thin

//taking snapshot for the LV
sudo lvcreate --snapshot --name thin_lv1_snap /dev/vg_thin/thin_lv1

//activating a dm-thin LV from the snapshot
sudo lvchange -ay -K vg_thin/thin_lv1_snap

Thanks for the reminder!

After discussed with @WebberHuang1118, the new clone from snapshot would utilize the dm-thin pros.
That is, use lvm command to create another snapshot based on the above snapshot.
Thus, we could reuse the previous snapshot, and we could also utilize the dm-thin pros.

e.g. (like above example, we already create a snapshot)

//creating thin-pool
$ lvcreate --size 10G --thinpool vg_thin/pool_thin

// creating dm-thin LV
$ lvcreate --thin --name thin_lv1 --virtualsize 5G vg_thin/pool_thin
 
//taking snapshot for the LV
$ lvcreate --snapshot --name thin_lv1_snap /dev/vg_thin/thin_lv1

// taking another snapshot for the above snapshot (that is clone, we will active this snapshot to use)
$ lvcreate --snapshot -name <target pvc name> vg_thin/thin_lv1_snap

That is the new behavior for the dm-thin snapshot clone.
For others, we keep the device copy mechanism.

Thanks!

tserong

I found a few small naming discrepancies in command line arguments. Otherwise LGTM in general (note: I haven't tried testing the snapshot functionality myself)

cmd/provisioner/createsnap.go

cmd/provisioner/deletesnap.go

cmd/provisioner/createsnap.go

cmd/provisioner/deletesnap.go

pkg/lvm/controllerserver.go

- fix volume expansaion - update deploy/charts for snapshot Signed-off-by: Vicente Cheng <[email protected]>

Signed-off-by: Vicente Cheng <[email protected]>

- support clone from PVC or Snapshot - remove resource limit because clone pod need more memory Signed-off-by: Vicente Cheng <[email protected]>

- also bump google.golang.org/grpc v1.64.1 Signed-off-by: Vicente Cheng <[email protected]>

- also drop linear/mirror Signed-off-by: Vicente Cheng <[email protected]>

Signed-off-by: Vicente Cheng <[email protected]>

- also correct the description of command Signed-off-by: Vicente Cheng <[email protected]>

Vicente-Cheng · 2024-09-18T09:00:27Z

The commit 4e3dacd of this PR also related to harvester/harvester#6568

WebberHuang1118

LGTM, thanks for the PR.

deploy: correct the default image tag

757a008

- Also, remove unused variables Signed-off-by: Vicente Cheng <[email protected]>

Vicente-Cheng requested review from tserong and bk201 July 10, 2024 11:47

Vicente-Cheng added 7 commits July 11, 2024 17:06

misc: remove redundant and fix the command path

6de439a

Signed-off-by: Vicente Cheng <[email protected]>

vendor: add go-common

ab1e3e6

- We need to use the command executor from go-common Signed-off-by: Vicente Cheng <[email protected]>

lvm: use new executor to execute command

1cbb5d4

- use the executor to get an accurate command output - Improve the delete lv that can delete lv w/o vg name Signed-off-by: Vicente Cheng <[email protected]>

csi/controller: use vg from StorageClass

b743366

- Now we use the user-define volume group on the StorageClass. That will give more flexibility for creating lv with different vg Signed-off-by: Vicente Cheng <[email protected]>

csi/controller: remove vgname, device pattern, ephemeral parameter

ed8a71f

- After we support user-defined vg name, we could drop the vgname and device pattern. - Now we support generic ephemeral volume and drop the csi ephemeral volume support. Signed-off-by: Vicente Cheng <[email protected]>

vendor: remove go-units

59b1b46

Signed-off-by: Vicente Cheng <[email protected]>

csi/controller: make the volme expand really work

f2f2def

- Execute lvextend multiple times will get EIO if size not change. We need to add pre-check for avoiding that. Signed-off-by: Vicente Cheng <[email protected]>

Vicente-Cheng force-pushed the misc-improvement branch from 0a0f703 to f2f2def Compare July 11, 2024 09:07

vendor: add snapshot related packages

3494c28

Signed-off-by: Vicente Cheng <[email protected]>

Vicente-Cheng marked this pull request as draft July 15, 2024 16:05

Vicente-Cheng force-pushed the misc-improvement branch 5 times, most recently from 1509a84 to 6b2de1d Compare July 23, 2024 03:18

Vicente-Cheng marked this pull request as ready for review July 23, 2024 07:16

bk201 requested a review from WebberHuang1118 September 3, 2024 06:27

Vicente-Cheng force-pushed the misc-improvement branch from e26de43 to b226de6 Compare September 6, 2024 01:48

bk201 approved these changes Sep 9, 2024

View reviewed changes

pkg/lvm/controllerserver.go Outdated Show resolved Hide resolved

examples/pvc-clone.yaml Outdated Show resolved Hide resolved

WebberHuang1118 reviewed Sep 12, 2024

View reviewed changes

tserong reviewed Sep 13, 2024

View reviewed changes

Vicente-Cheng force-pushed the misc-improvement branch from b226de6 to d35e274 Compare September 17, 2024 16:13

Vicente-Cheng requested a review from tserong September 17, 2024 16:25

Vicente-Cheng requested a review from WebberHuang1118 September 17, 2024 16:25

Vicente-Cheng force-pushed the misc-improvement branch 2 times, most recently from d3824a8 to 57ed509 Compare September 17, 2024 16:32

tserong reviewed Sep 18, 2024

View reviewed changes

Vicente-Cheng requested a review from tserong September 18, 2024 03:17

tserong approved these changes Sep 18, 2024

View reviewed changes

Vicente-Cheng added 8 commits September 18, 2024 16:39

controller: add snapshot capability

6654197

- fix volume expansaion - update deploy/charts for snapshot Signed-off-by: Vicente Cheng <[email protected]>

vendor: update ioutil to support clone

5c4d021

Signed-off-by: Vicente Cheng <[email protected]>

controller: add clone capability

4991335

- support clone from PVC or Snapshot - remove resource limit because clone pod need more memory Signed-off-by: Vicente Cheng <[email protected]>

build: enable CGO and cross compiler

d872c1b

- also bump google.golang.org/grpc v1.64.1 Signed-off-by: Vicente Cheng <[email protected]>

controller: support lvm type dm-thin

e7d70d7

- also drop linear/mirror Signed-off-by: Vicente Cheng <[email protected]>

common: update README and example CRDs

b6ccf57

Signed-off-by: Vicente Cheng <[email protected]>

common: bump golang 1.22.7 and bci image 15.6

4e3dacd

Signed-off-by: Vicente Cheng <[email protected]>

controller: refact the duplcated codes for the clone operation

94e7c8c

- also correct the description of command Signed-off-by: Vicente Cheng <[email protected]>

Vicente-Cheng force-pushed the misc-improvement branch from e04ed47 to 94e7c8c Compare September 18, 2024 08:39

WebberHuang1118 approved these changes Sep 18, 2024

View reviewed changes

bk201 merged commit f14e909 into harvester:main Sep 18, 2024
2 checks passed

brandboat mentioned this pull request Sep 26, 2024

[ENHANCEMENT] Bump base image to BCI 15.6 harvester/harvester#6568

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

misc improvement #5

misc improvement #5

Vicente-Cheng commented Jul 10, 2024 •

edited

Loading

Vicente-Cheng commented Jul 23, 2024 •

edited

Loading

Vicente-Cheng commented Jul 25, 2024 •

edited

Loading

bk201 left a comment

WebberHuang1118 left a comment •

edited

Loading

tserong left a comment

tserong Sep 13, 2024

Vicente-Cheng Sep 13, 2024

tserong Sep 13, 2024

Vicente-Cheng Sep 17, 2024

tserong Sep 18, 2024

WebberHuang1118 commented Sep 16, 2024

Vicente-Cheng commented Sep 17, 2024

tserong left a comment

Vicente-Cheng commented Sep 18, 2024

WebberHuang1118 left a comment

		@@ -1,11 +1,11 @@

		FROM registry.suse.com/bci/golang:1.22
		FROM golang:1.22-bookworm

misc improvement #5

misc improvement #5

Conversation

Vicente-Cheng commented Jul 10, 2024 • edited Loading

Vicente-Cheng commented Jul 23, 2024 • edited Loading

Vicente-Cheng commented Jul 25, 2024 • edited Loading

bk201 left a comment

Choose a reason for hiding this comment

WebberHuang1118 left a comment • edited Loading

Choose a reason for hiding this comment

tserong left a comment

Choose a reason for hiding this comment

tserong Sep 13, 2024

Choose a reason for hiding this comment

Vicente-Cheng Sep 13, 2024

Choose a reason for hiding this comment

tserong Sep 13, 2024

Choose a reason for hiding this comment

Vicente-Cheng Sep 17, 2024

Choose a reason for hiding this comment

tserong Sep 18, 2024

Choose a reason for hiding this comment

WebberHuang1118 commented Sep 16, 2024

Vicente-Cheng commented Sep 17, 2024

tserong left a comment

Choose a reason for hiding this comment

Vicente-Cheng commented Sep 18, 2024

WebberHuang1118 left a comment

Choose a reason for hiding this comment

Vicente-Cheng commented Jul 10, 2024 •

edited

Loading

Vicente-Cheng commented Jul 23, 2024 •

edited

Loading

Vicente-Cheng commented Jul 25, 2024 •

edited

Loading

WebberHuang1118 left a comment •

edited

Loading