repository: Remove feature flag for direct fetchespks-repository-default-enable-direct-fetches

With the `ReplicateRepositoryDirectFetch` feature flag, we have fixed
`FetchInternalRemote()` to work in a context where called via Praefect.
Previously, we have propagated transactional information via an RPC call,
which then resulted in the remote side trying to cast votes against a
Gitaly connection, which cannot work. With the new code, we don't do an
RPC call but instead just call the relevant function directly.

This feature has been default-enabled for three weeks in production now,
and furthermore it's been backported to v13.12 and v14.0. No problems
were observed in production, and it fixed the original issue for
customers.

Given its exposure, let's remove the feature flag without taking the
intermediate step of first default-enabling it.

Changelog: fixed

3 files changed, 7 insertions, 60 deletions

diff --git a/internal/gitaly/service/repository/replicate.go b/internal/gitaly/service/repository/replicate.go
index a2982fbe4..8fa7c44b4 100644
--- a/internal/gitaly/service/repository/replicate.go
+++ b/internal/gitaly/service/repository/replicate.go
@@ -16,7 +16,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/service/remote"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/storage"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper"
-	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/safe"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/tempdir"
 	"gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb"
@@ -192,33 +191,12 @@ func (s *server) createFromSnapshot(ctx context.Context, in *gitalypb.ReplicateR
 }
 
 func (s *server) syncRepository(ctx context.Context, in *gitalypb.ReplicateRepositoryRequest) error {
-	if featureflag.ReplicateRepositoryDirectFetch.IsEnabled(ctx) {
-		repo := s.localrepo(in.GetRepository())
+	repo := s.localrepo(in.GetRepository())
 
-		if err := remote.FetchInternalRemote(ctx, s.cfg, s.conns, repo, in.GetSource()); err != nil {
-			return fmt.Errorf("fetch internal remote: %w", err)
-		}
-
-		return nil
-	}
-
-	remoteClient, err := s.newRemoteClient(ctx)
-	if err != nil {
-		return fmt.Errorf("new client: %w", err)
-	}
-
-	resp, err := remoteClient.FetchInternalRemote(ctx, &gitalypb.FetchInternalRemoteRequest{
-		Repository:       in.GetRepository(),
-		RemoteRepository: in.GetSource(),
-	})
-	if err != nil {
+	if err := remote.FetchInternalRemote(ctx, s.cfg, s.conns, repo, in.GetSource()); err != nil {
 		return fmt.Errorf("fetch internal remote: %w", err)
 	}
 
-	if !resp.Result {
-		return errors.New("FetchInternalRemote failed")
-	}
-
 	return nil
 }
 
@@ -319,16 +297,6 @@ func writeFile(path string, mode os.FileMode, reader io.Reader) error {
 	return nil
 }
 
-// newRemoteClient creates a new RemoteClient that talks to the same gitaly server
-func (s *server) newRemoteClient(ctx context.Context) (gitalypb.RemoteServiceClient, error) {
-	conn, err := s.conns.Dial(ctx, fmt.Sprintf("unix:%s", s.cfg.GitalyInternalSocketPath()), s.cfg.Auth.Token)
-	if err != nil {
-		return nil, err
-	}
-
-	return gitalypb.NewRemoteServiceClient(conn), nil
-}
-
 // newRepoClient creates a new RepositoryClient that talks to the gitaly of the source repository
 func (s *server) newRepoClient(ctx context.Context, storageName string) (gitalypb.RepositoryServiceClient, error) {
 	gitalyServerInfo, err := helper.ExtractGitalyServer(ctx, storageName)
diff --git a/internal/gitaly/service/repository/replicate_test.go b/internal/gitaly/service/repository/replicate_test.go
index a89228393..063367cbd 100644
--- a/internal/gitaly/service/repository/replicate_test.go
+++ b/internal/gitaly/service/repository/replicate_test.go
@@ -15,7 +15,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/service"
 	"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/testcfg"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper/testserver"
@@ -99,13 +98,7 @@ func TestReplicateRepository(t *testing.T) {
 	gittest.Exec(t, cfg, "-C", targetRepoPath, "cat-file", "-p", blobID)
 }
 
-func TestReplicateRepository_transactional(t *testing.T) {
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.ReplicateRepositoryDirectFetch,
-	}).Run(t, testReplicateRepositoryTransactional)
-}
-
-func testReplicateRepositoryTransactional(t *testing.T, ctx context.Context) {
+func TestReplicateRepositoryTransactional(t *testing.T) {
 	cfgBuilder := testcfg.NewGitalyCfgBuilder(testcfg.WithStorages("default", "replica"))
 	cfg := cfgBuilder.Build(t)
 
@@ -131,6 +124,8 @@ func testReplicateRepositoryTransactional(t *testing.T, ctx context.Context) {
 		},
 	}
 
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 	ctx, err := txinfo.InjectTransaction(ctx, 1, "primary", true)
 	require.NoError(t, err)
 	ctx = helper.IncomingToOutgoing(ctx)
@@ -167,19 +162,8 @@ func testReplicateRepositoryTransactional(t *testing.T, ctx context.Context) {
 		Source:     sourceRepo,
 	})
 
-	if featureflag.ReplicateRepositoryDirectFetch.IsEnabled(ctx) {
-		require.NoError(t, err)
-		require.Equal(t, 2, votes)
-	} else {
-		// This is failing because we do a nested mutating RPC in `ReplicateRepository()` to
-		// `FetchInternalRemote()`. Because we simply pass along the incoming context as an
-		// outgoing one, the server would try to vote on the backchannel. But given that the
-		// connection is not to Praefect but to Gitaly now, it's trying to cast votes on a
-		// non-multiplexed Gitaly connection instead of against the expected Praefect peer.
-		require.Error(t, err)
-		require.Contains(t, err.Error(), "FetchInternalRemote failed")
-		require.Equal(t, 0, votes)
-	}
+	require.NoError(t, err)
+	require.Equal(t, 2, votes)
 }
 
 func TestReplicateRepositoryInvalidArguments(t *testing.T) {
diff --git a/internal/metadata/featureflag/feature_flags.go b/internal/metadata/featureflag/feature_flags.go
index c32da572c..716757aa4 100644
--- a/internal/metadata/featureflag/feature_flags.go
+++ b/internal/metadata/featureflag/feature_flags.go
@@ -12,10 +12,6 @@ var (
 	// ResolveConflictsWithHooks will cause the ResolveConflicts RPC to run Git hooks after committing changes
 	// to the branch.
 	ResolveConflictsWithHooks = FeatureFlag{Name: "resolve_conflicts_with_hooks", OnByDefault: true}
-	// ReplicateRepositoryDirectFetch will cause the ReplicateRepository RPC to perform fetches
-	// via a direct call instead of doing an RPC call to its own server. This fixes calls of
-	// `ReplicateRepository()` in case it's invoked via Praefect with transactions enabled.
-	ReplicateRepositoryDirectFetch = FeatureFlag{Name: "replicate_repository_direct_fetch", OnByDefault: false}
 	// FindAllTagsPipeline enables the alternative pipeline implementation for finding
 	// tags via FindAllTags.
 	FindAllTagsPipeline = FeatureFlag{Name: "find_all_tags_pipeline", OnByDefault: false}
@@ -37,7 +33,6 @@ var All = []FeatureFlag{
 	GoSetConfig,
 	CreateRepositoryFromBundleAtomicFetch,
 	ResolveConflictsWithHooks,
-	ReplicateRepositoryDirectFetch,
 	FindAllTagsPipeline,
 	TxRemoveRepository,
 	QuarantinedUserCreateTag,