Skip to content

Commit

Permalink
ignore missing migration file errors, add docs and test
Browse files Browse the repository at this point in the history
  • Loading branch information
zackattack01 committed Jun 4, 2024
1 parent ed17220 commit 722f9c9
Show file tree
Hide file tree
Showing 3 changed files with 45 additions and 1 deletion.
11 changes: 10 additions & 1 deletion ee/agent/storage/sqlite/keyvalue_store_sqlite.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"os"
"path"
"path/filepath"
"regexp"
"strings"

"github.com/golang-migrate/migrate/v4"
Expand All @@ -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 {
Expand Down Expand Up @@ -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)
}

Expand Down Expand Up @@ -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())
}
32 changes: 32 additions & 0 deletions ee/agent/storage/sqlite/keyvalue_store_sqlite_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
3 changes: 3 additions & 0 deletions ee/agent/storage/sqlite/migrations.md
Original file line number Diff line number Diff line change
Expand Up @@ -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).

0 comments on commit 722f9c9

Please sign in to comment.