Skip to content

Commit a730024

Browse files
pritidesaipritidesai
committed
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.
1 parent 4348839 commit a730024

File tree

3 files changed

+133
-10
lines changed

3 files changed

+133
-10
lines changed

‎pkg/reconciler/pipeline/dag/dag.go‎

Lines changed: 4 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -126,28 +126,22 @@ func linkPipelineTasks(prev *Node, next *Node) error {
126126
return fmt.Errorf("cycle detected; task %q depends on itself", next.Task.HashKey())
127127
}
128128
// Check if we are adding cycles.
129-
visited := map[string]bool{prev.Task.HashKey(): true, next.Task.HashKey(): true}
130129
path := []string{next.Task.HashKey(), prev.Task.HashKey()}
131-
if err := visit(next.Task.HashKey(), prev.Prev, path, visited); err != nil {
130+
if err := lookForNode(prev.Prev, path, next.Task.HashKey()); err != nil {
132131
return fmt.Errorf("cycle detected: %w", err)
133132
}
134133
next.Prev = append(next.Prev, prev)
135134
prev.Next = append(prev.Next, next)
136135
return nil
137136
}
138137

139-
func visit(currentName string, nodes []*Node, path []string, visited map[string]bool) error {
140-
var sb strings.Builder
138+
func lookForNode(nodes []*Node, path []string, next string) error {
141139
for _, n := range nodes {
142140
path = append(path, n.Task.HashKey())
143-
if _, ok := visited[n.Task.HashKey()]; ok {
141+
if n.Task.HashKey() == next {
144142
return errors.New(getVisitedPath(path))
145143
}
146-
sb.WriteString(currentName)
147-
sb.WriteByte('.')
148-
sb.WriteString(n.Task.HashKey())
149-
visited[sb.String()] = true
150-
if err := visit(n.Task.HashKey(), n.Prev, path, visited); err != nil {
144+
if err := lookForNode(n.Prev, path, next); err != nil {
151145
return err
152146
}
153147
}

‎pkg/reconciler/pipeline/dag/dag_test.go‎

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ limitations under the License.
1717
package dag_test
1818

1919
import (
20+
"strings"
2021
"testing"
2122

2223
"github.com/google/go-cmp/cmp"
@@ -435,18 +436,39 @@ func TestBuild_Invalid(t *testing.T) {
435436
name string
436437
spec v1beta1.PipelineSpec
437438
}{{
439+
// a
440+
// |
441+
// a ("a" depends on resource from "a")
438442
name: "self-link-from",
439443
spec: v1beta1.PipelineSpec{Tasks: []v1beta1.PipelineTask{selfLinkFrom}},
440444
}, {
445+
// a
446+
// |
447+
// a ("a" runAfter "a")
441448
name: "self-link-after",
442449
spec: v1beta1.PipelineSpec{Tasks: []v1beta1.PipelineTask{selfLinkAfter}},
443450
}, {
451+
// a (also "a" depends on resource from "z")
452+
// |
453+
// x ("x" depends on resource from "a")
454+
// |
455+
// z ("z" depends on resource from "x")
444456
name: "cycle-from",
445457
spec: v1beta1.PipelineSpec{Tasks: []v1beta1.PipelineTask{xDependsOnA, zDependsOnX, aDependsOnZ}},
446458
}, {
459+
// a (also "a" runAfter "z")
460+
// |
461+
// x ("x" runAfter "a")
462+
// |
463+
// z ("z" runAfter "x")
447464
name: "cycle-runAfter",
448465
spec: v1beta1.PipelineSpec{Tasks: []v1beta1.PipelineTask{xAfterA, zAfterX, aAfterZ}},
449466
}, {
467+
// a (also "a" depends on resource from "z")
468+
// |
469+
// x ("x" depends on resource from "a")
470+
// |
471+
// z ("z" runAfter "x")
450472
name: "cycle-both",
451473
spec: v1beta1.PipelineSpec{Tasks: []v1beta1.PipelineTask{xDependsOnA, zAfterX, aDependsOnZ}},
452474
}, {
@@ -669,3 +691,56 @@ func TestBuild_ConditionsParamsFromTaskResults(t *testing.T) {
669691
}
670692
assertSameDAG(t, expectedDAG, g)
671693
}
694+
695+
// This test make sure we detect cycle when it exists.
696+
// This means we "visit" "a" twice, from two different paths.
697+
// And one of the paths is creating a cycle with "e -> a".
698+
// a
699+
// / \
700+
// b c
701+
// \ /
702+
// d
703+
// / \
704+
// e f
705+
// |
706+
// g
707+
func TestBuild_InvalidDAGWithCycle(t *testing.T) {
708+
a := v1beta1.PipelineTask{Name: "a", RunAfter: []string{"e"}}
709+
bDependsOnA := v1beta1.PipelineTask{
710+
Name: "b",
711+
Resources: &v1beta1.PipelineTaskResources{
712+
Inputs: []v1beta1.PipelineTaskInputResource{{From: []string{"a"}}},
713+
},
714+
}
715+
cRunsAfterA := v1beta1.PipelineTask{
716+
Name: "c",
717+
RunAfter: []string{"a"},
718+
}
719+
dDependsOnBAndC := v1beta1.PipelineTask{
720+
Name: "d",
721+
Resources: &v1beta1.PipelineTaskResources{
722+
Inputs: []v1beta1.PipelineTaskInputResource{{From: []string{"b", "c"}}},
723+
},
724+
}
725+
eRunsAfterD := v1beta1.PipelineTask{
726+
Name: "e",
727+
RunAfter: []string{"d"},
728+
}
729+
fRunsAfterD := v1beta1.PipelineTask{
730+
Name: "f",
731+
RunAfter: []string{"d"},
732+
}
733+
gDependsOnF := v1beta1.PipelineTask{
734+
Name: "g",
735+
Resources: &v1beta1.PipelineTaskResources{
736+
Inputs: []v1beta1.PipelineTaskInputResource{{From: []string{"f"}}},
737+
},
738+
}
739+
740+
tasks := v1beta1.PipelineTaskList{a, bDependsOnA, cRunsAfterA, dDependsOnBAndC, eRunsAfterD, fRunsAfterD, gDependsOnF}
741+
742+
_, err := dag.Build(tasks)
743+
if err == nil || !strings.Contains(err.Error(), "cycle detected") {
744+
t.Errorf("expected cycle error but got %v", err)
745+
}
746+
}

‎pkg/reconciler/pipeline/dag/dagv1beta1_test.go‎

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ package dag_test
2222
// dealing with v1beta1 in our code and we won't need 2 sets of tests.
2323

2424
import (
25+
"strings"
2526
"testing"
2627

2728
"github.com/google/go-cmp/cmp"
@@ -625,3 +626,56 @@ func TestBuild_ConditionsParamsFromTaskResults_v1beta1(t *testing.T) {
625626
}
626627
assertSameDAG(t, expectedDAG, g)
627628
}
629+
630+
// This test make sure we detect cycle when it exists.
631+
// This means we "visit" "a" twice, from two different paths.
632+
// And one of the paths is creating a cycle with "e -> a".
633+
// a
634+
// / \
635+
// b c
636+
// \ /
637+
// d
638+
// / \
639+
// e f
640+
// |
641+
// g
642+
func TestBuild_InvalidDAGWithCycle_v1beta1(t *testing.T) {
643+
a := v1beta1.PipelineTask{Name: "a", RunAfter: []string{"e"}}
644+
bDependsOnA := v1beta1.PipelineTask{
645+
Name: "b",
646+
Resources: &v1beta1.PipelineTaskResources{
647+
Inputs: []v1beta1.PipelineTaskInputResource{{From: []string{"a"}}},
648+
},
649+
}
650+
cRunsAfterA := v1beta1.PipelineTask{
651+
Name: "c",
652+
RunAfter: []string{"a"},
653+
}
654+
dDependsOnBAndC := v1beta1.PipelineTask{
655+
Name: "d",
656+
Resources: &v1beta1.PipelineTaskResources{
657+
Inputs: []v1beta1.PipelineTaskInputResource{{From: []string{"b", "c"}}},
658+
},
659+
}
660+
eRunsAfterD := v1beta1.PipelineTask{
661+
Name: "e",
662+
RunAfter: []string{"d"},
663+
}
664+
fRunsAfterD := v1beta1.PipelineTask{
665+
Name: "f",
666+
RunAfter: []string{"d"},
667+
}
668+
gDependsOnF := v1beta1.PipelineTask{
669+
Name: "g",
670+
Resources: &v1beta1.PipelineTaskResources{
671+
Inputs: []v1beta1.PipelineTaskInputResource{{From: []string{"f"}}},
672+
},
673+
}
674+
675+
tasks := v1beta1.PipelineTaskList{a, bDependsOnA, cRunsAfterA, dDependsOnBAndC, eRunsAfterD, fRunsAfterD, gDependsOnF}
676+
677+
_, err := dag.Build(tasks)
678+
if err == nil || !strings.Contains(err.Error(), "cycle detected") {
679+
t.Errorf("expected cycle error but got %v", err)
680+
}
681+
}

0 commit comments

Comments
 (0)