feat(backend): add postgres initialization #9798

Linchin · 2023-07-28T22:24:03Z

Description of your changes:
Part 1 of supporting PostgreSQL:

Adds driver for PostgreSQL
Supports initializing PostgreSQL
Adds integration tests for MySQL or PostgreSQL initialization
Moves client_manager.go to its own package and folder for ease of testing
Allows users to select which type of database they want to run integration test against
Fixes a typo in the PostgreSQL deployment guide
Makes PostgreSQL no longer load balancer

Checklist:

The title for your pull request (PR) should follow our title convention. Learn more about the pull request title convention used in this repository.

zijianjoy · 2023-08-03T19:47:10Z

backend/Dockerfile.persistenceagent

    go-licenses save ./backend/src/agent/persistence --save_path /tmp/NOTICES

+# RUN go-licenses csv ./backend/src/agent/persistence > /tmp/licenses.csv && \
+#    diff /tmp/licenses.csv backend/third_party_licenses/persistence_agent.csv && \
+#    go-licenses save ./backend/src/agent/persistence --save_path /tmp/NOTICES


Why do we save license file to /tmp/NOTICES twice? What is the reason for this change?

Thank you for pointing this out. I was temporarily disabling them, because the generated license files apparently changed although I didn't change this part of code. I intend to double check if we need to update all the license files.

We should have this sorted out before we can merge the PR.

backend/src/apiserver/client/sql.go

zijianjoy · 2023-08-03T19:50:51Z

backend/src/apiserver/client/sql.go

@@ -44,3 +45,26 @@ func CreateMySQLConfig(user, password string, mysqlServiceHost string,
 		AllowNativePasswords: true,
 	}
 }
+
+func CreatePostgreSQLConfig(user, password, postgresHost, dbName string, postgresPort uint16,


Do you plan to introduce sslmode for security support in the coming PRs? It is important to make sure we are using secure channel when reading/writing to DB.

Reference: https://github.com/google/ml-metadata/blob/master/ml_metadata/proto/metadata_store.proto#L714-L743

I think it would be great to improve security, but I wonder if it is necessary, since the DB and the backend are hosted in the same cluster.

This allows scenario like connecting to a managed database service. We can discuss about this in future implementation.

zijianjoy · 2023-08-03T19:52:01Z

backend/src/apiserver/client/sql.go

+	if password != "" {
+		fmt.Fprintf(&b, "password=%s ", password)
+	}


Can we provide an option for passfile as well? Reference: https://github.com/google/ml-metadata/blob/master/ml_metadata/proto/metadata_store.proto#L704

passfile sounds like a great way to enhance security. Could you tell me more about it? My guess is this will allow users to pass a file containing the password as part of the deployment config. Maybe this can be added together with other configs users can set.

That is right! So the passfile identify the location path to a password, so you don't need to expose the secret via deployment. https://www.postgresql.org/docs/current/libpq-pgpass.html When passing this parameter to postgresql, the password field can be omitted.

We can add this configuration in the coming PR, if this makes sense to you.

zijianjoy · 2023-08-03T19:52:31Z

backend/src/apiserver/client/sql.go

+	if postgresHost != "" {
+		fmt.Fprintf(&b, "host=%s ", postgresHost)
+	}


Can we provide an option for hostaddr? https://github.com/google/ml-metadata/blob/master/ml_metadata/proto/metadata_store.proto#L690

I wonder if this is necessary, since the host address here is simply for communications inside the cluster. Could you tell me more why this is beneficial? Thank you!

Sure thing, hostaddr is helpful when we are connecting to external database, for example: CloudSQL. Yes we can use host which is handy for Unix-domain communication, but hostaddr allows postgresql client to make TCP/IP communication instead.

Again, we can implement this feature in future PR.

zijianjoy · 2023-08-03T20:25:43Z

backend/src/apiserver/client_manager/client_manager.go

+func InitDBClient(initConnectionTimeout time.Duration) *storage.DB {
+	// Allowed driverName values:
+	// 1) To use MySQL, use `mysql`
+	// 2) To use PostgreSQL, use `pgx`


Why do we choose pgx for postgresql in DBDriverName flag? Is there an alternative which is easier for people to understand?

+1, pgx feels weird, albeit I saw that's the name of the go package you're using. I think postgres would be still better here.

I used this because we are using the package pgx as the PostgreSQL driver, and it is needed in the sql.Open() function to create the correct DB instance. See the example here. I agree it may be quite confusing to people, but I was weighting this against being consistent with the driver name we are using, so I eventually opted for making it clear in the comments to reduce the confusion. WDYT?

Okay I think @Linchin 's point is reasonable, we just need to make sure we document this parameter is being used as a sql driver flag: https://github.com/golang/go/wiki/SQLDrivers.

zijianjoy · 2023-08-03T20:28:01Z

backend/src/apiserver/client_manager/client_manager.go

+	case "mysql":
+		textFormat = "longtext not null"
+	case "pgx":
+		textFormat = "text"


We might need to define some const for value like text and longtext not null.

Could you help me understand why it is better to use const values here? Thank you.

DB dialects should be stored in a centralized place, so it helps reviewing, modifying and extending. For example: If we integrate with a new DB type in the future, I hope that I can view the existing syntax differences easily in one place, instead of trying to catch those adhoc variables during implementation.

Therefore, my recommendation is to define a const for MYSQL_TEXT_FORMAT and PGX_TEXT_FORMAT in a centralized file: https://github.com/kubeflow/pipelines/blob/master/backend/src/apiserver/storage/db.go

Thank you so much for the thourought explanation! I added the constants in backend/src/apiserver/client/sql.go instead, because this is a SQL related dependency that client_manager is already using.

backend/src/apiserver/client_manager/client_manager.go

zijianjoy · 2023-08-03T20:31:45Z

backend/src/apiserver/client_manager/client_manager.go

+			common.GetStringConfigWithDefault(postgresUser, ""),
+			common.GetStringConfigWithDefault(postgresPassword, ""),


Do we want to set a default user like mysql does?

Yes, thanks for finding this! I should add a default user here.

zijianjoy · 2023-08-03T20:34:21Z

backend/src/apiserver/client_manager/client_manager.go

-		AddForeignKey("RunUUID", "run_details(UUID)", "CASCADE" /* onDelete */, "CASCADE" /* update */)
-	if response.Error != nil {
-		glog.Fatalf("Failed to create a foreign key for RunUUID in task table. Error: %s", response.Error)
+	switch driverName {


I think extracting the different behavior of each driver to their own module is helpful. But feel free to defer it to future PRs.

I agree. This part was more of a patch work to make it work for now.

chensun · 2023-08-03T21:47:22Z

backend/Dockerfile.persistenceagent

    go-licenses save ./backend/src/agent/persistence --save_path /tmp/NOTICES

+# RUN go-licenses csv ./backend/src/agent/persistence > /tmp/licenses.csv && \
+#    diff /tmp/licenses.csv backend/third_party_licenses/persistence_agent.csv && \
+#    go-licenses save ./backend/src/agent/persistence --save_path /tmp/NOTICES


We should have this sorted out before we can merge the PR.

backend/src/apiserver/client_manager/client_manager.go

chensun · 2023-08-03T23:32:12Z

backend/src/apiserver/client_manager/client_manager.go

+	postgresPort     = "DBConfig.PostgreSQLConfig.Port"
+	postgresUser     = "DBConfig.PostgreSQLConfig.User"
+	postgresPassword = "DBConfig.PostgreSQLConfig.Password"
+	postgresDBName   = "DBConfig.PostgreSQLConfig.DBName"


The newly added Postgres configs seems so generic and completely covered by existing configs for mysql already. Do we need to separate them?

I agree that they are pretty much the same fields here, but I think it is possible that there can be different values for the two types of DB, as well as different configs available, so it's safer to have one set of values for each of them. For example, the default host name and port number for MySQL and PostgreSQL are different.

chensun · 2023-08-03T23:32:24Z

backend/src/apiserver/client_manager/client_manager.go

@@ -12,15 +12,17 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.

-package main
+package clientmanager


What's the reason for moving this file?

Good point! I moved it because I added integration test for functions here, but it's impossible to test stuff in "main" from a different package in Golang. Reference: https://stackoverflow.com/questions/53997114/how-to-test-the-main-package-in-golang-from-a-test-package

chensun · 2023-08-03T23:34:12Z

backend/src/apiserver/client_manager/client_manager.go

+func InitDBClient(initConnectionTimeout time.Duration) *storage.DB {
+	// Allowed driverName values:
+	// 1) To use MySQL, use `mysql`
+	// 2) To use PostgreSQL, use `pgx`


+1, pgx feels weird, albeit I saw that's the name of the go package you're using. I think postgres would be still better here.

chensun · 2023-08-03T23:37:29Z

backend/src/apiserver/client_manager/client_manager.go

+// Returns the same error, if it's not "already exists" related.
+// Otherwise, return nil.
+func ignoreAlreadyExistError(err error) error {
+	if err != nil && strings.Contains(err.Error(), "exists") {


This method seems very fragile. I imagine there's could be multiple scenarios matching keyword "exists".
By the way, why do we need to ignore this now? Was it an issue for MySQL in the past or does only applies to Postgres?

I added this because MySQL simply ignores if the foreign key already exist, but PostgreSQL reports an error. A more robust way I can think of is to use an if/else branch for the two SQLs and handle them separately.

backend/src/apiserver/client_manager/client_manager.go

chensun · 2023-08-03T23:50:10Z

backend/test/integration/README.md

+
+### Run database tests with PostgreSQL
+
+To run this test, you need to first deploy the PostgreSQL images on your Kubernetes cluster. For how to deploy, 


Is it in your next step to automate the test? We can rely on manual test.

Ideally we should, and also include PostgreSQL set of tests in the test infra.

zijianjoy · 2023-08-08T21:26:55Z

backend/src/apiserver/client_manager/client_manager.go

+	if driverName == "pgx" && err != nil && strings.Contains(err.Error(), "already exists") {
+		return nil
+	}
+	if driverName == "mysql" && err != nil && strings.Contains(err.Error(), "database exists") {


We can define consts for each error message keyword already exists and database exists in a centralized place: https://github.com/kubeflow/pipelines/blob/master/backend/src/apiserver/storage/db.go

zijianjoy · 2023-08-08T21:28:39Z

manifests/kustomize/third-party/postgresql/base/kustomization.yaml

@@ -2,7 +2,8 @@ apiVersion: kustomize.config.k8s.io/v1beta1
 kind: Kustomization
 resources:
 - pg-deployment.yaml
+- pg-pv.yaml


I shouldn't need a PV resource, it is automatically created by PVC when it is bound to a deployment resource.

I'm not sure about this. I added the PV file because the PVC had trouble binding due to lack of PV. Maybe I was doing something wrong during my deployment. Could you give me more reference on this? Thank you.

You can look at the Events for the PVC to learn more about why is PV not created. Maybe the disk driver has something wrong when provisioning a PV. But we shouldn't create PV resource manually in codebase. Refer to MySQL manifest folder: https://github.com/kubeflow/pipelines/tree/master/manifests/kustomize/third-party/mysql/base

You are right about this. I tried a fresh deployment without the PV, and it passed the tests. The previous error was likely caused by something else. I removed the pv declaration.

Linchin · 2023-08-09T18:28:07Z

/test kubeflow-pipeline-e2e-test
/test kubeflow-pipeline-upgrade-test

Linchin · 2023-08-09T21:27:16Z

part of #9813

Linchin · 2023-08-09T23:49:39Z

/test kubeflow-pipeline-e2e-test
/test kubeflow-pipeline-upgrade-test

backend/src/apiserver/client_manager/client_manager.go

backend/test/integration/run_tests_locally.sh

zijianjoy

/lgtm

Great work on this PR, Lingqing!
Waiting for Chen for a final approval.

chensun

/lgtm

Thanks!

chensun

/approve

Thanks!

google-oss-prow · 2023-08-10T21:50:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chensun

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

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

iciclespider · 2024-01-15T18:41:29Z

I get the following error when the db driver name is configured to "gpx":

client_manager.go:428] sql: unknown driver "pgx" (forgotten import?)

According to this, it appears to be a missing import which will register the needed driver: https://stackoverflow.com/a/52791919

* add postgres initialization * remove load balancer * go mod tidy * update license * license update for viewer * (test) disable controller license check * (test) disable persistence agence licence check * (test) disable scheduled workflow license check * (test) disable cacheserver license check * fix db config location * fix mysql support * test * test * no long set host address * address comments * address comments and enable license check * format * remove extra blank line * update licenses * cache server license * address comments * centralize error message * remove pv in postgres deployment

Linchin added 2 commits July 28, 2023 22:12

add postgres initialization

14773d0

remove load balancer

e2c0933

Linchin added the area/backend label Jul 28, 2023

Linchin assigned chensun and zijianjoy Jul 28, 2023

Linchin requested review from chensun and zijianjoy July 28, 2023 22:24

google-oss-prow bot added the size/L label Jul 28, 2023

google-oss-prow bot requested a review from gkcalat July 28, 2023 22:24

Linchin added 9 commits July 31, 2023 23:06

go mod tidy

e81fea2

update license

bd6c7f5

license update for viewer

408e3c2

(test) disable controller license check

6c7afd2

(test) disable persistence agence licence check

52d6dcf

(test) disable scheduled workflow license check

a73d6dd

(test) disable cacheserver license check

70ccd14

fix db config location

7356e14

fix mysql support

5b8b24a

google-oss-prow bot added size/XL and removed size/L labels Aug 2, 2023

Linchin added 3 commits August 2, 2023 23:08

test

5787bfb

test

8c4530d

no long set host address

bc53cb1

zijianjoy reviewed Aug 3, 2023

View reviewed changes

chensun reviewed Aug 3, 2023

View reviewed changes

Linchin added 2 commits August 8, 2023 19:33

Merge remote-tracking branch 'origin/master' into db-init

85223dd

address comments

84b9157

zijianjoy reviewed Aug 8, 2023

View reviewed changes

Linchin added 2 commits August 8, 2023 23:21

address comments and enable license check

df43a9b

format

fbf3d11

Linchin added 2 commits August 9, 2023 00:01

remove extra blank line

3764044

update licenses

4fa2fe1

cache server license

1566b21

zijianjoy reviewed Aug 10, 2023

View reviewed changes

backend/src/apiserver/client_manager/client_manager.go Outdated Show resolved Hide resolved

backend/test/integration/run_tests_locally.sh Outdated Show resolved Hide resolved

Linchin added 3 commits August 10, 2023 19:07

address comments

b5127b1

centralize error message

bb3d1cb

remove pv in postgres deployment

af3b299

zijianjoy reviewed Aug 10, 2023

View reviewed changes

google-oss-prow bot added the lgtm label Aug 10, 2023

chensun reviewed Aug 10, 2023

View reviewed changes

chensun approved these changes Aug 10, 2023

View reviewed changes

google-oss-prow bot added the approved label Aug 10, 2023

google-oss-prow bot merged commit e1f0c01 into kubeflow:master Aug 10, 2023

Tomcli mentioned this pull request Apr 3, 2024

[feature] Implement an agnostic way to initialize the database #7923

Closed

		common.GetStringConfigWithDefault(postgresUser, ""),
		common.GetStringConfigWithDefault(postgresPassword, ""),


		### Run database tests with PostgreSQL

		To run this test, you need to first deploy the PostgreSQL images on your Kubernetes cluster. For how to deploy,

feat(backend): add postgres initialization #9798

feat(backend): add postgres initialization #9798

Conversation

Linchin commented Jul 28, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zijianjoy Aug 8, 2023 • 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

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

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

Linchin Aug 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Linchin commented Aug 9, 2023

Linchin commented Aug 9, 2023

Linchin commented Aug 9, 2023

zijianjoy left a comment

Choose a reason for hiding this comment

chensun left a comment

Choose a reason for hiding this comment

chensun left a comment

Choose a reason for hiding this comment

google-oss-prow bot commented Aug 10, 2023

iciclespider commented Jan 15, 2024

Linchin commented Jul 28, 2023 •

edited

Loading

zijianjoy Aug 8, 2023 •

edited

Loading

Linchin Aug 8, 2023 •

edited

Loading