Add 3 labels to all resources, add e2e test that validate the existence of the three labels #142

Shai1-Levi · 2025-01-13T16:29:47Z

Why we need this PR

To meet k8s custom labels standardization:
Add 3 custom labels to all resources, add e2e that test the existence of name label

Changes made

Add recommended app.kubernetes.io/name label to everything

With these changes we prevent services selecting pods by other
operators, because they all have the control-plane: controller-manager
label by default.

Because deployment updates with modified labels fail, also rename the
deployment by modifying the namePrefix (causes bundle file renames as
well).

Add test that validate retrieval of operator controller pod by the name label.
Add the default container label, for easier getting desired logs.

Which issue(s) this PR fixes

Test plan

2. Add test that validate retrieval of operator controller pod by the label.

openshift-ci · 2025-01-13T16:29:58Z

Hi @Shai1-Levi. Thanks for your PR.

I'm waiting for a medik8s member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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-sigs/prow repository.

2. Add test that validate retrieval of operator controller pod by the label.

slintes · 2025-01-14T08:16:32Z

in the past at least, label additions broke updates... [1]
IIRC e2e has an update test, let's see what happens.

/test 4.17-openshift-e2e

[1] medik8s/node-healthcheck-operator@df1d2e8#diff-c006b1ef995eeeb57408cce42bd84db8dfc0cbe18df03222857665cebb1ac6fdR9-R18

slintes · 2025-01-14T12:36:35Z

update still fails 🤷🏼‍♂️

"Failed to run bundle upgrade: error waiting for CSV to install: csv failed: reason: \"InstallComponentFailed\", message: \"install strategy failed: Deployment.apps \\\"node-maintenance-operator-controller-manager\\\" is invalid: spec.selector: Invalid value: v1.LabelSelector{MatchLabels:map[string]string{\\\"app.kubernetes.io/name\\\":\\\"node-maintenance-operator\\\", \\\"control-plane\\\":\\\"controller-manager\\\", \\\"node-maintenance-operator\\\":\\\"\\\"}, MatchExpressions:[]v1.LabelSelectorRequirement(nil)}: field is immutable\"\n" make: *** [Makefile:208: bundle-run-update] Error 1

- add recommended app.kubernetes.io/name label to everything With these changes we prevent services selecting pods by other operators, because they all have the control-plane: controller-manager label by default. Because deployment updates with modified labels fail, also rename the deployment by modifying the namePrefix (causes bundle file renames as well). - Add test that validate retrieval of operator controller pod by the label.

Shai1-Levi · 2025-01-15T16:08:27Z

/test 4.17-openshift-e2e

openshift-ci · 2025-01-15T16:08:44Z

@Shai1-Levi: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/test 4.17-openshift-e2e

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-sigs/prow repository.

razo7 · 2025-01-15T16:50:46Z

/ok-to-test

- add recommended app.kubernetes.io/name label to everything With these changes we prevent services selecting pods by other operators, because they all have the control-plane: controller-manager label by default. Because deployment updates with modified labels fail, also rename the deployment by modifying the namePrefix (causes bundle file renames as well). - Add test that validate retrieval of operator controller pod by the label.

slintes · 2025-01-16T09:00:39Z

/test 4.17-openshift-e2e

slintes · 2025-01-16T13:31:36Z

operator update succeeded
test failure looks unrelated, looking at the test history this seems to be very flaky... :/

/ok-to-test
/lgtm
/approve

- Add recommended app.kubernetes.io/name label to everything With these changes we prevent services selecting pods by other operators, because they all have the control-plane: controller-manager label by default. Because deployment updates with modified labels fail, also rename the deployment by modifying the namePrefix (causes bundle file renames as well). - Add test that validate retrieval of operator controller pod by the label. - Add the default container label, for easier getting desired logs.

Shai1-Levi · 2025-01-16T18:01:43Z

Hey,
I added some labels to fix the e2e test and test-scorecard
@slintes I didn't saw your last comment before I pushed the code

/test 4.17-openshift-e2e

bundle/manifests/node-maintenance-operator.clusterserviceversion.yaml

- Add recommended app.kubernetes.io/name label to everything With these changes we prevent services selecting pods by other operators, because they all have the control-plane: controller-manager label by default. Because deployment updates with modified labels fail, also rename the deployment by modifying the namePrefix (causes bundle file renames as well). - Add test that validate retrieval of operator controller pod by the label. - Add the default container label, for easier getting desired logs.

slintes · 2025-01-17T14:17:34Z

/retest
/lgtm

slintes · 2025-01-17T16:37:39Z

/retest

slintes · 2025-01-18T08:09:51Z

setup failed...

/retest

slintes · 2025-01-18T12:48:52Z

@razo7 since when is the e2e test so flaky? :/

Error waiting for events: context deadline exceeded  [FAILED] Event SucceedMaintenance was missing for test-maintenance nm CR
  Unexpected error:
      <context.deadlineExceededError>: 
      context deadline exceeded
      {}
  occurred

slintes · 2025-01-18T12:49:04Z

/retest

razo7 · 2025-01-19T08:14:56Z

test/e2e/node_maintenance_test.go

@@ -419,6 +422,13 @@ func getOperatorPod() *corev1.Pod {
 	return &pods.Items[0]
 }

+func getOperatorLabel() bool {
+	pods, err := KubeClient.CoreV1().Pods(operatorNsName).List(context.Background(), metav1.ListOptions{LabelSelector: "app.kubernetes.io/name=node-maintenance-operator"})


IIUC the PR introduces 3 new labels while this function looks for the existence of only one of them. See also K8s recommended labels .

Thanks, I will add the new labels into thse e2e test.
Regard k8s recommended labels I over this doc and I think that all the relevant labels was added. Thank you for introducing me with these k8s convention

…labels existence - Add recommended app.kubernetes.io/name label to everything With these changes we prevent services selecting pods by other operators, because they all have the control-plane: controller-manager label by default. Because deployment updates with modified labels fail, also rename the deployment by modifying the namePrefix (causes bundle file renames as well). - Add tests that validate retrieval of operator controller pod by label. - Add the default container label, for easier getting desired logs.

slintes

/retest

slintes · 2025-01-22T07:26:23Z

test/e2e/node_maintenance_test.go

 	ExpectWithOffset(2, len(pods.Items)).ToNot(BeZero(), "no operator pod found")
 	return true
 }

+func validateOperatorCustomLabelComponent() bool {


not that relevant for test code, but for production code this would be very inefficient. You could get the pod with one label, and then check if it has the other labels as well, instead of looking for the pod 3 times

slintes · 2025-01-22T10:32:35Z

/hold

since this is not urgent, let's first try to fix the e2e test, these retests are a waste of resources...

openshift-ci · 2025-01-22T13:44:41Z

New changes are detected. LGTM label has been removed.

slintes · 2025-01-22T17:42:20Z

test/e2e/node_maintenance_test.go

@@ -179,6 +179,9 @@ var _ = Describe("Starting Maintenance", func() {
 			// it should be caused by the test deployment's termination graceperiod > drain timeout
 			Expect(getOperatorLogs()).To(ContainSubstring(nodemaintenance.FixedDurationReconcileLog))

+			//validate that operator controller pod have app.kubernetes.io/name label
+			Expect(validateOperatorCustomLabels()).To(BeTrue(), "operator custom label validation failed")


the return value and checking it isn't needed IMHO, everything is checked inside the function already?

slintes · 2025-01-22T17:43:15Z

test/e2e/node_maintenance_test.go

+	podName := podOperator.ObjectMeta.Name
+
+	// Get the Pod
+	pod, err := KubeClient.CoreV1().Pods(operatorNsName).Get(context.TODO(), podName, metav1.GetOptions{})


you have the pod already, why getting it here again?

slintes · 2025-01-22T17:54:14Z

test/e2e/node_maintenance_test.go

+	// Check if each label is present
+	for _, key := range requiredKeys {
+		_, exists := pod.Labels[key]
+		ExpectWithOffset(1, exists).Should(BeTrue(), fmt.Sprintf("Missing required label '%s' in pod '%s'", key, podName))


a) you don't check label values anymore
b) hint: ExpectWithOffset(1, pod.Labels).To(HaveKeyWithValue(key, value))

openshift-ci · 2025-01-22T17:54:40Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Shai1-Levi
Once this PR has been reviewed and has the lgtm label, please ask for approval from slintes. For more information see the Code Review Process.

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 · 2025-07-07T08:39:17Z

@Shai1-Levi: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/4.16-openshift-e2e	`807c5f9`	link	true	`/test 4.16-openshift-e2e`
ci/prow/4.17-openshift-e2e	`807c5f9`	link	true	`/test 4.17-openshift-e2e`
ci/prow/4.18-openshift-e2e	`807c5f9`	link	true	`/test 4.18-openshift-e2e`
ci/prow/4.20-ci-bundle-my-bundle	`1827784`	link	true	`/test 4.20-ci-bundle-my-bundle`

Full PR test history. Your PR dashboard.

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-sigs/prow repository. I understand the commands that are listed here.

1. Add custom label, propagate the label to all the right files.

9788816

2. Add test that validate retrieval of operator controller pod by the label.

openshift-ci bot requested review from razo7 and slintes January 13, 2025 16:29

openshift-ci bot added the needs-ok-to-test label Jan 13, 2025

1. Add custom label, propagate the label to all the right files.

52d10cf

2. Add test that validate retrieval of operator controller pod by the label.

Shai1-Levi changed the title ~~Add custom label to all resources, add e2e test with it~~ [WIP] Add custom label to all resources, add e2e test with it Jan 14, 2025

openshift-ci bot added the do-not-merge/work-in-progress label Jan 14, 2025

openshift-ci bot assigned slintes Jan 16, 2025

openshift-ci bot added ok-to-test lgtm approved and removed needs-ok-to-test labels Jan 16, 2025

openshift-ci bot removed the lgtm label Jan 16, 2025

slintes requested changes Jan 17, 2025

View reviewed changes

bundle/manifests/node-maintenance-operator.clusterserviceversion.yaml Outdated Show resolved Hide resolved

bundle/manifests/node-maintenance-operator.clusterserviceversion.yaml Outdated Show resolved Hide resolved

openshift-ci bot removed the approved label Jan 17, 2025

openshift-ci bot added the lgtm label Jan 17, 2025

Shai1-Levi changed the title ~~[WIP] Add custom label to all resources, add e2e test with it~~ Add custom label to all resources, add e2e test with it Jan 19, 2025

openshift-ci bot removed the do-not-merge/work-in-progress label Jan 19, 2025

razo7 reviewed Jan 19, 2025

View reviewed changes

Shai1-Levi changed the title ~~Add custom label to all resources, add e2e test with it~~ Add 3 custom labels to all resources, add e2e that test the existence of name label Jan 19, 2025

openshift-ci bot removed the lgtm label Jan 21, 2025

Shai1-Levi changed the title ~~Add 3 custom labels to all resources, add e2e that test the existence of name label~~ Add 3 labels to all resources, add e2e test that validate the existence of the three labels Jan 21, 2025

slintes approved these changes Jan 22, 2025

View reviewed changes

openshift-ci bot added lgtm approved labels Jan 22, 2025

openshift-ci bot added the do-not-merge/hold label Jan 22, 2025

Shai1-Levi marked this pull request as draft January 22, 2025 11:29

openshift-ci bot added the do-not-merge/work-in-progress label Jan 22, 2025

Refactor tests

1827784

openshift-ci bot removed the lgtm label Jan 22, 2025

slintes requested changes Jan 22, 2025

View reviewed changes

openshift-ci bot removed the approved label Jan 22, 2025

Add 3 labels to all resources, add e2e test that validate the existence of the three labels #142

Are you sure you want to change the base?

Add 3 labels to all resources, add e2e test that validate the existence of the three labels #142

Conversation

Shai1-Levi commented Jan 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why we need this PR

Changes made

Which issue(s) this PR fixes

Test plan

Uh oh!

openshift-ci bot commented Jan 13, 2025

Uh oh!

slintes commented Jan 14, 2025

Uh oh!

slintes commented Jan 14, 2025

Uh oh!

Shai1-Levi commented Jan 15, 2025

Uh oh!

openshift-ci bot commented Jan 15, 2025

Uh oh!

razo7 commented Jan 15, 2025

Uh oh!

slintes commented Jan 16, 2025

Uh oh!

slintes commented Jan 16, 2025

Uh oh!

Shai1-Levi commented Jan 16, 2025

Uh oh!

Uh oh!

Uh oh!

slintes commented Jan 17, 2025

Uh oh!

slintes commented Jan 17, 2025

Uh oh!

slintes commented Jan 18, 2025

Uh oh!

slintes commented Jan 18, 2025

Uh oh!

slintes commented Jan 18, 2025

Uh oh!

razo7 Jan 19, 2025

Choose a reason for hiding this comment

Uh oh!

Shai1-Levi Jan 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slintes left a comment

Choose a reason for hiding this comment

Uh oh!

slintes Jan 22, 2025

Choose a reason for hiding this comment

Uh oh!

slintes commented Jan 22, 2025

Uh oh!

openshift-ci bot commented Jan 22, 2025

Uh oh!

slintes Jan 22, 2025

Choose a reason for hiding this comment

Uh oh!

slintes Jan 22, 2025

Choose a reason for hiding this comment

Uh oh!

slintes Jan 22, 2025

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Jan 22, 2025

Uh oh!

openshift-ci bot commented Jul 7, 2025

Uh oh!

Uh oh!

Shai1-Levi commented Jan 13, 2025 •

edited

Loading

Shai1-Levi Jan 19, 2025 •

edited

Loading