Merge branch 'pks-tx-reintroduce-feature-flag' into 'master'

Revert "featureflag: Remove reference transaction feature flag"

See merge request gitlab-org/gitaly!2884

9 files changed, 79 insertions, 39 deletions

diff --git a/changelogs/unreleased/pks-tx-reintroduce-feature-flag.yml b/changelogs/unreleased/pks-tx-reintroduce-feature-flag.yml
new file mode 100644
index 000000000..2cb155714
--- /dev/null
+++ b/changelogs/unreleased/pks-tx-reintroduce-feature-flag.yml
@@ -0,0 +1,5 @@
+---
+title: 'Revert featureflag: Remove reference transaction feature flag'
+merge_request: 2884
+author:
+type: changed
diff --git a/doc/design_ha.md b/doc/design_ha.md
index bf8e06a2b..e3e58855c 100644
--- a/doc/design_ha.md
+++ b/doc/design_ha.md
@@ -461,8 +461,13 @@ changes.
 ## Using Strong Consistency
 
 The current implementation of strong consistency via reference-transaction hook
-is enabled by default. In order to observe reference transactions, the following
-metrics can be used:
+is enabled by default. You can use the following feature flags to change its
+behavior:
+
+- `gitaly_reference_transactions`: This feature flag is enabled by default. If
+  disabled, reference transactions will not be used.
+
+In order to observe reference transactions, the following metrics can be used:
 
 - `gitaly_praefect_transactions_total`: The number of transactions created.
 
diff --git a/internal/gitaly/service/operations/branches_test.go b/internal/gitaly/service/operations/branches_test.go
index 15ab842d7..f7e279d5e 100644
--- a/internal/gitaly/service/operations/branches_test.go
+++ b/internal/gitaly/service/operations/branches_test.go
@@ -214,7 +214,10 @@ func TestUserCreateBranchWithTransaction(t *testing.T) {
 }
 
 func TestSuccessfulGitHooksForUserCreateBranchRequest(t *testing.T) {
-	testWithFeature(t, featureflag.GoUserCreateBranch, testSuccessfulGitHooksForUserCreateBranchRequest)
+	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
+		featureflag.ReferenceTransactions,
+		featureflag.GoUserCreateBranch,
+	}).Run(t, testSuccessfulGitHooksForUserCreateBranchRequest)
 }
 
 func testSuccessfulGitHooksForUserCreateBranchRequest(t *testing.T, ctx context.Context) {
@@ -435,7 +438,10 @@ func testFailedUserCreateBranchRequest(t *testing.T, ctx context.Context) {
 }
 
 func TestSuccessfulUserDeleteBranchRequest(t *testing.T) {
-	testWithFeature(t, featureflag.GoUserDeleteBranch, testSuccessfulUserDeleteBranchRequest)
+	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
+		featureflag.ReferenceTransactions,
+		featureflag.GoUserDeleteBranch,
+	}).Run(t, testSuccessfulUserDeleteBranchRequest)
 }
 
 func testSuccessfulUserDeleteBranchRequest(t *testing.T, ctx context.Context) {
diff --git a/internal/gitaly/service/operations/tags_test.go b/internal/gitaly/service/operations/tags_test.go
index 06f04c161..3e420c0d5 100644
--- a/internal/gitaly/service/operations/tags_test.go
+++ b/internal/gitaly/service/operations/tags_test.go
@@ -20,7 +20,10 @@ import (
 )
 
 func TestSuccessfulUserDeleteTagRequest(t *testing.T) {
-	testWithFeature(t, featureflag.GoUserDeleteTag, testSuccessfulUserDeleteTagRequest)
+	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
+		featureflag.ReferenceTransactions,
+		featureflag.GoUserDeleteTag,
+	}).Run(t, testSuccessfulUserDeleteTagRequest)
 }
 
 func testSuccessfulUserDeleteTagRequest(t *testing.T, ctx context.Context) {
@@ -53,7 +56,10 @@ func testSuccessfulUserDeleteTagRequest(t *testing.T, ctx context.Context) {
 }
 
 func TestSuccessfulGitHooksForUserDeleteTagRequest(t *testing.T) {
-	testWithFeature(t, featureflag.GoUserDeleteTag, testSuccessfulGitHooksForUserDeleteTagRequest)
+	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
+		featureflag.ReferenceTransactions,
+		featureflag.GoUserDeleteTag,
+	}).Run(t, testSuccessfulGitHooksForUserDeleteTagRequest)
 }
 
 func testSuccessfulGitHooksForUserDeleteTagRequest(t *testing.T, ctx context.Context) {
@@ -145,9 +151,12 @@ end`, config.Config.Git.BinPath)
 }
 
 func TestSuccessfulUserCreateTagRequest(t *testing.T) {
-	ctx, cancel := testhelper.Context()
-	defer cancel()
+	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
+		featureflag.ReferenceTransactions,
+	}).Run(t, testSuccessfulUserCreateTagRequest)
+}
 
+func testSuccessfulUserCreateTagRequest(t *testing.T, ctx context.Context) {
 	serverSocketPath, stop := runOperationServiceServer(t)
 	defer stop()
 
@@ -242,7 +251,10 @@ func TestSuccessfulUserCreateTagRequest(t *testing.T) {
 // see
 // https://gitlab.com/gitlab-org/gitaly/-/merge_requests/2839#note_458751929
 func TestUserDeleteTagsuccessfulDeletionOfPrefixedTag(t *testing.T) {
-	testWithFeature(t, featureflag.GoUserDeleteTag, testUserDeleteTagsuccessfulDeletionOfPrefixedTag)
+	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
+		featureflag.ReferenceTransactions,
+		featureflag.GoUserDeleteTag,
+	}).Run(t, testUserDeleteTagsuccessfulDeletionOfPrefixedTag)
 }
 
 func testUserDeleteTagsuccessfulDeletionOfPrefixedTag(t *testing.T, ctx context.Context) {
@@ -342,7 +354,10 @@ func testSuccessfulGitHooksForUserCreateTagRequest(t *testing.T, ctx context.Con
 }
 
 func TestFailedUserDeleteTagRequestDueToValidation(t *testing.T) {
-	testWithFeature(t, featureflag.GoUserDeleteTag, testFailedUserDeleteTagRequestDueToValidation)
+	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
+		featureflag.GoUserDeleteTag,
+		featureflag.ReferenceTransactions,
+	}).Run(t, testFailedUserDeleteTagRequestDueToValidation)
 }
 
 func testFailedUserDeleteTagRequestDueToValidation(t *testing.T, ctx context.Context) {
@@ -401,7 +416,10 @@ func testFailedUserDeleteTagRequestDueToValidation(t *testing.T, ctx context.Con
 }
 
 func TestFailedUserDeleteTagDueToHooks(t *testing.T) {
-	testWithFeature(t, featureflag.GoUserDeleteTag, testFailedUserDeleteTagDueToHooks)
+	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
+		featureflag.GoUserDeleteTag,
+		featureflag.ReferenceTransactions,
+	}).Run(t, testFailedUserDeleteTagDueToHooks)
 }
 
 func testFailedUserDeleteTagDueToHooks(t *testing.T, ctx context.Context) {
@@ -569,7 +587,10 @@ func TestFailedUserCreateTagRequestDueToValidation(t *testing.T) {
 }
 
 func TestTagHookOutput(t *testing.T) {
-	testWithFeature(t, featureflag.GoUserDeleteTag, testTagHookOutput)
+	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
+		featureflag.GoUserDeleteTag,
+		featureflag.ReferenceTransactions,
+	}).Run(t, testTagHookOutput)
 }
 
 func testTagHookOutput(t *testing.T, ctx context.Context) {
diff --git a/internal/gitaly/service/operations/update_branches_test.go b/internal/gitaly/service/operations/update_branches_test.go
index c92bdb3b8..0af47e90e 100644
--- a/internal/gitaly/service/operations/update_branches_test.go
+++ b/internal/gitaly/service/operations/update_branches_test.go
@@ -8,6 +8,7 @@ import (
 
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/internal/git/log"
+	"gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
 	"google.golang.org/grpc/codes"
@@ -20,9 +21,12 @@ var (
 )
 
 func TestSuccessfulUserUpdateBranchRequest(t *testing.T) {
-	ctx, cancel := testhelper.Context()
-	defer cancel()
+	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
+		featureflag.ReferenceTransactions,
+	}).Run(t, testSuccessfulUserUpdateBranchRequest)
+}
 
+func testSuccessfulUserUpdateBranchRequest(t *testing.T, ctx context.Context) {
 	testRepo, _, cleanupFn := testhelper.NewTestRepo(t)
 	defer cleanupFn()
 
diff --git a/internal/gitaly/service/smarthttp/receive_pack_test.go b/internal/gitaly/service/smarthttp/receive_pack_test.go
index 529a9cb3b..c0aa2a630 100644
--- a/internal/gitaly/service/smarthttp/receive_pack_test.go
+++ b/internal/gitaly/service/smarthttp/receive_pack_test.go
@@ -21,6 +21,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/internal/gitaly/service/hook"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/internal/helper/text"
+	"gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag"
 	pconfig "gitlab.com/gitlab-org/gitaly/internal/praefect/config"
 	"gitlab.com/gitlab-org/gitaly/internal/praefect/metadata"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
@@ -456,6 +457,12 @@ func runSmartHTTPHookServiceServer(t *testing.T) (*grpc.Server, string) {
 }
 
 func TestPostReceiveWithTransactionsViaPraefect(t *testing.T) {
+	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
+		featureflag.ReferenceTransactions,
+	}).Run(t, testPostReceiveWithTransactionsViaPraefect)
+}
+
+func testPostReceiveWithTransactionsViaPraefect(t *testing.T, ctx context.Context) {
 	defer func(cfg config.Cfg) {
 		config.Config = cfg
 	}(config.Config)
@@ -525,9 +532,6 @@ func TestPostReceiveWithTransactionsViaPraefect(t *testing.T) {
 	client, conn := newSmartHTTPClient(t, "unix://"+gitalyServer.Socket())
 	defer conn.Close()
 
-	ctx, cancel := testhelper.Context()
-	defer cancel()
-
 	stream, err := client.PostReceivePack(ctx)
 	require.NoError(t, err)
 
diff --git a/internal/metadata/featureflag/feature_flags.go b/internal/metadata/featureflag/feature_flags.go
index af91c729f..d4f33cc58 100644
--- a/internal/metadata/featureflag/feature_flags.go
+++ b/internal/metadata/featureflag/feature_flags.go
@@ -13,6 +13,8 @@ var (
 	GoFetchSourceBranch = FeatureFlag{Name: "go_fetch_source_branch", OnByDefault: false}
 	// DistributedReads allows praefect to redirect accessor operations to up-to-date secondaries
 	DistributedReads = FeatureFlag{Name: "distributed_reads", OnByDefault: false}
+	// ReferenceTransactions will handle Git reference updates via the transaction service for strong consistency
+	ReferenceTransactions = FeatureFlag{Name: "reference_transactions", OnByDefault: true}
 	// LogCommandStats will log additional rusage stats for commands
 	LogCommandStats = FeatureFlag{Name: "log_command_stats", OnByDefault: false}
 	// GoUserMergeBranch enables the Go implementation of UserMergeBranch
@@ -108,6 +110,7 @@ var All = []FeatureFlag{
 	GoFetchSourceBranch,
 	DistributedReads,
 	LogCommandStats,
+	ReferenceTransactions,
 	GoUserMergeBranch,
 	GoUserFFBranch,
 	GoUserCreateBranch,
diff --git a/internal/praefect/coordinator.go b/internal/praefect/coordinator.go
index 9157d21d4..ad549f75e 100644
--- a/internal/praefect/coordinator.go
+++ b/internal/praefect/coordinator.go
@@ -4,7 +4,6 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"os"
 
 	"github.com/golang/protobuf/proto"
 	"github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus"
@@ -142,11 +141,8 @@ func init() {
 	}
 }
 
-const gitalyDisabledRefEnvVar = "GITALY_DISABLE_REF_TRANSACTIONS"
-
 func shouldUseTransaction(ctx context.Context, method string) bool {
-	// Disabling based on a environment variable, as a poor mans feature flag.
-	if _, ok := os.LookupEnv(gitalyDisabledRefEnvVar); ok {
+	if !featureflag.IsEnabled(ctx, featureflag.ReferenceTransactions) {
 		return false
 	}
 
diff --git a/internal/praefect/coordinator_test.go b/internal/praefect/coordinator_test.go
index db6bbe6eb..660850a83 100644
--- a/internal/praefect/coordinator_test.go
+++ b/internal/praefect/coordinator_test.go
@@ -5,7 +5,6 @@ import (
 	"crypto/sha1"
 	"errors"
 	"io/ioutil"
-	"os"
 	"sync"
 	"testing"
 	"time"
@@ -969,23 +968,20 @@ func TestStreamDirectorStorageScopeError(t *testing.T) {
 	})
 }
 
-func TestDisabledTransactionsWithEnvVar(t *testing.T) {
-	os.Setenv(gitalyDisabledRefEnvVar, "1")
-	defer os.Unsetenv(gitalyDisabledRefEnvVar)
-
-	ctx, cancel := testhelper.Context()
-	defer cancel()
-
-	var rpcname string
-	for name, enabledFn := range transactionRPCs {
-		if enabledFn(ctx) {
-			rpcname = name
-			break
+func TestDisabledTransactionsWithFeatureFlag(t *testing.T) {
+	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
+		featureflag.ReferenceTransactions,
+	}).Run(t, func(t *testing.T, ctx context.Context) {
+		for rpc, enabledFn := range transactionRPCs {
+			if enabledFn(ctx) {
+				require.Equal(t,
+					featureflag.IsEnabled(ctx, featureflag.ReferenceTransactions),
+					shouldUseTransaction(ctx, rpc),
+				)
+				break
+			}
 		}
-	}
-
-	require.NotEmpty(t, rpcname, "no enabled reference transaction RPCs")
-	require.False(t, shouldUseTransaction(ctx, rpcname))
+	})
 }
 
 func requireScopeOperation(t *testing.T, registry *protoregistry.Registry, fullMethod string, scope protoregistry.Scope, op protoregistry.OpType) {