From c51d68835b375dc6de1538157e9c7f60e692f19d Mon Sep 17 00:00:00 2001 From: Eric Ju Date: Tue, 21 Jan 2025 13:14:05 -0500 Subject: [PATCH 1/2] migration: Add a migration task function to remove info folder This commit introduces a task function to record the removal of Git's info directory. The removal process requires `relativePath` of the repository. As a result, the signature of migration function has been updated to include `relativePath` as an additional argument. --- .../partition/migration/manager_test.go | 18 +++++++++++++----- .../partition/migration/migration.go | 5 ++--- .../partition/migration/migration_tasks.go | 18 ++++++++++++++++++ .../partition/migration/migration_test.go | 4 ++-- 4 files changed, 35 insertions(+), 10 deletions(-) create mode 100644 internal/gitaly/storage/storagemgr/partition/migration/migration_tasks.go diff --git a/internal/gitaly/storage/storagemgr/partition/migration/manager_test.go b/internal/gitaly/storage/storagemgr/partition/migration/manager_test.go index d983b38423..0d0f41e444 100644 --- a/internal/gitaly/storage/storagemgr/partition/migration/manager_test.go +++ b/internal/gitaly/storage/storagemgr/partition/migration/manager_test.go @@ -30,9 +30,9 @@ func TestMigrationManager_Begin(t *testing.T) { disabledFn := func(context.Context) bool { return true } migrationErr := errors.New("migration error") - errFn := func(context.Context, storage.Transaction) error { return migrationErr } - recordingFn := func(id uint64) func(_ context.Context, txn storage.Transaction) error { - return func(_ context.Context, txn storage.Transaction) error { + errFn := func(context.Context, storage.Transaction, string) error { return migrationErr } + recordingFn := func(id uint64) func(_ context.Context, txn storage.Transaction, _ string) error { + return func(_ context.Context, txn storage.Transaction, _ string) error { return txn.KV().Set(uint64ToBytes(id), nil) } } @@ -131,6 +131,14 @@ func TestMigrationManager_Begin(t *testing.T) { expectedErr: errors.New("repository has invalid migration key: 4"), expectedLastID: 4, }, + { + desc: "info directory removal", + migrations: migrations, + startingMigration: &migration{id: 0}, + expectedState: &migrationState{}, + expectedMigrationIDs: map[uint64]struct{}{1: {}}, + expectedLastID: 1, + }, } { t.Run(tc.desc, func(t *testing.T) { t.Parallel() @@ -249,7 +257,7 @@ func TestMigrationManager_Begin(t *testing.T) { func TestMigrationManager_Concurrent(t *testing.T) { t.Parallel() ctx := testhelper.Context(t) - noopFn := func(context.Context, storage.Transaction) error { return nil } + noopFn := func(context.Context, storage.Transaction, string) error { return nil } setupMockPartition := func(firstTransactionFn func(context.Context) error) *mockPartition { kvFn := func() keyvalue.ReadWriter { @@ -423,7 +431,7 @@ func TestMigrationManager_Context(t *testing.T) { }, testhelper.NewLogger(t), NewMetrics(), - []migration{{id: 1, fn: func(ctx context.Context, tx storage.Transaction) error { + []migration{{id: 1, fn: func(ctx context.Context, tx storage.Transaction, _ string) error { // Canceling the context of the request that started this migraiton // should not lead to canceling the migration. requestCancel() diff --git a/internal/gitaly/storage/storagemgr/partition/migration/migration.go b/internal/gitaly/storage/storagemgr/partition/migration/migration.go index 8de7672120..27f3c77b20 100644 --- a/internal/gitaly/storage/storagemgr/partition/migration/migration.go +++ b/internal/gitaly/storage/storagemgr/partition/migration/migration.go @@ -5,7 +5,6 @@ import ( "encoding/binary" "errors" "fmt" - "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage" ) @@ -22,7 +21,7 @@ type migration struct { // name is a human-readable description for the migration to be used in logs/tracing. name string // fn is the function executed to modify the WAL entry during transaction commit. - fn func(context.Context, storage.Transaction) error + fn func(context.Context, storage.Transaction, string) error // isDisabled defines an optional check to prevent a migration from being executed. isDisabled func(ctx context.Context) bool } @@ -33,7 +32,7 @@ func (m migration) run(ctx context.Context, txn storage.Transaction, relativePat return errInvalidMigration } - if err := m.fn(ctx, txn); err != nil { + if err := m.fn(ctx, txn, relativePath); err != nil { return fmt.Errorf("migrate repository: %w", err) } diff --git a/internal/gitaly/storage/storagemgr/partition/migration/migration_tasks.go b/internal/gitaly/storage/storagemgr/partition/migration/migration_tasks.go new file mode 100644 index 0000000000..47345fd286 --- /dev/null +++ b/internal/gitaly/storage/storagemgr/partition/migration/migration_tasks.go @@ -0,0 +1,18 @@ +package migration + +import ( + "context" + "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage" + "path/filepath" +) + +// taskRemoveInfoFolder is a migration task that record the removal of git's info directory, (notice is +// not objects/info folder). +func taskRemoveInfoFolder(ctx context.Context, txn storage.Transaction, relativePath string) error { + infoDirRelPath := filepath.Join(relativePath, "info") + return txn.FS().RecordRemoval(infoDirRelPath) +} + +func taskRemoveInfoFolderDisabled(ctx context.Context) bool { + return false +} diff --git a/internal/gitaly/storage/storagemgr/partition/migration/migration_test.go b/internal/gitaly/storage/storagemgr/partition/migration/migration_test.go index 63b4ef2ed7..551feb93a6 100644 --- a/internal/gitaly/storage/storagemgr/partition/migration/migration_test.go +++ b/internal/gitaly/storage/storagemgr/partition/migration/migration_test.go @@ -32,7 +32,7 @@ func TestMigration_Run(t *testing.T) { }, { desc: "migration returns error", - migration: migration{fn: func(context.Context, storage.Transaction) error { + migration: migration{fn: func(context.Context, storage.Transaction, string) error { return migrationErr }}, expectedErr: fmt.Errorf("migrate repository: %w", migrationErr), @@ -41,7 +41,7 @@ func TestMigration_Run(t *testing.T) { desc: "migration modifies transaction", migration: migration{ id: 1, - fn: func(_ context.Context, txn storage.Transaction) error { + fn: func(_ context.Context, txn storage.Transaction, _ string) error { return txn.KV().Set([]byte("foo"), []byte("bar")) }, }, -- GitLab From 6ade068855c284bcbc775d5c38d479cd703433f8 Mon Sep 17 00:00:00 2001 From: Eric Ju Date: Tue, 21 Jan 2025 13:14:44 -0500 Subject: [PATCH 2/2] migration: Add info folder removal task into migration factory This commit introduces the info folder removal task into the migration factory, so that the migration framework can actually run it. --- .../gitaly/storage/storagemgr/partition/migration/factory.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/internal/gitaly/storage/storagemgr/partition/migration/factory.go b/internal/gitaly/storage/storagemgr/partition/migration/factory.go index aa58a70590..d5a26d1cf0 100644 --- a/internal/gitaly/storage/storagemgr/partition/migration/factory.go +++ b/internal/gitaly/storage/storagemgr/partition/migration/factory.go @@ -8,7 +8,9 @@ import ( ) // migrations is a list of configured migrations that must be performed on repositories. -var migrations []migration +var migrations = []migration{ + {1, "info directory removal", taskRemoveInfoFolder, taskRemoveInfoFolderDisabled}, +} // migrationFactory defines a partition factory that wraps another partition factory. type migrationFactory struct { -- GitLab