Add versioning to perf SQL database

PTAL

I am adding DB versioning to the dabase. I tried a third party tool (goose), but that did not work for what we needed. So I implemented a very basic approach that stores
the version in the data base. 

There is still an command missing (i.e. 'migratedb') that will prompt for the root password and do the actual migration. 

Btw, this should be generic enough to work for other apps where we need a DB. 

BUG=skia:

Committed: https://skia.googlesource.com/buildbot/+/6cc6891a4442c5245733c0d5c7e4340ae48d9018

[stephana, 2014-09-29 15:23:30]

stephana@google.com changed reviewers:
+ jcgregorio@google.com, rmistry@google.com

[jcgregorio, 2014-09-29 16:59:08]

https://codereview.chromium.org/608273002/diff/20001/perf/db/reset_mysqldb.sh
File perf/db/reset_mysqldb.sh (right):

https://codereview.chromium.org/608273002/diff/20001/perf/db/reset_mysqldb.sh...
perf/db/reset_mysqldb.sh:23: DROP DATABASE IF EXISTS skia;
This terrifies me. Can we just have the commands to add users and create
database? I'd rather have the DROP commands appear in a document that you have
to cut and paste as a hurdle to actually deleting our entire db.

https://codereview.chromium.org/608273002/diff/20001/perf/go/db/db.go
File perf/go/db/db.go (right):

https://codereview.chromium.org/608273002/diff/20001/perf/go/db/db.go#newcode169
perf/go/db/db.go:169: stmt := `CREATE TABLE IF NOT EXISTS sk_db_version (
I'm not sure if the current readwrite user has the ability to call CREATE, if
not then we may need to either add those permissions, or maybe migrations are
only done from a separate app?

I'm leery of giving the main app CREATE/DROP privileges, little bobby tables and
all.

http://xkcd.com/327/

[stephana, 2014-09-29 18:07:50]

I am going to get rid of the downgrade steps. 
If we need to to get rid of a table in production, we can always add an upgrade
that drops a table. 
That should reduce the danger of deleting content by accident.

https://codereview.chromium.org/608273002/diff/20001/perf/db/reset_mysqldb.sh
File perf/db/reset_mysqldb.sh (right):

https://codereview.chromium.org/608273002/diff/20001/perf/db/reset_mysqldb.sh...
perf/db/reset_mysqldb.sh:23: DROP DATABASE IF EXISTS skia;
On 2014/09/29 16:59:07, jcgregorio wrote:
> This terrifies me. Can we just have the commands to add users and create
> database? I'd rather have the DROP commands appear in a document that you have
> to cut and paste as a hurdle to actually deleting our entire db.

That's why I have the note on top about this being for development and testing
only and the hostname is hardcoded into it. 
But I am fine with removing the whole file and keep the SQL statements in the
documentation.

https://codereview.chromium.org/608273002/diff/20001/perf/go/db/db.go
File perf/go/db/db.go (right):

https://codereview.chromium.org/608273002/diff/20001/perf/go/db/db.go#newcode169
perf/go/db/db.go:169: stmt := `CREATE TABLE IF NOT EXISTS sk_db_version (
On 2014/09/29 16:59:07, jcgregorio wrote:
> I'm not sure if the current readwrite user has the ability to call CREATE, if
> not then we may need to either add those permissions, or maybe migrations are
> only done from a separate app?
> 
> I'm leery of giving the main app CREATE/DROP privileges, little bobby tables
and
> all.
> 
> http://xkcd.com/327/

Agreed. The readwrite user should not have those privileges. 
I am going to add a separate app that handles migration and that will prompt the
user for the root password. It will essentially just be a front to the functions
in here.

[tfarina, 2014-09-29 18:55:27]

https://codereview.chromium.org/608273002/diff/20001/perf/db/reset_mysqldb.sh
File perf/db/reset_mysqldb.sh (right):

https://codereview.chromium.org/608273002/diff/20001/perf/db/reset_mysqldb.sh...
perf/db/reset_mysqldb.sh:22: # uncomment to drop the database
this comment says "uncomment", but the following line is already uncommented.
typo?

https://codereview.chromium.org/608273002/diff/20001/perf/db/reset_mysqldb.sh...
perf/db/reset_mysqldb.sh:26: GRANT USAGE ON *.* TO 'readonly'@'%';
what '%' means? is it a shortcut to 'localhost'?

https://codereview.chromium.org/608273002/diff/20001/perf/db/reset_mysqldb.sh...
perf/db/reset_mysqldb.sh:31: CREATE DATABASE skia ;
nit: space between skia and ;

[tfarina, 2014-09-29 18:56:34]

https://codereview.chromium.org/608273002/diff/20001/perf/go/db/db.go
File perf/go/db/db.go (right):

https://codereview.chromium.org/608273002/diff/20001/perf/go/db/db.go#newcode132
perf/go/db/db.go:132: SELECT version
does this fits in one line?

[jcgregorio, 2014-09-29 20:19:10]

https://codereview.chromium.org/608273002/diff/20001/perf/db/reset_mysqldb.sh
File perf/db/reset_mysqldb.sh (right):

https://codereview.chromium.org/608273002/diff/20001/perf/db/reset_mysqldb.sh...
perf/db/reset_mysqldb.sh:23: DROP DATABASE IF EXISTS skia;
On 2014/09/29 18:07:50, stephana wrote:
> On 2014/09/29 16:59:07, jcgregorio wrote:
> > This terrifies me. Can we just have the commands to add users and create
> > database? I'd rather have the DROP commands appear in a document that you
have
> > to cut and paste as a hurdle to actually deleting our entire db.
> 
> That's why I have the note on top about this being for development and testing
> only and the hostname is hardcoded into it. 
> But I am fine with removing the whole file and keep the SQL statements in the
> documentation.

Thanks, let's keep the SQL statements in the docs.

[stephana, 2014-09-30 11:55:12]

On 2014/09/29 20:19:10, jcgregorio wrote:
> https://codereview.chromium.org/608273002/diff/20001/perf/db/reset_mysqldb.sh
> File perf/db/reset_mysqldb.sh (right):
> 
>
https://codereview.chromium.org/608273002/diff/20001/perf/db/reset_mysqldb.sh...
> perf/db/reset_mysqldb.sh:23: DROP DATABASE IF EXISTS skia;
> On 2014/09/29 18:07:50, stephana wrote:
> > On 2014/09/29 16:59:07, jcgregorio wrote:
> > > This terrifies me. Can we just have the commands to add users and create
> > > database? I'd rather have the DROP commands appear in a document that you
> have
> > > to cut and paste as a hurdle to actually deleting our entire db.
> > 
> > That's why I have the note on top about this being for development and
testing
> > only and the hostname is hardcoded into it. 
> > But I am fine with removing the whole file and keep the SQL statements in
the
> > documentation.
> 
> Thanks, let's keep the SQL statements in the docs.

We can keep the SQL to create the database itself in the docs, but it's 
ABSOLUTELY unsustainable to have long series of SQL in documentation and 
people have to figure out which portion they need to run in relationship to 
the state of their database.

[jcgregorio, 2014-09-30 12:54:05]

On 2014/09/30 11:55:12, stephana wrote:
> On 2014/09/29 20:19:10, jcgregorio wrote:
> >
https://codereview.chromium.org/608273002/diff/20001/perf/db/reset_mysqldb.sh
> > File perf/db/reset_mysqldb.sh (right):
> > 
> >
>
https://codereview.chromium.org/608273002/diff/20001/perf/db/reset_mysqldb.sh...
> > perf/db/reset_mysqldb.sh:23: DROP DATABASE IF EXISTS skia;
> > On 2014/09/29 18:07:50, stephana wrote:
> > > On 2014/09/29 16:59:07, jcgregorio wrote:
> > > > This terrifies me. Can we just have the commands to add users and create
> > > > database? I'd rather have the DROP commands appear in a document that
you
> > have
> > > > to cut and paste as a hurdle to actually deleting our entire db.
> > > 
> > > That's why I have the note on top about this being for development and
> testing
> > > only and the hostname is hardcoded into it. 
> > > But I am fine with removing the whole file and keep the SQL statements in
> the
> > > documentation.
> > 
> > Thanks, let's keep the SQL statements in the docs.
> 
> We can keep the SQL to create the database itself in the docs, but it's 
> ABSOLUTELY unsustainable to have long series of SQL in documentation and 
> people have to figure out which portion they need to run in relationship to 
> the state of their database.

I was only talking about the DROP table/users statements, keeping the CREATE
stuff is fine.

[stephana, 2014-09-30 13:43:07]

I added the 'migrateDB' command that connects to the database as root and
prompts the user for the password if necessary.

Please look at DESIGN.md first and see if the explanation makes sense. 

It allows to create a well defined DB schema for production and local MySQL
databases as well as a local SQLite DB. 

PTAL.

https://codereview.chromium.org/608273002/diff/20001/perf/db/reset_mysqldb.sh
File perf/db/reset_mysqldb.sh (right):

https://codereview.chromium.org/608273002/diff/20001/perf/db/reset_mysqldb.sh...
perf/db/reset_mysqldb.sh:23: DROP DATABASE IF EXISTS skia;
On 2014/09/29 20:19:09, jcgregorio wrote:
> On 2014/09/29 18:07:50, stephana wrote:
> > On 2014/09/29 16:59:07, jcgregorio wrote:
> > > This terrifies me. Can we just have the commands to add users and create
> > > database? I'd rather have the DROP commands appear in a document that you
> have
> > > to cut and paste as a hurdle to actually deleting our entire db.
> > 
> > That's why I have the note on top about this being for development and
testing
> > only and the hostname is hardcoded into it. 
> > But I am fine with removing the whole file and keep the SQL statements in
the
> > documentation.
> 
> Thanks, let's keep the SQL statements in the docs.

This file has been removed.

https://codereview.chromium.org/608273002/diff/20001/perf/go/db/db.go
File perf/go/db/db.go (right):

https://codereview.chromium.org/608273002/diff/20001/perf/go/db/db.go#newcode132
perf/go/db/db.go:132: SELECT version
On 2014/09/29 18:56:33, tfarina wrote:
> does this fits in one line?

It probably could, I find it easier to read this way.

[jcgregorio, 2014-09-30 14:54:08]

https://codereview.chromium.org/608273002/diff/40001/perf/Makefile
File perf/Makefile (right):

https://codereview.chromium.org/608273002/diff/40001/perf/Makefile#newcode4
perf/Makefile:4: go install -v ./go/migratedb
Can you instead create a migratedb target and then add that target as a
dependency to the "all" target?

https://codereview.chromium.org/608273002/diff/40001/perf/go/db/db.go
File perf/go/db/db.go (right):

https://codereview.chromium.org/608273002/diff/40001/perf/go/db/db.go#newcode39
perf/go/db/db.go:39: var migrationSteps [](func() ([]string, []string, []string,
[]string))
Instead of having the var, which is initialized elsewhere and then by a function
that's called, let's just do all of that in one step, keeping it together in one
place:

var migrationSteps = []struct {
  MySQLUp    []string,
  MySQLDown  []string,
  SQLiteUp   []string,
  SQLiteDown []string,

} {
  {
     MySQLUp: `CREATE TABLE clusters (
                       id         INTEGER      NOT NULL PRIMARY KEY
AUTOINCREMENT,
                       ts         TIMESTAMP    NOT NULL,
                       hash       TEXT         NOT NULL,
                       regression FLOAT        NOT NULL,
                       cluster    MEDIUMTEXT   NOT NULL,
                       status     TEXT         NOT NULL,
                       message    TEXT         NOT NULL
                 )`,
                 `CREATE TABLE shortcuts (
                 ...
                 )`,
    MySQLDown: ``,
    ....
  },
}

https://codereview.chromium.org/608273002/diff/40001/perf/go/db/db.go#newcode46
perf/go/db/db.go:46: migrationSteps = [](func() ([]string, []string, []string,
[]string)){
Move the initialization outside of here into the var declaration above.

https://codereview.chromium.org/608273002/diff/40001/perf/go/db/db.go#newcode98
perf/go/db/db.go:98: // will result in a local SQLite database.
What if we run in production and forget to set the metadata?

https://codereview.chromium.org/608273002/diff/40001/perf/go/db/db.go#newcode117
perf/go/db/db.go:117: currentVersion, txErr := DBVersion()
errors should always be called err

https://codereview.chromium.org/608273002/diff/40001/perf/go/db/db.go#newcode188
perf/go/db/db.go:188: txn, txnErr := DB.Begin()
txnErr -> err

https://codereview.chromium.org/608273002/diff/40001/perf/go/db/db_test.go
File perf/go/db/db_test.go (right):

https://codereview.chromium.org/608273002/diff/40001/perf/go/db/db_test.go#ne...
perf/go/db/db_test.go:14: // Connection string to the local MySQL database for
testing.
So I need a local MySQL instance to run this test?

[stephana, 2014-09-30 18:11:50]

I incorporated your feedback.

https://codereview.chromium.org/608273002/diff/40001/perf/Makefile
File perf/Makefile (right):

https://codereview.chromium.org/608273002/diff/40001/perf/Makefile#newcode4
perf/Makefile:4: go install -v ./go/migratedb
On 2014/09/30 14:54:08, jcgregorio wrote:
> Can you instead create a migratedb target and then add that target as a
> dependency to the "all" target?

Done.

https://codereview.chromium.org/608273002/diff/40001/perf/go/db/db.go
File perf/go/db/db.go (right):

https://codereview.chromium.org/608273002/diff/40001/perf/go/db/db.go#newcode39
perf/go/db/db.go:39: var migrationSteps [](func() ([]string, []string, []string,
[]string))
On 2014/09/30 14:54:08, jcgregorio wrote:
> Instead of having the var, which is initialized elsewhere and then by a
function
> that's called, let's just do all of that in one step, keeping it together in
one
> place:
> 
> var migrationSteps = []struct {
>   MySQLUp    []string,
>   MySQLDown  []string,
>   SQLiteUp   []string,
>   SQLiteDown []string,
> 
> } {
>   {
>      MySQLUp: `CREATE TABLE clusters (
>                        id         INTEGER      NOT NULL PRIMARY KEY
> AUTOINCREMENT,
>                        ts         TIMESTAMP    NOT NULL,
>                        hash       TEXT         NOT NULL,
>                        regression FLOAT        NOT NULL,
>                        cluster    MEDIUMTEXT   NOT NULL,
>                        status     TEXT         NOT NULL,
>                        message    TEXT         NOT NULL
>                  )`,
>                  `CREATE TABLE shortcuts (
>                  ...
>                  )`,
>     MySQLDown: ``,
>     ....
>   },
> }

Done.

https://codereview.chromium.org/608273002/diff/40001/perf/go/db/db.go#newcode46
perf/go/db/db.go:46: migrationSteps = [](func() ([]string, []string, []string,
[]string)){
On 2014/09/30 14:54:08, jcgregorio wrote:
> Move the initialization outside of here into the var declaration above.

Done.

https://codereview.chromium.org/608273002/diff/40001/perf/go/db/db.go#newcode98
perf/go/db/db.go:98: // will result in a local SQLite database.
On 2014/09/30 14:54:08, jcgregorio wrote:
> What if we run in production and forget to set the metadata?

I should behave exactly like it does now. 
If you look in skiaperf/main.go and trybot.go, if the right parameter is not
set, this function will return "" and Init("") will create a local SQLite
database.

We could use the "local" flag in skiaperf to make it fail unless the flag is set
?

https://codereview.chromium.org/608273002/diff/40001/perf/go/db/db.go#newcode117
perf/go/db/db.go:117: currentVersion, txErr := DBVersion()
On 2014/09/30 14:54:08, jcgregorio wrote:
> errors should always be called err

Done.

https://codereview.chromium.org/608273002/diff/40001/perf/go/db/db.go#newcode188
perf/go/db/db.go:188: txn, txnErr := DB.Begin()
On 2014/09/30 14:54:08, jcgregorio wrote:
> txnErr -> err

Done.

https://codereview.chromium.org/608273002/diff/40001/perf/go/db/db_test.go
File perf/go/db/db_test.go (right):

https://codereview.chromium.org/608273002/diff/40001/perf/go/db/db_test.go#ne...
perf/go/db/db_test.go:14: // Connection string to the local MySQL database for
testing.
On 2014/09/30 14:54:08, jcgregorio wrote:
> So I need a local MySQL instance to run this test?

Yes. I have changed it so that it checks whether the env variable is set and
skips the test with a log message if not set. 
(and still test against SQLite).

[jcgregorio, 2014-09-30 18:26:01]

https://codereview.chromium.org/608273002/diff/40001/perf/go/db/db.go
File perf/go/db/db.go (right):

https://codereview.chromium.org/608273002/diff/40001/perf/go/db/db.go#newcode98
perf/go/db/db.go:98: // will result in a local SQLite database.
On 2014/09/30 18:11:49, stephana wrote:
> On 2014/09/30 14:54:08, jcgregorio wrote:
> > What if we run in production and forget to set the metadata?
> 
> I should behave exactly like it does now. 
> If you look in skiaperf/main.go and trybot.go, if the right parameter is not
> set, this function will return "" and Init("") will create a local SQLite
> database.
> 
> We could use the "local" flag in skiaperf to make it fail unless the flag is
set
> ? 

Yeah, let's do that.

[stephana, 2014-09-30 18:59:58]

Added the local flag to skiaperf and ingest. In the later case I feed it into
trybot.Init(..) because that's where the db initialization happens.

[jcgregorio, 2014-09-30 19:12:03]

https://codereview.chromium.org/608273002/diff/80001/perf/go/trybot/trybot.go
File perf/go/trybot/trybot.go (right):

https://codereview.chromium.org/608273002/diff/80001/perf/go/trybot/trybot.go...
perf/go/trybot/trybot.go:179: db.Init(db.ProdConnectionString(local))
Can we move the call to db.Init() into skiaperf/main.go?

[benchen, 2014-09-30 19:24:45]

bensong@google.com changed reviewers:
+ bensong@google.com

[stephana, 2014-09-30 19:52:09]

https://codereview.chromium.org/608273002/diff/80001/perf/go/trybot/trybot.go
File perf/go/trybot/trybot.go (right):

https://codereview.chromium.org/608273002/diff/80001/perf/go/trybot/trybot.go...
perf/go/trybot/trybot.go:179: db.Init(db.ProdConnectionString(local))
On 2014/09/30 19:12:03, jcgregorio wrote:
> Can we move the call to db.Init() into skiaperf/main.go?

Done.

[jcgregorio, 2014-09-30 21:20:52]

On 2014/09/30 19:52:09, stephana wrote:
> https://codereview.chromium.org/608273002/diff/80001/perf/go/trybot/trybot.go
> File perf/go/trybot/trybot.go (right):
> 
>
https://codereview.chromium.org/608273002/diff/80001/perf/go/trybot/trybot.go...
> perf/go/trybot/trybot.go:179: db.Init(db.ProdConnectionString(local))
> On 2014/09/30 19:12:03, jcgregorio wrote:
> > Can we move the call to db.Init() into skiaperf/main.go?
> 
> Done.

LGTM

[stephana, 2014-09-30 23:44:39]

The CQ bit was checked by stephana@google.com

[commit-bot: I haz the power, 2014-09-30 23:45:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/608273002/100001

[commit-bot: I haz the power, 2014-09-30 23:45:48]

Committed patchset #6 (id:100001) as 6cc6891a4442c5245733c0d5c7e4340ae48d9018