From fdc321701780e3de28edd90a65835478216d06df Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 22 Oct 2024 14:58:32 -0400 Subject: [PATCH 1/4] deposed nodes should all be GraphNodeDestroyers --- internal/terraform/node_resource_destroy_deposed.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/internal/terraform/node_resource_destroy_deposed.go b/internal/terraform/node_resource_destroy_deposed.go index aedd88267064..ce4ce490066f 100644 --- a/internal/terraform/node_resource_destroy_deposed.go +++ b/internal/terraform/node_resource_destroy_deposed.go @@ -61,8 +61,13 @@ var ( _ GraphNodeExecutable = (*NodePlanDeposedResourceInstanceObject)(nil) _ GraphNodeProviderConsumer = (*NodePlanDeposedResourceInstanceObject)(nil) _ GraphNodeProvisionerConsumer = (*NodePlanDeposedResourceInstanceObject)(nil) + _ GraphNodeDestroyer = (*NodePlanDeposedResourceInstanceObject)(nil) ) +func (n *NodePlanDeposedResourceInstanceObject) DestroyAddr() *addrs.AbsResourceInstance { + return &n.Addr +} + func (n *NodePlanDeposedResourceInstanceObject) Name() string { return fmt.Sprintf("%s (deposed %s)", n.ResourceInstanceAddr().String(), n.DeposedKey) } @@ -226,6 +231,7 @@ var ( _ GraphNodeExecutable = (*NodeDestroyDeposedResourceInstanceObject)(nil) _ GraphNodeProviderConsumer = (*NodeDestroyDeposedResourceInstanceObject)(nil) _ GraphNodeProvisionerConsumer = (*NodeDestroyDeposedResourceInstanceObject)(nil) + _ GraphNodeDestroyer = (*NodeDestroyDeposedResourceInstanceObject)(nil) ) func (n *NodeDestroyDeposedResourceInstanceObject) Name() string { @@ -344,12 +350,17 @@ var ( _ GraphNodeExecutable = (*NodeForgetDeposedResourceInstanceObject)(nil) _ GraphNodeProviderConsumer = (*NodeForgetDeposedResourceInstanceObject)(nil) _ GraphNodeProvisionerConsumer = (*NodeForgetDeposedResourceInstanceObject)(nil) + _ GraphNodeDestroyer = (*NodeForgetDeposedResourceInstanceObject)(nil) ) func (n *NodeForgetDeposedResourceInstanceObject) Name() string { return fmt.Sprintf("%s (forget deposed %s)", n.ResourceInstanceAddr(), n.DeposedKey) } +func (n *NodeForgetDeposedResourceInstanceObject) DestroyAddr() *addrs.AbsResourceInstance { + return &n.Addr +} + func (n *NodeForgetDeposedResourceInstanceObject) DeposedInstanceObjectKey() states.DeposedKey { return n.DeposedKey } From 1a0c7b908e0e6a6002c0a2c446d643cf4c04b125 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 22 Oct 2024 14:59:32 -0400 Subject: [PATCH 2/4] forget nodes are also GraphNodeDestroyers --- internal/terraform/node_resource_forget.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/internal/terraform/node_resource_forget.go b/internal/terraform/node_resource_forget.go index 21b9d14af932..8118ee2d3d5d 100644 --- a/internal/terraform/node_resource_forget.go +++ b/internal/terraform/node_resource_forget.go @@ -27,8 +27,13 @@ var ( _ GraphNodeExecutable = (*NodeForgetResourceInstance)(nil) _ GraphNodeProviderConsumer = (*NodeForgetResourceInstance)(nil) _ GraphNodeProvisionerConsumer = (*NodeForgetResourceInstance)(nil) + _ GraphNodeDestroyer = (*NodeForgetResourceInstance)(nil) ) +func (n *NodeForgetResourceInstance) DestroyAddr() *addrs.AbsResourceInstance { + return &n.Addr +} + func (n *NodeForgetResourceInstance) Name() string { return n.ResourceInstanceAddr().String() + " (forget)" } From 9f920be95f3997315880b52bffb38169798460e6 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 22 Oct 2024 15:00:03 -0400 Subject: [PATCH 3/4] correct NodePlanDestroyableResourceInstance display name --- internal/terraform/node_resource_plan_destroy.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/internal/terraform/node_resource_plan_destroy.go b/internal/terraform/node_resource_plan_destroy.go index c3c21b264917..0962ce8fa853 100644 --- a/internal/terraform/node_resource_plan_destroy.go +++ b/internal/terraform/node_resource_plan_destroy.go @@ -37,6 +37,10 @@ var ( _ GraphNodeProviderConsumer = (*NodePlanDestroyableResourceInstance)(nil) ) +func (n *NodePlanDestroyableResourceInstance) Name() string { + return n.NodeAbstractResource.Name() + " (destroy)" +} + // GraphNodeDestroyer func (n *NodePlanDestroyableResourceInstance) DestroyAddr() *addrs.AbsResourceInstance { addr := n.ResourceInstanceAddr() From c63d236d5c2329521dcc7b303306c4db139e9f7f Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 22 Oct 2024 15:00:15 -0400 Subject: [PATCH 4/4] refactor and simplify pruneUnusedNodesTransformer With the missing destroy node types sorted out, this transformer can now be designed around finding the nodes we need to keep for destroy operations, rather than the whack-a-mole game of detecting nodes we don't want to keep. When executing a destroy operation, we can never be certain that the state will contain all the data required to evaluate the entire configuration. This could be because of a previously failed apply operation, or external changes made out of band. This means that once we process the configuration to get the providers hooked up, we need to remove any nodes that correspond to configuration values which may not be available to evaluate. In a destroy operation we primarily need three categories of nodes to complete the plan and apply: - Nodes corresponding to values in the state - Providers required to destroy those values obtained from the state - Any node which a provider depends on, to ensure its configuration can be fully evaluated Other than that minimal set of nodes, the rest are no longer necessary and should be skipped to avoid errors which could block the apply operation. --- internal/terraform/context_apply2_test.go | 10 +- internal/terraform/context_plan2_test.go | 4 +- internal/terraform/transform_destroy_edge.go | 128 +++++-------------- 3 files changed, 36 insertions(+), 106 deletions(-) diff --git a/internal/terraform/context_apply2_test.go b/internal/terraform/context_apply2_test.go index 7947dd2646b9..7ff301fa3237 100644 --- a/internal/terraform/context_apply2_test.go +++ b/internal/terraform/context_apply2_test.go @@ -2357,21 +2357,13 @@ locals { // The local value should remain in the graph because the external // reference uses it. gotGraph := g.String() - wantGraph := ` - local.local_value (expand) -local.local_value (expand) - test_object.a (expand) -provider["registry.terraform.io/hashicorp/test"] + wantGraph := `provider["registry.terraform.io/hashicorp/test"] provider["registry.terraform.io/hashicorp/test"] (close) test_object.a (destroy) - test_object.a (expand) root - provider["registry.terraform.io/hashicorp/test"] (close) test_object.a (destroy) provider["registry.terraform.io/hashicorp/test"] -test_object.a (expand) - provider["registry.terraform.io/hashicorp/test"] ` if diff := cmp.Diff(wantGraph, gotGraph); diff != "" { t.Errorf("wrong graph\n%s", diff) diff --git a/internal/terraform/context_plan2_test.go b/internal/terraform/context_plan2_test.go index 704076d85bd8..86479c953a0a 100644 --- a/internal/terraform/context_plan2_test.go +++ b/internal/terraform/context_plan2_test.go @@ -5886,7 +5886,7 @@ resource "test_object" "a" { }, }) if diags.HasErrors() { - t.Errorf("expected no errors, but got %s", diags) + t.Fatalf("expected no errors, but got %s", diags.ErrWithWarnings()) } planResult := plan.Checks.GetObjectResult(addrs.AbsInputVariableInstance{ @@ -5896,7 +5896,7 @@ resource "test_object" "a" { Module: addrs.RootModuleInstance, }) - if planResult.Status != checks.StatusUnknown { + if planResult != nil && planResult.Status != checks.StatusUnknown { // checks should not have been evaluated, because the variable is not required for destroy. t.Errorf("expected checks to be pass but was %s", planResult.Status) } diff --git a/internal/terraform/transform_destroy_edge.go b/internal/terraform/transform_destroy_edge.go index 6c8e201a9b1d..af29e0fdb478 100644 --- a/internal/terraform/transform_destroy_edge.go +++ b/internal/terraform/transform_destroy_edge.go @@ -311,105 +311,43 @@ func (t *pruneUnusedNodesTransformer) Transform(g *Graph) error { return nil } - // We need a reverse depth first walk of modules, processing them in order - // from the leaf modules to the root. This allows us to remove unneeded - // dependencies from child modules, freeing up nodes in the parent module - // to also be removed. - - nodes := g.Vertices() - - for removed := true; removed; { - removed = false - - for i := 0; i < len(nodes); i++ { - // run this in a closure, so we can return early rather than - // dealing with complex looping and labels - func() { - n := nodes[i] - switch n := n.(type) { - - case graphNodeTemporaryValue: - // root module outputs indicate they are not temporary by - // returning false here. - if !n.temporaryValue() { - return - } - - // temporary values, which consist of variables, locals, - // and outputs, must be kept if anything refers to them. - for _, v := range g.UpEdges(n) { - // keep any value which is connected through a - // reference - if _, ok := v.(GraphNodeReferencer); ok { - return - } - } - - case graphNodeExpandsInstances: - // FIXME: Ephemeral resources have kind of broken this - // transformer. They work like temporary values in that they - // should not exist when there are no dependencies, but also - // share expansion nodes with all other resource modes. We - // can't use graphNodeTemporaryValue because then all - // resources will be caught by the previous case here. - if n, ok := n.(GraphNodeConfigResource); ok && n.ResourceAddr().Resource.Mode == addrs.EphemeralResourceMode { - return - } - - // Any nodes that expand instances are kept when their - // instances may need to be evaluated. - for _, v := range g.UpEdges(n) { - switch v.(type) { - case graphNodeExpandsInstances: - // Root module output values (which the following - // condition matches) are exempt because we know - // there is only ever exactly one instance of the - // root module, and so it's not actually important - // to expand it and so this lets us do a bit more - // pruning than we'd be able to do otherwise. - if tmp, ok := v.(graphNodeTemporaryValue); ok && !tmp.temporaryValue() { - continue - } - - // expanders can always depend on module expansion - // themselves - return - case GraphNodeResourceInstance: - // resource instances always depend on their - // resource node, which is an expander - return - } - } + // we need to track nodes to keep, because the dependency trees can overlap, + // so we can't just remove all dependencies of nodes we don't want. + keep := make(dag.Set) + + // Only keep destroyers, their providers, and anything the providers need + // for configuration. Since the destroyer should already be hooked up to the + // provider, keeping all the destroyer dependencies should suffice. + for _, n := range g.Vertices() { + // a special case of destroyer, is that by convention Terraform expects + // root outputs to be "destroyed", and the output node is what writes + // the nil state. A root module output currently identifies itself as a + // temporary value which is not temporary for that reason. + if tmp, ok := n.(graphNodeTemporaryValue); ok && !tmp.temporaryValue() { + log.Printf("[TRACE] pruneUnusedNodesTransformer: keeping root output %s", dag.VertexName(n)) + keep.Add(n) + continue + } - case GraphNodeProvider: - // Only keep providers for evaluation if they have - // resources to handle. - // The provider transformers removed most unused providers - // earlier, however there may be more to prune now based on - // targeting or a destroy with no related instances in the - // state. - if g.MatchDescendant(n, func(v dag.Vertex) bool { - if _, ok := v.(GraphNodeProviderConsumer); ok { - return true - } - return false - }) { - return - } + // from here we only search for managed resource destroy nodes + n, ok := n.(GraphNodeDestroyer) + if !ok { + continue + } - default: - return - } + log.Printf("[TRACE] pruneUnusedNodesTransformer: keeping destroy node %s", dag.VertexName(n)) + keep.Add(n) - log.Printf("[DEBUG] pruneUnusedNodes: %s is no longer needed, removing", dag.VertexName(n)) - g.Remove(n) - removed = true + for _, anc := range g.Ancestors(n) { + log.Printf("[TRACE] pruneUnusedNodesTransformer: keeping %s as dependency of %s", dag.VertexName(anc), dag.VertexName(n)) + keep.Add(anc) + } + } - // remove the node from our iteration as well - last := len(nodes) - 1 - nodes[i], nodes[last] = nodes[last], nodes[i] - nodes = nodes[:last] - }() + for _, n := range g.Vertices() { + if !keep.Include(n) { + log.Printf("[TRACE] pruneUnusedNodesTransformer: removing %s", dag.VertexName(n)) + g.Remove(n) } }