From 769f5bd7354bc8e325313b1693a215cbd56f9b93 Mon Sep 17 00:00:00 2001 From: Sean Sullivan Date: Mon, 23 Aug 2021 09:42:00 -0700 Subject: [PATCH] Expose cyclic dependency error --- pkg/apply/applier.go | 5 ++- pkg/apply/destroyer.go | 5 ++- pkg/apply/solver/solver.go | 20 +++++++++--- pkg/apply/solver/solver_test.go | 56 +++++++++++++++++++++++++++++--- pkg/object/graph/depends.go | 17 ++++++---- pkg/object/graph/depends_test.go | 37 +++++++++++++++++++-- pkg/object/graph/graph.go | 25 ++++++++++++-- 7 files changed, 144 insertions(+), 21 deletions(-) diff --git a/pkg/apply/applier.go b/pkg/apply/applier.go index b11fb18..58ab2c8 100644 --- a/pkg/apply/applier.go +++ b/pkg/apply/applier.go @@ -173,12 +173,15 @@ func (a *Applier) Run(ctx context.Context, invInfo inventory.InventoryInfo, obje }, } // Build the task queue by appending tasks in the proper order. - taskQueue := taskBuilder. + taskQueue, err := taskBuilder. AppendInvAddTask(invInfo, applyObjs, options.DryRunStrategy). AppendApplyWaitTasks(invInfo, applyObjs, opts). AppendPruneWaitTasks(pruneObjs, pruneFilters, opts). AppendInvSetTask(invInfo, options.DryRunStrategy). Build() + if err != nil { + handleError(eventChannel, err) + } // Send event to inform the caller about the resources that // will be applied/pruned. eventChannel <- event.Event{ diff --git a/pkg/apply/destroyer.go b/pkg/apply/destroyer.go index ca13aaa..b0b5b62 100644 --- a/pkg/apply/destroyer.go +++ b/pkg/apply/destroyer.go @@ -131,10 +131,13 @@ func (d *Destroyer) Run(inv inventory.InventoryInfo, options DestroyerOptions) < }, } // Build the ordered set of tasks to execute. - taskQueue := taskBuilder. + taskQueue, err := taskBuilder. AppendPruneWaitTasks(deleteObjs, deleteFilters, opts). AppendDeleteInvTask(inv, options.DryRunStrategy). Build() + if err != nil { + handleError(eventChannel, err) + } // Send event to inform the caller about the resources that // will be pruned. eventChannel <- event.Event{ diff --git a/pkg/apply/solver/solver.go b/pkg/apply/solver/solver.go index 1319ca7..c3974da 100644 --- a/pkg/apply/solver/solver.go +++ b/pkg/apply/solver/solver.go @@ -54,6 +54,7 @@ type TaskQueueBuilder struct { waitCounter int pruneCounter int tasks []taskrunner.Task + err error } type TaskQueue struct { @@ -92,10 +93,15 @@ type Options struct { } // Build returns the queue of tasks that have been created. -func (t *TaskQueueBuilder) Build() *TaskQueue { +// TODO(seans): Now that we're reporting errors, we probably +// want to move away from the Builder patter for the TaskBuilder. +func (t *TaskQueueBuilder) Build() (*TaskQueue, error) { + if t.err != nil { + return nil, t.err + } return &TaskQueue{ tasks: t.tasks, - } + }, nil } // AppendInvAddTask appends an inventory add task to the task queue. @@ -211,7 +217,10 @@ func (t *TaskQueueBuilder) AppendApplyWaitTasks(inv inventory.InventoryInfo, applyObjs []*unstructured.Unstructured, o Options) *TaskQueueBuilder { // Use the "depends-on" annotation to create a graph, ands sort the // objects to apply into sets using a topological sort. - applySets := graph.SortObjs(applyObjs) + applySets, err := graph.SortObjs(applyObjs) + if err != nil { + t.err = err + } addWaitTask, waitTimeout := waitTaskTimeout(o.DryRunStrategy.ClientOrServerDryRun(), len(applySets), o.ReconcileTimeout) for _, applySet := range applySets { @@ -232,7 +241,10 @@ func (t *TaskQueueBuilder) AppendPruneWaitTasks(pruneObjs []*unstructured.Unstru if o.Prune { // Use the "depends-on" annotation to create a graph, ands sort the // objects to prune into sets using a (reverse) topological sort. - pruneSets := graph.ReverseSortObjs(pruneObjs) + pruneSets, err := graph.ReverseSortObjs(pruneObjs) + if err != nil { + t.err = err + } addWaitTask, waitTimeout := waitTaskTimeout(o.DryRunStrategy.ClientOrServerDryRun(), len(pruneSets), o.ReconcileTimeout) for _, pruneSet := range pruneSets { diff --git a/pkg/apply/solver/solver_test.go b/pkg/apply/solver/solver_test.go index 8583d6f..7a288f3 100644 --- a/pkg/apply/solver/solver_test.go +++ b/pkg/apply/solver/solver_test.go @@ -121,10 +121,12 @@ func TestTaskQueueBuilder_AppendApplyWaitTasks(t *testing.T) { applyObjs []*unstructured.Unstructured options Options expectedTasks []taskrunner.Task + isError bool }{ "no resources, no tasks": { applyObjs: []*unstructured.Unstructured{}, expectedTasks: []taskrunner.Task{}, + isError: false, }, "single resource, one apply task": { applyObjs: []*unstructured.Unstructured{ @@ -138,6 +140,7 @@ func TestTaskQueueBuilder_AppendApplyWaitTasks(t *testing.T) { }, }, }, + isError: false, }, "multiple resources with reconcile timeout": { applyObjs: []*unstructured.Unstructured{ @@ -164,6 +167,7 @@ func TestTaskQueueBuilder_AppendApplyWaitTasks(t *testing.T) { taskrunner.AllCurrent, 1*time.Second, testutil.NewFakeRESTMapper()), }, + isError: false, }, "multiple resources with reconcile timeout and dryrun": { applyObjs: []*unstructured.Unstructured{ @@ -184,6 +188,7 @@ func TestTaskQueueBuilder_AppendApplyWaitTasks(t *testing.T) { }, }, }, + isError: false, }, "multiple resources with reconcile timeout and server-dryrun": { applyObjs: []*unstructured.Unstructured{ @@ -204,6 +209,7 @@ func TestTaskQueueBuilder_AppendApplyWaitTasks(t *testing.T) { }, }, }, + isError: false, }, "multiple resources including CRD": { applyObjs: []*unstructured.Unstructured{ @@ -241,6 +247,7 @@ func TestTaskQueueBuilder_AppendApplyWaitTasks(t *testing.T) { taskrunner.AllCurrent, 1*time.Second, testutil.NewFakeRESTMapper()), }, + isError: false, }, "no wait with CRDs if it is a dryrun": { applyObjs: []*unstructured.Unstructured{ @@ -267,6 +274,7 @@ func TestTaskQueueBuilder_AppendApplyWaitTasks(t *testing.T) { }, }, }, + isError: false, }, "resources in namespace creates multiple apply tasks": { applyObjs: []*unstructured.Unstructured{ @@ -304,6 +312,7 @@ func TestTaskQueueBuilder_AppendApplyWaitTasks(t *testing.T) { taskrunner.AllCurrent, 1*time.Second, testutil.NewFakeRESTMapper()), }, + isError: false, }, "deployment depends on secret creates multiple tasks": { applyObjs: []*unstructured.Unstructured{ @@ -339,6 +348,17 @@ func TestTaskQueueBuilder_AppendApplyWaitTasks(t *testing.T) { taskrunner.AllCurrent, 1*time.Second, testutil.NewFakeRESTMapper()), }, + isError: false, + }, + "cyclic dependency returns error": { + applyObjs: []*unstructured.Unstructured{ + testutil.Unstructured(t, resources["deployment"], + testutil.AddDependsOn(t, testutil.Unstructured(t, resources["secret"]))), + testutil.Unstructured(t, resources["secret"], + testutil.AddDependsOn(t, testutil.Unstructured(t, resources["deployment"]))), + }, + expectedTasks: []taskrunner.Task{}, + isError: true, }, } @@ -351,8 +371,12 @@ func TestTaskQueueBuilder_AppendApplyWaitTasks(t *testing.T) { Mapper: testutil.NewFakeRESTMapper(), InvClient: fakeInvClient, } - tq := tqb.AppendApplyWaitTasks(localInv, tc.applyObjs, tc.options).Build() - + tq, err := tqb.AppendApplyWaitTasks(localInv, tc.applyObjs, tc.options).Build() + if tc.isError { + assert.NotNil(t, err, "expected error, but received none") + return + } + assert.Nil(t, err, "unexpected error received") assert.Equal(t, len(tc.expectedTasks), len(tq.tasks)) for i, expTask := range tc.expectedTasks { actualTask := tq.tasks[i] @@ -385,11 +409,13 @@ func TestTaskQueueBuilder_AppendPruneWaitTasks(t *testing.T) { pruneObjs []*unstructured.Unstructured options Options expectedTasks []taskrunner.Task + isError bool }{ "no resources, no tasks": { pruneObjs: []*unstructured.Unstructured{}, options: Options{Prune: true}, expectedTasks: []taskrunner.Task{}, + isError: false, }, "single resource, one prune task": { pruneObjs: []*unstructured.Unstructured{ @@ -404,6 +430,7 @@ func TestTaskQueueBuilder_AppendPruneWaitTasks(t *testing.T) { }, }, }, + isError: false, }, "multiple resources, one prune task": { pruneObjs: []*unstructured.Unstructured{ @@ -420,6 +447,7 @@ func TestTaskQueueBuilder_AppendPruneWaitTasks(t *testing.T) { }, }, }, + isError: false, }, "dependent resources, two prune tasks, two wait tasks": { pruneObjs: []*unstructured.Unstructured{ @@ -457,6 +485,7 @@ func TestTaskQueueBuilder_AppendPruneWaitTasks(t *testing.T) { taskrunner.AllCurrent, 1*time.Second, testutil.NewFakeRESTMapper()), }, + isError: false, }, "multiple resources with prune timeout and server-dryrun": { pruneObjs: []*unstructured.Unstructured{ @@ -478,6 +507,7 @@ func TestTaskQueueBuilder_AppendPruneWaitTasks(t *testing.T) { }, }, }, + isError: false, }, "multiple resources including CRD": { pruneObjs: []*unstructured.Unstructured{ @@ -517,6 +547,7 @@ func TestTaskQueueBuilder_AppendPruneWaitTasks(t *testing.T) { taskrunner.AllCurrent, 1*time.Second, testutil.NewFakeRESTMapper()), }, + isError: false, }, "no wait with CRDs if it is a dryrun": { pruneObjs: []*unstructured.Unstructured{ @@ -544,6 +575,7 @@ func TestTaskQueueBuilder_AppendPruneWaitTasks(t *testing.T) { }, }, }, + isError: false, }, "resources in namespace creates multiple apply tasks": { pruneObjs: []*unstructured.Unstructured{ @@ -582,6 +614,18 @@ func TestTaskQueueBuilder_AppendPruneWaitTasks(t *testing.T) { taskrunner.AllCurrent, 1*time.Second, testutil.NewFakeRESTMapper()), }, + isError: false, + }, + "cyclic dependency returns error": { + pruneObjs: []*unstructured.Unstructured{ + testutil.Unstructured(t, resources["deployment"], + testutil.AddDependsOn(t, testutil.Unstructured(t, resources["secret"]))), + testutil.Unstructured(t, resources["secret"], + testutil.AddDependsOn(t, testutil.Unstructured(t, resources["deployment"]))), + }, + options: Options{Prune: true}, + expectedTasks: []taskrunner.Task{}, + isError: true, }, } @@ -595,8 +639,12 @@ func TestTaskQueueBuilder_AppendPruneWaitTasks(t *testing.T) { InvClient: fakeInvClient, } emptyPruneFilters := []filter.ValidationFilter{} - tq := tqb.AppendPruneWaitTasks(tc.pruneObjs, emptyPruneFilters, tc.options).Build() - + tq, err := tqb.AppendPruneWaitTasks(tc.pruneObjs, emptyPruneFilters, tc.options).Build() + if tc.isError { + assert.NotNil(t, err, "expected error, but received none") + return + } + assert.Nil(t, err, "unexpected error received") assert.Equal(t, len(tc.expectedTasks), len(tq.tasks)) for i, expTask := range tc.expectedTasks { actualTask := tq.tasks[i] diff --git a/pkg/object/graph/depends.go b/pkg/object/graph/depends.go index a2cab17..2cc4154 100644 --- a/pkg/object/graph/depends.go +++ b/pkg/object/graph/depends.go @@ -16,9 +16,9 @@ import ( // Each of the objects in an apply set is applied together. The order of // the returned applied sets is a topological ordering of the sets to apply. // Returns an single empty apply set if there are no objects to apply. -func SortObjs(objs []*unstructured.Unstructured) [][]*unstructured.Unstructured { +func SortObjs(objs []*unstructured.Unstructured) ([][]*unstructured.Unstructured, error) { if len(objs) == 0 { - return [][]*unstructured.Unstructured{} + return [][]*unstructured.Unstructured{}, nil } // Create the graph, and build a map of object metadata to the object (Unstructured). g := New() @@ -35,7 +35,7 @@ func SortObjs(objs []*unstructured.Unstructured) [][]*unstructured.Unstructured objSets := [][]*unstructured.Unstructured{} sortedObjSets, err := g.Sort() if err != nil { - return objSets + return [][]*unstructured.Unstructured{}, err } // Map the object metadata back to the sorted sets of unstructured objects. for _, objSet := range sortedObjSets { @@ -49,18 +49,21 @@ func SortObjs(objs []*unstructured.Unstructured) [][]*unstructured.Unstructured } objSets = append(objSets, currentSet) } - return objSets + return objSets, nil } // ReverseSortObjs is the same as SortObjs but using reverse ordering. -func ReverseSortObjs(objs []*unstructured.Unstructured) [][]*unstructured.Unstructured { +func ReverseSortObjs(objs []*unstructured.Unstructured) ([][]*unstructured.Unstructured, error) { // Sorted objects using normal ordering. - s := SortObjs(objs) + s, err := SortObjs(objs) + if err != nil { + return [][]*unstructured.Unstructured{}, err + } // Reverse the ordering of the object sets using swaps. for i, j := 0, len(s)-1; i < j; i, j = i+1, j-1 { s[i], s[j] = s[j], s[i] } - return s + return s, nil } // addExplicitEdges updates the graph with edges from objects diff --git a/pkg/object/graph/depends_test.go b/pkg/object/graph/depends_test.go index d36742a..e66f7cb 100644 --- a/pkg/object/graph/depends_test.go +++ b/pkg/object/graph/depends_test.go @@ -6,6 +6,7 @@ package graph import ( "testing" + "github.com/stretchr/testify/assert" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "sigs.k8s.io/cli-utils/pkg/object" "sigs.k8s.io/cli-utils/pkg/testutil" @@ -235,11 +236,38 @@ func TestSortObjs(t *testing.T) { }, isError: false, }, + "two objects depends on each other is cyclic dependency": { + objs: []*unstructured.Unstructured{ + testutil.Unstructured(t, resources["deployment"], + testutil.AddDependsOn(t, testutil.Unstructured(t, resources["secret"]))), + testutil.Unstructured(t, resources["secret"], + testutil.AddDependsOn(t, testutil.Unstructured(t, resources["deployment"]))), + }, + expected: [][]*unstructured.Unstructured{}, + isError: true, + }, + "three objects in cyclic dependency": { + objs: []*unstructured.Unstructured{ + testutil.Unstructured(t, resources["deployment"], + testutil.AddDependsOn(t, testutil.Unstructured(t, resources["secret"]))), + testutil.Unstructured(t, resources["secret"], + testutil.AddDependsOn(t, testutil.Unstructured(t, resources["pod"]))), + testutil.Unstructured(t, resources["pod"], + testutil.AddDependsOn(t, testutil.Unstructured(t, resources["deployment"]))), + }, + expected: [][]*unstructured.Unstructured{}, + isError: true, + }, } for tn, tc := range testCases { t.Run(tn, func(t *testing.T) { - actual := SortObjs(tc.objs) + actual, err := SortObjs(tc.objs) + if tc.isError { + assert.NotNil(t, err, "expected error, but received none") + return + } + assert.Nil(t, err, "unexpected error received") verifyObjSets(t, tc.expected, actual) }) } @@ -328,7 +356,12 @@ func TestReverseSortObjs(t *testing.T) { for tn, tc := range testCases { t.Run(tn, func(t *testing.T) { - actual := ReverseSortObjs(tc.objs) + actual, err := ReverseSortObjs(tc.objs) + if tc.isError { + assert.NotNil(t, err, "expected error, but received none") + return + } + assert.Nil(t, err, "unexpected error received") verifyObjSets(t, tc.expected, actual) }) } diff --git a/pkg/object/graph/graph.go b/pkg/object/graph/graph.go index eb7b59f..08ec070 100644 --- a/pkg/object/graph/graph.go +++ b/pkg/object/graph/graph.go @@ -7,6 +7,7 @@ package graph import ( + "bytes" "fmt" "sigs.k8s.io/cli-utils/pkg/object" @@ -129,9 +130,12 @@ func (g *Graph) Sort() ([][]object.ObjMetadata, error) { leafVertices = append(leafVertices, v) } } - // No leaf vertices means cycle in the directed graph. + // No leaf vertices means cycle in the directed graph, + // where remaining edges define the cycle. if len(leafVertices) == 0 { - return sorted, fmt.Errorf("cycle in directed graph") + return [][]object.ObjMetadata{}, CyclicDependencyError{ + Edges: g.GetEdges(), + } } // Remove all edges to leaf vertices. for _, v := range leafVertices { @@ -141,3 +145,20 @@ func (g *Graph) Sort() ([][]object.ObjMetadata, error) { } return sorted, nil } + +// CyclicDependencyError when directed acyclic graph contains a cycle. +// The cycle makes it impossible to topological sort. +type CyclicDependencyError struct { + Edges []Edge +} + +func (cde CyclicDependencyError) Error() string { + var errorBuf bytes.Buffer + errorBuf.WriteString("cyclic dependency") + for _, edge := range cde.Edges { + from := fmt.Sprintf("%s/%s", edge.From.Namespace, edge.From.Name) + to := fmt.Sprintf("%s/%s", edge.To.Namespace, edge.To.Name) + errorBuf.WriteString(fmt.Sprintf("\n\t%s -> %s", from, to)) + } + return errorBuf.String() +}