feat: adds the LVMVolumeGroup CRD and processing logic #72

nbalacha · 2022-01-14T12:44:09Z

The LVMCluster reconciliation will now create individual LVMVolumeGroup objects for each deviceClass. The VGManager will now reconcile the LVMVolumeGroup instead of the LVMCluster CR.

sp98 · 2022-01-14T13:11:31Z

controllers/lvm_volumegroup.go

+	return lvmvgName
+}
+
+func (c lvmVG) ensureCreated(r *LVMClusterReconciler, ctx context.Context, lvmCluster *lvmv1alpha1.LVMCluster) error {


(Not related to this PR). Just realised the context should usually be the first argument of the function.

sp98 · 2022-01-14T13:23:32Z

cmd/vgmanager/main.go

-		Scheme: mgr.GetScheme(),
+		Client:   mgr.GetClient(),
+		Scheme:   mgr.GetScheme(),
+		Nodename: os.Getenv("NODE_NAME"),


do we need Nodename in the VGReconciler object ? It s used only when we update the status. So we can directly call os.Getenv("NODE_NAME") over there.

I don't see a problem with saving it. Similar to how we save the namespace elsewhere.

ok. name should be NodeName.

sp98 · 2022-01-14T13:26:43Z

pkg/vgmanager/vgmanager_controller.go

@@ -46,7 +46,7 @@ const (
 func (r *VGReconciler) SetupWithManager(mgr ctrl.Manager) error {
 	r.deviceAgeMap = newAgeMap(&wallTime{})
 	return ctrl.NewControllerManagedBy(mgr).
-		For(&lvmv1alpha1.LVMCluster{}).
+		For(&lvmv1alpha1.LVMVolumeGroup{}).


Will creation of a new LVMVolumeGroup trigger controller reconcile for all the other existing LVMVolumeGroups?
If yes, then I think we should avoid that.

No, the request will have the name of the object it was triggered for.

jmolmo

Just minor things related with the use of Caps letters in log messages

jmolmo · 2022-01-14T13:20:12Z

cmd/vgmanager/main.go

-		Scheme: mgr.GetScheme(),
+		Client:   mgr.GetClient(),
+		Scheme:   mgr.GetScheme(),
+		Nodename: os.Getenv("NODE_NAME"),


Where is set this env. variable?

jmolmo · 2022-01-14T13:24:04Z

controllers/lvm_volumegroup.go

+				r.Log.Info("LVMVG deleted", "Name", vgcr.Name)
+				return nil
+			}
+			r.Log.Error(err, "failed to retrieve LVMVG resource", "Name", vgcr.Name)


":s/Name/name" in log lines

The Name is the fieldname. shouldn't that be capitalized?

Its not consistent through out. Both name and Name are used in the logs.

Ack. Changing it to "name".

jmolmo · 2022-01-14T13:24:26Z

controllers/lvm_volumegroup.go

+		if err != nil {
+			// already deleted in previous reconcile
+			if errors.IsNotFound(err) {
+				r.Log.Info("LVMVG deleted", "Name", vgcr.Name)


":s/Name/name"

jmolmo · 2022-01-14T13:25:26Z

controllers/lvm_volumegroup.go

+		// if not deleted, initiate deletion
+		if existingLvmVg.GetDeletionTimestamp().IsZero() {
+			if err = r.Client.Delete(ctx, existingLvmVg); err != nil {
+				r.Log.Error(err, "failed to delete LVMVG", "Name", existingLvmVg.Name)


jmolmo · 2022-01-14T13:25:40Z

controllers/lvm_volumegroup.go

+				r.Log.Error(err, "failed to delete LVMVG", "Name", existingLvmVg.Name)
+				return err
+			} else {
+				r.Log.Info("initiated LVMVG deletion", "Name", existingLvmVg.Name)


jmolmo · 2022-01-14T13:27:50Z

pkg/vgmanager/vgmanager_controller.go

-	err := r.Client.Get(ctx, req.NamespacedName, lvmCluster)
+
+	var status string = "available"
+	var reason string = "Success"


":/s/Success/success"

Status strings are usually capitalized.

jmolmo · 2022-01-14T13:28:45Z

pkg/vgmanager/vgmanager_controller.go

+	var matchingDevices []internal.BlockDevice
+	_, matchingDevices, err = filterMatchingDevices(remainingValidDevices, volumeGroup)
+	if err != nil {
+		r.Log.Error(err, "could not filter matching Devices", "VGName", volumeGroup.Name)


":S/Devices/devices"

jmolmo · 2022-01-14T13:29:32Z

pkg/vgmanager/vgmanager_controller.go

+			reason = "failed to create VG"
+		} else {
+			status = "degraded"
+			reason = "Failed to extend VG"


":S/Failed/failed"

Status messages are usually capitalized.
Eg. This is a condition in a deployment status:

lastTransitionTime: "2022-01-14T13:48:03Z"
lastUpdateTime: "2022-01-14T13:48:03Z"
message: Deployment has minimum availability.
reason: MinimumReplicasAvailable
status: "True"

sp98 · 2022-01-14T13:32:08Z

controllers/lvm_volumegroup.go

+	return nil
+}
+
+func (c lvmVG) getLvmVolumeGroups(r *LVMClusterReconciler, instance *lvmv1alpha1.LVMCluster) ([]*lvmv1alpha1.LVMVolumeGroup, error) {


We can only pass the namespace instead of entire LVMClusterReconciler, if we only plan to use the namespace

I'd like to address that in a separate PR.

sp98 · 2022-01-14T13:33:59Z

controllers/lvm_volumegroup.go

+
+	deviceClasses := instance.Spec.DeviceClasses
+	for _, deviceClass := range deviceClasses {
+		lvmVolumeGroup := &lvmv1alpha1.LVMVolumeGroup{


how do we decide which deviceClass is default?

That will need to be changed in the CRD design. I will take that up in a separate PR.

sp98 · 2022-01-14T13:34:28Z

controllers/lvm_volumegroup.go

+		lvmVolumeGroups = append(lvmVolumeGroups, lvmVolumeGroup)
+	}
+	return lvmVolumeGroups, nil
+


blank line.

sp98 · 2022-01-14T13:35:47Z

controllers/lvm_volumegroup.go

+)
+
+const (
+	lvmvgName = "lvmvg-manager"


Suggested change

lvmvgName = "lvmvg-manager"

lvmVGName = "lvmvg-manager"

sp98 · 2022-01-14T13:44:41Z

controllers/lvm_volumegroup.go

+		})
+
+		if err != nil {
+			r.Log.Error(err, "LvmVolumeGroup reconcile failure", "name", volumeGroup.Name)


For consistency

Suggested change

r.Log.Error(err, "LvmVolumeGroup reconcile failure", "name", volumeGroup.Name)

r.Log.Error(err, "failed to reconcile LvmVolumeGroup", "name", volumeGroup.Name)

sp98 · 2022-01-14T13:49:09Z

controllers/lvm_volumegroup.go

+
+	for _, vgcr := range vgcrs {
+		err := r.Client.Get(ctx, types.NamespacedName{Name: vgcr.Name, Namespace: vgcr.Namespace}, existingLvmVg)
+


blank line.

sp98 · 2022-01-14T13:51:42Z

controllers/lvm_volumegroup.go

+			// already deleted in previous reconcile
+			if errors.IsNotFound(err) {
+				r.Log.Info("LVMVG deleted", "Name", vgcr.Name)
+				return nil


shouldn't we continue here rather than returning nil ?

sp98 · 2022-01-14T13:57:34Z

controllers/lvm_volumegroup.go

+	return nil
+}
+
+func (c lvmVG) getLvmVolumeGroups(r *LVMClusterReconciler, instance *lvmv1alpha1.LVMCluster) ([]*lvmv1alpha1.LVMVolumeGroup, error) {


getLvmVolumeGroups does not return any error. Should remove error from the return types.

sp98 · 2022-01-14T14:00:41Z

controllers/lvm_volumegroup.go

+		if err != nil {
+			// already deleted in previous reconcile
+			if errors.IsNotFound(err) {
+				r.Log.Info("LVMVG deleted", "Name", vgcr.Name)


logs should be more consistent. ensureDeleted uses LVMVG in the log messages while ensureCreated uses LvmVolumeGroup

sp98 · 2022-01-14T14:07:38Z

controllers/lvm_volumegroup.go

+			if err = r.Client.Delete(ctx, existingLvmVg); err != nil {
+				r.Log.Error(err, "failed to delete LVMVG", "Name", existingLvmVg.Name)
+				return err
+			} else {


This else block can be removed.

sp98 · 2022-01-14T14:33:28Z

pkg/vgmanager/vgmanager_controller.go

+			found = true
+			nodeStatus.Status = status
+			nodeStatus.Reason = reason
+		}


break for loop once NodeName is found.

sp98 · 2022-01-14T14:38:54Z

pkg/vgmanager/vgmanager_controller.go

+			nodeStatus.Reason = reason
+		}
+	}
+	if !found {


I don't think it will update the Status when the NodeName was found but it has different Status or Reason. So we should add status if NodeName not found and update status if NodeName is found but its Status or Reason are different.

Didn't understand this.

We are only updating the instance.Status.NodeStatus in line 307, inside the if !found block. What if found is true but Status and/or Reason is different. In this case, the Status and Reason will never be updated.

The status is being updated in the original array if the node is found. append should create a new slice which is why it is being assigned again.

sp98 · 2022-01-14T14:45:10Z

pkg/vgmanager/vgmanager_controller.go

-				VolumeGroup: deviceClass.Name,
-				Default:     true,
-			})
+			status = "unavailable"


Status should be used as constants (enums) so that its easier to know what all statuses are available.

That can be taken up in a different PR.

The LVM operator will create individual LVMVolumeGroup objects for the deviceClasses defined in the LVMCluster resource. Signed-off-by: N Balachandran <[email protected]>

This commit adds the LVMVolumeGroup CRD and rbac permissions for the LVM controller. Signed-off-by: N Balachandran <[email protected]>

sp98 · 2022-01-14T14:49:29Z

pkg/vgmanager/vgmanager_controller.go

+
+func (r *VGReconciler) updateStatus(ctx context.Context, status string, reason string, instance *lvmv1alpha1.LVMVolumeGroup) error {
+
+	found := false


We still have a situation where multiple controllers will be updating the same LVMVolumeGroup status, right?
If that's the case, should we first get the latest LVMVolumeGroup instance before verifying the NodeStatus?

I think this may still NOT* be enough to ensure that we are using the updated instance of LVMVolumeGroup.

If it is not the latest version, I expect the update to fail. I will confirm after testing on a multi node cluster.

sp98 · 2022-01-14T15:02:27Z

pkg/vgmanager/vgmanager_controller.go

-				Default:     true,
-			})
+			status = "unavailable"
+			reason = "failed to create VG"


reason for unavailable starts with small f, while that of degraded starts with capital F

travisn · 2022-01-14T18:02:46Z

controllers/lvm_volumegroup.go

+
+	lvmVolumeGroups := c.getLvmVolumeGroups(r, lvmCluster)
+
+	for _, volumeGroup := range lvmVolumeGroups {


Creating the LVMVolumeGroup CRs from the LVMCluster CR is still a design concern. The admin should create the LVMVolumeGroup CRs directly. Not a blocker for feature complete, but we really need to discuss this.

I disagree. The admin should create the LVMCluster CR and the controller should create the LVMVolumeGroup CRs. The intention is to have a single CR for the controller to create the internal pieces from.

I still haven't seen a design backing this decision, so my position remains to disagree with it so we can follow standard K8s patterns.

This commit adds the LVMVolumeGroupNodeStatus CRD and related files. It also removes the NodeStatus from the LVMVolumegroup.Status as that is now part of LVMVolumeGroupNodeStatus. Signed-off-by: N Balachandran <[email protected]>

This commit adds the rbac permissions required by the vgmanager for LVMVolumeGroup and LVMVolumeGroupNodeStatus. Signed-off-by: N Balachandran <[email protected]>

The vgmanager will watch and reconcile the LVMVolumeGroup instead of the LVMCluster CR. This commit also makes minor changes the vgmanager reconcile code. Signed-off-by: N Balachandran <[email protected]>

This commit adds tests to check the creation and deletion of LVMVolumeGroups by the LVM controller. Signed-off-by: N Balachandran <[email protected]>

sp98 · 2022-01-16T08:45:57Z

pkg/vgmanager/vgmanager_controller.go

 		return ctrl.Result{}, err
 	}

-	r.Log.Info("listing block devices")
+	//TODO: actually check the node against the nodeSelector.


Do we need to match with the node selector anymore? Since the LVMVolumeGroup CR is running on the correct node, should that already handle this check for us?

The CR is the object stored in etcd and does not run on a node. The vgmanager pod is expected to check the node it is running on against the nodeselector specified in the LVMVolumeGroup CR to see if it expected to act on it. This is currently not being done.

sp98 · 2022-01-16T14:55:26Z

Q: When we add the functionality to read any new device add events on the nodes, how will we tell all the LVMVolumeGroup CRs associated with the node to reconcile?

nbalacha · 2022-01-17T05:14:58Z

Q: When we add the functionality to read any new device add events on the nodes, how will we tell all the LVMVolumeGroup CRs associated with the node to reconcile?

The VG manager will need to reread the CRs once it receives the event. No changes will be made to the CR to trigger a reconcile.

leelavg · 2022-01-17T11:07:40Z

controllers/lvm_volumegroup.go

+
+	for _, volumeGroup := range lvmVolumeGroups {
+		result, err := cutil.CreateOrUpdate(ctx, r.Client, volumeGroup, func() error {
+			// no need to mutate any field


might not always be true, since deviceSelector isn't implemented fully yet we can get away without not using mutate function

Correct. For now this is fine. The comment can be removed when that is actually done.

leelavg · 2022-01-17T11:42:12Z

pkg/vgmanager/vgmanager_controller.go

+	remainingValidDevices, delayedDevices, err := r.filterAvailableDevices(blockDevices)
+	if err != nil {
+		_ = err
+	}


either don't handle the err at all, a, b, _ := func or pls add a comment on why this is being not handled

This was the earlier existing code which I have not changed.

leelavg · 2022-01-17T11:45:44Z

pkg/vgmanager/vgmanager_controller.go

+		} else {
+			if found {
+				// Update the status again just to be safe.
+				if statuserr := r.updateStatus(ctx, status, volumeGroup); statuserr != nil {


just a question, this has a possibility of conflicts when a vgcr matches multiple nodes right?

we didn't create the vg yet, why should we update the status as ready or the presence of name in config file is just enough for vg status to be ready?

Yes, because the lvmd.yaml file is only updated once the VG is created. This block is mainly to handle cases when the status might not have been updated correctly. That function can also be modified to not update if there is no change for this entry.

leelavg · 2022-01-17T12:01:59Z

pkg/vgmanager/vgmanager_controller.go

+
+	status := &lvmv1alpha1.VGStatus{
+		Name:   req.Name,
+		Status: lvmv1alpha1.VGStatusReady,


IMHO initial vgstatus should be unknown and we try to set it correctly by the end of reconcile 🤔

Can this be taken up in a separate PR?

leelavg · 2022-01-17T12:07:42Z

pkg/vgmanager/vgmanager_controller.go

-		if err != nil {
-			return ctrl.Result{}, err
+		if err == nil {
+			err = os.WriteFile(controllers.LvmdConfigFile, out, 0600)


why not move this line to 218 and remove this conditional?

The checks work like this:
if unmarshall fails,-> return error
If unmarshall succeeds -> attempt to write
if write fails -> return error

Notice that there is no else check - so the err check will work for both scenarios.

leelavg · 2022-01-17T12:16:28Z

pkg/vgmanager/vgmanager_controller.go

+		if nodeStatus.CreationTimestamp.IsZero() {
+			vgNodeStatus.DeepCopyInto(nodeStatus)
+			return nil
+		}


i think setStatus already handles this condition by creating a new status when one is not found?

This is working on the existing CR in the system so we need to update that object.

leelavg

/lgtm

nbalacha · 2022-01-17T14:05:53Z

/lgtm

openshift-ci · 2022-01-17T14:05:53Z

@nbalacha: you cannot LGTM your own PR.

In response to this:

/lgtm

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.

openshift-ci · 2022-01-17T14:05: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

nbalacha requested a review from jmolmo January 14, 2022 12:44

sp98 reviewed Jan 14, 2022

View reviewed changes

jmolmo approved these changes Jan 14, 2022

View reviewed changes

openshift-ci bot assigned jmolmo Jan 14, 2022

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

sp98 suggested changes Jan 14, 2022

View reviewed changes

openshift-ci bot assigned sp98 Jan 14, 2022

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

sp98 suggested changes Jan 14, 2022

View reviewed changes

sp98 reviewed Jan 14, 2022

View reviewed changes

sp98 suggested changes Jan 14, 2022

View reviewed changes

sp98 reviewed Jan 14, 2022

View reviewed changes

nbalacha added 2 commits January 14, 2022 20:16

feat: creates LVMVolumeGroups

4395977

The LVM operator will create individual LVMVolumeGroup objects for the deviceClasses defined in the LVMCluster resource. Signed-off-by: N Balachandran <[email protected]>

feat: adds rbac and crd files

2b9fe48

This commit adds the LVMVolumeGroup CRD and rbac permissions for the LVM controller. Signed-off-by: N Balachandran <[email protected]>

sp98 reviewed Jan 14, 2022

View reviewed changes

sp98 approved these changes Jan 14, 2022

View reviewed changes

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

travisn reviewed Jan 14, 2022

View reviewed changes

nbalacha added 4 commits January 15, 2022 20:43

feat: adds LVMVolumeGroupNodeStatus CRD

bd3272f

This commit adds the LVMVolumeGroupNodeStatus CRD and related files. It also removes the NodeStatus from the LVMVolumegroup.Status as that is now part of LVMVolumeGroupNodeStatus. Signed-off-by: N Balachandran <[email protected]>

feat: adds vgmanager rbacs

2908091

This commit adds the rbac permissions required by the vgmanager for LVMVolumeGroup and LVMVolumeGroupNodeStatus. Signed-off-by: N Balachandran <[email protected]>

feat: vgmanager watches LVMVolumeGroup now

bf436cf

The vgmanager will watch and reconcile the LVMVolumeGroup instead of the LVMCluster CR. This commit also makes minor changes the vgmanager reconcile code. Signed-off-by: N Balachandran <[email protected]>

test: adds tests for LVMVolumeGroup

4e6b63a

This commit adds tests to check the creation and deletion of LVMVolumeGroups by the LVM controller. Signed-off-by: N Balachandran <[email protected]>

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

sp98 reviewed Jan 16, 2022

View reviewed changes

leelavg reviewed Jan 17, 2022

View reviewed changes

leelavg approved these changes Jan 17, 2022

View reviewed changes

openshift-ci bot assigned leelavg Jan 17, 2022

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

nbalacha merged commit a5ee8b9 into openshift:main Jan 17, 2022

nbalacha deleted the new-crd-2 branch January 18, 2022 10:50

	r.Log.Error(err, "LvmVolumeGroup reconcile failure", "name", volumeGroup.Name)
	r.Log.Error(err, "failed to reconcile LvmVolumeGroup", "name", volumeGroup.Name)


		for _, vgcr := range vgcrs {
		err := r.Client.Get(ctx, types.NamespacedName{Name: vgcr.Name, Namespace: vgcr.Namespace}, existingLvmVg)


		func (r VGReconciler) updateStatus(ctx context.Context, status string, reason string, instance lvmv1alpha1.LVMVolumeGroup) error {

		found := false


		lvmVolumeGroups := c.getLvmVolumeGroups(r, lvmCluster)

		for _, volumeGroup := range lvmVolumeGroups {

feat: adds the LVMVolumeGroup CRD and processing logic #72

feat: adds the LVMVolumeGroup CRD and processing logic #72

Conversation

nbalacha commented Jan 14, 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

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

Choose a reason for hiding this comment

nbalacha Jan 14, 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

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 Jan 14, 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

sp98 commented Jan 16, 2022 • edited Loading

nbalacha commented Jan 17, 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

nbalacha Jan 14, 2022 •

edited

Loading

sp98 Jan 14, 2022 •

edited

Loading

sp98 commented Jan 16, 2022 •

edited

Loading