Flatten modules into main graph #1781
Conversation
(untested yet)
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I love how the big overarching test comes first in the diff. 😄
"WE GON' MAKE THIS WORK!"
| // Perform the transitive reduction to make our graph a bit | ||
| // more sane if possible (it usually is possible). | ||
| &TransitiveReductionTransformer{}, | ||
| ) |
There was a problem hiding this comment.
Ah yeah this is funky, but agreed with top-level description that it's better to land it working and then refactor.
|
Running this on a local sample infra with a cycle I thought would have been fixed by this yielded unexpected results. Digging in and I'll follow up. |
passing output of one module into input of the following module results in a cycle
core: failing test for a bad module cycle
Flatten modules into main graph
|
@mitchellh I can get you access to the repo if you would like to test with it in some capacity. I can also test that repo with this updated revision. That repo will be public soon.. |
|
@ketzacoatl I worked with @phinze and verified we've fixed your bvug. |
|
very exciting! |
This reimplements my prior attempt at nipping issues where a plan did not yield the same cycle an apply did. My prior attempt was to have ctx.Validate generate a "Verbose" worst-case graph. It turns out that skipping PruneDestroyTransformer to generate this graph misses important heuristics that prevent cycles by dropping destroy nodes that are determined to be unused. This resulted in Validate improperly failing in scenarios where these heuristics would have broken the cycle. We detected the problem during the work on #1781 and worked around the issue by reverting to the non-Verbose graph in Validate. This commit accomplishes the original goal in a better way - by generating the full graph and checking it once Plan has calculated the diff. This guarantees that any graph issue that would be caught by Apply will be caught by Plan.
|
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
Fixes #1582
Might affect #1637 (hopefully fixes, unsure due to private repo).
All context tests pass but this is a pretty big change and I'll want to try at least on our core ops repo before merging (since we use modules heavily).
This changes the way the graph treats modules. Prior to this PR, modules were their own isolated subgraphs. When evaluating, the entire subgraph would get walked in one step. The problem with this is that it turns out that modules can't always be seen as a single executable unit.
This PR flattens the subgraphs of modules into the main root graph, then does the destroy transformations after this flattening. This lets the destroy edge manipulations have a global view of the graph, and create the proper ordering of things.
Most of this was surprisingly straightforward. The only part I'm really unhappy about is the special casing in graph builder to only do some steps for the root path.
I think a future change/refactor can probably simplify this even further by just getting rid of static subgraphs altogether and just having the transform add all the nodes for all the modules at once. I was about to go down this route, but decided that if this works, we should get this in first, and refactor later, especially given that the changes here aren't hideous.
I have a feeling this will fix weird module-boundary create-before-destroy issues as well but I don't have a test case yet to reproduce anything of that sort so I can't confirm.
Additionally, @armon raised good points about this will hurt performance of Terraform for very large graphs. This is because the O(N) lookups for graph vertices will get bad since N Is getting quite large (before it was N within each subgraph). Given that almost all O(N) lookups are for interface implementations, I think we can just do interface-specific lookaside tables in the future and speed everything up to O(1).
@phinze: One thing for you is to ask if the graph functionality looks good? I don't want to affect the
terraform graphoutput we did for 0.5 for this.