From dabdf4d842797dd12136e7b910f67096420c2ee4 Mon Sep 17 00:00:00 2001 From: Christie Wilson Date: Wed, 10 Oct 2018 16:54:34 -0700 Subject: [PATCH] Check status of TaskRuns when finding TaskRun to start Added logic to check statuses of other TaskRuns when deciding if a new one should be started for #61 --- pkg/apis/pipeline/v1alpha1/taskrun_types.go | 7 ++ .../pipelinerun/resources/pipelinestate.go | 20 +++- .../resources/pipelinestate_test.go | 103 +++++++++++++++++- 3 files changed, 121 insertions(+), 9 deletions(-) diff --git a/pkg/apis/pipeline/v1alpha1/taskrun_types.go b/pkg/apis/pipeline/v1alpha1/taskrun_types.go index fe760ce42f6..f1e8490190a 100644 --- a/pkg/apis/pipeline/v1alpha1/taskrun_types.go +++ b/pkg/apis/pipeline/v1alpha1/taskrun_types.go @@ -74,6 +74,13 @@ type TaskRunStatus struct { Conditions duckv1alpha1.Conditions `json:"conditions,omitempty"` } +var taskRunCondSet = duckv1alpha1.NewBatchConditionSet() + +// GetCondition returns the Condition matching the given type. +func (tr *TaskRunStatus) GetCondition(t duckv1alpha1.ConditionType) *duckv1alpha1.Condition { + return taskRunCondSet.Manage(tr).GetCondition(t) +} + // StepRun reports the results of running a step in the Task. Each // task has the potential to succeed or fail (based on the exit code) // and produces logs. diff --git a/pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinestate.go b/pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinestate.go index c32bc623008..ad5ea85c2af 100644 --- a/pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinestate.go +++ b/pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinestate.go @@ -19,6 +19,8 @@ package resources import ( "fmt" + duckv1alpha1 "github.com/knative/pkg/apis/duck/v1alpha1" + corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" "github.com/knative/build-pipeline/pkg/apis/pipeline/v1alpha1" @@ -28,7 +30,17 @@ import ( // not have a corresponding TaskRun and can run. func GetNextTask(pipelineTaskRuns []*PipelineRunTaskRun) *PipelineRunTaskRun { for _, prtr := range pipelineTaskRuns { - if prtr.TaskRun == nil && canTaskRun(prtr.PipelineTask) { + if prtr.TaskRun != nil { + switch s := prtr.TaskRun.Status.GetCondition(duckv1alpha1.ConditionSucceeded); s.Status { + // if any of the TaskRuns failed, there is no new TaskRun to start + case corev1.ConditionFalse: + return nil + // if the current TaskRun is currently running, don't start another one + case corev1.ConditionUnknown: + return nil + } + // otherwise the TaskRun has finished successfully, so we should move on + } else if canTaskRun(prtr.PipelineTask) { return prtr } } @@ -36,11 +48,7 @@ func GetNextTask(pipelineTaskRuns []*PipelineRunTaskRun) *PipelineRunTaskRun { } func canTaskRun(pt *v1alpha1.PipelineTask) bool { - // Check if Task can run now. Go through all the input constraints and see if - // the upstream tasks have completed successfully and inputs are available. - - // TODO: only should try to run this Task if the previous one has completed - + // Check if Task can run now. Go through all the input constraints return true } diff --git a/pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinestate_test.go b/pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinestate_test.go index 7449a15f3c9..9ae8be7984b 100644 --- a/pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinestate_test.go +++ b/pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinestate_test.go @@ -21,6 +21,8 @@ import ( "testing" "github.com/google/go-cmp/cmp" + duckv1alpha1 "github.com/knative/pkg/apis/duck/v1alpha1" + corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" "github.com/knative/build-pipeline/pkg/apis/pipeline/v1alpha1" @@ -71,6 +73,39 @@ var trs = []v1alpha1.TaskRun{{ Spec: v1alpha1.TaskRunSpec{}, }} +func makeStarted(tr v1alpha1.TaskRun) *v1alpha1.TaskRun { + newTr := newTaskRun(tr) + newTr.Status.Conditions[0].Status = corev1.ConditionUnknown + return newTr +} + +func makeSucceeded(tr v1alpha1.TaskRun) *v1alpha1.TaskRun { + newTr := newTaskRun(tr) + newTr.Status.Conditions[0].Status = corev1.ConditionTrue + return newTr +} + +func makeFailed(tr v1alpha1.TaskRun) *v1alpha1.TaskRun { + newTr := newTaskRun(tr) + newTr.Status.Conditions[0].Status = corev1.ConditionFalse + return newTr +} + +func newTaskRun(tr v1alpha1.TaskRun) *v1alpha1.TaskRun { + return &v1alpha1.TaskRun{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: tr.Namespace, + Name: tr.Name, + }, + Spec: tr.Spec, + Status: v1alpha1.TaskRunStatus{ + Conditions: []duckv1alpha1.Condition{{ + Type: duckv1alpha1.ConditionSucceeded, + }}, + }, + } +} + func TestGetNextTask_NoneStarted(t *testing.T) { noneStartedState := []*PipelineRunTaskRun{{ Task: task, @@ -83,19 +118,61 @@ func TestGetNextTask_NoneStarted(t *testing.T) { TaskRunName: "pipelinerun-mytask2", TaskRun: nil, }} - // TODO: one started + oneStartedState := []*PipelineRunTaskRun{{ + Task: task, + PipelineTask: &pts[0], + TaskRunName: "pipelinerun-mytask1", + TaskRun: makeStarted(trs[0]), + }, { + Task: task, + PipelineTask: &pts[1], + TaskRunName: "pipelinerun-mytask2", + TaskRun: nil, + }} + oneFinishedState := []*PipelineRunTaskRun{{ + Task: task, + PipelineTask: &pts[0], + TaskRunName: "pipelinerun-mytask1", + TaskRun: makeSucceeded(trs[0]), + }, { + Task: task, + PipelineTask: &pts[1], + TaskRunName: "pipelinerun-mytask2", + TaskRun: nil, + }} + oneFailedState := []*PipelineRunTaskRun{{ + Task: task, + PipelineTask: &pts[0], + TaskRunName: "pipelinerun-mytask1", + TaskRun: makeFailed(trs[0]), + }, { + Task: task, + PipelineTask: &pts[1], + TaskRunName: "pipelinerun-mytask2", + TaskRun: nil, + }} firstFinishedState := []*PipelineRunTaskRun{{ Task: task, PipelineTask: &pts[0], TaskRunName: "pipelinerun-mytask1", - TaskRun: &trs[0], + TaskRun: makeSucceeded(trs[0]), }, { Task: task, PipelineTask: &pts[1], TaskRunName: "pipelinerun-mytask2", TaskRun: nil, }} - // TODO: all finished + allFinishedState := []*PipelineRunTaskRun{{ + Task: task, + PipelineTask: &pts[0], + TaskRunName: "pipelinerun-mytask1", + TaskRun: makeSucceeded(trs[0]), + }, { + Task: task, + PipelineTask: &pts[1], + TaskRunName: "pipelinerun-mytask2", + TaskRun: makeSucceeded(trs[0]), + }} tcs := []struct { name string state []*PipelineRunTaskRun @@ -106,11 +183,31 @@ func TestGetNextTask_NoneStarted(t *testing.T) { state: noneStartedState, expectedTask: noneStartedState[0], }, + { + name: "one-task-started", + state: oneStartedState, + expectedTask: nil, + }, + { + name: "one-task-finished", + state: oneFinishedState, + expectedTask: oneFinishedState[1], + }, + { + name: "one-task-failed", + state: oneFailedState, + expectedTask: nil, + }, { name: "first-task-finished", state: firstFinishedState, expectedTask: firstFinishedState[1], }, + { + name: "all-finished", + state: allFinishedState, + expectedTask: nil, + }, } for _, tc := range tcs { t.Run(tc.name, func(t *testing.T) {