update: change dsci monitoring type #1373

zdtsw · 2024-11-15T09:50:21Z

update: change dsci monitoring type:
move check into a function
fix the case when user remove entire monitoring block from DSCI, it should not still show namespace and cause confusion

Description

when in ODH or self-managed case, user can chose to either set monitoring.managementstate to remove or remove the default monitoring block, in order to disable monitoring function.

for the 2nd case "remove monitoring block"
by doing like this to create/patch DSCI

yaml:

spec:
  applicationsNamespace: redhat-ods-applications
  serviceMesh:
    controlPlane:
      metricsCollection: Istio
      name: data-science-smcp
      namespace: istio-system
    managementState: Managed

with the PR(pointer) after DSCI is updated:

spec:
  applicationsNamespace: redhat-ods-applications
  serviceMesh:
    auth:
      audiences:
        - 'https://kubernetes.default.svc'
    controlPlane:
      metricsCollection: Istio
      name: data-science-smcp
      namespace: istio-system
    managementState: Managed

with the current incubation (struct) after DSCI is updated:

spec:
  applicationsNamespace: redhat-ods-applications
  monitoring:
    namespace: redhat-ods-monitoring   =========> here, because we set default value for namespace, so it will always be shown there
   serviceMesh:
    auth:
      audiences:
        - 'https://kubernetes.default.svc'
    controlPlane:
      metricsCollection: Istio
      name: data-science-smcp
      namespace: istio-system
    managementState: Managed

related but not to fix https://issues.redhat.com/browse/RHOAISTRAT-407

How Has This Been Tested?

create DSCI with need sepcifiy monitoring namespace:

apiVersion:
kind: DSCInitialization 
metadata:
  name: default-dsci
spec:
  applicationsNamespace: redhat-ods-applications
   serviceMesh:
    managementState: Removed 
  trustedCABundle:
    managementState: Managed
    customCABundle: ""

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

openshift-ci · 2024-11-15T09:50:33Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from zdtsw. For more information see the Kubernetes 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

ykaliuta · 2024-11-15T11:16:26Z

I'm not arguing, but any background why it's needed? Just general improvement or some request?

zdtsw · 2024-11-15T11:28:06Z

I'm not arguing, but any background why it's needed? Just general improvement or some request?

was finding during the namespace test. #1321
the code we have now, regardless odh, self or managed, already have montiong set to enabled, and created namespace and some extra resources.
but these are not needed except for managed cluster.
so this change is a prepartion for the later to disable above logic, and give better visibility to user. since other managementstate, we show value when it is set to removed. but this one is not. but still leave namespace value there.

ykaliuta · 2024-11-15T13:15:43Z

Could you describe how it is supposed to work? Probably, omitempty is not involved since for both pointer and structure Monitoring structure is not empty (contains namespace).

In my check both work equally. I mean, if I create dsci with explicit value for monitoring state, it preserved, otherwise it is omitted.

Should it be configured with default value?

codecov · 2024-11-16T11:07:31Z

Codecov Report

Attention: Patch coverage is 16.66667% with 15 lines in your changes missing coverage. Please review.

Please upload report for BASE (incubation@0b4b1f4). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
.../dscinitialization/dscinitialization_controller.go	16.66%	5 Missing ⚠️
controllers/dscinitialization/monitoring.go	0.00%	5 Missing ⚠️
pkg/common/common.go	0.00%	5 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             incubation    #1373   +/-   ##
=============================================
  Coverage              ?   18.88%           
=============================================
  Files                 ?       30           
  Lines                 ?     3404           
  Branches              ?        0           
=============================================
  Hits                  ?      643           
  Misses                ?     2692           
  Partials              ?       69

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

zdtsw · 2024-11-16T11:13:08Z

Could you describe how it is supposed to work? Probably, omitempty is not involved since for both pointer and structure Monitoring structure is not empty (contains namespace).

In my check both work equally. I mean, if I create dsci with explicit value for monitoring state, it preserved, otherwise it is omitted.

Should it be configured with default value?

realize that i put a wrong description yesterday which caused confusion.
now it is updated.
hopefully this is more clear.

ykaliuta · 2024-11-17T23:06:10Z

pkg/common/common.go

@@ -116,3 +120,10 @@ func GetMonitoringData(data string) (string, error) {

 	return encodedData, nil
 }
+
+func IsMonitoringEnabled(dscMonitoring *dsciv1.Monitoring) bool {
+	if dscMonitoring != nil && dscMonitoring.ManagementState == operatorv1.Managed {


It's return dscMonitoring != nil && dscMonitoring.ManagementState == operatorv1.Managed
(probably shorter parameter name works better for one line)

you mean something like:

func IsMonitoringEnabled(monitoring *dsciv1.Monitoring) bool { if monitoring != nil && monitoring.ManagementState == operatorv1.Managed {

?

Like this

func IsMonitoringEnabled(m *dsciv1.Monitoring) bool { return m != nil && m.ManagementState == operatorv1.Managed }

since for such a short code it's obvious from the declaration what m means.
But I do not insist.

ykaliuta · 2024-11-18T09:59:40Z

realize that i put a wrong description yesterday which caused confusion. now it is updated. hopefully this is more clear.

Thanks!
I still get the same behaviour for 2 versions (incubation and the branch). What I'm doing:

I have the yaml:

apiVersion: dscinitialization.opendatahub.io/v1
kind: DSCInitialization
metadata:
  name: default-dsci
spec:
  applicationsNamespace: opendatahub
  trustedCABundle:
    managementState: Managed
  serviceMesh:
    controlPlane:
      metricsCollection: Istio
      name: data-science-smcp
      namespace: istio-system
    managementState: Managed

Automatic DSCI creation enabled.
I get

 % oc get dsci default-dsci -o yaml | yq .spec
applicationsNamespace: opendatahub
monitoring:
  managementState: Managed
  namespace: opendatahub
serviceMesh:
  auth:
    audiences:
      - https://kubernetes.default.svc
  controlPlane:
    metricsCollection: Istio
    name: data-science-smcp
    namespace: istio-system
  managementState: Managed
trustedCABundle:
  customCABundle: ""
  managementState: Managed

in both cases.

% oc apply -f /tmp/dsci.yaml                 
Warning: resource dscinitializations/default-dsci is missing the kubectl.kubernetes.io/last-applied-configuration annotation which is required by oc apply. oc apply should only be used on resources created declaratively by either oc create --save-config or oc apply. The missing annotation will be patched automatically.
dscinitialization.dscinitialization.opendatahub.io/default-dsci configured

does not change anything.

Manual dsci creation:

% oc get dsci default-dsci -o yaml | yq .spec
applicationsNamespace: opendatahub
serviceMesh:
  controlPlane:
    metricsCollection: Istio
    name: data-science-smcp
    namespace: istio-system
  managementState: Managed
trustedCABundle:
  customCABundle: ""
  managementState: Managed

in both cases.

zdtsw · 2024-11-18T10:22:33Z

monitoring:
managementState: Managed
namespace: opendatahub

you are saying,
when you do clean install ODH operator 2.20, and manual create DSCI with :

apiVersion: dscinitialization.opendatahub.io/v1
kind: DSCInitialization
metadata:
  name: default-dsci
spec:
  applicationsNamespace: opendatahub
  trustedCABundle:
    managementState: Managed
  serviceMesh:
    controlPlane:
      metricsCollection: Istio
      name: data-science-smcp
      namespace: istio-system
    managementState: Managed

when DSCI is in Ready status, you get result from .spec as:

applicationsNamespace: opendatahub
serviceMesh:
  controlPlane:
    metricsCollection: Istio
    name: data-science-smcp
    namespace: istio-system
  managementState: Managed
trustedCABundle:
  customCABundle: ""
  managementState: Managed

but not:

spec:
  applicationsNamespace: opendatahub
  monitoring:
    namespace: opendatahub
  serviceMesh:
    auth:
      audiences:
        - 'https://kubernetes.default.svc'
    controlPlane:
      metricsCollection: Istio
      name: data-science-smcp
      namespace: istio-system
    managementState: Managed
  trustedCABundle:
    customCABundle: ''
    managementState: Managed

ykaliuta · 2024-11-18T10:41:42Z

monitoring:
managementState: Managed
namespace: opendatahub

you are saying, when you do clean install ODH operator 2.20, and manual create DSCI with :

[...]

when DSCI is in Ready status, you get result from .spec as:

[...]

but not:

Well, I'm saying that it is the same for both cases for me, but I rechecked and no, you are right, it works as expected/you described.

ykaliuta · 2024-11-18T10:56:09Z

looks good to me

- move check into a function - fix the case when user remove entire monitoring block from DSCI Signed-off-by: Wen Zhou <[email protected]>

openshift-ci bot requested review from CFSNM and Sara4994 November 15, 2024 09:50

zdtsw requested review from lburgazzoli and VaishnaviHire and removed request for CFSNM and Sara4994 November 15, 2024 09:50

zdtsw marked this pull request as draft November 15, 2024 18:55

openshift-ci bot added the do-not-merge/work-in-progress label Nov 15, 2024

zdtsw force-pushed the chore_142 branch from a4d99f0 to a6e9da1 Compare November 16, 2024 11:05

zdtsw force-pushed the chore_142 branch 3 times, most recently from 2599747 to 48b2d0c Compare November 16, 2024 11:26

zdtsw marked this pull request as ready for review November 16, 2024 11:27

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

openshift-ci bot requested review from ajaypratap003 and mattmahoneyrh November 16, 2024 11:27

ykaliuta reviewed Nov 17, 2024

View reviewed changes

zdtsw force-pushed the chore_142 branch from 48b2d0c to fd2964e Compare November 18, 2024 07:50

zdtsw removed request for ajaypratap003 and mattmahoneyrh November 18, 2024 09:51

update: change dsci monitoring type

f46cfc2

- move check into a function - fix the case when user remove entire monitoring block from DSCI Signed-off-by: Wen Zhou <[email protected]>

zdtsw force-pushed the chore_142 branch from fd2964e to f46cfc2 Compare December 2, 2024 09:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update: change dsci monitoring type #1373

update: change dsci monitoring type #1373

zdtsw commented Nov 15, 2024 •

edited

Loading

openshift-ci bot commented Nov 15, 2024

ykaliuta commented Nov 15, 2024

zdtsw commented Nov 15, 2024 •

edited

Loading

ykaliuta commented Nov 15, 2024 •

edited

Loading

codecov bot commented Nov 16, 2024 •

edited

Loading

zdtsw commented Nov 16, 2024

ykaliuta Nov 17, 2024

zdtsw Nov 18, 2024

ykaliuta Nov 18, 2024

zdtsw Nov 18, 2024

ykaliuta commented Nov 18, 2024

zdtsw commented Nov 18, 2024

ykaliuta commented Nov 18, 2024

ykaliuta commented Nov 18, 2024

update: change dsci monitoring type #1373

Are you sure you want to change the base?

update: change dsci monitoring type #1373

Conversation

zdtsw commented Nov 15, 2024 • edited Loading

Description

for the 2nd case "remove monitoring block" by doing like this to create/patch DSCI

How Has This Been Tested?

Screenshot or short clip

Merge criteria

openshift-ci bot commented Nov 15, 2024

ykaliuta commented Nov 15, 2024

zdtsw commented Nov 15, 2024 • edited Loading

ykaliuta commented Nov 15, 2024 • edited Loading

codecov bot commented Nov 16, 2024 • edited Loading

Codecov Report

zdtsw commented Nov 16, 2024

ykaliuta Nov 17, 2024

Choose a reason for hiding this comment

zdtsw Nov 18, 2024

Choose a reason for hiding this comment

ykaliuta Nov 18, 2024

Choose a reason for hiding this comment

zdtsw Nov 18, 2024

Choose a reason for hiding this comment

ykaliuta commented Nov 18, 2024

zdtsw commented Nov 18, 2024

ykaliuta commented Nov 18, 2024

ykaliuta commented Nov 18, 2024

zdtsw commented Nov 15, 2024 •

edited

Loading

for the 2nd case "remove monitoring block"
by doing like this to create/patch DSCI

zdtsw commented Nov 15, 2024 •

edited

Loading

ykaliuta commented Nov 15, 2024 •

edited

Loading

codecov bot commented Nov 16, 2024 •

edited

Loading