Merge branch 'pks-feature-flag-go-lfs-pointers' into 'master'

blob: Remove feature flags for LFS pointer RPC ports

See merge request gitlab-org/gitaly!3309

7 files changed, 38 insertions, 161 deletions

diff --git a/changelogs/unreleased/pks-feature-flag-go-lfs-pointers.yml b/changelogs/unreleased/pks-feature-flag-go-lfs-pointers.yml
new file mode 100644
index 000000000..62d548b90
--- /dev/null
+++ b/changelogs/unreleased/pks-feature-flag-go-lfs-pointers.yml
@@ -0,0 +1,5 @@
+---
+title: 'blob: Remove feature flags for LFS pointer RPC ports'
+merge_request: 3309
+author:
+type: performance
diff --git a/internal/gitaly/service/blob/lfs_pointers.go b/internal/gitaly/service/blob/lfs_pointers.go
index f710f2295..f30594e1b 100644
--- a/internal/gitaly/service/blob/lfs_pointers.go
+++ b/internal/gitaly/service/blob/lfs_pointers.go
@@ -14,7 +14,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/git/catfile"
 	"gitlab.com/gitlab-org/gitaly/internal/git/localrepo"
-	"gitlab.com/gitlab-org/gitaly/internal/gitaly/rubyserver"
 	"gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
 	"google.golang.org/grpc/codes"
@@ -50,10 +49,6 @@ func (s *server) GetLFSPointers(req *gitalypb.GetLFSPointersRequest, stream gita
 		return status.Errorf(codes.InvalidArgument, "GetLFSPointers: %v", err)
 	}
 
-	if featureflag.IsDisabled(ctx, featureflag.GoGetLFSPointers) {
-		return s.rubyGetLFSPointers(req, stream)
-	}
-
 	repo := localrepo.New(s.gitCmdFactory, req.Repository, s.cfg)
 	objectIDs := strings.Join(req.BlobIds, "\n")
 
@@ -74,35 +69,6 @@ func (s *server) GetLFSPointers(req *gitalypb.GetLFSPointersRequest, stream gita
 	return nil
 }
 
-func (s *server) rubyGetLFSPointers(req *gitalypb.GetLFSPointersRequest, stream gitalypb.BlobService_GetLFSPointersServer) error {
-	ctx := stream.Context()
-
-	client, err := s.ruby.BlobServiceClient(ctx)
-	if err != nil {
-		return err
-	}
-
-	clientCtx, err := rubyserver.SetHeaders(ctx, s.locator, req.GetRepository())
-	if err != nil {
-		return err
-	}
-
-	rubyStream, err := client.GetLFSPointers(clientCtx, req)
-	if err != nil {
-		return err
-	}
-
-	return rubyserver.Proxy(func() error {
-		resp, err := rubyStream.Recv()
-		if err != nil {
-			md := rubyStream.Trailer()
-			stream.SetTrailer(md)
-			return err
-		}
-		return stream.Send(resp)
-	})
-}
-
 func validateGetLFSPointersRequest(req *gitalypb.GetLFSPointersRequest) error {
 	if req.GetRepository() == nil {
 		return gitaly_errors.ErrEmptyRepository
@@ -125,10 +91,6 @@ func (s *server) GetNewLFSPointers(in *gitalypb.GetNewLFSPointersRequest, stream
 		return status.Errorf(codes.InvalidArgument, "GetNewLFSPointers: %v", err)
 	}
 
-	if featureflag.IsDisabled(ctx, featureflag.GoGetNewLFSPointers) {
-		return s.rubyGetNewLFSPointers(in, stream)
-	}
-
 	repo := localrepo.New(s.gitCmdFactory, in.Repository, s.cfg)
 
 	var refs []string
@@ -161,35 +123,6 @@ func (s *server) GetNewLFSPointers(in *gitalypb.GetNewLFSPointersRequest, stream
 	return nil
 }
 
-func (s *server) rubyGetNewLFSPointers(in *gitalypb.GetNewLFSPointersRequest, stream gitalypb.BlobService_GetNewLFSPointersServer) error {
-	ctx := stream.Context()
-
-	client, err := s.ruby.BlobServiceClient(ctx)
-	if err != nil {
-		return err
-	}
-
-	clientCtx, err := rubyserver.SetHeaders(ctx, s.locator, in.GetRepository())
-	if err != nil {
-		return err
-	}
-
-	rubyStream, err := client.GetNewLFSPointers(clientCtx, in)
-	if err != nil {
-		return err
-	}
-
-	return rubyserver.Proxy(func() error {
-		resp, err := rubyStream.Recv()
-		if err != nil {
-			md := rubyStream.Trailer()
-			stream.SetTrailer(md)
-			return err
-		}
-		return stream.Send(resp)
-	})
-}
-
 func validateGetLfsPointersByRevisionRequest(in getLFSPointerByRevisionRequest) error {
 	if in.GetRepository() == nil {
 		return fmt.Errorf("empty Repository")
@@ -207,10 +140,6 @@ func (s *server) GetAllLFSPointers(in *gitalypb.GetAllLFSPointersRequest, stream
 		return status.Errorf(codes.InvalidArgument, "GetAllLFSPointers: %v", err)
 	}
 
-	if featureflag.IsDisabled(ctx, featureflag.GoGetAllLFSPointers) {
-		return s.rubyGetAllLFSPointers(in, stream)
-	}
-
 	repo := localrepo.New(s.gitCmdFactory, in.Repository, s.cfg)
 
 	lfsPointers, err := findLFSPointersByRevisions(ctx, repo, s.gitCmdFactory, 0, "--all")
@@ -233,35 +162,6 @@ func (s *server) GetAllLFSPointers(in *gitalypb.GetAllLFSPointersRequest, stream
 	return nil
 }
 
-func (s *server) rubyGetAllLFSPointers(in *gitalypb.GetAllLFSPointersRequest, stream gitalypb.BlobService_GetAllLFSPointersServer) error {
-	ctx := stream.Context()
-
-	client, err := s.ruby.BlobServiceClient(ctx)
-	if err != nil {
-		return err
-	}
-
-	clientCtx, err := rubyserver.SetHeaders(ctx, s.locator, in.GetRepository())
-	if err != nil {
-		return err
-	}
-
-	rubyStream, err := client.GetAllLFSPointers(clientCtx, in)
-	if err != nil {
-		return err
-	}
-
-	return rubyserver.Proxy(func() error {
-		resp, err := rubyStream.Recv()
-		if err != nil {
-			md := rubyStream.Trailer()
-			stream.SetTrailer(md)
-			return err
-		}
-		return stream.Send(resp)
-	})
-}
-
 func validateGetAllLFSPointersRequest(in *gitalypb.GetAllLFSPointersRequest) error {
 	if in.GetRepository() == nil {
 		return fmt.Errorf("empty Repository")
diff --git a/internal/gitaly/service/blob/lfs_pointers_test.go b/internal/gitaly/service/blob/lfs_pointers_test.go
index e2f616968..c51c214b3 100644
--- a/internal/gitaly/service/blob/lfs_pointers_test.go
+++ b/internal/gitaly/service/blob/lfs_pointers_test.go
@@ -14,8 +14,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/internal/git/localrepo"
-	"gitlab.com/gitlab-org/gitaly/internal/gitaly/config"
-	"gitlab.com/gitlab-org/gitaly/internal/gitaly/rubyserver"
 	"gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper/testcfg"
@@ -67,17 +65,19 @@ var (
 	}
 )
 
-func testSuccessfulGetLFSPointersRequest(t *testing.T, cfg config.Cfg, rubySrv *rubyserver.Server) {
+func TestSuccessfulGetLFSPointersRequest(t *testing.T) {
 	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.GoGetLFSPointers,
 		featureflag.LFSPointersUseBitmapIndex,
 	}).Run(t, func(t *testing.T, ctx context.Context) {
-		testSuccessfulGetLFSPointersRequestFeatured(t, ctx, cfg, rubySrv)
+		testSuccessfulGetLFSPointersRequestFeatured(t)
 	})
 }
 
-func testSuccessfulGetLFSPointersRequestFeatured(t *testing.T, ctx context.Context, cfg config.Cfg, rubySrv *rubyserver.Server) {
-	repo, _, client := setupWithRuby(t, cfg, rubySrv)
+func testSuccessfulGetLFSPointersRequestFeatured(t *testing.T) {
+	_, repo, _, client := setup(t)
+
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
 	lfsPointerIds := []string{
 		lfsPointer1,
@@ -120,7 +120,6 @@ func testSuccessfulGetLFSPointersRequestFeatured(t *testing.T, ctx context.Conte
 
 func TestFailedGetLFSPointersRequestDueToValidations(t *testing.T) {
 	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.GoGetLFSPointers,
 		featureflag.LFSPointersUseBitmapIndex,
 	}).Run(t, testFailedGetLFSPointersRequestDueToValidations)
 }
@@ -163,17 +162,19 @@ func testFailedGetLFSPointersRequestDueToValidations(t *testing.T, ctx context.C
 	}
 }
 
-func testSuccessfulGetNewLFSPointersRequest(t *testing.T, cfg config.Cfg, rubySrv *rubyserver.Server) {
+func TestSuccessfulGetNewLFSPointersRequest(t *testing.T) {
 	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.GoGetNewLFSPointers,
 		featureflag.LFSPointersUseBitmapIndex,
 	}).Run(t, func(t *testing.T, ctx context.Context) {
-		testSuccessfulGetNewLFSPointersRequestFeatured(t, ctx, cfg, rubySrv)
+		testSuccessfulGetNewLFSPointersRequestFeatured(t)
 	})
 }
 
-func testSuccessfulGetNewLFSPointersRequestFeatured(t *testing.T, ctx context.Context, cfg config.Cfg, rubySrv *rubyserver.Server) {
-	_, _, client := setupWithRuby(t, cfg, rubySrv)
+func testSuccessfulGetNewLFSPointersRequestFeatured(t *testing.T) {
+	cfg, _, _, client := setup(t)
+
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
 	repo, repoPath, cleanup := gittest.CloneRepoWithWorktreeAtStorage(t, cfg.Storages[0])
 	t.Cleanup(cleanup)
@@ -308,7 +309,6 @@ func testSuccessfulGetNewLFSPointersRequestFeatured(t *testing.T, ctx context.Co
 
 func TestFailedGetNewLFSPointersRequestDueToValidations(t *testing.T) {
 	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.GoGetNewLFSPointers,
 		featureflag.LFSPointersUseBitmapIndex,
 	}).Run(t, testFailedGetNewLFSPointersRequestDueToValidations)
 }
@@ -364,17 +364,19 @@ func drainNewPointers(c gitalypb.BlobService_GetNewLFSPointersClient) error {
 	}
 }
 
-func testSuccessfulGetAllLFSPointersRequest(t *testing.T, cfg config.Cfg, rubySrv *rubyserver.Server) {
+func TestSuccessfulGetAllLFSPointersRequest(t *testing.T) {
 	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.GoGetAllLFSPointers,
 		featureflag.LFSPointersUseBitmapIndex,
 	}).Run(t, func(t *testing.T, ctx context.Context) {
-		testSuccessfulGetAllLFSPointersRequestFeatured(t, ctx, cfg, rubySrv)
+		testSuccessfulGetAllLFSPointersRequestFeatured(t)
 	})
 }
 
-func testSuccessfulGetAllLFSPointersRequestFeatured(t *testing.T, ctx context.Context, cfg config.Cfg, rubySrv *rubyserver.Server) {
-	repo, _, client := setupWithRuby(t, cfg, rubySrv)
+func testSuccessfulGetAllLFSPointersRequestFeatured(t *testing.T) {
+	_, repo, _, client := setup(t)
+
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
 	request := &gitalypb.GetAllLFSPointersRequest{
 		Repository: repo,
@@ -412,7 +414,6 @@ func getAllPointers(t *testing.T, c gitalypb.BlobService_GetAllLFSPointersClient
 
 func TestFailedGetAllLFSPointersRequestDueToValidations(t *testing.T) {
 	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.GoGetAllLFSPointers,
 		featureflag.LFSPointersUseBitmapIndex,
 	}).Run(t, testFailedGetAllLFSPointersRequestDueToValidations)
 }
@@ -455,19 +456,21 @@ func drainAllPointers(c gitalypb.BlobService_GetAllLFSPointersClient) error {
 	}
 }
 
-func testGetAllLFSPointersVerifyScope(t *testing.T, cfg config.Cfg, rubySrv *rubyserver.Server) {
+func TestGetAllLFSPointersVerifyScope(t *testing.T) {
 	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.GoGetAllLFSPointers,
 		featureflag.LFSPointersUseBitmapIndex,
 	}).Run(t, func(t *testing.T, ctx context.Context) {
-		testGetAllLFSPointersVerifyScopeFeatured(t, ctx, cfg, rubySrv)
+		testGetAllLFSPointersVerifyScopeFeatured(t)
 	})
 }
 
-// TestGetAllLFSPointersVerifyScope verifies that this RPC returns all LFS
+// testGetAllLFSPointersVerifyScope verifies that this RPC returns all LFS
 // pointers in a repository, not only ones reachable from the default branch
-func testGetAllLFSPointersVerifyScopeFeatured(t *testing.T, ctx context.Context, cfg config.Cfg, rubySrv *rubyserver.Server) {
-	repo, repoPath, client := setupWithRuby(t, cfg, rubySrv)
+func testGetAllLFSPointersVerifyScopeFeatured(t *testing.T) {
+	_, repo, repoPath, client := setup(t)
+
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
 	request := &gitalypb.GetAllLFSPointersRequest{
 		Repository: repo,
diff --git a/internal/gitaly/service/blob/server.go b/internal/gitaly/service/blob/server.go
index 42b0809ab..4567d2ff2 100644
--- a/internal/gitaly/service/blob/server.go
+++ b/internal/gitaly/service/blob/server.go
@@ -3,22 +3,19 @@ package blob
 import (
 	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/gitaly/config"
-	"gitlab.com/gitlab-org/gitaly/internal/gitaly/rubyserver"
 	"gitlab.com/gitlab-org/gitaly/internal/storage"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
 )
 
 type server struct {
-	ruby          *rubyserver.Server
 	cfg           config.Cfg
 	locator       storage.Locator
 	gitCmdFactory git.CommandFactory
 }
 
 // NewServer creates a new instance of a grpc BlobServer
-func NewServer(rs *rubyserver.Server, cfg config.Cfg, locator storage.Locator, gitCmdFactory git.CommandFactory) gitalypb.BlobServiceServer {
+func NewServer(cfg config.Cfg, locator storage.Locator, gitCmdFactory git.CommandFactory) gitalypb.BlobServiceServer {
 	return &server{
-		ruby:          rs,
 		cfg:           cfg,
 		locator:       locator,
 		gitCmdFactory: gitCmdFactory,
diff --git a/internal/gitaly/service/blob/testhelper_test.go b/internal/gitaly/service/blob/testhelper_test.go
index e0c08f7ee..4dd6986a4 100644
--- a/internal/gitaly/service/blob/testhelper_test.go
+++ b/internal/gitaly/service/blob/testhelper_test.go
@@ -8,7 +8,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/internal/gitaly/config"
-	"gitlab.com/gitlab-org/gitaly/internal/gitaly/rubyserver"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper/testcfg"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
@@ -28,39 +27,21 @@ func testMain(m *testing.M) int {
 	return m.Run()
 }
 
-func TestWithRubySidecar(t *testing.T) {
-	cfg := testcfg.Build(t)
-
-	rubySrv := rubyserver.New(cfg)
-	require.NoError(t, rubySrv.Start())
-	t.Cleanup(rubySrv.Stop)
-
-	t.Run("testSuccessfulGetLFSPointersRequest", func(t *testing.T) { testSuccessfulGetLFSPointersRequest(t, cfg, rubySrv) })
-	t.Run("testSuccessfulGetAllLFSPointersRequest", func(t *testing.T) { testSuccessfulGetAllLFSPointersRequest(t, cfg, rubySrv) })
-	t.Run("testSuccessfulGetNewLFSPointersRequest", func(t *testing.T) { testSuccessfulGetNewLFSPointersRequest(t, cfg, rubySrv) })
-	t.Run("testGetAllLFSPointersVerifyScope", func(t *testing.T) { testGetAllLFSPointersVerifyScope(t, cfg, rubySrv) })
-}
-
 func setup(t *testing.T) (config.Cfg, *gitalypb.Repository, string, gitalypb.BlobServiceClient) {
 	cfg := testcfg.Build(t)
-	repo, repoPath, client := setupWithRuby(t, cfg, nil)
-
-	return cfg, repo, repoPath, client
-}
 
-func setupWithRuby(t *testing.T, cfg config.Cfg, rubySrv *rubyserver.Server) (*gitalypb.Repository, string, gitalypb.BlobServiceClient) {
 	repo, repoPath, cleanup := gittest.CloneRepoAtStorage(t, cfg.Storages[0], t.Name())
 	t.Cleanup(cleanup)
 
 	srv := testhelper.NewServer(t, nil, nil)
 	t.Cleanup(srv.Stop)
 
-	gitalypb.RegisterBlobServiceServer(srv.GrpcServer(), NewServer(rubySrv, cfg, config.NewLocator(cfg), git.NewExecCommandFactory(cfg)))
+	gitalypb.RegisterBlobServiceServer(srv.GrpcServer(), NewServer(cfg, config.NewLocator(cfg), git.NewExecCommandFactory(cfg)))
 	srv.Start(t)
 
 	conn, err := grpc.Dial("unix://"+srv.Socket(), grpc.WithInsecure())
 	require.NoError(t, err)
 	t.Cleanup(func() { conn.Close() })
 
-	return repo, repoPath, gitalypb.NewBlobServiceClient(conn)
+	return cfg, repo, repoPath, gitalypb.NewBlobServiceClient(conn)
 }
diff --git a/internal/gitaly/service/register.go b/internal/gitaly/service/register.go
index 89df6b041..7b9934bf2 100644
--- a/internal/gitaly/service/register.go
+++ b/internal/gitaly/service/register.go
@@ -71,7 +71,7 @@ func RegisterAll(
 	gitCmdFactory git.CommandFactory,
 	ling *linguist.Instance,
 ) {
-	gitalypb.RegisterBlobServiceServer(grpcServer, blob.NewServer(rubyServer, cfg, locator, gitCmdFactory))
+	gitalypb.RegisterBlobServiceServer(grpcServer, blob.NewServer(cfg, locator, gitCmdFactory))
 	gitalypb.RegisterCleanupServiceServer(grpcServer, cleanup.NewServer(cfg, gitCmdFactory))
 	gitalypb.RegisterCommitServiceServer(grpcServer, commit.NewServer(cfg, locator, gitCmdFactory, ling))
 	gitalypb.RegisterDiffServiceServer(grpcServer, diff.NewServer(cfg, locator, gitCmdFactory))
diff --git a/internal/metadata/featureflag/feature_flags.go b/internal/metadata/featureflag/feature_flags.go
index c18638e24..cee9a116a 100644
--- a/internal/metadata/featureflag/feature_flags.go
+++ b/internal/metadata/featureflag/feature_flags.go
@@ -25,12 +25,6 @@ var (
 	// GoUserRevert enables the Go implementation of UserRevert
 	GoUserRevert = FeatureFlag{Name: "go_user_revert", OnByDefault: false}
 	// GoGetAllLFSPointers enables the Go implementation of GetAllLFSPointers
-	GoGetAllLFSPointers = FeatureFlag{Name: "go_get_all_lfs_pointers", OnByDefault: true}
-	// GoGetLFSPointers enables the Go implementation of GetLFSPointers
-	GoGetLFSPointers = FeatureFlag{Name: "go_get_lfs_pointers", OnByDefault: true}
-	// GoGetNewPointers enables the Go implementation of GetNewLFSPointers
-	GoGetNewLFSPointers = FeatureFlag{Name: "go_get_new_lfs_pointers", OnByDefault: true}
-	// UploadPackGitalyHooks makes git-upload-pack use gitaly-hooks to run pack-objects
 	UploadPackGitalyHooks = FeatureFlag{Name: "upload_pack_gitaly_hooks", OnByDefault: false}
 	// LFSPointersUseBitmapIndex enables the use of bitmap indices when searching LFS pointers.
 	LFSPointersUseBitmapIndex = FeatureFlag{Name: "lfs_pointers_use_bitmap_index", OnByDefault: false}
@@ -47,9 +41,6 @@ var All = []FeatureFlag{
 	GoResolveConflicts,
 	GoUserUpdateSubmodule,
 	GoUserRevert,
-	GoGetAllLFSPointers,
-	GoGetLFSPointers,
-	GoGetNewLFSPointers,
 	LFSPointersUseBitmapIndex,
 	GoUpdateRemoteMirror,
 	UploadPackGitalyHooks,