From 4c96cebf82fba35b9ddcdf2c29ee6c29081a5347 Mon Sep 17 00:00:00 2001 From: Kushal Harish Naidu Date: Thu, 24 Oct 2024 12:28:43 +0000 Subject: [PATCH 01/12] Improve approve time - commit 1 --- pkg/cache/cache_test.go | 2 +- pkg/cache/draft.go | 40 +++++++++++++++++++++++-- pkg/cache/repository.go | 45 +++++++++++++++++++++++++++++ pkg/engine/engine.go | 16 +++++----- pkg/engine/fake/packagerevision.go | 7 +++++ pkg/git/draft.go | 8 +++-- pkg/git/git.go | 21 ++------------ pkg/git/git_test.go | 12 ++++---- pkg/git/package.go | 18 ++++++++++++ pkg/oci/mutate.go | 6 +++- pkg/oci/oci.go | 5 ++++ pkg/registry/porch/packagecommon.go | 2 +- pkg/repository/repository.go | 5 +++- 13 files changed, 146 insertions(+), 41 deletions(-) diff --git a/pkg/cache/cache_test.go b/pkg/cache/cache_test.go index d663d502..5418f253 100644 --- a/pkg/cache/cache_test.go +++ b/pkg/cache/cache_test.go @@ -111,7 +111,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) } diff --git a/pkg/cache/draft.go b/pkg/cache/draft.go index cb8e5bca..d8d25955 100644 --- a/pkg/cache/draft.go +++ b/pkg/cache/draft.go @@ -17,7 +17,10 @@ package cache import ( "context" + "github.com/nephio-project/porch/api/porch/v1alpha1" "github.com/nephio-project/porch/pkg/repository" + "go.opentelemetry.io/otel/trace" + "k8s.io/klog/v2" ) type cachedDraft struct { @@ -27,8 +30,41 @@ type cachedDraft struct { var _ repository.PackageDraft = &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 + } + klog.Infof("Pkgrev version mismatch in cache and repo - Refreshing cache.") + } + + revisions, err := cd.cache.ListPackageRevisions(ctx, repository.ListPackageRevisionFilter{ + Package: cd.GetName(), + }) + if err != nil { + return nil, err + } + + var revs []string + for _, rev := range revisions { + if v1alpha1.LifecycleIsPublished(rev.Lifecycle()) { + revs = append(revs, rev.Key().Revision) + } + } + + v, err = repository.NextRevisionNumber(revs) + if err != nil { + return nil, err + } + + if closed, err := cd.PackageDraft.Close(ctx, v); err != nil { return nil, err } else { return cd.cache.update(ctx, closed) diff --git a/pkg/cache/repository.go b/pkg/cache/repository.go index 80649934..93fd6723 100644 --- a/pkg/cache/repository.go +++ b/pkg/cache/repository.go @@ -27,6 +27,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" @@ -264,6 +265,50 @@ func (r *cachedRepository) update(ctx context.Context, updated repository.Packag // TODO: Just updated package? identifyLatestRevisions(r.cachedPackageRevisions) + if (updated.Lifecycle() == (v1alpha1.PackageRevisionLifecycleProposed)) || (updated.Lifecycle() == v1alpha1.PackageRevisionLifecycleDraft) { + version, err := r.repo.Version(ctx) + if err != nil { + return nil, err + } + r.lastVersion = version + } + + if updated.Lifecycle() == (v1alpha1.PackageRevisionLifecyclePublished) { + updatedMain := updated.ToMainPackageRevision() + oldMainKey := repository.PackageRevisionKey{ + Repository: updatedMain.Key().Repository, + Package: updatedMain.Key().Package, + Revision: updatedMain.Key().Revision, + } + delete(r.cachedPackageRevisions, oldMainKey) + cachedMain := &cachedPackageRevision{PackageRevision: updatedMain} + r.cachedPackageRevisions[updatedMain.Key()] = cachedMain + + pkgRevMetaNN := types.NamespacedName{ + Name: updatedMain.KubeObjectName(), + Namespace: updatedMain.KubeObjectNamespace(), + } + // TODO: error handling for metadataStore.Get + _, err := r.metadataStore.Get(ctx, pkgRevMetaNN) + // Error can be different + 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 nil, err + } + r.lastVersion = version + } + // TODO: Update the latest revisions for the r.cachedPackages return cached, nil } diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go index b1898949..357c0f65 100644 --- a/pkg/engine/engine.go +++ b/pkg/engine/engine.go @@ -66,7 +66,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) @@ -326,7 +326,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 } @@ -520,7 +520,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() @@ -593,7 +593,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 } @@ -694,7 +694,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 } @@ -1036,7 +1036,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 } @@ -1391,7 +1391,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() @@ -1410,7 +1410,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 diff --git a/pkg/engine/fake/packagerevision.go b/pkg/engine/fake/packagerevision.go index 697e9614..713c0b99 100644 --- a/pkg/engine/fake/packagerevision.go +++ b/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 } diff --git a/pkg/git/draft.go b/pkg/git/draft.go index f5fa62ea..04c71542 100644 --- a/pkg/git/draft.go +++ b/pkg/git/draft.go @@ -63,9 +63,13 @@ 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 { + return d.path } diff --git a/pkg/git/git.go b/pkg/git/git.go index 0276c23d..ea796438 100644 --- a/pkg/git/git.go +++ b/pkg/git/git.go @@ -1480,7 +1480,7 @@ func (r *gitRepository) UpdateDraftResources(ctx context.Context, draft *gitPack return nil } -func (r *gitRepository) CloseDraft(ctx context.Context, d *gitPackageDraft) (*gitPackageRevision, error) { +func (r *gitRepository) CloseDraft(ctx context.Context, version string, d *gitPackageDraft) (*gitPackageRevision, error) { r.mutex.Lock() defer r.mutex.Unlock() @@ -1492,25 +1492,8 @@ func (r *gitRepository) CloseDraft(ctx context.Context, d *gitPackageDraft) (*gi switch d.lifecycle { case v1alpha1.PackageRevisionLifecyclePublished, v1alpha1.PackageRevisionLifecycleDeletionProposed: - // Finalize the package revision. Assign it a revision number of latest + 1. - revisions, err := r.listPackageRevisions(ctx, repository.ListPackageRevisionFilter{ - Package: d.path, - }) - if err != nil { - return nil, err - } - - var revs []string - for _, rev := range revisions { - if v1alpha1.LifecycleIsPublished(r.getLifecycle(ctx, rev)) { - revs = append(revs, rev.Key().Revision) - } - } - d.revision, err = repository.NextRevisionNumber(revs) - if err != nil { - return nil, err - } + d.revision = version // Finalize the package revision. Commit it to main branch. commitHash, newTreeHash, commitBase, err := r.commitPackageToMain(ctx, d) diff --git a/pkg/git/git_test.go b/pkg/git/git_test.go index 46c315cb..4684fe1c 100644 --- a/pkg/git/git_test.go +++ b/pkg/git/git_test.go @@ -180,7 +180,7 @@ func (g GitSuite) TestGitPackageRoundTrip(t *testing.T) { t.Fatalf("draft.UpdateResources(%#v, %#v) failed: %v", newResources, task, err) } - revision, err := draft.Close(ctx) + revision, err := draft.Close(ctx, "") if err != nil { t.Fatalf("draft.Close() failed: %v", err) } @@ -207,7 +207,7 @@ func (g GitSuite) TestGitPackageRoundTrip(t *testing.T) { if err := update.UpdateLifecycle(ctx, v1alpha1.PackageRevisionLifecyclePublished); err != nil { t.Fatalf("UpdateLifecycle failed: %v", err) } - approved, err := update.Close(ctx) + approved, err := update.Close(ctx, "") if err != nil { t.Fatalf("Close() of %q, %q failed: %v", packageName, workspace, err) } @@ -418,7 +418,7 @@ func (g GitSuite) TestListPackagesTrivial(t *testing.T) { }); err != nil { t.Fatalf("UpdateResources() failed: %v", err) } - newRevision, err := draft.Close(ctx) + newRevision, err := draft.Close(ctx, "") if err != nil { t.Fatalf("draft.Close() failed: %v", err) } @@ -506,7 +506,7 @@ func (g GitSuite) TestCreatePackageInTrivialRepository(t *testing.T) { }); err != nil { t.Fatalf("UpdateResources() failed: %v", err) } - newRevision, err := draft.Close(ctx) + newRevision, err := draft.Close(ctx, "") if err != nil { t.Fatalf("draft.Close() failed: %v", err) } @@ -688,7 +688,7 @@ func (g GitSuite) TestApproveDraft(t *testing.T) { update.UpdateLifecycle(ctx, v1alpha1.PackageRevisionLifecyclePublished) - new, err := update.Close(ctx) + new, err := update.Close(ctx, "") if err != nil { t.Fatalf("Close failed: %v", err) } @@ -750,7 +750,7 @@ func (g GitSuite) TestApproveDraftWithHistory(t *testing.T) { update.UpdateLifecycle(ctx, v1alpha1.PackageRevisionLifecyclePublished) - new, err := update.Close(ctx) + new, err := update.Close(ctx, "") if err != nil { t.Fatalf("Close failed: %v", err) } diff --git a/pkg/git/package.go b/pkg/git/package.go index b23c05f0..425d6ca4 100644 --- a/pkg/git/package.go +++ b/pkg/git/package.go @@ -219,6 +219,24 @@ func (p *gitPackageRevision) GetResources(ctx context.Context) (*v1alpha1.Packag }, nil } +// Workspace name needs to be changed as well. +func (p *gitPackageRevision) ToMainPackageRevision() repository.PackageRevision { + p1 := &gitPackageRevision{ + repo: p.repo, + path: p.path, + revision: string(p.repo.branch), + workspaceName: p.workspaceName, + updated: p.updated, + updatedBy: p.updatedBy, + ref: p.ref, + tree: p.tree, + commit: p.commit, + tasks: p.tasks, + } + return p1 + +} + func (p *gitPackageRevision) GetKptfile(ctx context.Context) (kptfile.KptFile, error) { resources, err := p.repo.GetResources(p.tree) if err != nil { diff --git a/pkg/oci/mutate.go b/pkg/oci/mutate.go index e25b9716..adbf9719 100644 --- a/pkg/oci/mutate.go +++ b/pkg/oci/mutate.go @@ -195,8 +195,12 @@ func (p *ociPackageDraft) UpdateLifecycle(ctx context.Context, new v1alpha1.Pack return nil } +func (p *ociPackageDraft) GetName() string { + return p.packageName +} + // Finish round of updates. -func (p *ociPackageDraft) Close(ctx context.Context) (repository.PackageRevision, error) { +func (p *ociPackageDraft) Close(ctx context.Context, version string) (repository.PackageRevision, error) { ctx, span := tracer.Start(ctx, "ociPackageDraft::Close", trace.WithAttributes()) defer span.End() diff --git a/pkg/oci/oci.go b/pkg/oci/oci.go index de629486..ca6cb06e 100644 --- a/pkg/oci/oci.go +++ b/pkg/oci/oci.go @@ -314,6 +314,11 @@ func GetSingleFromAnnotation(key string, manifest *v1.Manifest) string { return fmt.Sprintf("annotation %v unset", key) } +// ToMainPackageRevision implements repository.PackageRevision. +func (p *ociPackageRevision) ToMainPackageRevision() repository.PackageRevision { + panic("unimplemented") +} + func (r *ociRepository) ListFunctions(ctx context.Context) ([]repository.Function, error) { // Repository whose content type is not Function contains no Function resources. if r.content != configapi.RepositoryContentFunction { diff --git a/pkg/registry/porch/packagecommon.go b/pkg/registry/porch/packagecommon.go index 9a6c6254..cb2f66af 100644 --- a/pkg/registry/porch/packagecommon.go +++ b/pkg/registry/porch/packagecommon.go @@ -309,7 +309,7 @@ func (r *packageCommon) updatePackageRevision(ctx context.Context, name string, } if !isCreate { - rev, err := r.cad.UpdatePackageRevision(ctx, &repositoryObj, oldRepoPkgRev, oldApiPkgRev.(*api.PackageRevision), newApiPkgRev, parentPackage) + rev, err := r.cad.UpdatePackageRevision(ctx, "", &repositoryObj, oldRepoPkgRev, oldApiPkgRev.(*api.PackageRevision), newApiPkgRev, parentPackage) if err != nil { return nil, false, apierrors.NewInternalError(err) } diff --git a/pkg/repository/repository.go b/pkg/repository/repository.go index 2bd0b041..74c57e78 100644 --- a/pkg/repository/repository.go +++ b/pkg/repository/repository.go @@ -94,6 +94,8 @@ type PackageRevision interface { // ResourceVersion returns the Kube resource version of the package ResourceVersion() string + + ToMainPackageRevision() PackageRevision } // Package is an abstract package. @@ -118,7 +120,8 @@ type PackageDraft interface { // Updates desired lifecycle of the package. The lifecycle is applied on Close. UpdateLifecycle(ctx context.Context, new v1alpha1.PackageRevisionLifecycle) error // Finish round of updates. - Close(ctx context.Context) (PackageRevision, error) + Close(ctx context.Context, version string) (PackageRevision, error) + GetName() string } // Function is an abstract function. From 8fc058b9601767bbd011781a0abc2b9b1bd7f900 Mon Sep 17 00:00:00 2001 From: Kushal Harish Naidu Date: Thu, 24 Oct 2024 16:49:15 +0000 Subject: [PATCH 02/12] Improve approve time - commit 2 --- pkg/cache/repository.go | 30 +++++++++++++++++------------- pkg/repository/repository.go | 8 +++++--- 2 files changed, 22 insertions(+), 16 deletions(-) diff --git a/pkg/cache/repository.go b/pkg/cache/repository.go index 93fd6723..fef7eb2e 100644 --- a/pkg/cache/repository.go +++ b/pkg/cache/repository.go @@ -265,22 +265,20 @@ func (r *cachedRepository) update(ctx context.Context, updated repository.Packag // TODO: Just updated package? identifyLatestRevisions(r.cachedPackageRevisions) - if (updated.Lifecycle() == (v1alpha1.PackageRevisionLifecycleProposed)) || (updated.Lifecycle() == v1alpha1.PackageRevisionLifecycleDraft) { - version, err := r.repo.Version(ctx) - if err != nil { - return nil, err - } - r.lastVersion = version - } - if updated.Lifecycle() == (v1alpha1.PackageRevisionLifecyclePublished) { updatedMain := updated.ToMainPackageRevision() - oldMainKey := repository.PackageRevisionKey{ - Repository: updatedMain.Key().Repository, - Package: updatedMain.Key().Package, - Revision: updatedMain.Key().Revision, + //Search and delete any old main pkgRev in the cache + for k := range r.cachedPackageRevisions { + if (k.Repository == updatedMain.Key().Repository) && (k.Package == updatedMain.Key().Package) && (k.Revision == updatedMain.Key().Revision) { + oldMainKey := repository.PackageRevisionKey{ + Repository: updatedMain.Key().Repository, + Package: updatedMain.Key().Package, + Revision: updatedMain.Key().Revision, + WorkspaceName: v1alpha1.WorkspaceName(string(k.WorkspaceName)), + } + delete(r.cachedPackageRevisions, oldMainKey) + } } - delete(r.cachedPackageRevisions, oldMainKey) cachedMain := &cachedPackageRevision{PackageRevision: updatedMain} r.cachedPackageRevisions[updatedMain.Key()] = cachedMain @@ -307,6 +305,12 @@ func (r *cachedRepository) update(ctx context.Context, updated repository.Packag return nil, err } r.lastVersion = version + } else { + version, err := r.repo.Version(ctx) + if err != nil { + return nil, err + } + r.lastVersion = version } // TODO: Update the latest revisions for the r.cachedPackages diff --git a/pkg/repository/repository.go b/pkg/repository/repository.go index 74c57e78..fe278a8a 100644 --- a/pkg/repository/repository.go +++ b/pkg/repository/repository.go @@ -152,13 +152,15 @@ type ListPackageRevisionFilter struct { // Matches returns true if the provided PackageRevision satisfies the conditions in the filter. func (f *ListPackageRevisionFilter) Matches(p PackageRevision) bool { - if f.Package != "" && f.Package != p.Key().Package { + packageKey := p.Key() + + if f.Package != "" && f.Package != packageKey.Package { return false } - if f.Revision != "" && f.Revision != p.Key().Revision { + if f.Revision != "" && f.Revision != packageKey.Revision { return false } - if f.WorkspaceName != "" && f.WorkspaceName != p.Key().WorkspaceName { + if f.WorkspaceName != "" && f.WorkspaceName != packageKey.WorkspaceName { return false } if f.KubeObjectName != "" && f.KubeObjectName != p.KubeObjectName() { From a4738904d01deb374a464f1225d40a42069b9f50 Mon Sep 17 00:00:00 2001 From: Gergely Nagy Date: Wed, 30 Oct 2024 15:42:31 +0100 Subject: [PATCH 03/12] Added a testcase to isolate the e2e test failure --- pkg/cache/cache_test.go | 88 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 88 insertions(+) diff --git a/pkg/cache/cache_test.go b/pkg/cache/cache_test.go index 5418f253..e2401bf1 100644 --- a/pkg/cache/cache_test.go +++ b/pkg/cache/cache_test.go @@ -126,6 +126,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("UpdatePackaeg(%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) *cachedRepository { t.Helper() From 7db5fc2d75685b239f52f3dd1e640c4d503f9473 Mon Sep 17 00:00:00 2001 From: Gergely Nagy Date: Wed, 30 Oct 2024 15:43:14 +0100 Subject: [PATCH 04/12] Generating a reference pointing at the main branch in ToMainPackageRevision --- pkg/git/package.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/pkg/git/package.go b/pkg/git/package.go index 425d6ca4..c39962d6 100644 --- a/pkg/git/package.go +++ b/pkg/git/package.go @@ -219,8 +219,13 @@ func (p *gitPackageRevision) GetResources(ctx context.Context) (*v1alpha1.Packag }, nil } -// Workspace name needs to be changed as well. +// Creates a gitPackageRevision reference that is acting as the main branch package revision. +// It doesn't do any git operations, so this package should only be used if the actual git updates +// were exectued on the main branch. func (p *gitPackageRevision) ToMainPackageRevision() repository.PackageRevision { + //Need to compute a separate reference, otherwise the ref will be the same as the versioned package, + //while the main gitPackageRevision needs to point at the main branch. + mainBranchRef := plumbing.NewHashReference(p.repo.branch.RefInLocal(), p.commit) p1 := &gitPackageRevision{ repo: p.repo, path: p.path, @@ -228,7 +233,7 @@ func (p *gitPackageRevision) ToMainPackageRevision() repository.PackageRevision workspaceName: p.workspaceName, updated: p.updated, updatedBy: p.updatedBy, - ref: p.ref, + ref: mainBranchRef, tree: p.tree, commit: p.commit, tasks: p.tasks, From 4b768a99be3597bc514642a471c8396d64773988 Mon Sep 17 00:00:00 2001 From: Kushal Harish Naidu Date: Thu, 31 Oct 2024 11:18:30 +0000 Subject: [PATCH 05/12] Remove cache refresh from pkgRev update in engine --- pkg/cache/cache_test.go | 2 +- pkg/engine/engine.go | 8 -------- 2 files changed, 1 insertion(+), 9 deletions(-) diff --git a/pkg/cache/cache_test.go b/pkg/cache/cache_test.go index e2401bf1..6ce144b9 100644 --- a/pkg/cache/cache_test.go +++ b/pkg/cache/cache_test.go @@ -152,7 +152,7 @@ func TestDeletePublishedMain(t *testing.T) { update, err := cachedRepo.UpdatePackageRevision(ctx, bucket) if err != nil { - t.Fatalf("UpdatePackaeg(%s) failed: %v", bucket.Key(), err) + t.Fatalf("UpdatePackage(%s) failed: %v", bucket.Key(), err) } if err := update.UpdateLifecycle(ctx, api.PackageRevisionLifecyclePublished); err != nil { t.Fatalf("UpdateLifecycle failed; %v", err) diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go index 357c0f65..3d7a703d 100644 --- a/pkg/engine/engine.go +++ b/pkg/engine/engine.go @@ -707,14 +707,6 @@ func (cad *cadEngine) UpdatePackageRevision(ctx context.Context, version string, 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 } From cdc4a5b8e293546216b75b52f7105d67b7dfd323 Mon Sep 17 00:00:00 2001 From: Gergely Nagy Date: Mon, 4 Nov 2024 12:12:27 +0100 Subject: [PATCH 06/12] Fixed git unittests --- pkg/git/git_test.go | 8 ++++---- test/e2e/benchmark/benchmark_test.go | 23 +++++++++++++++++++++++ 2 files changed, 27 insertions(+), 4 deletions(-) create mode 100644 test/e2e/benchmark/benchmark_test.go diff --git a/pkg/git/git_test.go b/pkg/git/git_test.go index 4684fe1c..1578caeb 100644 --- a/pkg/git/git_test.go +++ b/pkg/git/git_test.go @@ -180,7 +180,7 @@ func (g GitSuite) TestGitPackageRoundTrip(t *testing.T) { t.Fatalf("draft.UpdateResources(%#v, %#v) failed: %v", newResources, task, err) } - revision, err := draft.Close(ctx, "") + revision, err := draft.Close(ctx, "v1") if err != nil { t.Fatalf("draft.Close() failed: %v", err) } @@ -207,7 +207,7 @@ func (g GitSuite) TestGitPackageRoundTrip(t *testing.T) { if err := update.UpdateLifecycle(ctx, v1alpha1.PackageRevisionLifecyclePublished); err != nil { t.Fatalf("UpdateLifecycle failed: %v", err) } - approved, err := update.Close(ctx, "") + approved, err := update.Close(ctx, "v1") if err != nil { t.Fatalf("Close() of %q, %q failed: %v", packageName, workspace, err) } @@ -688,7 +688,7 @@ func (g GitSuite) TestApproveDraft(t *testing.T) { update.UpdateLifecycle(ctx, v1alpha1.PackageRevisionLifecyclePublished) - new, err := update.Close(ctx, "") + new, err := update.Close(ctx, "v1") if err != nil { t.Fatalf("Close failed: %v", err) } @@ -750,7 +750,7 @@ func (g GitSuite) TestApproveDraftWithHistory(t *testing.T) { update.UpdateLifecycle(ctx, v1alpha1.PackageRevisionLifecyclePublished) - new, err := update.Close(ctx, "") + new, err := update.Close(ctx, "v1") if err != nil { t.Fatalf("Close failed: %v", err) } diff --git a/test/e2e/benchmark/benchmark_test.go b/test/e2e/benchmark/benchmark_test.go new file mode 100644 index 00000000..32f5de8c --- /dev/null +++ b/test/e2e/benchmark/benchmark_test.go @@ -0,0 +1,23 @@ +package benchmark + +import ( + "context" + "testing" + + e2e "github.com/nephio-project/porch/test/e2e" +) + +// This ugliness is needed so the e2e test suite setup can be reused. +type BenchmarkSuite struct { + //Inherit the test setup from the e2e test suite for now. Can change to something more production-ready like ginkgo later. + e2e.TestSuiteWithGit +} + +func TestBenchmark(t *testing.T) { + e2e.RunSuite(&BenchmarkSuite{}, t) +} + +func (t *BenchmarkSuite) TestCreateScenario(ctx context.Context) { + + t.Logf("Testrun") +} From 11bc11e59585a1c126a3e29de8d2c1c67edcf7b8 Mon Sep 17 00:00:00 2001 From: Kushal Harish Naidu Date: Mon, 4 Nov 2024 14:32:04 +0000 Subject: [PATCH 07/12] fix packageName causing E2E test to fail --- pkg/git/draft.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/pkg/git/draft.go b/pkg/git/draft.go index 04c71542..00ec1a8a 100644 --- a/pkg/git/draft.go +++ b/pkg/git/draft.go @@ -16,6 +16,7 @@ package git import ( "context" + "strings" "time" "github.com/go-git/go-git/v5/plumbing" @@ -71,5 +72,7 @@ func (d *gitPackageDraft) Close(ctx context.Context, version string) (repository } func (d *gitPackageDraft) GetName() string { - return d.path + packageDirectory := d.parent.directory + packageName := strings.TrimPrefix(d.path, packageDirectory+"/") + return packageName } From 3eed7429296b4a1ba34bfda82e470ac5c243a766 Mon Sep 17 00:00:00 2001 From: Kushal Harish Naidu Date: Mon, 4 Nov 2024 16:53:10 +0000 Subject: [PATCH 08/12] Remoove e2e benchmark test --- test/e2e/benchmark/benchmark_test.go | 23 ----------------------- 1 file changed, 23 deletions(-) delete mode 100644 test/e2e/benchmark/benchmark_test.go diff --git a/test/e2e/benchmark/benchmark_test.go b/test/e2e/benchmark/benchmark_test.go deleted file mode 100644 index 32f5de8c..00000000 --- a/test/e2e/benchmark/benchmark_test.go +++ /dev/null @@ -1,23 +0,0 @@ -package benchmark - -import ( - "context" - "testing" - - e2e "github.com/nephio-project/porch/test/e2e" -) - -// This ugliness is needed so the e2e test suite setup can be reused. -type BenchmarkSuite struct { - //Inherit the test setup from the e2e test suite for now. Can change to something more production-ready like ginkgo later. - e2e.TestSuiteWithGit -} - -func TestBenchmark(t *testing.T) { - e2e.RunSuite(&BenchmarkSuite{}, t) -} - -func (t *BenchmarkSuite) TestCreateScenario(ctx context.Context) { - - t.Logf("Testrun") -} From 5acb87c0b63e85e3be8502b70147e1f4918ed940 Mon Sep 17 00:00:00 2001 From: Kushal Harish Naidu Date: Mon, 4 Nov 2024 17:13:26 +0000 Subject: [PATCH 09/12] Add check for empty version in git CloseDraft --- pkg/cache/memory/draft.go | 6 ++---- pkg/git/git.go | 3 +++ 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/pkg/cache/memory/draft.go b/pkg/cache/memory/draft.go index 8642cd64..42f6eca0 100644 --- a/pkg/cache/memory/draft.go +++ b/pkg/cache/memory/draft.go @@ -21,7 +21,6 @@ import ( "github.com/nephio-project/porch/pkg/cache" "github.com/nephio-project/porch/pkg/repository" "go.opentelemetry.io/otel/trace" - "k8s.io/klog/v2" ) type cachedDraft struct { @@ -44,7 +43,6 @@ func (cd *cachedDraft) Close(ctx context.Context, version string) (repository.Pa if err != nil { return nil, err } - klog.Infof("Pkgrev version mismatch in cache and repo - Refreshing cache.") } revisions, err := cd.cache.ListPackageRevisions(ctx, repository.ListPackageRevisionFilter{ @@ -61,12 +59,12 @@ func (cd *cachedDraft) Close(ctx context.Context, version string) (repository.Pa } } - v, err = repository.NextRevisionNumber(revs) + nextVersion, err := repository.NextRevisionNumber(revs) if err != nil { return nil, err } - if closed, err := cd.PackageDraft.Close(ctx, v); err != nil { + if closed, err := cd.PackageDraft.Close(ctx, nextVersion); err != nil { return nil, err } else { return cd.cache.update(ctx, closed) diff --git a/pkg/git/git.go b/pkg/git/git.go index ea796438..4794acba 100644 --- a/pkg/git/git.go +++ b/pkg/git/git.go @@ -1493,6 +1493,9 @@ func (r *gitRepository) CloseDraft(ctx context.Context, version string, d *gitPa switch d.lifecycle { case v1alpha1.PackageRevisionLifecyclePublished, v1alpha1.PackageRevisionLifecycleDeletionProposed: + if version == "" { + return nil, errors.New("Version cannot be empty for the next package revision") + } d.revision = version // Finalize the package revision. Commit it to main branch. From 0f4b414f2ab244b79fe8f0d3b89d7ba83c5e64da Mon Sep 17 00:00:00 2001 From: Kushal Harish Naidu Date: Tue, 5 Nov 2024 10:34:02 +0000 Subject: [PATCH 10/12] Fix TestDeletePublishedMain unit test path cache_test.go --- pkg/cache/memory/cache_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cache/memory/cache_test.go b/pkg/cache/memory/cache_test.go index 9cd88c05..df74a63d 100644 --- a/pkg/cache/memory/cache_test.go +++ b/pkg/cache/memory/cache_test.go @@ -130,7 +130,7 @@ func TestPublishedLatest(t *testing.T) { func TestDeletePublishedMain(t *testing.T) { ctx := context.Background() - testPath := filepath.Join("..", "git", "testdata") + testPath := filepath.Join("../..", "git", "testdata") cachedRepo := openRepositoryFromArchive(t, ctx, testPath, "nested") revisions, err := cachedRepo.ListPackageRevisions(ctx, repository.ListPackageRevisionFilter{ From 0c39b8bc194928639b4d27d7df2e914aef798a51 Mon Sep 17 00:00:00 2001 From: Kushal Harish Naidu Date: Tue, 5 Nov 2024 13:05:04 +0000 Subject: [PATCH 11/12] Move main pkg revision creation into a function --- pkg/cache/memory/repository.go | 84 ++++++++++++++++++---------------- 1 file changed, 45 insertions(+), 39 deletions(-) diff --git a/pkg/cache/memory/repository.go b/pkg/cache/memory/repository.go index 50ee9ae4..6d4e1858 100644 --- a/pkg/cache/memory/repository.go +++ b/pkg/cache/memory/repository.go @@ -264,59 +264,65 @@ 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) + // Create the main package revision if updated.Lifecycle() == (v1alpha1.PackageRevisionLifecyclePublished) { updatedMain := updated.ToMainPackageRevision() - //Search and delete any old main pkgRev in the cache - for k := range r.cachedPackageRevisions { - if (k.Repository == updatedMain.Key().Repository) && (k.Package == updatedMain.Key().Package) && (k.Revision == updatedMain.Key().Revision) { - oldMainKey := repository.PackageRevisionKey{ - Repository: updatedMain.Key().Repository, - Package: updatedMain.Key().Package, - Revision: updatedMain.Key().Revision, - WorkspaceName: v1alpha1.WorkspaceName(string(k.WorkspaceName)), - } - delete(r.cachedPackageRevisions, oldMainKey) + 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 worksapce in the cache + for k := range r.cachedPackageRevisions { + if (k.Repository == updatedMain.Key().Repository) && (k.Package == updatedMain.Key().Package) && (k.Revision == updatedMain.Key().Revision) { + oldMainKey := repository.PackageRevisionKey{ + Repository: updatedMain.Key().Repository, + Package: updatedMain.Key().Package, + Revision: updatedMain.Key().Revision, + WorkspaceName: v1alpha1.WorkspaceName(string(k.WorkspaceName)), } + delete(r.cachedPackageRevisions, oldMainKey) } - cachedMain := &cachedPackageRevision{PackageRevision: updatedMain} - r.cachedPackageRevisions[updatedMain.Key()] = cachedMain + } + cachedMain := &cachedPackageRevision{PackageRevision: updatedMain} + r.cachedPackageRevisions[updatedMain.Key()] = cachedMain - pkgRevMetaNN := types.NamespacedName{ + 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(), } - // TODO: error handling for metadataStore.Get - _, err := r.metadataStore.Get(ctx, pkgRevMetaNN) - // Error can be different - 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 nil, err - } - r.lastVersion = version - } else { - version, err := r.repo.Version(ctx) + _, err := r.metadataStore.Create(ctx, pkgRevMeta, r.repoSpec.Name, updatedMain.UID()) if err != nil { - return nil, err + klog.Warningf("unable to create PackageRev CR for %s/%s: %v", + updatedMain.KubeObjectNamespace(), updatedMain.KubeObjectName(), err) } - r.lastVersion = version } + version, err := r.repo.Version(ctx) + if err != nil { + return err + } + r.lastVersion = version - // TODO: Update the latest revisions for the r.cachedPackages - return cached, nil + return nil } func (r *cachedRepository) DeletePackageRevision(ctx context.Context, old repository.PackageRevision) error { From 3eb22045936d32daf1a1cb378161f25528135c8f Mon Sep 17 00:00:00 2001 From: Kushal Harish Naidu Date: Tue, 5 Nov 2024 16:02:24 +0000 Subject: [PATCH 12/12] Update parameter names and fix typo --- pkg/cache/memory/draft.go | 6 +++--- pkg/cache/memory/repository.go | 10 +++++----- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/pkg/cache/memory/draft.go b/pkg/cache/memory/draft.go index 42f6eca0..1a9066a1 100644 --- a/pkg/cache/memory/draft.go +++ b/pkg/cache/memory/draft.go @@ -52,14 +52,14 @@ func (cd *cachedDraft) Close(ctx context.Context, version string) (repository.Pa return nil, err } - var revs []string + var publishedRevisions []string for _, rev := range revisions { if v1alpha1.LifecycleIsPublished(rev.Lifecycle()) { - revs = append(revs, rev.Key().Revision) + publishedRevisions = append(publishedRevisions, rev.Key().Revision) } } - nextVersion, err := repository.NextRevisionNumber(revs) + nextVersion, err := repository.NextRevisionNumber(publishedRevisions) if err != nil { return nil, err } diff --git a/pkg/cache/memory/repository.go b/pkg/cache/memory/repository.go index 6d4e1858..d2e202b7 100644 --- a/pkg/cache/memory/repository.go +++ b/pkg/cache/memory/repository.go @@ -267,7 +267,7 @@ func (r *cachedRepository) update(ctx context.Context, updated repository.Packag identifyLatestRevisions(r.cachedPackageRevisions) // Create the main package revision - if updated.Lifecycle() == (v1alpha1.PackageRevisionLifecyclePublished) { + if v1alpha1.LifecycleIsPublished(updated.Lifecycle()) { updatedMain := updated.ToMainPackageRevision() r.createMainPackageRevision(ctx, updatedMain) } else { @@ -283,14 +283,14 @@ func (r *cachedRepository) update(ctx context.Context, updated repository.Packag func (r *cachedRepository) createMainPackageRevision(ctx context.Context, updatedMain repository.PackageRevision) error { - //Search and delete any old main pkgRev of an older worksapce in the cache - for k := range r.cachedPackageRevisions { - if (k.Repository == updatedMain.Key().Repository) && (k.Package == updatedMain.Key().Package) && (k.Revision == updatedMain.Key().Revision) { + //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(k.WorkspaceName)), + WorkspaceName: v1alpha1.WorkspaceName(string(pkgRevKey.WorkspaceName)), } delete(r.cachedPackageRevisions, oldMainKey) }