From 0b4864bfd671868964a66bc6bb36a6758d521ea9 Mon Sep 17 00:00:00 2001 From: Priti Desai Date: Tue, 17 Nov 2020 23:11:34 -0800 Subject: [PATCH] fix cycle detection logic visited map holds name of the nodes been visited as keys and "true" as value. This map is being updated with currentName.HashKey on every iteration which results in keys such as a.d, b.d where a, b, and d are nodes of a dag. But such keys are not utilized anywhere in the visit function. Visit function checks existence of the node just by the name without any string concatenation. This extra addition in the map is causing severe delay for a graph with more than >60 nodes. --- pkg/reconciler/pipeline/dag/dag.go | 14 ++--- pkg/reconciler/pipeline/dag/dag_test.go | 83 ++++++++++++++++++++++++- 2 files changed, 85 insertions(+), 12 deletions(-) diff --git a/pkg/reconciler/pipeline/dag/dag.go b/pkg/reconciler/pipeline/dag/dag.go index 16f39fc4fce..52f607c8d51 100644 --- a/pkg/reconciler/pipeline/dag/dag.go +++ b/pkg/reconciler/pipeline/dag/dag.go @@ -126,9 +126,8 @@ func linkPipelineTasks(prev *Node, next *Node) error { return fmt.Errorf("cycle detected; task %q depends on itself", next.Task.HashKey()) } // Check if we are adding cycles. - visited := map[string]bool{prev.Task.HashKey(): true, next.Task.HashKey(): true} path := []string{next.Task.HashKey(), prev.Task.HashKey()} - if err := visit(next.Task.HashKey(), prev.Prev, path, visited); err != nil { + if err := lookForNode(prev.Prev, path, next.Task.HashKey()); err != nil { return fmt.Errorf("cycle detected: %w", err) } next.Prev = append(next.Prev, prev) @@ -136,18 +135,13 @@ func linkPipelineTasks(prev *Node, next *Node) error { return nil } -func visit(currentName string, nodes []*Node, path []string, visited map[string]bool) error { - var sb strings.Builder +func lookForNode(nodes []*Node, path []string, next string) error { for _, n := range nodes { path = append(path, n.Task.HashKey()) - if _, ok := visited[n.Task.HashKey()]; ok { + if n.Task.HashKey() == next { return errors.New(getVisitedPath(path)) } - sb.WriteString(currentName) - sb.WriteByte('.') - sb.WriteString(n.Task.HashKey()) - visited[sb.String()] = true - if err := visit(n.Task.HashKey(), n.Prev, path, visited); err != nil { + if err := lookForNode(n.Prev, path, next); err != nil { return err } } diff --git a/pkg/reconciler/pipeline/dag/dag_test.go b/pkg/reconciler/pipeline/dag/dag_test.go index 14ee4f69a1b..0eed7963ce5 100644 --- a/pkg/reconciler/pipeline/dag/dag_test.go +++ b/pkg/reconciler/pipeline/dag/dag_test.go @@ -17,6 +17,7 @@ limitations under the License. package dag_test import ( + "strings" "testing" "github.com/google/go-cmp/cmp" @@ -479,7 +480,7 @@ func TestBuild_ConditionsParamsFromTaskResults(t *testing.T) { assertSameDAG(t, expectedDAG, g) } -func TestBuild_Invalid(t *testing.T) { +func TestBuild_InvalidDAG(t *testing.T) { a := v1beta1.PipelineTask{Name: "a"} xDependsOnA := v1beta1.PipelineTask{ Name: "x", @@ -539,37 +540,114 @@ func TestBuild_Invalid(t *testing.T) { Resources: []v1beta1.PipelineTaskInputResource{{From: []string{"none"}}}, }}, } + aRunsAfterE := v1beta1.PipelineTask{Name: "a", RunAfter: []string{"e"}} + bDependsOnA := v1beta1.PipelineTask{ + Name: "b", + Resources: &v1beta1.PipelineTaskResources{ + Inputs: []v1beta1.PipelineTaskInputResource{{From: []string{"a"}}}, + }, + } + cRunsAfterA := v1beta1.PipelineTask{ + Name: "c", + RunAfter: []string{"a"}, + } + dDependsOnBAndC := v1beta1.PipelineTask{ + Name: "d", + Resources: &v1beta1.PipelineTaskResources{ + Inputs: []v1beta1.PipelineTaskInputResource{{From: []string{"b", "c"}}}, + }, + } + eRunsAfterD := v1beta1.PipelineTask{ + Name: "e", + RunAfter: []string{"d"}, + } + fRunsAfterD := v1beta1.PipelineTask{ + Name: "f", + RunAfter: []string{"d"}, + } + gDependsOnF := v1beta1.PipelineTask{ + Name: "g", + Resources: &v1beta1.PipelineTaskResources{ + Inputs: []v1beta1.PipelineTaskInputResource{{From: []string{"f"}}}, + }, + } tcs := []struct { name string spec v1beta1.PipelineSpec + err string }{{ + // a + // | + // a ("a" depends on resource from "a") name: "self-link-from", spec: v1beta1.PipelineSpec{Tasks: []v1beta1.PipelineTask{selfLinkFrom}}, + err: "cycle detected", }, { + // a + // | + // a ("a" runAfter "a") name: "self-link-after", spec: v1beta1.PipelineSpec{Tasks: []v1beta1.PipelineTask{selfLinkAfter}}, + err: "cycle detected", }, { + // a (also "a" depends on resource from "z") + // | + // x ("x" depends on resource from "a") + // | + // z ("z" depends on resource from "x") name: "cycle-from", spec: v1beta1.PipelineSpec{Tasks: []v1beta1.PipelineTask{xDependsOnA, zDependsOnX, aDependsOnZ}}, + err: "cycle detected", }, { + // a (also "a" runAfter "z") + // | + // x ("x" runAfter "a") + // | + // z ("z" runAfter "x") name: "cycle-runAfter", spec: v1beta1.PipelineSpec{Tasks: []v1beta1.PipelineTask{xAfterA, zAfterX, aAfterZ}}, + err: "cycle detected", }, { + // a (also "a" depends on resource from "z") + // | + // x ("x" depends on resource from "a") + // | + // z ("z" runAfter "x") name: "cycle-both", spec: v1beta1.PipelineSpec{Tasks: []v1beta1.PipelineTask{xDependsOnA, zAfterX, aDependsOnZ}}, + err: "cycle detected", + }, { + // This test make sure we detect a cyclic branch in a DAG with multiple branches. + // The following DAG is having a cyclic branch with an additional dependency (a runAfter e) + // a + // / \ + // b c + // \ / + // d + // / \ + // e f + // | + // g + name: "multiple-branches-with-one-cyclic-branch", + spec: v1beta1.PipelineSpec{Tasks: []v1beta1.PipelineTask{aRunsAfterE, bDependsOnA, cRunsAfterA, dDependsOnBAndC, eRunsAfterD, fRunsAfterD, gDependsOnF}}, + err: "cycle detected", }, { name: "duplicate-tasks", spec: v1beta1.PipelineSpec{Tasks: []v1beta1.PipelineTask{a, a}}, + err: "duplicate pipeline task", }, { name: "invalid-task-name-from", spec: v1beta1.PipelineSpec{Tasks: []v1beta1.PipelineTask{invalidTaskFrom}}, + err: "wasn't present in Pipeline", }, { name: "invalid-task-name-after", spec: v1beta1.PipelineSpec{Tasks: []v1beta1.PipelineTask{invalidTaskAfter}}, + err: "wasn't present in Pipeline", }, { name: "invalid-task-name-from-conditional", spec: v1beta1.PipelineSpec{Tasks: []v1beta1.PipelineTask{invalidConditionalTask}}, + err: "wasn't present in Pipeline", }, } for _, tc := range tcs { @@ -578,7 +656,8 @@ func TestBuild_Invalid(t *testing.T) { ObjectMeta: metav1.ObjectMeta{Name: tc.name}, Spec: tc.spec, } - if _, err := dag.Build(v1beta1.PipelineTaskList(p.Spec.Tasks)); err == nil { + _, err := dag.Build(v1beta1.PipelineTaskList(p.Spec.Tasks)) + if err == nil || !strings.Contains(err.Error(), tc.err) { t.Errorf("expected to see an error for invalid DAG in pipeline %v but had none", tc.spec) } })