Merge branch 'pks-praefect-remove-repository-locking-semantics' into 'master'

praefect: Support new locking semantics in RemoveRepository handler

See merge request gitlab-org/gitaly!4187

3 files changed, 41 insertions, 8 deletions

diff --git a/internal/gitaly/service/repository/remove_test.go b/internal/gitaly/service/repository/remove_test.go
index 4c18951c4..89ea92d39 100644
--- a/internal/gitaly/service/repository/remove_test.go
+++ b/internal/gitaly/service/repository/remove_test.go
@@ -3,9 +3,11 @@ package repository
 import (
 	"context"
 	"os"
+	"path/filepath"
 	"testing"
 
 	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper"
@@ -15,16 +17,31 @@ import (
 
 func TestRemoveRepository(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.AtomicRemoveRepository).Run(t, testRemoveRepository)
+	testhelper.NewFeatureSets(
+		featureflag.AtomicRemoveRepository,
+		featureflag.TxAtomicRepositoryCreation,
+	).Run(t, testRemoveRepository)
 }
 
 func testRemoveRepository(t *testing.T, ctx context.Context) {
-	_, repo, repoPath, client := setupRepositoryService(t)
+	cfg, client := setupRepositoryServiceWithoutRepo(t)
 
-	_, err := client.RemoveRepository(ctx, &gitalypb.RemoveRepositoryRequest{Repository: repo})
+	repo := &gitalypb.Repository{
+		StorageName:  cfg.Storages[0].Name,
+		RelativePath: gittest.NewRepositoryName(t, true),
+	}
+
+	_, err := client.CreateRepository(ctx, &gitalypb.CreateRepositoryRequest{
+		Repository: repo,
+	})
+	require.NoError(t, err)
+
+	_, err = client.RemoveRepository(ctx, &gitalypb.RemoveRepositoryRequest{
+		Repository: repo,
+	})
 	require.NoError(t, err)
 
-	require.NoFileExists(t, repoPath)
+	require.NoFileExists(t, filepath.Join(cfg.Storages[0].Path, repo.RelativePath))
 }
 
 func TestRemoveRepository_doesNotExist(t *testing.T) {
diff --git a/internal/praefect/remove_repository.go b/internal/praefect/remove_repository.go
index 233788a34..594d6fb71 100644
--- a/internal/praefect/remove_repository.go
+++ b/internal/praefect/remove_repository.go
@@ -9,6 +9,8 @@ import (
 
 	"github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus"
 	"github.com/sirupsen/logrus"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/helper"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/praefect/commonerr"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/praefect/datastore"
 	"gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb"
@@ -37,6 +39,10 @@ func RemoveRepositoryHandler(rs datastore.RepositoryStore, conns Connections) gr
 			// Gitaly doesn't return an error if the repository is not found, so Praefect follows the
 			// same protocol.
 			if errors.As(err, new(commonerr.RepositoryNotFoundError)) {
+				if featureflag.AtomicRemoveRepository.IsEnabled(ctx) {
+					return helper.ErrNotFoundf("repository does not exist")
+				}
+
 				return stream.SendMsg(&gitalypb.RemoveRepositoryResponse{})
 			}
 
diff --git a/internal/praefect/remove_repository_test.go b/internal/praefect/remove_repository_test.go
index 95067e6b5..0aec0c1f8 100644
--- a/internal/praefect/remove_repository_test.go
+++ b/internal/praefect/remove_repository_test.go
@@ -9,6 +9,8 @@ import (
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/service/setup"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/helper"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/praefect/config"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/praefect/datastore"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/praefect/datastore/glsql"
@@ -25,10 +27,20 @@ import (
 
 func TestRemoveRepositoryHandler(t *testing.T) {
 	t.Parallel()
+	testhelper.NewFeatureSets(featureflag.AtomicRemoveRepository).Run(t, testRemoveRepositoryHandler)
+}
+
+func testRemoveRepositoryHandler(t *testing.T, ctx context.Context) {
+	t.Parallel()
 
 	errServedByGitaly := status.Error(codes.Unknown, "request passed to Gitaly")
 	const virtualStorage, relativePath = "virtual-storage", "relative-path"
 
+	var errNotFound error
+	if featureflag.AtomicRemoveRepository.IsEnabled(ctx) {
+		errNotFound = helper.ErrNotFoundf("repository does not exist")
+	}
+
 	db := glsql.NewDB(t)
 	for _, tc := range []struct {
 		desc          string
@@ -44,6 +56,7 @@ func TestRemoveRepositoryHandler(t *testing.T) {
 		{
 			desc:       "repository not found",
 			repository: &gitalypb.Repository{StorageName: "virtual-storage", RelativePath: "doesn't exist"},
+			error:      errNotFound,
 		},
 		{
 			desc:        "repository found",
@@ -85,9 +98,6 @@ func TestRemoveRepositoryHandler(t *testing.T) {
 
 			rs := datastore.NewPostgresRepositoryStore(db, cfg.StorageNames())
 
-			ctx, cancel := testhelper.Context()
-			defer cancel()
-
 			require.NoError(t, rs.CreateRepository(ctx, 0, virtualStorage, relativePath, relativePath, gitaly1Storage, []string{gitaly2Storage, "non-existent-storage"}, nil, false, false))
 
 			tmp := testhelper.TempDir(t)
@@ -139,7 +149,7 @@ func TestRemoveRepositoryHandler(t *testing.T) {
 
 			resp, err := client.RemoveRepository(ctx, &gitalypb.RemoveRepositoryRequest{Repository: tc.repository})
 			if tc.error != nil {
-				require.Equal(t, tc.error, err)
+				testhelper.RequireGrpcError(t, tc.error, err)
 				assertExistence(t, gitaly1RepoPath)
 				assertExistence(t, gitaly2RepoPath)
 				return