Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use a unique label for monitoring resources #178

Merged

Conversation

afreen23
Copy link

@afreen23 afreen23 commented Feb 23, 2022

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=2043034

Issue

Both csi-addon operator and odf-operator are using the default label and label-selector provided by operator-sdk - "control-plane: controller-manager" for its services and deployments.
This is allowing deployments, services and service monitors of each operator to select the pods ans services of other operator.
One critical impact of this is in UI monitoring where odf metrics monitor is picking up csiaddon service and hence showing an extra storage system. This is breaking the prometheus query and hence missing storage system on UI.

Fix

Use a unique label selector in odf-operator to avoid conflicts.

Before:
Screenshot from 2022-02-23 21-28-06

After:
Screenshot from 2022-02-23 19-44-12

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 23, 2022

@afreen23: This pull request references Bugzilla bug 2043034, which is valid.

No validations were run on this bug

Requesting review from QA contact:
/cc @ebenahar

In response to this:

Bug 2043034: Use a unique label for monitoring resources

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 openshift-ci bot requested a review from ebenahar February 23, 2022 12:25
csi-addon operator and odf-operator are using the default label and
labelSelector provided by operator-sdk:control-plane:controller-manager
for its services. This is allowing deployments, services and service
monitors of each operator to select the pods and services of other
operator.One critical impact of this is in UI dashboard where odf
service monitor is picking up csiaddon service and hence showing an
extra storage system.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=2043034

Signed-off-by: Afreen Rahman <afrahman@redhat.com>
@afreen23
Copy link
Author

@iamniting @umangachapagain please review

@iamniting iamniting changed the title Bug 2043034: Use a unique label for monitoring resources Use a unique label for monitoring resources Feb 23, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 23, 2022

@afreen23: No Bugzilla bug is referenced in the title of this pull request.
To reference a bug, add 'Bug XXX:' to the title of this pull request and request another bug refresh with /bugzilla refresh.

In response to this:

Use a unique label for monitoring resources

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.

@iamniting
Copy link
Member

iamniting commented Feb 23, 2022

@afreen23 changes look good to me as they are really simple, But I have questions before I give LGTM.

  1. Will this require a change in the openshift 4.10 console code too.
  2. If yes Will openshift console code work with the odf 4.9 until we release 4.10 as openshift gets released before odf.
  3. or this has nothing to do with the openshift console code and matches labels b/w themselves.

@iamniting iamniting added the backport-release-4.10 Indicate PR requires to be backported label Feb 23, 2022
@iamniting
Copy link
Member

/cherry-pick release-4.10

@openshift-cherrypick-robot

@iamniting: once the present PR merges, I will cherry-pick it on top of release-4.10 in a new PR and assign it to you.

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.

@afreen23
Copy link
Author

@afreen23 changes look good to me as they are really simple, But I have questions before I give LGTM.

  1. Will this require a change in the openshift 4.10 console code too.
  2. If yes Will openshift console code work with the odf 4.9 until we release 4.10 as openshift gets released before odf.
  3. or this has nothing to do with the openshift console code and matches labels b/w themselves.

Thanks for the review @iamniting
No change is required in openshift 4.10 console code .

Details:
The prometheus query used on dashboard is failing currently. odf_system_map query is listing two storage systems (due to #178 (comment)) and when we join it with another query it starts failing since many to many matching occurs.

Screenshot from 2022-02-24 00-35-25

@afreen23 afreen23 changed the title Use a unique label for monitoring resources Bug 2043034: Use a unique label for monitoring resources Feb 23, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 23, 2022

@afreen23: This pull request references Bugzilla bug 2043034, which is valid.

No validations were run on this bug

No GitHub users were found matching the public email listed for the QA contact in Bugzilla (musoni@redhat.com), skipping review request.

In response to this:

Bug 2043034: Use a unique label for monitoring resources

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.

@agarwal-mudit agarwal-mudit changed the title Bug 2043034: Use a unique label for monitoring resources Use a unique label for monitoring resources Feb 24, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 24, 2022

@afreen23: No Bugzilla bug is referenced in the title of this pull request.
To reference a bug, add 'Bug XXX:' to the title of this pull request and request another bug refresh with /bugzilla refresh.

In response to this:

Use a unique label for monitoring resources

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.

Copy link
Member

@iamniting iamniting left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 24, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 24, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afreen23, iamniting

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:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 24, 2022
@openshift-merge-robot openshift-merge-robot merged commit 2755398 into red-hat-storage:main Feb 24, 2022
21 checks passed
@openshift-cherrypick-robot

@iamniting: new pull request created: #180

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. backport-release-4.10 Indicate PR requires to be backported lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants