From a18904b6fcf557d899c2527c24a80bd3be09a031 Mon Sep 17 00:00:00 2001 From: Erik Seliger Date: Mon, 3 Jun 2024 16:37:11 +0200 Subject: [PATCH] gitserver: Remove IsCloneable check from clone path (#63028) We made this preflight check always, and the doc string said that it's for "better error messages". I checked on that, and the additional RPS claim and the additional latency of the iscloneable check plus load on the code host don't seem justified. Error message before this change: ``` Error updating repo: failed to clone dev.azure.com/sourcegraph-source/src-cli/src-cli: error cloning repo: repo dev.azure.com/sourcegraph-source/src-cli/src-cli not cloneable: failed to check remote access: fatal: Authentication failed for 'https://dev.azure.com/sourcegraph-source/src-cli/_git/src-cli/': exit status 128 ``` Error message after this change: ``` Error updating repo: failed to clone dev.azure.com/sourcegraph-source/src-cli/src-cli: clone failed. Output: Creating bare repo Created bare repo at /Users/erik/.sourcegraph/repos_1/.tmp/clone-2786287217/.git Fetching remote contents fatal: Authentication failed for 'https://dev.azure.com/sourcegraph-source/src-cli/_git/src-cli/': failed to fetch: exit status 128: command failed: exit status 128 ``` Not much worse or less readable. Closes https://github.com/sourcegraph/sourcegraph/issues/62786 Test plan: Adjusted tests, made sure repos still clone alright and throw a somewhat useful error message on failure. --- .../internal/integration_tests/clone_test.go | 47 ++----------------- cmd/gitserver/internal/server.go | 22 +-------- cmd/gitserver/internal/server_test.go | 20 -------- cmd/gitserver/internal/vcssyncer/git.go | 8 ---- 4 files changed, 6 insertions(+), 91 deletions(-) diff --git a/cmd/gitserver/internal/integration_tests/clone_test.go b/cmd/gitserver/internal/integration_tests/clone_test.go index cf8fad8d40d..d548178fc9b 100644 --- a/cmd/gitserver/internal/integration_tests/clone_test.go +++ b/cmd/gitserver/internal/integration_tests/clone_test.go @@ -188,59 +188,20 @@ func TestClone_Fail(t *testing.T) { Hostname: "test-shard", }) - // Requesting a repo update should figure out that the repo is not yet - // cloned and call clone. We expect that clone to fail, because vcssyncer.IsCloneable - // fails here. + // Requesting a repo update should figure out that the repo is not yet cloned and call clone. + // We expect that clone to fail. _, _, err := s.FetchRepository(ctx, repo) require.Error(t, err) - // Note that this error is from IsCloneable(), not from Clone(). - require.Contains(t, err.Error(), "error cloning repo: repo github.com/test/repo not cloneable:") - require.Contains(t, err.Error(), "exit status 128") - - mockassert.CalledOnce(t, locker.TryAcquireFunc) - mockassert.CalledOnce(t, lock.ReleaseFunc) - - // Check we reported an error. - // Check that it was called exactly once total. - mockrequire.CalledOnce(t, gsStore.SetLastErrorFunc) - // And that it was called for the right repo, setting the last error value. - mockassert.CalledWith(t, gsStore.SetLastErrorFunc, mockassert.Values(mockassert.Skip, repo, mockassert.Skip, "test-shard")) - require.Contains(t, gsStore.SetLastErrorFunc.History()[0].Arg2, `error cloning repo: repo github.com/test/repo not cloneable:`) - require.Contains(t, gsStore.SetLastErrorFunc.History()[0].Arg2, "exit status 128") - - // And no other DB activity has happened. - mockassert.NotCalled(t, gsStore.SetCloneStatusFunc) - mockassert.NotCalled(t, gsStore.SetLastOutputFunc) - - // =================== - - // Now, fake that the IsCloneable check passes, then Clone will be called - // and is expected to fail. - vcssyncer.TestGitRepoExists = func(ctx context.Context, name api.RepoName) error { - return nil - } - t.Cleanup(func() { - vcssyncer.TestGitRepoExists = nil - }) - // Reset mock counters. - gsStore = dbmocks.NewMockGitserverRepoStore() - db.GitserverReposFunc.SetDefaultReturn(gsStore) - - // Requesting another repo update should figure out that the repo is not yet - // cloned and call clone. We expect that clone to fail, but in the vcssyncer.Clone - // stage this time, not vcssyncer.IsCloneable. - _, _, err = s.FetchRepository(ctx, repo) - require.Error(t, err) require.Contains(t, err.Error(), "failed to clone github.com/test/repo: clone failed. Output: Creating bare repo\nCreated bare repo at") // Should have acquired a lock. - mockassert.CalledN(t, locker.TryAcquireFunc, 2) + mockassert.CalledN(t, locker.TryAcquireFunc, 1) // Should have reported status. 7 lines is the output git currently produces. // This number might need to be adjusted over time, but before doing so please // check that the calls actually use the args you would expect them to use. mockassert.CalledN(t, lock.SetStatusFunc, 7) // Should have released the lock. - mockassert.CalledN(t, lock.ReleaseFunc, 2) + mockassert.CalledN(t, lock.ReleaseFunc, 1) // Check it was set to cloning first, then uncloned again (since clone failed). mockassert.CalledN(t, gsStore.SetCloneStatusFunc, 2) diff --git a/cmd/gitserver/internal/server.go b/cmd/gitserver/internal/server.go index 21402e7f62d..3808ab8600d 100644 --- a/cmd/gitserver/internal/server.go +++ b/cmd/gitserver/internal/server.go @@ -392,27 +392,9 @@ func (s *Server) cloneRepo(ctx context.Context, repo api.RepoName, lock Reposito logger := s.logger.Scoped("cloneRepo").With(log.String("repo", string(repo))) - syncer, err := func() (_ vcssyncer.VCSSyncer, err error) { - syncer, err := s.getVCSSyncer(ctx, repo) - if err != nil { - return nil, errors.Wrap(err, "get VCS syncer") - } - - if err = s.rpsLimiter.Wait(ctx); err != nil { - return nil, err - } - - if err := syncer.IsCloneable(ctx, repo); err != nil { - return nil, errors.Wrapf(err, "error cloning repo: repo %s not cloneable", repo) - } - - return syncer, nil - }() + syncer, err := s.getVCSSyncer(ctx, repo) if err != nil { - if ctx.Err() != nil { - return ctx.Err() - } - return err + return errors.Wrap(err, "get VCS syncer") } dir := s.fs.RepoDir(repo) diff --git a/cmd/gitserver/internal/server_test.go b/cmd/gitserver/internal/server_test.go index f896c7bf373..4520cf84ffd 100644 --- a/cmd/gitserver/internal/server_test.go +++ b/cmd/gitserver/internal/server_test.go @@ -185,15 +185,6 @@ func TestExecRequest(t *testing.T) { return false, nil }) - vcssyncer.TestGitRepoExists = func(ctx context.Context, repoName api.RepoName) error { - if strings.Contains(string(repoName), "nicksnyder/go-i18n") { - return nil - } - - return errors.New("not cloneable") - } - t.Cleanup(func() { vcssyncer.TestGitRepoExists = nil }) - for _, test := range tests { t.Run(test.Name, func(t *testing.T) { ss := gitserver.NewMockGitserverService_ExecServer() @@ -447,17 +438,6 @@ func TestCloneRepoRecordsFailures(t *testing.T) { getVCSSyncer func(ctx context.Context, name api.RepoName) (vcssyncer.VCSSyncer, error) wantErr string }{ - { - name: "Not cloneable", - getVCSSyncer: func(ctx context.Context, name api.RepoName) (vcssyncer.VCSSyncer, error) { - m := vcssyncer.NewMockVCSSyncer() - m.IsCloneableFunc.SetDefaultHook(func(context.Context, api.RepoName) error { - return errors.New("not_cloneable") - }) - return m, nil - }, - wantErr: "failed to clone example.com/foo/bar: error cloning repo: repo example.com/foo/bar not cloneable: not_cloneable", - }, { name: "Failing clone", getVCSSyncer: func(ctx context.Context, name api.RepoName) (vcssyncer.VCSSyncer, error) { diff --git a/cmd/gitserver/internal/vcssyncer/git.go b/cmd/gitserver/internal/vcssyncer/git.go index a06e1955893..47f4c1122f8 100644 --- a/cmd/gitserver/internal/vcssyncer/git.go +++ b/cmd/gitserver/internal/vcssyncer/git.go @@ -42,20 +42,12 @@ func (s *gitRepoSyncer) Type() string { return "git" } -// TestGitRepoExists is a test fixture that overrides the return value for -// GitRepoSyncer.IsCloneable when it is set. -var TestGitRepoExists func(ctx context.Context, repoName api.RepoName) error - // IsCloneable checks to see if the Git remote URL is cloneable. func (s *gitRepoSyncer) IsCloneable(ctx context.Context, repoName api.RepoName) (err error) { if isAlwaysCloningTest(repoName) { return nil } - if TestGitRepoExists != nil { - return TestGitRepoExists(ctx, repoName) - } - source, err := s.getRemoteURLSource(ctx, repoName) if err != nil { return errors.Wrapf(err, "failed to get remote URL source for %s", repoName)