Repository: tektoncd/results (go)

Issue:
TaskRuns gets deleted  before PipelineRun completes if check_owner flag is disabled
# Expected Behavior

TaskRuns which are part of a PipelineRun should not get deleted separately. Instead should wait for parent deletion.

# Actual Behavior

TaskRuns gets deleted  before PipelineRun completes if check_owner flag is disabled


Note:
Right now this is a bug because the Pipeline Controller doesn't handle the edge case where the child TaskRun can be deleted without deleting the parent PipelineRun, and as a result the deletion causes the PipelineRun object to get updated with a "Pending" status for that task. 


Pull request:
Fix deletion when TaskRun is part of PipelineRun
Fixes #689 

# Changes

Prevent deletion of any object OwnerRef check is disabled and yet the object has a PipelineRun as owner ref.


This is needed because the Pipeline Controller doesn't handle the edge case where the child TaskRun can be deleted without deleting the parent PipelineRun, and as a result the deletion causes the PipelineRun object to get updated with a "Pending" status for that task.

If an aggressive pruning is required, this can be removed in future once the above scenario is handled by the Pipeline Controller.

# Submitter Checklist

These are the criteria that every PR should meet, please check them off as you review them:

- [ ] Has [Docs](https://github.com/tektoncd/community/blob/main/standards.md#docs) included if any changes are user facing
- [x] Has [Tests](https://github.com/tektoncd/community/blob/main/standards.md#tests) included if any functionality added or changed
- [x] [Tested your changes locally](https://github.com/tektoncd/results/blob/main/docs/DEVELOPMENT.md) (if this is a code change)
- [x] Follows the [commit message standard](https://github.com/tektoncd/community/blob/main/standards.md#commits)
- [x] Meets the [Tekton contributor standards](https://github.com/tektoncd/community/blob/main/standards.md) (including functionality, content, code)
- [x] Has a kind label. You can add a comment on this PR that contains `/kind <type>`. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tep
- [x] Release notes block below has been updated with any user-facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings)
- [ ] Release notes contain the string "action required" if the change requires additional action from users switching to the new release

# Release Notes

```release-note
This change will prevent the controller from directly pruning TaskRuns which are initiated by a PipelineRun. Instead these TaskRuns will be deleted only when the owner PipelineRun is deleted.
```

Patch:
diff --git a/pkg/watcher/reconciler/dynamic/dynamic.go b/pkg/watcher/reconciler/dynamic/dynamic.go
index c2c236e1..5242c68a 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)


Test patch:
diff --git a/pkg/watcher/reconciler/dynamic/dynamic_test.go b/pkg/watcher/reconciler/dynamic/dynamic_test.go
index fa3db4f3..6c3f4f18 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)