Extracted DB and Object Storage Error Logs to display on DSPA #487

amadhusu · 2023-11-22T20:58:51Z

The issue resolved by this Pull Request:

Resolves #451

Description of your changes:

Extracted DB and Object Storage Error Logs which were only visible in DSPO to display on DSPA Conditions with a brief message of any error.

Testing instructions

Deploy DSPO
Deploy a Minio in a self-signed external OCP cluster (preferablly different from the dsp host cluster)

oc new-project minio
oc -n minio apply -f https://gist.githubusercontent.com/HumairAK/f1358278ab70dde2ee074d1a35f8f058/raw/f234ba638f408c3aff8ed836858d0d1e332f43be/minio.yaml

# Retrieve the route
$MINIO_ROUTE=$(oc get routes -n minio minio-secure --template={{.spec.host}})

On DSP host cluster, deploy a dspa, first without the cabundle:

kind: Secret
apiVersion: v1
metadata:
  name: s3secret
  namespace: dspa
stringData:
  customaccessKey: accesskey
  customsecretKey: secretkey
type: Opaque
---
apiVersion: datasciencepipelinesapplications.opendatahub.io/v1alpha1
kind: DataSciencePipelinesApplication
metadata:
  name: sample
  namespace: dspa
spec:
  apiServer:
    enableSamplePipeline: false
  objectStorage:
    externalStorage:
      bucket: mlpipeline
      host: $MINIO_ROUTE  # <------------- add the route retrieved earlier !
      s3CredentialsSecret:
        accessKey: customaccessKey
        secretKey: customsecretKey
        secretName: s3secret
      scheme: https
  mlpipelineUI:
    image: 'quay.io/opendatahub/odh-ml-pipelines-frontend-container:beta-ui'

Check DSPO logs, you should see an error mentioning x509 and instructions on adding a cabundle.
Verify in the Conditions for sample DSPA instance that Condition Type ObjectStoreAvailable shows the same error mentioned in DSPO logs.

Checklist

The commits are squashed in a cohesive manner and have meaningful messages.
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

dsp-developers · 2023-11-22T21:01:07Z

A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-487
An OCP cluster where you are logged in as cluster admin is required.

To use this image run the following:

cd $(mktemp -d)
git clone git@github.com:opendatahub-io/data-science-pipelines-operator.git
cd data-science-pipelines-operator/
git fetch origin pull/487/head
git checkout -b pullrequest 0f483ae689090cb85936aa973847d10df6e73b2a
make deploy IMG="quay.io/opendatahub/data-science-pipelines-operator:pr-487"

More instructions here on how to deploy and test a Data Science Pipelines Application.

dsp-developers · 2023-11-23T09:16:21Z

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-487

controllers/database.go

dsp-developers · 2023-11-23T15:19:55Z

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-487

hbelmiro · 2023-11-24T12:55:08Z

/lgtm

gmfrasca · 2023-11-28T16:00:47Z

controllers/database.go

@@ -37,47 +38,49 @@ var mariadbTemplates = []string{
 }

 // extract to var for mocking in testing
-var ConnectAndQueryDatabase = func(host, port, username, password, dbname string, dbConnectionTimeout time.Duration) bool {
+var ConnectAndQueryDatabase = func(host, port, username, password, dbname string, dbConnectionTimeout time.Duration) (bool, string) {


if the intended purpose of message is for error handling, why don't we just return errors rather than strings and let the caller of this function get the message from errror.Error()? This is much closer to what is conventional in golang

I took your suggestion and did the same, but I decided to return errors where custom messaging was provided which makes it easier to read on the Web UI.

controllers/database.go

gmfrasca · 2023-11-28T16:14:06Z

controllers/database.go

 	}

 	decodePass, _ := b64.StdEncoding.DecodeString(params.DBConnection.Password)
 	dbConnectionTimeout := config.GetDurationConfigWithDefault(config.DBConnectionTimeoutConfigName, config.DefaultDBConnectionTimeout)

 	log.V(1).Info(fmt.Sprintf("Database Heath Check connection timeout: %s", dbConnectionTimeout))

-	dbHealthCheckPassed := ConnectAndQueryDatabase(params.DBConnection.Host,
+	dbHealthCheckPassed, message := ConnectAndQueryDatabase(params.DBConnection.Host,


alternatively/additionally to above, we could just return an error, and then assume success/failure below based on err != nil, which is a standard and common check

Since the function itself returns a bool apart from the error , I let it be and decided to assume the success/failure below based on err != nil though.

dsp-developers · 2023-11-30T10:46:37Z

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-487

dsp-developers · 2023-11-30T10:54:52Z

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-487

dsp-developers · 2023-11-30T11:02:10Z

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-487

dsp-developers · 2023-11-30T11:06:15Z

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-487

dsp-developers · 2023-11-30T11:12:10Z

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-487

dsp-developers · 2023-11-30T11:43:52Z

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-487

controllers/database.go

controllers/storage.go

hbelmiro

/lgtm

dsp-developers · 2023-11-30T14:02:29Z

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-487

gmfrasca · 2023-11-30T17:10:35Z

controllers/database.go

-		return true
+		infoMessage := "Database health check disabled, assuming database is available and ready."
+		log.V(1).Info(infoMessage)
+		return true, errors.New(infoMessage)


i'm not sure we should return the infoMessage as an error rather than just log a message and return nil. Seems a bit questionable to have a 'happy error', per se, esp since this is mostly a dev enablement feature, but interested what others think.

I agree with @gmfrasca.

I believe that was the case before I converted from string to error which makes sense to me as well.

Signed-off-by: Achyut Madhusudan <amadhusu@redhat.com>

dsp-developers · 2023-11-30T18:37:13Z

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-487

hbelmiro · 2023-12-01T19:16:02Z

/lgtm

openshift-ci · 2023-12-01T21:14:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: diegolovison, gmfrasca, hbelmiro

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

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

amadhusu added the do-not-merge/work-in-progress label Nov 22, 2023

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

openshift-ci bot requested review from DharmitD and gmfrasca November 22, 2023 20:58

amadhusu added the do-not-merge/work-in-progress label Nov 22, 2023

amadhusu changed the title ~~[Draft] Extracted DB and Object Storage Error Logs to display on DSPA~~ [WIP] Extracted DB and Object Storage Error Logs to display on DSPA Nov 22, 2023

amadhusu force-pushed the am/451 branch from 0f483ae to 2676666 Compare November 23, 2023 09:13

diegolovison reviewed Nov 23, 2023

View reviewed changes

controllers/database.go Outdated Show resolved Hide resolved

amadhusu changed the title ~~[WIP] Extracted DB and Object Storage Error Logs to display on DSPA~~ Extracted DB and Object Storage Error Logs to display on DSPA Nov 23, 2023

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

amadhusu force-pushed the am/451 branch from 2676666 to 9b5afdc Compare November 23, 2023 15:17

diegolovison approved these changes Nov 23, 2023

View reviewed changes

openshift-ci bot assigned diegolovison Nov 23, 2023

openshift-ci bot added the lgtm label Nov 23, 2023

openshift-ci bot assigned hbelmiro Nov 24, 2023

hbelmiro approved these changes Nov 24, 2023

View reviewed changes

gmfrasca reviewed Nov 28, 2023

View reviewed changes

controllers/database.go Outdated Show resolved Hide resolved

gmfrasca reviewed Nov 28, 2023

View reviewed changes

amadhusu force-pushed the am/451 branch from 9b5afdc to b8a758a Compare November 30, 2023 10:36

openshift-ci bot removed the lgtm label Nov 30, 2023

amadhusu force-pushed the am/451 branch from b8a758a to 27e2f43 Compare November 30, 2023 10:44

amadhusu force-pushed the am/451 branch from 27e2f43 to 78992dd Compare November 30, 2023 10:52

amadhusu force-pushed the am/451 branch from 78992dd to 7704475 Compare November 30, 2023 10:59

amadhusu force-pushed the am/451 branch from 7704475 to b66677a Compare November 30, 2023 11:04

amadhusu force-pushed the am/451 branch from b66677a to d85327a Compare November 30, 2023 11:09

amadhusu force-pushed the am/451 branch from d85327a to 9c28a03 Compare November 30, 2023 11:41

hbelmiro reviewed Nov 30, 2023

View reviewed changes

controllers/database.go Outdated Show resolved Hide resolved

controllers/storage.go Outdated Show resolved Hide resolved

amadhusu force-pushed the am/451 branch from 9c28a03 to d33d45c Compare November 30, 2023 14:00

hbelmiro approved these changes Nov 30, 2023

View reviewed changes

openshift-ci bot added the lgtm label Nov 30, 2023

amadhusu requested a review from gmfrasca November 30, 2023 15:53

gmfrasca reviewed Nov 30, 2023

View reviewed changes

Extracted DB and Object Storage Logs to display on DSPA

f9410d8

Signed-off-by: Achyut Madhusudan <amadhusu@redhat.com>

amadhusu force-pushed the am/451 branch from d33d45c to f9410d8 Compare November 30, 2023 18:34

openshift-ci bot removed the lgtm label Nov 30, 2023

amadhusu requested a review from gmfrasca November 30, 2023 18:36

openshift-ci bot added the lgtm label Dec 1, 2023

gmfrasca approved these changes Dec 1, 2023

View reviewed changes

openshift-ci bot assigned gmfrasca Dec 1, 2023

openshift-ci bot added the approved label Dec 1, 2023

openshift-merge-bot bot merged commit 325b3e8 into opendatahub-io:main Dec 1, 2023
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extracted DB and Object Storage Error Logs to display on DSPA #487

Extracted DB and Object Storage Error Logs to display on DSPA #487

amadhusu commented Nov 22, 2023 •

edited

Loading

dsp-developers commented Nov 22, 2023

dsp-developers commented Nov 23, 2023

dsp-developers commented Nov 23, 2023

hbelmiro commented Nov 24, 2023

gmfrasca Nov 28, 2023

amadhusu Nov 30, 2023

gmfrasca Nov 28, 2023 •

edited

Loading

amadhusu Nov 30, 2023 •

edited

Loading

dsp-developers commented Nov 30, 2023

dsp-developers commented Nov 30, 2023

dsp-developers commented Nov 30, 2023

dsp-developers commented Nov 30, 2023

dsp-developers commented Nov 30, 2023

dsp-developers commented Nov 30, 2023

hbelmiro left a comment

dsp-developers commented Nov 30, 2023

gmfrasca Nov 30, 2023

hbelmiro Nov 30, 2023

amadhusu Nov 30, 2023

dsp-developers commented Nov 30, 2023

hbelmiro commented Dec 1, 2023

openshift-ci bot commented Dec 1, 2023

Extracted DB and Object Storage Error Logs to display on DSPA #487

Extracted DB and Object Storage Error Logs to display on DSPA #487

Conversation

amadhusu commented Nov 22, 2023 • edited Loading

The issue resolved by this Pull Request:

Description of your changes:

Testing instructions

Checklist

dsp-developers commented Nov 22, 2023

dsp-developers commented Nov 23, 2023

dsp-developers commented Nov 23, 2023

hbelmiro commented Nov 24, 2023

gmfrasca Nov 28, 2023

Choose a reason for hiding this comment

amadhusu Nov 30, 2023

Choose a reason for hiding this comment

gmfrasca Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

amadhusu Nov 30, 2023 • edited Loading

Choose a reason for hiding this comment

dsp-developers commented Nov 30, 2023

dsp-developers commented Nov 30, 2023

dsp-developers commented Nov 30, 2023

dsp-developers commented Nov 30, 2023

dsp-developers commented Nov 30, 2023

dsp-developers commented Nov 30, 2023

hbelmiro left a comment

Choose a reason for hiding this comment

dsp-developers commented Nov 30, 2023

gmfrasca Nov 30, 2023

Choose a reason for hiding this comment

hbelmiro Nov 30, 2023

Choose a reason for hiding this comment

amadhusu Nov 30, 2023

Choose a reason for hiding this comment

dsp-developers commented Nov 30, 2023

hbelmiro commented Dec 1, 2023

openshift-ci bot commented Dec 1, 2023

amadhusu commented Nov 22, 2023 •

edited

Loading

gmfrasca Nov 28, 2023 •

edited

Loading

amadhusu Nov 30, 2023 •

edited

Loading