Use plan graph builder for destroy #31163

jbardin · 2022-05-31T18:20:32Z

This PR aims to allow the same components used in PlanGraphBuilder to create the full destroy plan, and eliminate the special case of the DestroyPlanGraph.

The destroy plan was originally envisioned to be completely "offline", where we could generate a simple Delete action for every resource in the state. It turns out that this is not possible, since in order to read each resource from the state, we must call UpgradeResourceState to ensure the schema matches the encoded state, requiring interaction with the provider aside from fetching the schema. Provider developers have also expressed the desire to be able to plan or validate destroy actions, which would also require providers to be correctly configured.

Because the simplistic DestroyPlanGraphBuilder was only intended to create Delete actions, it was not sufficient to ensure we had all the data required to configure a provider before calling UpgradeResourceState. The complete plan graph is where all the setup to properly evaluate the entire config is already done, and since a destroy plan is still a form of planning, we can extend the existing PlanGraphBuilder to accommodate a complete destroy operation rather than duplicating all the logic in the DestroyPlanGraphBuilder. A secondary reason for extending the PlanGraphBuilder (rather than duplicating the logic) is that the destroy graph is often forgotten when changing how plans work, which has led to numerous bugs we can avoid with a single plan codepath.

This does not yet fix the un-configured UpgradeResourceState calls reported in #30460, but does lay the foundation for being able to make that call. This also provides the necessary infrastructure to be able to address #30140, since that also requires a fully configured provider.

Aside from the above mentioned issues, another possibility from this point is to combine the separate refresh step into the destroy plan, removing the special case where we need 2 individual plans to complete a destroy operation. That was purposely left out of consideration for this PR to prevent it from growing too large, and it's not yet clear whether there are any blockers to that being possible.

Destroy nodes should never participate in references. These edges didn't come up before, because we weren't building a complete graph including all temporary values.

We want to use the normal plan graph for destroy, so we need to flag off configuration for that process.

All instances in state are being removed for destroy, so we can skip checking for orphans. Because we want to use the normal plan graph, we need to be able to still call this during destroy, so flag it off.

Rather than maintain a separate graph builder for destroy, use the normal plan graph with some extra options. Utilize the same pattern as the validate graph for now, where we take the normal plan graph builder and inject a new concrete function for the destroy nodes.

apparentlymart

This makes sense to me!

Aside from the very nitty nits inline, my inline comments are areas where I stumbled a bit trying to follow what was going on, and so my motivation there is just to share some ideas that might make this code easier to follow in future without having all of the mental context already loaded. They're all subjective, so I'll leave it up to your judgement what (if anything) to do about them.

apparentlymart · 2022-06-01T17:57:47Z

internal/terraform/context_apply2_test.go

+  type = map(string)
+  default = {
+    "a" = "first"
+	"b" = "second"


The nittiest nit: this line seems to have a tab, while the others have spaces.

thanks, my vim setup doesn't like switching contexts like that

FWIW, I've typically just used Go-style tab-based indentation for situations where we have configuration embedded in raw strings inside Go code, because it avoids this sort of editor fighting and isn't really important unless the test is actually testing something related to formatting.

So making this all be tabs would also have been an acceptable answer as far as I'm concerned; it was the inconsistency that bothered me. 😀

apparentlymart · 2022-06-01T18:02:59Z

internal/terraform/context_apply2_test.go

+	// TODO: extend this to ensure the otherProvider is always properly
+	// configured during the destroy plan


Heh... this comment represents exactly what I thought when I first read through this test and saw that it's only testing that the plan succeeds, not that it succeeded for the right reason.

Did you TODO this because there's something blocking us from actually configuring the provider right now? My first instinct here was that it should be possible to check otherProvider.ConfigureProviderResponse after the plan succeeds, but probably there's some extra complexity here I'm not understanding! If so, it would be nice to capture that in this comment too so we can see what needs to be true in order for us TODO what this comment says. 😀

yup, the provider still won't be configured yet, that will be enabled in a follow up PR. This was intended to be a drop-in replacement of the destroy plan graph with no change in behavior.

apparentlymart · 2022-06-01T18:20:22Z

internal/terraform/context_plan.go

@@ -574,7 +574,7 @@ func (c *Context) planGraph(config *configs.Config, prevRunState *states.State,
 		}).Build(addrs.RootModuleInstance)
 		return graph, walkPlan, diags
 	case plans.DestroyMode:
-		graph, diags := (&DestroyPlanGraphBuilder{
+		graph, diags := DestroyPlanGraphBuilder(&PlanGraphBuilder{


Would it be reasonable to make this be consistent with how we deal with RefreshOnlyMode above and have this instead "just" set a flag in the PlanGraphBuilder struct and let the Build method worry about the details?

I think I ended up leaving it in this odd shape where NormalMode and RefreshOnlyMode are largely the same just because the DestroyMode case was a drastically different shape, but if we normalize these to all just be conditional flags on the struct then we could potentially rework this to avoid duplicating the common fields for each case and only set skipPlanChanges / destroy in the non-normal modes.

Of course fine to leave this for another day if it seems too risky, since it's just some further cleanup after all.

This was following the pattern used by the validate graph, because I thought I would end up injecting more concrete functions that I ended using. I think as part of the follow up here we should get rid of both the validate and destroy builder entry points, and just make everything a plan ;)

apparentlymart · 2022-06-01T18:33:42Z

internal/terraform/transform_config.go

+	// destroyPlan indicated this is being called from a destroy plan.
+	destroyPlan bool


It feels a little weird to me that this flag essentially means to disable the behavior of this transformer entirely, but it's named as if it's going to modify the behavior in a more subtle way than that.

I guess being able to flag it off is probably easier than dynamically omitting it from the list of graph builders given that we currently just write those out as a slice constructor, but maybe it would read more clearly to just call this disable, ignoreConfig or similar instead, so that e.g. the caller will say ignoreConfig: b.destroy which (to me at least) reads as "ignore configuration if we are in destroy mode".

That makes sense to me. Adding conditional logic to the builder steps is quite cumbersome, and we managed to get all that out of the way a while back. The old flags were called destroy, but making them a bit more descriptive can definitely help.

apparentlymart · 2022-06-01T18:35:03Z

internal/terraform/transform_destroy_edge.go

@@ -4,9 +4,7 @@ import (
 	"log"

 	"github.com/hashicorp/terraform/internal/addrs"
-	"github.com/hashicorp/terraform/internal/states"



Another nitty nit: it isn't clear to me why this blank line was here in the first place, but in any event maybe now we can remove it along with the two lines that were around it. 😀

apparentlymart · 2022-06-01T18:36:00Z

internal/terraform/transform_orphan_resource.go

@@ -26,9 +26,17 @@ type OrphanResourceInstanceTransformer struct {
 	// Config is the root node in the configuration tree. We'll look up
 	// the appropriate note in this tree using the path in each node.
 	Config *configs.Config
+
+	// There are no orphans when doing a full destroy
+	destroyPlan bool


Same thought here as about ConfigTransformer above: can we make this flag have a name that describes more specifically what effect it has?

apparentlymart · 2022-06-01T18:37:39Z

internal/terraform/transform_output.go

@@ -21,7 +21,7 @@ type OutputTransformer struct {

 	// If this is a planned destroy, root outputs are still in the configuration
 	// so we need to record that we wish to remove them
-	Destroy bool
+	destroyPlan bool


Maybe forceDestroyAll bool to represent that this field specifies to destroy all outputs regardless of what action we might otherwise have planned for them? (and then maybe nodeExpandOutput itself has forceDestroy instead of just Destroy, for consistency?

github-actions · 2022-06-01T19:37:30Z

Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch.

github-actions · 2022-07-02T02:36:43Z

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

jbardin added 7 commits May 27, 2022 10:50

do not connect references _to_ destroyers either

b8e362d

Destroy nodes should never participate in references. These edges didn't come up before, because we weren't building a complete graph including all temporary values.

remove unused fields from DestroyEdgeTransformer

e3a6e1f

don't add config nodes during destroy plan

d6f0d1e

We want to use the normal plan graph for destroy, so we need to flag off configuration for that process.

don't add orphan nodes during destroy

6860596

All instances in state are being removed for destroy, so we can skip checking for orphans. Because we want to use the normal plan graph, we need to be able to still call this during destroy, so flag it off.

rename field to destroyPlan for consistency

77d1380

test for configured destroy plan provider

ec476af

jbardin requested a review from a team May 31, 2022 18:20

apparentlymart approved these changes Jun 1, 2022

View reviewed changes

minor cleanup from review

e36ee75

vercel bot deployed to Preview June 1, 2022 19:34 View deployment

jbardin merged commit 93ff272 into main Jun 1, 2022

jbardin deleted the jbardin/plan-destroy branch June 1, 2022 19:37

jbardin mentioned this pull request Jun 20, 2022

Use plan graph for importing resources #31283

Merged

github-actions bot locked as resolved and limited conversation to collaborators Jul 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use plan graph builder for destroy #31163

Use plan graph builder for destroy #31163

jbardin commented May 31, 2022 •

edited

apparentlymart left a comment •

edited

apparentlymart Jun 1, 2022

jbardin Jun 1, 2022

apparentlymart Jun 1, 2022

apparentlymart Jun 1, 2022

jbardin Jun 1, 2022

apparentlymart Jun 1, 2022

jbardin Jun 1, 2022

apparentlymart Jun 1, 2022

jbardin Jun 1, 2022

apparentlymart Jun 1, 2022

apparentlymart Jun 1, 2022

apparentlymart Jun 1, 2022

github-actions bot commented Jun 1, 2022

github-actions bot commented Jul 2, 2022

		// TODO: extend this to ensure the otherProvider is always properly
		// configured during the destroy plan

		// destroyPlan indicated this is being called from a destroy plan.
		destroyPlan bool

Use plan graph builder for destroy #31163

Use plan graph builder for destroy #31163

Conversation

jbardin commented May 31, 2022 • edited

apparentlymart left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jun 1, 2022

github-actions bot commented Jul 2, 2022

jbardin commented May 31, 2022 •

edited

apparentlymart left a comment •

edited