-
Notifications
You must be signed in to change notification settings - Fork 416
Add Flexible Flexible ResetDB and Docker Support #631
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
80f0149 to
f09ef41
Compare
AlCutter
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just nits :)
scripts/resetdb.sh
Outdated
| # set unset envars to defaults | ||
| [ -z ${DB_USER+x} ] && DB_USER="root" | ||
| [ -z ${DB_NAME+x} ] && DB_NAME="test" | ||
| # format reused supplied envas |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
envars?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
environment variables. i'm so used to calling them envars in other places i never considered writing the whole word :)
(just fixed)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think "envars" would have been ok; Al was highlighting the typo that was here I think ("envas").
| ENTRYPOINT /go/bin/trillian_log_server \ | ||
| --mysql_uri="${DB_USER}:${DB_PASSWORD}@tcp(${DB_HOST})/${DB_DATABASE}" \ | ||
| --rpc_endpoint="$RPC_HOST:$RPC_PORT" \ | ||
| --http_endpoint="$HTTP_HOST:$HTTP_PORT" \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
funny indent here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eek, tabs! still configuring IDE on new machine. fixed.
| DUMP_METRICS=0s \ | ||
| FORCE_MASTER=true | ||
|
|
||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can remove this extra blank line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
scripts/resetdb.sh
Outdated
| then | ||
| # A command line supplied -u will override the first argument. | ||
| echo "Resetting DB..." | ||
| mysql -u $DB_USER $FLAGS -e "DROP DATABASE IF EXISTS ${DB_NAME};" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some mysql flags (e.g. --defaults-extra-file) need to come first. Putting -u $DB_USER first will prevent them being used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed order to: mysql $FLAGS -u $DB_USER -e "DROP DATABASE IF EXISTS ${DB_NAME};"
scripts/resetdb.sh
Outdated
| echo "Resetting DB..." | ||
| mysql -u $DB_USER $FLAGS -e "DROP DATABASE IF EXISTS ${DB_NAME};" | ||
| mysql -u $DB_USER $FLAGS -e "CREATE DATABASE ${DB_NAME};" | ||
| mysql -u $DB_USER $FLAGS -e "GRANT ALL ON ${DB_NAME}.* TO '${DB_NAME}'@'localhost' IDENTIFIED BY 'zaphod';" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're going to want to allow access to the Trillian database from hosts other than localhost, I expect. I think this only works at the moment because MySQL servers grant unrestricted access by default to any table named "test" or starting with "test_".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, i'm not yet sure if/how these permissions are being used. the function of this line is unchanged from what's currently in maser so i'd prefer to merge this PR as a refactor of hosts, and fix the grants in a future PR if a problem is identified.
for the purposes of this PR, the seeding works against external DBs (e.g. aurora) when used with the root user that's currently recommended.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the database isn't called "test", can you then connect to that database as a user named ${DB_NAME} from a machine other than localhost? I'm ok with fixing this as a separate PR, but adding a TODO here and/or creating an issue to track it would be helpful.
server/trillian_db_client/Dockerfile
Outdated
| FROM golang:1.8 | ||
|
|
||
| RUN apt-get update | ||
| RUN apt-get install -y mysql-client |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is important to do apt-get update and apt-get install as a single RUN command, otherwise you can get screwed over by Docker layer caching. See https://docs.docker.com/engine/userguide/eng-image/dockerfile_best-practices/#run.
scripts/resetdb.sh
Outdated
|
|
||
| if [ -z ${REPLY+x} ] || [[ $REPLY =~ ^[Yy]$ ]] | ||
| then | ||
| # A command line supplied -u will override the first argument. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is no longer true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed: comment is now removed.
|
incorporated into #667, but please comment if you'd rather merge separately. |
Problem
Trillian has a number of hardcoded assumptions that DB and Trillian are running on the same host. This is problematic running most cloud environments and in most configurations of docker.
Updates
This PR adds Docker support, cherrypicked from @gdbelvin's work in 607. I'd prefer that his PR is merged first and I rebase on his work. Even though that PR is recent (10 days), it's out of date enough that this PR was easier to cherrypick in directly.
ResetDB is generalized to run on any host
Usage