Merge branch 'smh-transactional-create-repository' into 'master'

Handle repository creations transactionally

See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/6492

Merged-by: Justin Tobler <jtobler@gitlab.com>
Approved-by: Justin Tobler <jtobler@gitlab.com>
Approved-by: karthik nayak <knayak@gitlab.com>
Reviewed-by: Sami Hiltunen <shiltunen@gitlab.com>
Co-authored-by: Sami Hiltunen <shiltunen@gitlab.com>

7 files changed, 47 insertions, 28 deletions

diff --git a/internal/gitaly/service/repository/create_repository_from_bundle_test.go b/internal/gitaly/service/repository/create_repository_from_bundle_test.go
index 59692b3af..54c3e0579 100644
--- a/internal/gitaly/service/repository/create_repository_from_bundle_test.go
+++ b/internal/gitaly/service/repository/create_repository_from_bundle_test.go
@@ -11,7 +11,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git/localrepo"
-	"gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/config"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/transaction"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/grpc/metadata"
@@ -223,8 +222,13 @@ func TestCreateRepositoryFromBundle_transactional(t *testing.T) {
 		return transaction.PhasedVote{Vote: vote, Phase: phase}
 	}
 
-	createdRepoPath, err := config.NewLocator(cfg).GetRepoPath(gittest.RewrittenRepository(t, ctx, cfg, createdRepo))
-	require.NoError(t, err)
+	// This test is asserting storage layout as part of computing the expected vote hash. In particular,
+	// the references are not necessarily packed when the transaction is committed. Compute the expected
+	// vote thus from the state of the bundled repository. For the vote to match, we have to first modify
+	// the state to match what the RPC handler would produce. The created repository has its default branch
+	// set to master and has references packed.
+	gittest.Exec(t, cfg, "-C", repoPath, "symbolic-ref", "HEAD", "refs/heads/master")
+	gittest.Exec(t, cfg, "-C", repoPath, "pack-refs", "--all")
 
 	// Compute the vote hash to assert that we really hash exactly the files that we
 	// expect to hash. Furthermore, this is required for cross-platform compatibility given that
@@ -235,7 +239,7 @@ func TestCreateRepositoryFromBundle_transactional(t *testing.T) {
 		"config",
 		"packed-refs",
 	} {
-		file, err := os.Open(filepath.Join(createdRepoPath, filePath))
+		file, err := os.Open(filepath.Join(repoPath, filePath))
 		require.NoError(t, err)
 
 		_, err = io.Copy(hash, file)
diff --git a/internal/gitaly/service/repository/create_repository_from_snapshot_test.go b/internal/gitaly/service/repository/create_repository_from_snapshot_test.go
index fa0c6b310..a0dee4caa 100644
--- a/internal/gitaly/service/repository/create_repository_from_snapshot_test.go
+++ b/internal/gitaly/service/repository/create_repository_from_snapshot_test.go
@@ -70,6 +70,12 @@ func generateTarFile(t *testing.T, path string) ([]byte, []string) {
 }
 
 func TestCreateRepositoryFromSnapshot_success(t *testing.T) {
+	if testhelper.IsWALEnabled() && gittest.DefaultObjectHash.Format == git.ObjectHashSHA256.Format {
+		t.Skip(`
+CreateRepositoryFromSnapshot is broken with SHA256 but the test failures only surface with transactions
+enabled. For more details, see: https://gitlab.com/gitlab-org/gitaly/-/issues/5662`)
+	}
+
 	t.Parallel()
 
 	ctx := testhelper.Context(t)
@@ -104,6 +110,16 @@ func TestCreateRepositoryFromSnapshot_success(t *testing.T) {
 	repoAbsolutePath := filepath.Join(cfg.Storages[0].Path, gittest.GetReplicaPath(t, ctx, cfg, repo))
 	require.DirExists(t, repoAbsolutePath)
 	for _, entry := range entries {
+		if testhelper.IsWALEnabled() {
+			// The transaction manager repacks the objects from the snapshot into a single pack file.
+			// Skip asserting the exact objects on the disk and assert afterwards that the objects
+			// from the source repo exist in the target repo.
+			switch {
+			case strings.HasPrefix(entry, "./objects"):
+				continue
+			}
+		}
+
 		if strings.HasSuffix(entry, "/") {
 			require.DirExists(t, filepath.Join(repoAbsolutePath, entry), "directory %q not unpacked", entry)
 		} else {
@@ -113,6 +129,14 @@ func TestCreateRepositoryFromSnapshot_success(t *testing.T) {
 
 	// hooks/ and config were excluded, but the RPC should create them
 	require.FileExists(t, filepath.Join(repoAbsolutePath, "config"), "Config file not created")
+
+	targetPath, err := config.NewLocator(cfg).GetRepoPath(gittest.RewrittenRepository(t, ctx, cfg, repo))
+	require.NoError(t, err)
+
+	require.ElementsMatch(t,
+		gittest.ListObjects(t, cfg, sourceRepoPath),
+		gittest.ListObjects(t, cfg, targetPath),
+	)
 }
 
 func TestCreateRepositoryFromSnapshot_repositoryExists(t *testing.T) {
diff --git a/internal/gitaly/service/repository/create_repository_from_url_test.go b/internal/gitaly/service/repository/create_repository_from_url_test.go
index b7573f0da..02c71223b 100644
--- a/internal/gitaly/service/repository/create_repository_from_url_test.go
+++ b/internal/gitaly/service/repository/create_repository_from_url_test.go
@@ -106,9 +106,10 @@ func TestCreateRepositoryFromURL_existingTarget(t *testing.T) {
 	ctx := testhelper.Context(t)
 
 	testCases := []struct {
-		desc     string
-		repoPath string
-		isDir    bool
+		desc        string
+		repoPath    string
+		isDir       bool
+		skipWithWAL string
 	}{
 		{
 			desc:  "target is a directory",
@@ -117,11 +118,18 @@ func TestCreateRepositoryFromURL_existingTarget(t *testing.T) {
 		{
 			desc:  "target is a file",
 			isDir: false,
+			skipWithWAL: `
+The transaction commit fails as the TransactionManager fails to initialize with the state
+directory being a file. This is testing storage details rather than the RPC implementation
+testing of this scenario should be left to the relevant package.
+`,
 		},
 	}
 
 	for _, testCase := range testCases {
 		t.Run(testCase.desc, func(t *testing.T) {
+			testhelper.SkipWithWAL(t, testCase.skipWithWAL)
+
 			cfg, client := setupRepositoryService(t)
 
 			importedRepo := &gitalypb.Repository{
diff --git a/internal/gitaly/service/repository/fsck_test.go b/internal/gitaly/service/repository/fsck_test.go
index 93156fbaf..0d78d1327 100644
--- a/internal/gitaly/service/repository/fsck_test.go
+++ b/internal/gitaly/service/repository/fsck_test.go
@@ -1,7 +1,6 @@
 package repository
 
 import (
-	"fmt"
 	"os"
 	"path/filepath"
 	"strings"
@@ -14,8 +13,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v16/internal/structerr"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v16/proto/go/gitalypb"
-	"google.golang.org/grpc/codes"
-	"google.golang.org/grpc/status"
 )
 
 func TestFsck(t *testing.T) {
@@ -80,15 +77,6 @@ func TestFsck(t *testing.T) {
 				require.NoError(t, os.RemoveAll(filepath.Join(repoPath, "objects")))
 				require.NoError(t, os.WriteFile(filepath.Join(repoPath, "objects"), nil, perm.SharedFile))
 
-				if testhelper.IsWALEnabled() {
-					return setupData{
-						repo: repo,
-						expectedErr: status.Error(codes.Internal,
-							fmt.Sprintf("begin transaction: get partition: assign partition ID: get alternate partition ID: read alternates file: open: open %s/objects/info/alternates: not a directory", repoPath),
-						),
-					}
-				}
-
 				return setupData{
 					repo: repo,
 					requireResponse: func(actual *gitalypb.FsckResponse) {
diff --git a/internal/gitaly/service/smarthttp/receive_pack_test.go b/internal/gitaly/service/smarthttp/receive_pack_test.go
index 2e503ae70..f5ef91820 100644
--- a/internal/gitaly/service/smarthttp/receive_pack_test.go
+++ b/internal/gitaly/service/smarthttp/receive_pack_test.go
@@ -130,7 +130,7 @@ func TestPostReceivePack_successful(t *testing.T) {
 
 	var transactionID storage.TransactionID
 	if testhelper.IsWALEnabled() {
-		transactionID = 1
+		transactionID = 2
 	}
 
 	require.Equal(t, git.HooksPayload{
diff --git a/internal/gitaly/service/ssh/receive_pack_test.go b/internal/gitaly/service/ssh/receive_pack_test.go
index 83ddb0bb4..923b5d2e2 100644
--- a/internal/gitaly/service/ssh/receive_pack_test.go
+++ b/internal/gitaly/service/ssh/receive_pack_test.go
@@ -199,7 +199,7 @@ func TestReceivePack_success(t *testing.T) {
 
 	var transactionID storage.TransactionID
 	if testhelper.IsWALEnabled() {
-		transactionID = 2
+		transactionID = 4
 	}
 
 	require.Equal(t, git.HooksPayload{
diff --git a/internal/gitaly/storage/storagemgr/middleware.go b/internal/gitaly/storage/storagemgr/middleware.go
index 1ed793387..101376463 100644
--- a/internal/gitaly/storage/storagemgr/middleware.go
+++ b/internal/gitaly/storage/storagemgr/middleware.go
@@ -37,13 +37,8 @@ var nonTransactionalRPCs = map[string]struct{}{
 	"/gitaly.ObjectPoolService/GetObjectPool":              {},
 	// GetSnapshot is testing logic with object pools as well.
 	"/gitaly.RepositoryService/GetSnapshot": {},
-
-	// Repository creations are not yet handled through the WAL.
-	"/gitaly.RepositoryService/CreateRepository":             {},
-	"/gitaly.RepositoryService/CreateRepositoryFromURL":      {},
-	"/gitaly.RepositoryService/CreateRepositoryFromBundle":   {},
-	"/gitaly.RepositoryService/CreateFork":                   {},
-	"/gitaly.RepositoryService/CreateRepositoryFromSnapshot": {},
+	// CreateFork relies on object pools.
+	"/gitaly.RepositoryService/CreateFork": {},
 
 	// ReplicateRepository is replicating the attributes and config which the
 	// WAL won't support. This is pending removal of their replication.