Update Component rules in Prometheus #1503

VaishnaviHire · 2025-01-16T07:08:51Z

Description

The scope of this PR is reduced to only watch and update prometheus configmap for components.

How Has This Been Tested?

Deploy Managed Instance:
- Create dummy secrets - (Refer olm install script in gitlab)
- Deploy Managed Catalogsource: quay.io/vhire/opendatahub-operator-catalog:v2.17.0-rc4
Expect alerts only for components that are in Managed state

Screenshot or short clip

Merge criteria

You have read the contributors guide.
Commit messages are meaningful - have a clear and concise summary and detailed explanation of what was changed and why.
Pull Request contains a description of the solution, a link to the JIRA issue, and to any dependent or related Pull Request.
Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
The developer has manually tested the changes and verified that the changes work

zdtsw · 2025-01-16T09:24:31Z

Dockerfiles/Dockerfile

+# Copy monitoring config
+COPY config/monitoring/ /opt/manifests/monitoring
+# Copy partners config
+COPY config/partners/ /opt/manifests/partners


we can skip partners one, with #1504

we can skip partners one, with #1504

Why for building it cannot copy just everything?

is the question about why not make it as COPY config/ /opt/manifests/ in Dockefile?
if so, we have more under config folder which no need to get into final image

VaishnaviHire · 2025-01-17T05:43:29Z

/test opendatahub-operator-e2e

codecov · 2025-01-17T06:52:36Z

Codecov Report

Attention: Patch coverage is 0.79681% with 249 lines in your changes missing coverage. Please review.

Project coverage is 19.69%. Comparing base (e838877) to head (29a8c29).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
controllers/services/monitoring/monitoring.go	0.00%	88 Missing ⚠️
...rvices/monitoring/monitoring_controller_actions.go	0.00%	67 Missing ⚠️
.../dscinitialization/dscinitialization_controller.go	4.87%	39 Missing ⚠️
pkg/controller/predicates/resources/resources.go	0.00%	23 Missing ⚠️
pkg/controller/handlers/handlers.go	0.00%	15 Missing ⚠️
pkg/controller/predicates/component/component.go	0.00%	11 Missing ⚠️
...llers/services/monitoring/monitoring_controller.go	0.00%	3 Missing ⚠️
controllers/dscinitialization/monitoring.go	0.00%	2 Missing ⚠️
pkg/services/monitoring/prometheus.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1503      +/-   ##
==========================================
- Coverage   20.04%   19.69%   -0.35%     
==========================================
  Files         160      161       +1     
  Lines       10900    11102     +202     
==========================================
+ Hits         2185     2187       +2     
- Misses       8483     8683     +200     
  Partials      232      232

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

controllers/dscinitialization/monitoring.go

main.go

controllers/services/monitoring/monitoring_controller_actions.go

zdtsw · 2025-01-17T08:02:05Z

controllers/services/monitoring/monitoring_controller_actions.go

-		}),
-	)
+	err := cr.ForEach(func(ch cr.ComponentHandler) error {
+		enabled := cr.IsManaged(ch, dsc) && dsc.Status.Phase == status.ReadySuffix


i am not sure about this part "dsc.Status.Phase == status.ReadySuffix"
do we need a special case on odh-model-controller?

The Status is for Dsc, so we should not need any additional condition for odh-model-controller

To get the actual component status, a better way would be:

ci := component.NewCRObject(instance) // read the component instance to get tha actual status err = cli.Get(ctx, client.ObjectKeyFromObject(ci), ci) switch { case k8serr.IsNotFound(err): enabled = false case err != nil: enabled = false // TODO: error handling default: enabled = meta.IsStatusConditionTrue(ci.GetStatus().Conditions, status.ConditionTypeReady) }

thinking a little bit more about this comment, we probably want to distinguish between enabled and ready. If I recall the old implementation, we were supposed to install the rule after the first time the component become ready, and we remove the rules of the component gets removed

I would like also mention this issue red-hat-data-services#316 which was from my opinion hidden by changing of timings.

controllers/services/monitoring/monitoring_controller.go

controllers/dscinitialization/dscinitialization_controller.go

controllers/services/monitoring/monitoring_controller_actions.go

controllers/services/monitoring/monitoring_controller.go

VaishnaviHire · 2025-01-17T16:30:54Z

/test opendatahub-operator-e2e

controllers/services/monitoring/monitoring_controller_actions.go

lburgazzoli · 2025-01-17T16:42:31Z

controllers/services/monitoring/monitoring_controller_actions.go

+		return nil
+	}
+	dsc := &dscList.Items[0]
+	componentRules := map[string]string{


nit: this map can probably be moved to a global var

controllers/services/monitoring/monitoring_controller_actions.go

lburgazzoli · 2025-01-17T16:45:43Z

controllers/services/monitoring/monitoring_controller_actions.go

 	}

+	m.Status.Phase = "Ready"
+	m.Status.ObservedGeneration = m.GetObjectMeta().GetGeneration()


ideally we should also have a Ready condition

add an NoReady case, only set to Ready when Monitoring Prom deployment is ready

I meant a ready Condition, phase is not that much in use

sorry, did not see this, let me get a new one next week for this comment

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

lburgazzoli · 2025-01-18T05:41:58Z

controllers/services/monitoring/monitoring_controller_actions.go

+		switch {
+		case err != nil:
+			enabled = false
+			if !k8serr.IsNotFound(err) {


nit: this could have been a switch case

same as here, will have the new commit to follow

openshift-ci · 2025-01-18T06:22:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: zdtsw

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

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

VaishnaviHire requested review from zdtsw and lburgazzoli January 16, 2025 07:08

openshift-ci bot requested review from lphiri and Sara4994 January 16, 2025 07:09

VaishnaviHire marked this pull request as draft January 16, 2025 07:09

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

VaishnaviHire removed request for lphiri and Sara4994 January 16, 2025 07:09

zdtsw reviewed Jan 16, 2025

View reviewed changes

VaishnaviHire force-pushed the add_monitoring_ctrl branch 2 times, most recently from 60d1f2c to ef677d0 Compare January 16, 2025 19:52

VaishnaviHire marked this pull request as ready for review January 16, 2025 19:53

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

openshift-ci bot requested review from grdryn and ugiordan January 16, 2025 19:53

VaishnaviHire changed the title ~~Implement Monitoring Controller~~ Update Component rules in Prometheus Jan 16, 2025

VaishnaviHire force-pushed the add_monitoring_ctrl branch 2 times, most recently from 9da2003 to b9d91bd Compare January 17, 2025 03:48

zdtsw reviewed Jan 17, 2025

View reviewed changes

VaishnaviHire force-pushed the add_monitoring_ctrl branch 6 times, most recently from 71a8618 to 9db60ba Compare January 17, 2025 13:50

zdtsw approved these changes Jan 17, 2025

View reviewed changes

controllers/services/monitoring/monitoring_controller.go Show resolved Hide resolved

openshift-ci bot assigned zdtsw Jan 17, 2025

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

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

Update Component rules in Prometheus

bcdd7c8

VaishnaviHire force-pushed the add_monitoring_ctrl branch from 9db60ba to bcdd7c8 Compare January 17, 2025 14:40

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

zdtsw approved these changes Jan 17, 2025

View reviewed changes

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

lburgazzoli reviewed Jan 17, 2025

View reviewed changes

controllers/services/monitoring/monitoring_controller_actions.go Outdated Show resolved Hide resolved

lburgazzoli reviewed Jan 17, 2025

View reviewed changes

controllers/services/monitoring/monitoring_controller_actions.go Show resolved Hide resolved

lburgazzoli reviewed Jan 17, 2025

View reviewed changes

controllers/services/monitoring/monitoring_controller_actions.go Show resolved Hide resolved

lburgazzoli reviewed Jan 17, 2025

View reviewed changes

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

update: address some comments

cab3a3a

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

zdtsw force-pushed the add_monitoring_ctrl branch from 96b545c to cab3a3a Compare January 17, 2025 17:21

zdtsw added 2 commits January 17, 2025 18:30

update: add monitoring CR status on condition of prom deployment

e0b7c39

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

Merge branch 'main' into add_monitoring_ctrl

29a8c29

lburgazzoli reviewed Jan 18, 2025

View reviewed changes

zdtsw approved these changes Jan 18, 2025

View reviewed changes

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

zdtsw merged commit 9b062f6 into opendatahub-io:main Jan 18, 2025
9 of 10 checks passed

zdtsw mentioned this pull request Jan 20, 2025

chore: address comments for monitoring component #1520

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Component rules in Prometheus #1503

Update Component rules in Prometheus #1503

VaishnaviHire commented Jan 16, 2025 •

edited

Loading

zdtsw Jan 16, 2025

ykaliuta Jan 16, 2025

zdtsw Jan 19, 2025

VaishnaviHire commented Jan 17, 2025

codecov bot commented Jan 17, 2025 •

edited

Loading

zdtsw Jan 17, 2025

VaishnaviHire Jan 17, 2025

lburgazzoli Jan 17, 2025

lburgazzoli Jan 18, 2025

ykaliuta Jan 18, 2025

VaishnaviHire commented Jan 17, 2025

lburgazzoli Jan 17, 2025

zdtsw Jan 17, 2025

lburgazzoli Jan 17, 2025

zdtsw Jan 17, 2025

lburgazzoli Jan 18, 2025

zdtsw Jan 18, 2025

lburgazzoli Jan 18, 2025

zdtsw Jan 18, 2025

openshift-ci bot commented Jan 18, 2025

Update Component rules in Prometheus #1503

Update Component rules in Prometheus #1503

Conversation

VaishnaviHire commented Jan 16, 2025 • edited Loading

Description

How Has This Been Tested?

Screenshot or short clip

Merge criteria

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VaishnaviHire commented Jan 17, 2025

codecov bot commented Jan 17, 2025 • edited Loading

Codecov Report

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

VaishnaviHire commented Jan 17, 2025

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

openshift-ci bot commented Jan 18, 2025

VaishnaviHire commented Jan 16, 2025 •

edited

Loading

codecov bot commented Jan 17, 2025 •

edited

Loading