Revert mutex changes in git.go and cr-cache (kptdev#372)

Author: kushnaidu

pkg/cache/crcache/cache.go

@@ -56,10 +56,10 @@ func (c *Cache) OpenRepository(ctx context.Context, repositorySpec *configapi.Re
 	lock.Lock()
 	defer lock.Unlock()
 
-	c.mainLock.Lock()
-	defer c.mainLock.Unlock()
+	c.mainLock.RLock()
 
 	if repo, ok := c.repositories[key]; ok && repo != nil {
+		c.mainLock.RUnlock()
 		// Keep the spec updated in the cache.
 		repo.repoSpec = repositorySpec
 		// Check external repo connectivity
@@ -74,6 +74,7 @@ func (c *Cache) OpenRepository(ctx context.Context, repositorySpec *configapi.Re
 		}
 		return repo, nil
 	}
+	c.mainLock.RUnlock()
 
 	externalRepo, err := externalrepo.CreateRepositoryImpl(ctx, repositorySpec, c.options.ExternalRepoOptions)
 	if err != nil {
@@ -82,7 +83,9 @@ func (c *Cache) OpenRepository(ctx context.Context, repositorySpec *configapi.Re
 
 	cachedRepo := newRepository(key, repositorySpec, externalRepo, c.metadataStore, c.options)
 
+	c.mainLock.Lock()
 	c.repositories[key] = cachedRepo
+	c.mainLock.Unlock()
 
 	return cachedRepo, nil
 }

pkg/externalrepo/git/git.go

@@ -300,15 +300,12 @@ func (r *gitRepository) Version(ctx context.Context) (string, error) {
 	defer span.End()
 
 	r.mutex.Lock()
-	err := r.fetchRemoteRepositoryWithRetry(ctx)
-	r.mutex.Unlock()
-	if err != nil {
+	defer r.mutex.Unlock()
+
+	if err := r.fetchRemoteRepositoryWithRetry(ctx); err != nil {
 		return "", err
 	}
 
-	r.mutex.RLock()
-	defer r.mutex.RUnlock()
-
 	refs, err := r.repo.References()
 	if err != nil {
 		return "", err
@@ -452,9 +449,9 @@ func (r *gitRepository) CreatePackageRevisionDraft(ctx context.Context, obj *por
 	defer span.End()
 
 	_, mutexSpan := tracer.Start(ctx, "gitRepository::CreatePackageRevisionDraft::acquire_mutex")
-	r.mutex.RLock()
+	r.mutex.Lock()
 	mutexSpan.End()
-	defer r.mutex.RUnlock()
+	defer r.mutex.Unlock()
 
 	var base plumbing.Hash
 	refName := r.branch.RefInLocal()
@@ -703,8 +700,8 @@ func (r *gitRepository) fetchRemoteRepositoryWithRetry(ctx context.Context) erro
 func (r *gitRepository) GetPackageRevision(ctx context.Context, version, path string) (repository.PackageRevision, kptfilev1.GitLock, error) {
 	ctx, span := tracer.Start(ctx, "gitRepository::GetPackageRevision", trace.WithAttributes())
 	defer span.End()
-	r.mutex.RLock()
-	defer r.mutex.RUnlock()
+	r.mutex.Lock()
+	defer r.mutex.Unlock()
 
 	var hash plumbing.Hash
 
@@ -1056,6 +1053,8 @@ func (r *gitRepository) getAuthMethod(ctx context.Context, forceRefresh bool) (t
 }
 
 func (r *gitRepository) GetRepo() (string, error) {
+	r.mutex.Lock()
+	defer r.mutex.Unlock()
 
 	origin, err := r.repo.Remote("origin")
 	if err != nil {
@@ -1346,8 +1345,8 @@ func visitCommitsCollectErrors(iterator object.CommitIter, callback commitCallba
 }
 
 func (r *gitRepository) GetResources(hash plumbing.Hash) (map[string]string, error) {
-	r.mutex.RLock()
-	defer r.mutex.RUnlock()
+	r.mutex.Lock()
+	defer r.mutex.Unlock()
 
 	resources := map[string]string{}
 
@@ -1412,8 +1411,8 @@ type commitCallback func(*object.Commit) error
 func (r *gitRepository) GetLifecycle(ctx context.Context, pkgRev *gitPackageRevision) porchapi.PackageRevisionLifecycle {
 	_, span := tracer.Start(ctx, "gitRepository::GetLifecycle", trace.WithAttributes())
 	defer span.End()
-	r.mutex.RLock()
-	defer r.mutex.RUnlock()
+	r.mutex.Lock()
+	defer r.mutex.Unlock()
 
 	return r.getLifecycle(pkgRev)
 }

test/e2e/e2e_test.go

@@ -1665,7 +1665,7 @@ func (t *PorchSuite) TestPackageUpgrade() {
 	)
 
 	t.RegisterGitRepositoryF(t.GetTestBlueprintsRepoURL(), TestBlueprintsRepoName, "", GiteaUser, GiteaPassword)
-	
+
 	// Register the repository as 'downstream'
 	t.RegisterGitRepositoryF(t.GetPorchTestRepoURL(), gitRepository, "", GiteaUser, GiteaPassword)
 
@@ -1811,11 +1811,10 @@ func (t *PorchSuite) TestRegisterRepository() {
 	const (
 		repository = "register"
 	)
-	t.RegisterGitRepositoryF(t.GetPorchTestRepoURL(), repository, "", GiteaUser, 
-	GiteaPassword, RepositoryOptions{RepOpts: withType(configapi.RepositoryTypeGit)},
+	t.RegisterGitRepositoryF(t.GetPorchTestRepoURL(), repository, "", GiteaUser,
+		GiteaPassword, RepositoryOptions{RepOpts: withType(configapi.RepositoryTypeGit)},
 		RepositoryOptions{RepOpts: WithDeployment()})
 
-
 	var repo configapi.Repository
 	t.GetF(client.ObjectKey{
 		Namespace: t.Namespace,
@@ -2309,7 +2308,7 @@ func (t *PorchSuite) TestLargePackageRevision() {
 	const testDataSize = 5 * 1024 * 1024
 
 	setAnnotationsImage := t.GcrPrefix + "/set-annotations:v0.1.5" // set-annotations:v0.1.5 is an older version that porch maps neither to built-in nor exec.
-	
+
 	t.RegisterGitRepositoryF(t.GetPorchTestRepoURL(), "git-fn-pod-large", "", GiteaUser, GiteaPassword)
 
 	// Create Package Revision
@@ -2780,7 +2779,7 @@ func (t *PorchSuite) TestPackageRevisionGCWithOwner() {
 }
 
 func (t *PorchSuite) TestPackageRevisionGCAsOwner() {
-	// TODO: Garbage collection is not working when a DB cache PackageRevision resource owner is deleted. 
+	// TODO: Garbage collection is not working when a DB cache PackageRevision resource owner is deleted.
 	// We need to get this test running in DB cache
 	if _, ok := os.LookupEnv("DB_CACHE"); ok {
 		return
@@ -2957,7 +2956,7 @@ func (t *PorchSuite) TestPackageRevisionFinalizers() {
 func (t *PorchSuite) TestPackageRevisionInMultipleNamespaces() {
 
 	registerRepoAndTestRevisions := func(repoName string, ns string, oldPRs []porchapi.PackageRevision) []porchapi.PackageRevision {
-		t.RegisterGitRepositoryF(t.GetTestBlueprintsRepoURL(), repoName, "", GiteaUser, GiteaPassword, 
+		t.RegisterGitRepositoryF(t.GetTestBlueprintsRepoURL(), repoName, "", GiteaUser, GiteaPassword,
 			RepositoryOptions{RepOpts: InNamespace(ns), SecOpts: SecretInNamespace(ns)})
 		prList := porchapi.PackageRevisionList{}
 		t.ListF(&prList, client.InNamespace(ns))
@@ -3269,6 +3268,11 @@ func (t *PorchSuite) TestRepositoryModify() {
 			continue
 		}
 
+		if ready.Status == metav1.ConditionFalse && ready.Reason == configapi.ReasonReconciling {
+			t.Logf("Repository sync in progress")
+			continue
+		}
+
 		if got, want := ready.Status, metav1.ConditionTrue; got != want {
 			t.Errorf("Repository Ready Condition Status; got %q, want %q", got, want)
 		}
@@ -3497,7 +3501,7 @@ func (t *PorchSuite) TestPackageRevisionListWithTwoHangingRepositories() {
 			Description: "Working Git repository",
 			Type:        configapi.RepositoryTypeGit,
 			Git: &configapi.GitRepository{
-				Repo:      t.GetPorchTestRepoURL(),
+				Repo: t.GetPorchTestRepoURL(),
 				SecretRef: configapi.SecretRef{
 					Name: t.CreateOrUpdateSecret(workingRepoName, GiteaUser, GiteaPassword),
 				},