From bb31dbd13ba6c3c0e5b37c338275dcc7f726f9c5 Mon Sep 17 00:00:00 2001
From: Sayan Biswas <sayan-biswas@live.com>
Date: Fri, 5 Jan 2024 22:31:48 +0530
Subject: [PATCH] Fix deletion when TaskRun is part of PipelineRun

In case a TaskRun is initiated by a PipelineRun, the ownerRef is internally used by the pipeline controller to create dependency. In these cases, we don't want the TaskRuns to be deleted separately.
---
 pkg/watcher/reconciler/dynamic/dynamic.go     | 18 +++++++--
 .../reconciler/dynamic/dynamic_test.go        | 38 ++++++++++++++++++-
 2 files changed, 51 insertions(+), 5 deletions(-)

diff --git a/pkg/watcher/reconciler/dynamic/dynamic.go b/pkg/watcher/reconciler/dynamic/dynamic.go
index c2c236e13..5242c68aa 100644
--- a/pkg/watcher/reconciler/dynamic/dynamic.go
+++ b/pkg/watcher/reconciler/dynamic/dynamic.go
@@ -182,9 +182,21 @@ func (r *Reconciler) deleteUponCompletion(ctx context.Context, o results.Object)
 		return nil
 	}
 
-	if ownerReferences := o.GetOwnerReferences(); r.cfg.CheckOwner && len(ownerReferences) > 0 {
-		logger.Debugw("Resource is owned by another object, deferring deletion to parent resource(s)", zap.Any("results.tekton.dev/ownerReferences", ownerReferences))
-		return nil
+	if ownerReferences := o.GetOwnerReferences(); len(ownerReferences) > 0 {
+		// do not delete if the object is owned by a PipelineRun object
+		// This can be removed once the PipelineRun controller is patched to stop updating the PipelineRun object
+		// when child TaskRuns are deleted
+		for _, or := range ownerReferences {
+			if or.Kind == "PipelineRun" {
+				logger.Debugw("Resource is owned by a PipelineRun, deferring deletion to parent PipelineRun", zap.Any("tekton.dev/PipelineRun", or.Name))
+				return nil
+			}
+		}
+		// do not delete if CheckOwner flag is enabled and the object has some owner references
+		if r.cfg.CheckOwner {
+			logger.Debugw("Resource is owned by another object, deferring deletion to parent resource(s)", zap.Any("results.tekton.dev/ownerReferences", ownerReferences))
+			return nil
+		}
 	}
 
 	completionTime, err := getCompletionTime(o)
diff --git a/pkg/watcher/reconciler/dynamic/dynamic_test.go b/pkg/watcher/reconciler/dynamic/dynamic_test.go
index fa3db4f37..6c3f4f185 100644
--- a/pkg/watcher/reconciler/dynamic/dynamic_test.go
+++ b/pkg/watcher/reconciler/dynamic/dynamic_test.go
@@ -335,9 +335,12 @@ func TestReconcile_TaskRun(t *testing.T) {
 		taskrun.OwnerReferences = []metav1.OwnerReference{{
 			APIVersion: "v1",
 			Kind:       "test",
-			Name:       "test-parent",
+			Name:       "test-owner",
 		}}
 
+		// delete taskrun before creating
+		trclient.Delete(ctx, taskrun.GetName(), metav1.DeleteOptions{})
+
 		// Recreate the object to retest the deletion
 		if _, err := trclient.Create(ctx, taskrun, metav1.CreateOptions{}); err != nil {
 			t.Fatal(err)
@@ -353,16 +356,47 @@ func TestReconcile_TaskRun(t *testing.T) {
 		}
 	})
 
+	t.Run("do not delete object with owner references has PipelineRun object and owner check is disabled", func(t *testing.T) {
+		r.cfg.CheckOwner = false
+
+		taskrun.OwnerReferences = []metav1.OwnerReference{{
+			APIVersion: "tekton.dev/v1",
+			Kind:       "PipelineRun",
+			Name:       "test-pipelinerun",
+			UID:        types.UID(uuid.NewString()),
+		}}
+
+		// Delete taskrun before creating
+		trclient.Delete(ctx, taskrun.GetName(), metav1.DeleteOptions{})
+
+		// Recreate TaskRun object to retest the deletion
+		if _, err := trclient.Create(ctx, taskrun, metav1.CreateOptions{}); err != nil {
+			t.Fatal(err)
+		}
+
+		if err := r.Reconcile(ctx, taskrun); err != nil {
+			t.Fatal(err)
+		}
+
+		// Make sure that the TaskRun is not deleted
+		if _, err := trclient.Get(ctx, taskrun.GetName(), metav1.GetOptions{}); apierrors.IsNotFound(err) {
+			t.Fatalf("Want Found, but got %v", err)
+		}
+	})
+
 	t.Run("do not delete object with owner references when owner check is enabled", func(t *testing.T) {
 		r.cfg.CheckOwner = true
 
 		taskrun.OwnerReferences = []metav1.OwnerReference{{
 			APIVersion: "v1",
 			Kind:       "test",
-			Name:       "test-parent",
+			Name:       "test-owner",
 			UID:        types.UID(uuid.NewString()),
 		}}
 
+		// delete taskrun before creating
+		trclient.Delete(ctx, taskrun.GetName(), metav1.DeleteOptions{})
+
 		// Recreate the object to retest the deletion
 		if _, err := trclient.Create(ctx, taskrun, metav1.CreateOptions{}); err != nil {
 			t.Fatal(err)