From 722f9c950b62b04195f95b7b8f934e7a12fbf671 Mon Sep 17 00:00:00 2001 From: zackattack01 Date: Tue, 4 Jun 2024 12:53:50 -0400 Subject: [PATCH] ignore missing migration file errors, add docs and test --- .../storage/sqlite/keyvalue_store_sqlite.go | 11 ++++++- .../sqlite/keyvalue_store_sqlite_test.go | 32 +++++++++++++++++++ ee/agent/storage/sqlite/migrations.md | 3 ++ 3 files changed, 45 insertions(+), 1 deletion(-) diff --git a/ee/agent/storage/sqlite/keyvalue_store_sqlite.go b/ee/agent/storage/sqlite/keyvalue_store_sqlite.go index c63056047..6ecb4f2e9 100644 --- a/ee/agent/storage/sqlite/keyvalue_store_sqlite.go +++ b/ee/agent/storage/sqlite/keyvalue_store_sqlite.go @@ -10,6 +10,7 @@ import ( "os" "path" "path/filepath" + "regexp" "strings" "github.com/golang-migrate/migrate/v4" @@ -24,6 +25,8 @@ const ( StartupSettingsStore storeName = iota ) +var missingMigrationErrFormat = regexp.MustCompile(`no migration found for version \d+`) + // String translates the exported int constant to the actual name of the // supported table in the sqlite database. func (s storeName) String() string { @@ -170,7 +173,9 @@ func (s *sqliteStore) migrate(ctx context.Context) error { return fmt.Errorf("creating migrate instance: %w", err) } - if err := m.Up(); err != nil && !errors.Is(err, migrate.ErrNoChange) { + // don't prevent DB access for a missing migration, this is the result of a downgrade after previously + // running a migration + if err := m.Up(); err != nil && !errors.Is(err, migrate.ErrNoChange) && !isMissingMigrationError(err) { return fmt.Errorf("running migrations: %w", err) } @@ -317,3 +322,7 @@ ON CONFLICT (name) DO UPDATE SET value=excluded.value;` return deletedKeys, nil } + +func isMissingMigrationError(err error) bool { + return missingMigrationErrFormat.MatchString(err.Error()) +} diff --git a/ee/agent/storage/sqlite/keyvalue_store_sqlite_test.go b/ee/agent/storage/sqlite/keyvalue_store_sqlite_test.go index ed75e7bb1..80f4b3a6f 100644 --- a/ee/agent/storage/sqlite/keyvalue_store_sqlite_test.go +++ b/ee/agent/storage/sqlite/keyvalue_store_sqlite_test.go @@ -234,3 +234,35 @@ func Test_Migrations(t *testing.T) { require.NoError(t, srcErr, "source error closing migration") require.NoError(t, dbErr, "database error closing migration") } + +func Test_MissingMigrations(t *testing.T) { + t.Parallel() + + tempRootDir := t.TempDir() + + conn, err := validatedDbConn(context.TODO(), tempRootDir) + require.NoError(t, err, "setting up db connection") + require.NoError(t, conn.Close(), "closing test db") + + d, err := iofs.New(migrations, "migrations") + require.NoError(t, err, "loading migration files") + + m, err := migrate.NewWithSourceInstance("iofs", d, fmt.Sprintf("sqlite://%s?query", dbLocation(tempRootDir))) + require.NoError(t, err, "creating migrate instance") + require.NoError(t, m.Up(), "expected no error running all migrations") + currentVersion, dirty, err := m.Version() + require.NoError(t, err, "error looking for current version") + require.False(t, dirty, "did not expect dirty migration state") + missingMigrationVersion := int(currentVersion) + 1 + forceVersionErr := m.Force(missingMigrationVersion) + require.NoError(t, forceVersionErr, "error forcing version") + + srcErr, dbErr := m.Close() + require.NoError(t, srcErr, "source error closing migration") + require.NoError(t, dbErr, "database error closing migration") + + // now re-open and re-attempt migrations, this will only work if we correctly ignore the missing + // migration file error + _, migrationError := OpenRW(context.TODO(), tempRootDir, StartupSettingsStore) + require.NoError(t, migrationError, "database error running missing migration") +} diff --git a/ee/agent/storage/sqlite/migrations.md b/ee/agent/storage/sqlite/migrations.md index ded94194a..462b22148 100644 --- a/ee/agent/storage/sqlite/migrations.md +++ b/ee/agent/storage/sqlite/migrations.md @@ -15,3 +15,6 @@ Add your SQL to these files. Adhere to best practices, including: 1. All migrations should be able to run multiple times without error (e.g. use `CREATE TABLE IF NOT EXISTS` instead of `CREATE TABLE`) The database will automatically apply new migrations on startup. + +Note that because we currently package our migration files with launcher, it is possible for us to run a migration and then downgrade to a version of launcher which cannot find the corresponding migration file. +To avoid this issue, we ignore missing migration file errors when migrating up on startup. This is enough for now, because all migrations can be run multiple times without error (see best practices note above). If these migrations become more complex, (e.g. table altercations that will break prior launcher versions, instead of completely new tables), we will need to explore another path forward here (likely hosting migrations with rollbacks outside of launcher, so that older versions can rollback newer migrations).