fix(pipelinerun): resolve issue where canceling active pipelinerun fails

fix #8172 When canceling a PipelineRun, validation errors returned when canceling a completed TaskRun should be ignored.
tektoncd · Aug 3, 2024 · a5d5378 · a5d5378
1 parent dd36119
commit a5d5378
Show file tree

Hide file tree

Showing 2 changed files with 135 additions and 0 deletions.
diff --git a/pkg/reconciler/pipelinerun/cancel.go b/pkg/reconciler/pipelinerun/cancel.go
@@ -88,6 +88,11 @@ func cancelTaskRun(ctx context.Context, taskRunName string, namespace string, cl
 		// still be able to cancel the PipelineRun
 		return nil
 	}
+	if errors.IsBadRequest(err) && strings.Contains(err.Error(), "no updates are allowed") {
+		// The TaskRun may have completed and the spec field is immutable.
+		// validation code: https://github.com/tektoncd/pipeline/blob/v0.62.0/pkg/apis/pipeline/v1/taskrun_validation.go#L136-L138
+		return nil
+	}
 	return err
 }
 

diff --git a/test/cancel_test.go b/test/cancel_test.go
@@ -184,3 +184,133 @@ spec:
 		})
 	}
 }
+
+// TestCancelActivePipelineRunWithCompletedTaskRuns cancels a PipelineRun with completed TaskRuns and verifies TaskRun statuses.
+func TestCancelActivePipelineRunWithCompletedTaskRuns(t *testing.T) {
+	specStatus := v1.PipelineRunSpecStatusCancelled
+	t.Run(fmt.Sprintf("status=%s", specStatus), func(t *testing.T) {
+		ctx := context.Background()
+		ctx, cancel := context.WithCancel(ctx)
+		defer cancel()
+		requirements := []func(context.Context, *testing.T, *clients, string){}
+		c, namespace := setup(ctx, t, requirements...)
+		t.Parallel()
+
+		knativetest.CleanupOnInterrupt(func() { tearDown(ctx, t, c, namespace) }, t.Logf)
+		defer tearDown(ctx, t, c, namespace)
+
+		pipelineRun := parse.MustParseV1PipelineRun(t, fmt.Sprintf(`
+metadata:
+  name: %s
+  namespace: %s
+spec:
+  pipelineSpec:
+    tasks:
+    - name: task-succeeded
+      taskSpec:
+        steps:
+        - image: mirror.gcr.io/busybox
+          script: 'sleep 1'
+    - name: task-running
+      taskSpec:
+        steps:
+        - image: mirror.gcr.io/busybox
+          script: 'sleep 5000'
+`, helpers.ObjectNameForTest(t), namespace))
+
+		t.Logf("Creating PipelineRun in namespace %s", namespace)
+		if _, err := c.V1PipelineRunClient.Create(ctx, pipelineRun, metav1.CreateOptions{}); err != nil {
+			t.Fatalf("Failed to create PipelineRun `%s`: %s", pipelineRun.Name, err)
+		}
+
+		t.Logf("Waiting for Pipelinerun %s in namespace %s to be started", pipelineRun.Name, namespace)
+		if err := WaitForPipelineRunState(ctx, c, pipelineRun.Name, timeout, Running(pipelineRun.Name), "PipelineRunRunning", v1Version); err != nil {
+			t.Fatalf("Error waiting for PipelineRun %s to be running: %s", pipelineRun.Name, err)
+		}
+
+		taskrunList, err := c.V1TaskRunClient.List(ctx, metav1.ListOptions{LabelSelector: "tekton.dev/pipelineRun=" + pipelineRun.Name})
+		if err != nil {
+			t.Fatalf("Error listing TaskRuns for PipelineRun %s: %s", pipelineRun.Name, err)
+		}
+
+		t.Logf("Waiting for PipelineRun %s from namespace %s with one TaskRun succeeded and another one running.", pipelineRun.Name, namespace)
+		for _, taskrunItem := range taskrunList.Items {
+			name := taskrunItem.Name
+			switch n := taskrunItem.Labels["tekton.dev/pipelineTask"]; {
+			case n == "task-succeeded":
+				err := WaitForTaskRunState(ctx, c, name, TaskRunSucceed(name), "TaskRunSuccess", v1Version)
+				if err != nil {
+					t.Errorf("Error waiting for TaskRun %s to be succeed: %v", name, err)
+				}
+			case n == "task-running":
+				err := WaitForTaskRunState(ctx, c, name, Running(name), "TaskRunRunning", v1Version)
+				if err != nil {
+					t.Errorf("Error waiting for TaskRun %s to be running: %v", name, err)
+				}
+			}
+		}
+
+		pr, err := c.V1PipelineRunClient.Get(ctx, pipelineRun.Name, metav1.GetOptions{})
+		if err != nil {
+			t.Fatalf("Failed to get PipelineRun `%s`: %s", pipelineRun.Name, err)
+		}
+
+		patches := []jsonpatch.JsonPatchOperation{{
+			Operation: "add",
+			Path:      "/spec/status",
+			Value:     specStatus,
+		}}
+		patchBytes, err := json.Marshal(patches)
+		if err != nil {
+			t.Fatalf("failed to marshal patch bytes in order to cancel")
+		}
+		if _, err := c.V1PipelineRunClient.Patch(ctx, pr.Name, types.JSONPatchType, patchBytes, metav1.PatchOptions{}, ""); err != nil {
+			t.Fatalf("Failed to patch PipelineRun `%s` with cancellation: %s", pipelineRun.Name, err)
+		}
+
+		expectedReason := v1.PipelineRunReasonCancelled.String()
+		expectedCondition := FailedWithReason(expectedReason, pipelineRun.Name)
+		t.Logf("Waiting for PipelineRun %s in namespace %s to be cancelled", pipelineRun.Name, namespace)
+		if err := WaitForPipelineRunState(ctx, c, pipelineRun.Name, timeout, expectedCondition, expectedReason, v1Version); err != nil {
+			t.Errorf("Error waiting for PipelineRun %q to finished: %s", pipelineRun.Name, err)
+		}
+
+		t.Logf("Waiting for TaskRuns in PipelineRun %s in namespace %s to be cancelled", pipelineRun.Name, namespace)
+		for _, taskrunItem := range taskrunList.Items {
+			name := taskrunItem.Name
+			switch n := taskrunItem.Labels["tekton.dev/pipelineTask"]; {
+			case n == "task-running":
+				err := WaitForTaskRunState(ctx, c, name, FailedWithReason("TaskRunCancelled", name), "TaskRunCancelled", v1Version)
+				if err != nil {
+					t.Errorf("Error waiting for TaskRun %s to be finished: %v", name, err)
+				}
+			}
+		}
+
+		taskrunList, err = c.V1TaskRunClient.List(ctx, metav1.ListOptions{LabelSelector: "tekton.dev/pipelineRun=" + pipelineRun.Name})
+		if err != nil {
+			t.Fatalf("Error listing TaskRuns for PipelineRun %s: %s", pipelineRun.Name, err)
+		}
+		for _, taskrunItem := range taskrunList.Items {
+			switch n := taskrunItem.Labels["tekton.dev/pipelineTask"]; {
+			case n == "task-succeeded":
+				// the completed TaskRun should not be changed
+				if taskrunItem.Spec.Status != "" {
+					t.Fatalf("The status is %s, but it should be empty.", taskrunItem.Spec.Status)
+				}
+				if taskrunItem.Spec.StatusMessage != "" {
+					t.Fatalf("Status message is set to %s while it should be empty.", taskrunItem.Spec.StatusMessage)
+				}
+			case n == "task-running":
+				// the running TaskRun should be cancelled
+				if taskrunItem.Spec.Status != v1.TaskRunSpecStatusCancelled {
+					t.Fatalf("Status is %s while it should have been %s", taskrunItem.Spec.Status, v1.TaskRunSpecStatusCancelled)
+				}
+				if taskrunItem.Spec.StatusMessage != v1.TaskRunCancelledByPipelineMsg {
+					t.Fatalf("Status message is set to %s while it should be %s.", taskrunItem.Spec.StatusMessage, v1.TaskRunCancelledByPipelineMsg)
+				}
+			}
+		}
+
+	})
+}