feat: cleanup lvm resources on delete #146

nbalacha · 2022-04-06T15:04:45Z

The vgmanager will now remove the vg and the lvmd.conf
file when the LVMCluster is deleted.

Signed-off-by: N Balachandran [email protected]

sp98 · 2022-04-12T08:27:30Z

controllers/lvm_volumegroup.go

+			// Has the VG been cleaned up on all hosts?
+			exists, _ := doesVGExistOnHosts(ctx, existingLvmVg.Name, lvmCluster)
+			if !exists {
+				//remove finalizer


Suggested change

//remove finalizer

// remove finalizer

sp98 · 2022-04-12T08:39:55Z

controllers/lvmcluster_controller.go

-				r.Log.Info("failed to remove finalizer from LvmCluster")
-				return reconcile.Result{}, err
-			}
+		//Check for existing LogicalVolumes


Suggested change

//Check for existing LogicalVolumes

// Check for existing LogicalVolumes

sp98 · 2022-04-12T08:41:01Z

controllers/lvmcluster_controller.go

+		//Check for existing LogicalVolumes
+		lvsExist, err := r.logicalVolumesExist(ctx, instance)
+		if err != nil {
+			r.Log.Error(err, "failed to get LogicalVolumeList")


Suggested change

r.Log.Error(err, "failed to get LogicalVolumeList")

r.Log.Error(err, "failed to check if logicalVolumes exist")

sp98 · 2022-04-12T08:46:52Z

controllers/lvmcluster_controller.go

+		//Check for existing LogicalVolumes
+		lvsExist, err := r.logicalVolumesExist(ctx, instance)
+		if err != nil {
+			r.Log.Error(err, "failed to get LogicalVolumeList")


return this error?

leelavg

there are some commented out log lines, can those be removed?

leelavg · 2022-04-12T08:15:07Z

controllers/lvm_volumegroup.go

@@ -27,7 +28,8 @@ import (
 )

 const (
-	lvmVGName = "lvmvg-manager"
+	lvmVGName      = "lvmvg-manager"
+	lvmvgFinalizer = "lvm.openshift.com/volumegroup"


minor: i think openshift.io is used more often

leelavg · 2022-04-12T08:17:54Z

controllers/lvmcluster_controller.go

@@ -115,14 +118,15 @@ func (r *LVMClusterReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 	result, reconcileError := r.reconcile(ctx, lvmCluster)

 	statusError := r.updateLVMClusterStatus(ctx, lvmCluster)
-	if statusError != nil {
+	/* 	if statusError != nil {


commented out block can be removed?

leelavg · 2022-04-12T08:22:02Z

controllers/lvmcluster_controller.go

+
+	logicalVolumeList := &topolvmv1.LogicalVolumeList{}
+
+	if err := r.Client.List(ctx, logicalVolumeList); err != nil {


Suggested change

if err := r.Client.List(ctx, logicalVolumeList); err != nil {

if err := r.Client.List(ctx, logicalVolumeList, &client.ListOptions{Limit: int64(1)}); err != nil {

What is the advantage of this?

we are checking the existence of logicalvolume crs so why not limit the request to list only one item

Does it make any difference from a performance point of view?

Afaik, yes as the underlying rest call will be paginated.

leaving this then, even though we might be reading from cache I think limiting entries doesn't harm anything for the operation that we are doing in this func

leelavg · 2022-04-12T08:31:00Z

pkg/vgmanager/vgmanager_controller.go

 			r.Log.Error(statuserr, "failed to update status", "VGName", volumeGroup.Name)
+			return reconcileAgain, nil


shouldn't nil be statuserr here?

same comment in this file

leelavg · 2022-04-12T08:32:46Z

pkg/vgmanager/vgmanager_controller.go

@@ -208,11 +216,11 @@ func (r *VGReconciler) reconcile(ctx context.Context, req ctrl.Request, volumeGr
 		r.Log.Info("updated lvmd config", "VGName", volumeGroup.Name)
 	}

-	volGrpHostInfo, err := GetVolumeGroup(r.executor, volumeGroup.Name)
+	// volGrpHostInfo, err := GetVolumeGroup(r.executor, volumeGroup.Name)


this wasn't being used even before this PR?

leelavg · 2022-04-12T08:37:14Z

pkg/vgmanager/vgmanager_controller.go

+	return vgNodeStatus, nil
+}
+
+func (r *VGReconciler) updateStatus(ctx context.Context, lvmdConfig *lvmdCMD.Config) error {


I don't see lvmdConfig is being used directly in this func or other funcs being called in this func

If it's intentional we don't need this param at all?

I was thinking this can be used to filter the vgs that lvmo is managing?

I had initially iterated over the entries in lvmconf to get the VGs but moved directly to querying the VGs from the host.( when I asked about tagging the lvm objects the LVMO created) . I forgot to remove the argument.
I dropped the idea of using the lvmdconfig to get the list of VGs because that may not be consistent (in case writing to the file failed).

ack, makes sense, we can use tags then if that's still an options, maybe in another pr.

Updated the function signatures.

leelavg · 2022-04-12T08:40:34Z

pkg/vgmanager/vgmanager_controller.go

+	if err != nil {
+		r.Log.Error(err, "failed to create or update lvmvolumegroupnodestatus", "name", vgNodeStatus.Name)
+		return err
+	} else if result != controllerutil.OperationResultNone {


any reason for not checking against OperationResultUpdated

result != controllerutil.OperationResultNone means the object was either created or modified. IMO, there is no need to check in greater detail.

ack, I read the log lvmvolumegroupnodestatus modified and thought this condition is checking solely the modification.

leelavg · 2022-04-12T08:45:27Z

pkg/vgmanager/vgmanager_controller.go

+		if err != ErrVolumeGroupNotFound {
+			return fmt.Errorf("failed to get volume group. %v, %v", volumeGroup.Name, err)
+		}
+		return nil


why can't directly return err?

if not above, why return nil in line 268?

why can't directly return err?
There would not be a log message then.

if not above, why return nil in line 268?
If the error is ErrVolumeGroupNotFound , there is nothing more to be done.

ErrVolumeGroupNotFound , there is nothing more to be done.

ya, missed this detail

sp98 · 2022-04-12T08:52:22Z

pkg/vgmanager/vgmanager_controller.go

@@ -108,6 +108,12 @@ var reconcileAgain ctrl.Result = ctrl.Result{Requeue: true, RequeueAfter: reconc

 func (r *VGReconciler) reconcile(ctx context.Context, req ctrl.Request, volumeGroup *lvmv1alpha1.LVMVolumeGroup) (ctrl.Result, error) {

+	//The LVMVolumeGroup resource was deleted


Suggested change

//The LVMVolumeGroup resource was deleted

// The LVMVolumeGroup resource was deleted

sp98 · 2022-04-12T08:52:46Z

pkg/vgmanager/vgmanager_controller.go

@@ -226,6 +234,71 @@ func (r *VGReconciler) reconcile(ctx context.Context, req ctrl.Request, volumeGr
 	return ctrl.Result{RequeueAfter: requeueAfter}, nil
 }

+func (r *VGReconciler) processDelete(ctx context.Context, volumeGroup *lvmv1alpha1.LVMVolumeGroup) error {
+
+	//Read the lvmd config file


Suggested change

//Read the lvmd config file

// Read the lvmd config file

sp98 · 2022-04-12T08:54:56Z

pkg/vgmanager/vgmanager_controller.go

+
+	err = vg.Delete(r.executor)
+	if err != nil {
+		return fmt.Errorf("failed to delete volume group. %v, %v", volumeGroup.Name, err)


Suggested change

return fmt.Errorf("failed to delete volume group. %v, %v", volumeGroup.Name, err)

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

leelavg · 2022-04-12T10:34:40Z

controllers/lvm_volumegroup.go

@@ -113,3 +136,14 @@ func (c lvmVG) getLvmVolumeGroups(r *LVMClusterReconciler, instance *lvmv1alpha1
 	}
 	return lvmVolumeGroups
 }
+
+func doesVGExistOnHosts(ctx context.Context, volumeGroup string, instance *lvmv1alpha1.LVMCluster) (bool, error) {


it seems error is always nil and can be removed

not a time limited func so ctx can be removed as well

removed error.

leelavg · 2022-04-12T10:40:55Z

controllers/lvm_volumegroup.go

+
+	dcStatuses := instance.Status.DeviceClassStatuses
+	for _, dc := range dcStatuses {
+		if dc.Name == volumeGroup {


only for my understanding, we are finding whether VG exists or not by means of status of lvmcluster cr which inturn gets the info from groupnodestatus cr right?

is it possible to club volumegroup and groupnodestatus crd? (if past discussions were a no, then nntr)

only for my understanding, we are finding whether VG exists or not by means of status of lvmcluster cr which inturn gets the info from groupnodestatus cr right?
Correct

is it possible to club volumegroup and groupnodestatus crd? (if past discussions were a no, then nntr)
Can you elaborate?

Can you elaborate?

not related to this PR, can be taken up separately

in this single function either directly or indirectly we are using all the three CRs with some duplicate info, even though we discussed in length about the current state, if possible we want to re-look in reducing the duplicate info there by reducing num of CRs

leelavg · 2022-04-12T10:46:01Z

controllers/lvmcluster_controller.go

+
+	logicalVolumeList := &topolvmv1.LogicalVolumeList{}
+
+	if err := r.Client.List(ctx, logicalVolumeList); err != nil {


we are checking the existence of logicalvolume crs so why not limit the request to list only one item

This commit cleans up the LVM resources created by the vgmanager when the LVMCluster is deleted. All PVCs provisioned by topolvm must be deleted before the LVMCluster is deleted. The LVMCluster controller will wait until there are no existing LogicalVolumes before it proceeds to remove the resources. When the LVMCluster is deleted, the LVMCluster controller deletes the LVMVolumeGroup resource. The resource is not removed immediately because of the finalizer. The vg-manager deletes the vg on the host and updates the LVMVolumeGroupNodeStatus, which triggers the LVMCluster reconciliation to update the status. When the VG is removed from the LVMCluster.status, the LVMCluster controller removes the finalizer on the LVMVolumeGroup. Signed-off-by: N Balachandran <[email protected]>

leelavg

/lgtm

leelavg · 2022-04-13T04:45:59Z

controllers/lvm_volumegroup.go

+
+	dcStatuses := instance.Status.DeviceClassStatuses
+	for _, dc := range dcStatuses {
+		if dc.Name == volumeGroup {


Can you elaborate?

not related to this PR, can be taken up separately

in this single function either directly or indirectly we are using all the three CRs with some duplicate info, even though we discussed in length about the current state, if possible we want to re-look in reducing the duplicate info there by reducing num of CRs

leelavg · 2022-04-13T04:47:45Z

controllers/lvmcluster_controller.go

+
+	logicalVolumeList := &topolvmv1.LogicalVolumeList{}
+
+	if err := r.Client.List(ctx, logicalVolumeList); err != nil {


leaving this then, even though we might be reading from cache I think limiting entries doesn't harm anything for the operation that we are doing in this func

openshift-ci · 2022-04-13T04:49:53Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

Yuggupta27 · 2022-04-13T04:47:52Z

controllers/lvmcluster_controller.go


 	// Reconcile errors have higher priority than status update errors
 	if reconcileError != nil {
 		return result, reconcileError
-	} else if statusError != nil && errors.IsNotFound(statusError) {
+	} else if statusError != nil && !errors.IsNotFound(statusError) {


Do we don't need to return errors in the case of NotFound scenarios now?

I think this was originally a mistake (as Nitin pointed out).

openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 6, 2022

sp98 suggested changes Apr 12, 2022

View reviewed changes

openshift-ci bot assigned sp98 Apr 12, 2022

leelavg reviewed Apr 12, 2022

View reviewed changes

sp98 suggested changes Apr 12, 2022

View reviewed changes

leelavg reviewed Apr 12, 2022

View reviewed changes

nbalacha changed the title ~~[WIP] feat: cleanup lvm resources on delete~~ feat: cleanup lvm resources on delete Apr 12, 2022

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 12, 2022

leelavg approved these changes Apr 13, 2022

View reviewed changes

openshift-ci bot assigned leelavg Apr 13, 2022

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

openshift-merge-robot merged commit 2366c24 into openshift:main Apr 13, 2022

Yuggupta27 reviewed Apr 13, 2022

View reviewed changes

	//Check for existing LogicalVolumes
	// Check for existing LogicalVolumes

	r.Log.Error(err, "failed to get LogicalVolumeList")
	r.Log.Error(err, "failed to check if logicalVolumes exist")


		logicalVolumeList := &topolvmv1.LogicalVolumeList{}

		if err := r.Client.List(ctx, logicalVolumeList); err != nil {

		r.Log.Error(statuserr, "failed to update status", "VGName", volumeGroup.Name)
		return reconcileAgain, nil

		@@ -108,6 +108,12 @@ var reconcileAgain ctrl.Result = ctrl.Result{Requeue: true, RequeueAfter: reconc

		func (r VGReconciler) reconcile(ctx context.Context, req ctrl.Request, volumeGroup lvmv1alpha1.LVMVolumeGroup) (ctrl.Result, error) {

		//The LVMVolumeGroup resource was deleted

	//The LVMVolumeGroup resource was deleted
	// The LVMVolumeGroup resource was deleted

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

feat: cleanup lvm resources on delete #146

feat: cleanup lvm resources on delete #146

Conversation

nbalacha commented Apr 6, 2022

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

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

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

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

openshift-ci bot commented Apr 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment