diff options
author | Sami Hiltunen <shiltunen@gitlab.com> | 2023-03-29 14:08:18 +0300 |
---|---|---|
committer | Sami Hiltunen <shiltunen@gitlab.com> | 2023-03-29 14:08:18 +0300 |
commit | c4cf1e4f9f6859ee870ab76cb69140e162257832 (patch) | |
tree | 5dc2746ec936547f8812723ba5609ff8f86cf37c | |
parent | 82da6c9eb6897bf3bbb7a791ba13a184f5679d24 (diff) | |
parent | f07547da9702663d5331e306ad1d1fe7c5c107b9 (diff) |
Merge branch 'jt-remove-hooks-transaction-ff' into 'master'
featureflag: Remove `tx_restore_custom_hooks`
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5562
Merged-by: Sami Hiltunen <shiltunen@gitlab.com>
Approved-by: Sami Hiltunen <shiltunen@gitlab.com>
Co-authored-by: Justin Tobler <jtobler@gitlab.com>
-rw-r--r-- | internal/backup/backup_test.go | 7 | ||||
-rw-r--r-- | internal/gitaly/service/repository/replicate.go | 2 | ||||
-rw-r--r-- | internal/gitaly/service/repository/set_custom_hooks.go | 33 | ||||
-rw-r--r-- | internal/gitaly/service/repository/set_custom_hooks_test.go | 32 | ||||
-rw-r--r-- | internal/metadata/featureflag/ff_transactional_restore_custom_hooks.go | 10 | ||||
-rw-r--r-- | internal/praefect/coordinator.go | 13 | ||||
-rw-r--r-- | internal/praefect/coordinator_test.go | 4 |
7 files changed, 18 insertions, 83 deletions
diff --git a/internal/backup/backup_test.go b/internal/backup/backup_test.go index 63342f5c2..6f6ddeeb0 100644 --- a/internal/backup/backup_test.go +++ b/internal/backup/backup_test.go @@ -19,7 +19,6 @@ import ( "gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/service/setup" "gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/storage" "gitlab.com/gitlab-org/gitaly/v15/internal/helper/perm" - "gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag" "gitlab.com/gitlab-org/gitaly/v15/internal/testhelper" "gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testcfg" "gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testserver" @@ -298,11 +297,7 @@ func TestManager_Create_incremental(t *testing.T) { func TestManager_Restore(t *testing.T) { t.Parallel() - testhelper.NewFeatureSets(featureflag.TransactionalRestoreCustomHooks). - Run(t, testManagerRestore) -} - -func testManagerRestore(t *testing.T, ctx context.Context) { + ctx := testhelper.Context(t) cfg := testcfg.Build(t) testcfg.BuildGitalyHooks(t, cfg) diff --git a/internal/gitaly/service/repository/replicate.go b/internal/gitaly/service/repository/replicate.go index 2362dd2ce..cc5675fd7 100644 --- a/internal/gitaly/service/repository/replicate.go +++ b/internal/gitaly/service/repository/replicate.go @@ -283,7 +283,7 @@ func (s *server) syncCustomHooks(ctx context.Context, in *gitalypb.ReplicateRepo return request.GetData(), err }) - if err := s.setCustomHooksTransaction(ctx, reader, in.GetRepository()); err != nil { + if err := s.setCustomHooks(ctx, reader, in.GetRepository()); err != nil { return fmt.Errorf("setting custom hooks: %w", err) } diff --git a/internal/gitaly/service/repository/set_custom_hooks.go b/internal/gitaly/service/repository/set_custom_hooks.go index 0898eee43..3616b8136 100644 --- a/internal/gitaly/service/repository/set_custom_hooks.go +++ b/internal/gitaly/service/repository/set_custom_hooks.go @@ -16,7 +16,6 @@ import ( "gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/service" "gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/transaction" "gitlab.com/gitlab-org/gitaly/v15/internal/helper/perm" - "gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag" "gitlab.com/gitlab-org/gitaly/v15/internal/safe" "gitlab.com/gitlab-org/gitaly/v15/internal/structerr" "gitlab.com/gitlab-org/gitaly/v15/internal/tempdir" @@ -54,7 +53,7 @@ func (s *server) SetCustomHooks(stream gitalypb.RepositoryService_SetCustomHooks }) if err := s.setCustomHooks(ctx, reader, repo); err != nil { - return structerr.NewInternal("%w", err) + return structerr.NewInternal("setting custom hooks: %w", err) } return stream.SendAndClose(&gitalypb.SetCustomHooksResponse{}) @@ -88,36 +87,16 @@ func (s *server) RestoreCustomHooks(stream gitalypb.RepositoryService_RestoreCus }) if err := s.setCustomHooks(ctx, reader, repo); err != nil { - return structerr.NewInternal("%w", err) + return structerr.NewInternal("setting custom hooks: %w", err) } return stream.SendAndClose(&gitalypb.RestoreCustomHooksResponse{}) } +// setCustomHooks transactionally and atomically sets custom hooks for a +// repository. The provided reader should be a tarball containing the custom +// hooks to be extracted to the specified Git repository. func (s *server) setCustomHooks(ctx context.Context, reader io.Reader, repo repository.GitRepo) error { - if featureflag.TransactionalRestoreCustomHooks.IsEnabled(ctx) { - if err := s.setCustomHooksTransaction(ctx, reader, repo); err != nil { - return fmt.Errorf("setting custom hooks: %w", err) - } - - return nil - } - - repoPath, err := s.locator.GetRepoPath(repo) - if err != nil { - return fmt.Errorf("getting repo path: %w", err) - } - - if err := hook.ExtractHooks(ctx, reader, repoPath, false); err != nil { - return fmt.Errorf("extracting hooks: %w", err) - } - - return nil -} - -// setCustomHooksTransaction transactionally and atomically sets the provided -// custom hooks for the specified repository. -func (s *server) setCustomHooksTransaction(ctx context.Context, tar io.Reader, repo repository.GitRepo) error { repoPath, err := s.locator.GetRepoPath(repo) if err != nil { return fmt.Errorf("getting repo path: %w", err) @@ -157,7 +136,7 @@ func (s *server) setCustomHooksTransaction(ctx context.Context, tar io.Reader, r } }() - if err := hook.ExtractHooks(ctx, tar, tmpDir.Path(), false); err != nil { + if err := hook.ExtractHooks(ctx, reader, tmpDir.Path(), false); err != nil { return fmt.Errorf("extracting hooks: %w", err) } diff --git a/internal/gitaly/service/repository/set_custom_hooks_test.go b/internal/gitaly/service/repository/set_custom_hooks_test.go index c39d587ab..4e1b47cd3 100644 --- a/internal/gitaly/service/repository/set_custom_hooks_test.go +++ b/internal/gitaly/service/repository/set_custom_hooks_test.go @@ -18,7 +18,6 @@ import ( "gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/transaction" "gitlab.com/gitlab-org/gitaly/v15/internal/helper/perm" "gitlab.com/gitlab-org/gitaly/v15/internal/metadata" - "gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag" "gitlab.com/gitlab-org/gitaly/v15/internal/testhelper" "gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testcfg" "gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testserver" @@ -32,12 +31,8 @@ import ( func TestSetCustomHooksRequest_success(t *testing.T) { t.Parallel() - testhelper.NewFeatureSets(featureflag.TransactionalRestoreCustomHooks). - Run(t, testSuccessfulSetCustomHooksRequest) -} -func testSuccessfulSetCustomHooksRequest(t *testing.T, ctx context.Context) { - t.Parallel() + ctx := testhelper.Context(t) for _, tc := range []struct { desc string @@ -136,27 +131,18 @@ func testSuccessfulSetCustomHooksRequest(t *testing.T, ctx context.Context) { require.NoError(t, err) require.FileExists(t, filepath.Join(repoPath, "custom_hooks", "pre-push.sample")) - - if featureflag.TransactionalRestoreCustomHooks.IsEnabled(ctx) { - require.Equal(t, 2, len(txManager.Votes())) - assert.Equal(t, voting.Prepared, txManager.Votes()[0].Phase) - assert.Equal(t, expectedVote, txManager.Votes()[1].Vote) - assert.Equal(t, voting.Committed, txManager.Votes()[1].Phase) - } else { - require.Equal(t, 0, len(txManager.Votes())) - } + require.Equal(t, 2, len(txManager.Votes())) + assert.Equal(t, voting.Prepared, txManager.Votes()[0].Phase) + assert.Equal(t, expectedVote, txManager.Votes()[1].Vote) + assert.Equal(t, voting.Committed, txManager.Votes()[1].Phase) }) } } func TestSetCustomHooks_failedValidation(t *testing.T) { t.Parallel() - testhelper.NewFeatureSets(featureflag.TransactionalRestoreCustomHooks). - Run(t, testFailedSetCustomHooksDueToValidations) -} -func testFailedSetCustomHooksDueToValidations(t *testing.T, ctx context.Context) { - t.Parallel() + ctx := testhelper.Context(t) for _, tc := range []struct { desc string @@ -202,12 +188,8 @@ func testFailedSetCustomHooksDueToValidations(t *testing.T, ctx context.Context) func TestSetCustomHooks_corruptTar(t *testing.T) { t.Parallel() - testhelper.NewFeatureSets(featureflag.TransactionalRestoreCustomHooks). - Run(t, testFailedSetCustomHooksDueToBadTar) -} -func testFailedSetCustomHooksDueToBadTar(t *testing.T, ctx context.Context) { - t.Parallel() + ctx := testhelper.Context(t) for _, tc := range []struct { desc string diff --git a/internal/metadata/featureflag/ff_transactional_restore_custom_hooks.go b/internal/metadata/featureflag/ff_transactional_restore_custom_hooks.go deleted file mode 100644 index 6e92a1b77..000000000 --- a/internal/metadata/featureflag/ff_transactional_restore_custom_hooks.go +++ /dev/null @@ -1,10 +0,0 @@ -package featureflag - -// TransactionalRestoreCustomHooks will use transactional voting in the -// RestoreCustomHooks RPC -var TransactionalRestoreCustomHooks = NewFeatureFlag( - "tx_restore_custom_hooks", - "v15.0.0", - "https://gitlab.com/gitlab-org/gitaly/-/issues/4203", - true, -) diff --git a/internal/praefect/coordinator.go b/internal/praefect/coordinator.go index 3a2c00497..44f21e82b 100644 --- a/internal/praefect/coordinator.go +++ b/internal/praefect/coordinator.go @@ -42,12 +42,6 @@ type transactionsCondition func(context.Context) bool func transactionsEnabled(context.Context) bool { return true } func transactionsDisabled(context.Context) bool { return false } -func transactionsFlag(flag featureflag.FeatureFlag) transactionsCondition { - return func(ctx context.Context) bool { - return flag.IsEnabled(ctx) - } -} - // transactionRPCs contains the list of repository-scoped mutating calls which may take part in // transactions. An optional feature flag can be added to conditionally enable transactional // behaviour. If none is given, it's always enabled. @@ -82,6 +76,8 @@ var transactionRPCs = map[string]transactionsCondition{ "/gitaly.RepositoryService/FetchSourceBranch": transactionsEnabled, "/gitaly.RepositoryService/RemoveRepository": transactionsEnabled, "/gitaly.RepositoryService/ReplicateRepository": transactionsEnabled, + "/gitaly.RepositoryService/RestoreCustomHooks": transactionsEnabled, + "/gitaly.RepositoryService/SetCustomHooks": transactionsEnabled, "/gitaly.RepositoryService/SetFullPath": transactionsEnabled, "/gitaly.RepositoryService/WriteRef": transactionsEnabled, "/gitaly.SSHService/SSHReceivePack": transactionsEnabled, @@ -95,11 +91,6 @@ var transactionRPCs = map[string]transactionsCondition{ "/gitaly.ObjectPoolService/LinkRepositoryToObjectPool": transactionsDisabled, "/gitaly.ObjectPoolService/ReduplicateRepository": transactionsDisabled, "/gitaly.RepositoryService/RenameRepository": transactionsDisabled, - - // The `RestoreCustomHooks` RPC can be make transactional by enabling the - // `TransactionalRestoreCustomHooks` feature flag. - "/gitaly.RepositoryService/RestoreCustomHooks": transactionsFlag(featureflag.TransactionalRestoreCustomHooks), - "/gitaly.RepositoryService/SetCustomHooks": transactionsFlag(featureflag.TransactionalRestoreCustomHooks), } // forcePrimaryRoutingRPCs tracks RPCs which need to always get routed to the primary. This should diff --git a/internal/praefect/coordinator_test.go b/internal/praefect/coordinator_test.go index ae6d72e19..7b101c03c 100644 --- a/internal/praefect/coordinator_test.go +++ b/internal/praefect/coordinator_test.go @@ -1993,10 +1993,8 @@ func TestStreamDirectorStorageScopeError(t *testing.T) { func TestDisabledTransactionsWithFeatureFlag(t *testing.T) { t.Parallel() - testhelper.NewFeatureSets(featureflag.TransactionalRestoreCustomHooks).Run(t, testDisabledTransactionsWithFeatureFlag) -} + ctx := testhelper.Context(t) -func testDisabledTransactionsWithFeatureFlag(t *testing.T, ctx context.Context) { for rpc, enabledFn := range transactionRPCs { if enabledFn(ctx) { require.True(t, shouldUseTransaction(ctx, rpc)) |