From b4e40cf466098f1f209e868307f467627d67b230 Mon Sep 17 00:00:00 2001 From: Zachary Wasserman Date: Thu, 9 Mar 2017 10:40:52 -0800 Subject: [PATCH] Warn before running migrations (#1385) - Refactor MigrationStatus() to return relevant info - Warn before running migrations Closes #1368 --- CHANGELOG.md | 4 +++ cli/prepare.go | 33 +++++++++++++++++++ cli/serve.go | 21 +++++++++--- docs/infrastructure/updating-kolide.md | 2 ++ server/datastore/datastore_migrations_test.go | 17 +++++++--- server/datastore/inmem/inmem.go | 4 +-- server/datastore/mysql/mysql.go | 30 +++++++++-------- server/kolide/datastore.go | 10 +++++- server/mock/datastore.go | 4 +-- tools/ci/deploy-k8s-testing.sh | 6 ++-- 10 files changed, 101 insertions(+), 30 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f00ecf095d..eccb8b7d04 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,10 @@ * Kolide will now warn on startup if there are database migrations not yet completed. +* Kolide will prompt for confirmation before running database migrations. + + To disable this, use `kolide prepare db --no-prompt`. + * Kolide now supports emoji, so you can 🔥 to your heart's content. * When setting the platform for a scheduled query, selecting "All" now clears individually selected platforms. diff --git a/cli/prepare.go b/cli/prepare.go index 9b11b7c54d..bbbf94b751 100644 --- a/cli/prepare.go +++ b/cli/prepare.go @@ -1,6 +1,10 @@ package cli import ( + "bufio" + "fmt" + "os" + "github.com/WatchBeam/clock" kitlog "github.com/go-kit/kit/log" "github.com/kolide/kolide/server/config" @@ -27,6 +31,8 @@ To setup kolide infrastructure, use one of the available commands. }, } + noPrompt := false + var dbCmd = &cobra.Command{ Use: "db", Short: "Given correct database configurations, prepare the databases for use", @@ -38,6 +44,29 @@ To setup kolide infrastructure, use one of the available commands. initFatal(err, "creating db connection") } + status, err := ds.MigrationStatus() + if err != nil { + initFatal(err, "retrieving migration status") + } + + switch status { + case kolide.AllMigrationsCompleted: + fmt.Println("Migrations already completed. Nothing to do.") + return + + case kolide.SomeMigrationsCompleted: + if !noPrompt { + fmt.Printf("################################################################################\n" + + "# WARNING:\n" + + "# This will perform Kolide database migrations. Please back up your data before\n" + + "# continuing.\n" + + "#\n" + + "# Press Enter to continue, or Control-c to exit.\n" + + "################################################################################\n") + bufio.NewScanner(os.Stdin).Scan() + } + } + if err := ds.MigrateTables(); err != nil { initFatal(err, "migrating db schema") } @@ -45,9 +74,13 @@ To setup kolide infrastructure, use one of the available commands. if err := ds.MigrateData(); err != nil { initFatal(err, "migrating builtin data") } + + fmt.Println("Migrations completed.") }, } + dbCmd.PersistentFlags().BoolVar(&noPrompt, "no-prompt", false, "disable prompting before migrations (for use in scripts)") + prepareCmd.AddCommand(dbCmd) var testDataCmd = &cobra.Command{ diff --git a/cli/serve.go b/cli/serve.go index 1a3900a5fc..1da3172438 100644 --- a/cli/serve.go +++ b/cli/serve.go @@ -68,7 +68,13 @@ the way that the kolide server works. initFatal(err, "initializing datastore") } - if ds.MigrationStatus() != nil { + migrationStatus, err := ds.MigrationStatus() + if err != nil { + initFatal(err, "retrieving migration status") + } + + switch migrationStatus { + case kolide.SomeMigrationsCompleted: fmt.Printf("################################################################################\n"+ "# WARNING:\n"+ "# Your Kolide database is missing required migrations. This is likely to cause\n"+ @@ -77,9 +83,16 @@ the way that the kolide server works. "# Run `%s prepare db` to perform migrations.\n"+ "################################################################################\n", os.Args[0]) - if config.Logging.Debug { - fmt.Println("error: ", err.Error()) - } + + case kolide.NoMigrationsCompleted: + fmt.Printf("################################################################################\n"+ + "# ERROR:\n"+ + "# Your Kolide database is not initialized. Kolide cannot start up.\n"+ + "#\n"+ + "# Run `%s prepare db` to initialize the database.\n"+ + "################################################################################\n", + os.Args[0]) + os.Exit(1) } if initializingDS, ok := ds.(initializer); ok { diff --git a/docs/infrastructure/updating-kolide.md b/docs/infrastructure/updating-kolide.md index f80154ddc5..ff574d0f25 100644 --- a/docs/infrastructure/updating-kolide.md +++ b/docs/infrastructure/updating-kolide.md @@ -78,6 +78,8 @@ Before running the updated server, perform necessary database migrations: kolide prepare db ``` +Note, if you would like to run this in a script, you can use the `--no-prompt` option to disable prompting before the migrations. + The updated Kolide server should now be ready to run: ``` diff --git a/server/datastore/datastore_migrations_test.go b/server/datastore/datastore_migrations_test.go index 35728f8027..1054dfc2a4 100644 --- a/server/datastore/datastore_migrations_test.go +++ b/server/datastore/datastore_migrations_test.go @@ -4,6 +4,7 @@ import ( "testing" "github.com/kolide/kolide/server/kolide" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -13,12 +14,20 @@ func testMigrationStatus(t *testing.T, ds kolide.Datastore) { } require.Nil(t, ds.Drop()) - require.NotNil(t, ds.MigrationStatus()) + + status, err := ds.MigrationStatus() + require.Nil(t, err) + assert.EqualValues(t, kolide.NoMigrationsCompleted, status) require.Nil(t, ds.MigrateTables()) - require.NotNil(t, ds.MigrationStatus()) - // Should return nil with all migrations completed + status, err = ds.MigrationStatus() + require.Nil(t, err) + assert.EqualValues(t, kolide.SomeMigrationsCompleted, status) + require.Nil(t, ds.MigrateData()) - require.Nil(t, ds.MigrationStatus()) + + status, err = ds.MigrationStatus() + require.Nil(t, err) + assert.EqualValues(t, kolide.AllMigrationsCompleted, status) } diff --git a/server/datastore/inmem/inmem.go b/server/datastore/inmem/inmem.go index 0d8592821e..82a89fff93 100644 --- a/server/datastore/inmem/inmem.go +++ b/server/datastore/inmem/inmem.go @@ -128,8 +128,8 @@ func (d *Datastore) MigrateData() error { return nil } -func (m *Datastore) MigrationStatus() error { - return nil +func (m *Datastore) MigrationStatus() (kolide.MigrationStatus, error) { + return 0, nil } func (d *Datastore) Drop() error { diff --git a/server/datastore/mysql/mysql.go b/server/datastore/mysql/mysql.go index 5009fe104c..70e9e94dbb 100644 --- a/server/datastore/mysql/mysql.go +++ b/server/datastore/mysql/mysql.go @@ -103,40 +103,42 @@ func (d *Datastore) MigrateData() error { return nil } -func (d *Datastore) MigrationStatus() error { +func (d *Datastore) MigrationStatus() (kolide.MigrationStatus, error) { if tables.MigrationClient.Migrations == nil || data.MigrationClient.Migrations == nil { - return errors.New("unexpected nil migrations list") + return 0, errors.New("unexpected nil migrations list") } lastTablesMigration, err := tables.MigrationClient.Migrations.Last() if err != nil { - return errors.New("missing tables migrations") + return 0, errors.New("missing tables migrations") } currentTablesVersion, err := tables.MigrationClient.GetDBVersion(d.db.DB) if err != nil { - return errors.New("cannot get table migration status") - } - - if currentTablesVersion != lastTablesMigration.Version { - return errors.New("table migrations must be run") + return 0, errors.New("cannot get table migration status") } lastDataMigration, err := data.MigrationClient.Migrations.Last() if err != nil { - return errors.New("missing data migrations") + return 0, errors.New("missing data migrations") } currentDataVersion, err := data.MigrationClient.GetDBVersion(d.db.DB) if err != nil { - return errors.New("cannot get table migration status") + return 0, errors.New("cannot get table migration status") } - if currentDataVersion != lastDataMigration.Version { - return errors.New("data migrations must be run") - } + switch { + case currentDataVersion == 0 && currentTablesVersion == 0: + return kolide.NoMigrationsCompleted, nil - return nil + case currentTablesVersion != lastTablesMigration.Version || + currentDataVersion != lastDataMigration.Version: + return kolide.SomeMigrationsCompleted, nil + + default: + return kolide.AllMigrationsCompleted, nil + } } // Drop removes database diff --git a/server/kolide/datastore.go b/server/kolide/datastore.go index 86544b3dad..dcb8925c06 100644 --- a/server/kolide/datastore.go +++ b/server/kolide/datastore.go @@ -26,9 +26,17 @@ type Datastore interface { MigrateData() error // MigrationStatus returns nil if migrations are complete, and an error // if migrations need to be run. - MigrationStatus() error + MigrationStatus() (MigrationStatus, error) } +type MigrationStatus int + +const ( + NoMigrationsCompleted = iota + SomeMigrationsCompleted + AllMigrationsCompleted +) + // NotFoundError is returned when the datastore resource cannot be found. type NotFoundError interface { error diff --git a/server/mock/datastore.go b/server/mock/datastore.go index cfaba21ced..976fd2cbb6 100644 --- a/server/mock/datastore.go +++ b/server/mock/datastore.go @@ -38,8 +38,8 @@ func (m *Store) MigrateTables() error { func (m *Store) MigrateData() error { return nil } -func (m *Store) MigrationStatus() error { - return nil +func (m *Store) MigrationStatus() (kolide.MigrationStatus, error) { + return 0, nil } func (m *Store) Name() string { return "mock" diff --git a/tools/ci/deploy-k8s-testing.sh b/tools/ci/deploy-k8s-testing.sh index e2e3169b2f..4fac6201cc 100755 --- a/tools/ci/deploy-k8s-testing.sh +++ b/tools/ci/deploy-k8s-testing.sh @@ -49,7 +49,7 @@ copy_db() { migrate_kolide_db() { dbname=$1 - ./build/kolide prepare db \ + ./build/kolide prepare db --no-prompt \ --mysql_address=127.0.0.1:3310 \ --mysql_database=${dbname} \ --mysql_username=${CLOUDSQL_USER} \ @@ -63,7 +63,7 @@ deploy_pr() { $exec_template -json="$jsn" -template=./tools/ci/k8s-templates/pr-deployment.template > /tmp/deployment.yml # TODO(@groob): - # we have to deploy a new copy of redis for each PR. In the future, + # we have to deploy a new copy of redis for each PR. In the future, # it would be nice to deploy a single redis instance and allow multiple DBs to connect. $exec_template -json="$jsn" -template=./tools/ci/k8s-templates/redis-pr-service.template > /tmp/redis-service.yml $exec_template -json="$jsn" -template=./tools/ci/k8s-templates/redis-pr-deployment.template > /tmp/redis-deployment.yml @@ -117,7 +117,7 @@ main() { migrate_kolide_db "${dbname}" deploy_pr fi - + docker stop $(docker ps -a -q) }