chore: address comments for monitoring component #1520

zdtsw · 2025-01-19T16:47:30Z

move if to switch...case: Update Component rules in Prometheus #1503 (comment)
add .status.condition.MonitoringReady type: Update Component rules in Prometheus #1503 (comment)
- change Monitoring CR .status.condition Reason and Message and Type name
- remove unused predicate var from DSCI
- change check on promethus deployment ready
- update: change to use Apply than Create
- update: add or remove prom rules
- add field manager for monitoring CR to DSCI
- add isComponentReady()
- update predicate for monitoring on DSC change on both .spec.components and .status.condition

Description

How Has This Been Tested?

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-19T19:23:34Z

/test opendatahub-operator-e2e

codecov · 2025-01-19T20:51:45Z

Codecov Report

Attention: Patch coverage is 0% with 57 lines in your changes missing coverage. Please review.

Project coverage is 19.63%. Comparing base (3e897ce) to head (971099e).
Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
...rvices/monitoring/monitoring_controller_actions.go	0.00%	55 Missing ⚠️
.../dscinitialization/dscinitialization_controller.go	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1520      +/-   ##
==========================================
- Coverage   19.69%   19.63%   -0.07%     
==========================================
  Files         161      161              
  Lines       11102    11137      +35     
==========================================
  Hits         2187     2187              
- Misses       8683     8718      +35     
  Partials      232      232

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

biswassri · 2025-01-20T07:34:12Z

@zdtsw Can you help also link the PR comment here? So that anyone trying to understand the changes can go back and read the context?

zdtsw · 2025-01-20T07:46:39Z

@zdtsw Can you help also link the PR comment here? So that anyone trying to understand the changes can go back and read the context?

updated in description.

lburgazzoli · 2025-01-20T08:27:06Z

controllers/services/monitoring/monitoring.go

 	odhdeploy "github.com/opendatahub-io/opendatahub-operator/v2/pkg/deploy"
 )

 var (
 	ComponentName        = serviceApi.MonitoringServiceName
 	prometheusConfigPath = filepath.Join(odhdeploy.DefaultManifestPath, ComponentName, "prometheus", "apps", "prometheus-configs.yaml")
+	ReadyConditionType   = conditionsv1.ConditionType(serviceApi.MonitoringKind + status.ReadySuffix)


for service/component, the ready condition should just be Ready, the suffix is only needed if the service/component specific readiness condition is exposed in an higher level API (i.e. DSC/DSCI)

lburgazzoli · 2025-01-20T08:32:02Z

controllers/services/monitoring/monitoring_controller_actions.go

 	if len(promDeployment.Items) == 1 && ready == 1 {
+		// TODO: deprecate phase


the if before should either be rewritten like len(promDeployment.Items) == ready or there are more than the expected number of deployment should be reported as part of the failure condition

lburgazzoli · 2025-01-20T08:34:18Z

controllers/services/monitoring/monitoring_controller_actions.go

+	nc := metav1.Condition{
+		Type:    string(ReadyConditionType),
+		Status:  metav1.ConditionFalse,
+		Reason:  status.ReconcileInit,


I'm not sure if ReconcileInit makes any sense to be honest, so if the deployment won't get up and running, then we would leave the reason to ReconcileInit forever

updated not usre if it is good to use

Type: string(ReadyConditionType), Status: metav1.ConditionFalse, Reason: status.PhaseNotReady, Message: "Prometheus deployment is not ready",

- more if to switch...case - add .status.condition.MonitoringReady type Signed-off-by: Wen Zhou <[email protected]>

- change Monitoring CR .status.condition Reason and Message and Type name - remove unused predicate var - change check on promethus deployment ready Signed-off-by: Wen Zhou <[email protected]>

lburgazzoli · 2025-01-20T11:42:32Z

controllers/dscinitialization/dscinitialization_controller.go

@@ -501,7 +501,10 @@ func (r *DSCInitializationReconciler) configureMonitoring(ctx context.Context, d
 	)

 	if dsci.Spec.Monitoring.ManagementState == operatorv1.Managed {
-		err := r.Create(ctx, defaultMonitoring)
+		// for generic case if we need to support configable monitoring namespace
+		_, err := controllerutil.CreateOrUpdate(ctx, r.Client, defaultMonitoring, func() error {


you can probably replace it with server side apply r.Apply(..)

Signed-off-by: Wen Zhou <[email protected]>

lburgazzoli · 2025-01-20T17:56:07Z

controllers/dscinitialization/dscinitialization_controller.go

-		err := r.Create(ctx, defaultMonitoring)
-		if err != nil && !k8serr.IsAlreadyExists(err) {
+		// for generic case if we need to support configable monitoring namespace
+		if err := r.Apply(ctx, defaultMonitoring); err != nil && !k8serr.IsAlreadyExists(err) {


setting field owner is required, something like

opendatahub-operator/controllers/datasciencecluster/datasciencecluster_controller.go

Lines 243 to 250 in 6a762ad

err := ctrl.SetControllerReference(instance, componentCR, r.Scheme)

if err != nil {

return nil, err

}

err = r.Client.Apply(ctx, componentCR, client.FieldOwner(fieldOwner), client.ForceOwnership)

if err != nil {

return nil, err

}

lburgazzoli · 2025-01-20T17:57:06Z

controllers/services/monitoring/monitoring_controller_actions.go

+			}
+		} else {
+			falseVal := false
+			updated = &falseVal


you can use pointer.Bool

the meta.IsStatusConditionTrue(ci.GetStatus().Conditions, status.ConditionTypeReady) == true can also be moved to a case

lburgazzoli · 2025-01-20T18:31:27Z

controllers/services/monitoring/monitoring_controller_actions.go

 		}

 		// Check for shared components
 		if ch.GetName() == componentApi.KserveComponentName || ch.GetName() == componentApi.ModelMeshServingComponentName {
-			if err := UpdatePrometheusConfig(ctx, enabled, componentRules[componentApi.ModelControllerComponentName]); err != nil {
+			if err := UpdatePrometheusConfig(ctx, *updated, componentRules[componentApi.ModelControllerComponentName]); err != nil {


can this result in a panic due to a nil ptr ?

lburgazzoli · 2025-01-20T18:31:32Z

controllers/services/monitoring/monitoring_controller_actions.go

 				return err
 			}
 		}

-		if err := UpdatePrometheusConfig(ctx, enabled, componentRules[ch.GetName()]); err != nil {
+		if err := UpdatePrometheusConfig(ctx, *updated, componentRules[ch.GetName()]); err != nil {


can this result in a panic due to a nil ptr ?

lburgazzoli · 2025-01-20T18:32:22Z

controllers/services/monitoring/monitoring_controller_actions.go

 		}

 		// Check for shared components
 		if ch.GetName() == componentApi.KserveComponentName || ch.GetName() == componentApi.ModelMeshServingComponentName {
-			if err := UpdatePrometheusConfig(ctx, enabled, componentRules[componentApi.ModelControllerComponentName]); err != nil {
+			if err := UpdatePrometheusConfig(ctx, *updated, componentRules[componentApi.ModelControllerComponentName]); err != nil {


I guess here you have to check also for model controller readiness ?

lburgazzoli · 2025-01-20T20:08:17Z

controllers/services/monitoring/monitoring_controller_actions.go

+				trueVal := true
+				updated = &trueVal
+
+				if err := UpdatePrometheusConfig(ctx, *updated, componentRules[ch.GetName()]); err != nil {


looks like you don't need the updated var since it is always true

lburgazzoli · 2025-01-20T20:08:24Z

controllers/services/monitoring/monitoring_controller_actions.go

+			if !dsc.Status.InstalledComponents["model-mesh"] && !dsc.Status.InstalledComponents["kserve"] {
+				falseVal := false
+				updated = &falseVal
+				if err := UpdatePrometheusConfig(ctx, *updated, componentRules[ch.GetName()]); err != nil {


looks like you don't need the updated var since it is always false

controllers/services/monitoring/monitoring_controller_actions.go

- update on modelcontroller - add filed manager for monitoring CR to DSCI Signed-off-by: Wen Zhou <[email protected]>

- remove duplicated function from pkg/service/montiroing - add isComponentReady() Signed-off-by: Wen Zhou <[email protected]>

Signed-off-by: Wen Zhou <[email protected]>

- remove duplicated monitoring predicates from DSCI controller - update predicate for monitoring on DSC change on both .spec.components and .status.components Signed-off-by: Wen Zhou <[email protected]>

Signed-off-by: Wen Zhou <[email protected]>

lburgazzoli · 2025-01-21T10:11:05Z

pkg/controller/predicates/resources/resources.go

+		}
+
+		// compare type one by one with their status if not equal return true
+		for _, nc := range newConditions {


this can probably made a little bit more efficient, but ok for now

lburgazzoli · 2025-01-21T10:12:36Z

controllers/dscinitialization/dscinitialization_controller.go

-		if err != nil && !k8serr.IsAlreadyExists(err) {
+		// for generic case if we need to support configable monitoring namespace
+		// set filed manager to DSCI
+		if err := r.Apply(ctx, defaultMonitoring, client.FieldOwner("dscinitialization.opendatahub.io"), client.ForceOwnership); err != nil && !k8serr.IsAlreadyExists(err) {


nit: there is a finalizerName const

openshift-ci · 2025-01-21T10:18:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lburgazzoli

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

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

openshift-ci bot requested review from biswassri and jackdelahunt January 19, 2025 16:47

zdtsw requested review from lburgazzoli and VaishnaviHire and removed request for jackdelahunt and biswassri January 19, 2025 16:47

zdtsw changed the title ~~chore: address comments from pervious review~~ chore: address comments for monitoring component Jan 19, 2025

zdtsw force-pushed the chore_pr_1503 branch from 4f3d21a to 0122376 Compare January 19, 2025 17:33

lburgazzoli reviewed Jan 20, 2025

View reviewed changes

zdtsw added 2 commits January 20, 2025 12:29

chore: address comment from pervious review

9567c6d

- more if to switch...case - add .status.condition.MonitoringReady type Signed-off-by: Wen Zhou <[email protected]>

update:

b506c3f

- change Monitoring CR .status.condition Reason and Message and Type name - remove unused predicate var - change check on promethus deployment ready Signed-off-by: Wen Zhou <[email protected]>

lburgazzoli reviewed Jan 20, 2025

View reviewed changes

zdtsw added the do-not-merge/work-in-progress label Jan 20, 2025

zdtsw mentioned this pull request Jan 20, 2025

[rhoai] Update Component rules in Prometheus #1511

Draft

5 tasks

update: change to use Apply than CreateOrUpdate call

e64e0a5

Signed-off-by: Wen Zhou <[email protected]>

zdtsw force-pushed the chore_pr_1503 branch from 9916c7b to 39a51c2 Compare January 20, 2025 17:47

lburgazzoli reviewed Jan 20, 2025

View reviewed changes

zdtsw force-pushed the chore_pr_1503 branch from 39a51c2 to 971099e Compare January 20, 2025 19:50

lburgazzoli reviewed Jan 20, 2025

View reviewed changes

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

zdtsw added 2 commits January 21, 2025 08:32

update: add or remove prom rules

acddae1

- update on modelcontroller - add filed manager for monitoring CR to DSCI Signed-off-by: Wen Zhou <[email protected]>

update:

0f35430

- remove duplicated function from pkg/service/montiroing - add isComponentReady() Signed-off-by: Wen Zhou <[email protected]>

zdtsw force-pushed the chore_pr_1503 branch from 971099e to 0f35430 Compare January 21, 2025 08:10

zdtsw added 3 commits January 21, 2025 09:27

update: remove special check on modelcontroller

1beffea

Signed-off-by: Wen Zhou <[email protected]>

update: predicates

cdb17df

- remove duplicated monitoring predicates from DSCI controller - update predicate for monitoring on DSC change on both .spec.components and .status.components Signed-off-by: Wen Zhou <[email protected]>

update: more on the predicated for monitoring on DSC .status.condition

3756f1c

Signed-off-by: Wen Zhou <[email protected]>

lburgazzoli reviewed Jan 21, 2025

View reviewed changes

lburgazzoli approved these changes Jan 21, 2025

View reviewed changes

openshift-ci bot assigned lburgazzoli Jan 21, 2025

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

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

zdtsw merged commit 05c7947 into opendatahub-io:main Jan 21, 2025
6 of 8 checks passed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: address comments for monitoring component #1520

chore: address comments for monitoring component #1520

zdtsw commented Jan 19, 2025 •

edited

Loading

zdtsw commented Jan 19, 2025

codecov bot commented Jan 19, 2025 •

edited

Loading

biswassri commented Jan 20, 2025

zdtsw commented Jan 20, 2025

lburgazzoli Jan 20, 2025

zdtsw Jan 20, 2025

lburgazzoli Jan 20, 2025

zdtsw Jan 20, 2025

lburgazzoli Jan 20, 2025

zdtsw Jan 20, 2025

lburgazzoli Jan 20, 2025

lburgazzoli Jan 20, 2025 •

edited

Loading

lburgazzoli Jan 20, 2025

lburgazzoli Jan 20, 2025

lburgazzoli Jan 20, 2025

lburgazzoli Jan 20, 2025

lburgazzoli Jan 20, 2025

lburgazzoli Jan 20, 2025

lburgazzoli Jan 20, 2025

lburgazzoli Jan 21, 2025

lburgazzoli Jan 21, 2025 •

edited

Loading

openshift-ci bot commented Jan 21, 2025

		if len(promDeployment.Items) == 1 && ready == 1 {
		// TODO: deprecate phase

	err := ctrl.SetControllerReference(instance, componentCR, r.Scheme)
	if err != nil {
	return nil, err
	}
	err = r.Client.Apply(ctx, componentCR, client.FieldOwner(fieldOwner), client.ForceOwnership)
	if err != nil {
	return nil, err
	}

chore: address comments for monitoring component #1520

chore: address comments for monitoring component #1520

Conversation

zdtsw commented Jan 19, 2025 • edited Loading

Description

How Has This Been Tested?

Screenshot or short clip

Merge criteria

zdtsw commented Jan 19, 2025

codecov bot commented Jan 19, 2025 • edited Loading

Codecov Report

biswassri commented Jan 20, 2025

zdtsw commented Jan 20, 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

lburgazzoli Jan 20, 2025 • edited Loading

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

Choose a reason for hiding this comment

lburgazzoli Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

openshift-ci bot commented Jan 21, 2025

zdtsw commented Jan 19, 2025 •

edited

Loading

codecov bot commented Jan 19, 2025 •

edited

Loading

lburgazzoli Jan 20, 2025 •

edited

Loading

lburgazzoli Jan 21, 2025 •

edited

Loading