Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

Commit f9d6775

Browse files
eseligereseliger
committed
gitserver: Better clean up after state changes
The database is not super clean when we do state changes, for example we keep the repo size after marking as not cloned, and keep shard assignments, too. This PR splits those operations into three, and resets more rows. Test plan: Cloning and tests around clone status calls are still passing.
1 parent c6b1485 commit f9d6775

File tree

16 files changed

+453
-117
lines changed

16 files changed

+453
-117
lines changed

‎cmd/frontend/graphqlbackend/repositories_test.go

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -741,8 +741,19 @@ func TestRepositories_Integration(t *testing.T) {
741741
if err := gitserverRepos.SetRepoSize(ctx, rsc.repo.Name, rsc.size, "shard-1"); err != nil {
742742
t.Fatal(err)
743743
}
744-
if err := gitserverRepos.SetCloneStatus(ctx, rsc.repo.Name, rsc.cloneStatus, "shard-1"); err != nil {
745-
t.Fatal(err)
744+
switch rsc.cloneStatus {
745+
case types.CloneStatusCloned:
746+
if err := gitserverRepos.SetCloned(ctx, rsc.repo.Name, "shard-1"); err != nil {
747+
t.Fatal(err)
748+
}
749+
case types.CloneStatusCloning:
750+
if err := gitserverRepos.SetCloning(ctx, rsc.repo.Name, "shard-1"); err != nil {
751+
t.Fatal(err)
752+
}
753+
case types.CloneStatusNotCloned:
754+
if err := gitserverRepos.SetNotCloned(ctx, rsc.repo.Name); err != nil {
755+
t.Fatal(err)
756+
}
746757
}
747758

748759
if rsc.indexed {

‎cmd/gitserver/internal/cleanup.go

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,6 @@ import (
3737
"github.com/sourcegraph/sourcegraph/internal/goroutine"
3838
"github.com/sourcegraph/sourcegraph/internal/hostname"
3939
"github.com/sourcegraph/sourcegraph/internal/lazyregexp"
40-
"github.com/sourcegraph/sourcegraph/internal/types"
4140
"github.com/sourcegraph/sourcegraph/internal/wrexec"
4241
"github.com/sourcegraph/sourcegraph/lib/errors"
4342
)
@@ -78,7 +77,7 @@ func NewJanitor(ctx context.Context, cfg JanitorConfig, db database.DB, fs gitse
7877

7978
toFree := howManyBytesToFree(logger, usage, cfg.DesiredPercentFree)
8079

81-
if err := freeUpSpace(ctx, logger, db, fs, cfg.ShardID, usage, cfg.DesiredPercentFree, toFree); err != nil {
80+
if err := freeUpSpace(ctx, logger, db, fs, usage, cfg.DesiredPercentFree, toFree); err != nil {
8281
logger.Error("error freeing up space", log.Error(err))
8382
}
8483

@@ -366,7 +365,7 @@ func cleanupRepos(
366365
reposRemoved.WithLabelValues(reason).Inc()
367366

368367
// Set as not_cloned in the database.
369-
if err := db.GitserverRepos().SetCloneStatus(ctx, repoName, types.CloneStatusNotCloned, shardID); err != nil {
368+
if err := db.GitserverRepos().SetNotCloned(ctx, repoName); err != nil {
370369
return true, errors.Wrap(err, "failed to update clone status")
371370
}
372371

@@ -486,7 +485,7 @@ func cleanupRepos(
486485
return true, errors.Wrap(err, "failed to remove repo")
487486
}
488487
// Set as not_cloned in the database.
489-
if err := db.GitserverRepos().SetCloneStatus(ctx, repoName, types.CloneStatusNotCloned, shardID); err != nil {
488+
if err := db.GitserverRepos().SetNotCloned(ctx, repoName); err != nil {
490489
return true, errors.Wrap(err, "failed to update clone status")
491490
}
492491

@@ -716,7 +715,7 @@ func howManyBytesToFree(logger log.Logger, usage diskusage.DiskUsage, desiredPer
716715

717716
// freeUpSpace removes git directories under the fs, in order from least
718717
// recently to most recently used, until it has freed howManyBytesToFree.
719-
func freeUpSpace(ctx context.Context, logger log.Logger, db database.DB, fs gitserverfs.FS, shardID string, usage diskusage.DiskUsage, desiredPercentFree int, howManyBytesToFree int64) error {
718+
func freeUpSpace(ctx context.Context, logger log.Logger, db database.DB, fs gitserverfs.FS, usage diskusage.DiskUsage, desiredPercentFree int, howManyBytesToFree int64) error {
720719
if howManyBytesToFree <= 0 {
721720
return nil
722721
}
@@ -773,7 +772,7 @@ func freeUpSpace(ctx context.Context, logger log.Logger, db database.DB, fs gits
773772
continue
774773
}
775774
// Set as not_cloned in the database.
776-
if err := db.GitserverRepos().SetCloneStatus(ctx, repoName, types.CloneStatusNotCloned, shardID); err != nil {
775+
if err := db.GitserverRepos().SetNotCloned(ctx, repoName); err != nil {
777776
logger.Warn("failed to update clone status", log.Error(err))
778777
}
779778
spaceFreed += delta

‎cmd/gitserver/internal/cleanup_test.go

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -811,12 +811,12 @@ func TestFreeUpSpace(t *testing.T) {
811811
fs := gitserverfs.New(observation.TestContextTB(t), root)
812812
require.NoError(t, fs.Initialize())
813813
t.Run("no error if no space requested and no repos", func(t *testing.T) {
814-
if err := freeUpSpace(context.Background(), logger, newMockedGitserverDB(), fs, "test-gitserver", &fakeDiskUsage{}, 10, 0); err != nil {
814+
if err := freeUpSpace(context.Background(), logger, newMockedGitserverDB(), fs, &fakeDiskUsage{}, 10, 0); err != nil {
815815
t.Fatal(err)
816816
}
817817
})
818818
t.Run("error if space requested and no repos", func(t *testing.T) {
819-
if err := freeUpSpace(context.Background(), logger, newMockedGitserverDB(), fs, "test-gitserver", &fakeDiskUsage{}, 10, 1); err == nil {
819+
if err := freeUpSpace(context.Background(), logger, newMockedGitserverDB(), fs, &fakeDiskUsage{}, 10, 1); err == nil {
820820
t.Fatal("want error")
821821
}
822822
})
@@ -843,7 +843,7 @@ func TestFreeUpSpace(t *testing.T) {
843843
gr := dbmocks.NewMockGitserverRepoStore()
844844
db.GitserverReposFunc.SetDefaultReturn(gr)
845845
// Run.
846-
if err := freeUpSpace(context.Background(), logger, db, fs, "test-gitserver", &fakeDiskUsage{}, 10, 1000); err != nil {
846+
if err := freeUpSpace(context.Background(), logger, db, fs, &fakeDiskUsage{}, 10, 1000); err != nil {
847847
t.Fatal(err)
848848
}
849849

@@ -860,10 +860,9 @@ func TestFreeUpSpace(t *testing.T) {
860860
t.Errorf("repo dir size is %d, want no more than %d", rds, wantSize)
861861
}
862862

863-
if len(gr.SetCloneStatusFunc.History()) == 0 {
864-
t.Fatal("expected gitserverRepos.SetCloneStatus to be called, but wasn't")
863+
if len(gr.SetNotClonedFunc.History()) == 0 {
864+
t.Fatal("expected gitserverRepos.SetNotCloned to be called, but wasn't")
865865
}
866-
require.Equal(t, gr.SetCloneStatusFunc.History()[0].Arg2, types.CloneStatusNotCloned)
867866
})
868867
}
869868

‎cmd/gitserver/internal/integration_tests/clone_test.go

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@ import (
2626
"github.com/sourcegraph/sourcegraph/internal/database/dbmocks"
2727
"github.com/sourcegraph/sourcegraph/internal/observation"
2828
"github.com/sourcegraph/sourcegraph/internal/ratelimit"
29-
"github.com/sourcegraph/sourcegraph/internal/types"
3029
"github.com/sourcegraph/sourcegraph/internal/vcs"
3130
"github.com/sourcegraph/sourcegraph/internal/wrexec"
3231
)
@@ -104,9 +103,10 @@ func TestClone(t *testing.T) {
104103
mockassert.CalledOnce(t, lock.ReleaseFunc)
105104

106105
// Check it was set to cloning first, then cloned.
107-
mockassert.CalledN(t, gsStore.SetCloneStatusFunc, 2)
108-
mockassert.CalledWith(t, gsStore.SetCloneStatusFunc, mockassert.Values(mockassert.Skip, repo, types.CloneStatusCloning, "test-shard"))
109-
mockassert.CalledWith(t, gsStore.SetCloneStatusFunc, mockassert.Values(mockassert.Skip, repo, types.CloneStatusCloned, "test-shard"))
106+
mockassert.CalledN(t, gsStore.SetCloningFunc, 1)
107+
mockassert.CalledN(t, gsStore.SetClonedFunc, 1)
108+
mockassert.CalledWith(t, gsStore.SetCloningFunc, mockassert.Values(mockassert.Skip, repo, "test-shard"))
109+
mockassert.CalledWith(t, gsStore.SetClonedFunc, mockassert.Values(mockassert.Skip, repo, "test-shard"))
110110

111111
// Last output should have been stored for the repo.
112112
mockrequire.CalledOnce(t, gsStore.SetLastOutputFunc)
@@ -212,7 +212,9 @@ func TestClone_Fail(t *testing.T) {
212212
require.Contains(t, gsStore.SetLastErrorFunc.History()[0].Arg2, "exit status 128")
213213

214214
// And no other DB activity has happened.
215-
mockassert.NotCalled(t, gsStore.SetCloneStatusFunc)
215+
mockassert.NotCalled(t, gsStore.SetClonedFunc)
216+
mockassert.NotCalled(t, gsStore.SetCloningFunc)
217+
mockassert.NotCalled(t, gsStore.SetNotClonedFunc)
216218
mockassert.NotCalled(t, gsStore.SetLastOutputFunc)
217219

218220
// ===================
@@ -246,9 +248,10 @@ func TestClone_Fail(t *testing.T) {
246248
mockassert.CalledN(t, lock.ReleaseFunc, 2)
247249

248250
// Check it was set to cloning first, then uncloned again (since clone failed).
249-
mockassert.CalledN(t, gsStore.SetCloneStatusFunc, 2)
250-
mockassert.CalledWith(t, gsStore.SetCloneStatusFunc, mockassert.Values(mockassert.Skip, repo, types.CloneStatusCloning, "test-shard"))
251-
mockassert.CalledWith(t, gsStore.SetCloneStatusFunc, mockassert.Values(mockassert.Skip, repo, types.CloneStatusNotCloned, "test-shard"))
251+
mockassert.CalledN(t, gsStore.SetCloningFunc, 1)
252+
mockassert.CalledN(t, gsStore.SetNotClonedFunc, 1)
253+
mockassert.CalledWith(t, gsStore.SetCloningFunc, mockassert.Values(mockassert.Skip, repo, "test-shard"))
254+
mockassert.CalledWith(t, gsStore.SetNotClonedFunc, mockassert.Values(mockassert.Skip, repo))
252255

253256
// Last output should have been stored for the repo.
254257
mockrequire.CalledOnce(t, gsStore.SetLastOutputFunc)

‎cmd/gitserver/internal/integration_tests/utils_test.go

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,6 @@ import (
2929
"github.com/sourcegraph/sourcegraph/internal/api"
3030
"github.com/sourcegraph/sourcegraph/internal/database/dbmocks"
3131
"github.com/sourcegraph/sourcegraph/internal/extsvc"
32-
"github.com/sourcegraph/sourcegraph/internal/gitserver"
3332
proto "github.com/sourcegraph/sourcegraph/internal/gitserver/v1"
3433
internalgrpc "github.com/sourcegraph/sourcegraph/internal/grpc"
3534
"github.com/sourcegraph/sourcegraph/internal/grpc/defaults"
@@ -44,9 +43,8 @@ var root string
4443
// This is a default gitserver test client currently used for RequestRepoUpdate
4544
// gitserver calls during invocation of MakeGitRepository function
4645
var (
47-
testGitserverClient gitserver.Client
48-
GitserverAddresses []string
49-
testServer *server.Server
46+
GitserverAddresses []string
47+
testServer *server.Server
5048
)
5149

5250
func InitGitserver() {
@@ -135,8 +133,6 @@ func InitGitserver() {
135133
}()
136134

137135
serverAddress := l.Addr().String()
138-
source := gitserver.NewTestClientSource(&t, []string{serverAddress})
139-
testGitserverClient = gitserver.NewTestClient(&t).WithClientSource(source)
140136
GitserverAddresses = []string{serverAddress}
141137
testServer = s
142138
}

‎cmd/gitserver/internal/repo_info.go

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ import (
77
"github.com/sourcegraph/sourcegraph/internal/api"
88
"github.com/sourcegraph/sourcegraph/internal/database"
99
"github.com/sourcegraph/sourcegraph/internal/gitserver/protocol"
10-
"github.com/sourcegraph/sourcegraph/internal/types"
1110
"github.com/sourcegraph/sourcegraph/lib/errors"
1211
)
1312

@@ -35,7 +34,6 @@ func repoCloneProgress(fs gitserverfs.FS, locker RepositoryLocker, repo api.Repo
3534
func deleteRepo(
3635
ctx context.Context,
3736
db database.DB,
38-
shardID string,
3937
fs gitserverfs.FS,
4038
repo api.RepoName,
4139
) error {
@@ -44,7 +42,7 @@ func deleteRepo(
4442
return errors.Wrap(err, "removing repo directory")
4543
}
4644

47-
err = db.GitserverRepos().SetCloneStatus(ctx, repo, types.CloneStatusNotCloned, shardID)
45+
err = db.GitserverRepos().SetNotCloned(ctx, repo)
4846
if err != nil {
4947
return errors.Wrap(err, "setting clone status after delete")
5048
}

‎cmd/gitserver/internal/repo_info_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,7 @@ func testDeleteRepo(t *testing.T, deletedInDB bool) {
9292
}
9393

9494
// Now we can delete it
95-
require.NoError(t, deleteRepo(ctx, db, "", s.fs, dbRepo.Name))
95+
require.NoError(t, deleteRepo(ctx, db, s.fs, dbRepo.Name))
9696

9797
size, err = s.fs.DirSize(string(s.fs.RepoDir(repoName)))
9898
require.NoError(t, err)

‎cmd/gitserver/internal/repositoryservice.go

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ func (s *repositoryServiceServer) DeleteRepository(ctx context.Context, req *pro
7272
return nil, status.New(codes.NotFound, "repository not found").Err()
7373
}
7474

75-
if err := deleteRepo(ctx, s.db, s.hostname, s.fs, repoName); err != nil {
75+
if err := deleteRepo(ctx, s.db, s.fs, repoName); err != nil {
7676
s.logger.Error("failed to delete repository", log.String("repo", string(repoName)), log.Error(err))
7777
return &proto.DeleteRepositoryResponse{}, status.Errorf(codes.Internal, "failed to delete repository %s: %s", repoName, err)
7878
}
@@ -121,6 +121,8 @@ func (s *repositoryServiceServer) FetchRepository(req *proto.FetchRepositoryRequ
121121

122122
lastFetched, lastChanged, err := s.svc.FetchRepository(ss.Context(), repoName)
123123
if err != nil {
124+
s.svc.LogIfCorrupt(context.Background(), repoName, err)
125+
124126
return status.New(codes.Internal, errors.Wrap(err, "failed to fetch repository").Error()).Err()
125127
}
126128

‎cmd/gitserver/internal/server.go

Lines changed: 12 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,6 @@ import (
3535
"github.com/sourcegraph/sourcegraph/internal/gitserver/protocol"
3636
"github.com/sourcegraph/sourcegraph/internal/limiter"
3737
"github.com/sourcegraph/sourcegraph/internal/ratelimit"
38-
"github.com/sourcegraph/sourcegraph/internal/types"
3938
"github.com/sourcegraph/sourcegraph/internal/vcs"
4039
"github.com/sourcegraph/sourcegraph/internal/wrexec"
4140
"github.com/sourcegraph/sourcegraph/lib/errors"
@@ -468,21 +467,21 @@ func (s *Server) cloneRepo(ctx context.Context, repo api.RepoName, lock Reposito
468467
defer os.RemoveAll(tmpDir)
469468
tmpPath := filepath.Join(tmpDir, ".git")
470469

471-
if err := s.db.GitserverRepos().SetCloneStatus(ctx, repo, types.CloneStatusCloning, s.hostname); err != nil {
470+
// Mark as cloning in the DB.
471+
if err := s.db.GitserverRepos().SetCloning(ctx, repo, s.hostname); err != nil {
472472
s.logger.Error("Setting clone status in DB", log.Error(err))
473473
}
474474
defer func() {
475-
cloned, err := s.fs.RepoCloned(repo)
476-
if err != nil {
477-
s.logger.Error("failed to check if repo is cloned", log.Error(err))
478-
} else if err := s.db.GitserverRepos().SetCloneStatus(
479-
// Use a background context to ensure we still update the DB even if we time out
480-
context.Background(),
481-
repo,
482-
cloneStatus(cloned, false),
483-
s.hostname,
484-
); err != nil {
485-
s.logger.Error("Setting clone status in DB", log.Error(err))
475+
// If the clone was a success, mark the repo as cloned in the DB.
476+
if err == nil {
477+
if err := s.db.GitserverRepos().SetCloned(
478+
// Use a background context to ensure we still update the DB even if we time out
479+
context.Background(),
480+
repo,
481+
s.hostname,
482+
); err != nil {
483+
s.logger.Error("Setting clone status in DB", log.Error(err))
484+
}
486485
}
487486
}()
488487

‎cmd/gitserver/internal/server_test.go

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,8 @@ import (
1212
"strings"
1313
"testing"
1414

15+
"github.com/keegancsmith/sqlf"
16+
1517
"github.com/sourcegraph/sourcegraph/internal/actor"
1618

1719
"github.com/google/go-cmp/cmp"
@@ -887,9 +889,9 @@ func TestSyncRepoState(t *testing.T) {
887889

888890
t.Run("sync deleted repo", func(t *testing.T) {
889891
// Fake setting an incorrect status
890-
if err := db.GitserverRepos().SetCloneStatus(ctx, dbRepo.Name, types.CloneStatusUnknown, hostname); err != nil {
891-
t.Fatal(err)
892-
}
892+
q := sqlf.Sprintf("UPDATE gitserver_repos SET clone_status = %s WHERE repo_id = (SELECT id FROM repo where name = %s)", "unknown", dbRepo.Name)
893+
_, err := db.ExecContext(ctx, q.Query(sqlf.PostgresBindVar), q.Args()...)
894+
require.NoError(t, err)
893895

894896
// We should continue to sync deleted repos
895897
if err := db.Repos().Delete(ctx, dbRepo.ID); err != nil {

0 commit comments

Comments
 (0)