localrepo: Remove flag to switch to sidechannels for internal fetches

In commit 8e5cb6419 (git: Convert internal fetches to use sidechannel,
2022-02-16), we have converted internalf teches to use the sidechannel.
With this change, communication exchanged between git-upload-pack(1) and
git-fetch(1) don't use gRPC encoding anymore, but instead use pkt-line
encoding such that we can avoid having to serialize and deserialize lots
of gRPC messages. This avoids the overhead induced by grpc-go and should
thus reduce the load while serving an internal fetch.

This change has been rolled out to production on February 23rd without
any issues observed. Remove the feature flag to unconditionally enable
use of the sidechannel.

Changelog: changed

9 files changed, 14 insertions, 65 deletions

diff --git a/internal/git/localrepo/remote.go b/internal/git/localrepo/remote.go
index 823372a3f..16a4d4e36 100644
--- a/internal/git/localrepo/remote.go
+++ b/internal/git/localrepo/remote.go
@@ -9,7 +9,6 @@ import (
 	"strings"
 
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git"
-	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb"
 )
 
@@ -137,24 +136,13 @@ func (repo *Repo) FetchInternal(
 		// shouldn't even be included in the negotiation phase, so they aren't going to
 		// matter in the connectivity check either.
 		git.WithConfig(git.ConfigPair{Key: "core.alternateRefsCommand", Value: "exit 0 #"}),
-	}
-
-	if featureflag.FetchInternalWithSidechannel.IsEnabled(ctx) {
-		commandOptions = append(commandOptions, git.WithInternalFetchWithSidechannel(
+		git.WithInternalFetchWithSidechannel(
 			&gitalypb.SSHUploadPackWithSidechannelRequest{
 				Repository:       remoteRepo,
 				GitConfigOptions: []string{"uploadpack.allowAnySHA1InWant=true"},
 				GitProtocol:      git.ProtocolV2,
 			},
-		))
-	} else {
-		commandOptions = append(commandOptions, git.WithInternalFetch(
-			&gitalypb.SSHUploadPackRequest{
-				Repository:       remoteRepo,
-				GitConfigOptions: []string{"uploadpack.allowAnySHA1InWant=true"},
-				GitProtocol:      git.ProtocolV2,
-			},
-		))
+		),
 	}
 
 	if opts.DisableTransactions {
diff --git a/internal/git/localrepo/remote_extra_test.go b/internal/git/localrepo/remote_extra_test.go
index 834fcd261..7f4afd350 100644
--- a/internal/git/localrepo/remote_extra_test.go
+++ b/internal/git/localrepo/remote_extra_test.go
@@ -2,7 +2,6 @@ package localrepo_test
 
 import (
 	"bytes"
-	"context"
 	"path/filepath"
 	"testing"
 
@@ -14,7 +13,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/service/hook"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/service/repository"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/service/ssh"
-	"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/testcfg"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper/testserver"
@@ -24,10 +22,8 @@ import (
 
 func TestRepo_FetchInternal(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.FetchInternalWithSidechannel).Run(t, testRepoFetchInternal)
-}
 
-func testRepoFetchInternal(t *testing.T, ctx context.Context) {
+	ctx := testhelper.Context(t)
 	cfg := testcfg.Build(t)
 	gitCmdFactory, readGitProtocol := gittest.NewProtocolDetectingCommandFactory(ctx, t, cfg)
 
diff --git a/internal/gitaly/service/conflicts/resolve_conflicts_test.go b/internal/gitaly/service/conflicts/resolve_conflicts_test.go
index 6c29067ac..0cc778069 100644
--- a/internal/gitaly/service/conflicts/resolve_conflicts_test.go
+++ b/internal/gitaly/service/conflicts/resolve_conflicts_test.go
@@ -18,7 +18,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/localrepo"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/hook"
-	"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/testcfg"
 	"gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb"
@@ -200,10 +199,7 @@ func TestSuccessfulResolveConflictsRequestHelper(t *testing.T) {
 func TestResolveConflictsWithRemoteRepo(t *testing.T) {
 	t.Parallel()
 
-	testhelper.NewFeatureSets(featureflag.FetchInternalWithSidechannel).Run(t, testResolveConflictsWithRemoteRepo)
-}
-
-func testResolveConflictsWithRemoteRepo(t *testing.T, ctx context.Context) {
+	ctx := testhelper.Context(t)
 	hookManager := hook.NewMockManager(t, hook.NopPreReceive, hook.NopPostReceive, hook.NopUpdate, hook.NopReferenceTransaction)
 	cfg, sourceRepo, sourceRepoPath, client := SetupConflictsService(ctx, t, true, hookManager)
 
@@ -817,10 +813,9 @@ func TestFailedResolveConflictsRequestDueToValidation(t *testing.T) {
 }
 
 func TestResolveConflictsQuarantine(t *testing.T) {
-	testhelper.NewFeatureSets(featureflag.FetchInternalWithSidechannel).Run(t, testResolveConflictsQuarantine)
-}
+	t.Parallel()
 
-func testResolveConflictsQuarantine(t *testing.T, ctx context.Context) {
+	ctx := testhelper.Context(t)
 	cfg, sourceRepoProto, sourceRepoPath, client := SetupConflictsService(ctx, t, true, nil)
 
 	testcfg.BuildGitalySSH(t, cfg)
diff --git a/internal/gitaly/service/operations/rebase_test.go b/internal/gitaly/service/operations/rebase_test.go
index 901db8214..1d388ded7 100644
--- a/internal/gitaly/service/operations/rebase_test.go
+++ b/internal/gitaly/service/operations/rebase_test.go
@@ -1,7 +1,6 @@
 package operations
 
 import (
-	"context"
 	"fmt"
 	"io"
 	"path/filepath"
@@ -16,7 +15,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/config"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/transaction"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata"
-	"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"
@@ -656,10 +654,8 @@ func TestRebaseRequestWithDeletedFile(t *testing.T) {
 
 func TestRebaseOntoRemoteBranch(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.FetchInternalWithSidechannel).Run(t, testRebaseOntoRemoteBranch)
-}
 
-func testRebaseOntoRemoteBranch(t *testing.T, ctx context.Context) {
+	ctx := testhelper.Context(t)
 	ctx, cfg, repoProto, repoPath, client := setupOperationsService(t, ctx)
 
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
diff --git a/internal/gitaly/service/operations/revert_test.go b/internal/gitaly/service/operations/revert_test.go
index 27a6cda0d..22e10efa2 100644
--- a/internal/gitaly/service/operations/revert_test.go
+++ b/internal/gitaly/service/operations/revert_test.go
@@ -1,7 +1,6 @@
 package operations
 
 import (
-	"context"
 	"fmt"
 	"path/filepath"
 	"testing"
@@ -11,7 +10,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/localrepo"
 	"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/proto/go/gitalypb"
 	"google.golang.org/grpc/codes"
@@ -275,10 +273,7 @@ func TestServer_UserRevert_stableID(t *testing.T) {
 func TestServer_UserRevert_successfulIntoEmptyRepo(t *testing.T) {
 	t.Parallel()
 
-	testhelper.NewFeatureSets(featureflag.FetchInternalWithSidechannel).Run(t, testServerUserRevertSuccessfulIntoEmptyRepo)
-}
-
-func testServerUserRevertSuccessfulIntoEmptyRepo(t *testing.T, ctx context.Context) {
+	ctx := testhelper.Context(t)
 	ctx, cfg, startRepoProto, _, client := setupOperationsService(t, ctx)
 
 	startRepo := localrepo.NewTestRepo(t, cfg, startRepoProto)
diff --git a/internal/gitaly/service/repository/fetch_test.go b/internal/gitaly/service/repository/fetch_test.go
index 30c7b14af..360ced75d 100644
--- a/internal/gitaly/service/repository/fetch_test.go
+++ b/internal/gitaly/service/repository/fetch_test.go
@@ -1,14 +1,12 @@
 package repository
 
 import (
-	"context"
 	"testing"
 
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/localrepo"
-	"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/testcfg"
 	"gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb"
@@ -18,10 +16,7 @@ import (
 func TestFetchSourceBranchSourceRepositorySuccess(t *testing.T) {
 	t.Parallel()
 
-	testhelper.NewFeatureSets(featureflag.FetchInternalWithSidechannel).Run(t, testFetchSourceBranchSourceRepositorySuccess)
-}
-
-func testFetchSourceBranchSourceRepositorySuccess(t *testing.T, ctx context.Context) {
+	ctx := testhelper.Context(t)
 	cfg, sourceRepo, sourcePath, client := setupRepositoryService(ctx, t)
 
 	md := testcfg.GitalyServersMetadataFromCfg(t, cfg)
diff --git a/internal/gitaly/service/repository/replicate_test.go b/internal/gitaly/service/repository/replicate_test.go
index 7c2231aea..7b87f8923 100644
--- a/internal/gitaly/service/repository/replicate_test.go
+++ b/internal/gitaly/service/repository/replicate_test.go
@@ -43,7 +43,6 @@ import (
 func TestReplicateRepository(t *testing.T) {
 	t.Parallel()
 	testhelper.NewFeatureSets(
-		featureflag.FetchInternalWithSidechannel,
 		featureflag.TransactionalSymbolicRefUpdates,
 	).Run(t, testReplicateRepository)
 }
@@ -121,7 +120,6 @@ func testReplicateRepository(t *testing.T, ctx context.Context) {
 func TestReplicateRepositoryTransactional(t *testing.T) {
 	t.Parallel()
 	testhelper.NewFeatureSets(
-		featureflag.FetchInternalWithSidechannel,
 		featureflag.TransactionalSymbolicRefUpdates,
 	).Run(t, testReplicateRepositoryTransactional)
 }
@@ -307,7 +305,6 @@ func TestReplicateRepositoryInvalidArguments(t *testing.T) {
 func TestReplicateRepository_BadRepository(t *testing.T) {
 	t.Parallel()
 	testhelper.NewFeatureSets(
-		featureflag.FetchInternalWithSidechannel,
 		featureflag.TransactionalSymbolicRefUpdates,
 	).Run(t, testReplicateRepositoryBadRepository)
 }
@@ -395,7 +392,6 @@ func testReplicateRepositoryBadRepository(t *testing.T, ctx context.Context) {
 func TestReplicateRepository_FailedFetchInternalRemote(t *testing.T) {
 	t.Parallel()
 	testhelper.NewFeatureSets(
-		featureflag.FetchInternalWithSidechannel,
 		featureflag.TransactionalSymbolicRefUpdates,
 	).Run(t, testReplicateRepositoryFailedFetchInternalRemote)
 }
@@ -481,7 +477,6 @@ func listenGitalySSHCalls(t *testing.T, conf config.Cfg) func() gitalySSHParams
 func TestFetchInternalRemote_successful(t *testing.T) {
 	t.Parallel()
 	testhelper.NewFeatureSets(
-		featureflag.FetchInternalWithSidechannel,
 		featureflag.TransactionalSymbolicRefUpdates,
 	).Run(t, testFetchInternalRemoteSuccessful)
 }
@@ -573,7 +568,6 @@ func testFetchInternalRemoteSuccessful(t *testing.T, ctx context.Context) {
 func TestFetchInternalRemote_failure(t *testing.T) {
 	t.Parallel()
 	testhelper.NewFeatureSets(
-		featureflag.FetchInternalWithSidechannel,
 		featureflag.TransactionalSymbolicRefUpdates,
 	).Run(t, testFetchInternalRemoteFailure)
 }
diff --git a/internal/metadata/featureflag/ff_fetch_internal_with_sidechannel.go b/internal/metadata/featureflag/ff_fetch_internal_with_sidechannel.go
deleted file mode 100644
index 726aeb0ea..000000000
--- a/internal/metadata/featureflag/ff_fetch_internal_with_sidechannel.go
+++ /dev/null
@@ -1,5 +0,0 @@
-package featureflag
-
-// FetchInternalWithSidechannel enables the use of SSHUploadPackWithSidechannel for internal
-// fetches.
-var FetchInternalWithSidechannel = NewFeatureFlag("fetch_internal_with_sidechannel", false)
diff --git a/internal/praefect/replicator_test.go b/internal/praefect/replicator_test.go
index 7cd48ff3a..6ddb85128 100644
--- a/internal/praefect/replicator_test.go
+++ b/internal/praefect/replicator_test.go
@@ -26,7 +26,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/storage"
 	"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/middleware/metadatahandler"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/praefect/config"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/praefect/datastore"
@@ -52,10 +51,9 @@ func TestMain(m *testing.M) {
 
 func TestReplMgr_ProcessBacklog(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.FetchInternalWithSidechannel).Run(t, testReplMgrProcessBacklog)
-}
 
-func testReplMgrProcessBacklog(t *testing.T, ctx context.Context) {
+	ctx := testhelper.Context(t)
+
 	primaryCfg, testRepoProto, testRepoPath := testcfg.BuildWithRepo(t, testcfg.WithStorages("primary"))
 	testRepo := localrepo.NewTestRepo(t, primaryCfg, testRepoProto)
 	primaryCfg.SocketPath = testserver.RunGitalyServer(t, primaryCfg, nil, setup.RegisterAll, testserver.WithDisablePraefect())
@@ -681,10 +679,9 @@ func getChecksumFunc(ctx context.Context, client gitalypb.RepositoryServiceClien
 
 func TestProcessBacklog_FailedJobs(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.FetchInternalWithSidechannel).Run(t, testProcessBacklogFailedJobs)
-}
 
-func testProcessBacklogFailedJobs(t *testing.T, ctx context.Context) {
+	ctx := testhelper.Context(t)
+
 	primaryCfg, testRepo, _ := testcfg.BuildWithRepo(t, testcfg.WithStorages("default"))
 	primaryAddr := testserver.RunGitalyServer(t, primaryCfg, nil, setup.RegisterAll, testserver.WithDisablePraefect())
 
@@ -787,10 +784,8 @@ func testProcessBacklogFailedJobs(t *testing.T, ctx context.Context) {
 
 func TestProcessBacklog_Success(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.FetchInternalWithSidechannel).Run(t, testProcessBacklogSuccess)
-}
 
-func testProcessBacklogSuccess(t *testing.T, ctx context.Context) {
+	ctx := testhelper.Context(t)
 	ctx, cancel := context.WithCancel(ctx)
 
 	primaryCfg, testRepo, _ := testcfg.BuildWithRepo(t, testcfg.WithStorages("primary"))