From 5c68ca3dcb96a2f6bc6a5c4948c5e55b6aaa3ce1 Mon Sep 17 00:00:00 2001 From: qingliu Date: Sat, 3 Aug 2024 12:51:59 +0800 Subject: [PATCH] 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. --- pkg/reconciler/pipelinerun/cancel.go | 5 + test/cancel_test.go | 131 +++++++++++++++++++++++++++ 2 files changed, 136 insertions(+) diff --git a/pkg/reconciler/pipelinerun/cancel.go b/pkg/reconciler/pipelinerun/cancel.go index 9b7dc48c440..7370080d3f7 100644 --- 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, we should ignore this error. + // 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 index 3f220b70de5..e5c10bd3e64 100644 --- a/test/cancel_test.go +++ b/test/cancel_test.go @@ -184,3 +184,134 @@ spec: }) } } + +// TestCancelActivePipelineRunWithCompletedTaskRuns cancels a PipelineRun with completed TaskRuns and verifies TaskRun statuses. +func TestCancelActivePipelineRunWithCompletedTaskRuns(t *testing.T) { + specStatus := v1.PipelineRunSpecStatusCancelled + t.Run("status="+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-succeeded": + // the completed TaskRun no need to wait + 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) + } + } + } + }) +}