terraform: new plan graph #9973
Conversation
terraform: more specific resource references terraform: outputs need to know about the new reference format terraform: resources w/o a config still have a referencable name
terraform: remove final TODO
| // on the count argument given. | ||
| // | ||
| // Orphans are found by comparing the count to what is found in the state. | ||
| // This tranform assumes that if an element in the state is within the count |
There was a problem hiding this comment.
seriously, is the "tranform" typo the only thing fix you're going to throw me ;)
| split[i] = s + ".destroy" | ||
| } | ||
|
|
||
| result = append(result, strings.Join(split, "/")) |
There was a problem hiding this comment.
Can you explain what's going on here? I'm not sure I understand what this reference format is doing.
There was a problem hiding this comment.
Yes, definitely, I'm glad you caught this.
So, this is a lamentable example of where we're using strings when we should probably be using some sort of structured data. I actually want to look into that refactor shortly after this, but didn't want to merge the two since I think that'll end up being fairly significant.
In this case, the syntax A/B means "I depend on A, but if A is not available, I depend on B. If A is available, don't connect to B."
The use case for this is in expanded resources. There are three cases for a resource reference:
aws_instance.foo.bar- Equivalent toaws_instance.foo.0.bar. See next.aws_instance.foo.0.bar,aws_instance.foo.5.bar, etc. (exact index) - In this case, you want to depend EXACTLY on a specific instance if it exists. However, if it doesn't exist (computed count), you want to depend on the thing that will expand into an exact instance. So, in this case, we set the depends on to beaws_instance.foo.0/aws_instance.foo.Nwhere "N" is a special syntax used to denote "I will expand to more things at runtime."aws_instance.foo.*.bar(splat) - Straightforward, you depend on allaws_instance.fooinstances. This creates a depends on ofaws_instance.foo.*which every matching resource will advertise as something they can be depended on as.
The key thing is (2) above. The existing reference system didn't support this. THE WAY IT WORKED BEFORE:
- Everything would depend purely on the
aws_instance.foogranularity (not count-specific) - During count expansion (
DynamicExpandintransform_resource.go, the old graph), we'd overrideDependsOnandDependableNameto append the index if we have one. Then we'd reconnect.
This doesn't work in the new graph because some specific indexes (like orphans) are created outside the graph. I also wanted to lay the groundwork so that we can avoid cycles in computed counts if we know a specific index exists (in the state, for example). This isn't fixed yet with this new graph but we're heading in that direction and we'll have to anyways.
In conclusion, the right long term solution is a structure rather than a string. I patched on the / syntax since a resource name can't contain a / so its a safe character to use. However, I do want to take a look at this again to clean it up, but wanted to save that for another time since that refactor will touch a lot more than just plan-time stuff.
| // | ||
| // Unlike ConfigTransformerOld, this transformer creates a graph with | ||
| // all resources including module resources, rather than creating module | ||
| // nodes that are then "flattened". |
There was a problem hiding this comment.
Is there no more graph "flattening", or the does this create the equivalent of the "flattened" graph?
There was a problem hiding this comment.
Yes to both, but there is no flattening operation.
A history on flattening: Terraform used to treat modules as sub-graphs (that were never flattened) and when you "enter" the subgraph you'd built a new EvalContext and when you exited you'd throw it away. This was how modules worked in TF 0.3 (first version with modules I think).
We soon realized with bug reports that modules are not an isolated graph. There are complex ordering requirements that require entering/exiting the module boundary multiple times. The simplest example is:
- module input
Acreated byroot.fooresource - module resource
module.barcreated with inputA - module output
Xcreated with a value frommodule.bar root.bazresource created with module outputX- module input
Buses value from resourceroot.baz - module resource
module.quxuses inputB
Notice how this requires in/out, in/out at least twice. Hence, the isolated subgraph didn't work. This was a serious serious bug that made modules basically unusable in Terraform 0.3. We needed a quick bolt-on solution to fix this in TF 0.3.1. That solution ended up being "well, we already have these correct subgraphs, what if we just flattened them in and added the necessary extra edges?" And so we did.
And graph flattening has been with us since.
But it is complicated. We knew even in 0.3.1 the right solution would've just been to put ALL resources in ONE graph from the beginning. There is no "flatten" step: you just have all resources with the proper address from the beginning.
This transform does that. Gone with flattening.
|
Has anybody mentioned how annoying this is for people that are not Terraform developers ? Please, please please please, make "features" like this something I can disable at runtime next time. I'd love to report bugs, and maybe even fix them, but the output doesn't even tell me what went wrong, other than I know, shame on me for compulsively upgrading Terraform to get access to bugfixes. |
|
Hey there! Yes we know it's annoying, but it's meant to be. It's far better to see this annoying message than to run a future Terraform version that does something horrible to your infrastructure. There are a few additional notes here:
For folks who update often like yourself, you shouldn't be discouraged or shamed by it, that's not what we want. Any shadow errors we prioritize and fix exactly cause we know it's annoying. Every release thereafter has less and less. And, to see the fruits of this labor: look at the 0.8 changelog! Those features there that I think you'll like or at least understand would've been far more difficult without the internal changes such as this PR which require shadowing. In conclusion: it's annoying, and if you have ideas for improving it I'm open to them, but we want to ship stable software since this can affect real production infra. Sent from my iPhone
|
|
@mitchellh - Thanks for the speedy and lengthy reply. I understand what and why you're doing it, but I'm just asking for a way to opt-out. Did I say thank you for Terraform? I use it every day, a lot. My life would be significantly less pleasant if I had to use CloudFormation as much as I use Terraform. My only "idea" is... If I'm a good tester/citizen/member of the community, we can presume that when I see a bug, I report the bug (and provide a patch) and move on with my life. At this point, I don't need to see the many screenfuls of messaging. I've done the world good! If I'm a bad tester/citizen/member of the community, we can presume I'm a leech who only takes takes takes, and I won't be reporting the bugs. As such, the shadow graph is useless to me, until you release the new version and I just complain about problems. So maybe next time, give me a way to disable the shadow graph (or whatever new feature), or output to STDERR so I can filter it and decide whether to be good tester or bad tester. Since Terraform state embeds the Terraform version, I can't just downgrade to $PRIOR_VERSION without fiddling around with tfstate. Today, I'm a bad tester, and just need to get stuff done, but I can't see what TF is doing. ❤️ |
|
@mrwacky42 You bring up good points. We've actually had great success with people reporting crashing bugs where the crash log is written to a file. I wonder if we can do the same thing here. I'm going to think on it and promise to ship a better user experience here. Thanks! |
|
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
Similar to the new apply/destroy graphs.
Experiment flag:
-Xnew-apply(merging into that)High level PR notes:
terraform planI originally wanted to wait until the new apply/destroy graphs were more stable, but as I looked at the core issues as well as the relative stability of the new apply/destroy graphs, I believe its more valuable to get this in for 0.8 beta cycle than to wait. There are many complex bugs that have to be fixed in the plan graph moreso than the apply graph.
An example is #9853: this issue never even cropped up with this new graph due to the simplicity of it (plans don't need to even care about CBD). The fix still is necessary since the new apply graph uses CBD, but at least the failure would've been smaller in scope.
There are other issues that I've been wanting to work on that are simply too complicated with the current all-in-one-graph.
Another huge benefit of this graph is that there is no more graph flattening. All complex graphs (plan, apply, destroy) no longer have module flattening. The graphs contain the modules from the getgo and this simplifies cycle-enablers greatly.
Just as with the other graphs, we enable shadowing on plan which compares the diffs. I want to get this into the 0.7.10 release so we can get more shadow graph errors with plans as we get going.