diff --git a/porch/pkg/cache/repository.go b/porch/pkg/cache/repository.go index 2ef7e96498..d4e6f7f8d0 100644 --- a/porch/pkg/cache/repository.go +++ b/porch/pkg/cache/repository.go @@ -299,6 +299,30 @@ func (r *cachedRepository) DeletePackage(ctx context.Context, old repository.Pac func (r *cachedRepository) Close() error { r.cancel() + + // Make sure that watch events are sent for packagerevisions that are + // removed as part of closing the repository. + for _, pr := range r.cachedPackageRevisions { + nn := types.NamespacedName{ + Name: pr.KubeObjectName(), + Namespace: pr.KubeObjectNamespace(), + } + // There isn't really any correct way to handle finalizers here. We are removing + // the repository, so we have to just delete the PackageRevision regardless of any + // finalizers. + pkgRevMeta, err := r.metadataStore.Delete(context.TODO(), nn, true) + if err != nil { + // There isn't much use in returning an error here, so we just log it + // and create a PackageRevisionMeta with just name and namespace. This + // makes sure that the Delete event is sent. + klog.Warningf("Error looking up PackageRev CR for %s: %v") + pkgRevMeta = meta.PackageRevisionMeta{ + Name: nn.Name, + Namespace: nn.Namespace, + } + } + r.objectNotifier.NotifyPackageRevisionChange(watch.Deleted, pr, pkgRevMeta) + } return nil } @@ -370,18 +394,19 @@ func (r *cachedRepository) refreshAllCachedPackages(ctx context.Context) (map[re } newPackageRevisionMap := make(map[repository.PackageRevisionKey]*cachedPackageRevision, len(newPackageRevisions)) - newPackageRevisionNames := make(map[string]bool) + newPackageRevisionNames := make(map[string]*cachedPackageRevision, len(newPackageRevisions)) for _, newPackage := range newPackageRevisions { k := newPackage.Key() if newPackageRevisionMap[k] != nil { klog.Warningf("found duplicate packages with key %v", k) } - newPackageRevisionMap[k] = &cachedPackageRevision{ + pkgRev := &cachedPackageRevision{ PackageRevision: newPackage, isLatestRevision: false, } - newPackageRevisionNames[newPackage.KubeObjectName()] = true + newPackageRevisionMap[k] = pkgRev + newPackageRevisionNames[newPackage.KubeObjectName()] = pkgRev } identifyLatestRevisions(newPackageRevisionMap) @@ -410,10 +435,10 @@ func (r *cachedRepository) refreshAllCachedPackages(ctx context.Context) (map[re if _, err := r.metadataStore.Delete(ctx, types.NamespacedName{ Name: prm.Name, Namespace: prm.Namespace, - }); err != nil { + }, true); err != nil { if !apierrors.IsNotFound(err) { // This will be retried the next time the sync runs. - klog.Warningf("unable to create PackageRev CR for %s/%s: %w", + klog.Warningf("unable to delete PackageRev CR for %s/%s: %w", prm.Name, prm.Namespace, err) } } @@ -422,13 +447,13 @@ 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 := range newPackageRevisionNames { + 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); err != nil { + 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. @@ -455,11 +480,19 @@ func (r *cachedRepository) refreshAllCachedPackages(ctx context.Context) (map[re } for k, oldPackage := range oldPackageRevisions { - metaPackage, found := existingPkgRevCRsMap[oldPackage.KubeObjectName()] - if !found { - klog.Warningf("no PackageRev CR found for PackageRevision %s", oldPackage.KubeObjectName()) - } if newPackageRevisionMap[k] == nil { + nn := types.NamespacedName{ + Name: oldPackage.KubeObjectName(), + Namespace: oldPackage.KubeObjectNamespace(), + } + metaPackage, err := r.metadataStore.Delete(ctx, nn, true) + if err != nil { + klog.Warningf("Error deleting PkgRevMeta %s: %v") + metaPackage = meta.PackageRevisionMeta{ + Name: nn.Name, + Namespace: nn.Namespace, + } + } r.objectNotifier.NotifyPackageRevisionChange(watch.Deleted, oldPackage, metaPackage) } } diff --git a/porch/pkg/engine/engine.go b/porch/pkg/engine/engine.go index 659be9c109..51b5a8aaa5 100644 --- a/porch/pkg/engine/engine.go +++ b/porch/pkg/engine/engine.go @@ -116,6 +116,9 @@ func (p *PackageRevision) GetPackageRevision(ctx context.Context) (*api.PackageR repoPkgRev.Labels[api.LatestPackageRevisionKey] = api.LatestPackageRevisionValue } repoPkgRev.Annotations = p.packageRevisionMeta.Annotations + repoPkgRev.Finalizers = p.packageRevisionMeta.Finalizers + repoPkgRev.OwnerReferences = p.packageRevisionMeta.OwnerReferences + repoPkgRev.DeletionTimestamp = p.packageRevisionMeta.DeletionTimestamp return repoPkgRev, nil } @@ -330,12 +333,14 @@ func (cad *cadEngine) CreatePackageRevision(ctx context.Context, repositoryObj * return nil, err } pkgRevMeta := meta.PackageRevisionMeta{ - Name: repoPkgRev.KubeObjectName(), - Namespace: repoPkgRev.KubeObjectNamespace(), - Labels: obj.Labels, - Annotations: obj.Annotations, - } - pkgRevMeta, err = cad.metadataStore.Create(ctx, pkgRevMeta, repositoryObj) + Name: repoPkgRev.KubeObjectName(), + Namespace: repoPkgRev.KubeObjectNamespace(), + Labels: obj.Labels, + Annotations: obj.Annotations, + Finalizers: obj.Finalizers, + OwnerReferences: obj.OwnerReferences, + } + pkgRevMeta, err = cad.metadataStore.Create(ctx, pkgRevMeta, repositoryObj.Name, repoPkgRev.UID()) if err != nil { return nil, err } @@ -534,6 +539,26 @@ func (cad *cadEngine) UpdatePackageRevision(ctx context.Context, repositoryObj * return nil, err } + // Check if the PackageRevision is in the terminating state and + // and this request removes the last finalizer. + repoPkgRev := oldPackage.repoPackageRevision + pkgRevMetaNN := types.NamespacedName{ + Name: repoPkgRev.KubeObjectName(), + Namespace: repoPkgRev.KubeObjectNamespace(), + } + pkgRevMeta, err := cad.metadataStore.Get(ctx, pkgRevMetaNN) + if err != nil { + return nil, err + } + // If this is in the terminating state and we are removing the last finalizer, + // we delete the resource instead of updating it. + if pkgRevMeta.DeletionTimestamp != nil && len(newObj.Finalizers) == 0 { + if err := cad.deletePackageRevision(ctx, repo, repoPkgRev, pkgRevMeta); err != nil { + return nil, err + } + return ToPackageRevision(repoPkgRev, pkgRevMeta), nil + } + // Validate package lifecycle. Can only update a draft. switch lifecycle := oldObj.Spec.Lifecycle; lifecycle { default: @@ -542,21 +567,13 @@ func (cad *cadEngine) UpdatePackageRevision(ctx context.Context, repositoryObj * // Draft or proposed can be updated. case api.PackageRevisionLifecyclePublished: // Only metadata (currently labels and annotations) can be updated for published packages. - repoPkgRev := oldPackage.repoPackageRevision - - pkgRevMeta := meta.PackageRevisionMeta{ - Name: repoPkgRev.KubeObjectName(), - Namespace: repoPkgRev.KubeObjectNamespace(), - Labels: newObj.Labels, - Annotations: newObj.Annotations, + pkgRevMeta, err = cad.updatePkgRevMeta(ctx, repoPkgRev, newObj) + if err != nil { + return nil, err } - cad.metadataStore.Update(ctx, pkgRevMeta) cad.watcherManager.NotifyPackageRevisionChange(watch.Modified, repoPkgRev, pkgRevMeta) - return &PackageRevision{ - repoPackageRevision: repoPkgRev, - packageRevisionMeta: pkgRevMeta, - }, nil + return ToPackageRevision(repoPkgRev, pkgRevMeta), nil } switch lifecycle := newObj.Spec.Lifecycle; lifecycle { default: @@ -575,19 +592,13 @@ func (cad *cadEngine) UpdatePackageRevision(ctx context.Context, repositoryObj * return nil, err } - pkgRevMeta := meta.PackageRevisionMeta{ - Name: repoPkgRev.KubeObjectName(), - Namespace: repoPkgRev.KubeObjectNamespace(), - Labels: newObj.Labels, - Annotations: newObj.Annotations, + pkgRevMeta, err = cad.updatePkgRevMeta(ctx, repoPkgRev, newObj) + if err != nil { + return nil, err } - cad.metadataStore.Update(ctx, pkgRevMeta) cad.watcherManager.NotifyPackageRevisionChange(watch.Modified, repoPkgRev, pkgRevMeta) - return &PackageRevision{ - repoPackageRevision: repoPkgRev, - packageRevisionMeta: pkgRevMeta, - }, nil + return ToPackageRevision(repoPkgRev, pkgRevMeta), nil } var mutations []mutation @@ -676,24 +687,30 @@ func (cad *cadEngine) UpdatePackageRevision(ctx context.Context, repositoryObj * } // Updates are done. - repoPkgRev, err := draft.Close(ctx) + repoPkgRev, err = draft.Close(ctx) if err != nil { return nil, err } - pkgRevMeta := meta.PackageRevisionMeta{ - Name: repoPkgRev.KubeObjectName(), - Namespace: repoPkgRev.KubeObjectNamespace(), - Labels: newObj.Labels, - Annotations: newObj.Annotations, + pkgRevMeta, err = cad.updatePkgRevMeta(ctx, repoPkgRev, newObj) + if err != nil { + return nil, err } - cad.metadataStore.Update(ctx, pkgRevMeta) cad.watcherManager.NotifyPackageRevisionChange(watch.Modified, repoPkgRev, pkgRevMeta) - return &PackageRevision{ - repoPackageRevision: repoPkgRev, - packageRevisionMeta: pkgRevMeta, - }, nil + return ToPackageRevision(repoPkgRev, pkgRevMeta), nil +} + +func (cad *cadEngine) updatePkgRevMeta(ctx context.Context, repoPkgRev repository.PackageRevision, apiPkgRev *api.PackageRevision) (meta.PackageRevisionMeta, error) { + pkgRevMeta := meta.PackageRevisionMeta{ + Name: repoPkgRev.KubeObjectName(), + Namespace: repoPkgRev.KubeObjectNamespace(), + Labels: apiPkgRev.Labels, + Annotations: apiPkgRev.Annotations, + Finalizers: apiPkgRev.Finalizers, + OwnerReferences: apiPkgRev.OwnerReferences, + } + return cad.metadataStore.Update(ctx, pkgRevMeta) } func createKptfilePatchTask(ctx context.Context, oldPackage repository.PackageRevision, newObj *api.PackageRevision) (*api.Task, bool, error) { @@ -814,19 +831,49 @@ func (cad *cadEngine) DeletePackageRevision(ctx context.Context, repositoryObj * return err } - if err := repo.DeletePackageRevision(ctx, oldPackage.repoPackageRevision); err != nil { - return err - } - + // We delete the PackageRev regardless of any finalizers, since it + // will always have the same finalizers as the PackageRevision. This + // will put the PackageRev, and therefore the PackageRevision in the + // terminating state. + // But we only delete the PackageRevision from the repo once all finalizers + // have been removed. namespacedName := types.NamespacedName{ Name: oldPackage.repoPackageRevision.KubeObjectName(), Namespace: oldPackage.repoPackageRevision.KubeObjectNamespace(), } - if _, err := cad.metadataStore.Delete(ctx, namespacedName); err != nil { + pkgRevMeta, err := cad.metadataStore.Delete(ctx, namespacedName, false) + if err != nil { return err } - cad.watcherManager.NotifyPackageRevisionChange(watch.Deleted, oldPackage.repoPackageRevision, oldPackage.packageRevisionMeta) + if len(pkgRevMeta.Finalizers) > 0 { + klog.Infof("PackageRevision %s deleted, but still have finalizers: %s", oldPackage.KubeObjectName(), strings.Join(pkgRevMeta.Finalizers, ",")) + cad.watcherManager.NotifyPackageRevisionChange(watch.Modified, oldPackage.repoPackageRevision, oldPackage.packageRevisionMeta) + return nil + } + klog.Infof("PackageRevision %s deleted for real since no finalizers", oldPackage.KubeObjectName()) + + return cad.deletePackageRevision(ctx, repo, oldPackage.repoPackageRevision, oldPackage.packageRevisionMeta) +} + +func (cad *cadEngine) deletePackageRevision(ctx context.Context, repo repository.Repository, repoPkgRev repository.PackageRevision, pkgRevMeta meta.PackageRevisionMeta) error { + ctx, span := tracer.Start(ctx, "cadEngine::deletePackageRevision", trace.WithAttributes()) + defer span.End() + + if err := repo.DeletePackageRevision(ctx, repoPkgRev); err != nil { + return err + } + + nn := types.NamespacedName{ + Name: pkgRevMeta.Name, + Namespace: pkgRevMeta.Namespace, + } + if _, err := cad.metadataStore.Delete(ctx, nn, true); err != nil { + // If this fails, the CR will be cleaned up by the background job. + klog.Warningf("Error deleting PkgRevMeta %s: %v", nn.String(), err) + } + + cad.watcherManager.NotifyPackageRevisionChange(watch.Deleted, repoPkgRev, pkgRevMeta) return nil } diff --git a/porch/pkg/engine/fake/packagerevision.go b/porch/pkg/engine/fake/packagerevision.go index 290086c045..c2ff44328c 100644 --- a/porch/pkg/engine/fake/packagerevision.go +++ b/porch/pkg/engine/fake/packagerevision.go @@ -20,12 +20,14 @@ import ( kptfile "github.com/GoogleContainerTools/kpt/pkg/api/kptfile/v1" "github.com/GoogleContainerTools/kpt/porch/api/porch/v1alpha1" "github.com/GoogleContainerTools/kpt/porch/pkg/repository" + "k8s.io/apimachinery/pkg/types" ) // Implementation of the repository.PackageRevision interface for testing. type PackageRevision struct { Name string Namespace string + Uid types.UID PackageRevisionKey repository.PackageRevisionKey PackageLifecycle v1alpha1.PackageRevisionLifecycle PackageRevision *v1alpha1.PackageRevision @@ -41,6 +43,10 @@ func (pr *PackageRevision) KubeObjectNamespace() string { return pr.Namespace } +func (pr *PackageRevision) UID() types.UID { + return pr.Uid +} + func (pr *PackageRevision) Key() repository.PackageRevisionKey { return pr.PackageRevisionKey } diff --git a/porch/pkg/git/package.go b/porch/pkg/git/package.go index a23e191f62..e5ed0c7f5e 100644 --- a/porch/pkg/git/package.go +++ b/porch/pkg/git/package.go @@ -72,6 +72,10 @@ func (p *gitPackageRevision) KubeObjectNamespace() string { return p.repo.namespace } +func (p *gitPackageRevision) UID() types.UID { + return p.uid() +} + func (p *gitPackageRevision) Key() repository.PackageRevisionKey { return repository.PackageRevisionKey{ Repository: p.repo.name, diff --git a/porch/pkg/meta/fake/memorystore.go b/porch/pkg/meta/fake/memorystore.go index 3a7a7a7102..2eb6cbe44c 100644 --- a/porch/pkg/meta/fake/memorystore.go +++ b/porch/pkg/meta/fake/memorystore.go @@ -48,7 +48,7 @@ func (m *MemoryMetadataStore) List(ctx context.Context, repo *configapi.Reposito return m.Metas, nil } -func (m *MemoryMetadataStore) Create(ctx context.Context, pkgRevMeta meta.PackageRevisionMeta, repo *configapi.Repository) (meta.PackageRevisionMeta, error) { +func (m *MemoryMetadataStore) Create(ctx context.Context, pkgRevMeta meta.PackageRevisionMeta, repoName string, pkgRevUID types.UID) (meta.PackageRevisionMeta, error) { for _, m := range m.Metas { if m.Name == pkgRevMeta.Name && m.Namespace == pkgRevMeta.Namespace { return m, apierrors.NewAlreadyExists( @@ -78,7 +78,7 @@ func (m *MemoryMetadataStore) Update(ctx context.Context, pkgRevMeta meta.Packag return pkgRevMeta, nil } -func (m *MemoryMetadataStore) Delete(ctx context.Context, namespacedName types.NamespacedName) (meta.PackageRevisionMeta, error) { +func (m *MemoryMetadataStore) Delete(ctx context.Context, namespacedName types.NamespacedName, _ bool) (meta.PackageRevisionMeta, error) { var metas []meta.PackageRevisionMeta found := false var deletedMeta meta.PackageRevisionMeta diff --git a/porch/pkg/meta/store.go b/porch/pkg/meta/store.go index aa41edcf34..82cfd83768 100644 --- a/porch/pkg/meta/store.go +++ b/porch/pkg/meta/store.go @@ -17,6 +17,7 @@ package meta import ( "context" + porchapi "github.com/GoogleContainerTools/kpt/porch/api/porch/v1alpha1" configapi "github.com/GoogleContainerTools/kpt/porch/api/porchconfig/v1alpha1" internalapi "github.com/GoogleContainerTools/kpt/porch/internal/api/porchinternal/v1alpha1" "go.opentelemetry.io/otel" @@ -31,6 +32,11 @@ var tracer = otel.Tracer("meta") const ( PkgRevisionRepoLabel = "internal.porch.kpt.dev/repository" + PkgRevisionFinalizer = "internal.porch.kpt.dev/packagerevision" +) + +var ( + packageRevisionGVK = porchapi.SchemeGroupVersion.WithKind("PackageRevision") ) // MetadataStore is the store for keeping metadata about PackageRevisions. Typical @@ -39,18 +45,21 @@ const ( type MetadataStore interface { Get(ctx context.Context, namespacedName types.NamespacedName) (PackageRevisionMeta, error) List(ctx context.Context, repo *configapi.Repository) ([]PackageRevisionMeta, error) - Create(ctx context.Context, pkgRevMeta PackageRevisionMeta, repo *configapi.Repository) (PackageRevisionMeta, error) + Create(ctx context.Context, pkgRevMeta PackageRevisionMeta, repoName string, pkgRevUID types.UID) (PackageRevisionMeta, error) Update(ctx context.Context, pkgRevMeta PackageRevisionMeta) (PackageRevisionMeta, error) - Delete(ctx context.Context, namespacedName types.NamespacedName) (PackageRevisionMeta, error) + Delete(ctx context.Context, namespacedName types.NamespacedName, clearFinalizer bool) (PackageRevisionMeta, error) } // PackageRevisionMeta contains metadata about a specific PackageRevision. The // PackageRevision is identified by the name and namespace. type PackageRevisionMeta struct { - Name string - Namespace string - Labels map[string]string - Annotations map[string]string + Name string + Namespace string + Labels map[string]string + Annotations map[string]string + Finalizers []string + OwnerReferences []metav1.OwnerReference + DeletionTimestamp *metav1.Time } var _ MetadataStore = &crdMetadataStore{} @@ -77,15 +86,7 @@ func (c *crdMetadataStore) Get(ctx context.Context, namespacedName types.Namespa return PackageRevisionMeta{}, err } - labels := internalPkgRev.Labels - delete(labels, PkgRevisionRepoLabel) - - return PackageRevisionMeta{ - Name: internalPkgRev.Name, - Namespace: internalPkgRev.Namespace, - Labels: labels, - Annotations: internalPkgRev.Annotations, - }, nil + return toPackageRevisionMeta(&internalPkgRev), nil } func (c *crdMetadataStore) List(ctx context.Context, repo *configapi.Repository) ([]PackageRevisionMeta, error) { @@ -98,22 +99,13 @@ func (c *crdMetadataStore) List(ctx context.Context, repo *configapi.Repository) return nil, err } var pkgRevMetas []PackageRevisionMeta - var names []string for _, ipr := range internalPkgRevList.Items { - labels := ipr.Labels - delete(labels, PkgRevisionRepoLabel) - pkgRevMetas = append(pkgRevMetas, PackageRevisionMeta{ - Name: ipr.Name, - Namespace: ipr.Namespace, - Labels: labels, - Annotations: ipr.Annotations, - }) - names = append(names, ipr.Name) + pkgRevMetas = append(pkgRevMetas, toPackageRevisionMeta(&ipr)) } return pkgRevMetas, nil } -func (c *crdMetadataStore) Create(ctx context.Context, pkgRevMeta PackageRevisionMeta, repo *configapi.Repository) (PackageRevisionMeta, error) { +func (c *crdMetadataStore) Create(ctx context.Context, pkgRevMeta PackageRevisionMeta, repoName string, pkgRevUID types.UID) (PackageRevisionMeta, error) { ctx, span := tracer.Start(ctx, "crdMetadataStore::Create", trace.WithAttributes()) defer span.End() @@ -121,25 +113,25 @@ func (c *crdMetadataStore) Create(ctx context.Context, pkgRevMeta PackageRevisio if labels == nil { labels = make(map[string]string) } - labels[PkgRevisionRepoLabel] = repo.Name + 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{ ObjectMeta: metav1.ObjectMeta{ - Name: pkgRevMeta.Name, - Namespace: pkgRevMeta.Namespace, - Labels: labels, - Annotations: pkgRevMeta.Annotations, - // We probably should make these owner refs point to the PackageRevision CRs instead. - // But we need to make sure that deletion of these are correctly picked up by the - // GC. Currently we delete PackageRevisions through polling of the git/oci repos, and - // that doesn't get picked up by the GC. - OwnerReferences: []metav1.OwnerReference{ - { - APIVersion: configapi.RepositoryGVK.GroupVersion().String(), - Kind: configapi.RepositoryGVK.Kind, - Name: repo.Name, - UID: repo.UID, - }, - }, + Name: pkgRevMeta.Name, + Namespace: pkgRevMeta.Namespace, + Labels: labels, + Annotations: pkgRevMeta.Annotations, + Finalizers: finalizers, + OwnerReferences: ownerReferences, }, } if err := c.coreClient.Create(ctx, &internalPkgRev); err != nil { @@ -148,12 +140,7 @@ func (c *crdMetadataStore) Create(ctx context.Context, pkgRevMeta PackageRevisio } return PackageRevisionMeta{}, err } - return PackageRevisionMeta{ - Name: internalPkgRev.Name, - Namespace: internalPkgRev.Namespace, - Labels: internalPkgRev.Labels, - Annotations: internalPkgRev.Annotations, - }, nil + return toPackageRevisionMeta(&internalPkgRev), nil } func (c *crdMetadataStore) Update(ctx context.Context, pkgRevMeta PackageRevisionMeta) (PackageRevisionMeta, error) { @@ -170,6 +157,8 @@ func (c *crdMetadataStore) Update(ctx context.Context, pkgRevMeta PackageRevisio return PackageRevisionMeta{}, err } + // Copy updated labels to the CR and add the repository label + // that is only used on the CR. var labels map[string]string if pkgRevMeta.Labels != nil { labels = pkgRevMeta.Labels @@ -178,52 +167,81 @@ func (c *crdMetadataStore) Update(ctx context.Context, pkgRevMeta PackageRevisio } labels[PkgRevisionRepoLabel] = internalPkgRev.Labels[PkgRevisionRepoLabel] internalPkgRev.Labels = labels - - var annotations map[string]string - if pkgRevMeta.Annotations != nil { - annotations = pkgRevMeta.Annotations - } else { - annotations = make(map[string]string) + 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.Annotations = annotations + internalPkgRev.OwnerReferences = ownerReferences + internalPkgRev.Finalizers = append(pkgRevMeta.Finalizers, PkgRevisionFinalizer) if err := c.coreClient.Update(ctx, &internalPkgRev); err != nil { return PackageRevisionMeta{}, err } - delete(labels, PkgRevisionRepoLabel) - return PackageRevisionMeta{ - Name: pkgRevMeta.Name, - Namespace: pkgRevMeta.Namespace, - Labels: labels, - Annotations: internalPkgRev.Annotations, - }, nil + return toPackageRevisionMeta(&internalPkgRev), nil } -func (c *crdMetadataStore) Delete(ctx context.Context, namespacedName types.NamespacedName) (PackageRevisionMeta, error) { +func (c *crdMetadataStore) Delete(ctx context.Context, namespacedName types.NamespacedName, clearFinalizers bool) (PackageRevisionMeta, error) { ctx, span := tracer.Start(ctx, "crdMetadataStore::Delete", trace.WithAttributes()) defer span.End() var internalPkgRev internalapi.PackageRev err := c.coreClient.Get(ctx, namespacedName, &internalPkgRev) if err != nil { - if apierrors.IsNotFound(err) { - return PackageRevisionMeta{}, nil - } return PackageRevisionMeta{}, err } - if err := c.coreClient.Delete(ctx, &internalPkgRev); err != nil { - if apierrors.IsNotFound(err) { - return PackageRevisionMeta{}, nil + if clearFinalizers { + internalPkgRev.Finalizers = []string{} + if err = c.coreClient.Update(ctx, &internalPkgRev); err != nil { + return PackageRevisionMeta{}, err } + } + + if err := c.coreClient.Delete(ctx, &internalPkgRev); err != nil { return PackageRevisionMeta{}, err } + return toPackageRevisionMeta(&internalPkgRev), nil +} + +func toPackageRevisionMeta(internalPkgRev *internalapi.PackageRev) PackageRevisionMeta { 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 { + finalizers = append(finalizers, f) + } + } + return PackageRevisionMeta{ - Name: internalPkgRev.Name, - Namespace: internalPkgRev.Namespace, - Labels: labels, - Annotations: internalPkgRev.Annotations, - }, nil + Name: internalPkgRev.Name, + Namespace: internalPkgRev.Namespace, + Labels: labels, + Annotations: internalPkgRev.Annotations, + Finalizers: finalizers, + 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 } diff --git a/porch/pkg/oci/oci.go b/porch/pkg/oci/oci.go index 7d43a7bad6..1e2414dabb 100644 --- a/porch/pkg/oci/oci.go +++ b/porch/pkg/oci/oci.go @@ -392,6 +392,10 @@ func (p *ociPackageRevision) KubeObjectNamespace() string { return p.parent.namespace } +func (p *ociPackageRevision) UID() types.UID { + return p.uid +} + func (p *ociPackageRevision) Key() repository.PackageRevisionKey { return repository.PackageRevisionKey{ Repository: p.parent.name, diff --git a/porch/pkg/registry/porch/watch.go b/porch/pkg/registry/porch/watch.go index a993635d9f..f7ab03d670 100644 --- a/porch/pkg/registry/porch/watch.go +++ b/porch/pkg/registry/porch/watch.go @@ -95,7 +95,7 @@ func (w *watcher) ResultChan() <-chan watch.Event { func (w *watcher) listAndWatch(ctx context.Context, r *packageRevisions, filter packageRevisionFilter, selector labels.Selector) { if err := w.listAndWatchInner(ctx, r, filter, selector); err != nil { // TODO: We need to populate the object on this error - klog.Warningf("sending error to watch stream") + klog.Warningf("sending error to watch stream: %v", err) ev := watch.Event{ Type: watch.Error, } diff --git a/porch/pkg/repository/repository.go b/porch/pkg/repository/repository.go index 6ed8c658c3..8f0e9dbad4 100644 --- a/porch/pkg/repository/repository.go +++ b/porch/pkg/repository/repository.go @@ -21,6 +21,7 @@ import ( kptfile "github.com/GoogleContainerTools/kpt/pkg/api/kptfile/v1" "github.com/GoogleContainerTools/kpt/porch/api/porch/v1alpha1" "github.com/go-git/go-git/v5/plumbing/transport" + "k8s.io/apimachinery/pkg/types" ) // TODO: "sigs.k8s.io/kustomize/kyaml/filesys" FileSystem? @@ -55,8 +56,13 @@ type PackageRevision interface { // More "readable" values are returned by Key() KubeObjectName() string + // KubeObjectNamespace returns the namespace in which the PackageRevision + // belongs. KubeObjectNamespace() string + // UID returns a unique identifier for the PackageRevision. + UID() types.UID + // Key returns the "primary key" of the package. Key() PackageRevisionKey diff --git a/porch/test/e2e/e2e_test.go b/porch/test/e2e/e2e_test.go index 97639be8ae..fe7ef9da91 100644 --- a/porch/test/e2e/e2e_test.go +++ b/porch/test/e2e/e2e_test.go @@ -50,6 +50,11 @@ const ( kptRepo = "https://github.com/GoogleContainerTools/kpt.git" ) +var ( + packageRevisionGVK = porchapi.SchemeGroupVersion.WithKind("PackageRevision") + configMapGVK = corev1.SchemeGroupVersion.WithKind("ConfigMap") +) + func TestE2E(t *testing.T) { e2e := os.Getenv("E2E") if e2e == "" { @@ -1687,19 +1692,14 @@ func (t *PorchSuite) TestRegisteredPackageRevisionLabels(ctx context.Context) { ) } -func (t *PorchSuite) TestPackageRevisionGarbageCollector(ctx context.Context) { +func (t *PorchSuite) TestPackageRevisionGCWithOwner(ctx context.Context) { const ( - repository = "pkgrevgc" - workspace = "test-workspace" + repository = "pkgrevgcwithowner" + workspace = "pkgrevgcwithowner-workspace" description = "empty-package description" cmName = "foo" ) - var ( - packageRevisionGVK = porchapi.SchemeGroupVersion.WithKind("PackageRevision") - configMapGVK = corev1.SchemeGroupVersion.WithKind("ConfigMap") - ) - t.registerMainGitRepositoryF(ctx, repository) // Create a new package (via init) @@ -1730,7 +1730,7 @@ func (t *PorchSuite) TestPackageRevisionGarbageCollector(ctx context.Context) { OwnerReferences: []metav1.OwnerReference{ { APIVersion: porchapi.SchemeGroupVersion.String(), - Kind: "PackageRevision", + Kind: packageRevisionGVK.Kind, Name: pr.Name, UID: pr.UID, }, @@ -1763,6 +1763,224 @@ func (t *PorchSuite) TestPackageRevisionGarbageCollector(ctx context.Context) { ) } +func (t *PorchSuite) TestPackageRevisionGCAsOwner(ctx context.Context) { + const ( + repository = "pkgrevgcasowner" + workspace = "pkgrevgcasowner-workspace" + description = "empty-package description" + cmName = "foo" + ) + + t.registerMainGitRepositoryF(ctx, repository) + + cm := &corev1.ConfigMap{ + TypeMeta: metav1.TypeMeta{ + Kind: configMapGVK.Kind, + APIVersion: configMapGVK.GroupVersion().String(), + }, + ObjectMeta: metav1.ObjectMeta{ + Name: cmName, + Namespace: t.namespace, + }, + Data: map[string]string{ + "foo": "bar", + }, + } + t.CreateF(ctx, cm) + + pr := &porchapi.PackageRevision{ + TypeMeta: metav1.TypeMeta{ + Kind: packageRevisionGVK.Kind, + APIVersion: packageRevisionGVK.GroupVersion().String(), + }, + ObjectMeta: metav1.ObjectMeta{ + Namespace: t.namespace, + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: "v1", + Kind: "ConfigMap", + Name: cm.Name, + UID: cm.UID, + }, + }, + }, + Spec: porchapi.PackageRevisionSpec{ + PackageName: "empty-package", + WorkspaceName: workspace, + RepositoryName: repository, + }, + } + t.CreateF(ctx, pr) + + t.DeleteF(ctx, cm) + t.waitUntilObjectDeleted( + ctx, + configMapGVK, + types.NamespacedName{ + Name: cm.Name, + Namespace: cm.Namespace, + }, + 10*time.Second, + ) + t.waitUntilObjectDeleted( + ctx, + packageRevisionGVK, + types.NamespacedName{ + Name: pr.Name, + Namespace: pr.Namespace, + }, + 10*time.Second, + ) +} + +func (t *PorchSuite) TestPackageRevisionOwnerReferences(ctx context.Context) { + const ( + repository = "pkgrevownerrefs" + workspace = "pkgrevownerrefs-workspace" + description = "empty-package description" + cmName = "foo" + ) + + t.registerMainGitRepositoryF(ctx, repository) + + cm := &corev1.ConfigMap{ + TypeMeta: metav1.TypeMeta{ + Kind: configMapGVK.Kind, + APIVersion: configMapGVK.GroupVersion().String(), + }, + ObjectMeta: metav1.ObjectMeta{ + Name: cmName, + Namespace: t.namespace, + }, + Data: map[string]string{ + "foo": "bar", + }, + } + t.CreateF(ctx, cm) + + pr := &porchapi.PackageRevision{ + TypeMeta: metav1.TypeMeta{ + Kind: packageRevisionGVK.Kind, + APIVersion: packageRevisionGVK.GroupVersion().String(), + }, + ObjectMeta: metav1.ObjectMeta{ + Namespace: t.namespace, + }, + Spec: porchapi.PackageRevisionSpec{ + PackageName: "empty-package", + WorkspaceName: workspace, + RepositoryName: repository, + }, + } + t.CreateF(ctx, pr) + t.validateOwnerReferences(ctx, pr.Name, []metav1.OwnerReference{}) + + ownerRef := metav1.OwnerReference{ + APIVersion: "v1", + Kind: "ConfigMap", + Name: cm.Name, + UID: cm.UID, + } + pr.ObjectMeta.OwnerReferences = []metav1.OwnerReference{ownerRef} + t.UpdateF(ctx, pr) + t.validateOwnerReferences(ctx, pr.Name, []metav1.OwnerReference{ownerRef}) + + pr.ObjectMeta.OwnerReferences = []metav1.OwnerReference{} + t.UpdateF(ctx, pr) + t.validateOwnerReferences(ctx, pr.Name, []metav1.OwnerReference{}) +} + +func (t *PorchSuite) TestPackageRevisionFinalizers(ctx context.Context) { + const ( + repository = "pkgrevfinalizers" + workspace = "pkgrevfinalizers-workspace" + description = "empty-package description" + ) + + t.registerMainGitRepositoryF(ctx, repository) + + pr := &porchapi.PackageRevision{ + TypeMeta: metav1.TypeMeta{ + Kind: packageRevisionGVK.Kind, + APIVersion: packageRevisionGVK.GroupVersion().String(), + }, + ObjectMeta: metav1.ObjectMeta{ + Namespace: t.namespace, + }, + Spec: porchapi.PackageRevisionSpec{ + PackageName: "empty-package", + WorkspaceName: workspace, + RepositoryName: repository, + }, + } + t.CreateF(ctx, pr) + t.validateFinalizers(ctx, pr.Name, []string{}) + + pr.Finalizers = append(pr.Finalizers, "foo-finalizer") + t.UpdateF(ctx, pr) + t.validateFinalizers(ctx, pr.Name, []string{"foo-finalizer"}) + + t.DeleteF(ctx, pr) + t.validateFinalizers(ctx, pr.Name, []string{"foo-finalizer"}) + + pr.Finalizers = []string{} + t.UpdateF(ctx, pr) + t.waitUntilObjectDeleted(ctx, packageRevisionGVK, types.NamespacedName{ + Name: pr.Name, + Namespace: pr.Namespace, + }, 10*time.Second) +} + +func (t *PorchSuite) validateFinalizers(ctx context.Context, name string, finalizers []string) { + var pr porchapi.PackageRevision + t.GetF(ctx, client.ObjectKey{ + Namespace: t.namespace, + Name: name, + }, &pr) + + if len(finalizers) != len(pr.Finalizers) { + diff := cmp.Diff(finalizers, pr.Finalizers) + t.Errorf("Expected %d finalizers, but got %s", len(finalizers), diff) + } + + for _, finalizer := range finalizers { + var found bool + for _, f := range pr.Finalizers { + if f == finalizer { + found = true + } + } + if !found { + t.Errorf("Expected finalizer %v, but didn't find it", finalizer) + } + } +} + +func (t *PorchSuite) validateOwnerReferences(ctx context.Context, name string, ownerRefs []metav1.OwnerReference) { + var pr porchapi.PackageRevision + t.GetF(ctx, client.ObjectKey{ + Namespace: t.namespace, + Name: name, + }, &pr) + + if len(ownerRefs) != len(pr.OwnerReferences) { + diff := cmp.Diff(ownerRefs, pr.OwnerReferences) + t.Errorf("Expected %d ownerReferences, but got %s", len(ownerRefs), diff) + } + + for _, ownerRef := range ownerRefs { + var found bool + for _, or := range pr.OwnerReferences { + if or == ownerRef { + found = true + } + } + if !found { + t.Errorf("Expected ownerRef %v, but didn't find it", ownerRef) + } + } +} + func (t *PorchSuite) validateLabelsAndAnnos(ctx context.Context, name string, labels, annos map[string]string) { var pr porchapi.PackageRevision t.GetF(ctx, client.ObjectKey{