diff options
author | Sami Hiltunen <shiltunen@gitlab.com> | 2022-04-07 13:54:25 +0300 |
---|---|---|
committer | Sami Hiltunen <shiltunen@gitlab.com> | 2022-04-07 13:54:25 +0300 |
commit | 7f7b3c14e36f799090baf48857dcd5ba019bbbc6 (patch) | |
tree | de19f25acb666d37b2b22d826d78391c1119fe83 /internal/gitaly | |
parent | 23efb30243f15dd8c29e222ba0552a21af9b1cbe (diff) | |
parent | f6d989353d8180abf2559c7212883980f8fad830 (diff) |
Merge branch 'smh-delete-object-pool-type' into 'master'
Handle DeleteObjectPool calls in Praefect
Closes #3742 and #4078
See merge request gitlab-org/gitaly!4395
Diffstat (limited to 'internal/gitaly')
-rw-r--r-- | internal/gitaly/service/objectpool/create.go | 20 | ||||
-rw-r--r-- | internal/gitaly/service/objectpool/create_test.go | 39 | ||||
-rw-r--r-- | internal/gitaly/service/objectpool/testhelper_test.go | 14 | ||||
-rw-r--r-- | internal/gitaly/service/repository/remove_test.go | 5 |
4 files changed, 56 insertions, 22 deletions
diff --git a/internal/gitaly/service/objectpool/create.go b/internal/gitaly/service/objectpool/create.go index 1e97f8c88..2dfd35f0a 100644 --- a/internal/gitaly/service/objectpool/create.go +++ b/internal/gitaly/service/objectpool/create.go @@ -57,18 +57,28 @@ func (s *server) DeleteObjectPool(ctx context.Context, in *gitalypb.DeleteObject return &gitalypb.DeleteObjectPoolResponse{}, nil } -type poolRequest interface { +// PoolRequest is the interface of a gRPC request that carries an object pool. +type PoolRequest interface { GetObjectPool() *gitalypb.ObjectPool } -func (s *server) poolForRequest(req poolRequest) (*objectpool.ObjectPool, error) { - reqPool := req.GetObjectPool() - - poolRepo := reqPool.GetRepository() +// ExtractPool returns the pool repository from the request or an error if the +// request did no contain a pool. +func ExtractPool(req PoolRequest) (*gitalypb.Repository, error) { + poolRepo := req.GetObjectPool().GetRepository() if poolRepo == nil { return nil, errMissingPool } + return poolRepo, nil +} + +func (s *server) poolForRequest(req PoolRequest) (*objectpool.ObjectPool, error) { + poolRepo, err := ExtractPool(req) + if err != nil { + return nil, err + } + pool, err := objectpool.NewObjectPool(s.locator, s.gitCmdFactory, s.catfileCache, s.txManager, s.housekeepingManager, poolRepo.GetStorageName(), poolRepo.GetRelativePath()) if err != nil { if err == objectpool.ErrInvalidPoolDir { diff --git a/internal/gitaly/service/objectpool/create_test.go b/internal/gitaly/service/objectpool/create_test.go index f7dc13371..c8b77063c 100644 --- a/internal/gitaly/service/objectpool/create_test.go +++ b/internal/gitaly/service/objectpool/create_test.go @@ -1,7 +1,6 @@ package objectpool import ( - "fmt" "path/filepath" "strings" "testing" @@ -10,8 +9,6 @@ import ( "github.com/stretchr/testify/require" "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" - "gitlab.com/gitlab-org/gitaly/v14/internal/praefect/commonerr" "gitlab.com/gitlab-org/gitaly/v14/internal/testhelper" "gitlab.com/gitlab-org/gitaly/v14/internal/testhelper/testserver" "gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb" @@ -143,6 +140,8 @@ func TestDelete(t *testing.T) { cfg, repoProto, _, _, client := setup(ctx, t) repo := localrepo.NewTestRepo(t, cfg, repoProto) + repositoryClient := gitalypb.NewRepositoryServiceClient(extractConn(client)) + pool := initObjectPool(t, cfg, cfg.Storages[0]) _, err := client.CreateObjectPool(ctx, &gitalypb.CreateObjectPoolRequest{ ObjectPool: pool.ToProto(), @@ -154,10 +153,21 @@ func TestDelete(t *testing.T) { for _, tc := range []struct { desc string + noPool bool relativePath string error error }{ { + desc: "no pool in request fails", + noPool: true, + error: errMissingPool, + }, + { + desc: "deleting outside pools directory fails", + relativePath: ".", + error: errInvalidPoolDir, + }, + { desc: "deleting outside pools directory fails", relativePath: ".", error: errInvalidPoolDir, @@ -197,22 +207,25 @@ func TestDelete(t *testing.T) { }, } { t.Run(tc.desc, func(t *testing.T) { - _, err := client.DeleteObjectPool(ctx, &gitalypb.DeleteObjectPoolRequest{ObjectPool: &gitalypb.ObjectPool{ + request := &gitalypb.DeleteObjectPoolRequest{ObjectPool: &gitalypb.ObjectPool{ Repository: &gitalypb.Repository{ StorageName: repo.GetStorageName(), RelativePath: tc.relativePath, }, - }}) - - expectedErr := tc.error - if tc.error == errInvalidPoolDir && testhelper.IsPraefectEnabled() { - expectedErr = helper.ErrNotFound(fmt.Errorf( - "mutator call: route repository mutator: get repository id: %w", - commonerr.NewRepositoryNotFoundError(repo.GetStorageName(), tc.relativePath), - )) + }} + + if tc.noPool { + request.ObjectPool = nil } - testhelper.RequireGrpcError(t, expectedErr, err) + _, err := client.DeleteObjectPool(ctx, request) + testhelper.RequireGrpcError(t, tc.error, err) + + response, err := repositoryClient.RepositoryExists(ctx, &gitalypb.RepositoryExistsRequest{ + Repository: pool.ToProto().GetRepository(), + }) + require.NoError(t, err) + require.Equal(t, tc.error != nil, response.GetExists()) }) } } diff --git a/internal/gitaly/service/objectpool/testhelper_test.go b/internal/gitaly/service/objectpool/testhelper_test.go index f0ba73dd8..3ea7f981c 100644 --- a/internal/gitaly/service/objectpool/testhelper_test.go +++ b/internal/gitaly/service/objectpool/testhelper_test.go @@ -30,6 +30,18 @@ func TestMain(m *testing.M) { testhelper.Run(m) } +// clientWithConn allows for passing through the ClientConn to tests which need +// to access other services than ObjectPoolService. +type clientWithConn struct { + gitalypb.ObjectPoolServiceClient + conn *grpc.ClientConn +} + +// extractConn returns the underlying ClientConn from the client. +func extractConn(client gitalypb.ObjectPoolServiceClient) *grpc.ClientConn { + return client.(clientWithConn).conn +} + func setup(ctx context.Context, t *testing.T, opts ...testserver.GitalyServerOpt) (config.Cfg, *gitalypb.Repository, string, storage.Locator, gitalypb.ObjectPoolServiceClient) { t.Helper() @@ -48,7 +60,7 @@ func setup(ctx context.Context, t *testing.T, opts ...testserver.GitalyServerOpt Seed: gittest.SeedGitLabTest, }) - return cfg, repo, repoPath, locator, gitalypb.NewObjectPoolServiceClient(conn) + return cfg, repo, repoPath, locator, clientWithConn{ObjectPoolServiceClient: gitalypb.NewObjectPoolServiceClient(conn), conn: conn} } func runObjectPoolServer(t *testing.T, cfg config.Cfg, locator storage.Locator, logger *logrus.Logger, opts ...testserver.GitalyServerOpt) string { diff --git a/internal/gitaly/service/repository/remove_test.go b/internal/gitaly/service/repository/remove_test.go index 962b6e0b8..b6c922ecb 100644 --- a/internal/gitaly/service/repository/remove_test.go +++ b/internal/gitaly/service/repository/remove_test.go @@ -41,8 +41,7 @@ func TestRemoveRepository_doesNotExist(t *testing.T) { t.Parallel() ctx := testhelper.Context(t) - // Praefect special-cases repository removals, so we disable Praefect here. - cfg, client := setupRepositoryServiceWithoutRepo(t, testserver.WithDisablePraefect()) + cfg, client := setupRepositoryServiceWithoutRepo(t) _, err := client.RemoveRepository(ctx, &gitalypb.RemoveRepositoryRequest{ Repository: &gitalypb.Repository{StorageName: cfg.Storages[0].Name, RelativePath: "/does/not/exist"}, @@ -54,7 +53,7 @@ func TestRemoveRepository_locking(t *testing.T) { t.Parallel() ctx := testhelper.Context(t) - // Praefect special-cases repository removals, so we disable Praefect here. + // Praefect does not acquire a lock on repository deletion so disable the test case for Praefect. _, repo, repoPath, client := setupRepositoryService(ctx, t, testserver.WithDisablePraefect()) // Simulate a concurrent RPC holding the repository lock. |