refactor: add simillar labels to the each components #148

iamniting · 2022-04-11T05:22:18Z

Add labels to the topolvm-controller, topolvm-node, vg-manager
for example:
app.lvm.openshift.io: topolvm-controller
app.lvm.openshift.io: topolvm-node
app.lvm.openshift.io: vg-manager

Signed-off-by: Nitin Goyal [email protected]

iamniting · 2022-04-11T05:23:26Z

controllers/topolvm_controller.go

 		},
 		Spec: appsv1.DeploymentSpec{
 			Replicas: &replicas,
 			Selector: &metav1.LabelSelector{
 				MatchLabels: map[string]string{
-					AppAttr: TopolvmControllerDeploymentName,
+					AppAttr:         TopolvmControllerDeploymentName,


Are we using this existing label anywhere? If not I would like to remove it.

can be removed

iamniting · 2022-04-11T05:23:46Z

controllers/topolvm_node.go

@@ -177,6 +177,8 @@ func getNodeDaemonSet(lvmCluster *lvmv1alpha1.LVMCluster, namespace string, init
 	}
 	labels := map[string]string{
 		"app": topolvmNodeName,


Are we using this existing label anywhere? If not I would like to remove it.

can be removed

iamniting · 2022-04-11T05:23:58Z

controllers/vgmanager_daemonset.go

@@ -172,6 +172,7 @@ func newVGManagerDaemonset(lvmCluster *lvmv1alpha1.LVMCluster, namespace string,
 	}
 	labels := map[string]string{
 		"app":                   VGManagerUnit,


Are we using this existing label anywhere? If not I would like to remove it.

can be removed

leelavg

pardon, forgot to mention earlier, we might also need a label on manager itself.
in addition to this label if we can haveapp.kubernetes.io/part-of: lvmo as well then we can get all possible resources under lvmo using a single label.

iamniting · 2022-04-11T05:51:21Z

/hold

Require discussion before merging the PR, As this will break the upgrades.

The Deployment "nginx-deployment" is invalid: spec.selector: Invalid value: v1.LabelSelector{MatchLabels:map[string]string{"app":"nginx", "nigoyal":"nigoyal"}, MatchExpressions:[]v1.LabelSelectorRequirement(nil)}: field is immutable```

iamniting · 2022-04-11T06:25:19Z

pardon, forgot to mention earlier, we might also need a label on manager itself.

That is a topic of discussion and should be taken care of separately as that is a CSV change and will break upgrades very badly.

in addition to this label if we can haveapp.kubernetes.io/part-of: lvmo as well then we can get all possible resources under lvmo using a single label.

We can even list all of them without adding the above labels using only key I added in the PR for example

$ oc get pod -l app
NAME                                READY   STATUS    RESTARTS   AGE
nginx-deployment-74dc8cc494-b4fpn   1/1     Running   0          21m
topolvm-node-lgps4                  4/4     Running   0          2d19h
vg-manager-7k5lf                    1/1     Running   0          2d19h

iamniting · 2022-04-14T07:25:37Z

/hold cancel

iamniting · 2022-04-14T07:27:31Z

┌─[nigoyalॐ nigoyal ~/code/go/src/github.com/red-hat-storage/lvm-operator  labels]
└──╼$ oc get pods -l app.lvm.topolvm.io
NAME                                  READY   STATUS    RESTARTS   AGE
topolvm-controller-7546d896f6-rqkgh   4/4     Running   0          3m41s
topolvm-node-ljt7d                    4/4     Running   0          3m41s
vg-manager-2mcj2                      1/1     Running   0          3m41s

┌─[nigoyalॐ nigoyal ~/code/go/src/github.com/red-hat-storage/lvm-operator  labels]
└──╼$ oc get pods -l app.lvm.topolvm.io=topolvm-controller
NAME                                  READY   STATUS    RESTARTS   AGE
topolvm-controller-7546d896f6-rqkgh   4/4     Running   0          3m48s

┌─[nigoyalॐ nigoyal ~/code/go/src/github.com/red-hat-storage/lvm-operator  labels]
└──╼$ oc get pods -l app.lvm.topolvm.io=topolvm-node
NAME                 READY   STATUS    RESTARTS   AGE
topolvm-node-ljt7d   4/4     Running   0          4m10s

┌─[nigoyalॐ nigoyal ~/code/go/src/github.com/red-hat-storage/lvm-operator  labels]
└──╼$ oc get pods -l app.lvm.topolvm.io=vg-manager
NAME               READY   STATUS    RESTARTS   AGE
vg-manager-2mcj2   1/1     Running   0          4m14s

controllers/defaults.go

nbalacha

Please add a label to the main controller-manager as well.

Add labels to the topolvm-controller, topolvm-node, vg-manager for example: app.lvm.topolvm.io: topolvm-controller app.lvm.topolvm.io: topolvm-node app.lvm.topolvm.io: vg-manager Signed-off-by: Nitin Goyal <[email protected]>

iamniting · 2022-04-14T09:28:43Z

Please add a label to the main controller-manager as well.

I would like to take it as separate PR as the changes are in the kustomize and related files and not in the controller.

openshift-ci · 2022-04-14T11:23:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: iamniting, 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

iamniting requested review from nbalacha and leelavg and removed request for nbalacha April 11, 2022 05:22

iamniting commented Apr 11, 2022

View reviewed changes

leelavg reviewed Apr 11, 2022

View reviewed changes

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 11, 2022

iamniting force-pushed the labels branch 3 times, most recently from d5f04a3 to 7c16ae8 Compare April 14, 2022 07:25

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 14, 2022

iamniting requested a review from leelavg April 14, 2022 07:27

nbalacha reviewed Apr 14, 2022

View reviewed changes

controllers/defaults.go Outdated Show resolved Hide resolved

nbalacha suggested changes Apr 14, 2022

View reviewed changes

openshift-ci bot assigned nbalacha Apr 14, 2022

refactor: add simillar labels to the each components

f9e42c2

Add labels to the topolvm-controller, topolvm-node, vg-manager for example: app.lvm.topolvm.io: topolvm-controller app.lvm.topolvm.io: topolvm-node app.lvm.topolvm.io: vg-manager Signed-off-by: Nitin Goyal <[email protected]>

iamniting force-pushed the labels branch from 7c16ae8 to f9e42c2 Compare April 14, 2022 09:11

iamniting requested a review from nbalacha April 14, 2022 09:32

nbalacha approved these changes Apr 14, 2022

View reviewed changes

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

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

openshift-merge-robot merged commit 8ecfd77 into openshift:main Apr 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: add simillar labels to the each components #148

refactor: add simillar labels to the each components #148

iamniting commented Apr 11, 2022 •

edited

Loading

iamniting Apr 11, 2022

leelavg Apr 11, 2022

iamniting Apr 11, 2022

leelavg Apr 11, 2022

iamniting Apr 11, 2022

leelavg Apr 11, 2022

leelavg left a comment

iamniting commented Apr 11, 2022

iamniting commented Apr 11, 2022

iamniting commented Apr 14, 2022

iamniting commented Apr 14, 2022

nbalacha left a comment

iamniting commented Apr 14, 2022

openshift-ci bot commented Apr 14, 2022

refactor: add simillar labels to the each components #148

refactor: add simillar labels to the each components #148

Conversation

iamniting commented Apr 11, 2022 • edited Loading

iamniting Apr 11, 2022

Choose a reason for hiding this comment

leelavg Apr 11, 2022

Choose a reason for hiding this comment

iamniting Apr 11, 2022

Choose a reason for hiding this comment

leelavg Apr 11, 2022

Choose a reason for hiding this comment

iamniting Apr 11, 2022

Choose a reason for hiding this comment

leelavg Apr 11, 2022

Choose a reason for hiding this comment

leelavg left a comment

Choose a reason for hiding this comment

iamniting commented Apr 11, 2022

iamniting commented Apr 11, 2022

iamniting commented Apr 14, 2022

iamniting commented Apr 14, 2022

nbalacha left a comment

Choose a reason for hiding this comment

iamniting commented Apr 14, 2022

openshift-ci bot commented Apr 14, 2022

iamniting commented Apr 11, 2022 •

edited

Loading