Correct and optimize reconciliation on deletion

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
  },
  }
 

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))
  }
  }
  }

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
  },
  }
 

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"))
+ })
+ })
+ })
 })

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
+}