From c280fecb9847b77d9a70602f208747f4b38574a9 Mon Sep 17 00:00:00 2001 From: James Liu Date: Tue, 3 Jun 2025 09:38:21 +1000 Subject: [PATCH 1/3] ci: Add job to test reftables with WAL enabled We currently lack a Makefile target and associated CI job to execute tests with both the reftable backend in Git, and write-ahead logging in Gitaly enabled together. --- .gitlab-ci.yml | 4 +++- Makefile | 5 +++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 41ace8a383..e18cdcd105 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -317,7 +317,9 @@ test:reftable: <<: *test_definition variables: <<: *test_variables - TEST_TARGET: test-with-reftable + parallel: + matrix: + - TEST_TARGET: [test-with-reftable, test-with-reftable-wal] test:nightly: <<: *test_definition diff --git a/Makefile b/Makefile index 3c0494548b..eacc403276 100644 --- a/Makefile +++ b/Makefile @@ -447,6 +447,11 @@ bench: ${BENCHMARK_REPO} prepare-tests test-with-reftable: export GITALY_TEST_REF_FORMAT = reftable test-with-reftable: test-go +.PHONY: test-with-reftable-wal +## Run Go tests with git's reftable backend and write-ahead logging enabled. +test-with-reftable-wal: export GITALY_TEST_WAL = YesPlease +test-with-reftable-wal: test-with-reftable + .PHONY: test-with-sha256 ## Run Go tests with SHA256 repositories. test-with-sha256: export GITALY_TEST_WITH_SHA256 = YesPlease -- GitLab From b91aeb1ee49ced91bd8fcce182127e7126a9ff09 Mon Sep 17 00:00:00 2001 From: James Liu Date: Tue, 3 Jun 2025 15:06:36 +1000 Subject: [PATCH 2/3] test: Fix reftableMigrator goroutine leaks The reftableMigrator's Run() function kicks off a goroutine to listen for new migration requests. The function manages a WaitGroup which should be used in conjunction with the Close() function to ensure we wait for the goroutine to finish before exiting. When we set up the reftableMigrator in the middleware tests, we don't defer the Close() function. This causes a goroutine leak if any of the `require` assertions fail. Modify the tests so we defer the Close() function after checking if the reftableMigrator variable has been set. --- .../partition/migration/reftable/middleware_test.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/internal/gitaly/storage/storagemgr/partition/migration/reftable/middleware_test.go b/internal/gitaly/storage/storagemgr/partition/migration/reftable/middleware_test.go index 33cd2e53f3..c1500dc639 100644 --- a/internal/gitaly/storage/storagemgr/partition/migration/reftable/middleware_test.go +++ b/internal/gitaly/storage/storagemgr/partition/migration/reftable/middleware_test.go @@ -40,6 +40,11 @@ func testUnaryInterceptor(t *testing.T, ctx context.Context) { } var reftableMigrator *migrator + defer func() { + if reftableMigrator != nil { + reftableMigrator.Close() + } + }() callback := func(logger log.Logger, node storage.Node, factory localrepo.Factory) ([]grpc.UnaryServerInterceptor, []grpc.StreamServerInterceptor) { reftableMigrator = NewMigrator(logger, NewMetrics(), node, factory) reftableMigrator.Run() @@ -119,6 +124,11 @@ func testStreamInterceptor(t *testing.T, ctx context.Context) { } var reftableMigrator *migrator + defer func() { + if reftableMigrator != nil { + reftableMigrator.Close() + } + }() callback := func(logger log.Logger, node storage.Node, factory localrepo.Factory) ([]grpc.UnaryServerInterceptor, []grpc.StreamServerInterceptor) { reftableMigrator = NewMigrator(logger, NewMetrics(), node, factory) reftableMigrator.Run() -- GitLab From 2e82e2585a1f4f81bd82711146a24356515841af Mon Sep 17 00:00:00 2001 From: James Liu Date: Wed, 11 Jun 2025 14:25:30 +1000 Subject: [PATCH 3/3] test: Recreate reftable file When the TestCalculateChecksum/invalid_reference test executes with reftables enabled, it rewrites the reftables file by replacing the HEAD ref with an invalid one. When the WAL is also enabled, the file becomes read-only as it belongs to the canonical repository. To cicumvent the read-only permissions, delete the file first before calling os.WriteFile. --- .../gitaly/service/repository/calculate_checksum_test.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/internal/gitaly/service/repository/calculate_checksum_test.go b/internal/gitaly/service/repository/calculate_checksum_test.go index 9505687f3b..596c3f415d 100644 --- a/internal/gitaly/service/repository/calculate_checksum_test.go +++ b/internal/gitaly/service/repository/calculate_checksum_test.go @@ -186,10 +186,13 @@ func TestCalculateChecksum(t *testing.T) { headRefIdx := bytes.Index(reftableFileContent, []byte("HEAD")) nope := []byte("NOPE") - for i := 0; i < len(nope); i++ { + for i := range len(nope) { reftableFileContent[i+headRefIdx] = nope[i] } + // When the WAL is enabled, files in the canonical repository will have read-only permissions. + // We need to first remove the file and re-create it. + require.NoError(t, os.Remove(reftableFilePath)) require.NoError(t, os.WriteFile(reftableFilePath, reftableFileContent, os.ModePerm)) } else { commitID := gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch(git.DefaultBranch)) @@ -200,7 +203,7 @@ func TestCalculateChecksum(t *testing.T) { // repository from a corrupt repository given that both cases return the zero checksum. require.NoError(t, os.WriteFile( filepath.Join(repoPath, "packed-refs"), - []byte(fmt.Sprintf("# pack-refs with: peeled fully-peeled sorted\n%s refs/heads/broken:reference\n", commitID)), + fmt.Appendf(nil, "# pack-refs with: peeled fully-peeled sorted\n%s refs/heads/broken:reference\n", commitID), mode.File, )) } -- GitLab