From d45879ff46d5b3fdfe3eae2383c3c1ab18dd9e2a Mon Sep 17 00:00:00 2001 From: Nader Ziada Date: Thu, 18 Oct 2018 10:12:54 -0400 Subject: [PATCH] Add passedConstraint checking when creating taskrun - Before a pipeline run creates a taskrun for the next task, it needs to check the passed constraints to respect dependecnies between tasks - add implementation of canTaskRun and unit tests --- .../resources/passedconstraint_test.go | 174 ++++++++++++++++++ .../pipelinerun/resources/pipelinestate.go | 28 ++- test/helm_task_test.go | 25 ++- 3 files changed, 224 insertions(+), 3 deletions(-) create mode 100644 pkg/reconciler/v1alpha1/pipelinerun/resources/passedconstraint_test.go diff --git a/pkg/reconciler/v1alpha1/pipelinerun/resources/passedconstraint_test.go b/pkg/reconciler/v1alpha1/pipelinerun/resources/passedconstraint_test.go new file mode 100644 index 00000000000..cd6ba52fb36 --- /dev/null +++ b/pkg/reconciler/v1alpha1/pipelinerun/resources/passedconstraint_test.go @@ -0,0 +1,174 @@ +/* +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{ + 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{ + v1alpha1.TaskResource{ + Name: "myresource1", + Type: v1alpha1.PipelineResourceTypeGit, + }, + }, + }, + }, +} + +var mypipelinetasks = []v1alpha1.PipelineTask{{ + Name: "mypipelinetask1", + TaskRef: v1alpha1.TaskRef{Name: "mytask1"}, + InputSourceBindings: []v1alpha1.SourceBinding{{ + Name: "some-name-1", + Key: "myresource1", + ResourceRef: v1alpha1.PipelineResourceRef{ + Name: "myresource1", + }, + }}, +}, { + Name: "mypipelinetask2", + TaskRef: v1alpha1.TaskRef{Name: "mytask2"}, + InputSourceBindings: []v1alpha1.SourceBinding{{ + Name: "some-name-2", + Key: "myresource1", + ResourceRef: v1alpha1.PipelineResourceRef{ + Name: "myresource1", + }, + PassedConstraints: []string{"mytask1"}, + }}, +}} + +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 []*PipelineRunTaskRun + canSecondTaskRun bool + }{ + { + name: "first-task-not-started", + state: []*PipelineRunTaskRun{{ + Task: mytask1, + PipelineTask: &mypipelinetasks[0], + TaskRunName: "pipelinerun-mytask1", + TaskRun: nil, + }, { + Task: mytask2, + PipelineTask: &mypipelinetasks[1], + TaskRunName: "pipelinerun-mytask2", + TaskRun: nil, + }}, + canSecondTaskRun: false, + }, + { + name: "first-task-running", + state: []*PipelineRunTaskRun{{ + Task: mytask1, + PipelineTask: &mypipelinetasks[0], + TaskRunName: "pipelinerun-mytask1", + TaskRun: makeStarted(mytaskruns[0]), + }, { + Task: mytask2, + PipelineTask: &mypipelinetasks[1], + TaskRunName: "pipelinerun-mytask2", + TaskRun: nil, + }}, + canSecondTaskRun: false, + }, + { + name: "first-task-failed", + state: []*PipelineRunTaskRun{{ + Task: mytask1, + PipelineTask: &mypipelinetasks[0], + TaskRunName: "pipelinerun-mytask1", + TaskRun: makeFailed(mytaskruns[0]), + }, { + Task: mytask2, + PipelineTask: &mypipelinetasks[1], + TaskRunName: "pipelinerun-mytask2", + TaskRun: nil, + }}, + canSecondTaskRun: false, + }, + { + name: "first-task-finished", + state: []*PipelineRunTaskRun{{ + Task: mytask1, + PipelineTask: &mypipelinetasks[0], + TaskRunName: "pipelinerun-mytask1", + TaskRun: makeSucceeded(mytaskruns[0]), + }, { + Task: mytask2, + 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) + } + }) + } +} diff --git a/pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinestate.go b/pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinestate.go index 107a8330c92..97ba67ce3e2 100644 --- a/pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinestate.go +++ b/pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinestate.go @@ -45,7 +45,7 @@ func GetNextTask(prName string, state []*PipelineRunTaskRun, logger *zap.Sugared 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) { + } else if canTaskRun(prtr.PipelineTask, state) { logger.Infof("TaskRun %s should be started for PipelineRun %s", prtr.TaskRunName, prName) return prtr } @@ -54,8 +54,32 @@ func GetNextTask(prName string, state []*PipelineRunTaskRun, logger *zap.Sugared return nil } -func canTaskRun(pt *v1alpha1.PipelineTask) bool { +func canTaskRun(pt *v1alpha1.PipelineTask, state []*PipelineRunTaskRun) bool { // Check if Task can run now. Go through all the input constraints + for _, input := range pt.InputSourceBindings { + if len(input.PassedConstraints) > 0 { + for _, constrainingTaskName := range input.PassedConstraints { + for _, prtr := range state { + // the constraining task must have a successful task run to allow this task to run + if prtr.Task.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 } diff --git a/test/helm_task_test.go b/test/helm_task_test.go index e729fe4c4a4..337785633c5 100644 --- a/test/helm_task_test.go +++ b/test/helm_task_test.go @@ -144,7 +144,7 @@ func getCreateImageTask(namespace string, t *testing.T) *v1alpha1.Task { } imageName = fmt.Sprintf("%s/%s", dockerRepo, AppendRandomString(sourceImageName)) - t.Log("Image to be pusblished: %s", imageName) + t.Logf("Image to be pusblished: %s", imageName) return &v1alpha1.Task{ ObjectMeta: metav1.ObjectMeta{ @@ -255,6 +255,7 @@ func getHelmDeployPipeline(namespace string) *v1alpha1.Pipeline { ResourceRef: v1alpha1.PipelineResourceRef{ Name: sourceResourceName, }, + PassedConstraints: []string{createImageTaskName}, }}, Params: []v1alpha1.Param{{ Name: "pathToHelmCharts", @@ -306,7 +307,29 @@ func setupClusterBindingForHelm(c *clients, t *testing.T, namespace string) { }}, } + t.Logf("Creating Cluster Role binding in kube-system for helm in namespace %s", namespace) if _, err := c.KubeClient.Kube.RbacV1beta1().ClusterRoleBindings().Create(defaultClusterRoleBinding); err != nil { t.Fatalf("Failed to create default Service account for Helm in namespace: %s - %s", namespace, err) } + + kubesystemClusterRoleBinding := &rbacv1.ClusterRoleBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: AppendRandomString("default-tiller"), + }, + RoleRef: rbacv1.RoleRef{ + APIGroup: "rbac.authorization.k8s.io", + Kind: "ClusterRole", + Name: "cluster-admin", + }, + Subjects: []rbacv1.Subject{{ + Kind: "ServiceAccount", + Name: "default", + Namespace: "kube-system", + }}, + } + + t.Logf("Creating Cluster Role binding in kube-system for helm") + if _, err := c.KubeClient.Kube.RbacV1beta1().ClusterRoleBindings().Create(kubesystemClusterRoleBinding); err != nil { + t.Fatalf("Failed to create default Service account for Helm in kube-system - %s", err) + } }