From 940e5c27d03c558d477d26cd98d5f03877de25bd Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 23 Apr 2024 15:52:58 -0400 Subject: [PATCH 1/2] optimize reducePreservingRelationships The reducePreservingRelationships method is strictly removing nodes and passing through the connecting edges. This does not take multiple rounds if the removal is done immediately, so we can make the process much simpler. --- internal/terraform/graph.go | 66 ++++++++++++++++--------------------- 1 file changed, 28 insertions(+), 38 deletions(-) diff --git a/internal/terraform/graph.go b/internal/terraform/graph.go index b4010874df36..63ceb3220311 100644 --- a/internal/terraform/graph.go +++ b/internal/terraform/graph.go @@ -285,14 +285,23 @@ func (g *Graph) ResourceGraph() addrs.DirectedGraph[addrs.ConfigResource] { // and then using that temporary graph to construct the final graph to // return. + log.Printf("[TRACE] ResourceGraph: copying source graph\n") tmpG := Graph{} tmpG.Subsume(&g.Graph) + log.Printf("[TRACE] ResourceGraph: reducing graph\n") tmpG.reducePreservingRelationships(func(n dag.Vertex) bool { _, ret := n.(GraphNodeConfigResource) return ret }) + log.Printf("[TRACE] ResourceGraph: TransitiveReduction\n") + + // The resulting graph could have many more edges now, but alternate paths + // are not a problem for the deferral system, so we may choose not to run + // this as it may be very time consuming. The reducePreservingRelationships + // method also doesn't add many (if any) redundant new edges to most graphs. tmpG.TransitiveReduction() + log.Printf("[TRACE] ResourceGraph: creating address graph\n") ret := addrs.NewDirectedGraph[addrs.ConfigResource]() for _, n := range tmpG.Vertices() { sourceR := n.(GraphNodeConfigResource) @@ -300,9 +309,11 @@ func (g *Graph) ResourceGraph() addrs.DirectedGraph[addrs.ConfigResource] { ret.Add(sourceAddr) for _, dn := range tmpG.DownEdges(n) { targetR := dn.(GraphNodeConfigResource) + ret.AddDependency(sourceAddr, targetR.ResourceAddr()) } } + log.Printf("[TRACE] ResourceGraph: completed with %d nodes\n", len(ret.AllNodes())) return ret } @@ -311,46 +322,25 @@ func (g *Graph) ResourceGraph() addrs.DirectedGraph[addrs.ConfigResource] { // edges to preserve the dependency relationships for all of the nodes // that still remain. func (g *Graph) reducePreservingRelationships(keepNode func(dag.Vertex) bool) { - // This is a naive algorithm for now. Maybe we'll improve it later. - - // We'll keep iterating as long as we find new edges to add because we - // might need to bridge across multiple nodes that we're going to remove - // in order to retain the relationships, and one iteration can only - // bridge a single node at a time. - changed := true - for changed { - changed = false - for _, n := range g.Vertices() { - if keepNode(n) { - continue - } - - // If we're not going to keep this node then we need to connect - // all of its dependents to all of its dependencies so that the - // ordering is still preserved for those nodes that remain. - // However, this will often generate more edges than are strictly - // required and so it could be productive to run a transitive - // reduction afterwards. - dependents := g.UpEdges(n) - dependencies := g.DownEdges(n) - for dependent := range dependents { - for dependency := range dependencies { - edge := dag.BasicEdge(dependent, dependency) - if !g.HasEdge(edge) { - g.Connect(edge) - changed = true - } - } - } + for _, n := range g.Vertices() { + if keepNode(n) { + continue } - } - // With all of the extra supporting edges in place, we can now safely - // remove the nodes we aren't going to keep without changing the - // relationships of the remaining nodes. - for _, n := range g.Vertices() { - if !keepNode(n) { - g.Remove(n) + // If we're not going to keep this node then we need to connect + // all of its dependents to all of its dependencies so that the + // ordering is still preserved for those nodes that remain. + // However, this will often generate more edges than are strictly + // required and so it could be productive to run a transitive + // reduction afterwards. + dependents := g.UpEdges(n) + dependencies := g.DownEdges(n) + for dependent := range dependents { + for dependency := range dependencies { + edge := dag.BasicEdge(dependent, dependency) + g.Connect(edge) + } } + g.Remove(n) } } From bfa6f7ab88a681a4be6acfa16641950ae961debc Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 23 Apr 2024 15:58:00 -0400 Subject: [PATCH 2/2] don't create the deferrals graph if not needed When deferrals are not being used, don't spend time building the deferrals graph. --- internal/terraform/context_plan2_test.go | 6 ++++++ internal/terraform/context_walk.go | 25 ++++++++++++++++++------ 2 files changed, 25 insertions(+), 6 deletions(-) diff --git a/internal/terraform/context_plan2_test.go b/internal/terraform/context_plan2_test.go index 8bcb85fdba16..9694ec4c84a1 100644 --- a/internal/terraform/context_plan2_test.go +++ b/internal/terraform/context_plan2_test.go @@ -4818,6 +4818,12 @@ func TestContext2Apply_externalDependencyDeferred(t *testing.T) { cfg := testModuleInline(t, map[string]string{ "main.tf": ` + // TEMP: unknown for_each currently requires an experiment opt-in. + // We should remove this block if the experiment gets stabilized. + terraform { + experiments = [unknown_instances] + } + resource "test" "a" { name = "a" } diff --git a/internal/terraform/context_walk.go b/internal/terraform/context_walk.go index ccdaf7bf7044..aedc6c9bd732 100644 --- a/internal/terraform/context_walk.go +++ b/internal/terraform/context_walk.go @@ -10,6 +10,7 @@ import ( "github.com/hashicorp/terraform/internal/addrs" "github.com/hashicorp/terraform/internal/checks" "github.com/hashicorp/terraform/internal/configs" + "github.com/hashicorp/terraform/internal/experiments" "github.com/hashicorp/terraform/internal/instances" "github.com/hashicorp/terraform/internal/moduletest/mocking" "github.com/hashicorp/terraform/internal/namedvals" @@ -165,12 +166,24 @@ func (c *Context) graphWalker(graph *Graph, operation walkOperation, opts *graph } } - // We'll produce a derived graph that only includes the static resource - // blocks, since we need that for deferral tracking. - resourceGraph := graph.ResourceGraph() - deferred := deferring.NewDeferred(resourceGraph) - if opts.ExternalDependencyDeferred { - deferred.SetExternalDependencyDeferred() + deferralsAllowed := false + opts.Config.DeepEach(func(c *configs.Config) { + if c.Module != nil && c.Module.ActiveExperiments.Has(experiments.UnknownInstances) { + deferralsAllowed = true + } + }) + + var deferred *deferring.Deferred + if deferralsAllowed { + // We'll produce a derived graph that only includes the static resource + // blocks, since we need that for deferral tracking. + resourceGraph := graph.ResourceGraph() + deferred = deferring.NewDeferred(resourceGraph) + if opts.ExternalDependencyDeferred { + deferred.SetExternalDependencyDeferred() + } + } else { + deferred = deferring.NewDeferred(addrs.NewDirectedGraph[addrs.ConfigResource]()) } return &ContextGraphWalker{