From 379ba8db1936970003ff58cb1374d5d53430c7cd 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 | 138 +++++++++++- 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 | 139 ++++++++++++ .../resources/pipelinerunresolution.go | 125 ++++++++--- .../resources/pipelinerunresolution_test.go | 210 +++++++++++++++++- .../pipelinerun/resources/pipelinerunstate.go | 22 +- 13 files changed, 636 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..be4f98e7a7e 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 WhenExpressions 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 deff9b68e0d..2976e5b7cbe 100644 --- a/docs/deprecations.md +++ b/docs/deprecations.md @@ -25,3 +25,5 @@ being deprecated. | [`Conditions` CRD is deprecated and will be removed. Use `WhenExpressions` 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.26.0](https://github.com/tektoncd/pipeline/releases/tag/v0.26.0) | Beta | February 10 2022 | +| [The `scope-when-expressions-to-task` flag will be removed](https://github.com/tektoncd/pipeline/issues/1836) | [v0.26.0](https://github.com/tektoncd/pipeline/releases/tag/v0.26.0) | Beta | February 10 2022 | diff --git a/docs/install.md b/docs/install.md index 5ca60d67f56..4096d579452 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 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 20c4531badd..0754aa49d1e 100644 --- a/docs/pipelines.md +++ b/docs/pipelines.md @@ -465,6 +465,133 @@ There are a lot of scenarios where `WhenExpressions` 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 `when` expressions to the specific dependent `Tasks` to be guarded as well. + +``` + 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` and its dependent `Tasks` can design the +`Pipeline` as such: + +```yaml +tasks: +... +- name: manual-approval + runAfter: + - integration-tests + when: + - input: $(params.git-action) + operator: in + values: + - merge + taskRef: + name: manual-approval + +- name: slack-msg + params: + - name: approver + value: $(tasks.manual-approval.results.approver) + taskRef: + name: slack-msg + +- 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 +``` + +#### 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". + +``` + 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: slack-msg + params: + - name: approver + value: $(tasks.manual-approval.results.approver) + taskRef: + name: slack-msg + +- name: build-image + runAfter: + - manual-approval + taskRef: + name: build-image + +- name: deploy-image + runAfter: + - build-image + taskRef: + name: deploy-image +``` + ### Guard `Task` execution using `Conditions` **Note:** `Conditions` are [deprecated](./deprecations.md), use [`WhenExpressions`](#guard-task-execution-using-whenexpressions) instead. @@ -691,10 +818,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 3e125f332ba..f231767bdb8 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun.go +++ b/pkg/reconciler/pipelinerun/pipelinerun.go @@ -493,10 +493,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 { @@ -647,7 +648,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 9c9a1758cf9..80766a39c03 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun_test.go +++ b/pkg/reconciler/pipelinerun/pipelinerun_test.go @@ -3487,6 +3487,145 @@ 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 because it runs after a-task and when expressions are scoped to task + tb.PipelineTask("b-task", "b-task", + tb.RunAfter("a-task"), + ), + // c-task is attempted because when expressions are scoped to task but then get skipped because of + // missing result references from a-task + tb.PipelineTask("c-task", "c-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")), + } + + // 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: 1, Skipped: 2", + } + pipelineRun, clients := prt.reconcileRun("foo", "test-pipeline-run-different-service-accs", wantEvents, false) + + // b-task that should be executed because it runs after a-task and when expressions are scoped to task + expectedTaskRunName := "test-pipeline-run-different-service-accs-b-task-mz4c7" + expectedTaskRun := tb.TaskRun(expectedTaskRunName, + 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/pipeline", "test-pipeline"), + tb.TaskRunLabel("tekton.dev/pipelineRun", "test-pipeline-run-different-service-accs"), + tb.TaskRunLabel("tekton.dev/pipelineTask", "b-task"), + tb.TaskRunSpec( + tb.TaskRunTaskRef("b-task"), + tb.TaskRunServiceAccountName("test-sa-0"), + ), + ) + // Check that the expected TaskRun from b-task was created + actual, err := clients.Pipeline.TektonV1beta1().TaskRuns("foo").List(prt.TestAssets.Ctx, metav1.ListOptions{ + LabelSelector: "tekton.dev/pipelineTask=b-task,tekton.dev/pipelineRun=test-pipeline-run-different-service-accs", + 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", expectedTaskRunName, diff.PrintWantGot(d)) + } + + 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"}, + }}, + }, { + // was attempted, but has missing results references + Name: "c-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 and c-task + skippedTasks := []string{"a-task", "c-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 a4759fad948..d23e8731f19 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" + MissingResultsReferenceSkip SkippingReason = "MissingResultsReferenceSkip" + 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 = MissingResultsReferenceSkip + 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 = MissingResultsReferenceSkip + 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..0a677d65037 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,196 @@ 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 parent 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-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) @@ -991,17 +1182,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.Skip(&facts).IsSkipped); d != "" { + t.Errorf("Didn't get expected isSkipped from task %s: %s", taskName, diff.PrintWantGot(d)) } } }) @@ -2408,7 +2600,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: