Instance state only destroys #24083

jbardin · 2020-02-12T01:25:02Z

This PR completes the implementation of resource destruction only using the resource's state (configuration is still required for providers). By making sure destroy provisioners cannot reference other resources (more info in #23559), there is no need to connect resources to other values during destroy, preventing numerous cycle issues.

This also completes the implementation of storing the destroy references in the state (more info in #23252). The NodeAbstractResourceInstance.References implementation can now be simplified because there will be no more depends_on references from the state during destroy. This also means we no longer need to search again for destroy dependencies during apply, since we stored them already, removing an extra graph-building and traversal step.

Finally we greatly simplify the CBDEdgeTransformer, because we now know that the resource destroy nodes can only depend on other resource destroy nodes and can easily reverse the required GraphNodeCreator edges.

Closes #14548
Closes #16237
Closes #17944
Closes #18026
Closes #18088
Closes #18356
Closes #21610
Closes #23196
Closes #23365
Closes #23365
Closes #23875
Closes #23957

Destroy nodes only require their own state to evaluate. Do not connect any of their references in the graph.

The requires destroy dependencies are what is stored in the state, and have already been connected. There is no need to search further, since new nodes from the config may interfere with the original destroy ordering.

This special edge type is no longer used. While we still have the option of encoding more meaning into the edged themselves, having one special edge type used only in one specific case was easily overlooked, as dag.BasicEdge is assumed in all other cases.

Start by removing the DestroyEdge type altogether. This is only used to detect the natural edge between a resource's create and destroy nodes, but that's not necessary for any transformations. The custom edge type also interferes with normal graph manipulations, because you can't delete an arbitrary edge without knowing the type, so deletion of the edge based only on the endpoints is often done incorrectly. The dag package itself does this incorrectly in TransitiveReduction, which always assumes the BasicEdge type. Now that inter-resource destroy dependencies are already connected in the DestroyEdgeTransformer (from the stored deps in state), there's no need to search out all dependant resources in the CBD transformation, as they should all be connected. This makes the CBD transformation rule quite simple: reverse any edges from create nodes.

The tests require working node implementations with real state.

The AttachStateTransformer was never run in the destroy plan. This means that resource without configuration that used a non-default provider would not be connected to the correct provider for the plan. The test that was attempting to catch this only worked because the temporary graph used in the DestroyEdgeTransformer would add the state and detect some issues.

Remove all the destroy provisioner tests that are testing what is no longer allowed. Add missing state dependencies to remaining tests that require it.

since destroy nodes are no longer connected to values, there's no need to try and wrangle their edges to prevent cycles during destroy.

We no longer need special cases for most things during a full destroy, so remove those from the graph transformations. The only remaining cases are: - remove the root outputs, so destroy ends up with a clean state - reverse the target deps when targeting a destroy.

pselle · 2020-02-13T21:08:34Z

command/apply_destroy_test.go

@@ -195,7 +195,7 @@ func TestApply_destroyTargeted(t *testing.T) {
 				Mode: addrs.ManagedResourceMode,
 				Type: "test_instance",
 				Name: "foo",
-			}.Instance(addrs.NoKey).Absolute(addrs.RootModuleInstance),
+			}.Instance(addrs.IntKey(0)).Absolute(addrs.RootModuleInstance),


Should this change in this test? Seems suspicious.

The original test didn't match because the instance has count = 3.

pselle · 2020-02-13T21:09:07Z

configs/provisioner.go

@@ -147,8 +147,8 @@ func onlySelfRefs(body hcl.Body) hcl.Diagnostics {

 			if !valid {
 				diags = append(diags, &hcl.Diagnostic{
-					Severity: hcl.DiagWarning,
-					Summary:  "External references from destroy provisioners are deprecated",
+					Severity: hcl.DiagError,


pselle · 2020-02-13T21:09:39Z

terraform/node_resource_abstract.go

@@ -241,46 +240,6 @@ func (n *NodeAbstractResourceInstance) References() []*addrs.Reference {
 		return n.NodeAbstractResource.References()
 	}

-	// FIXME: remove once the deprecated DependsOn values have been removed from state


apparentlymart

My inline feedback is just copy-pasta comment nits. This looks great otherwise... nice to see how much simpler things can be now.

configs/parser_config_test.go

apparentlymart · 2020-02-13T22:11:48Z

terraform/context_apply_test.go

-					},
-				},
-			},
+	state := states.NewState()


Yay for removing uses of MustShimLegacyState!

apparentlymart · 2020-02-13T22:14:43Z

terraform/transform_destroy_cbd.go

@@ -134,25 +134,16 @@ type CBDEdgeTransformer struct {
 	// obtain schema information from providers and provisioners so we can
 	// properly resolve implicit dependencies.
 	Schemas *Schemas
-
-	// If the operation is a simple destroy, no transformation is done.
-	Destroy bool


This was the only remaining thing that was different between terraform plan -destroy and terraform destroy, right? If so, awesome!

terraform/graph_builder_apply.go

okgolove · 2020-03-19T16:23:44Z

When will it be included in release?

ghost · 2020-03-20T01:50:23Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

ghost added the sdkv1 [PRs only] Marks changes that may potentially need to be ported to the plugi nSDK label Feb 12, 2020

jbardin mentioned this pull request Feb 12, 2020

Add CreateBeforeDestroy to instance state #24084

Merged

jbardin changed the title ~~State only destroys~~ Instance state only destroys Feb 12, 2020

jbardin marked this pull request as ready for review February 12, 2020 21:13

jbardin requested a review from a team February 13, 2020 19:11

jbardin added the breaking-change label Feb 13, 2020

jbardin added 15 commits February 13, 2020 15:42

do not connect references from destroy nodes

45f2a61

Destroy nodes only require their own state to evaluate. Do not connect any of their references in the graph.

do not attempt to find more destroy dependencies

8b5522a

The requires destroy dependencies are what is stored in the state, and have already been connected. There is no need to search further, since new nodes from the config may interfere with the original destroy ordering.

remove DestroyEdge

451190a

This special edge type is no longer used. While we still have the option of encoding more meaning into the edged themselves, having one special edge type used only in one specific case was easily overlooked, as dag.BasicEdge is assumed in all other cases.

convert destroy provisioner warnings to errors

84d1f5c

fix DestroyEdgeTransformer tests

681d197

The tests require working node implementations with real state.

new deps are more precise

4a41af0

add state where it's now needed for tests

a0ba481

remove invalid destroy provisioner tests

a4bc91a

Remove all the destroy provisioner tests that are testing what is no longer allowed. Add missing state dependencies to remaining tests that require it.

add missing deps to targeted destroy test

dc8cdd2

no longer need DestroyValueReferenceTransformer

ca5b0e6

since destroy nodes are no longer connected to values, there's no need to try and wrangle their edges to prevent cycles during destroy.

remove old references code from abstract resource

8c5853e

fixup LocalProviderConfig literal

099806c

jbardin force-pushed the jbardin/state-only-destroys branch from 57027dc to 099806c Compare February 13, 2020 20:44

fixup provider types in new tests

b4f06c2

pselle reviewed Feb 13, 2020

View reviewed changes

apparentlymart approved these changes Feb 13, 2020

View reviewed changes

fix comment text

37e60cd

jbardin merged commit ded207f into master Feb 14, 2020

jbardin deleted the jbardin/state-only-destroys branch February 14, 2020 02:03

schmichri mentioned this pull request Mar 9, 2020

Allow destroy-time provisioners to access variables #23679

Open

ghost locked and limited conversation to collaborators Mar 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Instance state only destroys #24083

Instance state only destroys #24083

jbardin commented Feb 12, 2020 •

edited

Loading

pselle Feb 13, 2020

jbardin Feb 13, 2020

pselle Feb 13, 2020

pselle Feb 13, 2020

apparentlymart left a comment

apparentlymart Feb 13, 2020

apparentlymart Feb 13, 2020

okgolove commented Mar 19, 2020

ghost commented Mar 20, 2020

Instance state only destroys #24083

Instance state only destroys #24083

Conversation

jbardin commented Feb 12, 2020 • edited Loading

pselle Feb 13, 2020

Choose a reason for hiding this comment

jbardin Feb 13, 2020

Choose a reason for hiding this comment

pselle Feb 13, 2020

Choose a reason for hiding this comment

pselle Feb 13, 2020

Choose a reason for hiding this comment

apparentlymart left a comment

Choose a reason for hiding this comment

apparentlymart Feb 13, 2020

Choose a reason for hiding this comment

apparentlymart Feb 13, 2020

Choose a reason for hiding this comment

okgolove commented Mar 19, 2020

ghost commented Mar 20, 2020

jbardin commented Feb 12, 2020 •

edited

Loading