Merge pull request nephio-project#129 from Nordix/improve-approve-time

Fixes #812 - Porch approval takes more time with each package in the repository

pkg/cache/memory/cache_test.go

@@ -113,7 +113,7 @@ func TestPublishedLatest(t *testing.T) {
 	if err := update.UpdateLifecycle(ctx, api.PackageRevisionLifecyclePublished); err != nil {
 		t.Fatalf("UpdateLifecycle failed; %v", err)
 	}
-	closed, err := update.Close(ctx)
+	closed, err := update.Close(ctx, "")
 	if err != nil {
 		t.Fatalf("Close failed: %v", err)
 	}
@@ -128,6 +128,94 @@ func TestPublishedLatest(t *testing.T) {
 	}
 }
 
+func TestDeletePublishedMain(t *testing.T) {
+	ctx := context.Background()
+	testPath := filepath.Join("../..", "git", "testdata")
+	cachedRepo := openRepositoryFromArchive(t, ctx, testPath, "nested")
+
+	revisions, err := cachedRepo.ListPackageRevisions(ctx, repository.ListPackageRevisionFilter{
+		Package:       "catalog/gcp/bucket",
+		WorkspaceName: "v2",
+	})
+	if err != nil {
+		t.Fatalf("ListPackageRevisions failed: %v", err)
+	}
+
+	// Expect a single result
+	if got, want := len(revisions), 1; got != want {
+		t.Fatalf("ListPackageRevisions returned %d packages; want %d", got, want)
+	}
+
+	bucket := revisions[0]
+	// Expect draft package
+	if got, want := bucket.Lifecycle(), api.PackageRevisionLifecycleDraft; got != want {
+		t.Fatalf("Bucket package lifecycle: got %s, want %s", got, want)
+	}
+
+	update, err := cachedRepo.UpdatePackageRevision(ctx, bucket)
+	if err != nil {
+		t.Fatalf("UpdatePackage(%s) failed: %v", bucket.Key(), err)
+	}
+	if err := update.UpdateLifecycle(ctx, api.PackageRevisionLifecyclePublished); err != nil {
+		t.Fatalf("UpdateLifecycle failed; %v", err)
+	}
+	closed, err := update.Close(ctx, "")
+	if err != nil {
+		t.Fatalf("Close failed: %v", err)
+	}
+	_, err = closed.GetPackageRevision(ctx)
+	if err != nil {
+		t.Errorf("didn't expect error, but got %v", err)
+	}
+
+	publishedRevisions, err := cachedRepo.ListPackageRevisions(ctx, repository.ListPackageRevisionFilter{
+		Package:       "catalog/gcp/bucket",
+		WorkspaceName: "v2",
+		Lifecycle:     api.PackageRevisionLifecyclePublished,
+		Revision:      "main",
+	})
+	if err != nil {
+		t.Fatalf("ListPackageRevisions failed: %v", err)
+	}
+
+	// Expect a single result
+	if got, want := len(publishedRevisions), 1; got != want {
+		t.Fatalf("ListPackageRevisions returned %d packages; want %d", got, want)
+	}
+
+	approvedBucket := publishedRevisions[0]
+
+	if got, want := approvedBucket.Lifecycle(), api.PackageRevisionLifecyclePublished; got != want {
+		t.Fatalf("Approved Bucket package lifecycle: got %s, want %s", got, want)
+	}
+
+	err = approvedBucket.UpdateLifecycle(ctx, api.PackageRevisionLifecycleDeletionProposed)
+	if err != nil {
+		t.Fatalf("Deletion proposal for approved Bucket failed; %v", err)
+	}
+	err = cachedRepo.DeletePackageRevision(ctx, approvedBucket)
+	if err != nil {
+		t.Fatalf("Deleting Main packageRevision failed; %v", err)
+	}
+
+	postDeletePublishedRevisions, err := cachedRepo.ListPackageRevisions(ctx, repository.ListPackageRevisionFilter{
+		Package:       "catalog/gcp/bucket",
+		WorkspaceName: "v2",
+		Lifecycle:     api.PackageRevisionLifecyclePublished,
+		Revision:      "main",
+	})
+
+	if err != nil {
+		t.Fatalf("ListPackageRevisions failed: %v", err)
+	}
+
+	//Expect 0 entries
+	if got, want := len(postDeletePublishedRevisions), 0; got != want {
+		t.Fatalf("ListPackageRevisions returned %d packages; want %d", got, want)
+	}
+
+}
+
 func openRepositoryFromArchive(t *testing.T, ctx context.Context, testPath, name string) cache.CachedRepository {
 	t.Helper()
 

pkg/cache/memory/draft.go

@@ -17,8 +17,10 @@ package memory
 import (
 	"context"
 
+	"github.com/nephio-project/porch/api/porch/v1alpha1"
 	"github.com/nephio-project/porch/pkg/cache"
 	"github.com/nephio-project/porch/pkg/repository"
+	"go.opentelemetry.io/otel/trace"
 )
 
 type cachedDraft struct {
@@ -29,8 +31,40 @@ type cachedDraft struct {
 var _ repository.PackageDraft = &cachedDraft{}
 var _ cache.CachedPackageDraft = &cachedDraft{}
 
-func (cd *cachedDraft) Close(ctx context.Context) (repository.PackageRevision, error) {
-	if closed, err := cd.PackageDraft.Close(ctx); err != nil {
+func (cd *cachedDraft) Close(ctx context.Context, version string) (repository.PackageRevision, error) {
+	ctx, span := tracer.Start(ctx, "cachedDraft::Close", trace.WithAttributes())
+	defer span.End()
+	v, err := cd.cache.Version(ctx)
+	if err != nil {
+		return nil, err
+	}
+	if v != cd.cache.lastVersion {
+		_, _, err = cd.cache.refreshAllCachedPackages(ctx)
+		if err != nil {
+			return nil, err
+		}
+	}
+
+	revisions, err := cd.cache.ListPackageRevisions(ctx, repository.ListPackageRevisionFilter{
+		Package: cd.GetName(),
+	})
+	if err != nil {
+		return nil, err
+	}
+
+	var publishedRevisions []string
+	for _, rev := range revisions {
+		if v1alpha1.LifecycleIsPublished(rev.Lifecycle()) {
+			publishedRevisions = append(publishedRevisions, rev.Key().Revision)
+		}
+	}
+
+	nextVersion, err := repository.NextRevisionNumber(publishedRevisions)
+	if err != nil {
+		return nil, err
+	}
+
+	if closed, err := cd.PackageDraft.Close(ctx, nextVersion); err != nil {
 		return nil, err
 	} else {
 		return cd.cache.update(ctx, closed)

pkg/cache/memory/repository.go

@@ -28,6 +28,7 @@ import (
 	"github.com/nephio-project/porch/pkg/repository"
 	"go.opentelemetry.io/otel"
 	"go.opentelemetry.io/otel/trace"
+	"k8s.io/apimachinery/pkg/api/errors"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/watch"
@@ -222,13 +223,67 @@ func (r *cachedRepository) update(ctx context.Context, updated repository.Packag
 	r.cachedPackageRevisions[k] = cached
 
 	// Recompute latest package revisions.
-	// TODO: Just updated package?
 	identifyLatestRevisions(r.cachedPackageRevisions)
 
-	// TODO: Update the latest revisions for the r.cachedPackages
+	// Create the main package revision
+	if v1alpha1.LifecycleIsPublished(updated.Lifecycle()) {
+		updatedMain := updated.ToMainPackageRevision()
+		r.createMainPackageRevision(ctx, updatedMain)
+	} else {
+		version, err := r.repo.Version(ctx)
+		if err != nil {
+			return nil, err
+		}
+		r.lastVersion = version
+	}
+
 	return cached, nil
 }
 
+func (r *cachedRepository) createMainPackageRevision(ctx context.Context, updatedMain repository.PackageRevision) error {
+
+	//Search and delete any old main pkgRev of an older workspace in the cache
+	for pkgRevKey := range r.cachedPackageRevisions {
+		if (pkgRevKey.Repository == updatedMain.Key().Repository) && (pkgRevKey.Package == updatedMain.Key().Package) && (pkgRevKey.Revision == updatedMain.Key().Revision) {
+			oldMainKey := repository.PackageRevisionKey{
+				Repository:    updatedMain.Key().Repository,
+				Package:       updatedMain.Key().Package,
+				Revision:      updatedMain.Key().Revision,
+				WorkspaceName: v1alpha1.WorkspaceName(string(pkgRevKey.WorkspaceName)),
+			}
+			delete(r.cachedPackageRevisions, oldMainKey)
+		}
+	}
+	cachedMain := &cachedPackageRevision{PackageRevision: updatedMain}
+	r.cachedPackageRevisions[updatedMain.Key()] = cachedMain
+
+	pkgRevMetaNN := types.NamespacedName{
+		Name:      updatedMain.KubeObjectName(),
+		Namespace: updatedMain.KubeObjectNamespace(),
+	}
+
+	// Create the package if it doesn't exist
+	_, err := r.metadataStore.Get(ctx, pkgRevMetaNN)
+	if errors.IsNotFound(err) {
+		pkgRevMeta := meta.PackageRevisionMeta{
+			Name:      updatedMain.KubeObjectName(),
+			Namespace: updatedMain.KubeObjectNamespace(),
+		}
+		_, err := r.metadataStore.Create(ctx, pkgRevMeta, r.repoSpec.Name, updatedMain.UID())
+		if err != nil {
+			klog.Warningf("unable to create PackageRev CR for %s/%s: %v",
+				updatedMain.KubeObjectNamespace(), updatedMain.KubeObjectName(), err)
+		}
+	}
+	version, err := r.repo.Version(ctx)
+	if err != nil {
+		return err
+	}
+	r.lastVersion = version
+
+	return nil
+}
+
 func (r *cachedRepository) DeletePackageRevision(ctx context.Context, old repository.PackageRevision) error {
 	// Unwrap
 	unwrapped := old.(*cachedPackageRevision).PackageRevision

pkg/engine/engine.go

@@ -65,7 +65,7 @@ type CaDEngine interface {
 
 	ListPackageRevisions(ctx context.Context, repositorySpec *configapi.Repository, filter repository.ListPackageRevisionFilter) ([]*PackageRevision, error)
 	CreatePackageRevision(ctx context.Context, repositoryObj *configapi.Repository, obj *api.PackageRevision, parent *PackageRevision) (*PackageRevision, error)
-	UpdatePackageRevision(ctx context.Context, repositoryObj *configapi.Repository, oldPackage *PackageRevision, old, new *api.PackageRevision, parent *PackageRevision) (*PackageRevision, error)
+	UpdatePackageRevision(ctx context.Context, version string, repositoryObj *configapi.Repository, oldPackage *PackageRevision, old, new *api.PackageRevision, parent *PackageRevision) (*PackageRevision, error)
 	DeletePackageRevision(ctx context.Context, repositoryObj *configapi.Repository, obj *PackageRevision) error
 
 	ListPackages(ctx context.Context, repositorySpec *configapi.Repository, filter repository.ListPackageFilter) ([]*Package, error)
@@ -313,7 +313,7 @@ func (cad *cadEngine) CreatePackageRevision(ctx context.Context, repositoryObj *
 	}
 
 	// Updates are done.
-	repoPkgRev, err := draft.Close(ctx)
+	repoPkgRev, err := draft.Close(ctx, "")
 	if err != nil {
 		return nil, err
 	}
@@ -507,7 +507,7 @@ func (cad *cadEngine) mapTaskToMutation(ctx context.Context, obj *api.PackageRev
 	}
 }
 
-func (cad *cadEngine) UpdatePackageRevision(ctx context.Context, repositoryObj *configapi.Repository, oldPackage *PackageRevision, oldObj, newObj *api.PackageRevision, parent *PackageRevision) (*PackageRevision, error) {
+func (cad *cadEngine) UpdatePackageRevision(ctx context.Context, version string, repositoryObj *configapi.Repository, oldPackage *PackageRevision, oldObj, newObj *api.PackageRevision, parent *PackageRevision) (*PackageRevision, error) {
 	ctx, span := tracer.Start(ctx, "cadEngine::UpdatePackageRevision", trace.WithAttributes())
 	defer span.End()
 
@@ -580,7 +580,7 @@ func (cad *cadEngine) UpdatePackageRevision(ctx context.Context, repositoryObj *
 		if err != nil {
 			return nil, err
 		}
-		repoPkgRev, err := cad.recloneAndReplay(ctx, repo, repositoryObj, newObj, packageConfig)
+		repoPkgRev, err := cad.recloneAndReplay(ctx, version, repo, repositoryObj, newObj, packageConfig)
 		if err != nil {
 			return nil, err
 		}
@@ -681,7 +681,7 @@ func (cad *cadEngine) UpdatePackageRevision(ctx context.Context, repositoryObj *
 	}
 
 	// Updates are done.
-	repoPkgRev, err = draft.Close(ctx)
+	repoPkgRev, err = draft.Close(ctx, version)
 	if err != nil {
 		return nil, err
 	}
@@ -694,14 +694,6 @@ func (cad *cadEngine) UpdatePackageRevision(ctx context.Context, repositoryObj *
 	sent := cad.watcherManager.NotifyPackageRevisionChange(watch.Modified, repoPkgRev, pkgRevMeta)
 	klog.Infof("engine: sent %d for updated PackageRevision %s/%s", sent, repoPkgRev.KubeObjectNamespace(), repoPkgRev.KubeObjectName())
 
-	// Refresh Cache after package is approved so that 'main' package rev is available instantly after creation
-	if repoPkgRev.Lifecycle() == api.PackageRevisionLifecyclePublished {
-		err := repo.RefreshCache(ctx)
-		if err != nil {
-			return nil, err
-		}
-	}
-
 	return ToPackageRevision(repoPkgRev, pkgRevMeta), nil
 }
 
@@ -1023,7 +1015,7 @@ func (cad *cadEngine) UpdatePackageResources(ctx context.Context, repositoryObj
 		}})
 
 	// No lifecycle change when updating package resources; updates are done.
-	repoPkgRev, err := draft.Close(ctx)
+	repoPkgRev, err := draft.Close(ctx, "")
 	if err != nil {
 		return nil, renderStatus, err
 	}
@@ -1354,7 +1346,7 @@ func isRecloneAndReplay(oldObj, newObj *api.PackageRevision) bool {
 
 // recloneAndReplay performs an update by recloning the upstream package and replaying all tasks.
 // This is more like a git rebase operation than the "classic" kpt update algorithm, which is more like a git merge.
-func (cad *cadEngine) recloneAndReplay(ctx context.Context, repo repository.Repository, repositoryObj *configapi.Repository, newObj *api.PackageRevision, packageConfig *builtins.PackageConfig) (repository.PackageRevision, error) {
+func (cad *cadEngine) recloneAndReplay(ctx context.Context, version string, repo repository.Repository, repositoryObj *configapi.Repository, newObj *api.PackageRevision, packageConfig *builtins.PackageConfig) (repository.PackageRevision, error) {
 	ctx, span := tracer.Start(ctx, "cadEngine::recloneAndReplay", trace.WithAttributes())
 	defer span.End()
 
@@ -1373,7 +1365,7 @@ func (cad *cadEngine) recloneAndReplay(ctx context.Context, repo repository.Repo
 		return nil, err
 	}
 
-	return draft.Close(ctx)
+	return draft.Close(ctx, version)
 }
 
 // ExtractContextConfigMap returns the package-context configmap, if found

pkg/engine/fake/packagerevision.go

@@ -39,6 +39,13 @@ func (pr *PackageRevision) KubeObjectName() string {
 	return pr.Name
 }
 
+var _ repository.PackageRevision = &PackageRevision{}
+
+// ToMainPackageRevision implements repository.PackageRevision.
+func (f *PackageRevision) ToMainPackageRevision() repository.PackageRevision {
+	panic("unimplemented")
+}
+
 func (pr *PackageRevision) KubeObjectNamespace() string {
 	return pr.Namespace
 }

pkg/git/draft.go

@@ -16,6 +16,7 @@ package git
 
 import (
 	"context"
+	"strings"
 	"time"
 
 	"github.com/go-git/go-git/v5/plumbing"
@@ -63,9 +64,15 @@ func (d *gitPackageDraft) UpdateLifecycle(ctx context.Context, new v1alpha1.Pack
 }
 
 // Finish round of updates.
-func (d *gitPackageDraft) Close(ctx context.Context) (repository.PackageRevision, error) {
+func (d *gitPackageDraft) Close(ctx context.Context, version string) (repository.PackageRevision, error) {
 	ctx, span := tracer.Start(ctx, "gitPackageDraft::Close", trace.WithAttributes())
 	defer span.End()
 
-	return d.parent.CloseDraft(ctx, d)
+	return d.parent.CloseDraft(ctx, version, d)
+}
+
+func (d *gitPackageDraft) GetName() string {
+	packageDirectory := d.parent.directory
+	packageName := strings.TrimPrefix(d.path, packageDirectory+"/")
+	return packageName
 }