Merge branch 'pks-drop-tx-flags' into 'master'

featureflag: Drop transactional feature flags

Closes #3652

See merge request gitlab-org/gitaly!3589

6 files changed, 106 insertions, 156 deletions

diff --git a/internal/gitaly/service/remote/remotes.go b/internal/gitaly/service/remote/remotes.go
index 9e2f7232a..cc2392521 100644
--- a/internal/gitaly/service/remote/remotes.go
+++ b/internal/gitaly/service/remote/remotes.go
@@ -12,7 +12,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/rubyserver"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/transaction"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper"
-	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/transaction/txinfo"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/transaction/voting"
 	"gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb"
@@ -139,10 +138,6 @@ func validateRemoveRemoteRequest(req *gitalypb.RemoveRemoteRequest) error {
 }
 
 func (s *server) voteOnRemote(ctx context.Context, repo *gitalypb.Repository, remoteName string) error {
-	if featureflag.IsDisabled(ctx, featureflag.TxRemote) {
-		return nil
-	}
-
 	return transaction.RunOnContext(ctx, func(tx txinfo.Transaction) error {
 		localrepo := s.localrepo(repo)
 
diff --git a/internal/gitaly/service/remote/remotes_test.go b/internal/gitaly/service/remote/remotes_test.go
index 0dd518210..f79f62630 100644
--- a/internal/gitaly/service/remote/remotes_test.go
+++ b/internal/gitaly/service/remote/remotes_test.go
@@ -16,7 +16,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/transaction"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper/text"
-	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper/testserver"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/transaction/txinfo"
@@ -103,42 +102,36 @@ func testSuccessfulAddRemote(t *testing.T, cfg config.Cfg, rubySrv *rubyserver.S
 }
 
 func testAddRemoteTransactional(t *testing.T, cfg config.Cfg, rubySrv *rubyserver.Server) {
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.TxRemote,
-	}).Run(t, func(t *testing.T, ctx context.Context) {
-		var votes []voting.Vote
-		txManager := transaction.MockManager{
-			VoteFn: func(_ context.Context, _ txinfo.Transaction, vote voting.Vote) error {
-				votes = append(votes, vote)
-				return nil
-			},
-		}
+	var votes []voting.Vote
+	txManager := transaction.MockManager{
+		VoteFn: func(_ context.Context, _ txinfo.Transaction, vote voting.Vote) error {
+			votes = append(votes, vote)
+			return nil
+		},
+	}
 
-		_, repo, repoPath, client := setupRemoteServiceWithRuby(t, cfg, rubySrv, testserver.WithTransactionManager(&txManager))
+	_, repo, repoPath, client := setupRemoteServiceWithRuby(t, cfg, rubySrv, testserver.WithTransactionManager(&txManager))
 
-		ctx, err := txinfo.InjectTransaction(ctx, 1, "node", true)
-		require.NoError(t, err)
-		ctx = helper.IncomingToOutgoing(ctx)
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+	ctx, err := txinfo.InjectTransaction(ctx, 1, "node", true)
+	require.NoError(t, err)
+	ctx = helper.IncomingToOutgoing(ctx)
 
-		preimageURL := text.ChompBytes(gittest.Exec(t, cfg, "-C", repoPath, "remote", "get-url", "origin"))
+	preimageURL := text.ChompBytes(gittest.Exec(t, cfg, "-C", repoPath, "remote", "get-url", "origin"))
 
-		_, err = client.AddRemote(ctx, &gitalypb.AddRemoteRequest{
-			Repository: repo,
-			Name:       "origin",
-			Url:        "foo/bar",
-		})
-		require.NoError(t, err)
-
-		if featureflag.IsEnabled(ctx, featureflag.TxRemote) {
-			preimageVote := fmt.Sprintf("remote.origin.url\t%s\n", preimageURL)
-			require.Equal(t, []voting.Vote{
-				voting.VoteFromData([]byte(preimageVote)),
-				voting.VoteFromData([]byte("remote.origin.url\tfoo/bar\n")),
-			}, votes)
-		} else {
-			require.Len(t, votes, 0)
-		}
+	_, err = client.AddRemote(ctx, &gitalypb.AddRemoteRequest{
+		Repository: repo,
+		Name:       "origin",
+		Url:        "foo/bar",
 	})
+	require.NoError(t, err)
+
+	preimageVote := fmt.Sprintf("remote.origin.url\t%s\n", preimageURL)
+	require.Equal(t, []voting.Vote{
+		voting.VoteFromData([]byte(preimageVote)),
+		voting.VoteFromData([]byte("remote.origin.url\tfoo/bar\n")),
+	}, votes)
 }
 
 func TestFailedAddRemoteDueToValidation(t *testing.T) {
@@ -237,41 +230,35 @@ func TestFailedRemoveRemoteDueToValidation(t *testing.T) {
 }
 
 func TestRemoveRemoteTransactional(t *testing.T) {
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.TxRemote,
-	}).Run(t, func(t *testing.T, ctx context.Context) {
-		var votes []voting.Vote
-		txManager := transaction.MockManager{
-			VoteFn: func(_ context.Context, _ txinfo.Transaction, vote voting.Vote) error {
-				votes = append(votes, vote)
-				return nil
-			},
-		}
+	var votes []voting.Vote
+	txManager := transaction.MockManager{
+		VoteFn: func(_ context.Context, _ txinfo.Transaction, vote voting.Vote) error {
+			votes = append(votes, vote)
+			return nil
+		},
+	}
 
-		cfg, repo, repoPath, client := setupRemoteService(t, testserver.WithTransactionManager(&txManager))
+	cfg, repo, repoPath, client := setupRemoteService(t, testserver.WithTransactionManager(&txManager))
 
-		ctx, err := txinfo.InjectTransaction(ctx, 1, "node", true)
-		require.NoError(t, err)
-		ctx = helper.IncomingToOutgoing(ctx)
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+	ctx, err := txinfo.InjectTransaction(ctx, 1, "node", true)
+	require.NoError(t, err)
+	ctx = helper.IncomingToOutgoing(ctx)
 
-		preimageURL := text.ChompBytes(gittest.Exec(t, cfg, "-C", repoPath, "remote", "get-url", "origin"))
+	preimageURL := text.ChompBytes(gittest.Exec(t, cfg, "-C", repoPath, "remote", "get-url", "origin"))
 
-		_, err = client.RemoveRemote(ctx, &gitalypb.RemoveRemoteRequest{
-			Repository: repo,
-			Name:       "origin",
-		})
-		require.NoError(t, err)
-
-		if featureflag.IsEnabled(ctx, featureflag.TxRemote) {
-			preimageVote := fmt.Sprintf("remote.origin.url\t%s\n", preimageURL)
-			require.Equal(t, []voting.Vote{
-				voting.VoteFromData([]byte(preimageVote)),
-				voting.VoteFromData([]byte{}),
-			}, votes)
-		} else {
-			require.Len(t, votes, 0)
-		}
+	_, err = client.RemoveRemote(ctx, &gitalypb.RemoveRemoteRequest{
+		Repository: repo,
+		Name:       "origin",
 	})
+	require.NoError(t, err)
+
+	preimageVote := fmt.Sprintf("remote.origin.url\t%s\n", preimageURL)
+	require.Equal(t, []voting.Vote{
+		voting.VoteFromData([]byte(preimageVote)),
+		voting.VoteFromData([]byte{}),
+	}, votes)
 }
 
 func TestFindRemoteRepository(t *testing.T) {
diff --git a/internal/gitaly/service/repository/config.go b/internal/gitaly/service/repository/config.go
index 5bde6a1a1..7926ef46d 100644
--- a/internal/gitaly/service/repository/config.go
+++ b/internal/gitaly/service/repository/config.go
@@ -12,7 +12,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/rubyserver"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/transaction"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper"
-	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/transaction/txinfo"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/transaction/voting"
 	"gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb"
@@ -133,10 +132,6 @@ func (s *server) SetConfig(ctx context.Context, req *gitalypb.SetConfigRequest)
 }
 
 func (s *server) voteOnConfig(ctx context.Context, repo *gitalypb.Repository) error {
-	if featureflag.IsDisabled(ctx, featureflag.TxConfig) {
-		return nil
-	}
-
 	return transaction.RunOnContext(ctx, func(tx txinfo.Transaction) error {
 		repoPath, err := s.locator.GetPath(repo)
 		if err != nil {
diff --git a/internal/gitaly/service/repository/config_test.go b/internal/gitaly/service/repository/config_test.go
index 599962547..747833079 100644
--- a/internal/gitaly/service/repository/config_test.go
+++ b/internal/gitaly/service/repository/config_test.go
@@ -16,7 +16,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/rubyserver"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/transaction"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper"
-	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper/testassert"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper/testserver"
@@ -136,42 +135,36 @@ func TestDeleteConfig(t *testing.T) {
 }
 
 func TestDeleteConfigTransactional(t *testing.T) {
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.TxConfig,
-	}).Run(t, func(t *testing.T, ctx context.Context) {
-		var votes []voting.Vote
-		txManager := transaction.MockManager{
-			VoteFn: func(_ context.Context, _ txinfo.Transaction, vote voting.Vote) error {
-				votes = append(votes, vote)
-				return nil
-			},
-		}
-
-		cfg, repo, repoPath, client := setupRepositoryService(t, testserver.WithTransactionManager(&txManager))
+	var votes []voting.Vote
+	txManager := transaction.MockManager{
+		VoteFn: func(_ context.Context, _ txinfo.Transaction, vote voting.Vote) error {
+			votes = append(votes, vote)
+			return nil
+		},
+	}
 
-		ctx, err := txinfo.InjectTransaction(ctx, 1, "node", true)
-		require.NoError(t, err)
-		ctx = helper.IncomingToOutgoing(ctx)
+	cfg, repo, repoPath, client := setupRepositoryService(t, testserver.WithTransactionManager(&txManager))
 
-		unmodifiedContents := testhelper.MustReadFile(t, filepath.Join(repoPath, "config"))
-		gittest.Exec(t, cfg, "-C", repoPath, "config", "delete.me", "now")
-		modifiedContents := testhelper.MustReadFile(t, filepath.Join(repoPath, "config"))
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+	ctx, err := txinfo.InjectTransaction(ctx, 1, "node", true)
+	require.NoError(t, err)
+	ctx = helper.IncomingToOutgoing(ctx)
 
-		_, err = client.DeleteConfig(ctx, &gitalypb.DeleteConfigRequest{
-			Repository: repo,
-			Keys:       []string{"delete.me"},
-		})
-		require.NoError(t, err)
+	unmodifiedContents := testhelper.MustReadFile(t, filepath.Join(repoPath, "config"))
+	gittest.Exec(t, cfg, "-C", repoPath, "config", "delete.me", "now")
+	modifiedContents := testhelper.MustReadFile(t, filepath.Join(repoPath, "config"))
 
-		if featureflag.IsEnabled(ctx, featureflag.TxConfig) {
-			require.Equal(t, []voting.Vote{
-				voting.VoteFromData(modifiedContents),
-				voting.VoteFromData(unmodifiedContents),
-			}, votes)
-		} else {
-			require.Len(t, votes, 0)
-		}
+	_, err = client.DeleteConfig(ctx, &gitalypb.DeleteConfigRequest{
+		Repository: repo,
+		Keys:       []string{"delete.me"},
 	})
+	require.NoError(t, err)
+
+	require.Equal(t, []voting.Vote{
+		voting.VoteFromData(modifiedContents),
+		voting.VoteFromData(unmodifiedContents),
+	}, votes)
 }
 
 func testSetConfig(t *testing.T, cfg config.Cfg, rubySrv *rubyserver.Server) {
@@ -234,47 +227,41 @@ func testSetConfig(t *testing.T, cfg config.Cfg, rubySrv *rubyserver.Server) {
 }
 
 func testSetConfigTransactional(t *testing.T, cfg config.Cfg, rubySrv *rubyserver.Server) {
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.TxConfig,
-	}).Run(t, func(t *testing.T, ctx context.Context) {
-		var votes []voting.Vote
-
-		txManager := transaction.MockManager{
-			VoteFn: func(_ context.Context, _ txinfo.Transaction, vote voting.Vote) error {
-				votes = append(votes, vote)
-				return nil
-			},
-		}
+	var votes []voting.Vote
 
-		_, repo, repoPath, client := setupRepositoryServiceWithRuby(t, cfg, rubySrv, testserver.WithTransactionManager(&txManager))
+	txManager := transaction.MockManager{
+		VoteFn: func(_ context.Context, _ txinfo.Transaction, vote voting.Vote) error {
+			votes = append(votes, vote)
+			return nil
+		},
+	}
 
-		ctx, err := txinfo.InjectTransaction(ctx, 1, "node", true)
-		require.NoError(t, err)
-		ctx = helper.IncomingToOutgoing(ctx)
+	_, repo, repoPath, client := setupRepositoryServiceWithRuby(t, cfg, rubySrv, testserver.WithTransactionManager(&txManager))
 
-		unmodifiedContents := testhelper.MustReadFile(t, filepath.Join(repoPath, "config"))
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+	ctx, err := txinfo.InjectTransaction(ctx, 1, "node", true)
+	require.NoError(t, err)
+	ctx = helper.IncomingToOutgoing(ctx)
 
-		_, err = client.SetConfig(ctx, &gitalypb.SetConfigRequest{
-			Repository: repo,
-			Entries: []*gitalypb.SetConfigRequest_Entry{
-				&gitalypb.SetConfigRequest_Entry{
-					Key: "set.me",
-					Value: &gitalypb.SetConfigRequest_Entry_ValueStr{
-						"something",
-					},
+	unmodifiedContents := testhelper.MustReadFile(t, filepath.Join(repoPath, "config"))
+
+	_, err = client.SetConfig(ctx, &gitalypb.SetConfigRequest{
+		Repository: repo,
+		Entries: []*gitalypb.SetConfigRequest_Entry{
+			&gitalypb.SetConfigRequest_Entry{
+				Key: "set.me",
+				Value: &gitalypb.SetConfigRequest_Entry_ValueStr{
+					"something",
 				},
 			},
-		})
-		require.NoError(t, err)
-
-		if featureflag.IsEnabled(ctx, featureflag.TxConfig) {
-			modifiedContents := string(unmodifiedContents) + "[set]\n\tme = something\n"
-			require.Equal(t, []voting.Vote{
-				voting.VoteFromData(unmodifiedContents),
-				voting.VoteFromData([]byte(modifiedContents)),
-			}, votes)
-		} else {
-			require.Len(t, votes, 0)
-		}
+		},
 	})
+	require.NoError(t, err)
+
+	modifiedContents := string(unmodifiedContents) + "[set]\n\tme = something\n"
+	require.Equal(t, []voting.Vote{
+		voting.VoteFromData(unmodifiedContents),
+		voting.VoteFromData([]byte(modifiedContents)),
+	}, votes)
 }
diff --git a/internal/metadata/featureflag/feature_flags.go b/internal/metadata/featureflag/feature_flags.go
index 4b8d05bf6..0ec6a989c 100644
--- a/internal/metadata/featureflag/feature_flags.go
+++ b/internal/metadata/featureflag/feature_flags.go
@@ -13,10 +13,6 @@ var (
 	GoUpdateRemoteMirror = FeatureFlag{Name: "go_update_remote_mirror", OnByDefault: false}
 	// FetchInternalRemoteErrors makes FetchInternalRemote return actual errors instead of a boolean
 	FetchInternalRemoteErrors = FeatureFlag{Name: "fetch_internal_remote_errors", OnByDefault: false}
-	// TxConfig enables transactional voting for SetConfig and DeleteConfig RPCs.
-	TxConfig = FeatureFlag{Name: "tx_config", OnByDefault: true}
-	// TxRemote enables transactional voting for AddRemote and DeleteRemote.
-	TxRemote = FeatureFlag{Name: "tx_remote", OnByDefault: true}
 	// UserMergeToRefSkipPrecursorRefUpdate causes UserMergeToRef to not update the
 	// target reference in case computing the merge fails.
 	UserMergeToRefSkipPrecursorRefUpdate = FeatureFlag{Name: "user_merge_to_ref_skip_precursor_ref_update", OnByDefault: true}
@@ -29,8 +25,6 @@ var (
 var All = []FeatureFlag{
 	GoUpdateRemoteMirror,
 	FetchInternalRemoteErrors,
-	TxConfig,
-	TxRemote,
 	UserMergeToRefSkipPrecursorRefUpdate,
 	LFSPointersPipeline,
 }
diff --git a/internal/praefect/coordinator.go b/internal/praefect/coordinator.go
index 6a372d34a..afd59271c 100644
--- a/internal/praefect/coordinator.go
+++ b/internal/praefect/coordinator.go
@@ -13,7 +13,6 @@ import (
 	"github.com/sirupsen/logrus"
 	glerrors "gitlab.com/gitlab-org/gitaly/v14/internal/errors"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper"
-	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/middleware/metadatahandler"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/praefect/commonerr"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/praefect/config"
@@ -39,12 +38,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 featureflag.IsEnabled(ctx, flag)
-	}
-}
-
 // 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.
@@ -79,18 +72,17 @@ var transactionRPCs = map[string]transactionsCondition{
 	"/gitaly.RepositoryService/CreateRepositoryFromBundle":   transactionsEnabled,
 	"/gitaly.RepositoryService/CreateRepositoryFromSnapshot": transactionsEnabled,
 	"/gitaly.RepositoryService/CreateRepositoryFromURL":      transactionsEnabled,
+	"/gitaly.RepositoryService/DeleteConfig":                 transactionsEnabled,
 	"/gitaly.RepositoryService/FetchRemote":                  transactionsEnabled,
 	"/gitaly.RepositoryService/FetchSourceBranch":            transactionsEnabled,
 	"/gitaly.RepositoryService/ReplicateRepository":          transactionsEnabled,
+	"/gitaly.RepositoryService/SetConfig":                    transactionsEnabled,
 	"/gitaly.RepositoryService/WriteRef":                     transactionsEnabled,
 	"/gitaly.SSHService/SSHReceivePack":                      transactionsEnabled,
 	"/gitaly.SmartHTTPService/PostReceivePack":               transactionsEnabled,
 	"/gitaly.WikiService/WikiUpdatePage":                     transactionsEnabled,
 	"/gitaly.WikiService/WikiWritePage":                      transactionsEnabled,
 
-	"/gitaly.RepositoryService/SetConfig":    transactionsFlag(featureflag.TxConfig),
-	"/gitaly.RepositoryService/DeleteConfig": transactionsFlag(featureflag.TxConfig),
-
 	// The following RPCs don't perform any reference updates and thus
 	// shouldn't use transactions.
 	"/gitaly.ObjectPoolService/CreateObjectPool":               transactionsDisabled,