Add initial support for PostgreSQL #920

dqminh · 2016-08-16T13:05:33Z

Closes #600

As title stated :)

A quick run with docker-compose -f docker-compose.postgresql.yml up and testing with a local docker client pushing to local registry doesn't show any errors so hopefully i didn't miss anything.

cc @thaJeztah @endophage

thaJeztah · 2016-08-16T13:24:04Z

migrations/migrate.sh

 		do
 			iter=$(( iter+1 ))
 			if [[ $iter -gt 30 ]]; then
 				echo "notaryserver database failed to come up within 30 seconds"
 				exit 1;
 			fi
-			echo "waiting for notarymysql to come up."
+      echo "waiting for $DB_URL to come up."


Looks like you're using spaces here (and a couple of other places), should be tabs in shell scripts 😄

there goes my secret plan to convert tabs to spaces.

thaJeztah · 2016-08-16T13:35:04Z

Thanks! I'll leave code-review to the notary team, but I like PostgreSQL, so 😍

riyazdf · 2016-08-17T02:13:52Z

@dqminh thank you for adding this feature! Could you please rebase? I think you're hitting a linter CI issue that we addressed a few days ago.

We should have a testing infrastructure for postgres if we choose to support it - I think a good starting point would be adding a case to buildscripts/integrationtest.sh if you don't mind :)

dqminh · 2016-08-17T11:14:04Z

@riyazdf i cant get the integration test to pass in my local environment. Is there any special tricks ?

Here is part of the build failure with rethinkdb as an example:

�[31msigner_1     |�[0m successfully reached and updated RethinkDB
�[31msigner_1     |�[0m {"level":"info","msg":"Version: 0.3, Git commit: 2a49e9a","time":"2016-08-17T11:10:27Z"}
�[31msigner_1     |�[0m {"level":"debug","msg":"Trusting 2 certs","time":"2016-08-17T11:10:27Z"}
�[31msigner_1     |�[0m {"level":"debug","msg":"Default Alias: timestamp_1","time":"2016-08-17T11:10:27Z"}
�[31msigner_1     |�[0m {"level":"debug","msg":"attempting to connect user signer to host rdb-proxy.rdb","time":"2016-08-17T11:10:27Z"}
�[31msigner_1     |�[0m {"level":"debug","msg":"Trusting 1 certs","time":"2016-08-17T11:10:27Z"}
�[34mserver_1     |�[0m trying to contact RethinkDB for 30 seconds before failing
�[34mserver_1     |�[0m {"level":"info","msg":"Version: 0.3, Git commit: 2a49e9a","time":"2016-08-17T11:10:26Z"}
�[34mserver_1     |�[0m {"level":"debug","msg":"Trusting 1 certs","time":"2016-08-17T11:10:26Z"}
�[34mserver_1     |�[0m {"level":"info","msg":"Using remote signing service","time":"2016-08-17T11:10:26Z"}
�[34mserver_1     |�[0m {"level":"info","msg":"Using rethinkdb backend","time":"2016-08-17T11:10:26Z"}
�[34mserver_1     |�[0m {"level":"debug","msg":"attempting to connect admin to host rdb-proxy.rdb","time":"2016-08-17T11:10:26Z"}
�[34mserver_1     |�[0m {"level":"debug","msg":"Trusting 1 certs","time":"2016-08-17T11:10:26Z"}
�[36;1mclient_1     |�[0m make: Entering directory '/go/src/github.com/docker/notary'
�[36;1mclient_1     |�[0m pkcs11 import was not found anywhere without a build tag, yay
�[36;1mclient_1     |�[0m + /go/src/github.com/docker/notary/bin/notary
�[34mserver_1     |�[0m {"level":"debug","msg":"creating user server for db notaryserver","time":"2016-08-17T11:10:29Z"}
�[34mserver_1     |�[0m successfully reached and updated RethinkDB
�[34mserver_1     |�[0m {"level":"info","msg":"Version: 0.3, Git commit: 2a49e9a","time":"2016-08-17T11:10:29Z"}
�[34mserver_1     |�[0m {"level":"debug","msg":"Trusting 1 certs","time":"2016-08-17T11:10:29Z"}
�[34mserver_1     |�[0m {"level":"info","msg":"Using remote signing service","time":"2016-08-17T11:10:29Z"}
�[34mserver_1     |�[0m {"level":"info","msg":"Using rethinkdb backend","time":"2016-08-17T11:10:29Z"}
�[34mserver_1     |�[0m {"level":"debug","msg":"attempting to connect user server to host rdb-proxy.rdb","time":"2016-08-17T11:10:29Z"}
�[34mserver_1     |�[0m {"level":"debug","msg":"Trusting 1 certs","time":"2016-08-17T11:10:29Z"}
�[34mserver_1     |�[0m {"level":"info","msg":"Starting Server","time":"2016-08-17T11:10:29Z"}
�[34mserver_1     |�[0m {"level":"info","msg":"Enabling TLS","time":"2016-08-17T11:10:29Z"}
�[34mserver_1     |�[0m {"level":"info","msg":"Starting on :4443","time":"2016-08-17T11:10:29Z"}
�[36;1mclient_1     |�[0m + client
�[36;1mclient_1     |�[0m make: Leaving directory '/go/src/github.com/docker/notary'
�[34mserver_1     |�[0m {"go.version":"go1.6.3","http.request.host":"notary-server:4443","http.request.id":"695d2c85-9e5b-43e1-bb0b-05a6f52e3b55","http.request.method":"GET","http.request.remoteaddr":"172.28.0.8:54948","http.request.uri":"/","http.request.useragent":"curl/7.38.0","level":"info","msg":"metadata not found: You have requested metadata that does not exist.","time":"2016-08-17T11:10:39Z"}
�[36;1mclient_1     |�[0m {"errors":[{"code":"METADATA_NOT_FOUND","message":"You have requested metadata that does not exist."}]}
�[34mserver_1     |�[0m {"go.version":"go1.6.3","http.request.host":"notary-server:4443","http.request.id":"695d2c85-9e5b-43e1-bb0b-05a6f52e3b55","http.request.method":"GET","http.request.remoteaddr":"172.28.0.8:54948","http.request.uri":"/","http.request.useragent":"curl/7.38.0","http.response.contenttype":"application/json; charset=utf-8","http.response.duration":"442.864µs","http.response.status":404,"http.response.written":104,"level":"info","msg":"response completed","time":"2016-08-17T11:10:39Z"}
�[36;1mclient_1     |�[0m Traceback (most recent call last):
�[36;1mclient_1     |�[0m   File "buildscripts/testclient.py", line 371, in <module>
�[36;1mclient_1     |�[0m     run()
�[36;1mclient_1     |�[0m   File "buildscripts/testclient.py", line 361, in run
�[36;1mclient_1     |�[0m building a new client binary
�[36;1mclient_1     |�[0m ---
�[36;1mclient_1     |�[0m ---- Initializing a repo, adding a target, and pushing ----
�[36;1mclient_1     |�[0m 
�[36;1mclient_1     |�[0m $ /go/src/github.com/docker/notary/bin/notary -c cmd/notary/config.json -d /tmp/tmpnPDD4Q_main init 809bb49b202f401a90735179ad4345cf
�[36;1mclient_1     |�[0m No root keys found. Generating a new root key...
�[36;1mclient_1     |�[0m 
�[36;1mclient_1     |�[0m * fatal: client is offline
�[36;1mclient_1     |�[0m 
�[36;1mclient_1     |�[0m     Tester(repo_name, Client(server, username_passwd)).run()
�[36;1mclient_1     |�[0m   File "buildscripts/testclient.py", line 302, in run
�[36;1mclient_1     |�[0m     test_func(tempfile, tempdir)
�[36;1mclient_1     |�[0m   File "buildscripts/testclient.py", line 157, in basic_repo_test
�[36;1mclient_1     |�[0m     self.client.run(["init", self.repo_name], self.dir)
�[36;1mclient_1     |�[0m   File "buildscripts/testclient.py", line 140, in run
�[36;1mclient_1     |�[0m     raise CalledProcessError(retcode, command, output=output)
�[36;1mclient_1     |�[0m subprocess.CalledProcessError: Command '['/go/src/github.com/docker/notary/bin/notary', '-c', 'cmd/notary/config.json', '-d', '/tmp/tmpnPDD4Q_main', 'init', '809bb49b202f401a90735179ad4345cf']' returned non-zero exit status 1
�[36;1mintegrationrethink_client_1 exited with code 1
�[0mAborting on container exit...
Makefile:130: recipe for target 'integration' failed

For mysql, it's either this or it timed out when trying to reach the server.

cyli · 2016-08-17T18:00:44Z

Actually ignore my comment - totally misread, sorry. :(

cyli · 2016-08-17T18:20:15Z

Could you instead change: https://github.com/docker/notary/blob/master/buildscripts/testclient.py#L100 to say

self.client = [binary, "-D", "-c", "cmd/notary/config.json"]

This should hopefully print out debug output for the client so we can see why it can't connect with the server, although the client debug logs and the actual client output may be slightly out of sync.

dqminh · 2016-08-18T11:50:35Z

client_1     | time="2016-08-17T21:22:13Z" level=error msg="could not reach https://notary-server:4443: Get https://notary-server:4443/v2/: net/http: request canceled while waiting for connection (Client.Timeout exceeded while awaiting headers)"
client_1     | time="2016-08-17T21:22:13Z" level=info msg="continuing in offline mode"

So it looks like something is probably wrong with DNS inside my VM. Amazingly enough when i tried it with Docker for Mac it works 😵
Going to continue with the thing that works until i have more time to figure out the DNS situation :/

riyazdf · 2016-08-18T17:07:06Z

signer/keydbstore/postgresql_test.go

+)
+
+func init() {
+	// Get the MYSQL connection string from an environment variable


nit typo: "PostgreSQL"

cyli · 2016-08-19T17:28:53Z

jenkins, test this please

cyli · 2016-08-22T18:10:12Z

fixtures/server-config.postgres.json

+	},
+	"storage": {
+		"backend": "postgres",
+		"db_url": "postgres://server@serverdb:5432/notaryserver?sslmode=disable"


Non-blocking: if you know of a good/easy way to add SSL certs to the postgres server, that would be awesome, but from reading the docs it looks like we'd probably have to set the PGDATA directory to a local volume, and I'm not sure whether that's a good idea.

cyli · 2016-08-22T18:16:08Z

LGTM! Thank you adding this!

endophage · 2016-08-22T23:17:18Z

development.postgresql.yml

+      DB_URL: postgres://signer@signerdb:5432/notarysigner?sslmode=disable
+    depends_on:
+      - signerdb
+  serverdb:


Would be good for consistency to handle these the same way we do with MySQL, a single DB container with multiple databases.

i would argue that this setup is actually better in term of separation. After all the official docs says that signer and server db should be separated :p

If we can agree these compose files are for dev and testing, which they are, then having a single database server instance makes it much more convenient for us to inspect state and even docker commit a single running database container to preserve specific states we may want to share for debugging/demonstration purposes.

using single image also means that we need to bootstrap two different database with extra scripts similar to notarymysql. But the CHANGELOG also stated that:

+ Update `docker-compose` configuration to use official mariadb image + deprecate `notarymysql` + default to using a volume for `data` directory + use separate databases for `notary-server` and `notary-signer` with separate users

I can add extra bootstrap scripts, but i just think its cleaner this way. I also dont think commiting two databases are much harder than one.

But IANAM, Let me know what you think.

What we want as maintainers is a consistent way to do things, and in this instance, you have created something that differs from the most similar existing example in the repo, mariadb.

You should have no reason to create a notarypostgres image, and we would ask you to change it if you did. We deprecated notarymysql (which was at one point a custom database image) because there was no need for us to use a custom built image when we could simply use the official images and their built in bootstrapping, speeding up our build and test times. Postgres has this same built in boostrapping as mariadb, you can find the instructions under "How to extend this image", and an example of our basic setup for mariadb, which should be very similar, here.

updated. PTAL.

endophage · 2016-09-01T17:21:09Z

Thank you for updating the postgres database setup. I realize I probably seem like I'm being unreasonably pedantic, but as a maintainer, we are responsible for all the code in the repo, including that contributed by external contributors who may not be available in the future to make changes. As such, we have to make conscious efforts to reduce variation, which is unnecessary complexity, so we can mentally stay on top of all the necessary complexity.

We're taking a look at why the ineffassign checks are suddenly failing. I have some time already scheduled tomorrow afternoon to do some related style housekeeping so I'll open a PR to fix those assuming we don't find some other reason ineffassign started raising errors.

riyazdf · 2016-09-02T20:15:27Z

@dqminh thanks for your work on this!

Could you please rebase? I think that should handle the unrelated CI issues :)

This LGTM on green, though I'd like @endophage to take a last look before merge

HuKeping · 2016-09-04T07:24:42Z

cmd/notary-server/config.go

@@ -90,7 +91,7 @@ func getStore(configuration *viper.Viper, hRegister healthRegister, doBootstrap
 	switch backend {
 	case notary.MemoryBackend:
 		return storage.NewMemStorage(), nil
-	case notary.MySQLBackend, notary.SQLiteBackend:
+	case notary.MySQLBackend, notary.SQLiteBackend, notary.PostgresBackend:


Hi @dqminh would you mind also update the relative configuration docs about this backend, it's OK to do it in a separated PR, but we may forget to update it.

HuKeping · 2016-09-04T08:18:55Z

signer/keydbstore/postgresql_test.go

+	}
+
+	for i := 0; i < 30; i++ {
+		gormDB, err := gorm.Open("postgres", dburl)


HuKeping · 2016-09-04T08:23:21Z

Thanks for your contribution @dqminh ! It seems need a rebase here :)

endophage · 2016-09-20T21:00:17Z

@dqminh sorry we're taking a while to merge this. If you want to enable the new GH feature that lets maintainers modify your branch we'll happily do the last rebase and work out any failures so we can get it merged.

HuKeping · 2016-09-21T03:22:12Z

FYI, https://github.com/blog/2247-improving-collaboration-with-forks

dqminh · 2016-09-21T19:40:04Z

@endophage @HuKeping i already did that :p not sure if how its reflected on your side though.

endophage · 2016-09-21T20:24:24Z

Hmmm, yeah, not seeing anything on my end. I guess I have to just try and push and see if it works.

this adds support for specifying backend=postgres in storage option in addition to mysql and sqlite. Since notary uses gorm and doesnt use any fancy column type, this probably should work out of the box. Signed-off-by: Daniel Dao <dqminh89@gmail.com>

- this changes migrate.sh script to accept a custom migration path and db url ( defaults to mysql settings) to we can inject postgresql settings to it. - add migration files for postgresql. - add compose file for postgresql. Run it with `docker-compose -f docker-compose.postgresql.yml`. Signed-off-by: Daniel Dao <dqminh89@gmail.com>

Signed-off-by: Daniel Dao <dqminh89@gmail.com>

- add a few entrypoint scripts for postgresql - rename existing notarymysql folders Signed-off-by: Daniel Dao <dqminh89@gmail.com>

Signed-off-by: Daniel Dao <dqminh89@gmail.com>

…g directly Signed-off-by: Daniel Dao <dqminh89@gmail.com>

Signed-off-by: Daniel Dao <dqminh89@gmail.com>

Signed-off-by: Hu Keping <hukeping@huawei.com>

HuKeping

LGTM, pending CI

cyli · 2016-09-22T02:27:59Z

Thank you for following up on this @HuKeping! Thanks for your patience @dqminh!

endophage · 2016-09-22T21:06:59Z

server/storage/mysql_test.go

 		gormDB, err := gorm.Open("mysql", dburl)
 		if err == nil {
 			err := gormDB.DB().Ping()
 			if err == nil {
 				break
 			}
 		}
-		if i == 29 {
+		if i == 30 {


This was already correctly retrying 30 times. The updated version will attempt 31 times. The iteration is zero indexed so when you reach line 33, gorm.Open at line 26 will have been called i + 1 times for any given iteration. Therefore, when i == 29, gorm.Open has been attempted 30 times.

@endophage you are right about the iteration, no matter using 30 or 29, I think it is used for line 34 to log out how much time has been past.

Because of the sleep statement is at the end of this for loop, so when i == 30 and the process reaches line 34 to log out sth, we may have just sleep 30 times, thus 60 seconds.

Yeah, I'm not overly bothered which it is in this case, just wanted to be clear the commit message isn't correct and it's a somewhat superfluous commit.

I'm agree with you @endophage , the commit message"properly retry 30 times in mysql|postgresql_test" is not correct.

endophage · 2016-09-23T16:19:33Z

LGTM

endophage

And LGTM again because new GH features :-/

HuKeping · 2016-09-23T16:50:50Z

Thank you for your work on this @dqminh !

GordonTheTurtle added status/0-triage labels Aug 16, 2016

thaJeztah reviewed Aug 16, 2016
View reviewed changes

dqminh force-pushed the postgresql branch 2 times, most recently from 6236002 to 961d11f Compare August 16, 2016 15:09

dqminh force-pushed the postgresql branch from 961d11f to 2a49e9a Compare August 17, 2016 10:18

riyazdf reviewed Aug 18, 2016
View reviewed changes

dqminh force-pushed the postgresql branch 2 times, most recently from 5b77699 to c791dfd Compare August 19, 2016 16:56

cyli reviewed Aug 22, 2016
View reviewed changes

endophage reviewed Aug 22, 2016
View reviewed changes

HuKeping reviewed Sep 4, 2016
View reviewed changes

signer/keydbstore/postgresql_test.go

}

for i := 0; i < 30; i++ {

gormDB, err := gorm.Open("postgres", dburl)

Copy link

Contributor

HuKeping Sep 4, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

dqminh force-pushed the postgresql branch 2 times, most recently from af8fda8 to ef46d84 Compare September 13, 2016 13:41

HuKeping force-pushed the postgresql branch from ef46d84 to 6d6f8e0 Compare September 22, 2016 01:33

dqminh and others added 10 commits September 22, 2016 10:00

add support for postgres backend

fc7267d

this adds support for specifying backend=postgres in storage option in addition to mysql and sqlite. Since notary uses gorm and doesnt use any fancy column type, this probably should work out of the box. Signed-off-by: Daniel Dao <dqminh89@gmail.com>

add buildscripts for TESTDB=postgresql

2ca64ed

Signed-off-by: Daniel Dao <dqminh89@gmail.com>

some changes to make test pass with TESTDB=postgresql

be4699c

Signed-off-by: Daniel Dao <dqminh89@gmail.com>

add postgres tests to the fastest worker in ci

ad0c18c

Signed-off-by: Daniel Dao <dqminh89@gmail.com>

single db for postgres in dev/test mode

96a3600

- add a few entrypoint scripts for postgresql - rename existing notarymysql folders Signed-off-by: Daniel Dao <dqminh89@gmail.com>

add postgres reference to server and signer config documentation

4cd0aaa

Signed-off-by: Daniel Dao <dqminh89@gmail.com>

nit: refers to notary.PostgresBackend instead of using postgres strin…

e2945a1

…g directly Signed-off-by: Daniel Dao <dqminh89@gmail.com>

properly retry 30 times in mysql|postgresql_test

59ad773

Signed-off-by: Daniel Dao <dqminh89@gmail.com>

Add a new constant for supported backends

12ec489

Signed-off-by: Hu Keping <hukeping@huawei.com>

HuKeping force-pushed the postgresql branch from 875f661 to 12ec489 Compare September 22, 2016 02:01

HuKeping approved these changes Sep 22, 2016

View reviewed changes

endophage reviewed Sep 22, 2016

View reviewed changes

endophage approved these changes Sep 23, 2016

View reviewed changes

endophage merged commit 6350e8a into notaryproject:master Sep 23, 2016

Add initial support for PostgreSQL #920

Add initial support for PostgreSQL #920

Conversation

dqminh commented Aug 16, 2016 • edited by HuKeping Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah commented Aug 16, 2016

riyazdf commented Aug 17, 2016

dqminh commented Aug 17, 2016

cyli commented Aug 17, 2016 • edited Loading

cyli commented Aug 17, 2016

dqminh commented Aug 18, 2016

Choose a reason for hiding this comment

cyli commented Aug 19, 2016

Choose a reason for hiding this comment

cyli commented Aug 22, 2016

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

endophage Aug 31, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

endophage commented Sep 1, 2016

riyazdf commented Sep 2, 2016 • edited Loading

HuKeping Sep 4, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HuKeping commented Sep 4, 2016

endophage commented Sep 20, 2016

HuKeping commented Sep 21, 2016

dqminh commented Sep 21, 2016

endophage commented Sep 21, 2016

HuKeping left a comment

Choose a reason for hiding this comment

cyli commented Sep 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HuKeping Sep 23, 2016 • edited Loading

Choose a reason for hiding this comment

endophage commented Sep 23, 2016

endophage left a comment

Choose a reason for hiding this comment

HuKeping commented Sep 23, 2016

dqminh commented Aug 16, 2016 •

edited by HuKeping

Loading

cyli commented Aug 17, 2016 •

edited

Loading

endophage Aug 31, 2016 •

edited

Loading

riyazdf commented Sep 2, 2016 •

edited

Loading

HuKeping Sep 4, 2016 •

edited

Loading

HuKeping Sep 23, 2016 •

edited

Loading