Merge pull request kptdev#309 from Nordix/creation-timestamp-upstream-lock

DB Cache locks and creationTimestamp enhancements

Author: liamfallon

api/sql/porch-db-1.5.2-1.5.3.sql

@@ -0,0 +1,18 @@
+/*
+Copyright 2025 The kpt and Nephio Authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+     http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+ALTER TABLE package_revisions 
+    ADD COLUMN ext_pr_id TEXT NOT NULL
+    DEFAULT '{}';

api/sql/porch-db-1.5.3-1.5.2.sql

@@ -0,0 +1,17 @@
+/*
+Copyright 2025 The kpt and Nephio Authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+     http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+ALTER TABLE package_revisions 
+    DROP COLUMN ext_pr_id;

api/sql/porch-db.sql

@@ -82,6 +82,7 @@ CREATE TABLE IF NOT EXISTS package_revisions (
     updated          TIMESTAMP NOT NULL,
     updatedby        TEXT NOT NULL,
     lifecycle        TEXT CHECK (lifecycle IN ('Draft', 'Proposed', 'Published', 'DeletionProposed')) NOT NULL,
+    ext_pr_id        TEXT NOT NULL,
     latest           BOOLEAN NOT NULL DEFAULT FALSE,
     tasks            TEXT NOT NULL,
     PRIMARY KEY (k8s_name_space, k8s_name),

pkg/cache/dbcache/dbcache_test.go

@@ -26,7 +26,7 @@ import (
 	cachetypes "github.com/nephio-project/porch/pkg/cache/types"
 	"github.com/nephio-project/porch/pkg/externalrepo"
 	"github.com/nephio-project/porch/pkg/repository"
-	mocksql "github.com/nephio-project/porch/test/mockery/mocks/porch/pkg/cache/dbcache"
+	mockdbcache "github.com/nephio-project/porch/test/mockery/mocks/porch/pkg/cache/dbcache"
 	"github.com/stretchr/testify/assert"
 	v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/watch"
@@ -126,7 +126,7 @@ func (n *mockNotifier) NotifyPackageRevisionChange(eventType watch.EventType, ob
 }
 
 func switchToMockSQL(t *testing.T) {
-	mockSQL := mocksql.NewMockdbSQLInterface(t)
+	mockDBCache := mockdbcache.NewMockdbSQLInterface(t)
 
 	savedDBHandler = GetDB()
 	dbHandler = nil
@@ -137,7 +137,7 @@ func switchToMockSQL(t *testing.T) {
 	dbHandler = &DBHandler{
 		dBCacheOptions: savedDBHandler.dBCacheOptions,
 		dataSource:     savedDBHandler.dataSource,
-		db:             mockSQL,
+		db:             mockDBCache,
 	}
 	assert.NotNil(t, dbHandler)
 }
@@ -180,18 +180,25 @@ func TestDBRepositoryCrud(t *testing.T) {
 	assert.Nil(t, err)
 }
 
-func createTestRepo(t *testing.T, namespace, name string) dbRepository {
+func createTestRepo(t *testing.T, namespace, name string) *dbRepository {
 	dbRepo := dbRepository{
 		repoKey: repository.RepositoryKey{
 			Namespace: namespace,
 			Name:      name,
 		},
 		repoPRChangeNotifier: &mockNotifier{returnVal: 1},
+		spec: &configapi.Repository{
+			Spec: configapi.RepositorySpec{
+				Git: &configapi.GitRepository{
+					Repo: "http://www.gitrepo.org/my-repo",
+				},
+			},
+		},
 	}
 	err := repoWriteToDB(context.TODO(), &dbRepo)
 	assert.Nil(t, err)
 
-	return dbRepo
+	return &dbRepo
 }
 
 func deleteTestRepo(t *testing.T, key repository.RepositoryKey) {

pkg/cache/dbcache/dbpackage.go

@@ -109,11 +109,11 @@ func (p *dbPackage) GetPackage(ctx context.Context) *v1alpha1.PorchPackage {
 	}
 }
 
-func (p *dbPackage) savePackageRevision(ctx context.Context, d *dbPackageRevision) (*dbPackageRevision, error) {
+func (p *dbPackage) savePackageRevision(ctx context.Context, d *dbPackageRevision, saveResources bool) (*dbPackageRevision, error) {
 	_, span := tracer.Start(ctx, "dbPackage:savePackageRevision", trace.WithAttributes())
 	defer span.End()
 
-	return d.savePackageRevision(ctx, true)
+	return d.savePackageRevision(ctx, saveResources)
 }
 
 func (p *dbPackage) DeletePackageRevision(ctx context.Context, old repository.PackageRevision, deleteExternal bool) error {

pkg/cache/dbcache/dbpackagerevision.go

@@ -50,6 +50,7 @@ type dbPackageRevision struct {
 	updated   time.Time
 	updatedBy string
 	lifecycle porchapi.PackageRevisionLifecycle
+	extPRID   kptfile.UpstreamLock
 	latest    bool
 	tasks     []porchapi.Task
 	resources map[string]string
@@ -83,7 +84,7 @@ func (pr *dbPackageRevision) savePackageRevision(ctx context.Context, saveResour
 	_, err := pkgRevReadFromDB(ctx, pr.Key(), false)
 	if err == nil {
 		updErr := pkgRevUpdateDB(ctx, pr, saveResources)
-		if updErr == nil {
+		if updErr == nil && saveResources {
 			sent := pr.repo.repoPRChangeNotifier.NotifyPackageRevisionChange(watch.Modified, pr)
 			klog.Infof("DB cache %+v: sent %d notifications for updated package revision %+v", pr.repo.Key(), sent, pr.Key())
 		}
@@ -151,29 +152,11 @@ func (pr *dbPackageRevision) GetPackageRevision(ctx context.Context) (*porchapi.
 		}
 	}
 
-	_, lock, _ := pr.GetUpstreamLock(ctx)
-	lockCopy := &porchapi.UpstreamLock{}
-
-	// TODO: Comment copied from pkg/externalrepo/git/package.go
-	// Use kpt definition of UpstreamLock in the package revision status
-	// when https://github.com/GoogleContainerTools/kpt/issues/3297 is complete.
-	// Until then, we have to translate from one type to another.
-	if lock.Git != nil {
-		lockCopy = &porchapi.UpstreamLock{
-			Type: porchapi.OriginType(lock.Type),
-			Git: &porchapi.GitLock{
-				Repo:      lock.Git.Repo,
-				Directory: lock.Git.Directory,
-				Commit:    lock.Git.Commit,
-				Ref:       lock.Git.Ref,
-			},
-		}
-	}
-
+	_, upstreamLock, _ := pr.GetUpstreamLock(ctx)
 	kf, _ := readPR.GetKptfile(ctx)
 
 	status := porchapi.PackageRevisionStatus{
-		UpstreamLock: lockCopy,
+		UpstreamLock: repository.KptUpstreamLock2APIUpstreamLock(upstreamLock),
 		Deployment:   pr.repo.deployment,
 		Conditions:   repository.ToAPIConditions(kf),
 	}
@@ -293,11 +276,14 @@ func (pr *dbPackageRevision) ToMainPackageRevision(ctx context.Context) reposito
 		updated:   time.Now(),
 		updatedBy: getCurrentUser(),
 		lifecycle: pr.lifecycle,
+		extPRID:   pr.extPRID,
 		latest:    false,
 		tasks:     pr.tasks,
 		resources: pr.resources,
 	}
 
+	mainPR.meta.CreationTimestamp = metav1.Time{Time: time.Now()}
+
 	if mainPR.pkgRevKey.WorkspaceName == "" {
 		mainPR.pkgRevKey.WorkspaceName = "main"
 	}
@@ -336,31 +322,7 @@ func (pr *dbPackageRevision) GetKptfile(ctx context.Context) (kptfile.KptFile, e
 }
 
 func (pr *dbPackageRevision) GetLock() (kptfile.Upstream, kptfile.UpstreamLock, error) {
-	if porchapi.LifecycleIsPublished(pr.lifecycle) {
-		externalPr, err := pr.repo.getExternalPr(context.Background(), pr.Key())
-		if err != nil {
-			return kptfile.Upstream{}, kptfile.UpstreamLock{},
-				pkgerrors.Wrapf(err, "dbPackageRevision:GetLock: getting lock of %+v failed, could not find package revision on external repository", pr.Key())
-		}
-
-		return externalPr.GetLock()
-	} else {
-		return kptfile.Upstream{
-				Type: kptfile.GitOrigin,
-				Git: &kptfile.Git{
-					Repo:      pr.repo.spec.Spec.Git.Repo,
-					Directory: pr.Key().PKey().ToPkgPathname(),
-					Ref:       "drafts/" + pr.Key().PKey().ToPkgPathname() + "/" + pr.Key().WorkspaceName,
-				},
-			}, kptfile.UpstreamLock{
-				Type: kptfile.GitOrigin,
-				Git: &kptfile.GitLock{
-					Repo:      pr.repo.spec.Spec.Git.Repo,
-					Directory: pr.Key().PKey().ToPkgPathname(),
-					Ref:       "drafts/" + pr.Key().PKey().ToPkgPathname() + "/" + pr.Key().WorkspaceName,
-				},
-			}, nil
-	}
+	return repository.KptUpstreamLock2KptUpstream(pr.extPRID), pr.extPRID, nil
 }
 
 func (pr *dbPackageRevision) ResourceVersion() string {
@@ -427,25 +389,50 @@ func (pr *dbPackageRevision) publishPR(ctx context.Context, newLifecycle porchap
 	pr.pkgRevKey.Revision = latestRev + 1
 	pr.lifecycle = newLifecycle
 
-	if _, err := engine.PushPackageRevision(ctx, pr.repo.externalRepo, pr); err != nil {
+	pushedPRExtID, err := engine.PushPackageRevision(ctx, pr.repo.externalRepo, pr)
+	if err != nil {
 		klog.Warningf("push of package revision %+v to external repo failed, %q", pr.Key(), err)
 		pr.pkgRevKey.Revision = 0
 		pr.lifecycle = porchapi.PackageRevisionLifecycleProposed
 		return pkgerrors.Wrapf(err, "dbPackageRevision:publishPR: push of package revision %+v to external repo failed", pr.Key())
 	}
 
-	if pr.pkgRevKey.Revision == 1 {
-		if err = pkgRevWriteToDB(ctx, pr.ToMainPackageRevision(ctx).(*dbPackageRevision)); err != nil {
-			return pkgerrors.Wrapf(err, "dbPackageRevision:UpdateLifecycle: could not write placeholder package revision for package revision %+v to DB", pr.Key())
+	pr.extPRID = pushedPRExtID
+
+	if err = pkgRevUpdateDB(ctx, pr, false); err != nil {
+		return pkgerrors.Wrapf(err, "dbPackageRevision:publishPR: failed to save package revision %+v to database after push to external repo", pr.Key())
+	}
+
+	return pr.publishPlaceholderPRForPR(ctx)
+}
+
+func (pr *dbPackageRevision) publishPlaceholderPRForPR(ctx context.Context) error {
+	_, span := tracer.Start(ctx, "dbPackageRevision::publishPlaceholderPRForPR", trace.WithAttributes())
+	defer span.End()
+
+	prWithResources := pr
+	if len(prWithResources.resources) == 0 {
+		if readPR, err := pkgRevReadFromDB(ctx, pr.Key(), true); err == nil {
+			prWithResources = readPR
+		} else {
+			return pkgerrors.Wrapf(err, "dbPackageRevision:publishPlaceholderPRForPR: could read resources for package revision %+v to DB", pr.Key())
+		}
+	}
+
+	placeholderPR := prWithResources.ToMainPackageRevision(ctx).(*dbPackageRevision)
+
+	if prWithResources.pkgRevKey.Revision == 1 {
+		if err := pkgRevWriteToDB(ctx, placeholderPR); err != nil {
+			return pkgerrors.Wrapf(err, "dbPackageRevision:publishPlaceholderPRForPR: could not write placeholder package revision for package revision %+v to DB", placeholderPR.Key())
 		}
-		sent := pr.repo.repoPRChangeNotifier.NotifyPackageRevisionChange(watch.Added, pr)
-		klog.Infof("DB cache %+v: sent %d notifications for added package revision %+v", pr.repo.Key(), sent, pr.Key())
-	} else if pr.pkgRevKey.Revision > 1 {
-		if err = pkgRevUpdateDB(ctx, pr.ToMainPackageRevision(ctx).(*dbPackageRevision), true); err != nil {
-			return pkgerrors.Wrapf(err, "dbPackageRevision:UpdateLifecycle: could not update placeholder package revision for package revision %+v to DB", pr.Key())
+		sent := placeholderPR.repo.repoPRChangeNotifier.NotifyPackageRevisionChange(watch.Added, placeholderPR)
+		klog.Infof("DB cache %+v: sent %d notifications for added package revision %+v", placeholderPR.repo.Key(), sent, placeholderPR.Key())
+	} else if prWithResources.pkgRevKey.Revision > 1 {
+		if err := pkgRevUpdateDB(ctx, placeholderPR, true); err != nil {
+			return pkgerrors.Wrapf(err, "dbPackageRevision:publishPlaceholderPRForPR: could not update placeholder package revision for package revision %+v to DB", placeholderPR.Key())
 		}
-		sent := pr.repo.repoPRChangeNotifier.NotifyPackageRevisionChange(watch.Modified, pr)
-		klog.Infof("DB cache %+v: sent %d notifications for updated package revision %+v", pr.repo.Key(), sent, pr.Key())
+		sent := placeholderPR.repo.repoPRChangeNotifier.NotifyPackageRevisionChange(watch.Modified, placeholderPR)
+		klog.Infof("DB cache %+v: sent %d notifications for updated package revision %+v", placeholderPR.repo.Key(), sent, placeholderPR.Key())
 	}
 
 	return nil

pkg/cache/dbcache/dbpackagerevision_test.go

@@ -47,7 +47,7 @@ func TestDBPackageRevision(t *testing.T) {
 			},
 		},
 	}
-	mockCache.EXPECT().GetRepository(mock.Anything).Return(&testRepo).Maybe()
+	mockCache.EXPECT().GetRepository(mock.Anything).Return(testRepo).Maybe()
 
 	err := testRepo.OpenRepository(ctx, externalrepotypes.ExternalRepoOptions{})
 	assert.Nil(t, err)