From 9bbebd307559e1c38243580430aa85f825792af1 Mon Sep 17 00:00:00 2001 From: Christie Wilson Date: Mon, 21 Jan 2019 15:11:38 -0800 Subject: [PATCH] On Pipeline creation, verify order of `providedBy` constraints Tasks should only rely on outputs of tasks that execute before them, they shouldn't be able to rely on outputs of future tasks. This also means we no longer need the `canRunTask` function: since Tasks currently execute linearly (see #168), the only way `canRunTask` could return false would be if `Tasks` in `providedBy` actually executed _after_ the one currently being evaluated, which would result in an unrunnable and invalid Pipeline. Now we will catch this on Pipeline creation. Part of #320 --- .../pipeline/v1alpha1/pipeline_validation.go | 18 ++- .../v1alpha1/pipeline_validation_test.go | 21 ++- .../resources/pipelinerunresolution.go | 33 +--- .../pipelinerun/resources/providedby_test.go | 153 ------------------ 4 files changed, 34 insertions(+), 191 deletions(-) delete mode 100644 pkg/reconciler/v1alpha1/pipelinerun/resources/providedby_test.go diff --git a/pkg/apis/pipeline/v1alpha1/pipeline_validation.go b/pkg/apis/pipeline/v1alpha1/pipeline_validation.go index c98870bd8b5..463a4eb03ad 100644 --- a/pkg/apis/pipeline/v1alpha1/pipeline_validation.go +++ b/pkg/apis/pipeline/v1alpha1/pipeline_validation.go @@ -48,12 +48,24 @@ func (ps *PipelineSpec) Validate() *apis.FieldError { taskNames[t.Name] = struct{}{} } - // providedBy should match other tasks. - for _, t := range ps.Tasks { + // providedBy should match future tasks + // TODO(#168) when pipelines don't just execute linearly this will need to be more sophisticated + for i, t := range ps.Tasks { if t.Resources != nil { for _, rd := range t.Resources.Inputs { for _, pb := range rd.ProvidedBy { - if _, ok := taskNames[pb]; !ok { + if i == 0 { + // First Task can't depend on anything before it (b/c there is nothing) + return apis.ErrInvalidKeyName(pb, fmt.Sprintf("spec.tasks.resources.%s", pb)) + } + found := false + // Look for previous Task that satisfies constraint + for j := i - 1; j >= 0; j-- { + if ps.Tasks[j].Name == pb { + found = true + } + } + if !found { return apis.ErrInvalidKeyName(pb, fmt.Sprintf("spec.tasks.resources.%s", pb)) } } diff --git a/pkg/apis/pipeline/v1alpha1/pipeline_validation_test.go b/pkg/apis/pipeline/v1alpha1/pipeline_validation_test.go index 9c3a5bce004..200e39de648 100644 --- a/pkg/apis/pipeline/v1alpha1/pipeline_validation_test.go +++ b/pkg/apis/pipeline/v1alpha1/pipeline_validation_test.go @@ -40,7 +40,7 @@ func TestPipelineSpec_Validate_Error(t *testing.T) { }, }, { - name: "invalid constraint tasks", + name: "providedby task doesnt exist", fields: fields{ Tasks: []PipelineTask{{ Name: "foo", @@ -52,6 +52,21 @@ func TestPipelineSpec_Validate_Error(t *testing.T) { }}, }, }, + { + name: "providedby task is afterward", + fields: fields{ + Tasks: []PipelineTask{{ + Name: "foo", + Resources: &PipelineTaskResources{ + Inputs: []PipelineTaskInputResource{{ + ProvidedBy: []string{"bar"}, + }}, + }, + }, { + Name: "bar", + }}, + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -89,14 +104,14 @@ func TestPipelineSpec_Validate_Valid(t *testing.T) { name: "valid constraint tasks", fields: fields{ Tasks: []PipelineTask{{ + Name: "bar", + }, { Name: "foo", Resources: &PipelineTaskResources{ Inputs: []PipelineTaskInputResource{{ ProvidedBy: []string{"bar"}, }}, }, - }, { - Name: "bar", }}, }, }, diff --git a/pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinerunresolution.go b/pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinerunresolution.go index 3047224a72a..f5a8a9fea19 100644 --- a/pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinerunresolution.go +++ b/pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinerunresolution.go @@ -60,7 +60,7 @@ func GetNextTask(prName string, state []*ResolvedPipelineRunTask, logger *zap.Su logger.Infof("TaskRun %s is still running so we shouldn't start more for PipelineRun %s", prtr.TaskRunName, prName) return nil } - } else if canTaskRun(prtr.PipelineTask, state) { + } else { logger.Infof("TaskRun %s should be started for PipelineRun %s", prtr.TaskRunName, prName) return prtr } @@ -69,37 +69,6 @@ func GetNextTask(prName string, state []*ResolvedPipelineRunTask, logger *zap.Su return nil } -func canTaskRun(pt *v1alpha1.PipelineTask, state []*ResolvedPipelineRunTask) bool { - // Check if Task can run now. Go through all the input constraints - if pt.Resources != nil { - for _, rd := range pt.Resources.Inputs { - if len(rd.ProvidedBy) > 0 { - for _, constrainingTaskName := range rd.ProvidedBy { - for _, prtr := range state { - // the constraining task must have a successful task run to allow this task to run - if prtr.PipelineTask.Name == constrainingTaskName { - if prtr.TaskRun == nil { - return false - } - c := prtr.TaskRun.Status.GetCondition(duckv1alpha1.ConditionSucceeded) - if c == nil { - return false - } - switch c.Status { - case corev1.ConditionFalse: - return false - case corev1.ConditionUnknown: - return false - } - } - } - } - } - } - } - return true -} - // ResolvedPipelineRunTask contains a Task and its associated TaskRun, if it // exists. TaskRun can be nil to represent there being no TaskRun. type ResolvedPipelineRunTask struct { diff --git a/pkg/reconciler/v1alpha1/pipelinerun/resources/providedby_test.go b/pkg/reconciler/v1alpha1/pipelinerun/resources/providedby_test.go deleted file mode 100644 index a7a52a7385f..00000000000 --- a/pkg/reconciler/v1alpha1/pipelinerun/resources/providedby_test.go +++ /dev/null @@ -1,153 +0,0 @@ -/* -Copyright 2018 The Knative 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 - - http://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. -*/ - -package resources - -import ( - "testing" - - "github.com/google/go-cmp/cmp" - - "github.com/knative/build-pipeline/pkg/apis/pipeline/v1alpha1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" -) - -var mytask1 = &v1alpha1.Task{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: "namespace", - Name: "mytask1", - }, - Spec: v1alpha1.TaskSpec{ - Inputs: &v1alpha1.Inputs{ - Resources: []v1alpha1.TaskResource{{ - Name: "myresource1", - Type: v1alpha1.PipelineResourceTypeGit, - }}, - }, - }, -} - -var mytask2 = &v1alpha1.Task{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: "namespace", - Name: "mytask2", - }, - Spec: v1alpha1.TaskSpec{ - Inputs: &v1alpha1.Inputs{ - Resources: []v1alpha1.TaskResource{{ - Name: "myresource1", - Type: v1alpha1.PipelineResourceTypeGit, - }}, - }, - }, -} - -var mypipelinetasks = []v1alpha1.PipelineTask{{ - Name: "mypipelinetask1", - TaskRef: v1alpha1.TaskRef{Name: "mytask1"}, -}, { - Name: "mypipelinetask2", - TaskRef: v1alpha1.TaskRef{Name: "mytask2"}, - Resources: &v1alpha1.PipelineTaskResources{ - Inputs: []v1alpha1.PipelineTaskInputResource{{ - Name: "myresource1", - ProvidedBy: []string{"mypipelinetask1"}, - }}, - }, -}} - -var mytaskruns = []v1alpha1.TaskRun{{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: "namespace", - Name: "pipelinerun-mytask1", - }, - Spec: v1alpha1.TaskRunSpec{}, -}, { - ObjectMeta: metav1.ObjectMeta{ - Namespace: "namespace", - Name: "pipelinerun-mytask2", - }, - Spec: v1alpha1.TaskRunSpec{}, -}} - -func TestCanTaskRun(t *testing.T) { - tcs := []struct { - name string - state []*ResolvedPipelineRunTask - canSecondTaskRun bool - }{ - { - name: "first-task-not-started", - state: []*ResolvedPipelineRunTask{{ - PipelineTask: &mypipelinetasks[0], - TaskRunName: "pipelinerun-mytask1", - TaskRun: nil, - }, { - PipelineTask: &mypipelinetasks[1], - TaskRunName: "pipelinerun-mytask2", - TaskRun: nil, - }}, - canSecondTaskRun: false, - }, - { - name: "first-task-running", - state: []*ResolvedPipelineRunTask{{ - PipelineTask: &mypipelinetasks[0], - TaskRunName: "pipelinerun-mytask1", - TaskRun: makeStarted(mytaskruns[0]), - }, { - PipelineTask: &mypipelinetasks[1], - TaskRunName: "pipelinerun-mytask2", - TaskRun: nil, - }}, - canSecondTaskRun: false, - }, - { - name: "first-task-failed", - state: []*ResolvedPipelineRunTask{{ - PipelineTask: &mypipelinetasks[0], - TaskRunName: "pipelinerun-mytask1", - TaskRun: makeFailed(mytaskruns[0]), - }, { - PipelineTask: &mypipelinetasks[1], - TaskRunName: "pipelinerun-mytask2", - TaskRun: nil, - }}, - canSecondTaskRun: false, - }, - { - name: "first-task-finished", - state: []*ResolvedPipelineRunTask{{ - PipelineTask: &mypipelinetasks[0], - TaskRunName: "pipelinerun-mytask1", - TaskRun: makeSucceeded(mytaskruns[0]), - }, { - PipelineTask: &mypipelinetasks[1], - TaskRunName: "pipelinerun-mytask2", - TaskRun: nil, - }}, - canSecondTaskRun: true, - }, - } - for _, tc := range tcs { - t.Run(tc.name, func(t *testing.T) { - cantaskrun := canTaskRun(&mypipelinetasks[1], tc.state) - if d := cmp.Diff(cantaskrun, tc.canSecondTaskRun); d != "" { - t.Fatalf("Expected second task availability to run should be %t, but different state returned: %s", tc.canSecondTaskRun, d) - } - }) - } -}