feat: make API and controller changes to support manual disk selection #229

iamniting · 2022-07-18T05:03:45Z

Allow user to select the block devices while VG creation in the LVMCluster CR.

Signed-off-by: Nitin Goyal nigoyal@redhat.com

api/v1alpha1/lvmcluster_types.go

api/v1alpha1/lvmvolumegroup_types.go

sp98 · 2022-07-28T06:12:52Z

bundle/manifests/lvm.topolvm.io_lvmclusters.yaml

@@ -254,6 +254,10 @@ spec:
                          node:
                            description: Node is the name of the node
                            type: string
+                          reason:


This looks like something that can be part of the make bundle commit.

sp98 · 2022-07-28T06:24:28Z

controllers/lvmcluster_controller.go

@@ -208,6 +215,46 @@ func (r *LVMClusterReconciler) reconcile(ctx context.Context, instance *lvmv1alp
 	return ctrl.Result{}, nil
 }

+func (r *LVMClusterReconciler) verifyLvmClusterSpec(ctx context.Context, instance *lvmv1alpha1.LVMCluster) error {
+
+	// make sure no device overlap with another VGs


what if the user decides to give the device names instead of device paths?
I know we encourage them to use paths, but still if they decide to use device names instead, then this validation will always fail, if I'm not wrong.

Even in that case, it should work, as the device names will only be the same. I am just loading the devices into a map; if the key already exists, we are erroring out.

sp98 · 2022-07-28T06:27:46Z

controllers/lvmcluster_controller.go

@@ -208,6 +215,46 @@ func (r *LVMClusterReconciler) reconcile(ctx context.Context, instance *lvmv1alp
 	return ctrl.Result{}, nil
 }

+func (r *LVMClusterReconciler) verifyLvmClusterSpec(ctx context.Context, instance *lvmv1alpha1.LVMCluster) error {


method name can be more specific about what we are trying to verify instead of verifyLvmClusterSpec

This comments still holds. The function only validates the device classes in the Spec. The function name should reflect that.

added a new func for this logic and that will be called from here

sp98 · 2022-07-28T06:43:49Z

api/v1alpha1/lvmcluster_types.go

@@ -108,6 +114,11 @@ type LVMClusterStatus struct {
 	// Ready describes if the LVMCluster is ready.
 	// +optional
 	Ready bool `json:"ready,omitempty"`
+
+	// Reason describes the reason behind any success or failure.


do you mean reason behind failure only? Don't think we add any success reason.

we can also have a reason behind the success which will always be the exact string. I think success reason should not matter much, as much as failure.

I would prefer to use conditions here if possible.

added conditions

sp98 · 2022-07-28T06:49:19Z

controllers/lvmcluster_controller.go

+					instance.Status.Reason = err.Error()
+
+					// Apply status changes
+					err = r.Client.Status().Update(ctx, instance)


IMO, updating status should not be part of the verification function. It should only be concerned about the verification part.

This verification function is called precisely after getting the instance and we won't get to an actual update call as we will be returning from here only, so we need to do an update here itself only and only if there is a failure.

This should also check for node selector when validating the devices overlaps. It is possible for the same device path in different nodes to be used in different device classes.

checking node selectors as well now

sp98 · 2022-07-28T07:00:37Z

api/v1alpha1/lvmcluster_types.go

+
+	// Reason describes the reason behind any success or failure.
+	// +optional
+	Reason string `json:"reason,omitempty"`


Do we add reason for error specific to this PR only?
If that's the case, then we need to add reasons for other failures as well in a separate PR.

I am updating the reason wherever required.

sp98 · 2022-07-28T07:25:57Z

pkg/vgmanager/vgmanager_controller.go

-// TODO: Implement this
-func filterMatchingDevices(blockDevices []internal.BlockDevice, volumeGroup *lvmv1alpha1.LVMVolumeGroup) ([]internal.BlockDevice, []internal.BlockDevice, error) {
-	// currently just match all devices
+func (r *VGReconciler) filterMatchingDevices(blockDevices []internal.BlockDevice, volumeGroup *lvmv1alpha1.LVMVolumeGroup) ([]internal.BlockDevice, []internal.BlockDevice, error) {


Unit test should be added for this function.

comment still valid.

I will add them later once we agree with the actual code changes to minimize the changes in unit tests.

sp98 · 2022-07-28T07:27:33Z

pkg/vgmanager/vgmanager_controller.go

-// TODO: Implement this
-func filterMatchingDevices(blockDevices []internal.BlockDevice, volumeGroup *lvmv1alpha1.LVMVolumeGroup) ([]internal.BlockDevice, []internal.BlockDevice, error) {
-	// currently just match all devices
+func (r *VGReconciler) filterMatchingDevices(blockDevices []internal.BlockDevice, volumeGroup *lvmv1alpha1.LVMVolumeGroup) ([]internal.BlockDevice, []internal.BlockDevice, error) {


do you plan to the use the unmatched disks return value? If not, then we can avoid returning that.

no plans we can remove it

Still see this return value in the code.

missed it earlier, made changes now

sp98 · 2022-07-28T07:45:03Z

pkg/vgmanager/vgmanager_controller.go

+				if ok {
+					filteredBlockDevices = append(filteredBlockDevices, blockDevice)
+				} else {
+					err := fmt.Errorf("can not find disk name %s in the available block devices", path)


Suggested change

err := fmt.Errorf("can not find disk name %s in the available block devices", path)

err := fmt.Errorf("can not find disk name %s in the available block devices", diskName)

both will be the same only, So updating to path should make any differences

nbalacha · 2022-08-01T09:04:45Z

api/v1alpha1/lvmcluster_types.go

@@ -108,6 +114,11 @@ type LVMClusterStatus struct {
 	// Ready describes if the LVMCluster is ready.
 	// +optional
 	Ready bool `json:"ready,omitempty"`
+
+	// Reason describes the reason behind any success or failure.


I would prefer to use conditions here if possible.

nbalacha · 2022-08-01T09:07:11Z

controllers/lvmcluster_controller.go

+					instance.Status.Reason = err.Error()
+
+					// Apply status changes
+					err = r.Client.Status().Update(ctx, instance)


This should also check for node selector when validating the devices overlaps. It is possible for the same device path in different nodes to be used in different device classes.

sp98 · 2022-08-04T05:20:53Z

pkg/vgmanager/vgmanager_controller.go

+		if device.DiskByPath != "" {
+			args = append(args, device.DiskByPath)
+		} else {
+			args = append(args, device.KName)


don't think it will work if complete device name with path (for example: dev/sdb) is not provided in the args

It should work as we are getting the full path now.

sp98 · 2022-08-04T05:56:55Z

pkg/internal/block_device.go

 }

 // ListBlockDevices using the lsblk command
 func ListBlockDevices(exec Executor) ([]BlockDevice, error) {
 	// var output bytes.Buffer
 	var blockDeviceMap map[string][]BlockDevice
 	columns := "NAME,ROTA,TYPE,SIZE,MODEL,VENDOR,RO,STATE,KNAME,SERIAL,PARTLABEL,FSTYPE"
-	args := []string{"--json", "-o", columns}
+	args := []string{"--json", "--paths", "-o", columns}


why is --paths argument needed?

using --paths will give us the full path and make our job easy while comparing the paths and while adding paths into some commands we don't have to add /dev/ anymore.

sp98

some comments are not addressed

bundle/manifests/lvm.topolvm.io_lvmclusters.yaml

api/v1alpha1/lvmcluster_types.go

controllers/conditions.go

controllers/lvm_volumegroup.go

sp98 · 2022-08-17T03:14:57Z

controllers/lvmcluster_controller.go

@@ -208,6 +215,46 @@ func (r *LVMClusterReconciler) reconcile(ctx context.Context, instance *lvmv1alp
 	return ctrl.Result{}, nil
 }

+func (r *LVMClusterReconciler) verifyLvmClusterSpec(ctx context.Context, instance *lvmv1alpha1.LVMCluster) error {


This comments still holds. The function only validates the device classes in the Spec. The function name should reflect that.

controllers/lvmcluster_controller.go

mulbc · 2022-08-26T07:29:47Z

controllers/lvmcluster_controller.go

+	devices := make(map[string]map[string]string)
+
+	for _, deviceClass := range instance.Spec.Storage.DeviceClasses {
+		if deviceClass.DeviceSelector != nil {


what if we (in the future) have two deviceClasses - one taking up all the disks and one with manual disk selection?

we should not support such cases IMO. This code will just check the manual disk paths only as other device class wont have any list of devices in the spec.

mulbc · 2022-08-26T07:31:40Z

controllers/lvmcluster_controller.go

+					r.Log.Error(err, "failed to verify device paths")
+
+					setLVMClusterCRvalidCondition(&instance.Status.Conditions, corev1.ConditionFalse, "CRInvalid", err.Error())
+					return err


shouldn't we use setLVMClusterCRvalidCondition when there's a verify issue?

We are setting the condition in both cases either in failure or in success.

Can you pls elaborate on what you mean?

I need more coffee :D sorry

pkg/vgmanager/vgmanager_controller.go

mulbc · 2022-08-26T07:50:41Z

pkg/vgmanager/vgmanager_controller.go

+					return []internal.BlockDevice{}, err
+				}
+			} else {
+				err = fmt.Errorf("unsupported disk path format %s", path)


please add to the error message that only /dev/disk/by-path and /dev/ links are currently supported

mulbc · 2022-08-26T07:51:38Z

pkg/vgmanager/vgmanager_controller.go

+
+			blockDevice, ok := hasExactDisk(blockDevices, diskName)
+
+			if filepath.Dir(path) == internal.DiskByPathPrefix {


why can't we use this flow for all /dev/disk/by-* options?

We can, We need more conditions to do that. Also as of now, all other patterns other than diskbypath will go into the else condition starting with dev.

mulbc · 2022-08-26T08:12:56Z

pkg/vgmanager/vgmanager_controller.go

+		}
+
+		devices := []string{}
+		if diskPattern == internal.DiskByPathPrefix {


idea: why not always use the disk path in the status?!

I had this discussion earlier with @nbalacha and we came to the conclusion that we will show the paths as given in the spec. It will be easier for a user to compare them from the spec.

controllers/lvmcluster_controller.go

pkg/vgmanager/vgmanager_controller.go

make sure device paths does not overlap with another VG or within the same VG. Signed-off-by: Nitin Goyal <nigoyal@redhat.com>

Signed-off-by: Nitin Goyal <nigoyal@redhat.com>

implement filterMatchingDevices Signed-off-by: Nitin Goyal <nigoyal@redhat.com>

if VolumeGroup CR uses paths add paths in the devices else add names Signed-off-by: Nitin Goyal <nigoyal@redhat.com>

update the error in the reason Signed-off-by: Nitin Goyal <nigoyal@redhat.com>

Signed-off-by: Nitin Goyal <nigoyal@redhat.com>

openshift-ci · 2022-08-31T10:52:06Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [nbalacha]

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

iamniting requested review from nbalacha, sp98, riya-singhal31 and Yuggupta27 July 18, 2022 05:04

sp98 suggested changes Jul 19, 2022

View reviewed changes

api/v1alpha1/lvmcluster_types.go Show resolved Hide resolved

api/v1alpha1/lvmcluster_types.go Outdated Show resolved Hide resolved

api/v1alpha1/lvmvolumegroup_types.go Outdated Show resolved Hide resolved

openshift-ci bot assigned sp98 Jul 19, 2022

iamniting force-pushed the api branch 4 times, most recently from a469dce to deb1d65 Compare July 22, 2022 10:21

iamniting force-pushed the api branch 2 times, most recently from 3bc331d to 66bdc36 Compare July 27, 2022 10:15

iamniting changed the title ~~feat: make API changes to support manual disk selection~~ feat: make API and controller changes to support manual disk selection Jul 27, 2022

sp98 suggested changes Jul 28, 2022

View reviewed changes

nbalacha suggested changes Aug 4, 2022

View reviewed changes

openshift-ci bot assigned nbalacha Aug 4, 2022

sp98 reviewed Aug 4, 2022

View reviewed changes

iamniting force-pushed the api branch from 2f5305f to 53e4c9e Compare August 5, 2022 07:46

iamniting requested review from nbalacha and sp98 August 5, 2022 14:12

sp98 reviewed Aug 8, 2022

View reviewed changes

iamniting force-pushed the api branch from 53e4c9e to 729154f Compare August 8, 2022 06:08

iamniting requested a review from sp98 August 8, 2022 06:11

sp98 suggested changes Aug 17, 2022

View reviewed changes

nbalacha reviewed Aug 18, 2022

View reviewed changes

controllers/lvmcluster_controller.go Outdated Show resolved Hide resolved

iamniting force-pushed the api branch 3 times, most recently from e1fda0d to a0a6098 Compare August 22, 2022 07:29

iamniting force-pushed the api branch from a4daf16 to e7fbc44 Compare August 25, 2022 05:46

iamniting requested a review from nbalacha August 25, 2022 06:19

iamniting force-pushed the api branch 3 times, most recently from 500639b to a0323b7 Compare August 26, 2022 06:55

mulbc reviewed Aug 26, 2022

View reviewed changes

pkg/vgmanager/vgmanager_controller.go Outdated Show resolved Hide resolved

mulbc reviewed Aug 26, 2022

View reviewed changes

iamniting force-pushed the api branch from a0323b7 to f164a79 Compare August 29, 2022 04:20

nbalacha reviewed Aug 30, 2022

View reviewed changes

controllers/lvmcluster_controller.go Outdated Show resolved Hide resolved

pkg/vgmanager/vgmanager_controller.go Show resolved Hide resolved

iamniting force-pushed the api branch from f164a79 to 7eb851c Compare August 30, 2022 05:38

iamniting added 8 commits August 30, 2022 12:58

feat: verify LvmCluster Spec before proceeding for reconcile

98639c6

make sure device paths does not overlap with another VG or within the same VG. Signed-off-by: Nitin Goyal <nigoyal@redhat.com>

feat: mutate LVMVolumeGroup spec inside CreateOrUpdate

572adbf

Signed-off-by: Nitin Goyal <nigoyal@redhat.com>

feat: implement logic to select the devices from the volumeGroup CR

33c24d7

implement filterMatchingDevices Signed-off-by: Nitin Goyal <nigoyal@redhat.com>

feat: generateVolumeGroupNodeStatus as of VolumeGroup CR

2a58d2a

if VolumeGroup CR uses paths add paths in the devices else add names Signed-off-by: Nitin Goyal <nigoyal@redhat.com>

feat: reflect error in the lvmvolumegroupnodestatus CR

3c324bd

update the error in the reason Signed-off-by: Nitin Goyal <nigoyal@redhat.com>

feat: update reason as well in the lvmcluster status

3198bcf

Signed-off-by: Nitin Goyal <nigoyal@redhat.com>

fix: list full paths while listing via lsblk --paths

a785c63

Signed-off-by: Nitin Goyal <nigoyal@redhat.com>

feat: extend the thinpool

3536b77

Signed-off-by: Nitin Goyal <nigoyal@redhat.com>

iamniting force-pushed the api branch from 7eb851c to 3536b77 Compare August 30, 2022 07:28

iamniting requested review from nbalacha and mulbc August 30, 2022 07:31

nbalacha approved these changes Aug 31, 2022

View reviewed changes

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

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 31, 2022

openshift-merge-robot merged commit e022411 into openshift:main Aug 31, 2022

	err := fmt.Errorf("can not find disk name %s in the available block devices", path)
	err := fmt.Errorf("can not find disk name %s in the available block devices", diskName)


		blockDevice, ok := hasExactDisk(blockDevices, diskName)

		if filepath.Dir(path) == internal.DiskByPathPrefix {

feat: make API and controller changes to support manual disk selection #229

feat: make API and controller changes to support manual disk selection #229

Conversation

iamniting commented Jul 18, 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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sp98 left a 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

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

openshift-ci bot commented Aug 31, 2022

iamniting commented Jul 18, 2022 •

edited

Loading