From e89aad676c917d1e7b8e7a05b462b731d435e358 Mon Sep 17 00:00:00 2001 From: Sascha Schwarze Date: Thu, 25 Feb 2021 13:39:14 +0100 Subject: [PATCH] Correct and optimize reconciliation on deletion --- pkg/reconciler/build/controller.go | 4 +- pkg/reconciler/buildrun/buildrun.go | 6 +- pkg/reconciler/buildrun/controller.go | 10 +-- .../integration/buildruns_to_taskruns_test.go | 62 +++++++++++++++++++ test/integration/utils/taskruns.go | 11 ++++ 5 files changed, 84 insertions(+), 9 deletions(-) diff --git a/pkg/reconciler/build/controller.go b/pkg/reconciler/build/controller.go index 686717330..1acfce0dc 100644 --- a/pkg/reconciler/build/controller.go +++ b/pkg/reconciler/build/controller.go @@ -74,8 +74,8 @@ func add(ctx context.Context, mgr manager.Manager, r reconcile.Reconciler) error return e.MetaOld.GetGeneration() != e.MetaNew.GetGeneration() || buildRunDeletionAnnotation }, DeleteFunc: func(e event.DeleteEvent) bool { - // Evaluates to false if the object has been confirmed deleted. - return !e.DeleteStateUnknown + // Never reconcile on deletion, there is nothing we have to do + return false }, } diff --git a/pkg/reconciler/buildrun/buildrun.go b/pkg/reconciler/buildrun/buildrun.go index d3095ed94..2e2096928 100644 --- a/pkg/reconciler/buildrun/buildrun.go +++ b/pkg/reconciler/buildrun/buildrun.go @@ -307,7 +307,7 @@ func (r *ReconcileBuildRun) GetBuildRunObject(ctx context.Context, objectName st } // VerifyRequestName parse a Reconcile request name and looks for an associated BuildRun name -// If the BuildRun object exists, it will update it with an error. +// If the BuildRun object exists and is not yet completed, it will update it with an error. func (r *ReconcileBuildRun) VerifyRequestName(ctx context.Context, request reconcile.Request, buildRun *buildv1alpha1.BuildRun) { regxBuildRun, _ := regexp.Compile(generatedNameRegex) @@ -319,11 +319,11 @@ func (r *ReconcileBuildRun) VerifyRequestName(ctx context.Context, request recon if split := regxBuildRun.Split(request.Name, 2); len(split) > 0 { // Update the related BuildRun err := r.GetBuildRunObject(ctx, split[0], request.Namespace, buildRun) - if err == nil { + if err == nil && buildRun.Status.CompletionTime == nil { // We ignore the errors from the following call, because the parent call of this function will always // return back a reconcile.Result{}, nil. This is done to avoid infinite reconcile loops when a BuildRun // does not longer exists - r.updateBuildRunErrorStatus(ctx, buildRun, fmt.Sprintf("taskRun %s doesn´t exist", request.Name)) + r.updateBuildRunErrorStatus(ctx, buildRun, fmt.Sprintf("taskRun %s doesn't exist", request.Name)) } } } diff --git a/pkg/reconciler/buildrun/controller.go b/pkg/reconciler/buildrun/controller.go index ac66a3e4b..a32b80485 100644 --- a/pkg/reconciler/buildrun/controller.go +++ b/pkg/reconciler/buildrun/controller.go @@ -65,8 +65,8 @@ func add(ctx context.Context, mgr manager.Manager, r reconcile.Reconciler) error return e.MetaOld.GetGeneration() != e.MetaNew.GetGeneration() }, DeleteFunc: func(e event.DeleteEvent) bool { - // Evaluates to false if the object has been confirmed deleted. - return !e.DeleteStateUnknown + // Never reconcile on deletion, there is nothing we have to do + return false }, } @@ -90,8 +90,10 @@ func add(ctx context.Context, mgr manager.Manager, r reconcile.Reconciler) error return false }, DeleteFunc: func(e event.DeleteEvent) bool { - // Evaluates to false if the object has been confirmed deleted. - return !e.DeleteStateUnknown + o := e.Object.(*v1beta1.TaskRun) + + // If the TaskRun was deleted before completion, then we reconcile to update the BuildRun to a Failed status + return o.Status.CompletionTime == nil }, } diff --git a/test/integration/buildruns_to_taskruns_test.go b/test/integration/buildruns_to_taskruns_test.go index 32914c774..b027a83a4 100644 --- a/test/integration/buildruns_to_taskruns_test.go +++ b/test/integration/buildruns_to_taskruns_test.go @@ -393,4 +393,66 @@ var _ = Describe("Integration tests BuildRuns and TaskRuns", func() { Expect(err).To(BeNil(), fmt.Sprintf("failed to get desired reason; expected %s, got %s", expectedReason, actualReason)) }) }) + + Context("when a buildrun is created and the taskrun deleted before completion", func() { + + BeforeEach(func() { + buildSample = []byte(test.BuildCBSMinimal) + buildRunSample = []byte(test.MinimalBuildRun) + }) + + It("should reflect a Failed reason", func() { + + Expect(tb.CreateBuild(buildObject)).To(BeNil()) + + buildObject, err = tb.GetBuildTillValidation(buildObject.Name) + Expect(err).To(BeNil()) + + Expect(tb.CreateBR(buildRunObject)).To(BeNil()) + + _, err = tb.GetBRTillStartTime(buildRunObject.Name) + Expect(err).To(BeNil()) + + tr, err := tb.GetTaskRunFromBuildRun(buildRunObject.Name) + Expect(err).To(BeNil()) + + tb.DeleteTR(tr.Name) + + expectedReason := fmt.Sprintf("taskRun %s doesn't exist", tr.Name) + actualReason, err := tb.GetBRTillDesiredReason(buildRunObject.Name, expectedReason) + Expect(err).To(BeNil(), fmt.Sprintf("failed to get desired reason; expected %s, got %s", expectedReason, actualReason)) + }) + }) + + Context("when a buildrun is created and the taskrun deleted after successful completion", func() { + + It("should reflect a Success reason", func() { + WithCustomClusterBuildStrategy([]byte(test.ClusterBuildStrategyNoOp), func() { + _, _, buildRunObject := setupBuildAndBuildRun([]byte(test.BuildCBSMinimal), []byte(test.MinimalBuildRun), STRATEGY+tb.Namespace+"custom") + + _, err = tb.GetBRTillCompletion(buildRunObject.Name) + Expect(err).To(BeNil()) + + reason, err := tb.GetBRReason(buildRunObject.Name) + Expect(err).To(BeNil()) + Expect(reason).To(Equal("Succeeded")) + + tr, err := tb.GetTaskRunFromBuildRun(buildRunObject.Name) + Expect(err).To(BeNil()) + Expect(tr.Status.CompletionTime).NotTo(BeNil()) + + tb.DeleteTR(tr.Name) + + // in a test case, it is hard to verify that something (marking the BuildRun failed) is not happening, we quickly check the TaskRun is gone and + // check one more time that the BuildRun is still Succeeded + _, err = tb.GetTaskRunFromBuildRun(buildRunObject.Name) + Expect(err).NotTo(BeNil()) + Expect(err.Error()).To(ContainSubstring("failed to find an owned TaskRun")) + + reason, err = tb.GetBRReason(buildRunObject.Name) + Expect(err).To(BeNil()) + Expect(reason).To(Equal("Succeeded")) + }) + }) + }) }) diff --git a/test/integration/utils/taskruns.go b/test/integration/utils/taskruns.go index 5d3bb79fb..2a6017e97 100644 --- a/test/integration/utils/taskruns.go +++ b/test/integration/utils/taskruns.go @@ -79,3 +79,14 @@ func (t *TestBuild) GetTRTillDesiredReason(buildRunName string, reason string) ( return } + +// DeleteTR deletes a TaskRun from a desired namespace +func (t *TestBuild) DeleteTR(name string) error { + trInterface := t.PipelineClientSet.TektonV1beta1().TaskRuns(t.Namespace) + + if err := trInterface.Delete(context.TODO(), name, metav1.DeleteOptions{}); err != nil { + return err + } + + return nil +}