Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[WIP] Add count on module #13855

Closed
wants to merge 4 commits into from
Closed

[WIP] Add count on module #13855

wants to merge 4 commits into from

Conversation

Pryz
Copy link
Contributor

@Pryz Pryz commented Apr 21, 2017

The intent of this PR is to support count on modules. This is a work in progress. I will update the description asap with more details.

Current status : handle only count = 1 or 0.

Fixing : #953

Todo :

  • Add tests with count = "0"
  • Handle count > 1

@apparentlymart
Copy link
Contributor

Thanks for taking this on, @Pryz!

I expect this will be some pretty sprawling work when done, touching lots of aspects of Terraform's behavior, so I just want to be explicit that we might need to take a few iterations here to make sure the design fits in well with other Terraform features. We'd love to work with you on this though, if you're willing. Please let us know if you want some pre-review on any intermediate steps here, or if you have any design ideas you'd like a second opinion on; we want to be respectful of your time and effort while working on this.

@Pryz
Copy link
Contributor Author

Pryz commented Apr 22, 2017

Thanks @apparentlymart !

I'm totally aware that it will take some iterations to make it right :) This feature will be a big improvement for my team. Since we are using the same code base for all our environment (only variables differ) having count on module will bring more flexibility.

I'm going to add some tests and will ask for review :)

@Pryz
Copy link
Contributor Author

Pryz commented Apr 25, 2017

Before moving forward and start working on the graph generation I have some questions :

  • I'm currently using count within module.Tree to "enable/disable" a Module depending of the count value. Basically here I'm handling only the cases count=0 or count=1. Is it the right place to do that ? It feels wrong and it seems that I should just use config/module to set RawCount and then dealing with count in the graph generation.
  • Would you consider merging a PR that handles only those two cases since it's fixing most of the issues from Support the count parameter for modules #953 ?

@apparentlymart
Copy link
Contributor

apparentlymart commented Apr 25, 2017

Hi @Pryz! Good questions. 😀

For count on resources this is handled by initially generating a single node in the graph and then using the "dynamic expand" idea to effectively replace that single node with a subgraph when we encounter it during the graph walk. This then deals with the count = 0 case by generating an (essentially-)empty subgraph. It looks like your solution here instead addresses it at the configuration level, stripping out the module node altogether.

Doing it at the configuration layer will have a few limitations:

  • The count.index interpolation won't be usable, because the core evaluation code can't see what's going on in order to populate it.
  • When we later do Partial/Progressive Configuration Changes #4149 to allow dynamic counts, it wouldn't work for module counts since the processing would happen too early for the deferral behavior to catch it. (The plan is to handle deferrals during graph processing.)
  • References to multi-instance modules in interpolation (e.g. module.foo.*.baz) won't work as expected because they can't be understood as such by the interpolation layer.

The idea of starting off with support for only count as zero or one is an interesting thought. However, based on the above I think it would end up being a bit of a "dead end" that would require most of the work to be thrown away and redone in order to support higher counts, and would be challenging to use effectively since configuration would never be able to reference outputs of any module that might be count = 0 (since the module would not exist in that case, preventing validation).

If you're interested in digging in to learn more about how count is handled on resources, I suggest starting with this DynamicExpand implementation. I suspect a robust implementation of count on modules would start with adding a DynamicExpand method to some of the nodes in the graph that represent the module, although I have a feeling it will be more complex than for resources since a module effectively already has a subgraph inside of it for the module's own contents.

I hope this is helpful! Please let me know if you'd like me to elaborate on anything here.

@alialamine
Copy link

Thanks for implementing this.

As a note, this will also allow recursion in modules, as the count will allow for the base case to be defined and interpolation will allow for the difference in passing the parameters. I would really love to have this as a side-effect as it would allow to create modules that have lists and maps as input, and can create nested objects from them. An example would be aws_api_gateway_resource where it's easy to see them being nested.

But checks have to be put in place to prevent infinite recursions that might cost a lot (or a huge runtime till terraform crash). A maximum module depth would do the trick for this.

@Pryz
Copy link
Contributor Author

Pryz commented May 1, 2017

Thanks @apparentlymart for all those details. Going to start working on the next iteration now :)

@apparentlymart
Copy link
Contributor

Hi again @Pryz!

I happened to be in the graph-building code today for a different reason and realized that my last comment contained incorrect information.

I'd noticed that modules are not actually subgraphs in the sense that I thought I remembered them being, but are instead just all flattened into a single graph by ConfigTransformer, which is the first step of plan-graph construction, on which everything else builds.

Unfortunately this means that even after the first step of graph construction we've already "lost" the module boundaries, with the module structure then just implied by the Path attribute of the addresses on all of the resource nodes. Thus with the current structure we can't wait until we walk the graph in order to expand for the count, because the module resources have already been created in the graph by then.

Unfortunately I don't have a good immediate suggestion for how to proceed here. Terraform's current architecture is making a bunch of assumptions that make count on modules rather hard to implement. If you'd like to see more details on how the graph is constructed I suggest starting at PlanGraphBuilder, which is the component that is responsible for orchestrating the steps needed to construct the graph used in the plan step.

@Pryz
Copy link
Contributor Author

Pryz commented May 2, 2017

The modification of the Path needs to happen before : https://github.com/hashicorp/terraform/blob/master/terraform/transform_config.go#L101.

One solution I see here is to deal with count when we create module.Tree. Meaning inside config. Basically like my first proposition but with a full consideration of count to create the full path (i.e "root.service.42")

I feel like doing so will simplify also things like Outputs since when we create the context, the module.Tree will be kind of definitive.

Also count.index will need some changes to work even if we figure out a way to do count on module as we do with resources. We could make it using the Path to find the index.

Something I don't get is why References to multi-instance modules in interpolation (e.g. module.foo.*.baz) won't work as expected because they can't be understood as such by the interpolation layer. will not work. Can you give me more information ?

@apparentlymart
Copy link
Contributor

Consider the requirement that module.foo.*.id must evaluate to the empty list when the module has count = 0.

For resources, this is possible because they exist in configuration (so we can prove that the interpolation variable is valid).

If we handle count during module loading then there would not be any trace of the module configuration block, so we would either need to reject "splat" references like the one above, or just accept any module name -- whether it was in config or not -- and just return an empty list. Neither of these options is appealing.

If we instead find a way to deal with it during graph construction then the module will still be present in config but its graph nodes elided. This way the interpolation system can still see that the block exists in config.

We would need to take some special care during graph construction to make sure the dependencies get wired together properly for the input variables and outputs; resources are represented by a single graph node for the entire set of instances (using DynamicExpand to produce a subgraph) but modules are more complex because the variables and outputs are each their own graph node, so there are lots of separate dependencies to wire to get everything to work in the correct order.

Note that modules do not use subgraphs because subgraphs are, from the perspective of the main graph, just single nodes that must complete in their entirety before their dependencies can run. Since modules can be large and have many resources, we want them flattened into a single top-level graph to get the parallelism that results when e.g. a resource depends on only one of several outputs in a module.

One way we could get around this is to do it in config as you suggested and then add some extra bookkeeping info to module.Tree to remember the count for each module block declared, separately from the Children member. That would allow it to be handled in the config layer as you suggest, but may have other implications I'm not thinking of yet. It may be interesting to prototype that (basic functionality, no polish) enough to try it out with some real examples and see how it interacts with other Terraform features.

@Pryz
Copy link
Contributor Author

Pryz commented May 2, 2017

Ok, I need to fix the failing test but here is basically the prototype. The idea is to take care of the count during the load of the configuration.

We end up with something like (and I will add tests to prove it), for example with a count = 2 :

+ module.caddy-0.service.aws_route53_record.main
...
+ module.caddy-1.service.aws_route53_record.main

Going to try to add more feature tests.

@apparentlymart any thought ? :)

@apparentlymart
Copy link
Contributor

This seems like a good start, @Pryz!

I think next it would be good to figure out how interpolation for count-ed resources would work. In particular:

  • Supporting module.foo.0.attr to reference a specific module. (I know your current prototype make module.foo-0 and module.foo-1, but I think adding an additional first-class path segment will be needed for the remaining points...)
  • Supporting module.foo.*.attr to get a list of the values of attr across all of the modules created from that block.
  • Smoothing over the issue where a module is referenced as module.foo when count = 1, but needs to be module.foo.0 when it's greater than 1. e.g. for resources we tolerate the use of the 0 form even when count = 1, so it's possible to write a module where count can be set from a variable that might be 1.

I think the last of these will be the most challenging part, but shouldn't be impossible... maybe for module.foo.0 we first look for a module path root.foo.0 and then fall back on root.foo if that doesn't exist?

I'm still a little concerned with how different this is from how count is handled on resources, but I think that could be okay as long as we can make sure the user experience is consistent.

@Pryz
Copy link
Contributor Author

Pryz commented May 2, 2017

Yeah I was starting to implement something similar as resources but for all the issues we discussed before it's not going to work. The idea was also to avoid bringing to many changes here.

@Pryz
Copy link
Contributor Author

Pryz commented May 3, 2017

Supporting module.foo.0.attr is really going to be tricky and require much more changes. Feels like if we want to do that we will need to adapt all the steps in GraphTransformer to use count.

This means that, for example, in ConfigTransformer, before added a node to the Graph we will need to "hack" the path. Seems tricky and error prone :/

@Pryz
Copy link
Contributor Author

Pryz commented May 3, 2017

@apparentlymart I've pushed an update but I don't think I'm in the right direction here.

Copy link
Contributor

@apparentlymart apparentlymart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a plausible starting point for the implementation path we discussed. Nice work!

I want to clarify that the design for this feature is not fully fleshed out yet, so I would consider the first goal to do some prototyping and learn more about the problem, rather than to implement a final solution.

If you're motivated to keep working on this I'm certainly happy to keep working with you, but I want to make sure we're aligned on where we are with this right now as I want to be respectful of your time; prototyping of these bigger features can be a rather time-consuming effort.

What you've done so far here has already helped a lot in evaluating a few different options and seeing how this feature might fit into Terraform, so thank you very much for that!

for j := 0; j < len(m.Path()); j++ {
path[j] = m.Path()[j]
}
path[len(path)-1] = strconv.Itoa(m.Count())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should m.Count() here actually be i?

@Pryz
Copy link
Contributor Author

Pryz commented May 11, 2017

Don't have much time this week but I'm definitively still motivated to go through that thing :)

@Pryz
Copy link
Contributor Author

Pryz commented Jun 26, 2017

Hi there,

I currently don't have time to work on this. So I will close this PR.

I will probably be able to work on that in few weeks but I'm sure someone will tackle that before me (I hope so actually :D)

@Pryz Pryz closed this Jun 26, 2017
@apparentlymart
Copy link
Contributor

Thanks for your work here @Pryz! It was a good prototyping exercise and I'm sure we can build on this work.

@peter-dolkens
Copy link

What ever happened to this? Even the 0/1 use case would be beneficial. The main thread at #953 seems to have gone dead since it was locked :(

@ghost
Copy link

ghost commented Aug 21, 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.

@ghost ghost locked and limited conversation to collaborators Aug 21, 2019
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants