feat(db-manager): configurable DB name with extra params #1462

Subreptivus · 2021-03-10T13:52:23Z

What this PR does / why we need it:

Demand non empty DB pass rather then hardcoded mysql DB name
No hardcoded mysql DB name
DB will be created if it not exists
Ability to provide extra params for DB connection, for example: "DB_EXTRA_PARAMS": "{\"tls\": \"true\"}" for secure connection

Special notes for your reviewer:
I may have left EnvVariables names as is, yet now it looks more cleaner, aligned with other names and less confusing

Release note:

Fully configurable external DB with ability to provide extra parameters with EnvVariable 'DB_EXTRA_PARAMS'

google-oss-robot · 2021-03-10T13:52:27Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Subreptivus
To complete the pull request process, please assign hougangliu after the PR has been reviewed.
You can assign the PR to them by writing /assign @hougangliu in a comment when ready.

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

aws-kf-ci-bot · 2021-03-10T13:52:34Z

Hi @Subreptivus. Thanks for your PR.

I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

andreyvelich · 2021-03-11T16:33:42Z

Thanks a lot for doing this @Subreptivus!
Let's include your change in the next release after the Katib 0.11 for Kubeflow 1.3 release which should happen in this week.
I am not sure that we have enough time to test it before the Katib 0.11 release.

We are still working on the manifest refactoring for the Katib: #1464.
/ok-to-test

andreyvelich · 2021-03-11T16:34:14Z

/hold for the review

aws-kf-ci-bot · 2021-03-11T17:40:52Z

@Subreptivus: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
kubeflow-katib-presubmit	`3e2d6fc`	link	`/test kubeflow-katib-presubmit`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

andreyvelich

@Subreptivus I apologies for waiting so long on this PR.
Since we finish our manifest refactoring, please can you rebase your PR ?

andreyvelich · 2021-04-22T20:58:30Z

pkg/db/v1beta1/db.go

-	}
-	return nil, errors.New("Invalid DB Name")
+func NewKatibDBInterface() (common.KatibDBInterface, error) {
+	return mysql.NewDBInterface()


In that logic how we can support other DB interfaces, like Postgres, MongoDB ?

andreyvelich · 2021-04-22T21:00:37Z

manifests/v1beta1/installs/katib-external-db/katib-db-manager-deployment.yaml

@@ -8,8 +8,6 @@ spec:
      containers:
        - name: katib-db-manager
          env:
-            - name: DB_NAME
-              value: mysql


Maybe we can introduce another env variable, like DB_TYPE: mysql, to define type of DB?

andreyvelich · 2021-04-22T21:06:03Z

cmd/db-manager/v1beta1/main.go

+	dbPasswordEnvName := common.DBPasswordEnvName
+	dbPassword := os.Getenv(dbPasswordEnvName)


Maybe we can omit dbPasswordEnvName var ?

Suggested change

dbPasswordEnvName := common.DBPasswordEnvName

dbPassword := os.Getenv(dbPasswordEnvName)

dbPassword := os.Getenv(common.DBPasswordEnvName)

andreyvelich · 2021-04-22T21:14:48Z

pkg/db/v1beta1/mysql/mysql.go

+				return "", fmt.Errorf("Timeout waiting for DB conn successfully opened")
+			}
+		}
+	} else {


Does the script reach this else since you verify an empty password here:

katib/cmd/db-manager/v1beta1/main.go

Line 78 in 3e2d6fc

if dbPassword == "" {

andreyvelich · 2021-04-22T21:20:52Z

pkg/db/v1beta1/mysql/mysql.go

+			case <-ticker.C:
+				if db, err := sql.Open(dbDriver, mysqlConfig.FormatDSN()); err == nil {
+					if _, err = db.Exec(fmt.Sprintf("CREATE DATABASE IF NOT EXISTS %s", dbName)); err == nil {
+						mysqlConfig.DBName = dbName


Do you need to set DBName here since you are doing this in the CreateMySQLConfig:

katib/pkg/db/v1beta1/mysql/mysql.go

Line 48 in 3e2d6fc

DBName: dbName,

?

andreyvelich · 2021-04-22T21:24:07Z

@gaocegege @johnugeorge Please take a look at these changes.

Subreptivus · 2021-04-23T07:36:33Z

@andreyvelich I'll answer here to all those comments above, because it falls into single answer.
First of all, I wanted to introduce as little changes as possible, that's why there so many quirks in the whole flow. So in few words, there were hardcoded values all over the places, for mysql type of the DB and for password, etc.
Yet if it has a value in it, I can refactor it to be more flexible and address all risen questions.

andreyvelich · 2021-04-23T14:55:13Z

@andreyvelich I'll answer here to all those comments above, because it falls into single answer.
First of all, I wanted to introduce as little changes as possible, that's why there so many quirks in the whole flow. So in few words, there were hardcoded values all over the places, for mysql type of the DB and for password, etc.
Yet if it has a value in it, I can refactor it to be more flexible and address all risen questions.

I think the main problem is that DB_NAME env variable points to the DB Type currently.
And KATIB_MYSQL_DB_DATABASE env points to the Database name.
You can check the env variables definition here: https://www.kubeflow.org/docs/components/katib/env-variables/.
I agree that these naming is not clear.

As I point here: #1462 (comment), we probably should rename DB_NAME to DB_TYPE.

In any case, we should design Katib DB Manager to be able to support multiply DB backend, not only MySQL.

What do you think @gaocegege @johnugeorge ?

stale · 2021-07-23T05:38:45Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale · 2021-08-21T04:01:55Z

This issue has been automatically closed because it has not had recent activity. Please comment "/reopen" to reopen it.

feat(db-manager): configurable DB name with extra params

3e2d6fc

google-oss-robot requested review from gaocegege, rui-vas and sperlingxx March 10, 2021 13:52

aws-kf-ci-bot added the needs-ok-to-test label Mar 10, 2021

google-oss-robot added the size/L label Mar 10, 2021

aws-kf-ci-bot added ok-to-test and removed needs-ok-to-test labels Mar 11, 2021

google-oss-robot added the do-not-merge/hold label Mar 11, 2021

andreyvelich reviewed Apr 22, 2021

View reviewed changes

stale bot added the lifecycle/stale label Jul 23, 2021

stale bot closed this Aug 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(db-manager): configurable DB name with extra params #1462

feat(db-manager): configurable DB name with extra params #1462

Subreptivus commented Mar 10, 2021

google-oss-robot commented Mar 10, 2021

aws-kf-ci-bot commented Mar 10, 2021

andreyvelich commented Mar 11, 2021

andreyvelich commented Mar 11, 2021

aws-kf-ci-bot commented Mar 11, 2021

andreyvelich left a comment

andreyvelich Apr 22, 2021

andreyvelich Apr 22, 2021

andreyvelich Apr 22, 2021

andreyvelich Apr 22, 2021 •

edited

Loading

andreyvelich Apr 22, 2021

andreyvelich commented Apr 22, 2021

Subreptivus commented Apr 23, 2021

andreyvelich commented Apr 23, 2021 •

edited

Loading

stale bot commented Jul 23, 2021

stale bot commented Aug 21, 2021

		dbPasswordEnvName := common.DBPasswordEnvName
		dbPassword := os.Getenv(dbPasswordEnvName)

	dbPasswordEnvName := common.DBPasswordEnvName
	dbPassword := os.Getenv(dbPasswordEnvName)
	dbPassword := os.Getenv(common.DBPasswordEnvName)

feat(db-manager): configurable DB name with extra params #1462

feat(db-manager): configurable DB name with extra params #1462

Conversation

Subreptivus commented Mar 10, 2021

google-oss-robot commented Mar 10, 2021

aws-kf-ci-bot commented Mar 10, 2021

andreyvelich commented Mar 11, 2021

andreyvelich commented Mar 11, 2021

aws-kf-ci-bot commented Mar 11, 2021

andreyvelich left a comment

Choose a reason for hiding this comment

andreyvelich Apr 22, 2021

Choose a reason for hiding this comment

andreyvelich Apr 22, 2021

Choose a reason for hiding this comment

andreyvelich Apr 22, 2021

Choose a reason for hiding this comment

andreyvelich Apr 22, 2021 • edited Loading

Choose a reason for hiding this comment

andreyvelich Apr 22, 2021

Choose a reason for hiding this comment

andreyvelich commented Apr 22, 2021

Subreptivus commented Apr 23, 2021

andreyvelich commented Apr 23, 2021 • edited Loading

stale bot commented Jul 23, 2021

stale bot commented Aug 21, 2021

andreyvelich Apr 22, 2021 •

edited

Loading

andreyvelich commented Apr 23, 2021 •

edited

Loading