Skip to content

Commit e0bcd34

Browse files
mortentmortent
authored
Finalizers and OwnerRefs for PackageRevision (#3655)
1 parent 946fa81 commit e0bcd34

File tree

10 files changed

+479
-143
lines changed

10 files changed

+479
-143
lines changed

‎porch/pkg/cache/repository.go

Lines changed: 44 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -299,6 +299,30 @@ func (r *cachedRepository) DeletePackage(ctx context.Context, old repository.Pac
299299

300300
func (r *cachedRepository) Close() error {
301301
r.cancel()
302+
303+
// Make sure that watch events are sent for packagerevisions that are
304+
// removed as part of closing the repository.
305+
for _, pr := range r.cachedPackageRevisions {
306+
nn := types.NamespacedName{
307+
Name: pr.KubeObjectName(),
308+
Namespace: pr.KubeObjectNamespace(),
309+
}
310+
// There isn't really any correct way to handle finalizers here. We are removing
311+
// the repository, so we have to just delete the PackageRevision regardless of any
312+
// finalizers.
313+
pkgRevMeta, err := r.metadataStore.Delete(context.TODO(), nn, true)
314+
if err != nil {
315+
// There isn't much use in returning an error here, so we just log it
316+
// and create a PackageRevisionMeta with just name and namespace. This
317+
// makes sure that the Delete event is sent.
318+
klog.Warningf("Error looking up PackageRev CR for %s: %v")
319+
pkgRevMeta = meta.PackageRevisionMeta{
320+
Name: nn.Name,
321+
Namespace: nn.Namespace,
322+
}
323+
}
324+
r.objectNotifier.NotifyPackageRevisionChange(watch.Deleted, pr, pkgRevMeta)
325+
}
302326
return nil
303327
}
304328

@@ -370,18 +394,19 @@ func (r *cachedRepository) refreshAllCachedPackages(ctx context.Context) (map[re
370394
}
371395

372396
newPackageRevisionMap := make(map[repository.PackageRevisionKey]*cachedPackageRevision, len(newPackageRevisions))
373-
newPackageRevisionNames := make(map[string]bool)
397+
newPackageRevisionNames := make(map[string]*cachedPackageRevision, len(newPackageRevisions))
374398
for _, newPackage := range newPackageRevisions {
375399
k := newPackage.Key()
376400
if newPackageRevisionMap[k] != nil {
377401
klog.Warningf("found duplicate packages with key %v", k)
378402
}
379403

380-
newPackageRevisionMap[k] = &cachedPackageRevision{
404+
pkgRev := &cachedPackageRevision{
381405
PackageRevision: newPackage,
382406
isLatestRevision: false,
383407
}
384-
newPackageRevisionNames[newPackage.KubeObjectName()] = true
408+
newPackageRevisionMap[k] = pkgRev
409+
newPackageRevisionNames[newPackage.KubeObjectName()] = pkgRev
385410
}
386411

387412
identifyLatestRevisions(newPackageRevisionMap)
@@ -410,10 +435,10 @@ func (r *cachedRepository) refreshAllCachedPackages(ctx context.Context) (map[re
410435
if _, err := r.metadataStore.Delete(ctx, types.NamespacedName{
411436
Name: prm.Name,
412437
Namespace: prm.Namespace,
413-
}); err != nil {
438+
}, true); err != nil {
414439
if !apierrors.IsNotFound(err) {
415440
// This will be retried the next time the sync runs.
416-
klog.Warningf("unable to create PackageRev CR for %s/%s: %w",
441+
klog.Warningf("unable to delete PackageRev CR for %s/%s: %w",
417442
prm.Name, prm.Namespace, err)
418443
}
419444
}
@@ -422,13 +447,13 @@ func (r *cachedRepository) refreshAllCachedPackages(ctx context.Context) (map[re
422447

423448
// We go through all the PackageRevisions and make sure they have
424449
// a corresponding PackageRev CR.
425-
for pkgRevName := range newPackageRevisionNames {
450+
for pkgRevName, pkgRev := range newPackageRevisionNames {
426451
if _, found := existingPkgRevCRsMap[pkgRevName]; !found {
427452
pkgRevMeta := meta.PackageRevisionMeta{
428453
Name: pkgRevName,
429454
Namespace: r.repoSpec.Namespace,
430455
}
431-
if _, err := r.metadataStore.Create(ctx, pkgRevMeta, r.repoSpec); err != nil {
456+
if _, err := r.metadataStore.Create(ctx, pkgRevMeta, r.repoSpec.Name, pkgRev.UID()); err != nil {
432457
// TODO: We should try to find a way to make these errors available through
433458
// either the repository CR or the PackageRevision CR. This will be
434459
// retried on the next sync.
@@ -455,11 +480,19 @@ func (r *cachedRepository) refreshAllCachedPackages(ctx context.Context) (map[re
455480
}
456481

457482
for k, oldPackage := range oldPackageRevisions {
458-
metaPackage, found := existingPkgRevCRsMap[oldPackage.KubeObjectName()]
459-
if !found {
460-
klog.Warningf("no PackageRev CR found for PackageRevision %s", oldPackage.KubeObjectName())
461-
}
462483
if newPackageRevisionMap[k] == nil {
484+
nn := types.NamespacedName{
485+
Name: oldPackage.KubeObjectName(),
486+
Namespace: oldPackage.KubeObjectNamespace(),
487+
}
488+
metaPackage, err := r.metadataStore.Delete(ctx, nn, true)
489+
if err != nil {
490+
klog.Warningf("Error deleting PkgRevMeta %s: %v")
491+
metaPackage = meta.PackageRevisionMeta{
492+
Name: nn.Name,
493+
Namespace: nn.Namespace,
494+
}
495+
}
463496
r.objectNotifier.NotifyPackageRevisionChange(watch.Deleted, oldPackage, metaPackage)
464497
}
465498
}

‎porch/pkg/engine/engine.go

Lines changed: 92 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -116,6 +116,9 @@ func (p *PackageRevision) GetPackageRevision(ctx context.Context) (*api.PackageR
116116
repoPkgRev.Labels[api.LatestPackageRevisionKey] = api.LatestPackageRevisionValue
117117
}
118118
repoPkgRev.Annotations = p.packageRevisionMeta.Annotations
119+
repoPkgRev.Finalizers = p.packageRevisionMeta.Finalizers
120+
repoPkgRev.OwnerReferences = p.packageRevisionMeta.OwnerReferences
121+
repoPkgRev.DeletionTimestamp = p.packageRevisionMeta.DeletionTimestamp
119122
return repoPkgRev, nil
120123
}
121124

@@ -330,12 +333,14 @@ func (cad *cadEngine) CreatePackageRevision(ctx context.Context, repositoryObj *
330333
return nil, err
331334
}
332335
pkgRevMeta := meta.PackageRevisionMeta{
333-
Name: repoPkgRev.KubeObjectName(),
334-
Namespace: repoPkgRev.KubeObjectNamespace(),
335-
Labels: obj.Labels,
336-
Annotations: obj.Annotations,
337-
}
338-
pkgRevMeta, err = cad.metadataStore.Create(ctx, pkgRevMeta, repositoryObj)
336+
Name: repoPkgRev.KubeObjectName(),
337+
Namespace: repoPkgRev.KubeObjectNamespace(),
338+
Labels: obj.Labels,
339+
Annotations: obj.Annotations,
340+
Finalizers: obj.Finalizers,
341+
OwnerReferences: obj.OwnerReferences,
342+
}
343+
pkgRevMeta, err = cad.metadataStore.Create(ctx, pkgRevMeta, repositoryObj.Name, repoPkgRev.UID())
339344
if err != nil {
340345
return nil, err
341346
}
@@ -534,6 +539,26 @@ func (cad *cadEngine) UpdatePackageRevision(ctx context.Context, repositoryObj *
534539
return nil, err
535540
}
536541

542+
// Check if the PackageRevision is in the terminating state and
543+
// and this request removes the last finalizer.
544+
repoPkgRev := oldPackage.repoPackageRevision
545+
pkgRevMetaNN := types.NamespacedName{
546+
Name: repoPkgRev.KubeObjectName(),
547+
Namespace: repoPkgRev.KubeObjectNamespace(),
548+
}
549+
pkgRevMeta, err := cad.metadataStore.Get(ctx, pkgRevMetaNN)
550+
if err != nil {
551+
return nil, err
552+
}
553+
// If this is in the terminating state and we are removing the last finalizer,
554+
// we delete the resource instead of updating it.
555+
if pkgRevMeta.DeletionTimestamp != nil && len(newObj.Finalizers) == 0 {
556+
if err := cad.deletePackageRevision(ctx, repo, repoPkgRev, pkgRevMeta); err != nil {
557+
return nil, err
558+
}
559+
return ToPackageRevision(repoPkgRev, pkgRevMeta), nil
560+
}
561+
537562
// Validate package lifecycle. Can only update a draft.
538563
switch lifecycle := oldObj.Spec.Lifecycle; lifecycle {
539564
default:
@@ -542,21 +567,13 @@ func (cad *cadEngine) UpdatePackageRevision(ctx context.Context, repositoryObj *
542567
// Draft or proposed can be updated.
543568
case api.PackageRevisionLifecyclePublished:
544569
// Only metadata (currently labels and annotations) can be updated for published packages.
545-
repoPkgRev := oldPackage.repoPackageRevision
546-
547-
pkgRevMeta := meta.PackageRevisionMeta{
548-
Name: repoPkgRev.KubeObjectName(),
549-
Namespace: repoPkgRev.KubeObjectNamespace(),
550-
Labels: newObj.Labels,
551-
Annotations: newObj.Annotations,
570+
pkgRevMeta, err = cad.updatePkgRevMeta(ctx, repoPkgRev, newObj)
571+
if err != nil {
572+
return nil, err
552573
}
553-
cad.metadataStore.Update(ctx, pkgRevMeta)
554574

555575
cad.watcherManager.NotifyPackageRevisionChange(watch.Modified, repoPkgRev, pkgRevMeta)
556-
return &PackageRevision{
557-
repoPackageRevision: repoPkgRev,
558-
packageRevisionMeta: pkgRevMeta,
559-
}, nil
576+
return ToPackageRevision(repoPkgRev, pkgRevMeta), nil
560577
}
561578
switch lifecycle := newObj.Spec.Lifecycle; lifecycle {
562579
default:
@@ -575,19 +592,13 @@ func (cad *cadEngine) UpdatePackageRevision(ctx context.Context, repositoryObj *
575592
return nil, err
576593
}
577594

578-
pkgRevMeta := meta.PackageRevisionMeta{
579-
Name: repoPkgRev.KubeObjectName(),
580-
Namespace: repoPkgRev.KubeObjectNamespace(),
581-
Labels: newObj.Labels,
582-
Annotations: newObj.Annotations,
595+
pkgRevMeta, err = cad.updatePkgRevMeta(ctx, repoPkgRev, newObj)
596+
if err != nil {
597+
return nil, err
583598
}
584-
cad.metadataStore.Update(ctx, pkgRevMeta)
585599

586600
cad.watcherManager.NotifyPackageRevisionChange(watch.Modified, repoPkgRev, pkgRevMeta)
587-
return &PackageRevision{
588-
repoPackageRevision: repoPkgRev,
589-
packageRevisionMeta: pkgRevMeta,
590-
}, nil
601+
return ToPackageRevision(repoPkgRev, pkgRevMeta), nil
591602
}
592603

593604
var mutations []mutation
@@ -676,24 +687,30 @@ func (cad *cadEngine) UpdatePackageRevision(ctx context.Context, repositoryObj *
676687
}
677688

678689
// Updates are done.
679-
repoPkgRev, err := draft.Close(ctx)
690+
repoPkgRev, err = draft.Close(ctx)
680691
if err != nil {
681692
return nil, err
682693
}
683694

684-
pkgRevMeta := meta.PackageRevisionMeta{
685-
Name: repoPkgRev.KubeObjectName(),
686-
Namespace: repoPkgRev.KubeObjectNamespace(),
687-
Labels: newObj.Labels,
688-
Annotations: newObj.Annotations,
695+
pkgRevMeta, err = cad.updatePkgRevMeta(ctx, repoPkgRev, newObj)
696+
if err != nil {
697+
return nil, err
689698
}
690-
cad.metadataStore.Update(ctx, pkgRevMeta)
691699

692700
cad.watcherManager.NotifyPackageRevisionChange(watch.Modified, repoPkgRev, pkgRevMeta)
693-
return &PackageRevision{
694-
repoPackageRevision: repoPkgRev,
695-
packageRevisionMeta: pkgRevMeta,
696-
}, nil
701+
return ToPackageRevision(repoPkgRev, pkgRevMeta), nil
702+
}
703+
704+
func (cad *cadEngine) updatePkgRevMeta(ctx context.Context, repoPkgRev repository.PackageRevision, apiPkgRev *api.PackageRevision) (meta.PackageRevisionMeta, error) {
705+
pkgRevMeta := meta.PackageRevisionMeta{
706+
Name: repoPkgRev.KubeObjectName(),
707+
Namespace: repoPkgRev.KubeObjectNamespace(),
708+
Labels: apiPkgRev.Labels,
709+
Annotations: apiPkgRev.Annotations,
710+
Finalizers: apiPkgRev.Finalizers,
711+
OwnerReferences: apiPkgRev.OwnerReferences,
712+
}
713+
return cad.metadataStore.Update(ctx, pkgRevMeta)
697714
}
698715

699716
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 *
814831
return err
815832
}
816833

817-
if err := repo.DeletePackageRevision(ctx, oldPackage.repoPackageRevision); err != nil {
818-
return err
819-
}
820-
834+
// We delete the PackageRev regardless of any finalizers, since it
835+
// will always have the same finalizers as the PackageRevision. This
836+
// will put the PackageRev, and therefore the PackageRevision in the
837+
// terminating state.
838+
// But we only delete the PackageRevision from the repo once all finalizers
839+
// have been removed.
821840
namespacedName := types.NamespacedName{
822841
Name: oldPackage.repoPackageRevision.KubeObjectName(),
823842
Namespace: oldPackage.repoPackageRevision.KubeObjectNamespace(),
824843
}
825-
if _, err := cad.metadataStore.Delete(ctx, namespacedName); err != nil {
844+
pkgRevMeta, err := cad.metadataStore.Delete(ctx, namespacedName, false)
845+
if err != nil {
826846
return err
827847
}
828848

829-
cad.watcherManager.NotifyPackageRevisionChange(watch.Deleted, oldPackage.repoPackageRevision, oldPackage.packageRevisionMeta)
849+
if len(pkgRevMeta.Finalizers) > 0 {
850+
klog.Infof("PackageRevision %s deleted, but still have finalizers: %s", oldPackage.KubeObjectName(), strings.Join(pkgRevMeta.Finalizers, ","))
851+
cad.watcherManager.NotifyPackageRevisionChange(watch.Modified, oldPackage.repoPackageRevision, oldPackage.packageRevisionMeta)
852+
return nil
853+
}
854+
klog.Infof("PackageRevision %s deleted for real since no finalizers", oldPackage.KubeObjectName())
855+
856+
return cad.deletePackageRevision(ctx, repo, oldPackage.repoPackageRevision, oldPackage.packageRevisionMeta)
857+
}
858+
859+
func (cad *cadEngine) deletePackageRevision(ctx context.Context, repo repository.Repository, repoPkgRev repository.PackageRevision, pkgRevMeta meta.PackageRevisionMeta) error {
860+
ctx, span := tracer.Start(ctx, "cadEngine::deletePackageRevision", trace.WithAttributes())
861+
defer span.End()
862+
863+
if err := repo.DeletePackageRevision(ctx, repoPkgRev); err != nil {
864+
return err
865+
}
866+
867+
nn := types.NamespacedName{
868+
Name: pkgRevMeta.Name,
869+
Namespace: pkgRevMeta.Namespace,
870+
}
871+
if _, err := cad.metadataStore.Delete(ctx, nn, true); err != nil {
872+
// If this fails, the CR will be cleaned up by the background job.
873+
klog.Warningf("Error deleting PkgRevMeta %s: %v", nn.String(), err)
874+
}
875+
876+
cad.watcherManager.NotifyPackageRevisionChange(watch.Deleted, repoPkgRev, pkgRevMeta)
830877
return nil
831878
}
832879

‎porch/pkg/engine/fake/packagerevision.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,12 +20,14 @@ import (
2020
kptfile "github.com/GoogleContainerTools/kpt/pkg/api/kptfile/v1"
2121
"github.com/GoogleContainerTools/kpt/porch/api/porch/v1alpha1"
2222
"github.com/GoogleContainerTools/kpt/porch/pkg/repository"
23+
"k8s.io/apimachinery/pkg/types"
2324
)
2425

2526
// Implementation of the repository.PackageRevision interface for testing.
2627
type PackageRevision struct {
2728
Name string
2829
Namespace string
30+
Uid types.UID
2931
PackageRevisionKey repository.PackageRevisionKey
3032
PackageLifecycle v1alpha1.PackageRevisionLifecycle
3133
PackageRevision *v1alpha1.PackageRevision
@@ -41,6 +43,10 @@ func (pr *PackageRevision) KubeObjectNamespace() string {
4143
return pr.Namespace
4244
}
4345

46+
func (pr *PackageRevision) UID() types.UID {
47+
return pr.Uid
48+
}
49+
4450
func (pr *PackageRevision) Key() repository.PackageRevisionKey {
4551
return pr.PackageRevisionKey
4652
}

‎porch/pkg/git/package.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,10 @@ func (p *gitPackageRevision) KubeObjectNamespace() string {
7272
return p.repo.namespace
7373
}
7474

75+
func (p *gitPackageRevision) UID() types.UID {
76+
return p.uid()
77+
}
78+
7579
func (p *gitPackageRevision) Key() repository.PackageRevisionKey {
7680
return repository.PackageRevisionKey{
7781
Repository: p.repo.name,

‎porch/pkg/meta/fake/memorystore.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ func (m *MemoryMetadataStore) List(ctx context.Context, repo *configapi.Reposito
4848
return m.Metas, nil
4949
}
5050

51-
func (m *MemoryMetadataStore) Create(ctx context.Context, pkgRevMeta meta.PackageRevisionMeta, repo *configapi.Repository) (meta.PackageRevisionMeta, error) {
51+
func (m *MemoryMetadataStore) Create(ctx context.Context, pkgRevMeta meta.PackageRevisionMeta, repoName string, pkgRevUID types.UID) (meta.PackageRevisionMeta, error) {
5252
for _, m := range m.Metas {
5353
if m.Name == pkgRevMeta.Name && m.Namespace == pkgRevMeta.Namespace {
5454
return m, apierrors.NewAlreadyExists(
@@ -78,7 +78,7 @@ func (m *MemoryMetadataStore) Update(ctx context.Context, pkgRevMeta meta.Packag
7878
return pkgRevMeta, nil
7979
}
8080

81-
func (m *MemoryMetadataStore) Delete(ctx context.Context, namespacedName types.NamespacedName) (meta.PackageRevisionMeta, error) {
81+
func (m *MemoryMetadataStore) Delete(ctx context.Context, namespacedName types.NamespacedName, _ bool) (meta.PackageRevisionMeta, error) {
8282
var metas []meta.PackageRevisionMeta
8383
found := false
8484
var deletedMeta meta.PackageRevisionMeta

0 commit comments

Comments
 (0)