Update Volume Group utility #73

sp98 · 2022-01-16T14:21:49Z

Refactor volume group utility.
Add helper methods to list Physical Volumes
Add helper methods to delete VGs and PVs
Add unit tests for volume group utility

nbalacha

I think we can just pass devices to vgcreate/vgextend as long as they are not already pvs. The code to filter devices can return the list of unused, matching disks for the VG.

nbalacha · 2022-01-17T05:20:12Z

pkg/vgmanager/lvm.go

+	lvmPath     = "/usr/sbin/lvm"
+	extendVGCmd = "/usr/sbin/vgextend"
+	createVGCmd = "/usr/sbin/vgcreate"
+	vgRemoveCmd = "/usr/sbin/vgremove"


Please use a common format. Either
extendVGCmd, createVGCmd, removeVGCmd, removePVCmd
or
vgExtendCmd, vgCreateCmd,vgRemoveCmd, pvRemoveCmd

I prefer the second over the first.

nbalacha · 2022-01-17T05:29:29Z

pkg/vgmanager/lvm.go

+	// Remove physical volumes
+	_, err = exec.ExecuteCommandWithOutputAsHost(pvRemoveCmd, vg.PVs...)
+	if err != nil {
+		return fmt.Errorf("failed to remove volume group %q. %v", vg.Name, err)


Suggested change

return fmt.Errorf("failed to remove volume group %q. %v", vg.Name, err)

return fmt.Errorf("failed to remove physical volumes for volume group %q. %v", vg.Name, err)

nbalacha · 2022-01-17T05:36:50Z

pkg/vgmanager/lvm.go

+
+	vgFound := false
+	volumeGroup := &VolumeGroup{}
+	for _, report := range res.Report {


vgs can take the name of the vg as an argument.

sh-4.4# vgs VG #PV #LV #SN Attr VSize VFree vg1 3 0 0 wz--n- <4.37t <4.37t sh-4.4# vgs vg2 Volume group "vg2" not found Cannot process volume group vg2 sh-4.4# vgs vg1 VG #PV #LV #SN Attr VSize VFree vg1 3 0 0 wz--n- <4.37t <4.37t sh-4.4# vgs vg1 --reportformat json { "report": [ { "vg": [ {"vg_name":"vg1", "pv_count":"3", "lv_count":"0", "snap_count":"0", "vg_attr":"wz--n-", "vg_size":"<4.37t", "vg_free":"<4.37t"} ] } ] } sh-4.4# vgs vg2 --reportformat json { "report": [ Volume group "vg2" not found Cannot process volume group vg2 { "vg": [ ] } ] }

If I try vgs vg2 --reportformat json, then I need to parse the output again and check for Volume group "vg2" not found string is present or not. To avoid this parsing, I'm trying get all the vgs first, then a trying to find if vg2 is one of them.

Wouldn't the output just be an empty array?

Looks like vgs returns exit code 5 when trying to list a non-existent VG, which just means command failed. Trying to figure out why would be more difficult than iterating over the list so the current code is fine for now.

Please add a comment that vgs can take take a vg name and that we will look into whether that can be used later.

added comment

nbalacha · 2022-01-17T05:43:08Z

pkg/vgmanager/lvm.go

+// Extend extends the volume group only if new physical volumes are available
+func (vg VolumeGroup) Extend(exec internal.Executor, pvs []string) error {
+	// Extend VG only if there is any new Physical Volume
+	newPVs := listDiff(pvs, vg.PVs)


How is the pvs list determined? The PVs may still be in use but belong to another volume.

The code currently doesn't create pvs - it is directly creating Vgs from the disks.

From man vgcreate

vgcreate creates a new VG on block devices. If the devices were not previously intialized as PVs with pvcreate(8), vgcreate will initi‐ tialize them, making them PVs. The pvcreate options for initializing devices are also available with vgcreate. When vgcreate uses an existing PV, that PV's existing values for metadata size, PE start, etc, are used, even if different values are specified in the vgcreate command. To change these values, first use pvremove on the device.

filterMatchingDevices would ensure that pvs list does not contains PVs that does not belong to other VGs. listDiff will ensure that we only run vgextend with new set of PVs.

FilterDevices does not list PVs. It is returning a list of devices only. However, this function is comparing PVs to the PVs in the VG. Based on the current code, the presence of a PV would mean that it is already being used in a VG (as it does not create them explicitly but as part of a vgcreate/vgextend call) so comparing it to a particular VGs PV list is incorrect and unnecessary.

vgextend command should always contain new physical volumes, else it will fail. This is a bug in the current code as well. For example:

# lsblk NAME MAJ:MIN RM SIZE RO TYPE MOUNTPOINT sda 8:0 0 19.5G 0 disk `-sda1 8:1 0 19.5G 0 part /mnt/sda1 sdb 8:16 0 10G 0 disk sdc 8:32 0 8G 0 disk sr0 11:0 1 225.6M 0 rom # vgs # vgcreate vg1 /dev/sdb Physical volume "/dev/sdb" successfully created. Volume group "vg1" successfully created # vgs VG #PV #LV #SN Attr VSize VFree vg1 1 0 0 wz--n- <10.00g <10.00g # vgextend vg1 /dev/sdb /dev/sdc Physical volume '/dev/sdb' is already in volume group 'vg1' Unable to add physical volume '/dev/sdb' to volume group 'vg1' /dev/sdb: physical volume not initialized. # vgs VG #PV #LV #SN Attr VSize VFree vg1 1 0 0 wz--n- <10.00g <10.00g #

listdiff tries to ensure that vgextend will always use new Physical volumes.

removed istdiff

leelavg

implementation of invoking vg deletion is in progress?

leelavg · 2022-01-17T07:21:33Z

pkg/vgmanager/lvm.go

+// Delete deletes a volume group and the physical volumes associated with it
+func (vg VolumeGroup) Delete(exec internal.Executor) error {
+	// Remove Volume Group
+	vgArgs := []string{vg.Name}


use --nolocking as well? rationale is if the volume group is huge acquiring lock takes time

leelavg · 2022-01-17T07:29:33Z

pkg/vgmanager/lvm.go

+}
+
+// GetVolumeGroup returns a volume group along with the associated physical volumes
+func GetVolumeGroup(exec internal.Executor, name string) (*VolumeGroup, error) {


Is this function a standby for now as I don't see it's usage?

I can see some functions as this only with tests but this function seems to be a duplicate or specific case of ListVolumeGroups?

GetVolumeGroup not used anywhere now.

Since we might move to one CR for each VG now, we can replace ListVolumeGroup with GetVolumeGroup. GetVolumeGroup checks internally if volume group is available or not.

- Add option to list all PVs - Add helper to get, delete, extend VG - Add helper to delete PV Signed-off-by: Santosh Pillai <sapillai@redhat.com>

sp98 · 2022-01-17T07:57:14Z

implementation of invoking vg deletion is in progress?

No. This PR only provides helper function to delete VG and PVs. Its invocation is not included in this PR.

nbalacha · 2022-01-17T08:45:28Z

pkg/vgmanager/lvm_test.go

+		pvCount int
+		wantErr bool
+	}{
+		{"Invalid volume group name", "invald-vg", 0, true},


Nit. invalid-vg

leelavg

/lgtm

jmolmo

LGTM, although i would like to have the output of the vg commands when they are executed successfully.

jmolmo · 2022-01-17T11:07:34Z

pkg/vgmanager/lvm.go

-	volumeGroupMap := map[string][]map[string][]VolumeGroup{}
-	arg := []string{
+// Create creates a new volume group
+func (vg VolumeGroup) Create(exec internal.Executor, pvs []string) error {


Create, extend and delete methods only returns error...and in case of success we are losing the output , which can be useful in logs just to trace what is happening. Maybe it could be a good idea to return the error and also and output string to provide the output produced by the vg command when it is executed succesfully.

I don't think it will be very useful. We would only be interested to know whether vg got created or not. If not, then what's the error.

openshift-ci · 2022-01-17T11:08:58Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jmolmo, leelavg, nbalacha, sp98

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Signed-off-by: Santosh Pillai <sapillai@redhat.com>

openshift-ci · 2022-01-17T11:55:36Z

New changes are detected. LGTM label has been removed.

sp98 requested a review from nbalacha January 16, 2022 14:21

sp98 force-pushed the vg-delete branch 2 times, most recently from cd58556 to 40a1107 Compare January 17, 2022 03:22

leelavg mentioned this pull request Jan 17, 2022

feat: cleanup volume groups on CR deletion #53

Closed

nbalacha suggested changes Jan 17, 2022

View reviewed changes

openshift-ci bot assigned nbalacha Jan 17, 2022

sp98 force-pushed the vg-delete branch 3 times, most recently from b0826fe to 02021c9 Compare January 17, 2022 07:17

sp98 requested a review from nbalacha January 17, 2022 07:20

leelavg reviewed Jan 17, 2022

View reviewed changes

sp98 force-pushed the vg-delete branch from 02021c9 to 853130a Compare January 17, 2022 07:53

feat: update VolumeGroup utility

14711db

- Add option to list all PVs - Add helper to get, delete, extend VG - Add helper to delete PV Signed-off-by: Santosh Pillai <sapillai@redhat.com>

sp98 force-pushed the vg-delete branch from 853130a to 5861256 Compare January 17, 2022 07:56

sp98 requested a review from leelavg January 17, 2022 07:57

nbalacha reviewed Jan 17, 2022

View reviewed changes

nbalacha approved these changes Jan 17, 2022

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 17, 2022

leelavg approved these changes Jan 17, 2022

View reviewed changes

openshift-ci bot assigned leelavg Jan 17, 2022

jmolmo approved these changes Jan 17, 2022

View reviewed changes

openshift-ci bot assigned jmolmo Jan 17, 2022

sp98 added 2 commits January 17, 2022 17:25

test: add tests for volume group utility

3e76eff

Signed-off-by: Santosh Pillai <sapillai@redhat.com>

chore: go mod tidy

51a2323

Signed-off-by: Santosh Pillai <sapillai@redhat.com>

sp98 force-pushed the vg-delete branch from 5861256 to 51a2323 Compare January 17, 2022 11:55

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 17, 2022

nbalacha merged commit 67ffe2f into openshift:main Jan 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Volume Group utility #73

Update Volume Group utility #73

sp98 commented Jan 16, 2022

nbalacha left a comment

nbalacha Jan 17, 2022 •

edited

Loading

sp98 Jan 17, 2022

nbalacha Jan 17, 2022

sp98 Jan 17, 2022

nbalacha Jan 17, 2022

sp98 Jan 17, 2022

nbalacha Jan 17, 2022

nbalacha Jan 17, 2022 •

edited

Loading

sp98 Jan 17, 2022

nbalacha Jan 17, 2022

sp98 Jan 17, 2022

nbalacha Jan 17, 2022

sp98 Jan 17, 2022

sp98 Jan 17, 2022

leelavg left a comment

leelavg Jan 17, 2022

leelavg Jan 17, 2022

sp98 Jan 17, 2022 •

edited

Loading

sp98 commented Jan 17, 2022 •

edited

Loading

nbalacha Jan 17, 2022

sp98 Jan 17, 2022

leelavg left a comment

jmolmo left a comment

jmolmo Jan 17, 2022

sp98 Jan 17, 2022

openshift-ci bot commented Jan 17, 2022

openshift-ci bot commented Jan 17, 2022

	return fmt.Errorf("failed to remove volume group %q. %v", vg.Name, err)
	return fmt.Errorf("failed to remove physical volumes for volume group %q. %v", vg.Name, err)

Update Volume Group utility #73

Update Volume Group utility #73

Conversation

sp98 commented Jan 16, 2022

nbalacha left a comment

Choose a reason for hiding this comment

nbalacha Jan 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbalacha Jan 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leelavg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sp98 Jan 17, 2022 • edited Loading

Choose a reason for hiding this comment

sp98 commented Jan 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leelavg left a comment

Choose a reason for hiding this comment

jmolmo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Jan 17, 2022

openshift-ci bot commented Jan 17, 2022

nbalacha Jan 17, 2022 •

edited

Loading

nbalacha Jan 17, 2022 •

edited

Loading

sp98 Jan 17, 2022 •

edited

Loading

sp98 commented Jan 17, 2022 •

edited

Loading