From 1672b5dd3a5a8d9e3e10ef4b0630758464f55100 Mon Sep 17 00:00:00 2001 From: Kushal Harish Naidu Date: Thu, 29 Aug 2024 07:06:11 +0000 Subject: [PATCH 1/3] Refactor ownerRefs in metadata Store --- pkg/meta/store.go | 36 +++--------------------------------- 1 file changed, 3 insertions(+), 33 deletions(-) diff --git a/pkg/meta/store.go b/pkg/meta/store.go index 6ff13ae1..92a565a7 100644 --- a/pkg/meta/store.go +++ b/pkg/meta/store.go @@ -116,13 +116,6 @@ func (c *crdMetadataStore) Create(ctx context.Context, pkgRevMeta PackageRevisio } labels[PkgRevisionRepoLabel] = repoName - ownerReferences := append(pkgRevMeta.OwnerReferences, metav1.OwnerReference{ - APIVersion: packageRevisionGVK.GroupVersion().String(), - Kind: packageRevisionGVK.Kind, - Name: pkgRevMeta.Name, - UID: pkgRevUID, - }) - finalizers := append(pkgRevMeta.Finalizers, PkgRevisionFinalizer) internalPkgRev := internalapi.PackageRev{ @@ -132,7 +125,7 @@ func (c *crdMetadataStore) Create(ctx context.Context, pkgRevMeta PackageRevisio Labels: labels, Annotations: pkgRevMeta.Annotations, Finalizers: finalizers, - OwnerReferences: ownerReferences, + OwnerReferences: pkgRevMeta.OwnerReferences, }, } klog.Infof("Creating packagerev %s/%s", internalPkgRev.Namespace, internalPkgRev.Name) @@ -171,15 +164,7 @@ func (c *crdMetadataStore) Update(ctx context.Context, pkgRevMeta PackageRevisio internalPkgRev.Labels = labels internalPkgRev.Annotations = pkgRevMeta.Annotations - // Copy update ownerReferences to the CR and make sure to also - // add the ownerReferences pointing to the PackageRevision. - ownerReferences := pkgRevMeta.OwnerReferences - for _, or := range internalPkgRev.OwnerReferences { - if isPackageRevOwnerRef(or, internalPkgRev.Name) { - ownerReferences = append(ownerReferences, or) - } - } - internalPkgRev.OwnerReferences = ownerReferences + internalPkgRev.OwnerReferences = pkgRevMeta.OwnerReferences internalPkgRev.Finalizers = append(pkgRevMeta.Finalizers, PkgRevisionFinalizer) klog.Infof("Updating packagerev %s/%s", internalPkgRev.Namespace, internalPkgRev.Name) @@ -217,15 +202,6 @@ func toPackageRevisionMeta(internalPkgRev *internalapi.PackageRev) PackageRevisi labels := internalPkgRev.Labels delete(labels, PkgRevisionRepoLabel) - var ownerReferences []metav1.OwnerReference - for _, or := range internalPkgRev.OwnerReferences { - // Don't include ownerReference to the PackageRevision itself. It is - // only used by Porch internally. - if !isPackageRevOwnerRef(or, internalPkgRev.Name) { - ownerReferences = append(ownerReferences, or) - } - } - var finalizers []string for _, f := range internalPkgRev.Finalizers { if f != PkgRevisionFinalizer { @@ -239,13 +215,7 @@ func toPackageRevisionMeta(internalPkgRev *internalapi.PackageRev) PackageRevisi Labels: labels, Annotations: internalPkgRev.Annotations, Finalizers: finalizers, - OwnerReferences: ownerReferences, + OwnerReferences: internalPkgRev.OwnerReferences, DeletionTimestamp: internalPkgRev.DeletionTimestamp, } } - -func isPackageRevOwnerRef(or metav1.OwnerReference, pkgRevName string) bool { - return or.APIVersion == packageRevisionGVK.GroupVersion().String() && - or.Kind == packageRevisionGVK.Kind && - or.Name == pkgRevName -} From 14eb297b9b27ea580b8004703201cb378166a8e9 Mon Sep 17 00:00:00 2001 From: Kushal Harish Naidu Date: Wed, 4 Sep 2024 05:51:21 +0000 Subject: [PATCH 2/3] Change flow of pkgRev creation to avoid race condition --- pkg/cache/repository.go | 37 +++++++++++++++++++------------------ pkg/meta/store.go | 36 +++++++++++++++++++++++++++++++++--- 2 files changed, 52 insertions(+), 21 deletions(-) diff --git a/pkg/cache/repository.go b/pkg/cache/repository.go index 05c466f0..47fa2f5e 100644 --- a/pkg/cache/repository.go +++ b/pkg/cache/repository.go @@ -472,25 +472,8 @@ func (r *cachedRepository) refreshAllCachedPackages(ctx context.Context) (map[re } } - // We go through all the PackageRevisions and make sure they have - // a corresponding PackageRev CR. - for pkgRevName, pkgRev := range newPackageRevisionNames { - if _, found := existingPkgRevCRsMap[pkgRevName]; !found { - pkgRevMeta := meta.PackageRevisionMeta{ - Name: pkgRevName, - Namespace: r.repoSpec.Namespace, - } - if _, err := r.metadataStore.Create(ctx, pkgRevMeta, r.repoSpec.Name, pkgRev.UID()); err != nil { - // TODO: We should try to find a way to make these errors available through - // either the repository CR or the PackageRevision CR. This will be - // retried on the next sync. - klog.Warningf("unable to create PackageRev CR for %s/%s: %v", - r.repoSpec.Namespace, pkgRevName, err) - } - } - } - // Send notification for packages that changed. + // Send notification first before creation of PkgRev to avoid race condition after an approve because of ownerReference. addSent := 0 modSent := 0 for kname, newPackage := range newPackageRevisionNames { @@ -508,6 +491,24 @@ func (r *cachedRepository) refreshAllCachedPackages(ctx context.Context) (map[re } } + // We go through all the PackageRevisions and make sure they have + // a corresponding PackageRev CR. + for pkgRevName, pkgRev := range newPackageRevisionNames { + if _, found := existingPkgRevCRsMap[pkgRevName]; !found { + pkgRevMeta := meta.PackageRevisionMeta{ + Name: pkgRevName, + Namespace: r.repoSpec.Namespace, + } + if _, err := r.metadataStore.Create(ctx, pkgRevMeta, r.repoSpec.Name, pkgRev.UID()); err != nil { + // TODO: We should try to find a way to make these errors available through + // either the repository CR or the PackageRevision CR. This will be + // retried on the next sync. + klog.Warningf("unable to create PackageRev CR for %s/%s: %v", + r.repoSpec.Namespace, pkgRevName, err) + } + } + } + delSent := 0 // Send notifications for packages that was deleted in the SoT for kname, oldPackage := range oldPackageRevisionNames { diff --git a/pkg/meta/store.go b/pkg/meta/store.go index 92a565a7..6ff13ae1 100644 --- a/pkg/meta/store.go +++ b/pkg/meta/store.go @@ -116,6 +116,13 @@ func (c *crdMetadataStore) Create(ctx context.Context, pkgRevMeta PackageRevisio } labels[PkgRevisionRepoLabel] = repoName + ownerReferences := append(pkgRevMeta.OwnerReferences, metav1.OwnerReference{ + APIVersion: packageRevisionGVK.GroupVersion().String(), + Kind: packageRevisionGVK.Kind, + Name: pkgRevMeta.Name, + UID: pkgRevUID, + }) + finalizers := append(pkgRevMeta.Finalizers, PkgRevisionFinalizer) internalPkgRev := internalapi.PackageRev{ @@ -125,7 +132,7 @@ func (c *crdMetadataStore) Create(ctx context.Context, pkgRevMeta PackageRevisio Labels: labels, Annotations: pkgRevMeta.Annotations, Finalizers: finalizers, - OwnerReferences: pkgRevMeta.OwnerReferences, + OwnerReferences: ownerReferences, }, } klog.Infof("Creating packagerev %s/%s", internalPkgRev.Namespace, internalPkgRev.Name) @@ -164,7 +171,15 @@ func (c *crdMetadataStore) Update(ctx context.Context, pkgRevMeta PackageRevisio internalPkgRev.Labels = labels internalPkgRev.Annotations = pkgRevMeta.Annotations - internalPkgRev.OwnerReferences = pkgRevMeta.OwnerReferences + // Copy update ownerReferences to the CR and make sure to also + // add the ownerReferences pointing to the PackageRevision. + ownerReferences := pkgRevMeta.OwnerReferences + for _, or := range internalPkgRev.OwnerReferences { + if isPackageRevOwnerRef(or, internalPkgRev.Name) { + ownerReferences = append(ownerReferences, or) + } + } + internalPkgRev.OwnerReferences = ownerReferences internalPkgRev.Finalizers = append(pkgRevMeta.Finalizers, PkgRevisionFinalizer) klog.Infof("Updating packagerev %s/%s", internalPkgRev.Namespace, internalPkgRev.Name) @@ -202,6 +217,15 @@ func toPackageRevisionMeta(internalPkgRev *internalapi.PackageRev) PackageRevisi labels := internalPkgRev.Labels delete(labels, PkgRevisionRepoLabel) + var ownerReferences []metav1.OwnerReference + for _, or := range internalPkgRev.OwnerReferences { + // Don't include ownerReference to the PackageRevision itself. It is + // only used by Porch internally. + if !isPackageRevOwnerRef(or, internalPkgRev.Name) { + ownerReferences = append(ownerReferences, or) + } + } + var finalizers []string for _, f := range internalPkgRev.Finalizers { if f != PkgRevisionFinalizer { @@ -215,7 +239,13 @@ func toPackageRevisionMeta(internalPkgRev *internalapi.PackageRev) PackageRevisi Labels: labels, Annotations: internalPkgRev.Annotations, Finalizers: finalizers, - OwnerReferences: internalPkgRev.OwnerReferences, + OwnerReferences: ownerReferences, DeletionTimestamp: internalPkgRev.DeletionTimestamp, } } + +func isPackageRevOwnerRef(or metav1.OwnerReference, pkgRevName string) bool { + return or.APIVersion == packageRevisionGVK.GroupVersion().String() && + or.Kind == packageRevisionGVK.Kind && + or.Name == pkgRevName +} From 644f2c8ab52c89cc20601e22c96b64bf65dd6e81 Mon Sep 17 00:00:00 2001 From: Kushal Harish Naidu <159911459+kushnaidu@users.noreply.github.com> Date: Wed, 4 Sep 2024 11:22:47 +0100 Subject: [PATCH 3/3] Update pkg/cache/repository.go comment --- pkg/cache/repository.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/pkg/cache/repository.go b/pkg/cache/repository.go index 47fa2f5e..5dad725d 100644 --- a/pkg/cache/repository.go +++ b/pkg/cache/repository.go @@ -472,8 +472,7 @@ func (r *cachedRepository) refreshAllCachedPackages(ctx context.Context) (map[re } } - // Send notification for packages that changed. - // Send notification first before creation of PkgRev to avoid race condition after an approve because of ownerReference. + // Send notification for packages that changed before the creation of PkgRev to avoid race conditions because of ownerReferences. addSent := 0 modSent := 0 for kname, newPackage := range newPackageRevisionNames {