core: fix the provisioner graphing #4877

svanharmelen · 2016-01-28T15:14:44Z

Without this change, all provisioners are added to the graph by default and they are never pruned from the graph if their not needed.

I must admit I'm not a 100% sure if this is the correct solution, but there are a few (weird) things that we should discuss and/or have a look at in order to determine the correct solution.

I wonder if it makes any sense at all to have a MissingProvisionerTransformer? It makes sense to have a MissingProviderTransformer because you could have used a resource without specifying the needed provider for that resource. But that isn't possible for a provisioner as that is a single entity which you either specify in your config, or you don't. So how can you ever have a missing provisioner in your config?
I think we could ask ourselves the exact same question for the PruneProvisionerTransformer. What could this transformer ever prune (if we don't add all available provisioners or selves that is, see point 3 😉).
If we still do want the MissingProvisionerTransformer, then I suppose we only want to add the provisioners that we actually need. Currently we just add all known provisioners and try to prune them in a later stage (which currently doesn't work, see point 4). The code in the PR changes the logic by only adding the provisioners that are needed by the supplied config in the same way as this is done for providers.
We should have a closer look at PruneProviderTransformer and PruneProvisionerTransformer. They currently both check if there are any UpEdges, and only if there none the provider or provisioner is removed from the graph. But when the RootTransformer is executed before one of the prune transformers, they will have at least 1 UpEdge, being the graphNodeRoot added by the RootTransformer. And since the current graph builder ordering places the RootTransformer before the prune transformers, it means the prune transformers are effectively broken at this moment.

The code in this PR fixes the problem described in point 4 by reordering the transformers, but that doesn't feel like a robust solution. Another (possibly better) fix would be to change this code to something like:

s := g.UpEdges(v)
if s.Len() == 0 {
  g.Remove(v)
}
if s.Len() == 1 {
  if _, ok := s.List()[0].(graphNodeRoot); ok {
    g.Remove(v)
  }
}

This would make the ordering of the graph transformations less critical, but I'm not sure if this is a 100% fix and if this also works when your using modules and/or already have a root, not being a graphNodeRoot (not sure that is even possible?). So I need some input to be sure this would indeed be a solid fix for the current prune transformers problem in all use cases...

I labeled this as a bug, because the current code allocates resources (mainly a goroutine with a yamux stream) for each provisioner for every action (e.g. refresh, plan, apply) which is never released. If your running Terraform as a long running daemon (API) you can see this will kill your service at a certain point in time 😁

Without this change, all provisioners are added to the graph by default and they are never pruned from the graph if their not needed.

phinze · 2016-01-28T15:24:11Z

Hi @svanharmelen, this seems reasonable at first glance!

I'm curious, though, does this have any functional effect?

Also same question for the RootTransformer move - is that required to make this change or was that just a cleanup you saw while in there?

svanharmelen · 2016-01-28T16:02:32Z

@phinze I was still typing the complete description for this PR, so please have another look as I suspect it will now answer your questions 😉

/CC @mitchellh as this is mainly code he wrote at the time...

svanharmelen · 2016-01-29T18:02:30Z

@phinze @mitchellh any feedback or thoughts about this one?

svanharmelen · 2016-02-02T11:47:32Z

Any chance we can discuss this one?

If it's up to me I would just remove MissingProvisionerTransformer and PruneProvisionerTransformer all together and then the only question left is how to properly fix PruneProviderTransformer.

The reordering I did in this PR still feel hacky. So would the suggested code change do the trick and be robust enough? Or should we extend that peace of code to check additional use cases/types?

mitchellh · 2016-02-02T17:17:09Z

Looking now, expect a follow up comment shortly.

mitchellh · 2016-02-02T19:12:20Z

My thoughts are: not having the missing and prune transformers make sense. If we can remove those without breaking any tests (other than those that might just test for their existence), then it probably will work fine.

In terms of this PR, lets go with the route that makes the code simpler vs more complex, if we can do that while having all tests still pass.

svanharmelen · 2016-02-02T19:39:43Z

Thanks for the feedback! I'll update the PR (somewhere tomorrow) accordingly 👍

svanharmelen · 2016-02-04T13:44:14Z

@phinze @mitchellh just tried to remove the MissingProvisionersTransformer, only to learn that this transformer is responsible for adding any provisioner to the graph. So without this, no provisioners are added to the graph at all 😞

I first added a few lines of code to the ProvisionersTransformer to add any needed provisioners in there. But then I though it would be cleaner to have that logic separated, so I ended up reverting some parts of that 😁

To make things a little more descriptive I renamed a few types that had missing in their names, as they are not a specific type for missing provisioners, they just represent the (any) provisioners and/or providers in the graph.

So I think the PR should be good to go now, however I do have one feedback question left... I noticed I had to change 1 test in order to make all tests pass: https://github.com/hashicorp/terraform/pull/4877/files#diff-1848efa7a9473a60ae890ae47d35cbf0

I looked at this one, but I fail to fully understand if the previous version is the correct and desired outcome or if this current version is in fact the expected outcome. I was unable to reason about it correctly, so I hope one of you can enlighten me on this one.

Thanks!

mitchellh · 2016-02-04T19:45:01Z

terraform/transform_provider.go

-			// Add our own missing provider node to the graph
-			m[p] = g.Add(&graphNodeMissingProvider{ProviderNameValue: p})
+			// Add the missing provider node to the graph
+			m[p] = g.Add(&graphNodeProvider{ProviderNameValue: p})


Why rename this?

I'm actually +1 on this rename or something similar (since this node really does represent the provider itself in the graph) but I'd prefer we do it as a separate PR.

Yeah, that was exactly the argument for renaming this... Since this node represents the provider itself in the graph, this name seemed a little more suitable and more descriptive.

I added the rename in this PR as the provisioner and provider transformers are very much inline with each other and I changed it for the provisioner as well.

But I'll take this code out if you prefer that and make a new PR for the provider renaming... 👍

mitchellh · 2016-02-04T19:45:22Z

One question about why renaming the missing provider when I believe this PR is about provisioners.

svanharmelen · 2016-02-04T20:23:18Z

Check... Replied inline, but will take out the provider rename and put in it a separate PR.

And renamed some types so they better reflect what they are for.

svanharmelen · 2016-02-04T20:36:37Z

Updated the PR so it only contains provisioner related changes... Still would like to get a conformation (double check) that the updated test case is now giving the expected results so we're sure to not introduce any kind of regression. Thx!

josephholsten · 2016-02-05T01:43:05Z

@svanharmelen I thought you were pulling out the renames from this branch? do you still need to push those changes?

josephholsten · 2016-02-05T01:51:43Z

ah, all the functional changes are in the first commit. LGTM, though I'm not qualified to vouch for test completeness.

phinze · 2016-02-05T17:21:07Z

Hello again! This is looking good - I believe it solves the Provisioner already initialized portion of #3114 - cooking up a test to verify that it does that I'd like to drop in. Will keep you posted.

svanharmelen · 2016-02-05T18:19:22Z

Sounds good... Thx!

Currently failing, but going to move this commit around to vet fixes. Confirmed that #4877 fixes the provisioner half of this!

phinze · 2016-02-05T18:36:02Z

Okay just confirmed on the linked branch/commit that this does indeed address the provisioner side of that problem. So I'll merge this now and then land that test once the Provider side of the issue is handled (looking into that now).

Thanks for all your work on this, Sander! 👍

core: fix the provisioner graphing

Context: As part of building up a Plan, Terraform needs to detect "orphaned" resources--resources which are present in the state but not in the config. This happens when config for those resources is removed by the user, making it Terraform's responsibility to destroy them. Both state and config are organized by Module into a logical tree, so the process of finding orphans involves checking for orphaned Resources in the current module and for orphaned Modules, which themselves will have all their Resources marked as orphans. Bug: In #3114 a problem was exposed where, given a module tree that looked like this: ``` root | +-- parent (empty, except for sub-modules) | +-- child1 (1 resource) | +-- child2 (1 resource) ``` If `parent` was removed, a bunch of error messages would occur during the plan. The root cause of this was duplicate orphans appearing for the resources in child1 and child2. Fix: This turned out to be a bug in orphaned module detection. When looking for deeply nested orphaned modules, root.parent was getting added twice as an orphaned module to the graph. Here, we add an additional check to prevent a double add, which addresses this scenario properly. Fixes #3114 (the Provisioner side of it was fixed in #4877)

svanharmelen · 2016-02-08T08:06:47Z

Thanks!

Context: As part of building up a Plan, Terraform needs to detect "orphaned" resources--resources which are present in the state but not in the config. This happens when config for those resources is removed by the user, making it Terraform's responsibility to destroy them. Both state and config are organized by Module into a logical tree, so the process of finding orphans involves checking for orphaned Resources in the current module and for orphaned Modules, which themselves will have all their Resources marked as orphans. Bug: In #3114 a problem was exposed where, given a module tree that looked like this: ``` root | +-- parent (empty, except for sub-modules) | +-- child1 (1 resource) | +-- child2 (1 resource) ``` If `parent` was removed, a bunch of error messages would occur during the plan. The root cause of this was duplicate orphans appearing for the resources in child1 and child2. Fix: This turned out to be a bug in orphaned module detection. When looking for deeply nested orphaned modules, root.parent was getting added twice as an orphaned module to the graph. Here, we add an additional check to prevent a double add, which addresses this scenario properly. Fixes #3114 (the Provisioner side of it was fixed in #4877)

Context: As part of building up a Plan, Terraform needs to detect "orphaned" resources--resources which are present in the state but not in the config. This happens when config for those resources is removed by the user, making it Terraform's responsibility to destroy them. Both state and config are organized by Module into a logical tree, so the process of finding orphans involves checking for orphaned Resources in the current module and for orphaned Modules, which themselves will have all their Resources marked as orphans. Bug: In hashicorp#3114 a problem was exposed where, given a module tree that looked like this: ``` root | +-- parent (empty, except for sub-modules) | +-- child1 (1 resource) | +-- child2 (1 resource) ``` If `parent` was removed, a bunch of error messages would occur during the plan. The root cause of this was duplicate orphans appearing for the resources in child1 and child2. Fix: This turned out to be a bug in orphaned module detection. When looking for deeply nested orphaned modules, root.parent was getting added twice as an orphaned module to the graph. Here, we add an additional check to prevent a double add, which addresses this scenario properly. Fixes hashicorp#3114 (the Provisioner side of it was fixed in hashicorp#4877)

ghost · 2020-04-28T02:07:54Z

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.

Fix the provisioner graphing

5c3da47

Without this change, all provisioners are added to the graph by default and they are never pruned from the graph if their not needed.

jen20 added enhancement core labels Jan 28, 2016

svanharmelen added bug and removed enhancement labels Jan 28, 2016

mitchellh reviewed Feb 4, 2016
View reviewed changes

Cleaning up the PruneProvisionerTransformer

1bec114

And renamed some types so they better reflect what they are for.

svanharmelen mentioned this pull request Feb 4, 2016

Rename graphNodeMissingProvider to graphNodeProvider #4999

Merged

phinze added a commit that referenced this pull request Feb 5, 2016

core: Add test reproducing #3114

be947d2

Currently failing, but going to move this commit around to vet fixes. Confirmed that #4877 fixes the provisioner half of this!

phinze added a commit that referenced this pull request Feb 5, 2016

Merge pull request #4877 from svanharmelen/b-provisioner-graphing

0846e32

core: fix the provisioner graphing

phinze merged commit 0846e32 into hashicorp:master Feb 5, 2016

phinze mentioned this pull request Feb 5, 2016

core: fix bug detecting deeply nested module orphans #5022

Merged

svanharmelen deleted the b-provisioner-graphing branch February 8, 2016 08:13

ghost locked and limited conversation to collaborators Apr 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: fix the provisioner graphing #4877

core: fix the provisioner graphing #4877

svanharmelen commented Jan 28, 2016

phinze commented Jan 28, 2016

svanharmelen commented Jan 28, 2016

svanharmelen commented Jan 29, 2016

svanharmelen commented Feb 2, 2016

mitchellh commented Feb 2, 2016

mitchellh commented Feb 2, 2016

svanharmelen commented Feb 2, 2016

svanharmelen commented Feb 4, 2016

mitchellh Feb 4, 2016

phinze Feb 4, 2016

svanharmelen Feb 4, 2016

mitchellh commented Feb 4, 2016

svanharmelen commented Feb 4, 2016

svanharmelen commented Feb 4, 2016

josephholsten commented Feb 5, 2016

josephholsten commented Feb 5, 2016

phinze commented Feb 5, 2016

svanharmelen commented Feb 5, 2016

phinze commented Feb 5, 2016

svanharmelen commented Feb 8, 2016

ghost commented Apr 28, 2020

core: fix the provisioner graphing #4877

core: fix the provisioner graphing #4877

Conversation

svanharmelen commented Jan 28, 2016

phinze commented Jan 28, 2016

svanharmelen commented Jan 28, 2016

svanharmelen commented Jan 29, 2016

svanharmelen commented Feb 2, 2016

mitchellh commented Feb 2, 2016

mitchellh commented Feb 2, 2016

svanharmelen commented Feb 2, 2016

svanharmelen commented Feb 4, 2016

mitchellh Feb 4, 2016

Choose a reason for hiding this comment

phinze Feb 4, 2016

Choose a reason for hiding this comment

svanharmelen Feb 4, 2016

Choose a reason for hiding this comment

mitchellh commented Feb 4, 2016

svanharmelen commented Feb 4, 2016

svanharmelen commented Feb 4, 2016

josephholsten commented Feb 5, 2016

josephholsten commented Feb 5, 2016

phinze commented Feb 5, 2016

svanharmelen commented Feb 5, 2016

phinze commented Feb 5, 2016

svanharmelen commented Feb 8, 2016

ghost commented Apr 28, 2020