Merge branch 'jt-remove-node-error-cancels-voter-flag' into 'master'

Praefect: Remove node voter cancel flag

See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5219

Merged-by: Will Chandler <wchandler@gitlab.com>
Approved-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Co-authored-by: Justin Tobler <jtobler@gitlab.com>

4 files changed, 8 insertions, 38 deletions

diff --git a/internal/metadata/featureflag/ff_node_error_cancels_voter.go b/internal/metadata/featureflag/ff_node_error_cancels_voter.go
deleted file mode 100644
index 42fce7073..000000000
--- a/internal/metadata/featureflag/ff_node_error_cancels_voter.go
+++ /dev/null
@@ -1,11 +0,0 @@
-package featureflag
-
-// NodeErrorCancelsVoter enables cancellation of the voter associated
-// with a failed node RPC. By canceling voters that can no longer vote,
-// the transaction can fail faster if quorum becomes impossible.
-var NodeErrorCancelsVoter = NewFeatureFlag(
-	"node_error_cancels_voter",
-	"v15.6.0",
-	"https://gitlab.com/gitlab-org/gitaly/-/issues/4552",
-	true,
-)
diff --git a/internal/praefect/coordinator.go b/internal/praefect/coordinator.go
index 170c456ee..51ddd0b45 100644
--- a/internal/praefect/coordinator.go
+++ b/internal/praefect/coordinator.go
@@ -473,14 +473,12 @@ func (c *Coordinator) mutatorStreamParameters(ctx context.Context, call grpcCall
 					ctxlogrus.Extract(ctx).WithError(err).
 						Error("proxying to secondary failed")
 
-					if featureflag.NodeErrorCancelsVoter.IsEnabled(ctx) {
-						// Cancels failed node's voter in its current subtransaction.
-						// Also updates internal state of subtransaction to fail and
-						// release blocked voters if quorum becomes impossible.
-						if err := c.txMgr.CancelTransactionNodeVoter(transaction.ID(), secondary.Storage); err != nil {
-							ctxlogrus.Extract(ctx).WithError(err).
-								Error("canceling secondary voter failed")
-						}
+					// Cancels failed node's voter in its current subtransaction.
+					// Also updates internal state of subtransaction to fail and
+					// release blocked voters if quorum becomes impossible.
+					if err := c.txMgr.CancelTransactionNodeVoter(transaction.ID(), secondary.Storage); err != nil {
+						ctxlogrus.Extract(ctx).WithError(err).
+							Error("canceling secondary voter failed")
 					}
 
 					// The error is ignored, so we do not abort transactions
diff --git a/internal/praefect/coordinator_test.go b/internal/praefect/coordinator_test.go
index 04cf1aa33..8366f3414 100644
--- a/internal/praefect/coordinator_test.go
+++ b/internal/praefect/coordinator_test.go
@@ -395,11 +395,7 @@ func TestStreamDirectorMutator_StopTransaction(t *testing.T) {
 
 func TestStreamDirectorMutator_SecondaryErrorHandling(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.NodeErrorCancelsVoter).Run(t, testStreamDirectorMutatorSecondaryErrorHandling)
-}
-
-func testStreamDirectorMutatorSecondaryErrorHandling(t *testing.T, ctx context.Context) {
-	ctx, ctxCancel := context.WithCancel(ctx)
+	ctx := testhelper.Context(t)
 
 	socket := testhelper.GetTemporaryGitalySocketFileName(t)
 	testhelper.NewServerWithHealth(t, socket)
@@ -476,11 +472,7 @@ func testStreamDirectorMutatorSecondaryErrorHandling(t *testing.T, ctx context.C
 
 		vote := voting.VoteFromData([]byte("vote"))
 		err := txMgr.VoteTransaction(ctx, transaction.ID, "praefect-internal-1", vote)
-		if featureflag.NodeErrorCancelsVoter.IsEnabled(ctx) {
-			require.ErrorIs(t, err, transactions.ErrTransactionFailed)
-		} else {
-			require.EqualError(t, err, "context canceled")
-		}
+		require.ErrorIs(t, err, transactions.ErrTransactionFailed)
 	}()
 
 	for _, secondary := range streamParams.Secondaries() {
@@ -494,16 +486,10 @@ func testStreamDirectorMutatorSecondaryErrorHandling(t *testing.T, ctx context.C
 		}()
 	}
 
-	// If the feature flag is not enabled the context must be canceled to unblock voters.
-	if !featureflag.NodeErrorCancelsVoter.IsEnabled(ctx) {
-		ctxCancel()
-	}
-
 	wg.Wait()
 
 	err = streamParams.RequestFinalizer()
 	require.NoError(t, err)
-	ctxCancel()
 }
 
 func TestStreamDirectorMutator_ReplicateRepository(t *testing.T) {
diff --git a/internal/testhelper/testhelper.go b/internal/testhelper/testhelper.go
index 07daa58bd..2d4a00943 100644
--- a/internal/testhelper/testhelper.go
+++ b/internal/testhelper/testhelper.go
@@ -197,9 +197,6 @@ func ContextWithoutCancel(opts ...ContextOpt) context.Context {
 	// context. The values of these flags should be randomized to increase the test coverage.
 	ctx = featureflag.ContextWithFeatureFlag(ctx, featureflag.RunCommandsInCGroup, true)
 	ctx = featureflag.ContextWithFeatureFlag(ctx, featureflag.ConvertErrToStatus, true)
-	// NodeErrorCancelsVoter affect many tests as it changes Praefect coordinator transaction logic.
-	// Randomly enable the flag to exercise both paths to some extent.
-	ctx = featureflag.ContextWithFeatureFlag(ctx, featureflag.NodeErrorCancelsVoter, rnd.Int()%2 == 0)
 	// PraefectUseYamuxConfigurationForGitaly gets tested in Praefect when routing RPCs and thus it affects many tests.
 	// Let's randomly select which connection we use so both sets of connections get tested somewhat.
 	ctx = featureflag.ContextWithFeatureFlag(ctx, featureflag.PraefectUseYamuxConfigurationForGitaly, rnd.Int()%2 == 0)