From 00365bdc9bb4825d0b29acb296f8c34ea8ce9244 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=B3zes=20L=C3=A1szl=C3=B3=20M=C3=A1t=C3=A9?= Date: Thu, 6 Feb 2025 10:03:44 +0100 Subject: [PATCH 1/4] add additional info to error when blockOwnerDeletion is not false --- pkg/engine/engine.go | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go index c80eb5e6..b8ea6a69 100644 --- a/pkg/engine/engine.go +++ b/pkg/engine/engine.go @@ -198,6 +198,9 @@ func (cad *cadEngine) CreatePackageRevision(ctx context.Context, repositoryObj * } pkgRevMeta, err = cad.metadataStore.Create(ctx, pkgRevMeta, repositoryObj.Name, repoPkgRev.UID()) if err != nil { + if (apierrors.IsUnauthorized(err) || apierrors.IsForbidden(err)) && anyBlockOwnerDeletionSet(obj) { + return nil, fmt.Errorf("failed to create PackageRev because blockOwnerDeletion is enabled for some ownerReference: %w", err) + } return nil, err } repoPkgRev.SetMeta(pkgRevMeta) @@ -218,6 +221,15 @@ func ensureUniqueWorkspaceName(obj *api.PackageRevision, existingRevs []reposito return nil } +func anyBlockOwnerDeletionSet(pr *api.PackageRevision) bool { + for _, owner := range pr.OwnerReferences { + if owner.BlockOwnerDeletion == nil || *owner.BlockOwnerDeletion { + return true + } + } + return false +} + func (cad *cadEngine) UpdatePackageRevision(ctx context.Context, version string, repositoryObj *configapi.Repository, repoPr repository.PackageRevision, oldObj, newObj *api.PackageRevision, parent repository.PackageRevision) (repository.PackageRevision, error) { ctx, span := tracer.Start(ctx, "cadEngine::UpdatePackageRevision", trace.WithAttributes()) defer span.End() @@ -318,6 +330,9 @@ func (cad *cadEngine) UpdatePackageRevision(ctx context.Context, version string, err = cad.updatePkgRevMeta(ctx, repoPkgRev, newObj) if err != nil { + if (apierrors.IsUnauthorized(err) || apierrors.IsForbidden(err)) && anyBlockOwnerDeletionSet(newObj) { + return nil, fmt.Errorf("failed to update PackageRev because blockOwnerDeletion is set for some ownerReference: %w", err) + } return nil, err } From a6d9701ca45901586d6718d76cabd23f30d2131f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=B3zes=20L=C3=A1szl=C3=B3=20M=C3=A1t=C3=A9?= Date: Thu, 6 Feb 2025 15:08:57 +0100 Subject: [PATCH 2/4] move AnyBlockOwnerDeletion to repository/utils.go, made it more generic --- pkg/engine/engine.go | 13 ++----------- pkg/repository/util.go | 12 ++++++++++++ 2 files changed, 14 insertions(+), 11 deletions(-) diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go index b8ea6a69..168b845f 100644 --- a/pkg/engine/engine.go +++ b/pkg/engine/engine.go @@ -198,7 +198,7 @@ func (cad *cadEngine) CreatePackageRevision(ctx context.Context, repositoryObj * } pkgRevMeta, err = cad.metadataStore.Create(ctx, pkgRevMeta, repositoryObj.Name, repoPkgRev.UID()) if err != nil { - if (apierrors.IsUnauthorized(err) || apierrors.IsForbidden(err)) && anyBlockOwnerDeletionSet(obj) { + if (apierrors.IsUnauthorized(err) || apierrors.IsForbidden(err)) && repository.AnyBlockOwnerDeletionSet(obj) { return nil, fmt.Errorf("failed to create PackageRev because blockOwnerDeletion is enabled for some ownerReference: %w", err) } return nil, err @@ -221,15 +221,6 @@ func ensureUniqueWorkspaceName(obj *api.PackageRevision, existingRevs []reposito return nil } -func anyBlockOwnerDeletionSet(pr *api.PackageRevision) bool { - for _, owner := range pr.OwnerReferences { - if owner.BlockOwnerDeletion == nil || *owner.BlockOwnerDeletion { - return true - } - } - return false -} - func (cad *cadEngine) UpdatePackageRevision(ctx context.Context, version string, repositoryObj *configapi.Repository, repoPr repository.PackageRevision, oldObj, newObj *api.PackageRevision, parent repository.PackageRevision) (repository.PackageRevision, error) { ctx, span := tracer.Start(ctx, "cadEngine::UpdatePackageRevision", trace.WithAttributes()) defer span.End() @@ -330,7 +321,7 @@ func (cad *cadEngine) UpdatePackageRevision(ctx context.Context, version string, err = cad.updatePkgRevMeta(ctx, repoPkgRev, newObj) if err != nil { - if (apierrors.IsUnauthorized(err) || apierrors.IsForbidden(err)) && anyBlockOwnerDeletionSet(newObj) { + if (apierrors.IsUnauthorized(err) || apierrors.IsForbidden(err)) && repository.AnyBlockOwnerDeletionSet(newObj) { return nil, fmt.Errorf("failed to update PackageRev because blockOwnerDeletion is set for some ownerReference: %w", err) } return nil, err diff --git a/pkg/repository/util.go b/pkg/repository/util.go index 01b7cb23..4677f8f0 100644 --- a/pkg/repository/util.go +++ b/pkg/repository/util.go @@ -26,6 +26,7 @@ import ( "go.opentelemetry.io/otel" "go.opentelemetry.io/otel/trace" "golang.org/x/mod/semver" + "sigs.k8s.io/controller-runtime/pkg/client" ) var tracer = otel.Tracer("repository/util") @@ -136,3 +137,14 @@ func ValidateWorkspaceName(workspace api.WorkspaceName) error { return nil } + +// AnyBlockOwnerDeletionSet checks whether there are any ownerReferences in the Object +// which have blockOwnerDeletion enabled (meaning either nil or true). +func AnyBlockOwnerDeletionSet(obj client.Object) bool { + for _, owner := range obj.GetOwnerReferences() { + if owner.BlockOwnerDeletion == nil || *owner.BlockOwnerDeletion { + return true + } + } + return false +} From e174980c97e224b1aaed6fbcd4fdcc8de424a09a Mon Sep 17 00:00:00 2001 From: sriharsh Date: Fri, 14 Feb 2025 13:33:58 +0530 Subject: [PATCH 3/4] changed mutex in cache repository to read write mutex so that all concurrent reads are allowed --- pkg/cache/memory/repository.go | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/pkg/cache/memory/repository.go b/pkg/cache/memory/repository.go index 9363a61e..f44b0408 100644 --- a/pkg/cache/memory/repository.go +++ b/pkg/cache/memory/repository.go @@ -54,7 +54,7 @@ type cachedRepository struct { lastVersion string - mutex sync.Mutex + mutex sync.RWMutex cachedPackageRevisions map[repository.PackageRevisionKey]*cachedPackageRevision cachedPackages map[repository.PackageKey]*cachedPackage // Error encountered on repository refresh by the refresh goroutine. @@ -108,8 +108,8 @@ func (r *cachedRepository) ListPackageRevisions(ctx context.Context, filter repo } func (r *cachedRepository) getRefreshError() error { - r.mutex.Lock() - defer r.mutex.Unlock() + r.mutex.RLock() + defer r.mutex.RUnlock() // TODO: This should also check r.refreshPkgsError when // the package resource is fully supported. @@ -123,8 +123,8 @@ func (r *cachedRepository) getPackageRevisions(ctx context.Context, filter repos if err != nil { return nil, err } - r.mutex.Lock() - defer r.mutex.Unlock() + r.mutex.RLock() + defer r.mutex.RUnlock() return toPackageRevisionSlice(ctx, packageRevisions, filter), nil } @@ -134,8 +134,8 @@ func (r *cachedRepository) getPackages(ctx context.Context, filter repository.Li if err != nil { return nil, err } - r.mutex.Lock() - defer r.mutex.Unlock() + r.mutex.RLock() + defer r.mutex.RUnlock() return toPackageSlice(packages, filter), nil } @@ -143,7 +143,7 @@ func (r *cachedRepository) getPackages(ctx context.Context, filter repository.Li // mutex must be held. func (r *cachedRepository) getCachedPackages(ctx context.Context, forceRefresh bool) (map[repository.PackageKey]*cachedPackage, map[repository.PackageRevisionKey]*cachedPackageRevision, error) { // must hold mutex - + r.mutex.Lock() packages := r.cachedPackages packageRevisions := r.cachedPackageRevisions @@ -157,6 +157,7 @@ func (r *cachedRepository) getCachedPackages(ctx context.Context, forceRefresh b // TODO: Figure out a way to do this without the cache layer // needing to know what type of repo we are working with. if err := gitRepo.UpdateDeletionProposedCache(); err != nil { + r.mutex.Unlock() return nil, nil, err } } From e0db88d512eeb6b289c1f2aefc95a3826bd6e166 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=B3zes=20L=C3=A1szl=C3=B3=20M=C3=A1t=C3=A9?= Date: Mon, 17 Feb 2025 10:50:02 +0100 Subject: [PATCH 4/4] make error message even more verbose --- pkg/engine/engine.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go index 168b845f..ecc8c928 100644 --- a/pkg/engine/engine.go +++ b/pkg/engine/engine.go @@ -199,7 +199,8 @@ func (cad *cadEngine) CreatePackageRevision(ctx context.Context, repositoryObj * pkgRevMeta, err = cad.metadataStore.Create(ctx, pkgRevMeta, repositoryObj.Name, repoPkgRev.UID()) if err != nil { if (apierrors.IsUnauthorized(err) || apierrors.IsForbidden(err)) && repository.AnyBlockOwnerDeletionSet(obj) { - return nil, fmt.Errorf("failed to create PackageRev because blockOwnerDeletion is enabled for some ownerReference: %w", err) + return nil, fmt.Errorf("failed to create internal PackageRev object, because blockOwnerDeletion is enabled for some ownerReference "+ + "(it is likely that the serviceaccount of porch-server does not have the rights to update finalizers in the owner object): %w", err) } return nil, err } @@ -322,7 +323,8 @@ func (cad *cadEngine) UpdatePackageRevision(ctx context.Context, version string, err = cad.updatePkgRevMeta(ctx, repoPkgRev, newObj) if err != nil { if (apierrors.IsUnauthorized(err) || apierrors.IsForbidden(err)) && repository.AnyBlockOwnerDeletionSet(newObj) { - return nil, fmt.Errorf("failed to update PackageRev because blockOwnerDeletion is set for some ownerReference: %w", err) + return nil, fmt.Errorf("failed to update internal PackageRev object, because blockOwnerDeletion is enabled for some ownerReference "+ + "(it is likely that the serviceaccount of porch-server does not have the rights to update finalizers in the owner object): %w", err) } return nil, err }