Skip to content

Commit 21a9742

Browse files
kushnaidukushnaidu
authored
Improve mutex handling in git.go and add log completion for API calls (#363)
* Remove r.repo from external git structure * Add log completion for all API calls * Add mutex.RLock to some of the git.go functions
1 parent b1be256 commit 21a9742

21 files changed

Lines changed: 345 additions & 47 deletions

‎pkg/cache/dbcache/dbpackagerevision.go‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -325,7 +325,9 @@ func (pr *dbPackageRevision) GetKptfile(ctx context.Context) (kptfile.KptFile, e
325325
return *kf, nil
326326
}
327327

328-
func (pr *dbPackageRevision) GetLock() (kptfile.Upstream, kptfile.UpstreamLock, error) {
328+
func (pr *dbPackageRevision) GetLock(ctx context.Context) (kptfile.Upstream, kptfile.UpstreamLock, error) {
329+
_, span := tracer.Start(ctx, "dbPackageRevision::GetLock", trace.WithAttributes())
330+
defer span.End()
329331
return repository.KptUpstreamLock2KptUpstream(pr.extPRID), pr.extPRID, nil
330332
}
331333

‎pkg/cache/dbcache/dbpackagerevision_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,7 @@ func (t *DbTestSuite) TestDBPackageRevision() {
104104
t.Require().Nil(newPrUp.Git)
105105
t.Require().Nil(newPrUpLock.Git)
106106

107-
newPrUp, newPrUpLock, err = dbPR.GetLock()
107+
newPrUp, newPrUpLock, err = dbPR.GetLock(ctx)
108108
t.Require().NoError(err)
109109
t.Require().NotNil(newPrUp.Git)
110110
t.Require().NotNil(newPrUpLock.Git)

‎pkg/cache/dbcache/dbreposync.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -228,7 +228,7 @@ func (s *repositorySync) cacheExternalPRs(ctx context.Context, externalPrMap map
228228
extAPIPR.CreationTimestamp.Time = time.Now()
229229
}
230230

231-
_, extPRUpstreamLock, _ := extPR.GetLock()
231+
_, extPRUpstreamLock, _ := extPR.GetLock(ctx)
232232

233233
dbPR := dbPackageRevision{
234234
repo: s.repo,

‎pkg/engine/pushpr.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ func PushPackageRevision(ctx context.Context, repo repository.Repository, pr rep
6868
return v1.UpstreamLock{}, pkgerrors.Wrapf(err, "push of package revision %+v to repository %+v failed, could not close package revision draft:", pr.Key(), repo.Key())
6969
}
7070

71-
_, pushedPRUpstreamLock, err := pushedPR.GetLock()
71+
_, pushedPRUpstreamLock, err := pushedPR.GetLock(ctx)
7272
if err != nil {
7373
return v1.UpstreamLock{}, pkgerrors.Wrapf(err, "read of upstream lock for package revision %+v pushed to repository %+v failed", pr.Key(), repo.Key())
7474
}

‎pkg/engine/pushpr_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -71,11 +71,11 @@ func TestPushPR(t *testing.T) {
7171
assert.NotNil(t, err)
7272

7373
mockRepo.EXPECT().ClosePackageRevisionDraft(mock.Anything, mock.Anything, mock.Anything).Return(mockPR, nil).Maybe()
74-
mockPR.EXPECT().GetLock().Return(v1.Upstream{}, v1.UpstreamLock{}, err).Once()
74+
mockPR.EXPECT().GetLock(mock.Anything).Return(v1.Upstream{}, v1.UpstreamLock{}, err).Once()
7575
_, err = PushPackageRevision(ctx, mockRepo, mockPR)
7676
assert.NotNil(t, err)
7777

78-
mockPR.EXPECT().GetLock().Return(v1.Upstream{}, v1.UpstreamLock{}, nil).Maybe()
78+
mockPR.EXPECT().GetLock(mock.Anything).Return(v1.Upstream{}, v1.UpstreamLock{}, nil).Maybe()
7979
_, err = PushPackageRevision(ctx, mockRepo, mockPR)
8080
assert.Nil(t, err)
8181
}

‎pkg/externalrepo/fake/packagerevision.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,7 @@ func (fpr *FakePackageRevision) GetUpstreamLock(context.Context) (kptfile.Upstre
9898
return *fpr.Kptfile.Upstream, *fpr.Kptfile.UpstreamLock, fpr.Err
9999
}
100100

101-
func (fpr *FakePackageRevision) GetLock() (kptfile.Upstream, kptfile.UpstreamLock, error) {
101+
func (fpr *FakePackageRevision) GetLock(ctx context.Context) (kptfile.Upstream, kptfile.UpstreamLock, error) {
102102
fpr.Ops = append(fpr.Ops, "GetLock")
103103
return *fpr.Kptfile.Upstream, *fpr.Kptfile.UpstreamLock, fpr.Err
104104
}

‎pkg/externalrepo/git/git.go‎

Lines changed: 25 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -267,7 +267,7 @@ type gitRepository struct {
267267
// through all the refs each time
268268
deletionProposedCache map[BranchName]bool
269269

270-
mutex sync.Mutex
270+
mutex sync.RWMutex
271271

272272
// caBundle to use for TLS communication towards git
273273
caBundle []byte
@@ -298,13 +298,17 @@ func (r *gitRepository) Close(context.Context) error {
298298
func (r *gitRepository) Version(ctx context.Context) (string, error) {
299299
_, span := tracer.Start(ctx, "gitRepository::Version", trace.WithAttributes())
300300
defer span.End()
301-
r.mutex.Lock()
302-
defer r.mutex.Unlock()
303301

304-
if err := r.fetchRemoteRepositoryWithRetry(ctx); err != nil {
302+
r.mutex.Lock()
303+
err := r.fetchRemoteRepositoryWithRetry(ctx)
304+
r.mutex.Unlock()
305+
if err != nil {
305306
return "", err
306307
}
307308

309+
r.mutex.RLock()
310+
defer r.mutex.RUnlock()
311+
308312
refs, err := r.repo.References()
309313
if err != nil {
310314
return "", err
@@ -444,10 +448,13 @@ func (r *gitRepository) listPackageRevisions(ctx context.Context, filter reposit
444448
}
445449

446450
func (r *gitRepository) CreatePackageRevisionDraft(ctx context.Context, obj *porchapi.PackageRevision) (repository.PackageRevisionDraft, error) {
447-
_, span := tracer.Start(ctx, "gitRepository::CreatePackageRevision", trace.WithAttributes())
451+
_, span := tracer.Start(ctx, "gitRepository::CreatePackageRevisionDraft", trace.WithAttributes())
448452
defer span.End()
449-
r.mutex.Lock()
450-
defer r.mutex.Unlock()
453+
454+
_, mutexSpan := tracer.Start(ctx, "gitRepository::CreatePackageRevisionDraft::acquire_mutex")
455+
r.mutex.RLock()
456+
mutexSpan.End()
457+
defer r.mutex.RUnlock()
451458

452459
var base plumbing.Hash
453460
refName := r.branch.RefInLocal()
@@ -696,8 +703,8 @@ func (r *gitRepository) fetchRemoteRepositoryWithRetry(ctx context.Context) erro
696703
func (r *gitRepository) GetPackageRevision(ctx context.Context, version, path string) (repository.PackageRevision, kptfilev1.GitLock, error) {
697704
ctx, span := tracer.Start(ctx, "gitRepository::GetPackageRevision", trace.WithAttributes())
698705
defer span.End()
699-
r.mutex.Lock()
700-
defer r.mutex.Unlock()
706+
r.mutex.RLock()
707+
defer r.mutex.RUnlock()
701708

702709
var hash plumbing.Hash
703710

@@ -1049,8 +1056,6 @@ func (r *gitRepository) getAuthMethod(ctx context.Context, forceRefresh bool) (t
10491056
}
10501057

10511058
func (r *gitRepository) GetRepo() (string, error) {
1052-
r.mutex.Lock()
1053-
defer r.mutex.Unlock()
10541059

10551060
origin, err := r.repo.Remote("origin")
10561061
if err != nil {
@@ -1341,8 +1346,8 @@ func visitCommitsCollectErrors(iterator object.CommitIter, callback commitCallba
13411346
}
13421347

13431348
func (r *gitRepository) GetResources(hash plumbing.Hash) (map[string]string, error) {
1344-
r.mutex.Lock()
1345-
defer r.mutex.Unlock()
1349+
r.mutex.RLock()
1350+
defer r.mutex.RUnlock()
13461351

13471352
resources := map[string]string{}
13481353

@@ -1407,8 +1412,8 @@ type commitCallback func(*object.Commit) error
14071412
func (r *gitRepository) GetLifecycle(ctx context.Context, pkgRev *gitPackageRevision) porchapi.PackageRevisionLifecycle {
14081413
_, span := tracer.Start(ctx, "gitRepository::GetLifecycle", trace.WithAttributes())
14091414
defer span.End()
1410-
r.mutex.Lock()
1411-
defer r.mutex.Unlock()
1415+
r.mutex.RLock()
1416+
defer r.mutex.RUnlock()
14121417

14131418
return r.getLifecycle(pkgRev)
14141419
}
@@ -1487,7 +1492,10 @@ func (r *gitRepository) UpdateLifecycle(ctx context.Context, pkgRev *gitPackageR
14871492
func (r *gitRepository) UpdateDraftResources(ctx context.Context, draft *gitPackageRevisionDraft, new *porchapi.PackageRevisionResources, change *porchapi.Task) error {
14881493
ctx, span := tracer.Start(ctx, "gitRepository::UpdateResources", trace.WithAttributes())
14891494
defer span.End()
1495+
1496+
_, mutexSpan := tracer.Start(ctx, "gitRepository::UpdateDraftResources::acquire_mutex")
14901497
r.mutex.Lock()
1498+
mutexSpan.End()
14911499
defer r.mutex.Unlock()
14921500

14931501
ch, err := newCommitHelper(r.repo, r.userInfoProvider, draft.commit, draft.Key().PkgKey.ToFullPathname(), plumbing.ZeroHash)
@@ -1548,7 +1556,9 @@ func (r *gitRepository) ClosePackageRevisionDraft(ctx context.Context, prd repos
15481556
ctx, span := tracer.Start(ctx, "gitRepository::ClosePackageRevisionDraft", trace.WithAttributes())
15491557
defer span.End()
15501558

1559+
_, mutexSpan := tracer.Start(ctx, "gitRepository::ClosePackageRevisionDraft::acquire_mutex")
15511560
r.mutex.Lock()
1561+
mutexSpan.End()
15521562
defer r.mutex.Unlock()
15531563

15541564
d := prd.(*gitPackageRevisionDraft)

‎pkg/externalrepo/git/package.go‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -229,7 +229,10 @@ func (p *gitPackageRevision) GetUpstreamLock(ctx context.Context) (kptfile.Upstr
229229
// GetLock returns the self version of the package. Think of it as the Git commit information
230230
// that represent the package revision of this package. Please note that it uses Upstream types
231231
// to represent this information but it has no connection with the associated upstream package (if any).
232-
func (p *gitPackageRevision) GetLock() (kptfile.Upstream, kptfile.UpstreamLock, error) {
232+
func (p *gitPackageRevision) GetLock(ctx context.Context) (kptfile.Upstream, kptfile.UpstreamLock, error) {
233+
_, span := tracer.Start(ctx, "gitPackageRevision::GetLock", trace.WithAttributes())
234+
defer span.End()
235+
233236
repo, err := p.repo.GetRepo()
234237
if err != nil {
235238
return kptfile.Upstream{}, kptfile.UpstreamLock{}, fmt.Errorf("cannot determine package lock: %w", err)

‎pkg/externalrepo/git/package_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ func (g GitSuite) TestLock(t *testing.T) {
7272
continue
7373
}
7474

75-
upstream, lock, err := rev.GetLock()
75+
upstream, lock, err := rev.GetLock(ctx)
7676
if err != nil {
7777
t.Errorf("GetUpstreamLock(%q) failed: %v", rev.Key(), err)
7878
}

‎pkg/externalrepo/git/ref.go‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -45,9 +45,8 @@ const (
4545
proposedPrefixInLocalRepo = branchPrefixInLocalRepo + proposedPrefix
4646
proposedPrefixInRemoteRepo = branchPrefixInRemoteRepo + proposedPrefix
4747

48-
deletionProposedPrefix = "deletionProposed/"
49-
deletionProposedPrefixInLocalRepo = branchPrefixInLocalRepo + deletionProposedPrefix
50-
deletionProposedPrefixInRemoteRepo = branchPrefixInRemoteRepo + deletionProposedPrefix
48+
deletionProposedPrefix = "deletionProposed/"
49+
deletionProposedPrefixInLocalRepo = branchPrefixInLocalRepo + deletionProposedPrefix
5150
)
5251

5352
var (

0 commit comments

Comments
 (0)