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

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.