From 7ae603e84e85ffeb9a4fd03758b54aafd8da7e29 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Moreno=20Garc=C3=ADa?= Date: Wed, 16 Aug 2023 12:28:35 +0200 Subject: [PATCH] feat(HACBS-2422): add finalizer to Release PRs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This change adds a finalizer to Release PipelineRuns to avoid their deletion by external entities until the Release service has finished processing them. Notice that tests relying on finalizeRelease don't check if the PipelineRun was successfully deleted anymore. The removal of the finalizer is not instant so to test that, we would have to use Eventually blocks, which we know are flaky. Signed-off-by: David Moreno García --- controllers/release/adapter.go | 29 ++++++++++++++++++++++------- controllers/release/adapter_test.go | 22 ++++++++++++++++++---- metadata/finalizers.go | 4 ++++ tekton/pipeline_run.go | 4 +++- tekton/pipeline_run_test.go | 1 + 5 files changed, 48 insertions(+), 12 deletions(-) create mode 100644 metadata/finalizers.go diff --git a/controllers/release/adapter.go b/controllers/release/adapter.go index e480dd314..b80f9b7ac 100644 --- a/controllers/release/adapter.go +++ b/controllers/release/adapter.go @@ -54,9 +54,6 @@ type adapter struct { syncer *syncer.Syncer } -// finalizerName is the finalizer name to be added to the Releases -const finalizerName string = "appstudio.redhat.com/release-finalizer" - // newAdapter creates and returns an adapter instance. func newAdapter(ctx context.Context, client client.Client, release *v1alpha1.Release, loader loader.ObjectLoader, logger *logr.Logger) *adapter { return &adapter{ @@ -79,13 +76,13 @@ func (a *adapter) EnsureFinalizersAreCalled() (controller.OperationResult, error return controller.ContinueProcessing() } - if controllerutil.ContainsFinalizer(a.release, finalizerName) { + if controllerutil.ContainsFinalizer(a.release, metadata.ReleaseFinalizer) { if err := a.finalizeRelease(); err != nil { return controller.RequeueWithError(err) } patch := client.MergeFrom(a.release.DeepCopy()) - controllerutil.RemoveFinalizer(a.release, finalizerName) + controllerutil.RemoveFinalizer(a.release, metadata.ReleaseFinalizer) err := a.client.Patch(a.ctx, a.release, patch) if err != nil { return controller.RequeueWithError(err) @@ -100,7 +97,7 @@ func (a *adapter) EnsureFinalizersAreCalled() (controller.OperationResult, error func (a *adapter) EnsureFinalizerIsAdded() (controller.OperationResult, error) { var finalizerFound bool for _, finalizer := range a.release.GetFinalizers() { - if finalizer == finalizerName { + if finalizer == metadata.ReleaseFinalizer { finalizerFound = true } } @@ -108,7 +105,7 @@ func (a *adapter) EnsureFinalizerIsAdded() (controller.OperationResult, error) { if !finalizerFound { a.logger.Info("Adding Finalizer to the Release") patch := client.MergeFrom(a.release.DeepCopy()) - controllerutil.AddFinalizer(a.release, finalizerName) + controllerutil.AddFinalizer(a.release, metadata.ReleaseFinalizer) err := a.client.Patch(a.ctx, a.release, patch) return controller.RequeueOnErrorOrContinue(err) @@ -374,6 +371,19 @@ func (a *adapter) finalizeRelease() error { } if pipelineRun != nil { + // The finalizer could still exist at this point. It should be removed if the Release is finalized + if controllerutil.ContainsFinalizer(pipelineRun, metadata.ReleaseFinalizer) { + patch := client.MergeFrom(pipelineRun) + removedFinalizer := controllerutil.RemoveFinalizer(pipelineRun, metadata.ReleaseFinalizer) + if !removedFinalizer { + return fmt.Errorf("finalizer not removed") + } + err := a.client.Patch(a.ctx, pipelineRun, patch) + if err != nil { + return err + } + } + err = a.client.Delete(a.ctx, pipelineRun) if err != nil && !errors.IsNotFound(err) { return err @@ -470,6 +480,11 @@ func (a *adapter) registerProcessingStatus(pipelineRun *v1beta1.PipelineRun) err a.release.MarkReleaseFailed("Release processing failed") } + // At this point it's safe to remove the PipelineRun, so the finalizer can be removed + if controllerutil.ContainsFinalizer(pipelineRun, metadata.ReleaseFinalizer) { + controllerutil.RemoveFinalizer(pipelineRun, metadata.ReleaseFinalizer) + } + return a.client.Status().Patch(a.ctx, a.release, patch) } diff --git a/controllers/release/adapter_test.go b/controllers/release/adapter_test.go index 8edf0f57e..805346301 100644 --- a/controllers/release/adapter_test.go +++ b/controllers/release/adapter_test.go @@ -127,7 +127,7 @@ var _ = Describe("Release adapter", Ordered, func() { Expect(err).NotTo(HaveOccurred()) pipelineRun, err := adapter.loader.GetReleasePipelineRun(adapter.ctx, adapter.client, adapter.release) - Expect(pipelineRun).To(BeNil()) + Expect(pipelineRun).To(Or(BeNil(), HaveField("DeletionTimestamp", Not(BeNil())))) Expect(err).NotTo(HaveOccurred()) _, err = adapter.loader.GetRelease(adapter.ctx, adapter.client, adapter.release.Name, adapter.release.Namespace) @@ -151,14 +151,14 @@ var _ = Describe("Release adapter", Ordered, func() { result, err := adapter.EnsureFinalizerIsAdded() Expect(!result.RequeueRequest && !result.CancelRequest).To(BeTrue()) Expect(err).NotTo(HaveOccurred()) - Expect(adapter.release.Finalizers).To(ContainElement(finalizerName)) + Expect(adapter.release.Finalizers).To(ContainElement(metadata.ReleaseFinalizer)) }) It("shouldn't fail if the Release already has the finalizer added", func() { result, err := adapter.EnsureFinalizerIsAdded() Expect(!result.RequeueRequest && !result.CancelRequest).To(BeTrue()) Expect(err).NotTo(HaveOccurred()) - Expect(adapter.release.Finalizers).To(ContainElement(finalizerName)) + Expect(adapter.release.Finalizers).To(ContainElement(metadata.ReleaseFinalizer)) result, err = adapter.EnsureFinalizerIsAdded() Expect(!result.RequeueRequest && !result.CancelRequest).To(BeTrue()) @@ -993,7 +993,7 @@ var _ = Describe("Release adapter", Ordered, func() { Expect(adapter.finalizeRelease()).To(Succeed()) pipelineRun, err = adapter.loader.GetReleasePipelineRun(adapter.ctx, adapter.client, adapter.release) - Expect(pipelineRun).To(BeNil()) + Expect(pipelineRun).To(Or(BeNil(), HaveField("DeletionTimestamp", Not(BeNil())))) Expect(err).NotTo(HaveOccurred()) }) }) @@ -1191,6 +1191,20 @@ var _ = Describe("Release adapter", Ordered, func() { Expect(adapter.release.HasProcessingFinished()).To(BeTrue()) Expect(adapter.release.IsProcessed()).To(BeFalse()) }) + + It("removes the finalizer if present", func() { + pipelineRun := &v1beta1.PipelineRun{ + ObjectMeta: ctrl.ObjectMeta{ + Finalizers: []string{metadata.ReleaseFinalizer}, + }, + } + pipelineRun.Status.MarkFailed("", "") + adapter.release.MarkProcessing("") + + Expect(adapter.registerProcessingStatus(pipelineRun)).To(Succeed()) + Expect(pipelineRun.Finalizers).To(BeEmpty()) + }) + }) When("calling syncResources", func() { diff --git a/metadata/finalizers.go b/metadata/finalizers.go new file mode 100644 index 000000000..bde232047 --- /dev/null +++ b/metadata/finalizers.go @@ -0,0 +1,4 @@ +package metadata + +// ReleaseFinalizer is the finalizer name to be added to the Releases +const ReleaseFinalizer string = "appstudio.redhat.com/release-finalizer" diff --git a/tekton/pipeline_run.go b/tekton/pipeline_run.go index d19667ea7..6ff8bcad5 100644 --- a/tekton/pipeline_run.go +++ b/tekton/pipeline_run.go @@ -20,6 +20,7 @@ import ( "encoding/json" "fmt" "os" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "strings" "unicode" @@ -123,9 +124,10 @@ func (r *ReleasePipelineRun) WithObjectReferences(objects ...client.Object) *Rel return r } -// WithOwner set's owner annotations to the release PipelineRun. +// WithOwner sets owner annotations to the release PipelineRun and a finalizer to prevent its deletion. func (r *ReleasePipelineRun) WithOwner(release *v1alpha1.Release) *ReleasePipelineRun { _ = libhandler.SetOwnerAnnotations(release, r) + controllerutil.AddFinalizer(r, metadata.ReleaseFinalizer) return r } diff --git a/tekton/pipeline_run_test.go b/tekton/pipeline_run_test.go index ca11eeda1..d242e3d78 100644 --- a/tekton/pipeline_run_test.go +++ b/tekton/pipeline_run_test.go @@ -172,6 +172,7 @@ var _ = Describe("PipelineRun", func() { It("can append owner release information to the object as annotations", func() { releasePipelineRun.WithOwner(release) Expect(releasePipelineRun.Annotations).NotTo(BeNil()) + Expect(releasePipelineRun.Finalizers).NotTo(BeEmpty()) }) It("can append the release Name, Namespace, and Application to a PipelineRun object and that these label key names match the correct label format", func() {