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

Remove some destroy cycles #22976

Merged
merged 4 commits into from Oct 24, 2019
Merged

Remove some destroy cycles #22976

merged 4 commits into from Oct 24, 2019

Conversation

@jbardin
Copy link
Member

jbardin commented Oct 2, 2019

This PR removes extra connections from the graph which are unneeded, however only cause problems in certain destroy scenarios. This solves a number of common cycles issues, while simplifying the graph itself. It does not solve all cycles, as destroy nodes still need their references evaluated in order for destroy-provisioners to function.


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 a direct 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.


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.


This depends on jbardin/cbd-modules, and can be merged into that first, or re-targeted to later to master.

Fixes #22502
Does not close #21662, but does fix some of the in-line examples that have been provided.

jbardin added 4 commits Sep 27, 2019
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.
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.
after the previous commit the cycle is broken, but we can't evaluate
resource counts that depends on data sources being destroyed.
@jbardin jbardin requested a review from hashicorp/terraform-core Oct 2, 2019
@hashibot hashibot bot added the sdkv1 label Oct 2, 2019
@@ -10527,3 +10527,225 @@ func TestContext2Apply_issue19908(t *testing.T) {
t.Errorf("test.foo attributes JSON doesn't contain %s after apply\ngot: %s", want, got)
}
}

func TestContext2Apply_moduleReplaceCycle(t *testing.T) {
for _, mode := range []string{"normal", "cbd"} {

This comment has been minimized.

Copy link
@pselle

pselle Oct 2, 2019

Member

huh! interesting approach to help with the super-bloat of this test file. since this string is only used for the switch, I feel like it would reduce my cognitive dissonance to see "create before destroy" vs cbd (fine in the file name below) just so I don't have to take that extra brain cycle not to think of hemp.

@pselle pselle requested a review from hashicorp/terraform-core Oct 2, 2019
@pselle
pselle approved these changes Oct 24, 2019
Copy link
Member

pselle left a comment

+1 from me

@jbardin

This comment has been minimized.

Copy link
Member Author

jbardin commented Oct 24, 2019

Merging into the other PR branch first, rather than rebasing

@jbardin jbardin merged commit c086787 into jbardin/cbd-modules Oct 24, 2019
1 check passed
1 check passed
license/cla Contributor License Agreement is signed.
Details
@jbardin jbardin deleted the jbardin/destroy-cycles branch Oct 24, 2019
@jbardin jbardin restored the jbardin/destroy-cycles branch Oct 24, 2019
@jbardin jbardin deleted the jbardin/destroy-cycles branch Oct 24, 2019
@appilon appilon removed the sdkv1 label Oct 29, 2019
@hashibot

This comment has been minimized.

Copy link

hashibot bot commented Nov 24, 2019

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.

@hashibot hashibot bot locked and limited conversation to collaborators Nov 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants
You can’t perform that action at this time.