feat: cleanup volume groups on CR deletion #53

leelavg · 2021-12-30T06:46:04Z

Signed-off-by: Leela Venkaiah G lgangava@redhat.com

sp98 · 2021-12-30T07:57:43Z

pkg/vgmanager/cleanup.go

+	// get limited number of logical volume CRs to verify their presence
+	lvs := &topolvmv1.LogicalVolumeList{}
+	if err := r.Client.List(ctx, lvs, &crc.ListOptions{Limit: int64(1)}); err == nil {
+		// do not proceed with cleanup if there are logicalVolumes


Aren't we suppose to check if the LV CR is mapped with the VG somehow? Right now we are aborting VG deletion if at least one LV CR is found. This won't be very useful in case of the cleaning up individual VGs (for example user removes a VG from the spec)

that can be tweaked but I don't want to do that in this PR

the flow will be more or less same however in controller itself there'll be some extra checks as well

so the question would be even if the user is deleting the entire cluster, do we want to skip all VG deletion even if one LV CR is available? or we delete whatever VGs can be deleted in the cluster?

Yes, until we have implementation of device selector and specific vg deletion, I'd like to abort any deletion.

When we implement the option 2, we can spend time on extra processing.

It would be better to have a vgCleanupAll function which calls a vgCleanup (vgname) for each vg.

the current function does the same, it'll remove volume group one by one based on vgName in lvmd config file

sp98 · 2021-12-30T08:01:50Z

pkg/vgmanager/vgmanager_controller.go

+	}
+
+	// set node uid as finalizer on CR
+	if !controllerutil.ContainsFinalizer(lvmCluster, vgFinalizer) {


Can't we use NodeName itself as the finalizer ?

IMHO, UID is the standard way for these type of tasks.

We can customize the finalizer to have prefix node-uid/<uid> for easy ref

pkg/vgmanager/vgmanager_controller.go

pkg/vgmanager/lvm.go

controllers/vgmanager_daemonset.go

nbalacha · 2021-12-30T10:01:07Z

pkg/vgmanager/cleanup.go

+	// get limited number of logical volume CRs to verify their presence
+	lvs := &topolvmv1.LogicalVolumeList{}
+	if err := r.Client.List(ctx, lvs, &crc.ListOptions{Limit: int64(1)}); err == nil {
+		// do not proceed with cleanup if there are logicalVolumes


It would be better to have a vgCleanupAll function which calls a vgCleanup (vgname) for each vg.

nbalacha · 2021-12-30T10:05:53Z

pkg/vgmanager/cleanup.go

+	}
+
+	// get all the volume groups from lvmd file
+	lvmdConfig := &lvmCMD.Config{


This should be a global var in the package or an argument to the function. Or an arg in the reconciler.

why should this be the case? my rationale is, if we fail to delete any VG then we need to read remaining content from config file in next reconcile and then this struct has to be re-declared again.

This struct should be the same one created by the VGmanager when creating the file. Don't regenerate it each time.

Actually scratch the last comment. What we should be doing is saving the socket and filename that was used when the file was created and using those in the struct.

pkg/vgmanager/cleanup.go

pkg/vgmanager/vgmanager_controller.go

nbalacha · 2021-12-30T10:20:45Z

pkg/vgmanager/lvm.go

+func RemoveVolumeGroup(name string) error {
+
+	// find volume group which contains name and list of PVs belonging to VG
+	vg, err := FindVolumeGroup(name)


Unrelated to this pr but don't export this function if it is not being used outside the pkg.

there are many functions like this, will take this in a separate PR

sp98 · 2022-01-06T12:56:04Z

pkg/vgmanager/lvm_test.go

+var fileName = "vg-pool-"
+var vgName = "test-vg"
+
+func setup(name string) string { //nolint:golint,unused


is the test creating a new VG and deleting that?
If yes, then do we really need to do that?

Currently this test will not work in CI as nsenter doesn't work and being skipped, need to refactor that to be testable

The intention is to create a volume group manually (eventually do that via vgmanager functions) and remove that via function being used in this PR, incase if deletion fails teardown will remove it quietly

pkg/vgmanager/cleanup.go

jmolmo

It seems ok to me, just take a look to my comment about the error log when findvolume fails.

pkg/vgmanager/cleanup.go

openshift-ci · 2022-01-07T11:26:45Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

openshift-ci · 2022-01-10T07:48:36Z

New changes are detected. LGTM label has been removed.

- Remove volumes groups on the node when LVMCluster CR is deleted - Remove LVM PVs which are part of volume group as well - On every reconcile try to delete VGs sequentially and on failure update the lvmd config file and retry - Do not touch VGs that are not created by VG Manager Signed-off-by: Leela Venkaiah G <lgangava@redhat.com>

sp98

So we are always assuming that we would delete all the VGs when the user uninstalls lvm operator. Can there be scenarios where user wants to delete the operator, but still want to use Persistent Volumes created by operator. That is, delete the operator but don't delete any data.

sp98 · 2022-01-11T03:24:51Z

pkg/vgmanager/cleanup.go

+		// not able to start cleanup, no need to overwrite config file, just return error
+		return fmt.Errorf("failed to initiate cleanup of Volume Groups")
+	} else if done == len(lvmdConfig.DeviceClasses)-1 || len(lvmdConfig.DeviceClasses) == 0 {
+		// every volume group is deleted and config file can be deleted


When one of the vg deletion request fails, will done be still equal to len(lvmdConfig.DeviceClasses)-1 and we will still remove the config file ?

sp98 · 2022-01-11T03:30:24Z

pkg/vgmanager/cleanup.go

+		for _, dc := range lvmdConfig.DeviceClasses {
+			pending = append(pending, dc.Name)
+		}
+		r.Log.Info("device classes are pending for deletion", "deviceClasses", pending)


Do we really need another list pending over here or can be just use lvmdConfig.DeviceClasses again?

Suggested change

r.Log.Info("device classes are pending for deletion", "deviceClasses", pending)

r.Log.Info("device classes are pending for deletion", "deviceClasses", lvmdConfig.DeviceClasses)

sp98 · 2022-01-12T07:29:10Z

pkg/vgmanager/vgmanager_controller.go

+
+	// set node uid as finalizer on CR
+	if !controllerutil.ContainsFinalizer(lvmCluster, vgFinalizer) {
+		controllerutil.AddFinalizer(lvmCluster, vgFinalizer)


If each vgManager daemonset pod is adding its own finalizer to the LVMCluster CR, then we might have issues in cleaning up of the finalizers. For example: if NodeSelector of the Vgmanager daemonset is updated, we might have stale finalizers in the LVMCluster CR finalizer.

openshift-ci · 2022-01-14T02:10:11Z

@leelavg: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

leelavg · 2022-01-17T05:03:26Z

closing in favour of Update Volume Group utility #73

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 30, 2021

leelavg requested review from sp98 and nbalacha and removed request for sp98 and nbalacha December 30, 2021 07:14

sp98 reviewed Dec 30, 2021

View reviewed changes

nbalacha suggested changes Dec 30, 2021

View reviewed changes

openshift-ci bot assigned nbalacha Dec 30, 2021

openshift-ci bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 1, 2022

leelavg marked this pull request as ready for review January 6, 2022 11:26

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 Jan 6, 2022

sp98 reviewed Jan 6, 2022

View reviewed changes

jmolmo reviewed Jan 7, 2022

View reviewed changes

pkg/vgmanager/cleanup.go Show resolved Hide resolved

jmolmo suggested changes Jan 7, 2022

View reviewed changes

pkg/vgmanager/cleanup.go Show resolved Hide resolved

openshift-ci bot assigned jmolmo Jan 7, 2022

jmolmo approved these changes Jan 7, 2022

View reviewed changes

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

leelavg requested a review from nbalacha January 10, 2022 06:30

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

sp98 reviewed Jan 11, 2022

View reviewed changes

sp98 reviewed Jan 12, 2022

View reviewed changes

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 14, 2022

leelavg closed this Jan 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: cleanup volume groups on CR deletion #53

feat: cleanup volume groups on CR deletion #53

leelavg commented Dec 30, 2021

sp98 Dec 30, 2021

leelavg Dec 30, 2021

sp98 Dec 30, 2021

leelavg Dec 30, 2021

nbalacha Dec 30, 2021

leelavg Dec 30, 2021

sp98 Dec 30, 2021

leelavg Dec 30, 2021

nbalacha Dec 30, 2021

nbalacha Dec 30, 2021

leelavg Jan 5, 2022

nbalacha Jan 5, 2022

nbalacha Jan 5, 2022

nbalacha Dec 30, 2021

leelavg Jan 5, 2022

sp98 Jan 6, 2022

leelavg Jan 6, 2022

jmolmo left a comment

openshift-ci bot commented Jan 7, 2022

openshift-ci bot commented Jan 10, 2022

sp98 left a comment

sp98 Jan 11, 2022

sp98 Jan 11, 2022

sp98 Jan 12, 2022

openshift-ci bot commented Jan 14, 2022

leelavg commented Jan 17, 2022

	r.Log.Info("device classes are pending for deletion", "deviceClasses", pending)
	r.Log.Info("device classes are pending for deletion", "deviceClasses", lvmdConfig.DeviceClasses)

feat: cleanup volume groups on CR deletion #53

feat: cleanup volume groups on CR deletion #53

Conversation

leelavg commented Dec 30, 2021

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

jmolmo left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Jan 7, 2022

openshift-ci bot commented Jan 10, 2022

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

openshift-ci bot commented Jan 14, 2022

leelavg commented Jan 17, 2022