From 7eda30fbc8bfc6bfde5bfee21d52f2ab5c7d02b9 Mon Sep 17 00:00:00 2001 From: Pavlo Strokov Date: Tue, 19 Oct 2021 15:02:52 +0300 Subject: [PATCH 1/3] sql: List of migrations provided from outside To pre-populate database with some data and run migrations on top of it we should have ability to inject additional migrations. This change extends 'Migrate' function to accept list of migrations. All callers now use 'migrations.All()' list, but it will be extended for the tests in upcoming commit. --- cmd/praefect/subcmd.go | 3 ++- internal/praefect/datastore/glsql/postgres.go | 4 ++-- internal/praefect/datastore/glsql/testing.go | 5 +++-- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/cmd/praefect/subcmd.go b/cmd/praefect/subcmd.go index a85f0b7721f..83afe3d5234 100644 --- a/cmd/praefect/subcmd.go +++ b/cmd/praefect/subcmd.go @@ -16,6 +16,7 @@ import ( "gitlab.com/gitlab-org/gitaly/v14/internal/praefect/config" "gitlab.com/gitlab-org/gitaly/v14/internal/praefect/datastore" "gitlab.com/gitlab-org/gitaly/v14/internal/praefect/datastore/glsql" + "gitlab.com/gitlab-org/gitaly/v14/internal/praefect/datastore/migrations" "google.golang.org/grpc" ) @@ -125,7 +126,7 @@ func (s *sqlMigrateSubcommand) Exec(flags *flag.FlagSet, conf config.Config) err } defer clean() - n, err := glsql.Migrate(db, s.ignoreUnknown) + n, err := glsql.Migrate(db, s.ignoreUnknown, migrations.All()) if err != nil { return fmt.Errorf("%s: fail: %v", subCmd, err) } diff --git a/internal/praefect/datastore/glsql/postgres.go b/internal/praefect/datastore/glsql/postgres.go index 4b5efcd86a3..28a80306ab4 100644 --- a/internal/praefect/datastore/glsql/postgres.go +++ b/internal/praefect/datastore/glsql/postgres.go @@ -28,14 +28,14 @@ func OpenDB(conf config.DB) (*sql.DB, error) { } // Migrate will apply all pending SQL migrations. -func Migrate(db *sql.DB, ignoreUnknown bool) (int, error) { +func Migrate(db *sql.DB, ignoreUnknown bool, mgs []*migrate.Migration) (int, error) { migrationSet := migrate.MigrationSet{ IgnoreUnknown: ignoreUnknown, TableName: migrations.MigrationTableName, } migrationSource := &migrate.MemoryMigrationSource{ - Migrations: migrations.All(), + Migrations: mgs, } return migrationSet.Exec(db, "postgres", migrationSource, migrate.Up) diff --git a/internal/praefect/datastore/glsql/testing.go b/internal/praefect/datastore/glsql/testing.go index 91557698565..9dd9e417e07 100644 --- a/internal/praefect/datastore/glsql/testing.go +++ b/internal/praefect/datastore/glsql/testing.go @@ -16,6 +16,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitaly/v14/internal/praefect/config" + "gitlab.com/gitlab-org/gitaly/v14/internal/praefect/datastore/migrations" ) const ( @@ -215,7 +216,7 @@ func initPraefectTestDB(t testing.TB, database string) *sql.DB { require.NoErrorf(t, templateDB.Close(), "release connection to the %q database", templateDBConf.DBName) }() - if _, err := Migrate(templateDB, false); err != nil { + if _, err := Migrate(templateDB, false, migrations.All()); err != nil { // If database has unknown migration we try to re-create template database with // current migration. It may be caused by other code changes done in another branch. if pErr := (*migrate.PlanError)(nil); errors.As(err, &pErr) { @@ -231,7 +232,7 @@ func initPraefectTestDB(t testing.TB, database string) *sql.DB { defer func() { require.NoErrorf(t, remigrateTemplateDB.Close(), "release connection to the %q database", templateDBConf.DBName) }() - _, err = Migrate(remigrateTemplateDB, false) + _, err = Migrate(remigrateTemplateDB, false, migrations.All()) require.NoErrorf(t, err, "failed to run database migration on %q", praefectTemplateDatabase) } else { require.NoErrorf(t, err, "failed to run database migration on %q", praefectTemplateDatabase) -- GitLab From b46c90894ef98304f189f2c5c830badc3b6f710f Mon Sep 17 00:00:00 2001 From: Pavlo Strokov Date: Wed, 27 Oct 2021 15:16:54 +0300 Subject: [PATCH 2/3] glsql: Ensure link_repository_id migration runs without issues One of our migrations was failing to run on the test env because of the data. The issue was fixed under e7a8c92b8 (sql-migrate: Update storage_repositories table, 2021-10-04), but to prevent such issues we should introduce a mechanism to check our migrations. This change adds a functionality that allows to include additional migration scripts into existing production migrations set. By including the new scripts before and after migration we can assert migration results. In this change we assert execution of the 20210906145021_link_repository_id migration. Part of: https://gitlab.com/gitlab-org/gitaly/-/issues/3820 --- internal/praefect/datastore/glsql/testing.go | 83 ++++++++++++++++++++ 1 file changed, 83 insertions(+) diff --git a/internal/praefect/datastore/glsql/testing.go b/internal/praefect/datastore/glsql/testing.go index 9dd9e417e07..d5c35efe8a1 100644 --- a/internal/praefect/datastore/glsql/testing.go +++ b/internal/praefect/datastore/glsql/testing.go @@ -331,3 +331,86 @@ func getDatabaseEnvironment(t testing.TB) map[string]string { return databaseEnv } + +// testDataMigrations function should be used to include additional migration scripts into the +// existing set of migrations. All migrations applied in order. The ordering is done via numeric +// prefix of the migration. To insert migration in between some existing migrations you should +// include it with the prefix that will be greater that the lowest and lesser than the upper one. +func testDataMigrations() []*migrate.Migration { + return []*migrate.Migration{ + { + Id: "10000000000000_helpers", + Up: []string{ + // random_hex_string generates a random HEX string with requested length. + `-- +migrate StatementBegin + CREATE OR REPLACE FUNCTION random_hex_string(length INTEGER) RETURNS TEXT AS + $$ + DECLARE + chars TEXT[] := '{0,1,2,3,4,5,6,7,8,9,a,b,c,d,e,f}'; + result TEXT := ''; + i INTEGER := 0; + BEGIN + IF length < 0 THEN + RAISE EXCEPTION 'Given length cannot be less than 0'; + END IF; + FOR i IN 1..length LOOP + result := result || chars[1+RANDOM()*(ARRAY_LENGTH(chars, 1)-1)]; + END LOOP; + RETURN result; + END; + $$ LANGUAGE plpgsql`, + + // repository_relative_path generates a relative path for the repository. + // It has a standard format: @hashed/ab/cd/abcd000000000000000000000000000000000000000000000000000000000000.git + `-- +migrate StatementBegin + CREATE OR REPLACE FUNCTION repository_relative_path() RETURNS TEXT AS + $$ + DECLARE + result TEXT; + BEGIN + SELECT CONCAT_WS('/', '@hashed', SUBSTR(path, 1, 2), SUBSTR(path, 3, 2), path || '.git') INTO result + FROM (SELECT random_hex_string(64) AS path) t; + RETURN result; + END; + $$ LANGUAGE plpgsql + -- +migrate StatementEnd`, + }, + }, + // Verify 20210906145021_link_repository_id migration runs without issues. + { + // Applied before 20210906145021_link_repository_id + // Migration populates database tables with artificial data. + // It is needed to check execution of the 20210906145021_link_repository_id migration. + Id: "20210906145020_artificial_repositories", + Up: []string{ + `ALTER TABLE storage_repositories DISABLE TRIGGER notify_on_insert`, + `INSERT INTO storage_repositories (virtual_storage, relative_path, storage, generation) + SELECT virtual_storages.name, repositories.relative_path, storages.name, 1 + FROM (SELECT UNNEST(ARRAY['artificial-praefect-1','artificial-praefect-2']) AS name) virtual_storages + CROSS JOIN + (SELECT UNNEST(ARRAY['artificial-gitaly-1','artificial-gitaly-2','artificial-gitaly-3']) AS name) storages + CROSS JOIN + (SELECT generate_series(1,300), repository_relative_path() AS relative_path) repositories`, + + `INSERT INTO repositories (virtual_storage, relative_path, generation) + SELECT DISTINCT virtual_storage, relative_path, 1 + FROM storage_repositories`, + `ALTER TABLE storage_repositories ENABLE TRIGGER notify_on_insert`, + }, + }, + { + // Applied after 20210906145021_link_repository_id + // Migration removes artificial data from the repository that was used to verify migration. + Id: "20210906145022_artificial_repositories_cleanup", + Up: []string{ + `ALTER TABLE storage_repositories DISABLE TRIGGER notify_on_delete`, + `DELETE FROM storage_repositories WHERE virtual_storage IN ('artificial-praefect-1','artificial-praefect-2')`, + `ALTER TABLE storage_repositories ENABLE TRIGGER notify_on_delete`, + + `ALTER TABLE repositories DISABLE TRIGGER notify_on_delete`, + `DELETE FROM repositories WHERE virtual_storage IN ('artificial-praefect-1','artificial-praefect-2')`, + `ALTER TABLE repositories ENABLE TRIGGER notify_on_delete`, + }, + }, + } +} -- GitLab From f01771a1200afdad20bcd1c8cc173edcd4a5dada Mon Sep 17 00:00:00 2001 From: Pavlo Strokov Date: Wed, 27 Oct 2021 15:31:43 +0300 Subject: [PATCH 3/3] glsql: Sanity check additional migration were applied To make sure the additional migrations applied to the database this change adds a sanity test to verify it is listed in the database. --- .../praefect/datastore/glsql/postgres_test.go | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/internal/praefect/datastore/glsql/postgres_test.go b/internal/praefect/datastore/glsql/postgres_test.go index b91db2b58b7..26d3d6482ff 100644 --- a/internal/praefect/datastore/glsql/postgres_test.go +++ b/internal/praefect/datastore/glsql/postgres_test.go @@ -4,6 +4,7 @@ import ( "testing" "github.com/stretchr/testify/require" + "gitlab.com/gitlab-org/gitaly/v14/internal/praefect/datastore/migrations" ) func TestOpenDB(t *testing.T) { @@ -63,3 +64,54 @@ func TestScanAll(t *testing.T) { require.NoError(t, ScanAll(emptyRows, ¬hing)) require.Equal(t, ([]uint64)(nil), nothing.Values()) } + +func TestAdditionalTestMigrationsWereApplied(t *testing.T) { + t.Parallel() + + const dbName = "migrations_verification" + + postgresDBCfg := GetDBConfig(t, "postgres") + postgresDB := requireSQLOpen(t, postgresDBCfg, true) + defer func() { + require.NoErrorf(t, postgresDB.Close(), "release connection to the %q database", postgresDBCfg.DBName) + }() + + _, err := postgresDB.Exec("DROP DATABASE IF EXISTS " + dbName + "") + require.NoErrorf(t, err, `failed to drop %q database`, dbName) + _, err = postgresDB.Exec("CREATE DATABASE " + dbName + " WITH ENCODING 'UTF8'") + require.NoErrorf(t, err, `failed to create %q database`, dbName) + + migrationsDBCfg := GetDBConfig(t, dbName) + migrationsDB := requireSQLOpen(t, migrationsDBCfg, true) + defer func() { + require.NoErrorf(t, migrationsDB.Close(), "release connection to the %q database", migrationsDBCfg.DBName) + }() + + migrationsList := append(migrations.All(), testDataMigrations()...) + _, err = Migrate(migrationsDB, false, migrationsList) + require.NoError(t, err) + + var ms StringProvider + rows, err := migrationsDB.Query(`SELECT id FROM ` + migrations.MigrationTableName) + require.NoError(t, err) + require.NoError(t, ScanAll(rows, &ms)) + require.Len(t, ms.Values(), len(migrationsList), "sanity check to verify all migrations were executed") + var additionalMigrations []string + for _, applied := range ms.Values() { + var found bool + for _, mig := range migrations.All() { + if mig.Id == applied { + found = true + break + } + } + if !found { + additionalMigrations = append(additionalMigrations, applied) + } + } + require.ElementsMatch(t, additionalMigrations, []string{ + "10000000000000_helpers", + "20210906145020_artificial_repositories", + "20210906145022_artificial_repositories_cleanup", + }, "sanity check to verify all additional test migrations were applied") +} -- GitLab