-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
terraform: destroy graph builder based on state #9527
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
It doesn't fully work so we want to wait until we think its ready before we start the shadowing.
This enables targeting to work properly on planning destroys
This is something that should be determined and done during an apply. It doesn't make a lot of sense that the plan is doing it (in its current form at least).
This is necessary to get the shadow working properly with the destroy graph since the destroy graph doesn't set this field but the end state is still the same.
This makes the new destroy nodes undestand data sourcs and call the correct apply function.
This enables the shadow graph since all tests pass! We also change the destroy node to check the resource type using the addr since that is always available and reliable. The configuration can be nil for orphans.
They're so similar we unify them, they only change in a select few places. This is very similar to the old graph but is still much simpler.
jbardin
approved these changes
Oct 26, 2016
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
locked and limited conversation to collaborators
Apr 21, 2020
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This introduces a new graph builder for the Destroy operation (
terraform destroy
) that builds a graph based on the state.This continues the trend of new graph builders and will be the last graph builder I'll propose into the next 0.7.x release. I only did this one as well since it was so similar to the apply builder (#9388).
NOTE: Same as the Apply builder, this doesn't remove any of the old code, so its purely additive. In the future when we remove the old logic, we should have a large deletion PR.
Why?
For higher level reasons see the apply graph why: #9388
For destroy in particular, the key new graph is in the plan phase. Planning a destroy is very, very simple:
-target
We don't and shouldn't even read most items in the configuration: lifecycle options (create before destroy), variables, outputs, module boundaries, etc. They don't matter for a pure destroy operation.
The previous graph would do most of those things and the result was that sometimes
terraform destroy
would have strange cycle errors and other issues that were completely unnecessary for what is mostly a simple operation. This new graph construction is much simpler and very hard for any of that to show up.Solution
The new graph is based on the state and doesn't even create nodes for things in the config (unless they're also in the state).
This results in a single primary improvement: changes to dependency ordering (interpolations or explicit
depends_on
) has zero effect onterraform destroy
. You could create an explicit cycle in the config andterraform destroy
would still work.Secondary improvements such as an easier to understand graph construction are also there but functionally aren't that important compared to the above.
Stability
Same as #9388, this feature is blocked behind a flag:
-Xnew-destroy
.You must call
terraform plan
orterraform destroy
with the-Xnew-destroy
flag to get the destruction graph constructed with the logic in this PR. As with #9388, a warning the user must confirm shows up before executing.In addition to this, this has been verified against the shadow graph where the real graph is the original and the shadow is this new destroy and all tests pass.
Guide for Review
There are minimal changes here (relative to #9388), so it should be much simpler to review:
terraform/graph_builder_destroy_plan.go
is the new graph builder.terraform/context.go
has changes to enable the new graph builder.terraform/graph_builder_apply.go
has changes to disable some transforms during destroy.And those are the primary changes.