Merge branch 'pks-repository-default-enable-direct-fetches' into 'master'

repository: Remove feature flag for direct fetches

Closes #3685

See merge request gitlab-org/gitaly!3716

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 c038e4c81..dfb7de795 100644
--- a/internal/metadata/featureflag/feature_flags.go
+++ b/internal/metadata/featureflag/feature_flags.go
@@ -9,10 +9,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}
@@ -33,7 +29,6 @@ var (
 var All = []FeatureFlag{
 	GoSetConfig,
 	ResolveConflictsWithHooks,
-	ReplicateRepositoryDirectFetch,
 	FindAllTagsPipeline,
 	TxRemoveRepository,
 	QuarantinedUserCreateTag,