diff options
author | Pavlo Strokov <pstrokov@gitlab.com> | 2021-03-30 09:33:22 +0300 |
---|---|---|
committer | Pavlo Strokov <pstrokov@gitlab.com> | 2021-03-30 09:33:22 +0300 |
commit | 24415e4973647f1b9be3b2cff0ebde4439917484 (patch) | |
tree | fbfc7587481fb5c1379b3cd8ea5d504c93056d5d | |
parent | fdca65c5e15c1f0c695f8d4d217841f196121596 (diff) | |
parent | e246efa00fae54ffa943f7b5b0ae2cb87273f91c (diff) |
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
-rw-r--r-- | changelogs/unreleased/pks-feature-flag-go-lfs-pointers.yml | 5 | ||||
-rw-r--r-- | internal/gitaly/service/blob/lfs_pointers.go | 100 | ||||
-rw-r--r-- | internal/gitaly/service/blob/lfs_pointers_test.go | 55 | ||||
-rw-r--r-- | internal/gitaly/service/blob/server.go | 5 | ||||
-rw-r--r-- | internal/gitaly/service/blob/testhelper_test.go | 23 | ||||
-rw-r--r-- | internal/gitaly/service/register.go | 2 | ||||
-rw-r--r-- | internal/metadata/featureflag/feature_flags.go | 9 |
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, |