From e86a357388798a9811443b0c5c020862570f98b9 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 17 Oct 2024 10:09:39 -0400 Subject: [PATCH 1/4] FirstAncestorsWith and MatchAncestor Add ancestor equivalents of FirstDescendantsWith and MatchDescendant --- internal/dag/dag.go | 50 +++++++++++++++++++++++++++++++++++++ internal/dag/dag_test.go | 53 +++++++++++++++++++++++++++++++++++++++- 2 files changed, 102 insertions(+), 1 deletion(-) diff --git a/internal/dag/dag.go b/internal/dag/dag.go index d95d40c9a386..dfb9ef95708e 100644 --- a/internal/dag/dag.go +++ b/internal/dag/dag.go @@ -51,6 +51,56 @@ func (g *AcyclicGraph) Ancestors(vs ...Vertex) Set { return s } +// FirstAncestorsWith returns a Set that includes every Vertex yielded by +// walking down from the provided starting Vertex v, and stopping each branch when +// match returns true. This will return the set of all first ancestors +// encountered which match some criteria. +func (g *AcyclicGraph) FirstAncestorsWith(v Vertex, match func(Vertex) bool) Set { + s := make(Set) + searchFunc := func(v Vertex, d int) error { + if match(v) { + s.Add(v) + return errStopWalkBranch + } + + return nil + } + + start := make(Set) + for _, dep := range g.downEdgesNoCopy(v) { + start.Add(dep) + } + + // our memoFunc doesn't return an error + g.DepthFirstWalk(start, searchFunc) + + return s +} + +// MatchAncestor returns true if the given match function returns true for any +// descendants of the given Vertex. +func (g *AcyclicGraph) MatchAncestor(v Vertex, match func(Vertex) bool) bool { + var ret bool + matchFunc := func(v Vertex, d int) error { + if match(v) { + ret = true + return errStopWalk + } + + return nil + } + + start := make(Set) + for _, dep := range g.downEdgesNoCopy(v) { + start.Add(dep) + } + + // our memoFunc doesn't return an error + g.DepthFirstWalk(start, matchFunc) + + return ret +} + // Descendants returns a Set that includes every Vertex yielded by walking up // from the provided starting Vertex v. func (g *AcyclicGraph) Descendants(v Vertex) Set { diff --git a/internal/dag/dag_test.go b/internal/dag/dag_test.go index 7936d476f12c..43c8c5d2c430 100644 --- a/internal/dag/dag_test.go +++ b/internal/dag/dag_test.go @@ -317,7 +317,7 @@ func TestAcyclicGraphFindDescendants(t *testing.T) { return v.(int) == 1 }) if !foundOne { - t.Fatal("did not match 6 in the graph") + t.Fatal("did not match 1 in the graph") } foundSix := g.MatchDescendant(6, func(v Vertex) bool { @@ -335,6 +335,57 @@ func TestAcyclicGraphFindDescendants(t *testing.T) { } } +func TestAcyclicGraphFindAncestors(t *testing.T) { + var g AcyclicGraph + g.Add(0) + g.Add(1) + g.Add(2) + g.Add(3) + g.Add(4) + g.Add(5) + g.Add(6) + g.Connect(BasicEdge(1, 0)) + g.Connect(BasicEdge(2, 1)) + g.Connect(BasicEdge(6, 2)) + g.Connect(BasicEdge(4, 3)) + g.Connect(BasicEdge(5, 4)) + g.Connect(BasicEdge(6, 5)) + + actual := g.FirstAncestorsWith(6, func(v Vertex) bool { + // looking for first odd ancestors + return v.(int)%2 != 0 + }) + + expected := make(Set) + expected.Add(1) + expected.Add(5) + + if expected.Intersection(actual).Len() != expected.Len() { + t.Fatalf("expected %#v, got %#v\n", expected, actual) + } + + foundOne := g.MatchAncestor(6, func(v Vertex) bool { + return v.(int) == 1 + }) + if !foundOne { + t.Fatal("did not match 1 in the graph") + } + + foundSix := g.MatchAncestor(6, func(v Vertex) bool { + return v.(int) == 6 + }) + if foundSix { + t.Fatal("6 should not be a descendant of itself") + } + + foundTen := g.MatchAncestor(6, func(v Vertex) bool { + return v.(int) == 10 + }) + if foundTen { + t.Fatal("10 is not in the graph at all") + } +} + func TestAcyclicGraphWalk(t *testing.T) { var g AcyclicGraph g.Add(1) From 2977dd87110c06e9cca5a8e777cbdec8f13923dc Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 17 Oct 2024 10:14:24 -0400 Subject: [PATCH 2/4] only record first resource dependencies In configurations with long chains of dependencies, we spend a lot of time recording redundant dependency edges, and writing all those dependencies in the state. Rather than record every possible dependency edge, only record the first managed resource in any lineage, such that the resulting graph is topologically equivalent having removed many transitive edges. For some configurations this can save considerable time and space when handling the state. --- internal/terraform/transform_reference.go | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/internal/terraform/transform_reference.go b/internal/terraform/transform_reference.go index be5d7eec0de8..a0e48c2645a4 100644 --- a/internal/terraform/transform_reference.go +++ b/internal/terraform/transform_reference.go @@ -257,25 +257,30 @@ func (t AttachDependenciesTransformer) Transform(g *Graph) error { // dedupe addrs when there's multiple instances involved, or // multiple paths in the un-reduced graph depMap := map[string]addrs.ConfigResource{} - for _, d := range g.Ancestors(v) { - var addr addrs.ConfigResource + // since we need to type-switch over the nodes anyway, we're going to + // insert the address directly into depMap and forget about the returned + // set. + g.FirstAncestorsWith(v, func(d dag.Vertex) bool { + var addr addrs.ConfigResource switch d := d.(type) { - case GraphNodeResourceInstance: - instAddr := d.ResourceInstanceAddr() + case GraphNodeCreator: + // most of the time we'll hit a GraphNodeConfigResource first since that represents the config structure, but + instAddr := d.CreateAddr() addr = instAddr.ContainingResource().Config() case GraphNodeConfigResource: addr = d.ResourceAddr() default: - continue + return false } if matchesSelf(addr) { - continue + return false } depMap[addr.String()] = addr - } + return true + }) deps := make([]addrs.ConfigResource, 0, len(depMap)) for _, d := range depMap { From 7ad61b5ceb4482f4a4f0534b1735036f7a963cf3 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 17 Oct 2024 10:57:58 -0400 Subject: [PATCH 3/4] more efficient depends_on Use the more efficient FirstAncestorsWith when connecting dependencies cause by `depends_on`. --- internal/terraform/transform_reference.go | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/internal/terraform/transform_reference.go b/internal/terraform/transform_reference.go index a0e48c2645a4..f279e873295a 100644 --- a/internal/terraform/transform_reference.go +++ b/internal/terraform/transform_reference.go @@ -386,10 +386,10 @@ func (m ReferenceMap) dependsOn(g *Graph, depender graphNodeDependsOn) ([]dag.Ve // upstream managed resource in order to check for a planned // change. if _, ok := rv.(GraphNodeConfigResource); !ok { - for _, v := range g.Ancestors(rv) { - if isDependableResource(v) { - res = append(res, v) - } + for _, v := range g.FirstAncestorsWith(rv, func(v dag.Vertex) bool { + return isDependableResource(v) + }) { + res = append(res, v) } } } @@ -459,8 +459,10 @@ func (m ReferenceMap) parentModuleDependsOn(g *Graph, depender graphNodeDependsO } } - for _, v := range g.Ancestors(deps...) { - if isDependableResource(v) { + for _, dep := range deps { + for _, v := range g.FirstAncestorsWith(dep, func(v dag.Vertex) bool { + return isDependableResource(v) + }) { res = append(res, v) } } From 146a52ab23cca205e4dce3490e5be8f3cd4d2561 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 17 Oct 2024 20:11:07 -0400 Subject: [PATCH 4/4] test for smaller state dependencies --- internal/terraform/context_apply2_test.go | 50 +++++++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/internal/terraform/context_apply2_test.go b/internal/terraform/context_apply2_test.go index 780f16ba53ef..7947dd2646b9 100644 --- a/internal/terraform/context_apply2_test.go +++ b/internal/terraform/context_apply2_test.go @@ -3582,3 +3582,53 @@ resource "test_instance" "obj" { t.Fatal("old instance not destroyed") } } + +// each resource only needs to record the first dependency in a chain +func TestContext2Apply_limitDependencies(t *testing.T) { + m := testModuleInline(t, map[string]string{ + "main.tf": ` +resource "test_object" "a" { +} + +resource "test_object" "b" { + test_string = test_object.a.test_string +} + +resource "test_object" "c" { + test_string = test_object.b.test_string +} +`, + }) + + p := simpleMockProvider() + + ctx := testContext2(t, &ContextOpts{ + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("test"): testProviderFuncFixed(p), + }, + }) + + plan, diags := ctx.Plan(m, states.NewState(), SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables))) + assertNoErrors(t, diags) + + state, diags := ctx.Apply(plan, m, nil) + assertNoErrors(t, diags) + + for _, res := range state.RootModule().Resources { + deps := res.Instances[addrs.NoKey].Current.Dependencies + switch res.Addr.Resource.Name { + case "a": + if len(deps) > 0 { + t.Error(res.Addr, "should have no dependencies") + } + case "b": + if len(deps) != 1 || deps[0].Resource.Name != "a" { + t.Error(res.Addr, "should only depend on 'a', got", deps) + } + case "c": + if len(deps) != 1 || deps[0].Resource.Name != "b" { + t.Error(res.Addr, "should only record a dependency of 'b', got", deps) + } + } + } +}