CBD GraphNodeCreators cannot depend on GraphNodeDestroyers #23399
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
jbardin
force-pushed
the
jbardin/cbd-cycle
branch
from
45a720f to
5d6ad36
Compare
Since a create node cannot both depend on its destroy node AND be CreateBeforeDestroy, the same goes for its dependencies. While we do connect resources with dependency destroy nodes so that updates are ordered correctly, this ordering does not make sense in the CreateBeforeDestroy case. If resource node is CreateBeforeDestroy, we need to remove any direct dependencies from it to destroy nodes to prevent cycles. Since we don't know for certain if a crate node is going to be CreateBeforeDestroy at the time the edge is added in the graph, we add it unconditionally and prune it out later on. The pruning happens during the CBD transformer when the CBD destroy node reverses it's own destroy edge. The reason this works for detecting the original edge, is that dependencies of CBD resources are forced to be CBD themselves. This does have a false positive where the case of the original node is NOT CBD, but this can be taken care of later when we gather enough information in the graph to prevent the connection in the first place.
jbardin
force-pushed
the
jbardin/cbd-cycle
branch
from
5d6ad36 to
b2a01a1
Compare
|
Fixed the |
mildwonkey
reviewed
Nov 18, 2019
terraform/eval_state.go
Outdated
| for _, d := range state.Dependencies { | ||
| depMap[d.String()] = d | ||
| // We have dependencies in state, so we need to trust those for refresh. We | ||
| // won't can't out new dependencies until apply time. |
There was a problem hiding this comment.
sorry, can you clarify this comment?
EvalRefreshDependencies is used to update resource dependencies when they don't exist, allow broken or old states to be updated. While appending any newly found dependencies is tempting to have the largest set available, changes to the config could conflict with the prior dependencies causing cycles.
jbardin
force-pushed
the
jbardin/cbd-cycle
branch
from
b2a01a1 to
33bef7c
Compare
jbardin
changed the title
pselle
approved these changes
Nov 18, 2019
jchitel
added a commit
to jchitel/jakechitel.com
that referenced
this pull request
Nov 19, 2019
…interpolation-only" expressions. Additionally, there is a bug in this version of terraform that is causing a circular dependency when creating an updated launch configuration. PR here: hashicorp/terraform#23399 We won't be able to deploy until this is fixed.
|
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.
GraphNodeCreators are connected directly to their dependency's destroy nodes, so that any updates in the creator happen after the dependency is destroyed. This ordering isn't required for our configuration evaluation, however it is implied in the dependency contract with providers, and many times resources cannot be updated correctly until the create-destroy lifecycle of their dependencies is complete.
This however fails when the GraphNodeCreator itself is CreateBeforeDestroy, because the inversion of the create order introduces a cycle. For example, the natural graph edges for replacing
AandBwhenAdepends onBis (elidingB's create node and edges since they don't participate in the cycle):When
Abecomes CreateBeforeDestroy, the first edge is reversed, introducing a cycle between the 3 nodes.We can solve this for now by pruning out any edges going into a CreateBeforeDestroy node, coming from a GraphNodeCreator. This works because any dependency of a CreateBeforeDestroy node also becomes CreateBeforeDestroy itself.
While it may be more precise to not connect CreateBeforeDestroy nodes to destroy nodes in the first place, we don't have the information readily available at that time in the graph. The use case for a resource update that can't happen once the dependency replacement has been created would be hard to find, but we should strive for the most precise ordering whenever possible, and can work that information into the graph transformation at a later time.
This also updates EvalRefreshDependencies to not append to existing dependencies in the state. While appending any newly found dependencies is tempting in order to have the largest set available, changes to the config could conflict with the prior dependencies causing cycles.
Fixes #23374