OCPBUGS-10222: [metrics] Ignore case of cluster monitoring label #1479

saifshaikh48 · 2023-03-14T16:11:22Z

This PR makes the cluster monitoring label for the WMCO namespace case insensitive to provide better user experience. Previously, not adhering to an all lowercase true value would result in WMCO ignoring the label and not configuring/exposing Prometheus metrics for Windows nodes.

Fixes OCPBUGS-10222

Fixes #1478

openshift-ci-robot · 2023-03-14T16:11:29Z

@saifshaikh48: This pull request references Jira Issue OCPBUGS-10222, which is invalid:

expected the bug to target the "4.14.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

This PR makes the cluster monitoring label for the WMCO namespace case insensitive to provide better user experience. Previously, not adhering to an all lowercase true value would result in WMCO ignoring the label and not configuring/exposing Prometheus metrics for Windows nodes.

Fixes OCPBUGS-10222

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 · 2023-03-14T16:11:36Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

saifshaikh48 · 2023-03-14T19:21:57Z

/jira refresh

openshift-ci-robot · 2023-03-14T19:22:00Z

@saifshaikh48: This pull request references Jira Issue OCPBUGS-10222, which is invalid:

expected the bug to target the "4.14.0" version, but it targets "4.14" instead

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

/jira refresh

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.

saifshaikh48 · 2023-03-14T19:31:30Z

/jira refresh

openshift-ci-robot · 2023-03-14T19:31:38Z

@saifshaikh48: This pull request references Jira Issue OCPBUGS-10222, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.14.0) matches configured target version for branch (4.14.0)
bug is in the state New, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @rrasouli

In response to this:

/jira refresh

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.

aravindhp · 2023-03-14T19:42:01Z

pkg/metrics/metrics.go

@@ -253,7 +254,7 @@ func (c *Config) validate(ctx context.Context) (bool, error) {
 	if err != nil {
 		return false, errors.Wrap(err, "error getting operator namespace")
 	}
-	if wmcoNamespace.Labels["openshift.io/cluster-monitoring"] != "true" {
+	if strings.ToLower(wmcoNamespace.Labels["openshift.io/cluster-monitoring"]) != "true" {


So this will accept strings like tRue etc. Do we want to do that or throw an error?

Should be fine IMO. I don't see a reason to accept True but not tRue etc

Thinking more about this, I suggest we rather do strconv.ParseBool.

alinaryan · 2023-03-14T21:13:18Z

Thanks for taking this on!
/lgtm

aravindhp · 2023-03-14T21:44:24Z

pkg/metrics/metrics.go

@@ -253,7 +254,7 @@ func (c *Config) validate(ctx context.Context) (bool, error) {
 	if err != nil {
 		return false, errors.Wrap(err, "error getting operator namespace")
 	}
-	if wmcoNamespace.Labels["openshift.io/cluster-monitoring"] != "true" {
+	if strings.ToLower(wmcoNamespace.Labels["openshift.io/cluster-monitoring"]) != "true" {


Thinking more about this, I suggest we rather do strconv.ParseBool.

aravindhp

It also makes WMCO able to react to a change in the namespace's cluster monitoring label.

How does that happen? Don't we have to restart the operator?

pkg/metrics/metrics.go

saifshaikh48 · 2023-03-15T13:46:45Z

It also makes WMCO able to react to a change in the namespace's cluster monitoring label.

How does that happen? Don't we have to restart the operator?

You're right, I read the metrics code path wrong. I'll drop that from the commit msg

mansikulkarni96 · 2023-03-15T14:11:06Z

pkg/metrics/metrics.go

-	if wmcoNamespace.Labels["openshift.io/cluster-monitoring"] != "true" {
-		metricsEnabled = false
+
+	boolVal := false


Instead of bool val, can we give it a monitoring specific name?
Maybe you can just use the metricsEnabled var here.

I opted for the non-specific name because there's already a global variable by the name of metricsEnabled (which gets assigned to the boolVal later in this function). I can make this labelValue instead?

openshift-ci · 2023-03-15T15:23:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aravindhp, saifshaikh48

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~~ [aravindhp]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

This commit makes check for the monitoring label on the WMCO namespace accept all valid string representations of booleans to provide better user experience. Previously, not adhering to an all lowercase `true` value would result in WMCO ignoring the label and not configuring Prometheus metrics for Windows nodes.

mansikulkarni96 · 2023-03-15T16:26:19Z

/lgtm

mansikulkarni96 · 2023-03-15T16:26:49Z

Fixes #1478

openshift-ci-robot · 2023-03-15T16:46:13Z

@saifshaikh48: This pull request references Jira Issue OCPBUGS-10222, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.14.0) matches configured target version for branch (4.14.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @rrasouli

In response to this:

This PR makes the cluster monitoring label for the WMCO namespace case insensitive to provide better user experience. Previously, not adhering to an all lowercase true value would result in WMCO ignoring the label and not configuring/exposing Prometheus metrics for Windows nodes.

Fixes OCPBUGS-10222

Fixes #1478

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-robot · 2023-03-15T18:19:02Z

/retest-required

Remaining retests: 0 against base HEAD 9216f3d and 2 for PR HEAD 767f93c in total

jrvaldes · 2023-03-15T18:25:18Z

pkg/metrics/metrics.go

-// validate will verify if cluster monitoring is enabled in the operator namespace.
-// If the label is not present, it will log and send warning events to the user.
+// validate will verify if cluster monitoring is enabled in the operator namespace. If the label is set to false or not
+// present, it will log and send warning events to the user. If the label holds a non-boolean value, returns an error.
 func (c *Config) validate(ctx context.Context) (bool, error) {


Does it make sense to add a unit test to cover the mentioned cases?

windows-machine-config-operator/pkg/metrics/metrics_test.go

Line 4 in 767f93c

"testing"

I don't think it's necessary to add one since there's e2e tests covering this functionality

saifshaikh48 · 2023-03-16T14:29:54Z

/retest-required

cluster spinup flakes

openshift-ci · 2023-03-16T17:15:15Z

@saifshaikh48: all tests passed!

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

openshift-ci-robot · 2023-03-16T17:18:25Z

@saifshaikh48: Jira Issue OCPBUGS-10222: All pull requests linked via external trackers have merged:

openshift/windows-machine-config-operator#1479

Jira Issue OCPBUGS-10222 has been moved to the MODIFIED state.

In response to this:

This PR makes the cluster monitoring label for the WMCO namespace case insensitive to provide better user experience. Previously, not adhering to an all lowercase true value would result in WMCO ignoring the label and not configuring/exposing Prometheus metrics for Windows nodes.

Fixes OCPBUGS-10222

Fixes #1478

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.

saifshaikh48 · 2023-03-16T17:35:22Z

/cherry-pick release-4.13

openshift-cherrypick-robot · 2023-03-16T17:36:04Z

@saifshaikh48: new pull request created: #1494

In response to this:

/cherry-pick release-4.13
/cherry-pick release-4.12
/cherry-pick release-4.11
/cherry-pick release-4.10

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.

saifshaikh48 · 2023-03-16T17:37:19Z

/cherry-pick release-4.12

openshift-cherrypick-robot · 2023-03-16T17:37:58Z

@saifshaikh48: new pull request created: #1495

In response to this:

/cherry-pick release-4.12

/cherry-pick release-4.11

/cherry-pick release-4.10

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.

saifshaikh48 · 2023-03-16T17:41:44Z

/cherry-pick release-4.11

saifshaikh48 · 2023-03-16T17:41:55Z

/cherry-pick release-4.10

openshift-cherrypick-robot · 2023-03-16T17:42:23Z

@saifshaikh48: new pull request created: #1496

In response to this:

/cherry-pick release-4.11

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-cherrypick-robot · 2023-03-16T17:42:35Z

@saifshaikh48: new pull request created: #1497

In response to this:

/cherry-pick release-4.10

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.

aravindhp · 2023-03-16T17:52:38Z

@saifshaikh48 for bugs please do not cherry-pick to all releases from the master PR. Just to the release-4.n-1 branch. Then do the release-4.n-2 cherry pick from the release-4.n-1PR. If you don't do this then you have to manually fix up all the Jira links. So please close all the other cherry picks.

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 Mar 14, 2023

openshift-ci bot requested a review from rrasouli March 14, 2023 19:31

aravindhp reviewed Mar 14, 2023

View reviewed changes

openshift-ci bot assigned alinaryan Mar 14, 2023

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

aravindhp suggested changes Mar 14, 2023

View reviewed changes

saifshaikh48 force-pushed the case-monitoring-label branch from 6605bcf to 3de259e Compare March 14, 2023 22:19

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

saifshaikh48 force-pushed the case-monitoring-label branch 2 times, most recently from d423559 to 8a3c4e3 Compare March 14, 2023 22:23

aravindhp suggested changes Mar 14, 2023

View reviewed changes

pkg/metrics/metrics.go Outdated Show resolved Hide resolved

saifshaikh48 force-pushed the case-monitoring-label branch from 8a3c4e3 to d80c060 Compare March 15, 2023 13:54

mansikulkarni96 reviewed Mar 15, 2023

View reviewed changes

saifshaikh48 force-pushed the case-monitoring-label branch from d80c060 to 12d3fb5 Compare March 15, 2023 14:46

aravindhp approved these changes Mar 15, 2023

View reviewed changes

saifshaikh48 force-pushed the case-monitoring-label branch from e099d6f to 767f93c Compare March 15, 2023 16:05

openshift-ci bot assigned mansikulkarni96 Mar 15, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 15, 2023

saifshaikh48 marked this pull request as ready for review March 15, 2023 17:21

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 Mar 15, 2023

openshift-ci bot requested review from aravindhp and sebsoto March 15, 2023 17:25

jrvaldes reviewed Mar 15, 2023

View reviewed changes

openshift-merge-robot merged commit 6273896 into openshift:master Mar 16, 2023

openshift-cherrypick-robot mentioned this pull request Mar 16, 2023

[release-4.13] OCPBUGS-10416: [metrics] Ignore case of cluster monitoring label #1494

Merged

openshift-cherrypick-robot mentioned this pull request Mar 16, 2023

[release-4.12] OCPBUGS-10417: [metrics] Ignore case of cluster monitoring label #1495

Merged

This was referenced Mar 16, 2023

[release-4.11] OCPBUGS-10418: [metrics] Ignore case of cluster monitoring label #1496

Merged

[release-4.10] OCPBUGS-10419: [metrics] Ignore case of cluster monitoring label #1497

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OCPBUGS-10222: [metrics] Ignore case of cluster monitoring label #1479

OCPBUGS-10222: [metrics] Ignore case of cluster monitoring label #1479

saifshaikh48 commented Mar 14, 2023 •

edited by aravindhp

openshift-ci-robot commented Mar 14, 2023

openshift-ci bot commented Mar 14, 2023

saifshaikh48 commented Mar 14, 2023

openshift-ci-robot commented Mar 14, 2023

saifshaikh48 commented Mar 14, 2023

openshift-ci-robot commented Mar 14, 2023

aravindhp Mar 14, 2023

saifshaikh48 Mar 14, 2023 •

edited

aravindhp Mar 14, 2023

alinaryan commented Mar 14, 2023

aravindhp Mar 14, 2023

aravindhp left a comment

saifshaikh48 commented Mar 15, 2023

mansikulkarni96 Mar 15, 2023 •

edited

saifshaikh48 Mar 15, 2023

openshift-ci bot commented Mar 15, 2023

mansikulkarni96 commented Mar 15, 2023

mansikulkarni96 commented Mar 15, 2023

openshift-ci-robot commented Mar 15, 2023

openshift-ci-robot commented Mar 15, 2023

jrvaldes Mar 15, 2023

saifshaikh48 Mar 16, 2023

saifshaikh48 commented Mar 16, 2023

openshift-ci bot commented Mar 16, 2023

openshift-ci-robot commented Mar 16, 2023

saifshaikh48 commented Mar 16, 2023 •

edited

openshift-cherrypick-robot commented Mar 16, 2023

saifshaikh48 commented Mar 16, 2023 •

edited

openshift-cherrypick-robot commented Mar 16, 2023

saifshaikh48 commented Mar 16, 2023

saifshaikh48 commented Mar 16, 2023

openshift-cherrypick-robot commented Mar 16, 2023

openshift-cherrypick-robot commented Mar 16, 2023

aravindhp commented Mar 16, 2023

OCPBUGS-10222: [metrics] Ignore case of cluster monitoring label #1479

OCPBUGS-10222: [metrics] Ignore case of cluster monitoring label #1479

Conversation

saifshaikh48 commented Mar 14, 2023 • edited by aravindhp

openshift-ci-robot commented Mar 14, 2023

openshift-ci bot commented Mar 14, 2023

saifshaikh48 commented Mar 14, 2023

openshift-ci-robot commented Mar 14, 2023

saifshaikh48 commented Mar 14, 2023

openshift-ci-robot commented Mar 14, 2023

aravindhp Mar 14, 2023

Choose a reason for hiding this comment

saifshaikh48 Mar 14, 2023 • edited

Choose a reason for hiding this comment

aravindhp Mar 14, 2023

Choose a reason for hiding this comment

alinaryan commented Mar 14, 2023

aravindhp Mar 14, 2023

Choose a reason for hiding this comment

aravindhp left a comment

Choose a reason for hiding this comment

saifshaikh48 commented Mar 15, 2023

mansikulkarni96 Mar 15, 2023 • edited

Choose a reason for hiding this comment

saifshaikh48 Mar 15, 2023

Choose a reason for hiding this comment

openshift-ci bot commented Mar 15, 2023

mansikulkarni96 commented Mar 15, 2023

mansikulkarni96 commented Mar 15, 2023

openshift-ci-robot commented Mar 15, 2023

openshift-ci-robot commented Mar 15, 2023

jrvaldes Mar 15, 2023

Choose a reason for hiding this comment

saifshaikh48 Mar 16, 2023

Choose a reason for hiding this comment

saifshaikh48 commented Mar 16, 2023

openshift-ci bot commented Mar 16, 2023

openshift-ci-robot commented Mar 16, 2023

saifshaikh48 commented Mar 16, 2023 • edited

openshift-cherrypick-robot commented Mar 16, 2023

saifshaikh48 commented Mar 16, 2023 • edited

openshift-cherrypick-robot commented Mar 16, 2023

saifshaikh48 commented Mar 16, 2023

saifshaikh48 commented Mar 16, 2023

openshift-cherrypick-robot commented Mar 16, 2023

openshift-cherrypick-robot commented Mar 16, 2023

aravindhp commented Mar 16, 2023

saifshaikh48 commented Mar 14, 2023 •

edited by aravindhp

saifshaikh48 Mar 14, 2023 •

edited

mansikulkarni96 Mar 15, 2023 •

edited

saifshaikh48 commented Mar 16, 2023 •

edited

saifshaikh48 commented Mar 16, 2023 •

edited