From c9fbb3d984f163ecfa0e5bed515b2ac924df5b58 Mon Sep 17 00:00:00 2001 From: Jerop Date: Tue, 8 Jun 2021 15:52:00 -0400 Subject: [PATCH] Skipping Strategies This change implements skipping strategies to give users the flexibility to skip a single guarded Task only and unblock execution of its dependent Tasks. Today, WhenExpressions are specified within Tasks but they guard the Task and its dependent Tasks. To provide flexible skipping strategies, we want to change the scope of WhenExpressions from guarding a Task and its dependent Tasks to guarding the Task only. If a user wants to guard a Task and its dependent Tasks, they can: - cascade the WhenExpressions to the dependent Tasks - compose the Task and its dependent Tasks as a sub-Pipeline that's guarded and executed together using Pipelines in Pipelines (but this is still an experimental feature) Changing the scope of WhenExpressions to guard the Task only is backwards-incompatible, so to make the transition smooth: - we'll provide a feature flag, scope-when-expressions-to-task, which: - will default to false to guard a Task and its dependent Tasks - can be set to true to guard a Task only - after migration, we'll change the global default for the feature flag to true to guard a Task only by default - eventually, we'll remove the feature flag and guard a Task only going forward Implements [TEP-0059: Skipping Strategies](https://github.com/tektoncd/community/blob/main/teps/0059-skipping-strategies.md) Closes https://github.com/tektoncd/pipeline/issues/2127 --- config/config-feature-flags.yaml | 3 + docs/deprecations.md | 2 + docs/install.md | 4 + docs/pipelines.md | 231 ++++++- pkg/apis/config/feature_flags.go | 6 + pkg/apis/config/feature_flags_test.go | 7 + .../testdata/feature-flags-all-flags-set.yaml | 1 + ...nvalid-scope-when-expressions-to-task.yaml | 21 + pkg/reconciler/pipelinerun/pipelinerun.go | 11 +- .../pipelinerun/pipelinerun_test.go | 162 +++++ .../resources/pipelinerunresolution.go | 125 +++- .../resources/pipelinerunresolution_test.go | 619 +++++++++++++++++- .../pipelinerun/resources/pipelinerunstate.go | 22 +- 13 files changed, 1161 insertions(+), 53 deletions(-) create mode 100644 pkg/apis/config/testdata/feature-flags-invalid-scope-when-expressions-to-task.yaml diff --git a/config/config-feature-flags.yaml b/config/config-feature-flags.yaml index 9ab11ef6e4b..870ceadd409 100644 --- a/config/config-feature-flags.yaml +++ b/config/config-feature-flags.yaml @@ -82,3 +82,6 @@ data: # Setting this flag will determine which gated features are enabled. # Acceptable values are "stable" or "alpha". enable-api-fields: "stable" + # Setting this flag to "true" scopes when expressions to guard a Task only + # instead of a Task and its dependent Tasks. + scope-when-expressions-to-task: "false" diff --git a/docs/deprecations.md b/docs/deprecations.md index 0e344d3638c..0b949e7fe78 100644 --- a/docs/deprecations.md +++ b/docs/deprecations.md @@ -25,3 +25,5 @@ being deprecated. | [`Conditions` CRD is deprecated and will be removed. Use `when` expressions instead.](https://github.com/tektoncd/community/blob/main/teps/0007-conditions-beta.md) | [v0.16.0](https://github.com/tektoncd/pipeline/releases/tag/v0.16.0) | Alpha | Nov 02 2020 | | [The `disable-home-env-overwrite` flag will be removed](https://github.com/tektoncd/pipeline/issues/2013) | [v0.24.0](https://github.com/tektoncd/pipeline/releases/tag/v0.24.0) | Beta | February 10 2022 | | [The `disable-working-dir-overwrite` flag will be removed](https://github.com/tektoncd/pipeline/issues/1836) | [v0.24.0](https://github.com/tektoncd/pipeline/releases/tag/v0.24.0) | Beta | February 10 2022 | +| [The `scope-when-expressions-to-task` flag will be flipped from "false" to "true"](https://github.com/tektoncd/pipeline/issues/1836) | [v0.27.0](https://github.com/tektoncd/pipeline/releases/tag/v0.27.0) | Beta | February 10 2022 | +| [The `scope-when-expressions-to-task` flag will be removed](https://github.com/tektoncd/pipeline/issues/1836) | [v0.27.0](https://github.com/tektoncd/pipeline/releases/tag/v0.27.0) | Beta | March 10 2022 | diff --git a/docs/install.md b/docs/install.md index 5ca60d67f56..f09487f914b 100644 --- a/docs/install.md +++ b/docs/install.md @@ -366,6 +366,10 @@ use of custom tasks in pipelines. most stable features to be used. Set it to "alpha" to allow alpha features to be used. +- `scope-when-expressions-to-task`: set this flag to "true" to scope `when` expressions to guard a `Task` only. Set it + to "false" to guard a `Task` and its dependent `Tasks`. It defaults to "false". For more information, see [guarding + `Task` execution using `when` expressions](pipelines.md#guard-task-execution-using-whenexpressions). + For example: ```yaml diff --git a/docs/pipelines.md b/docs/pipelines.md index 64abfa5125c..855cdd9aac2 100644 --- a/docs/pipelines.md +++ b/docs/pipelines.md @@ -474,6 +474,226 @@ There are a lot of scenarios where `when` expressions can be really useful. Some - Checking if the name of a CI job matches - Checking if an optional Workspace has been provided +#### Guarding a `Task` and its dependent `Tasks` + +When `when` expressions evaluate to `False`, the `Task` and its dependent `Tasks` will be skipped by default while the +rest of the `Pipeline` will execute. Dependencies between `Tasks` can be either ordering ([`runAfter`](https://github.com/tektoncd/pipeline/blob/main/docs/pipelines.md#using-the-runafter-parameter)) +or resource (e.g. [`Results`](https://github.com/tektoncd/pipeline/blob/main/docs/pipelines.md#using-results)) +dependencies, as further described in [configuring execution order](#configuring-the-task-execution-order). The global +default scope of `when` expressions is set to a `Task` and its dependent`Tasks`; `scope-when-expressions-to-task` field +in [`config/config-feature-flags.yaml`](install.md#customizing-the-pipelines-controller-behavior) defaults to "false". + +**Note:** Scoping `when` expressions to a `Task` and its dependent `Tasks` is deprecated + +To guard a `Task` and its dependent Tasks: +- cascade the `when` expressions to the specific dependent `Tasks` to be guarded as well +- compose the `Task` and its dependent `Tasks` as a unit to be guarded and executed together using `Pipelines` in `Pipelines` + +##### Cascade `when` expressions to the specific dependent `Tasks` + +Pick and choose which specific dependent `Tasks` to guard as well, and cascade the `when` expressions to those `Tasks`. + +Taking the use case below, a user who wants to guard `manual-approval` and its dependent `Tasks`: + +``` + tests + | + v + manual-approval + | | + v (approver) + build-image | + | v + v slack-msg + deploy-image +``` + +The user can design the `Pipeline` to solve their use case as such: + +```yaml +tasks: +... +- name: manual-approval + runAfter: + - tests + when: + - input: $(params.git-action) + operator: in + values: + - merge + taskRef: + name: manual-approval + +- name: build-image + when: + - input: $(params.git-action) + operator: in + values: + - merge + runAfter: + - manual-approval + taskRef: + name: build-image + +- name: deploy-image + when: + - input: $(params.git-action) + operator: in + values: + - merge + runAfter: + - build-image + taskRef: + name: deploy-image + +- name: slack-msg + params: + - name: approver + value: $(tasks.manual-approval.results.approver) + taskRef: + name: slack-msg +``` + +##### Compose using Pipelines in Pipelines + +Compose a set of `Tasks` as a unit of execution using `Pipelines` in `Pipelines`, which allows for guarding a `Task` and +its dependent `Tasks` (as a sub-`Pipeline`) using `when` expressions. + +**Note:** `Pipelines` in `Pipelines` is an [experimental feature](https://github.com/tektoncd/experimental/tree/main/pipelines-in-pipelines) + +Taking the use case below, a user who wants to guard `manual-approval` and its dependent `Tasks`: + +``` + tests + | + v + manual-approval + | | + v (approver) + build-image | + | v + v slack-msg + deploy-image +``` + +The user can design the `Pipelines` to solve their use case as such: + +```yaml +## sub pipeline (approve-build-deploy-slack) +tasks: + - name: manual-approval + runAfter: + - integration-tests + taskRef: + name: manual-approval + + - name: build-image + runAfter: + - manual-approval + taskRef: + name: build-image + + - name: deploy-image + runAfter: + - build-image + taskRef: + name: deploy-image + + - name: slack-msg + params: + - name: approver + value: $(tasks.manual-approval.results.approver) + taskRef: + name: slack-msg + +--- +## main pipeline +tasks: +... +- name: approve-build-deploy-slack + runAfter: + - tests + when: + - input: $(params.git-action) + operator: in + values: + - merge + taskRef: + apiVersion: tekton.dev/v1beta1 + kind: Pipeline + name: approve-build-deploy-slack +``` + +#### Guarding a `Task` only + +To guard a `Task` only and unblock execution of its dependent `Tasks`, set the global default scope of `when` expressions +to `Task` using the `scope-when-expressions-to-task` field in [`config/config-feature-flags.yaml`](install.md#customizing-the-pipelines-controller-behavior) +by changing it to "true" +- The ordering-dependent `Tasks` will be executed +- The resource-dependent `Tasks` (and their dependencies) will be skipped because of missing `Results` from the skipped + parent `Task`. When we add support for [default `Results`](https://github.com/tektoncd/community/pull/240), then the + resource-dependent `Tasks` may be executed if the default `Results` from the skipped parent `Task` are specified. In + addition, if a resource-dependent `Task` needs a file from a guarded parent `Task` in a shared `Workspace`, make sure + to handle the execution of the child `Task` in case the expected file is missing from the `Workspace` because the + guarded parent `Task` is skipped. + +``` + tests + | + v + manual-approval + | | + v (approver) + build-image | + | v + v slack-msg + deploy-image +``` + +Taking the use case above, a user who wants to guard `manual-approval` only can design the `Pipeline` as such: + +```yaml +tasks: +... +- name: manual-approval + runAfter: + - tests + when: + - input: $(params.git-action) + operator: in + values: + - merge + taskRef: + name: manual-approval + +- name: build-image + runAfter: + - manual-approval + taskRef: + name: build-image + +- name: deploy-image + runAfter: + - build-image + taskRef: + name: deploy-image + +- name: slack-msg + params: + - name: approver + value: $(tasks.manual-approval.results.approver) + taskRef: + name: slack-msg +``` + +With `when` expressions scoped to `Task`, if `manual-approval` is skipped, execution of it's dependent `Tasks` +(`slack-msg`, `build-image` and `deploy-image`) would be unblocked regardless: +- `build-image` and `deploy-image` should be executed successfully +- `slack-msg` will be skipped because it is missing the `approver` `Result` from `manual-approval` + - dependents of `slack-msg` would have been skipped too if it had any of them + - if `manual-approval` specifies a default `approver` `Result`, such as "None", then `slack-msg` would be executed + ([supporting default `Results` is in progress](https://github.com/tektoncd/community/pull/240)) + ### Guard `Task` execution using `Conditions` **Note:** `Conditions` are [deprecated](./deprecations.md), use [`when` expressions](#guard-task-execution-using-when-expressions) instead. @@ -700,10 +920,13 @@ so that one will run before another and the execution of the `Pipeline` progress without getting stuck in an infinite loop. This is done using: - -- [`from`](#using-the-from-parameter) clauses on the [`PipelineResources`](resources.md) used by each `Task` -- [`runAfter`](#using-the-runafter-parameter) clauses on the corresponding `Tasks` -- By linking the [`results`](#configuring-execution-results-at-the-pipeline-level) of one `Task` to the params of another +- _resource dependencies_: + - [`from`](#using-the-from-parameter) clauses on the [`PipelineResources`](resources.md) used by each `Task` + - [`results`](#configuring-execution-results-at-the-pipeline-level) of one `Task` being pa `params` or + `when` expressions of another + +- _ordering dependencies_: + - [`runAfter`](#using-the-runafter-parameter) clauses on the corresponding `Tasks` For example, the `Pipeline` defined as follows diff --git a/pkg/apis/config/feature_flags.go b/pkg/apis/config/feature_flags.go index 5073ea5d169..639c37148b5 100644 --- a/pkg/apis/config/feature_flags.go +++ b/pkg/apis/config/feature_flags.go @@ -37,6 +37,7 @@ const ( enableTektonOCIBundles = "enable-tekton-oci-bundles" enableCustomTasks = "enable-custom-tasks" enableAPIFields = "enable-api-fields" + scopeWhenExpressionsToTask = "scope-when-expressions-to-task" DefaultDisableHomeEnvOverwrite = true DefaultDisableWorkingDirOverwrite = true DefaultDisableAffinityAssistant = false @@ -45,6 +46,7 @@ const ( DefaultRequireGitSSHSecretKnownHosts = false DefaultEnableTektonOciBundles = false DefaultEnableCustomTasks = false + DefaultScopeWhenExpressionsToTask = false DefaultEnableAPIFields = StableAPIFields ) @@ -59,6 +61,7 @@ type FeatureFlags struct { RequireGitSSHSecretKnownHosts bool EnableTektonOCIBundles bool EnableCustomTasks bool + ScopeWhenExpressionsToTask bool EnableAPIFields string } @@ -105,6 +108,9 @@ func NewFeatureFlagsFromMap(cfgMap map[string]string) (*FeatureFlags, error) { if err := setFeature(requireGitSSHSecretKnownHostsKey, DefaultRequireGitSSHSecretKnownHosts, &tc.RequireGitSSHSecretKnownHosts); err != nil { return nil, err } + if err := setFeature(scopeWhenExpressionsToTask, DefaultScopeWhenExpressionsToTask, &tc.ScopeWhenExpressionsToTask); err != nil { + return nil, err + } if err := setEnabledAPIFields(cfgMap, DefaultEnableAPIFields, &tc.EnableAPIFields); err != nil { return nil, err } diff --git a/pkg/apis/config/feature_flags_test.go b/pkg/apis/config/feature_flags_test.go index 752069c6436..6fb7909e6b4 100644 --- a/pkg/apis/config/feature_flags_test.go +++ b/pkg/apis/config/feature_flags_test.go @@ -38,6 +38,7 @@ func TestNewFeatureFlagsFromConfigMap(t *testing.T) { DisableHomeEnvOverwrite: false, DisableWorkingDirOverwrite: false, RunningInEnvWithInjectedSidecars: config.DefaultRunningInEnvWithInjectedSidecars, + ScopeWhenExpressionsToTask: config.DefaultScopeWhenExpressionsToTask, EnableAPIFields: "stable", }, fileName: config.GetFeatureFlagsConfigName(), @@ -51,6 +52,7 @@ func TestNewFeatureFlagsFromConfigMap(t *testing.T) { RequireGitSSHSecretKnownHosts: true, EnableTektonOCIBundles: true, EnableCustomTasks: true, + ScopeWhenExpressionsToTask: true, EnableAPIFields: "alpha", }, fileName: "feature-flags-all-flags-set", @@ -66,6 +68,7 @@ func TestNewFeatureFlagsFromConfigMap(t *testing.T) { DisableHomeEnvOverwrite: true, DisableWorkingDirOverwrite: true, RunningInEnvWithInjectedSidecars: config.DefaultRunningInEnvWithInjectedSidecars, + ScopeWhenExpressionsToTask: config.DefaultScopeWhenExpressionsToTask, }, fileName: "feature-flags-enable-api-fields-overrides-bundles-and-custom-tasks", }, @@ -78,6 +81,7 @@ func TestNewFeatureFlagsFromConfigMap(t *testing.T) { DisableHomeEnvOverwrite: true, DisableWorkingDirOverwrite: true, RunningInEnvWithInjectedSidecars: config.DefaultRunningInEnvWithInjectedSidecars, + ScopeWhenExpressionsToTask: config.DefaultScopeWhenExpressionsToTask, }, fileName: "feature-flags-bundles-and-custom-tasks", }, @@ -98,6 +102,7 @@ func TestNewFeatureFlagsFromEmptyConfigMap(t *testing.T) { DisableHomeEnvOverwrite: true, DisableWorkingDirOverwrite: true, RunningInEnvWithInjectedSidecars: true, + ScopeWhenExpressionsToTask: config.DefaultScopeWhenExpressionsToTask, EnableAPIFields: "stable", } verifyConfigFileWithExpectedFeatureFlagsConfig(t, FeatureFlagsConfigEmptyName, expectedConfig) @@ -141,6 +146,8 @@ func TestNewFeatureFlagsConfigMapErrors(t *testing.T) { fileName: "feature-flags-invalid-boolean", }, { fileName: "feature-flags-invalid-enable-api-fields", + }, { + fileName: "feature-flags-invalid-scope-when-expressions-to-task", }} { t.Run(tc.fileName, func(t *testing.T) { cm := test.ConfigMapFromTestFile(t, tc.fileName) diff --git a/pkg/apis/config/testdata/feature-flags-all-flags-set.yaml b/pkg/apis/config/testdata/feature-flags-all-flags-set.yaml index e56766124ce..6e5e4958e51 100644 --- a/pkg/apis/config/testdata/feature-flags-all-flags-set.yaml +++ b/pkg/apis/config/testdata/feature-flags-all-flags-set.yaml @@ -25,4 +25,5 @@ data: require-git-ssh-secret-known-hosts: "true" enable-tekton-oci-bundles: "true" enable-custom-tasks: "true" + scope-when-expressions-to-task: "true" enable-api-fields: "alpha" diff --git a/pkg/apis/config/testdata/feature-flags-invalid-scope-when-expressions-to-task.yaml b/pkg/apis/config/testdata/feature-flags-invalid-scope-when-expressions-to-task.yaml new file mode 100644 index 00000000000..54569ae7cba --- /dev/null +++ b/pkg/apis/config/testdata/feature-flags-invalid-scope-when-expressions-to-task.yaml @@ -0,0 +1,21 @@ +# Copyright 2021 The Tekton Authors +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# https://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +apiVersion: v1 +kind: ConfigMap +metadata: + name: feature-flags + namespace: tekton-pipelines +data: + scope-when-expressions-to-task: "im-not-a-boolean" diff --git a/pkg/reconciler/pipelinerun/pipelinerun.go b/pkg/reconciler/pipelinerun/pipelinerun.go index 4992af282a2..a9370bf7e68 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun.go +++ b/pkg/reconciler/pipelinerun/pipelinerun.go @@ -482,10 +482,11 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun, get // Build PipelineRunFacts with a list of resolved pipeline tasks, // dag tasks graph and final tasks graph pipelineRunFacts := &resources.PipelineRunFacts{ - State: pipelineRunState, - SpecStatus: pr.Spec.Status, - TasksGraph: d, - FinalTasksGraph: dfinally, + State: pipelineRunState, + SpecStatus: pr.Spec.Status, + TasksGraph: d, + FinalTasksGraph: dfinally, + ScopeWhenExpressionsToTask: config.FromContextOrDefaults(ctx).FeatureFlags.ScopeWhenExpressionsToTask, } for _, rprt := range pipelineRunFacts.State { @@ -666,7 +667,7 @@ func (c *Reconciler) runNextSchedulableTask(ctx context.Context, pr *v1beta1.Pip } for _, rprt := range nextRprts { - if rprt == nil || rprt.Skip(pipelineRunFacts) || rprt.IsFinallySkipped(pipelineRunFacts) { + if rprt == nil || rprt.Skip(pipelineRunFacts).IsSkipped || rprt.IsFinallySkipped(pipelineRunFacts).IsSkipped { continue } if rprt.ResolvedConditionChecks == nil || rprt.ResolvedConditionChecks.IsSuccess() { diff --git a/pkg/reconciler/pipelinerun/pipelinerun_test.go b/pkg/reconciler/pipelinerun/pipelinerun_test.go index 80c45e73181..ddaf3589002 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun_test.go +++ b/pkg/reconciler/pipelinerun/pipelinerun_test.go @@ -3797,6 +3797,168 @@ func TestReconcileWithWhenExpressionsWithTaskResults(t *testing.T) { } } +func TestReconcileWithWhenExpressionsScopedToTask(t *testing.T) { + names.TestingSeed() + ps := []*v1beta1.Pipeline{tb.Pipeline("test-pipeline", tb.PipelineNamespace("foo"), tb.PipelineSpec( + // a-task is skipped because its when expressions evaluate to false + tb.PipelineTask("a-task", "a-task", + tb.PipelineTaskWhenExpression("foo", selection.In, []string{"bar"}), + ), + // b-task is executed regardless of running after skipped a-task because when expressions are scoped to task + tb.PipelineTask("b-task", "b-task", + tb.RunAfter("a-task"), + ), + // c-task is skipped because its when expressions evaluate to false (not because it's parent a-task is skipped) + tb.PipelineTask("c-task", "c-task", + tb.PipelineTaskWhenExpression("foo", selection.In, []string{"bar"}), + ), + // d-task is executed regardless of running after skipped parent c-task (and skipped grandparent a-task) + // because when expressions are scoped to task + tb.PipelineTask("d-task", "d-task", + tb.RunAfter("c-task"), + ), + // e-task is attempted regardless of running after skipped a-task because when expressions are scoped to task + // but then get skipped because of missing result references from a-task + tb.PipelineTask("e-task", "e-task", + tb.PipelineTaskWhenExpression("$(tasks.a-task.results.aResult)", selection.In, []string{"aResultValue"}), + ), + ))} + prs := []*v1beta1.PipelineRun{tb.PipelineRun("test-pipeline-run-different-service-accs", tb.PipelineRunNamespace("foo"), + tb.PipelineRunSpec("test-pipeline", + tb.PipelineRunServiceAccountName("test-sa-0"), + ), + )} + // initialize the pipelinerun with the skipped a-task + prs[0].Status.SkippedTasks = append(prs[0].Status.SkippedTasks, v1beta1.SkippedTask{ + Name: "a-task", + WhenExpressions: v1beta1.WhenExpressions{{ + Input: "foo", + Operator: selection.In, + Values: []string{"bar"}, + }}, + }) + // initialize a-task, b-task and c-task + ts := []*v1beta1.Task{ + tb.Task("a-task", tb.TaskNamespace("foo"), + tb.TaskSpec(tb.TaskResults("aResult", "a result")), + ), + tb.Task("b-task", tb.TaskNamespace("foo")), + tb.Task("c-task", tb.TaskNamespace("foo")), + tb.Task("d-task", tb.TaskNamespace("foo")), + tb.Task("e-task", tb.TaskNamespace("foo")), + } + + // set the scope of when expressions to task -- execution of dependent tasks is unblocked + cms := []*corev1.ConfigMap{ + { + ObjectMeta: metav1.ObjectMeta{Name: config.GetFeatureFlagsConfigName(), Namespace: system.Namespace()}, + Data: map[string]string{ + "scope-when-expressions-to-task": "true", + }, + }, + } + + var trs []*v1beta1.TaskRun + d := test.Data{ + PipelineRuns: prs, + Pipelines: ps, + Tasks: ts, + TaskRuns: trs, + ConfigMaps: cms, + } + prt := newPipelineRunTest(d, t) + defer prt.Cancel() + + wantEvents := []string{ + "Normal Started", + "Normal Running Tasks Completed: 0 \\(Failed: 0, Cancelled 0\\), Incomplete: 2, Skipped: 3", + } + pipelineRun, clients := prt.reconcileRun("foo", "test-pipeline-run-different-service-accs", wantEvents, false) + + taskRunExists := func(taskName string, taskRunName string) { + expectedTaskRun := tb.TaskRun(taskRunName, + tb.TaskRunNamespace("foo"), + tb.TaskRunOwnerReference("PipelineRun", "test-pipeline-run-different-service-accs", + tb.OwnerReferenceAPIVersion("tekton.dev/v1beta1"), + tb.Controller, tb.BlockOwnerDeletion, + ), + tb.TaskRunLabel("tekton.dev/memberOf", "tasks"), + tb.TaskRunLabel("tekton.dev/pipeline", "test-pipeline"), + tb.TaskRunLabel("tekton.dev/pipelineRun", "test-pipeline-run-different-service-accs"), + tb.TaskRunLabel("tekton.dev/pipelineTask", taskName), + tb.TaskRunSpec( + tb.TaskRunTaskRef(taskName), + tb.TaskRunServiceAccountName("test-sa-0"), + ), + ) + + actual, err := clients.Pipeline.TektonV1beta1().TaskRuns("foo").List(prt.TestAssets.Ctx, metav1.ListOptions{ + LabelSelector: fmt.Sprintf("tekton.dev/pipelineTask=%s,tekton.dev/pipelineRun=test-pipeline-run-different-service-accs", taskName), + Limit: 1, + }) + + if err != nil { + t.Fatalf("Failure to list TaskRuns %s", err) + } + if len(actual.Items) != 1 { + t.Fatalf("Expected 1 TaskRun got %d", len(actual.Items)) + } + actualTaskRun := actual.Items[0] + if d := cmp.Diff(&actualTaskRun, expectedTaskRun, ignoreResourceVersion); d != "" { + t.Errorf("expected to see TaskRun %v created. Diff %s", taskRunName, diff.PrintWantGot(d)) + } + } + + taskRunExists("b-task", "test-pipeline-run-different-service-accs-b-task-mz4c7") + taskRunExists("d-task", "test-pipeline-run-different-service-accs-d-task-78c5n") + + actualSkippedTasks := pipelineRun.Status.SkippedTasks + expectedSkippedTasks := []v1beta1.SkippedTask{{ + // its when expressions evaluate to false + Name: "a-task", + WhenExpressions: v1beta1.WhenExpressions{{ + Input: "foo", + Operator: "in", + Values: []string{"bar"}, + }}, + }, { + // its when expressions evaluate to false + Name: "c-task", + WhenExpressions: v1beta1.WhenExpressions{{ + Input: "foo", + Operator: "in", + Values: []string{"bar"}, + }}, + }, { + // was attempted, but has missing results references + Name: "e-task", + WhenExpressions: v1beta1.WhenExpressions{{ + Input: "$(tasks.a-task.results.aResult)", + Operator: "in", + Values: []string{"aResultValue"}, + }}, + }} + if d := cmp.Diff(actualSkippedTasks, expectedSkippedTasks); d != "" { + t.Errorf("expected to find Skipped Tasks %v. Diff %s", expectedSkippedTasks, diff.PrintWantGot(d)) + } + + // confirm that there are no taskruns created for the skipped tasks a-task, c-task, e-task + skippedTasks := []string{"a-task", "c-task", "e-task"} + for _, skippedTask := range skippedTasks { + labelSelector := fmt.Sprintf("tekton.dev/pipelineTask=%s,tekton.dev/pipelineRun=test-pipeline-run-different-service-accs", skippedTask) + actualSkippedTask, err := clients.Pipeline.TektonV1beta1().TaskRuns("foo").List(prt.TestAssets.Ctx, metav1.ListOptions{ + LabelSelector: labelSelector, + Limit: 1, + }) + if err != nil { + t.Fatalf("Failure to list TaskRun's %s", err) + } + if len(actualSkippedTask.Items) != 0 { + t.Fatalf("Expected 0 TaskRuns got %d", len(actualSkippedTask.Items)) + } + } +} + // TestReconcileWithAffinityAssistantStatefulSet tests that given a pipelineRun with workspaces, // an Affinity Assistant StatefulSet is created for each PVC workspace and // that the Affinity Assistant names is propagated to TaskRuns. diff --git a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go index 9210086080e..ba7df7339fb 100644 --- a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go +++ b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go @@ -39,6 +39,24 @@ const ( ReasonConditionCheckFailed = "ConditionCheckFailed" ) +type SkippingReason string + +const ( + WhenExpressionsSkip SkippingReason = "WhenExpressionsSkip" + ConditionsSkip SkippingReason = "ConditionsSkip" + ParentTasksSkip SkippingReason = "ParentTasksSkip" + IsStoppingSkip SkippingReason = "IsStoppingSkip" + IsGracefullyCancelledSkip SkippingReason = "IsGracefullyCancelledSkip" + IsGracefullyStoppedSkip SkippingReason = "IsGracefullyStoppedSkip" + MissingResultsSkip SkippingReason = "MissingResultsSkip" + None SkippingReason = "None" +) + +type TaskSkipStatus struct { + IsSkipped bool + SkippingReason SkippingReason +} + // TaskNotFoundError indicates that the resolution failed because a referenced Task couldn't be retrieved type TaskNotFoundError struct { Name string @@ -76,7 +94,7 @@ type ResolvedPipelineRunTask struct { // IsDone returns true only if the task is skipped, succeeded or failed func (t ResolvedPipelineRunTask) IsDone(facts *PipelineRunFacts) bool { - return t.Skip(facts) || t.IsSuccessful() || t.IsFailure() + return t.Skip(facts).IsSkipped || t.IsSuccessful() || t.IsFailure() } // IsRunning returns true only if the task is neither succeeded, cancelled nor failed @@ -172,17 +190,34 @@ func (t *ResolvedPipelineRunTask) checkParentsDone(facts *PipelineRunFacts) bool return true } -func (t *ResolvedPipelineRunTask) skip(facts *PipelineRunFacts) bool { - if facts.isFinalTask(t.PipelineTask.Name) || t.IsStarted() { - return false - } +func (t *ResolvedPipelineRunTask) skip(facts *PipelineRunFacts) TaskSkipStatus { + var skippingReason SkippingReason - if t.conditionsSkip() || t.whenExpressionsSkip(facts) || t.parentTasksSkip(facts) || - facts.IsStopping() || facts.IsGracefullyCancelled() || facts.IsGracefullyStopped() { - return true + switch { + case facts.isFinalTask(t.PipelineTask.Name) || t.IsStarted(): + skippingReason = None + case facts.IsStopping(): + skippingReason = IsStoppingSkip + case facts.IsGracefullyCancelled(): + skippingReason = IsGracefullyCancelledSkip + case facts.IsGracefullyStopped(): + skippingReason = IsGracefullyStoppedSkip + case t.skipBecauseParentTaskWasSkipped(facts): + skippingReason = ParentTasksSkip + case t.skipBecauseWhenExpressionsEvaluatedToFalse(facts): + skippingReason = WhenExpressionsSkip + case t.skipBecauseResultReferencesAreMissing(facts): + skippingReason = MissingResultsSkip + case t.skipBecauseConditionsFailed(): + skippingReason = ConditionsSkip + default: + skippingReason = None } - return false + return TaskSkipStatus{ + IsSkipped: skippingReason != None, + SkippingReason: skippingReason, + } } // Skip returns true if a PipelineTask will not be run because @@ -190,10 +225,11 @@ func (t *ResolvedPipelineRunTask) skip(facts *PipelineRunFacts) bool { // (2) its Condition Checks failed // (3) its parent task was skipped // (4) Pipeline is in stopping state (one of the PipelineTasks failed) +// (5) Pipeline is gracefully cancelled or stopped // Note that this means Skip returns false if a conditionCheck is in progress -func (t *ResolvedPipelineRunTask) Skip(facts *PipelineRunFacts) bool { +func (t *ResolvedPipelineRunTask) Skip(facts *PipelineRunFacts) TaskSkipStatus { if facts.SkipCache == nil { - facts.SkipCache = make(map[string]bool) + facts.SkipCache = make(map[string]TaskSkipStatus) } if _, cached := facts.SkipCache[t.PipelineTask.Name]; !cached { facts.SkipCache[t.PipelineTask.Name] = t.skip(facts) // t.skip() is same as our existing t.Skip() @@ -201,7 +237,9 @@ func (t *ResolvedPipelineRunTask) Skip(facts *PipelineRunFacts) bool { return facts.SkipCache[t.PipelineTask.Name] } -func (t *ResolvedPipelineRunTask) conditionsSkip() bool { +// skipBecauseConditionsFailed checks that the task has Conditions which have completed evaluating +// it returns true if any of the Conditions fails +func (t *ResolvedPipelineRunTask) skipBecauseConditionsFailed() bool { if len(t.ResolvedConditionChecks) > 0 { if t.ResolvedConditionChecks.IsDone() && !t.ResolvedConditionChecks.IsSuccess() { return true @@ -210,7 +248,9 @@ func (t *ResolvedPipelineRunTask) conditionsSkip() bool { return false } -func (t *ResolvedPipelineRunTask) whenExpressionsSkip(facts *PipelineRunFacts) bool { +// skipBecauseWhenExpressionsEvaluatedToFalse confirms that the when expressions have completed evaluating, and +// it returns true if any of the when expressions evaluate to false +func (t *ResolvedPipelineRunTask) skipBecauseWhenExpressionsEvaluatedToFalse(facts *PipelineRunFacts) bool { if t.checkParentsDone(facts) { if len(t.PipelineTask.WhenExpressions) > 0 { if !t.PipelineTask.WhenExpressions.HaveVariables() { @@ -223,11 +263,33 @@ func (t *ResolvedPipelineRunTask) whenExpressionsSkip(facts *PipelineRunFacts) b return false } -func (t *ResolvedPipelineRunTask) parentTasksSkip(facts *PipelineRunFacts) bool { +// skipBecauseParentTaskWasSkipped loops through the parent tasks and checks if the parent task skipped: +// if yes, is it because of when expressions and are when expressions? +// if yes, it ignores this parent skip and continue evaluating other parent tasks +// if no, it returns true to skip the current task because this parent task was skipped +// if no, it continues checking the other parent tasks +func (t *ResolvedPipelineRunTask) skipBecauseParentTaskWasSkipped(facts *PipelineRunFacts) bool { stateMap := facts.State.ToMap() node := facts.TasksGraph.Nodes[t.PipelineTask.Name] for _, p := range node.Prev { - if stateMap[p.Task.HashKey()].Skip(facts) { + parentTask := stateMap[p.Task.HashKey()] + if parentSkipStatus := parentTask.Skip(facts); parentSkipStatus.IsSkipped { + // if the `when` expressions are scoped to task and the parent task was skipped due to its `when` expressions, + // then we should ignore that and continue evaluating if we should skip because of other parent tasks + if parentSkipStatus.SkippingReason == WhenExpressionsSkip && facts.ScopeWhenExpressionsToTask { + continue + } + return true + } + } + return false +} + +// skipBecauseResultReferencesAreMissing checks if the task references results that cannot be resolved, which is a +// reason for skipping the task +func (t *ResolvedPipelineRunTask) skipBecauseResultReferencesAreMissing(facts *PipelineRunFacts) bool { + if t.checkParentsDone(facts) { + if _, err := ResolveResultRef(facts.State, t); err != nil { return true } } @@ -240,19 +302,30 @@ func (t *ResolvedPipelineRunTask) IsFinalTask(facts *PipelineRunFacts) bool { } // IsFinallySkipped returns true if a finally task is not executed and skipped due to task result validation failure -func (t *ResolvedPipelineRunTask) IsFinallySkipped(facts *PipelineRunFacts) bool { - if t.IsStarted() { - return false - } - if facts.checkDAGTasksDone() && facts.isFinalTask(t.PipelineTask.Name) { - if _, err := ResolveResultRef(facts.State, t); err != nil { - return true - } - if t.whenExpressionsSkip(facts) { - return true +func (t *ResolvedPipelineRunTask) IsFinallySkipped(facts *PipelineRunFacts) TaskSkipStatus { + var skippingReason SkippingReason + + switch { + case t.IsStarted(): + skippingReason = None + case facts.checkDAGTasksDone() && facts.isFinalTask(t.PipelineTask.Name): + switch { + case t.skipBecauseResultReferencesAreMissing(facts): + skippingReason = MissingResultsSkip + case t.skipBecauseWhenExpressionsEvaluatedToFalse(facts): + skippingReason = WhenExpressionsSkip + default: + skippingReason = None } + default: + skippingReason = None } - return false + + return TaskSkipStatus{ + IsSkipped: skippingReason != None, + SkippingReason: skippingReason, + } + } // GetRun is a function that will retrieve a Run by name. diff --git a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go index 7433e45454f..37570498e9b 100644 --- a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go +++ b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go @@ -673,9 +673,10 @@ func dagFromState(state PipelineRunState) (*dag.Graph, error) { func TestIsSkipped(t *testing.T) { for _, tc := range []struct { - name string - state PipelineRunState - expected map[string]bool + name string + state PipelineRunState + scopeWhenExpressionsToTask bool + expected map[string]bool }{{ name: "tasks-condition-passed", state: PipelineRunState{{ @@ -983,6 +984,605 @@ func TestIsSkipped(t *testing.T) { expected: map[string]bool{ "mytask13": false, }, + }, { + name: "tasks-with-when-expression-scoped-to-branch", + state: PipelineRunState{{ + // skipped because when expressions evaluate to false + PipelineTask: &pts[10], + TaskRunName: "pipelinerun-guardedtask", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }, { + // skipped because parent was skipped and when expressions are scoped to branch + PipelineTask: &v1beta1.PipelineTask{ + Name: "mytask18", + TaskRef: &v1beta1.TaskRef{Name: "task"}, + RunAfter: []string{"mytask11"}, + }, + TaskRunName: "pipelinerun-ordering-dependent-task-1", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }}, + scopeWhenExpressionsToTask: false, + expected: map[string]bool{ + "mytask11": true, + "mytask18": true, + }, + }, { + name: "tasks-when-expressions-scoped-to-task", + state: PipelineRunState{{ + // skipped because when expressions evaluate to false + PipelineTask: &pts[10], + TaskRunName: "pipelinerun-guardedtask", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }, { + // not skipped regardless of its parent task being skipped because when expressions are scoped to task + PipelineTask: &v1beta1.PipelineTask{ + Name: "mytask18", + TaskRef: &v1beta1.TaskRef{Name: "task"}, + RunAfter: []string{"mytask11"}, + }, + TaskRunName: "pipelinerun-ordering-dependent-task-1", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }}, + scopeWhenExpressionsToTask: true, + expected: map[string]bool{ + "mytask11": true, + "mytask18": false, + }, + }, { + name: "tasks-when-expressions-scoped-to-branch-skip-multiple-dependent-tasks", + state: PipelineRunState{{ + // skipped because when expressions evaluate to false + PipelineTask: &pts[10], + TaskRunName: "pipelinerun-guardedtask", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }, { + // skipped because parent was skipped and when expressions are scoped to branch + PipelineTask: &v1beta1.PipelineTask{ + Name: "mytask18", + TaskRef: &v1beta1.TaskRef{Name: "task"}, + RunAfter: []string{"mytask11"}, + }, + TaskRunName: "pipelinerun-ordering-dependent-task-1", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }, { + // skipped because parent was skipped and when expressions are scoped to branch + PipelineTask: &v1beta1.PipelineTask{ + Name: "mytask19", + TaskRef: &v1beta1.TaskRef{Name: "task"}, + RunAfter: []string{"mytask18"}, + }, + TaskRunName: "pipelinerun-ordering-dependent-task-2", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }}, + scopeWhenExpressionsToTask: false, + expected: map[string]bool{ + "mytask11": true, + "mytask18": true, + "mytask19": true, + }, + }, { + name: "tasks-when-expressions-scoped-to-task-run-multiple-dependent-tasks", + state: PipelineRunState{{ + // skipped because when expressions evaluate to false + PipelineTask: &pts[10], + TaskRunName: "pipelinerun-guardedtask", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }, { + // not skipped regardless of its parent task being skipped because when expressions are scoped to task + PipelineTask: &v1beta1.PipelineTask{ + Name: "mytask18", + TaskRef: &v1beta1.TaskRef{Name: "task"}, + RunAfter: []string{"mytask11"}, + }, + TaskRunName: "pipelinerun-ordering-dependent-task-1", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }, { + // not skipped regardless of its grandparent task being skipped because when expressions are scoped to task + PipelineTask: &v1beta1.PipelineTask{ + Name: "mytask19", + TaskRef: &v1beta1.TaskRef{Name: "task"}, + RunAfter: []string{"mytask18"}, + }, + TaskRunName: "pipelinerun-ordering-dependent-task-2", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }}, + scopeWhenExpressionsToTask: true, + expected: map[string]bool{ + "mytask11": true, + "mytask18": false, + "mytask19": false, + }, + }, { + name: "tasks-when-expressions-scoped-to-task-run-multiple-ordering-and-resource-dependent-tasks", + state: PipelineRunState{{ + // skipped because when expressions evaluate to false + PipelineTask: &pts[10], + TaskRunName: "pipelinerun-guardedtask", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }, { + // not skipped regardless of its parent task mytask11 being skipped because when expressions are scoped to task + PipelineTask: &v1beta1.PipelineTask{ + Name: "mytask18", + TaskRef: &v1beta1.TaskRef{Name: "task"}, + RunAfter: []string{"mytask11"}, + }, + TaskRunName: "pipelinerun-ordering-dependent-task-1", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }, { + // not skipped regardless of its grandparent task mytask11 being skipped because when expressions are scoped to task + PipelineTask: &v1beta1.PipelineTask{ + Name: "mytask19", + TaskRef: &v1beta1.TaskRef{Name: "task"}, + RunAfter: []string{"mytask18"}, + }, + TaskRunName: "pipelinerun-ordering-dependent-task-2", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }, { + // attempted but skipped because of missing result from parent task mytask11 which was skipped in params + PipelineTask: &v1beta1.PipelineTask{ + Name: "mytask20", + TaskRef: &v1beta1.TaskRef{Name: "task"}, + Params: []v1beta1.Param{{ + Name: "commit", + Value: *v1beta1.NewArrayOrString("$(tasks.mytask11.results.missingResult)"), + }}, + }, + TaskRunName: "pipelinerun-resource-dependent-task-1", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }, { + // skipped because of parent task mytask20 was skipping because of missing result from grandparent task + // mytask11 which was skipped + PipelineTask: &v1beta1.PipelineTask{ + Name: "mytask21", + TaskRef: &v1beta1.TaskRef{Name: "task"}, + RunAfter: []string{"mytask20"}, + }, + TaskRunName: "pipelinerun-ordering-dependent-task-3", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }, { + // attempted but skipped because of missing result from parent task mytask11 which was skipped in when expressions + PipelineTask: &v1beta1.PipelineTask{ + Name: "mytask22", + TaskRef: &v1beta1.TaskRef{Name: "task"}, + WhenExpressions: v1beta1.WhenExpressions{{ + Input: "$(tasks.mytask11.results.missingResult)", + Operator: selection.In, + Values: []string{"expectedResult"}, + }}, + }, + TaskRunName: "pipelinerun-resource-dependent-task-2", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }, { + // skipped because of parent task mytask22 was skipping because of missing result from grandparent task + // mytask11 which was skipped + PipelineTask: &v1beta1.PipelineTask{ + Name: "mytask23", + TaskRef: &v1beta1.TaskRef{Name: "task"}, + RunAfter: []string{"mytask22"}, + }, + TaskRunName: "pipelinerun-ordering-dependent-task-4", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }}, + scopeWhenExpressionsToTask: true, + expected: map[string]bool{ + "mytask11": true, + "mytask18": false, + "mytask19": false, + "mytask20": true, + "mytask21": true, + "mytask22": true, + "mytask23": true, + }, + }, { + name: "tasks-parent-condition-failed-parent-when-expressions-passed-scoped-to-task", + state: PipelineRunState{{ + // skipped because conditions fail + PipelineTask: &pts[5], + TaskRunName: "pipelinerun-conditionaltask", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + ResolvedConditionChecks: failedTaskConditionCheckState, + }, { + // skipped because when expressions evaluate to false + PipelineTask: &pts[10], + TaskRunName: "pipelinerun-guardedtask", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }, { + // skipped because of parent task guarded using conditions is skipped, regardless of another parent task + // being guarded with when expressions that are scoped to task + PipelineTask: &v1beta1.PipelineTask{ + Name: "mytask18", + TaskRef: &v1beta1.TaskRef{Name: "task"}, + RunAfter: []string{"mytask6", "mytask11"}, + }, + TaskRunName: "pipelinerun-ordering-dependent-task-1", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }, { + // not skipped regardless of its parent task being skipped because when expressions are scoped to task + PipelineTask: &v1beta1.PipelineTask{ + Name: "mytask19", + TaskRef: &v1beta1.TaskRef{Name: "task"}, + RunAfter: []string{"mytask11"}, + }, + TaskRunName: "pipelinerun-ordering-dependent-task-2", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }}, + scopeWhenExpressionsToTask: true, + expected: map[string]bool{ + "mytask6": true, + "mytask11": true, + "mytask18": true, + "mytask19": false, + }, + }} { + t.Run(tc.name, func(t *testing.T) { + d, err := dagFromState(tc.state) + if err != nil { + t.Fatalf("Could not get a dag from the TC state %#v: %v", tc.state, err) + } + stateMap := tc.state.ToMap() + facts := PipelineRunFacts{ + State: tc.state, + TasksGraph: d, + FinalTasksGraph: &dag.Graph{}, + ScopeWhenExpressionsToTask: tc.scopeWhenExpressionsToTask, + } + for taskName, isSkipped := range tc.expected { + rprt := stateMap[taskName] + if rprt == nil { + t.Fatalf("Could not get task %s from the state: %v", taskName, tc.state) + } + if d := cmp.Diff(isSkipped, rprt.Skip(&facts).IsSkipped); d != "" { + t.Errorf("Didn't get expected isSkipped from task %s: %s", taskName, diff.PrintWantGot(d)) + } + } + }) + } +} + +func TestSkipBecauseParentTaskWasSkipped(t *testing.T) { + for _, tc := range []struct { + name string + state PipelineRunState + scopeWhenExpressionsToTask bool + expected map[string]bool + }{{ + name: "tasks-parent-condition-passed", + state: PipelineRunState{{ + // parent task that has a condition that passed + PipelineTask: &pts[5], + TaskRunName: "pipelinerun-conditionaltask", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + ResolvedConditionChecks: successTaskConditionCheckState, + }, { + // child task that's not skipped (conditional parent task was not skipped) + PipelineTask: &pts[6], + }}, + expected: map[string]bool{ + "mytask7": false, + }, + }, { + name: "tasks-parent-condition-failed", + state: PipelineRunState{{ + // parent task that has a condition that failed and is skipped + PipelineTask: &pts[5], + TaskRunName: "pipelinerun-conditionaltask", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + ResolvedConditionChecks: failedTaskConditionCheckState, + }, { + // child task skipped because its parent has a condition that failed (and was skipped) + PipelineTask: &pts[6], + }}, + expected: map[string]bool{ + "mytask7": true, + }, + }, { + name: "tasks-parent-condition-running", + state: PipelineRunState{{ + // parent task has a condition that's still running + PipelineTask: &pts[5], + TaskRunName: "pipelinerun-conditionaltask", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + ResolvedConditionChecks: TaskConditionCheckState{{ + ConditionCheckName: "myconditionCheck", + Condition: &condition, + ConditionCheck: v1beta1.NewConditionCheck(makeStarted(conditionChecks[0])), + }}, + }, { + // child task not skipped because parent is not yet done + PipelineTask: &pts[6], + }}, + expected: map[string]bool{ + "mytask7": false, + }, + }, { + name: "when-expression-task-but-without-parent-done", + state: PipelineRunState{{ + // parent task has when expressions but is not yet done + PipelineTask: &pts[0], + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }, { + // child task not skipped because parent is not yet done + PipelineTask: &pts[11], + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }}, + expected: map[string]bool{ + "mytask12": false, + }, + }, { + name: "tasks-with-when-expression-scoped-to-branch", + state: PipelineRunState{{ + // parent task is skipped because when expressions evaluate to false + PipelineTask: &pts[10], + TaskRunName: "pipelinerun-guardedtask", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }, { + // child task is skipped because parent was skipped due to its when expressions evaluating to false when + // they are scoped to task and its dependent tasks + PipelineTask: &v1beta1.PipelineTask{ + Name: "mytask18", + TaskRef: &v1beta1.TaskRef{Name: "task"}, + RunAfter: []string{"mytask11"}, + }, + TaskRunName: "pipelinerun-ordering-dependent-task-1", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }}, + scopeWhenExpressionsToTask: false, + expected: map[string]bool{ + "mytask11": false, + "mytask18": true, + }, + }, { + name: "tasks-when-expressions-scoped-to-task", + state: PipelineRunState{{ + // parent task is skipped because when expressions evaluate to false, not because of its parent tasks + PipelineTask: &pts[10], + TaskRunName: "pipelinerun-guardedtask", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }, { + // child task is not skipped regardless of its parent task being skipped due to when expressions evaluating + // to false, because when expressions are scoped to task only + PipelineTask: &v1beta1.PipelineTask{ + Name: "mytask18", + TaskRef: &v1beta1.TaskRef{Name: "task"}, + RunAfter: []string{"mytask11"}, + }, + TaskRunName: "pipelinerun-ordering-dependent-task-1", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }}, + scopeWhenExpressionsToTask: true, + expected: map[string]bool{ + "mytask11": false, + "mytask18": false, + }, + }, { + name: "tasks-when-expressions-scoped-to-branch-skip-multiple-dependent-tasks", + state: PipelineRunState{{ + // parent task is skipped because when expressions evaluate to false, not because of its parent tasks + PipelineTask: &pts[10], + TaskRunName: "pipelinerun-guardedtask", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }, { + // child task is skipped because parent was skipped due to its when expressions evaluating to false when + // they are scoped to task and its dependent tasks + PipelineTask: &v1beta1.PipelineTask{ + Name: "mytask18", + TaskRef: &v1beta1.TaskRef{Name: "task"}, + RunAfter: []string{"mytask11"}, + }, + TaskRunName: "pipelinerun-ordering-dependent-task-1", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }, { + // child task is skipped because parent was skipped due to its when expressions evaluating to false when + // they are scoped to task and its dependent tasks + PipelineTask: &v1beta1.PipelineTask{ + Name: "mytask19", + TaskRef: &v1beta1.TaskRef{Name: "task"}, + RunAfter: []string{"mytask18"}, + }, + TaskRunName: "pipelinerun-ordering-dependent-task-2", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }}, + scopeWhenExpressionsToTask: false, + expected: map[string]bool{ + "mytask11": false, + "mytask18": true, + "mytask19": true, + }, + }, { + name: "tasks-when-expressions-scoped-to-task-run-multiple-dependent-tasks", + state: PipelineRunState{{ + // parent task is skipped because when expressions evaluate to false, not because of its parent tasks + PipelineTask: &pts[10], + TaskRunName: "pipelinerun-guardedtask", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }, { + // child task is not skipped regardless of its parent task being skipped due to when expressions evaluating + // to false, because when expressions are scoped to task only + PipelineTask: &v1beta1.PipelineTask{ + Name: "mytask18", + TaskRef: &v1beta1.TaskRef{Name: "task"}, + RunAfter: []string{"mytask11"}, + }, + TaskRunName: "pipelinerun-ordering-dependent-task-1", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }, { + // child task is not skipped regardless of its parent task being skipped due to when expressions evaluating + // to false, because when expressions are scoped to task only + PipelineTask: &v1beta1.PipelineTask{ + Name: "mytask19", + TaskRef: &v1beta1.TaskRef{Name: "task"}, + RunAfter: []string{"mytask18"}, + }, + TaskRunName: "pipelinerun-ordering-dependent-task-2", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }}, + scopeWhenExpressionsToTask: true, + expected: map[string]bool{ + "mytask11": false, + "mytask18": false, + "mytask19": false, + }, + }, { + name: "tasks-parent-condition-failed-parent-when-expressions-passed-scoped-to-task", + state: PipelineRunState{{ + // parent task is skipped because conditions fail, not because of its parent tasks + PipelineTask: &pts[5], + TaskRunName: "pipelinerun-conditionaltask", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + ResolvedConditionChecks: failedTaskConditionCheckState, + }, { + // parent task is skipped because when expressions evaluate to false, not because of its parent tasks + PipelineTask: &pts[10], + TaskRunName: "pipelinerun-guardedtask", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }, { + // skipped because of parent task guarded using conditions is skipped, regardless of another parent task + // being guarded with when expressions that are scoped to task + PipelineTask: &v1beta1.PipelineTask{ + Name: "mytask18", + TaskRef: &v1beta1.TaskRef{Name: "task"}, + RunAfter: []string{"mytask6", "mytask11"}, + }, + TaskRunName: "pipelinerun-ordering-dependent-task-1", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }, { + // child task is not skipped regardless of its parent task being skipped due to when expressions evaluating + // to false, because when expressions are scoped to task only + PipelineTask: &v1beta1.PipelineTask{ + Name: "mytask19", + TaskRef: &v1beta1.TaskRef{Name: "task"}, + RunAfter: []string{"mytask11"}, + }, + TaskRunName: "pipelinerun-ordering-dependent-task-2", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }}, + scopeWhenExpressionsToTask: true, + expected: map[string]bool{ + "mytask6": false, + "mytask11": false, + "mytask18": true, + "mytask19": false, + }, }} { t.Run(tc.name, func(t *testing.T) { d, err := dagFromState(tc.state) @@ -991,17 +1591,18 @@ func TestIsSkipped(t *testing.T) { } stateMap := tc.state.ToMap() facts := PipelineRunFacts{ - State: tc.state, - TasksGraph: d, - FinalTasksGraph: &dag.Graph{}, + State: tc.state, + TasksGraph: d, + FinalTasksGraph: &dag.Graph{}, + ScopeWhenExpressionsToTask: tc.scopeWhenExpressionsToTask, } for taskName, isSkipped := range tc.expected { rprt := stateMap[taskName] if rprt == nil { t.Fatalf("Could not get task %s from the state: %v", taskName, tc.state) } - if d := cmp.Diff(isSkipped, rprt.Skip(&facts)); d != "" { - t.Errorf("Didn't get expected isSkipped %s", diff.PrintWantGot(d)) + if d := cmp.Diff(isSkipped, rprt.skipBecauseParentTaskWasSkipped(&facts)); d != "" { + t.Errorf("Didn't get expected isSkipped from task %s: %s", taskName, diff.PrintWantGot(d)) } } }) @@ -2408,7 +3009,7 @@ func TestResolvedPipelineRunTask_IsFinallySkipped(t *testing.T) { for i := range state { if i > 0 { // first one is a dag task that produces a result finallyTaskName := state[i].PipelineTask.Name - if d := cmp.Diff(expected[finallyTaskName], state[i].IsFinallySkipped(facts)); d != "" { + if d := cmp.Diff(expected[finallyTaskName], state[i].IsFinallySkipped(facts).IsSkipped); d != "" { t.Fatalf("Didn't get expected isFinallySkipped from finally task %s: %s", finallyTaskName, diff.PrintWantGot(d)) } } diff --git a/pkg/reconciler/pipelinerun/resources/pipelinerunstate.go b/pkg/reconciler/pipelinerun/resources/pipelinerunstate.go index 1feea87443e..078b52fe262 100644 --- a/pkg/reconciler/pipelinerun/resources/pipelinerunstate.go +++ b/pkg/reconciler/pipelinerun/resources/pipelinerunstate.go @@ -41,7 +41,10 @@ const ( // state of the PipelineRun. type PipelineRunState []*ResolvedPipelineRunTask -// PipelineRunFacts is a collection of list of ResolvedPipelineTask, graph of DAG tasks, and graph of finally tasks +// PipelineRunFacts holds the state of all the components that make up the Pipeline graph that are used to track the +// PipelineRun state without passing all these components separately. It helps simplify our implementation for getting +// and scheduling the next tasks. It is a collection of list of ResolvedPipelineTask, graph of DAG tasks, graph of +// finally tasks, cache of skipped tasks, and the scope of when expressions. type PipelineRunFacts struct { State PipelineRunState SpecStatus v1beta1.PipelineRunSpecStatus @@ -56,7 +59,8 @@ type PipelineRunFacts struct { // needed, via the `Skip` method in pipelinerunresolution.go // The skip data is sensitive to changes in the state. The ResetSkippedCache method // can be used to clean the cache and force re-computation when needed. - SkipCache map[string]bool + SkipCache map[string]TaskSkipStatus + ScopeWhenExpressionsToTask bool } // pipelineRunStatusCount holds the count of successful, failed, cancelled, skipped, and incomplete tasks @@ -75,7 +79,7 @@ type pipelineRunStatusCount struct { // ResetSkippedCache resets the skipped cache in the facts map func (facts *PipelineRunFacts) ResetSkippedCache() { - facts.SkipCache = make(map[string]bool) + facts.SkipCache = make(map[string]TaskSkipStatus) } // ToMap returns a map that maps pipeline task name to the resolved pipeline run task @@ -406,14 +410,14 @@ func (facts *PipelineRunFacts) GetPipelineConditionStatus(pr *v1beta1.PipelineRu func (facts *PipelineRunFacts) GetSkippedTasks() []v1beta1.SkippedTask { var skipped []v1beta1.SkippedTask for _, rprt := range facts.State { - if rprt.Skip(facts) { + if rprt.Skip(facts).IsSkipped { skippedTask := v1beta1.SkippedTask{ Name: rprt.PipelineTask.Name, WhenExpressions: rprt.PipelineTask.WhenExpressions, } skipped = append(skipped, skippedTask) } - if rprt.IsFinallySkipped(facts) { + if rprt.IsFinallySkipped(facts).IsSkipped { skippedTask := v1beta1.SkippedTask{ Name: rprt.PipelineTask.Name, } @@ -465,7 +469,7 @@ func (facts *PipelineRunFacts) GetPipelineTaskStatus() map[string]string { } // if any of the dag task skipped, change the aggregate status to completed // but continue checking for any other failure - if t.Skip(facts) { + if t.Skip(facts).IsSkipped { aggregateStatus = v1beta1.PipelineRunReasonCompleted.String() } } @@ -481,7 +485,7 @@ func (facts *PipelineRunFacts) successfulOrSkippedDAGTasks() []string { tasks := []string{} for _, t := range facts.State { if facts.isDAGTask(t.PipelineTask.Name) { - if t.IsSuccessful() || t.Skip(facts) { + if t.IsSuccessful() || t.Skip(facts).IsSkipped { tasks = append(tasks, t.PipelineTask.Name) } } @@ -533,10 +537,10 @@ func (facts *PipelineRunFacts) getPipelineTasksCount() pipelineRunStatusCount { case t.IsFailure(): s.Failed++ // increment skip counter since the task is skipped - case t.Skip(facts): + case t.Skip(facts).IsSkipped: s.Skipped++ // checking if any finally tasks were referring to invalid/missing task results - case t.IsFinallySkipped(facts): + case t.IsFinallySkipped(facts).IsSkipped: s.Skipped++ // increment incomplete counter since the task is pending and not executed yet default: