diff options
author | Toon Claes <toon@gitlab.com> | 2021-10-08 17:08:28 +0300 |
---|---|---|
committer | Toon Claes <toon@gitlab.com> | 2021-10-08 17:08:28 +0300 |
commit | 2df83f8447aa8b77c4a3545260c2a62981a57907 (patch) | |
tree | 595bcad809c0702f7b10a7c5970dd764c07ca087 | |
parent | 6c860732e786ceb4cab715518a02b87a814e871a (diff) | |
parent | b8fd37283c975f5e31be08fb4451c6bdddde4273 (diff) |
Merge branch 'pks-supervisor-synchronize-kill' into 'master'
supervisor: Close writers before signalling process is done
See merge request gitlab-org/gitaly!3940
20 files changed, 204 insertions, 237 deletions
diff --git a/cmd/gitaly-hooks/hooks_test.go b/cmd/gitaly-hooks/hooks_test.go index 811c6f6d8..fe44c7ae8 100644 --- a/cmd/gitaly-hooks/hooks_test.go +++ b/cmd/gitaly-hooks/hooks_test.go @@ -161,6 +161,11 @@ func testHooksPrePostReceive(t *testing.T, cfg config.Cfg, repo *gitalypb.Reposi cfg.Gitlab.HTTPSettings.User = gitlabUser cfg.Gitlab.HTTPSettings.Password = gitlabPassword + gitlabClient, err := gitlab.NewHTTPClient(logger.Logger(), cfg.Gitlab, cfg.TLS, prometheus.Config{}) + require.NoError(t, err) + + runHookServiceWithGitlabClient(t, cfg, gitlabClient) + gitObjectDirRegex := regexp.MustCompile(`(?m)^GIT_OBJECT_DIRECTORY=(.*)$`) gitAlternateObjectDirRegex := regexp.MustCompile(`(?m)^GIT_ALTERNATE_OBJECT_DIRECTORIES=(.*)$`) @@ -170,11 +175,6 @@ func testHooksPrePostReceive(t *testing.T, cfg config.Cfg, repo *gitalypb.Reposi t.Run(fmt.Sprintf("hookName: %s", hookName), func(t *testing.T) { customHookOutputPath := gittest.WriteEnvToCustomHook(t, repoPath, hookName) - gitlabClient, err := gitlab.NewHTTPClient(logger.Logger(), cfg.Gitlab, cfg.TLS, prometheus.Config{}) - require.NoError(t, err) - - runHookServiceWithGitlabClient(t, cfg, gitlabClient) - var stderr, stdout bytes.Buffer stdin := bytes.NewBuffer([]byte(changes)) hookPath, err := filepath.Abs(fmt.Sprintf("../../ruby/git-hooks/%s", hookName)) @@ -353,6 +353,8 @@ func TestHooksPostReceiveFailed(t *testing.T) { gitlabClient, err := gitlab.NewHTTPClient(logger.Logger(), cfg.Gitlab, cfg.TLS, prometheus.Config{}) require.NoError(t, err) + runHookServiceWithGitlabClient(t, cfg, gitlabClient) + customHookOutputPath := gittest.WriteEnvToCustomHook(t, repoPath, "post-receive") var stdout, stderr bytes.Buffer @@ -395,8 +397,6 @@ func TestHooksPostReceiveFailed(t *testing.T) { for _, tc := range testcases { t.Run(tc.desc, func(t *testing.T) { - runHookServiceWithGitlabClient(t, cfg, gitlabClient) - hooksPayload, err := git.NewHooksPayload( cfg, repo, @@ -661,6 +661,9 @@ func TestGitalyHooksPackObjects(t *testing.T) { Logging: config.Logging{Config: internallog.Config{Dir: logDir}}, })) + logger, hook := test.NewNullLogger() + runHookServiceServer(t, cfg, testserver.WithLogger(logger)) + testhelper.BuildGitalyHooks(t, cfg) testhelper.BuildGitalySSH(t, cfg) @@ -692,14 +695,13 @@ func TestGitalyHooksPackObjects(t *testing.T) { for _, tc := range testCases { t.Run(tc.desc, func(t *testing.T) { + hook.Reset() + ctx := context.Background() if tc.ctx != nil { ctx = tc.ctx } - logger, hook := test.NewNullLogger() - runHookServiceServer(t, cfg, testserver.WithLogger(logger)) - tempDir := testhelper.TempDir(t) args := append(baseArgs[1:], tc.extraArgs...) diff --git a/internal/gitaly/service/hook/pack_objects_test.go b/internal/gitaly/service/hook/pack_objects_test.go index fc2c81cfc..a2d1aed21 100644 --- a/internal/gitaly/service/hook/pack_objects_test.go +++ b/internal/gitaly/service/hook/pack_objects_test.go @@ -59,10 +59,7 @@ func cfgWithCache(t *testing.T) (config.Cfg, *gitalypb.Repository, string) { } func TestServer_PackObjectsHook(t *testing.T) { - ctx, cancel := testhelper.Context() - defer cancel() - - cfg, repo, repoPath := cfgWithCache(t) + t.Parallel() testCases := []struct { desc string @@ -83,6 +80,11 @@ func TestServer_PackObjectsHook(t *testing.T) { for _, tc := range testCases { t.Run(tc.desc, func(t *testing.T) { + ctx, cancel := testhelper.Context() + defer cancel() + + cfg, repo, repoPath := cfgWithCache(t) + logger, hook := test.NewNullLogger() serverSocketPath := runHooksServer(t, cfg, nil, testserver.WithLogger(logger)) @@ -308,7 +310,7 @@ func TestServer_PackObjectsHook_usesCache(t *testing.T) { } func TestServer_PackObjectsHookWithSidechannel(t *testing.T) { - cfg, repo, repoPath := cfgWithCache(t) + t.Parallel() testCases := []struct { desc string @@ -329,6 +331,8 @@ func TestServer_PackObjectsHookWithSidechannel(t *testing.T) { for _, tc := range testCases { t.Run(tc.desc, func(t *testing.T) { + cfg, repo, repoPath := cfgWithCache(t) + ctx, cancel := testhelper.Context() defer cancel() diff --git a/internal/gitaly/service/hook/post_receive_test.go b/internal/gitaly/service/hook/post_receive_test.go index 9101585c0..5d22e488e 100644 --- a/internal/gitaly/service/hook/post_receive_test.go +++ b/internal/gitaly/service/hook/post_receive_test.go @@ -37,39 +37,12 @@ func TestPostReceiveInvalidArgument(t *testing.T) { } func TestHooksMissingStdin(t *testing.T) { + t.Parallel() + user, password, secretToken := "user", "password", "secret token" tempDir := testhelper.TempDir(t) gitlab.WriteShellSecretFile(t, tempDir, secretToken) - cfg, repo, repoPath := testcfg.BuildWithRepo(t) - - c := gitlab.TestServerOptions{ - User: user, - Password: password, - SecretToken: secretToken, - GLID: "key_id", - GLRepository: repo.GetGlRepository(), - Changes: "changes", - PostReceiveCounterDecreased: true, - Protocol: "protocol", - RepoPath: repoPath, - } - - serverURL, cleanup := gitlab.NewTestServer(t, c) - defer cleanup() - - cfg.Gitlab = config.Gitlab{ - SecretFile: filepath.Join(tempDir, ".gitlab_shell_secret"), - URL: serverURL, - HTTPSettings: config.HTTPSettings{ - User: user, - Password: password, - }, - } - - gitlabClient, err := gitlab.NewHTTPClient(testhelper.NewTestLogger(t), cfg.Gitlab, cfg.TLS, prometheus.Config{}) - require.NoError(t, err) - testCases := []struct { desc string primary bool @@ -88,6 +61,35 @@ func TestHooksMissingStdin(t *testing.T) { for _, tc := range testCases { t.Run(tc.desc, func(t *testing.T) { + cfg, repo, repoPath := testcfg.BuildWithRepo(t) + + c := gitlab.TestServerOptions{ + User: user, + Password: password, + SecretToken: secretToken, + GLID: "key_id", + GLRepository: repo.GetGlRepository(), + Changes: "changes", + PostReceiveCounterDecreased: true, + Protocol: "protocol", + RepoPath: repoPath, + } + + serverURL, cleanup := gitlab.NewTestServer(t, c) + defer cleanup() + + cfg.Gitlab = config.Gitlab{ + SecretFile: filepath.Join(tempDir, ".gitlab_shell_secret"), + URL: serverURL, + HTTPSettings: config.HTTPSettings{ + User: user, + Password: password, + }, + } + + gitlabClient, err := gitlab.NewHTTPClient(testhelper.NewTestLogger(t), cfg.Gitlab, cfg.TLS, prometheus.Config{}) + require.NoError(t, err) + serverSocketPath := runHooksServer(t, cfg, nil, testserver.WithGitLabClient(gitlabClient)) client, conn := newHooksClient(t, serverSocketPath) @@ -152,6 +154,8 @@ func TestHooksMissingStdin(t *testing.T) { } func TestPostReceiveMessages(t *testing.T) { + t.Parallel() + testCases := []struct { desc string basicMessages, alertMessages []string @@ -178,16 +182,16 @@ To create a merge request for okay, visit: }, } - cfg, repo, repoPath := testcfg.BuildWithRepo(t) - secretToken := "secret token" user, password := "user", "password" - tempDir := testhelper.TempDir(t) - gitlab.WriteShellSecretFile(t, tempDir, secretToken) - for _, tc := range testCases { t.Run(tc.desc, func(t *testing.T) { + cfg, repo, repoPath := testcfg.BuildWithRepo(t) + + tempDir := testhelper.TempDir(t) + gitlab.WriteShellSecretFile(t, tempDir, secretToken) + c := gitlab.TestServerOptions{ User: user, Password: password, diff --git a/internal/gitaly/service/hook/pre_receive_test.go b/internal/gitaly/service/hook/pre_receive_test.go index b3ea0a829..b4a23160e 100644 --- a/internal/gitaly/service/hook/pre_receive_test.go +++ b/internal/gitaly/service/hook/pre_receive_test.go @@ -201,7 +201,7 @@ func allowedHandler(t *testing.T, allowed bool) http.HandlerFunc { } func TestPreReceive_APIErrors(t *testing.T) { - cfg, repo, _ := testcfg.BuildWithRepo(t) + t.Parallel() testCases := []struct { desc string @@ -234,12 +234,14 @@ func TestPreReceive_APIErrors(t *testing.T) { }, } - tmpDir := testhelper.TempDir(t) - secretFilePath := filepath.Join(tmpDir, ".gitlab_shell_secret") - gitlab.WriteShellSecretFile(t, tmpDir, "token") - for _, tc := range testCases { t.Run(tc.desc, func(t *testing.T) { + cfg, repo, _ := testcfg.BuildWithRepo(t) + + tmpDir := testhelper.TempDir(t) + secretFilePath := filepath.Join(tmpDir, ".gitlab_shell_secret") + gitlab.WriteShellSecretFile(t, tmpDir, "token") + mux := http.NewServeMux() mux.Handle("/api/v4/internal/allowed", tc.allowedHandler) mux.Handle("/api/v4/internal/pre_receive", tc.preReceiveHandler) @@ -368,11 +370,10 @@ exit %d } func TestPreReceiveHook_Primary(t *testing.T) { - cfg := testcfg.Build(t) + t.Parallel() cwd, err := os.Getwd() require.NoError(t, err) - cfg.Ruby.Dir = filepath.Join(cwd, "testdata") testCases := []struct { desc string @@ -431,6 +432,9 @@ func TestPreReceiveHook_Primary(t *testing.T) { for _, tc := range testCases { t.Run(tc.desc, func(t *testing.T) { + cfg := testcfg.Build(t) + cfg.Ruby.Dir = filepath.Join(cwd, "testdata") + testRepo, testRepoPath := gittest.CloneRepo(t, cfg, cfg.Storages[0]) mux := http.NewServeMux() diff --git a/internal/gitaly/service/ref/testhelper_test.go b/internal/gitaly/service/ref/testhelper_test.go index f530ca28a..e933ecd7c 100644 --- a/internal/gitaly/service/ref/testhelper_test.go +++ b/internal/gitaly/service/ref/testhelper_test.go @@ -35,11 +35,7 @@ func TestMain(m *testing.M) { func setupRefService(t testing.TB) (config.Cfg, *gitalypb.Repository, string, gitalypb.RefServiceClient) { cfg, client := setupRefServiceWithoutRepo(t) - repo, repoPath := gittest.CloneRepo(t, cfg, cfg.Storages[0]) - - testhelper.BuildGitalyHooks(t, cfg) - return cfg, repo, repoPath, client } diff --git a/internal/gitaly/service/remote/update_remote_mirror_test.go b/internal/gitaly/service/remote/update_remote_mirror_test.go index 7eeb8128f..5b726cf55 100644 --- a/internal/gitaly/service/remote/update_remote_mirror_test.go +++ b/internal/gitaly/service/remote/update_remote_mirror_test.go @@ -38,10 +38,6 @@ func (w commandFactoryWrapper) New(ctx context.Context, repo repository.GitRepo, func TestUpdateRemoteMirror(t *testing.T) { t.Parallel() - cfg := testcfg.Build(t) - - testhelper.BuildGitalyGit2Go(t, cfg) - type refs map[string][]string for _, tc := range []struct { @@ -500,6 +496,10 @@ func TestUpdateRemoteMirror(t *testing.T) { ctx, cancel := testhelper.Context() defer cancel() + cfg := testcfg.Build(t) + + testhelper.BuildGitalyGit2Go(t, cfg) + mirrorRepoPb, mirrorRepoPath := gittest.InitRepo(t, cfg, cfg.Storages[0]) sourceRepoPb, sourceRepoPath := gittest.InitRepo(t, cfg, cfg.Storages[0]) diff --git a/internal/gitaly/service/repository/create_from_snapshot_test.go b/internal/gitaly/service/repository/create_from_snapshot_test.go index 7a446c2ae..5aa0c339b 100644 --- a/internal/gitaly/service/repository/create_from_snapshot_test.go +++ b/internal/gitaly/service/repository/create_from_snapshot_test.go @@ -148,9 +148,6 @@ func TestCreateRepositoryFromSnapshotFailsIfBadURL(t *testing.T) { func TestCreateRepositoryFromSnapshotBadRequests(t *testing.T) { t.Parallel() - cfg := testcfg.Build(t) - repo, repoPath := gittest.CloneRepo(t, cfg, cfg.Storages[0]) - require.NoError(t, os.RemoveAll(repoPath)) testCases := []struct { desc string @@ -187,6 +184,10 @@ func TestCreateRepositoryFromSnapshotBadRequests(t *testing.T) { for _, tc := range testCases { t.Run(tc.desc, func(t *testing.T) { + cfg := testcfg.Build(t) + repo, repoPath := gittest.CloneRepo(t, cfg, cfg.Storages[0]) + require.NoError(t, os.RemoveAll(repoPath)) + req := &gitalypb.CreateRepositoryFromSnapshotRequest{ Repository: repo, HttpUrl: srv.URL + tc.url, diff --git a/internal/gitaly/service/repository/license_test.go b/internal/gitaly/service/repository/license_test.go index 70701475a..e784b9cb5 100644 --- a/internal/gitaly/service/repository/license_test.go +++ b/internal/gitaly/service/repository/license_test.go @@ -3,7 +3,6 @@ package repository import ( "context" "os" - "path/filepath" "testing" "github.com/stretchr/testify/require" @@ -16,7 +15,7 @@ import ( "gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb" ) -func testSuccessfulFindLicenseRequest(t *testing.T, cfg config.Cfg, rubySrv *rubyserver.Server) { +func testSuccessfulFindLicenseRequest(t *testing.T, cfg config.Cfg, client gitalypb.RepositoryServiceClient, rubySrv *rubyserver.Server) { testhelper.NewFeatureSets([]featureflag.FeatureFlag{ featureflag.GoFindLicense, }).Run(t, func(t *testing.T, ctx context.Context) { @@ -75,8 +74,6 @@ SOFTWARE.`, }, } { t.Run(tc.desc, func(t *testing.T) { - client, _ := runRepositoryService(t, cfg, rubySrv) - repo, repoPath := gittest.InitRepo(t, cfg, cfg.Storages[0]) var treeEntries []gittest.TreeEntry @@ -110,23 +107,17 @@ SOFTWARE.`, }) } -func testFindLicenseRequestEmptyRepo(t *testing.T, cfg config.Cfg, rubySrv *rubyserver.Server) { +func testFindLicenseRequestEmptyRepo(t *testing.T, cfg config.Cfg, client gitalypb.RepositoryServiceClient, rubySrv *rubyserver.Server) { testhelper.NewFeatureSets([]featureflag.FeatureFlag{ featureflag.GoFindLicense, }).Run(t, func(t *testing.T, ctx context.Context) { - cfg, _, _, client := setupRepositoryServiceWithRuby(t, cfg, rubySrv) - - emptyRepo := &gitalypb.Repository{ - RelativePath: "test-liceense-empty-repo.git", - StorageName: cfg.Storages[0].Name, - } - emptyRepoPath := filepath.Join(cfg.Storages[0].Path, emptyRepo.GetRelativePath()) - defer require.NoError(t, os.RemoveAll(emptyRepoPath)) + repo, repoPath := gittest.InitRepo(t, cfg, cfg.Storages[0]) + require.NoError(t, os.RemoveAll(repoPath)) - _, err := client.CreateRepository(ctx, &gitalypb.CreateRepositoryRequest{Repository: emptyRepo}) + _, err := client.CreateRepository(ctx, &gitalypb.CreateRepositoryRequest{Repository: repo}) require.NoError(t, err) - resp, err := client.FindLicense(ctx, &gitalypb.FindLicenseRequest{Repository: emptyRepo}) + resp, err := client.FindLicense(ctx, &gitalypb.FindLicenseRequest{Repository: repo}) require.NoError(t, err) require.Empty(t, resp.GetLicenseShortName()) diff --git a/internal/gitaly/service/repository/testhelper_test.go b/internal/gitaly/service/repository/testhelper_test.go index 1e33f5ff9..bfcf64e8c 100644 --- a/internal/gitaly/service/repository/testhelper_test.go +++ b/internal/gitaly/service/repository/testhelper_test.go @@ -42,19 +42,23 @@ func TestWithRubySidecar(t *testing.T) { t.Parallel() cfg := testcfg.Build(t) - testhelper.BuildGitalyHooks(t, cfg) - rubySrv := rubyserver.New(cfg) require.NoError(t, rubySrv.Start()) t.Cleanup(rubySrv.Stop) - fs := []func(t *testing.T, cfg config.Cfg, rubySrv *rubyserver.Server){ + client, serverSocketPath := runRepositoryService(t, cfg, rubySrv) + cfg.SocketPath = serverSocketPath + + testhelper.BuildGitalyHooks(t, cfg) + testhelper.BuildGitalyGit2Go(t, cfg) + + fs := []func(t *testing.T, cfg config.Cfg, client gitalypb.RepositoryServiceClient, rubySrv *rubyserver.Server){ testSuccessfulFindLicenseRequest, testFindLicenseRequestEmptyRepo, } for _, f := range fs { t.Run(runtime.FuncForPC(reflect.ValueOf(f).Pointer()).Name(), func(t *testing.T) { - f(t, cfg, rubySrv) + f(t, cfg, client, rubySrv) }) } } @@ -80,16 +84,6 @@ func newMuxedRepositoryClient(t *testing.T, ctx context.Context, cfg config.Cfg, return gitalypb.NewRepositoryServiceClient(conn) } -func setupRepositoryServiceWithRuby(t testing.TB, cfg config.Cfg, rubySrv *rubyserver.Server, opts ...testserver.GitalyServerOpt) (config.Cfg, *gitalypb.Repository, string, gitalypb.RepositoryServiceClient) { - client, serverSocketPath := runRepositoryService(t, cfg, rubySrv, opts...) - testhelper.BuildGitalyGit2Go(t, cfg) - cfg.SocketPath = serverSocketPath - - repo, repoPath := gittest.CloneRepo(t, cfg, cfg.Storages[0]) - - return cfg, repo, repoPath, client -} - func assertModTimeAfter(t *testing.T, afterTime time.Time, paths ...string) bool { t.Helper() // NOTE: Since some filesystems don't have sub-second precision on `mtime` diff --git a/internal/gitaly/service/smarthttp/inforefs_test.go b/internal/gitaly/service/smarthttp/inforefs_test.go index d2ccaeb37..58f3ff36c 100644 --- a/internal/gitaly/service/smarthttp/inforefs_test.go +++ b/internal/gitaly/service/smarthttp/inforefs_test.go @@ -286,7 +286,7 @@ type mockStreamer struct { putStream func(context.Context, *gitalypb.Repository, proto.Message, io.Reader) error } -func (ms mockStreamer) PutStream(ctx context.Context, repo *gitalypb.Repository, req proto.Message, src io.Reader) error { +func (ms *mockStreamer) PutStream(ctx context.Context, repo *gitalypb.Repository, req proto.Message, src io.Reader) error { if ms.putStream != nil { return ms.putStream(ctx, repo, req, src) } @@ -299,7 +299,12 @@ func TestCacheInfoRefsUploadPack(t *testing.T) { locator := config.NewLocator(cfg) cache := cache.New(cfg, locator) - gitalyServer := startSmartHTTPServer(t, cfg, withInfoRefCache(newInfoRefCache(cache))) + streamer := mockStreamer{ + Streamer: cache, + } + mockInfoRefCache := newInfoRefCache(&streamer) + + gitalyServer := startSmartHTTPServer(t, cfg, withInfoRefCache(mockInfoRefCache)) rpcRequest := &gitalypb.InfoRefsRequest{Repository: repo} @@ -368,20 +373,13 @@ func TestCacheInfoRefsUploadPack(t *testing.T) { // if an error occurs while putting stream, it should not interrupt // request from being served happened := false - - mockInfoRefCache := newInfoRefCache(mockStreamer{ - Streamer: cache, - putStream: func(context.Context, *gitalypb.Repository, proto.Message, io.Reader) error { - happened = true - return errors.New("oopsie") - }, - }) - - gitalyServer.Shutdown() - addr := runSmartHTTPServer(t, cfg, withInfoRefCache(mockInfoRefCache)) + streamer.putStream = func(context.Context, *gitalypb.Repository, proto.Message, io.Reader) error { + happened = true + return errors.New("oopsie") + } invalidateCacheForRepo() - assertNormalResponse(addr) + assertNormalResponse(gitalyServer.Address()) require.True(t, happened) } diff --git a/internal/gitaly/service/ssh/upload_pack_test.go b/internal/gitaly/service/ssh/upload_pack_test.go index bdc7de33d..019cddb8c 100644 --- a/internal/gitaly/service/ssh/upload_pack_test.go +++ b/internal/gitaly/service/ssh/upload_pack_test.go @@ -416,6 +416,10 @@ func TestUploadPackCloneSuccessWithGitProtocol(t *testing.T) { func testUploadPackCloneSuccessWithGitProtocol(t *testing.T, opts ...testcfg.Option) { cfg, repo, repoPath := testcfg.BuildWithRepo(t, opts...) + readProto, cfg := gittest.EnableGitProtocolV2Support(t, cfg) + + serverSocketPath := runSSHServer(t, cfg) + testhelper.BuildGitalySSH(t, cfg) testhelper.BuildGitalyHooks(t, cfg) @@ -437,10 +441,6 @@ func testUploadPackCloneSuccessWithGitProtocol(t *testing.T, opts ...testcfg.Opt for _, tc := range tests { t.Run(tc.desc, func(t *testing.T) { - readProto, cfg := gittest.EnableGitProtocolV2Support(t, cfg) - - serverSocketPath := runSSHServer(t, cfg) - cmd := cloneCommand{ repository: repo, command: tc.cmd, diff --git a/internal/gitaly/service/wiki/find_page_test.go b/internal/gitaly/service/wiki/find_page_test.go index b48f0738e..1bf7477cf 100644 --- a/internal/gitaly/service/wiki/find_page_test.go +++ b/internal/gitaly/service/wiki/find_page_test.go @@ -15,11 +15,9 @@ import ( "google.golang.org/grpc/codes" ) -func testSuccessfulWikiFindPageRequest(t *testing.T, cfg config.Cfg, rubySrv *rubyserver.Server) { +func testSuccessfulWikiFindPageRequest(t *testing.T, cfg config.Cfg, client gitalypb.WikiServiceClient, rubySrv *rubyserver.Server) { wikiRepo, wikiRepoPath := setupWikiRepo(t, cfg) - client := setupWikiService(t, cfg, rubySrv) - page1Name := "Home Pagé" page2Name := "Instálling/Step 133-b" page3Name := "Installing/Step 133-c" @@ -253,11 +251,9 @@ func testSuccessfulWikiFindPageRequest(t *testing.T, cfg config.Cfg, rubySrv *ru } } -func testSuccessfulWikiFindPageSameTitleDifferentPathRequest(t *testing.T, cfg config.Cfg, rubySrv *rubyserver.Server) { +func testSuccessfulWikiFindPageSameTitleDifferentPathRequest(t *testing.T, cfg config.Cfg, client gitalypb.WikiServiceClient, rubySrv *rubyserver.Server) { wikiRepo, wikiRepoPath := setupWikiRepo(t, cfg) - client := setupWikiService(t, cfg, rubySrv) - page1Name := "page1" page1Content := []byte("content " + page1Name) @@ -450,7 +446,7 @@ func TestInvalidWikiFindPageRequestRevision(t *testing.T) { testhelper.RequireGrpcError(t, err, codes.InvalidArgument) } -func testSuccessfulWikiFindPageRequestWithTrailers(t *testing.T, cfg config.Cfg, rubySrv *rubyserver.Server) { +func testSuccessfulWikiFindPageRequestWithTrailers(t *testing.T, cfg config.Cfg, client gitalypb.WikiServiceClient, rubySrv *rubyserver.Server) { wikiRepo, worktreePath := gittest.InitRepo(t, cfg, cfg.Storages[0], gittest.InitRepoOpts{ WithWorktree: true, }) @@ -463,8 +459,6 @@ func testSuccessfulWikiFindPageRequestWithTrailers(t *testing.T, cfg config.Cfg, "-c", fmt.Sprintf("user.email=%s", committerEmail), "commit", "--allow-empty", "-m", "master branch, empty commit") - client := setupWikiService(t, cfg, rubySrv) - page1Name := "Home Pagé" createTestWikiPage(t, cfg, client, wikiRepo, worktreePath, createWikiPageOpts{title: page1Name}) diff --git a/internal/gitaly/service/wiki/get_all_pages_test.go b/internal/gitaly/service/wiki/get_all_pages_test.go index e29bc1e16..f0c4582f1 100644 --- a/internal/gitaly/service/wiki/get_all_pages_test.go +++ b/internal/gitaly/service/wiki/get_all_pages_test.go @@ -12,11 +12,10 @@ import ( "google.golang.org/grpc/codes" ) -func testSuccessfulWikiGetAllPagesRequest(t *testing.T, cfg config.Cfg, rubySrv *rubyserver.Server) { +func testSuccessfulWikiGetAllPagesRequest(t *testing.T, cfg config.Cfg, client gitalypb.WikiServiceClient, rubySrv *rubyserver.Server) { ctx, cancel := testhelper.Context() defer cancel() - client := setupWikiService(t, cfg, rubySrv) wikiRepo, wikiRepoPath := setupWikiRepo(t, cfg) expectedPages := createTestWikiPages(t, cfg, client, wikiRepo, wikiRepoPath) @@ -61,11 +60,10 @@ func testSuccessfulWikiGetAllPagesRequest(t *testing.T, cfg config.Cfg, rubySrv } } -func testWikiGetAllPagesSorting(t *testing.T, cfg config.Cfg, rubySrv *rubyserver.Server) { +func testWikiGetAllPagesSorting(t *testing.T, cfg config.Cfg, client gitalypb.WikiServiceClient, rubySrv *rubyserver.Server) { ctx, cancel := testhelper.Context() defer cancel() - client := setupWikiService(t, cfg, rubySrv) wikiRepo, wikiRepoPath := setupWikiRepo(t, cfg) expectedPages := createTestWikiPages(t, cfg, client, wikiRepo, wikiRepoPath) @@ -166,9 +164,7 @@ func testWikiGetAllPagesSorting(t *testing.T, cfg config.Cfg, rubySrv *rubyserve } } -func testFailedWikiGetAllPagesDueToValidation(t *testing.T, cfg config.Cfg, rubySrv *rubyserver.Server) { - client := setupWikiService(t, cfg, rubySrv) - +func testFailedWikiGetAllPagesDueToValidation(t *testing.T, cfg config.Cfg, client gitalypb.WikiServiceClient, rubySrv *rubyserver.Server) { testCases := []struct { desc string req *gitalypb.WikiGetAllPagesRequest diff --git a/internal/gitaly/service/wiki/list_pages_test.go b/internal/gitaly/service/wiki/list_pages_test.go index 73b24b843..e890e0f66 100644 --- a/internal/gitaly/service/wiki/list_pages_test.go +++ b/internal/gitaly/service/wiki/list_pages_test.go @@ -11,12 +11,10 @@ import ( "gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb" ) -func testSuccessfulWikiListPagesRequest(t *testing.T, cfg config.Cfg, rubySrv *rubyserver.Server) { +func testSuccessfulWikiListPagesRequest(t *testing.T, cfg config.Cfg, client gitalypb.WikiServiceClient, rubySrv *rubyserver.Server) { ctx, cancel := testhelper.Context() defer cancel() - client := setupWikiService(t, cfg, rubySrv) - wikiRepo, wikiRepoPath := setupWikiRepo(t, cfg) expectedPages := createTestWikiPages(t, cfg, client, wikiRepo, wikiRepoPath) @@ -63,12 +61,10 @@ func testSuccessfulWikiListPagesRequest(t *testing.T, cfg config.Cfg, rubySrv *r } } -func testWikiListPagesSorting(t *testing.T, cfg config.Cfg, rubySrv *rubyserver.Server) { +func testWikiListPagesSorting(t *testing.T, cfg config.Cfg, client gitalypb.WikiServiceClient, rubySrv *rubyserver.Server) { ctx, cancel := testhelper.Context() defer cancel() - client := setupWikiService(t, cfg, rubySrv) - wikiRepo, wikiRepoPath := setupWikiRepo(t, cfg) expectedPages := createTestWikiPages(t, cfg, client, wikiRepo, wikiRepoPath) diff --git a/internal/gitaly/service/wiki/testhelper_test.go b/internal/gitaly/service/wiki/testhelper_test.go index 247fd9ac8..b19b8acbc 100644 --- a/internal/gitaly/service/wiki/testhelper_test.go +++ b/internal/gitaly/service/wiki/testhelper_test.go @@ -44,7 +44,9 @@ func TestWithRubySidecar(t *testing.T) { require.NoError(t, rubySrv.Start()) t.Cleanup(rubySrv.Stop) - fs := []func(t *testing.T, cfg config.Cfg, rubySrv *rubyserver.Server){ + client := setupWikiService(t, cfg, rubySrv) + + fs := []func(t *testing.T, cfg config.Cfg, client gitalypb.WikiServiceClient, rubySrv *rubyserver.Server){ testSuccessfulWikiFindPageRequest, testSuccessfulWikiFindPageSameTitleDifferentPathRequest, testSuccessfulWikiFindPageRequestWithTrailers, @@ -61,7 +63,7 @@ func TestWithRubySidecar(t *testing.T) { } for _, f := range fs { t.Run(runtime.FuncForPC(reflect.ValueOf(f).Pointer()).Name(), func(t *testing.T) { - f(t, cfg, rubySrv) + f(t, cfg, client, rubySrv) }) } } diff --git a/internal/gitaly/service/wiki/update_page_test.go b/internal/gitaly/service/wiki/update_page_test.go index 3fd88f7d8..e48dfb157 100644 --- a/internal/gitaly/service/wiki/update_page_test.go +++ b/internal/gitaly/service/wiki/update_page_test.go @@ -15,15 +15,13 @@ import ( "google.golang.org/grpc/codes" ) -func testSuccessfulWikiUpdatePageRequest(t *testing.T, cfg config.Cfg, rubySrv *rubyserver.Server) { +func testSuccessfulWikiUpdatePageRequest(t *testing.T, cfg config.Cfg, client gitalypb.WikiServiceClient, rubySrv *rubyserver.Server) { wikiRepoProto, wikiRepoPath := setupWikiRepo(t, cfg) wikiRepo := localrepo.NewTestRepo(t, cfg, wikiRepoProto) ctx, cancel := testhelper.Context() defer cancel() - client := setupWikiService(t, cfg, rubySrv) - writeWikiPage(t, client, wikiRepoProto, createWikiPageOpts{title: "Instálling Gitaly", content: []byte("foobar")}) authorID := int32(1) @@ -110,11 +108,9 @@ func testSuccessfulWikiUpdatePageRequest(t *testing.T, cfg config.Cfg, rubySrv * } } -func testFailedWikiUpdatePageDueToValidations(t *testing.T, cfg config.Cfg, rubySrv *rubyserver.Server) { +func testFailedWikiUpdatePageDueToValidations(t *testing.T, cfg config.Cfg, client gitalypb.WikiServiceClient, rubySrv *rubyserver.Server) { wikiRepo, _ := setupWikiRepo(t, cfg) - client := setupWikiService(t, cfg, rubySrv) - writeWikiPage(t, client, wikiRepo, createWikiPageOpts{title: "Installing Gitaly", content: []byte("foobar")}) commitDetails := &gitalypb.WikiCommitDetails{ diff --git a/internal/gitaly/service/wiki/write_page_test.go b/internal/gitaly/service/wiki/write_page_test.go index 73168844e..6e19e17dd 100644 --- a/internal/gitaly/service/wiki/write_page_test.go +++ b/internal/gitaly/service/wiki/write_page_test.go @@ -17,15 +17,13 @@ import ( "google.golang.org/grpc/codes" ) -func testSuccessfulWikiWritePageRequest(t *testing.T, cfg config.Cfg, rubySrv *rubyserver.Server) { +func testSuccessfulWikiWritePageRequest(t *testing.T, cfg config.Cfg, client gitalypb.WikiServiceClient, rubySrv *rubyserver.Server) { wikiRepoProto, wikiRepoPath := setupWikiRepo(t, cfg) wikiRepo := localrepo.NewTestRepo(t, cfg, wikiRepoProto) ctx, cancel := testhelper.Context() defer cancel() - client := setupWikiService(t, cfg, rubySrv) - authorID := int32(1) authorUserName := []byte("ahmad") authorName := []byte("Ahmad Sherif") @@ -115,11 +113,9 @@ func testSuccessfulWikiWritePageRequest(t *testing.T, cfg config.Cfg, rubySrv *r } } -func testFailedWikiWritePageDueToDuplicatePage(t *testing.T, cfg config.Cfg, rubySrv *rubyserver.Server) { +func testFailedWikiWritePageDueToDuplicatePage(t *testing.T, cfg config.Cfg, client gitalypb.WikiServiceClient, rubySrv *rubyserver.Server) { wikiRepo, _ := setupWikiRepo(t, cfg) - client := setupWikiService(t, cfg, rubySrv) - pageName := "Installing Gitaly" content := []byte("Mock wiki page content") commitDetails := &gitalypb.WikiCommitDetails{ @@ -155,11 +151,9 @@ func testFailedWikiWritePageDueToDuplicatePage(t *testing.T, cfg config.Cfg, rub testassert.ProtoEqual(t, expectedResponse, response) } -func testFailedWikiWritePageInPathDueToDuplicatePage(t *testing.T, cfg config.Cfg, rubySrv *rubyserver.Server) { +func testFailedWikiWritePageInPathDueToDuplicatePage(t *testing.T, cfg config.Cfg, client gitalypb.WikiServiceClient, rubySrv *rubyserver.Server) { wikiRepo, _ := setupWikiRepo(t, cfg) - client := setupWikiService(t, cfg, rubySrv) - pageName := "foo/Installing Gitaly" content := []byte("Mock wiki page content") commitDetails := &gitalypb.WikiCommitDetails{ diff --git a/internal/supervisor/supervisor.go b/internal/supervisor/supervisor.go index f3a3dc0ee..56176dd81 100644 --- a/internal/supervisor/supervisor.go +++ b/internal/supervisor/supervisor.go @@ -156,9 +156,9 @@ spawnLoop: waitCh := make(chan struct{}) go func(cmd *exec.Cmd, waitCh chan struct{}) { - err := cmd.Wait() - close(waitCh) + defer close(waitCh) + err := cmd.Wait() cmd.Stdout.(io.WriteCloser).Close() cmd.Stderr.(io.WriteCloser).Close() logger.WithError(err).Warn("exited") diff --git a/internal/supervisor/supervisor_test.go b/internal/supervisor/supervisor_test.go index ccd8c1a93..419c37097 100644 --- a/internal/supervisor/supervisor_test.go +++ b/internal/supervisor/supervisor_test.go @@ -5,7 +5,6 @@ import ( "io" "net" "os" - "os/exec" "path/filepath" "strconv" "syscall" @@ -16,52 +15,27 @@ import ( "gitlab.com/gitlab-org/gitaly/v14/internal/testhelper" ) -var ( - testDir string - testExe string - socketPath string -) - func TestMain(m *testing.M) { - testhelper.Run(m, testhelper.WithSetup(func() error { - var err error - testDir, err = os.MkdirTemp("", "gitaly-supervisor-test") - if err != nil { - return err - } - - scriptPath, err := filepath.Abs("test-scripts/pid-server.go") - if err != nil { - return err - } - - testExe = filepath.Join(testDir, "pid-server") - - buildCmd := exec.Command("go", "build", "-o", testExe, scriptPath) - buildCmd.Dir = filepath.Dir(scriptPath) - buildCmd.Stderr = os.Stderr - buildCmd.Stdout = os.Stdout - if err := buildCmd.Run(); err != nil { - return err - } - - socketPath = filepath.Join(testDir, "socket") - - return nil - })) + testhelper.Run(m) } func TestRespawnAfterCrashWithoutCircuitBreaker(t *testing.T) { + t.Parallel() + + pidServer := buildPidServer(t) + tempDir := testhelper.TempDir(t) + config, err := NewConfigFromEnv() require.NoError(t, err) - process, err := New(config, t.Name(), nil, []string{testExe}, testDir, 0, nil, nil) + + process, err := New(config, t.Name(), nil, []string{pidServer}, tempDir, 0, nil, nil) require.NoError(t, err) defer process.Stop() attempts := config.CrashThreshold require.True(t, attempts > 2, "config.CrashThreshold sanity check") - pids, err := tryConnect(socketPath, attempts, 1*time.Second) + pids, err := tryConnect(filepath.Join(tempDir, "socket"), attempts, 1*time.Second) require.NoError(t, err) require.Equal(t, attempts, len(pids), "number of pids should equal number of attempts") @@ -75,45 +49,56 @@ func TestRespawnAfterCrashWithoutCircuitBreaker(t *testing.T) { } func TestTooManyCrashes(t *testing.T) { + t.Parallel() + + pidServer := buildPidServer(t) + tempDir := testhelper.TempDir(t) + config, err := NewConfigFromEnv() require.NoError(t, err) - process, err := New(config, t.Name(), nil, []string{testExe}, testDir, 0, nil, nil) + + process, err := New(config, t.Name(), nil, []string{pidServer}, tempDir, 0, nil, nil) require.NoError(t, err) defer process.Stop() attempts := config.CrashThreshold + 1 require.True(t, attempts > 2, "config.CrashThreshold sanity check") - pids, err := tryConnect(socketPath, attempts, 1*time.Second) + pids, err := tryConnect(filepath.Join(tempDir, "socket"), attempts, 1*time.Second) require.Error(t, err, "circuit breaker should cause a connection error / timeout") require.Equal(t, config.CrashThreshold, len(pids), "number of pids should equal circuit breaker threshold") } func TestSpawnFailure(t *testing.T) { + t.Parallel() + + pidServer := buildPidServer(t) + tempDir := testhelper.TempDir(t) + config, err := NewConfigFromEnv() require.NoError(t, err) config.CrashWaitTime = 2 * time.Second - notFoundExe := filepath.Join(testDir, "not-found") + notFoundExe := filepath.Join(tempDir, "not-found") require.NoError(t, os.RemoveAll(notFoundExe)) defer func() { require.NoError(t, os.Remove(notFoundExe)) }() - process, err := New(config, t.Name(), nil, []string{notFoundExe}, testDir, 0, nil, nil) + process, err := New(config, t.Name(), nil, []string{notFoundExe}, tempDir, 0, nil, nil) require.NoError(t, err) defer process.Stop() time.Sleep(1 * time.Second) - pids, err := tryConnect(socketPath, 1, 1*time.Millisecond) + pids, err := tryConnect(filepath.Join(tempDir, "socket"), 1, 1*time.Millisecond) require.Error(t, err, "connection must fail because executable cannot be spawned") require.Equal(t, 0, len(pids)) // 'Fix' the spawning problem of our process - require.NoError(t, os.Symlink(testExe, notFoundExe)) + require.NoError(t, os.Symlink(pidServer, notFoundExe)) // After CrashWaitTime, the circuit breaker should have closed - pids, err = tryConnect(socketPath, 1, config.CrashWaitTime) + pids, err = tryConnect(filepath.Join(tempDir, "socket"), 1, config.CrashWaitTime) require.NoError(t, err, "process should be accepting connections now") require.Equal(t, 1, len(pids), "we should have received the pid of the new process") @@ -181,3 +166,12 @@ func getPid(ctx context.Context, socket string) (int, error) { return strconv.Atoi(string(response)) } + +func buildPidServer(t *testing.T) string { + t.Helper() + + sourcePath, err := filepath.Abs("test-scripts/pid-server.go") + require.NoError(t, err) + + return testhelper.BuildBinary(t, testhelper.TempDir(t), sourcePath) +} diff --git a/internal/testhelper/build.go b/internal/testhelper/build.go index 2913b45b1..873e3a91f 100644 --- a/internal/testhelper/build.go +++ b/internal/testhelper/build.go @@ -13,17 +13,11 @@ import ( "gitlab.com/gitlab-org/gitaly/v14/internal/version" ) -var ( - buildGitalyGit2GoOnce sync.Once - buildGitalyLFSSmudgeOnce sync.Once - buildGitalyHooksOnce sync.Once - buildGitalySSHOnce sync.Once - buildPraefectOnce sync.Once -) +var buildOnceByName sync.Map // BuildGitalyGit2Go builds the gitaly-git2go command and installs it into the binary directory. func BuildGitalyGit2Go(t testing.TB, cfg config.Cfg) { - buildBinary(t, cfg.BinDir, "gitaly-git2go", &buildGitalyGit2GoOnce) + BuildBinary(t, cfg.BinDir, gitalyCommandPath("gitaly-git2go")) // The link is needed because gitaly uses version-named binary. // Please check out https://gitlab.com/gitlab-org/gitaly/-/issues/3647 for more info. if err := os.Link(filepath.Join(cfg.BinDir, "gitaly-git2go"), filepath.Join(cfg.BinDir, "gitaly-git2go-"+version.GetModuleVersion())); err != nil { @@ -37,62 +31,69 @@ func BuildGitalyGit2Go(t testing.TB, cfg config.Cfg) { // BuildGitalyLFSSmudge builds the gitaly-lfs-smudge command and installs it into the binary // directory. func BuildGitalyLFSSmudge(t *testing.T, cfg config.Cfg) { - buildBinary(t, cfg.BinDir, "gitaly-lfs-smudge", &buildGitalyLFSSmudgeOnce) + BuildBinary(t, cfg.BinDir, gitalyCommandPath("gitaly-lfs-smudge")) } // BuildGitalyHooks builds the gitaly-hooks command and installs it into the binary directory. func BuildGitalyHooks(t testing.TB, cfg config.Cfg) { - buildBinary(t, cfg.BinDir, "gitaly-hooks", &buildGitalyHooksOnce) + BuildBinary(t, cfg.BinDir, gitalyCommandPath("gitaly-hooks")) } // BuildGitalySSH builds the gitaly-ssh command and installs it into the binary directory. func BuildGitalySSH(t testing.TB, cfg config.Cfg) { - buildBinary(t, cfg.BinDir, "gitaly-ssh", &buildGitalySSHOnce) + BuildBinary(t, cfg.BinDir, gitalyCommandPath("gitaly-ssh")) } // BuildPraefect builds the praefect command and installs it into the binary directory. func BuildPraefect(t testing.TB, cfg config.Cfg) { - buildBinary(t, cfg.BinDir, "praefect", &buildPraefectOnce) + BuildBinary(t, cfg.BinDir, gitalyCommandPath("praefect")) } -func buildBinary(t testing.TB, dstDir, name string, buildOnce *sync.Once) { +// BuildBinary builds a Go binary once and copies it into the target directory. The source path can +// either be a ".go" file or a directory containing Go files. Returns the path to the executable in +// the destination directory. +func BuildBinary(t testing.TB, targetDir, sourcePath string) string { require.NotEmpty(t, testDirectory, "you must call testhelper.Configure() first") - // binsPath is a shared between all tests location where all compiled binaries should be placed - binsPath := filepath.Join(testDirectory, "bins") - // binPath is a path to a specific binary file - binPath := filepath.Join(binsPath, name) - - defer func() { - if t.Failed() { - return - } - - targetPath := filepath.Join(dstDir, name) - - // Exit early if the file exists. - if _, err := os.Stat(targetPath); err == nil { - return - } - - // copy compiled binary to the destination folder - require.NoError(t, os.MkdirAll(dstDir, os.ModePerm)) - CopyFile(t, binPath, targetPath) - require.NoError(t, os.Chmod(targetPath, 0o777)) - }() + var ( + // executableName is the name of the executable. + executableName = filepath.Base(sourcePath) + // sharedBinariesDir is where all binaries will be compiled into. This directory is + // shared between all tests. + sharedBinariesDir = filepath.Join(testDirectory, "bins") + // sharedBinaryPath is the path to the binary shared between all tests. + sharedBinaryPath = filepath.Join(sharedBinariesDir, executableName) + // targetPath is the final path where the binary should be copied to. + targetPath = filepath.Join(targetDir, executableName) + ) + + buildOnceInterface, _ := buildOnceByName.LoadOrStore(executableName, &sync.Once{}) + buildOnce, ok := buildOnceInterface.(*sync.Once) + require.True(t, ok) buildOnce.Do(func() { - require.NoError(t, os.MkdirAll(binsPath, os.ModePerm)) - require.NoFileExists(t, binPath, "binary has already been built") + require.NoError(t, os.MkdirAll(sharedBinariesDir, os.ModePerm)) + require.NoFileExists(t, sharedBinaryPath, "binary has already been built") MustRunCommand(t, nil, "go", "build", "-tags", "static,system_libgit2", - "-o", binPath, - fmt.Sprintf("gitlab.com/gitlab-org/gitaly/v14/cmd/%s", name), + "-o", sharedBinaryPath, + sourcePath, ) }) - require.FileExists(t, binPath, "%s does not exist", name) + require.FileExists(t, sharedBinaryPath, "%s does not exist", executableName) + require.NoFileExists(t, targetPath, "%s exists already -- do you try to build it twice?", executableName) + + require.NoError(t, os.MkdirAll(targetDir, os.ModePerm)) + CopyFile(t, sharedBinaryPath, targetPath) + require.NoError(t, os.Chmod(targetPath, 0o755)) + + return targetPath +} + +func gitalyCommandPath(command string) string { + return fmt.Sprintf("gitlab.com/gitlab-org/gitaly/v14/cmd/%s", command) } |