Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Instance state only destroys #24083

Merged
merged 17 commits into from
Feb 14, 2020
Merged

Instance state only destroys #24083

merged 17 commits into from
Feb 14, 2020

Conversation

jbardin
Copy link
Member

@jbardin jbardin commented Feb 12, 2020

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

@ghost ghost added the sdkv1 [PRs only] Marks changes that may potentially need to be ported to the plugi nSDK label Feb 12, 2020
@jbardin jbardin changed the title State only destroys Instance state only destroys Feb 12, 2020
@jbardin jbardin marked this pull request as ready for review February 12, 2020 21:13
@jbardin jbardin requested a review from a team February 13, 2020 19:11
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.
@jbardin jbardin force-pushed the jbardin/state-only-destroys branch from 57027dc to 099806c Compare February 13, 2020 20:44
@@ -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),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this change in this test? Seems suspicious.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RAD

Copy link
Contributor

@apparentlymart apparentlymart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
configs/parser_config_test.go Outdated Show resolved Hide resolved
configs/parser_config_test.go Outdated Show resolved Hide resolved
},
},
},
state := states.NewState()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yay for removing uses of MustShimLegacyState!

@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Show resolved Hide resolved
@jbardin jbardin merged commit ded207f into master Feb 14, 2020
@jbardin jbardin deleted the jbardin/state-only-destroys branch February 14, 2020 02:03
@okgolove
Copy link

When will it be included in release?

@ghost
Copy link

ghost commented Mar 20, 2020

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 ghost locked and limited conversation to collaborators Mar 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.