create_before_destroy across modules #22937
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
ghost
added
the
sdkv1
hashibot
added
bug
core
v0.12
We can't correctly resolve the destroy ordering if all references haven't been assigned to each node.
When looking for dependencies to fix when handling create_before_destroy, we need to look past more than one edge, as dependencies may appear transitively through outputs and variables. Use Descendants rather than UpEdges. We have the full graph to use for the CBD transformation, so there's no longer any need to create a temporary graph, which may differ from the original.
The CBDEdgeTransformer tests worked on fake data structures, with a synthetic graph, and configs that didn't match. Update them to generate a more complete graph, with real node implementations, from real configs. The output graph is filtered down to instances, and the results still functionally match the original expected test results, with some minor additions due to using the real implementation.
Destroy-time references are not correctly or fully inverted when crossing module boundaries, causing cycle during apply.
Destroy nodes do not need to be connected to the resource (prepare state) node when adding them to the graph. Destroy nodes already have a complete state in the graph (which is being destroyed), any references will be added in the ReferenceTransformer, and the proper connection to the create node will be added in the DestroyEdgeTransformer. Under normal circumstances this makes no difference, as create and destroy nodes always have an dependency, so having the prepare state handled before both only linearizes the operation slightly in the normal destroy-then-create scenario. However if there is a dependency on a resource being replaced in another module, there will be a dependency between the destroy nodes in each module (to complete the destroy ordering), while the resource node will depend on the variable->output->resource chain. If both the destroy and create nodes depend on the resource node, there will be a cycle.
after the previous commit the cycle is broken, but we can't evaluate resource counts that depends on data sources being destroyed.
If a resource is only destroying instances, there is no reason to prepare the state and we can remove the Resource (prepare state) nodes. They normally have pose no issue, but if the instances are being destroyed along with their dependencies, the resource node may fail to evaluate due to the missing dependencies (since destroy happens in the reverse order). These failures were previously blocked by there being a cycle when the destroy nodes were directly attached to the resource nodes.
jbardin
force-pushed
the
jbardin/cbd-modules
branch
from
c086787 to
f766bb8
Compare
appilon
added a commit
to hashicorp/terraform-plugin-sdk
that referenced
this pull request
Nov 4, 2019
appilon
removed
the
sdkv1
|
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
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.
When re-ordering the actions to account for
create_before_destroy, all resource descendants must be taken into account.The current algorithm only checked immediate
UpEdges, which would miss any dependents found transitively through module outputs and variables. UseGraph.Descendantsto find all downstream resources.We can also re-order the CBD transformer to run later in the graph building process, so that all references are correctly connected at transformation time. This prevents us from having to maintain a separate synthetic graph builder (which contains its own config loader as well), by using the actual operational graph to find the necessary nodes.
Fixes #17735
Fixes #21871
This now also includes PR #22976, but rebasing lost the GH link to the PR on those commits 😞