Change node pool resources to use for_each

[trotttrotttrott]

In the circumstance that a node pool must be replaced and workload transitioned with zero downtime, having all node pools defined in a list and launched with a single google_container_node_pool resource makes it difficult to carry out.

For example, if two node pools are defined and node pool 0 must be replaced. It seems like the only safe approach is to temporarily create a third node pool, transition the workload from node pool 0 to the new node pool, change or replace node pool 0, transition the workload back to node pool 0, and finally destroy the temporary node pool so you're back to two. Which is fine, but it's probably more work than necessary.

Another consideration is if there are many node pools defined and you need to destroy node pool 0 completely. There is no way to do this without affecting node pool 1 and any other subsequently defined node pools.

This adaptability seems to be a limitation of this module. But also perhaps the use of count in resources in general. It might be better if users had control over their node pools as independent resources. However, leveraging count and a list of node pools is likely the only way to make any GKE module flexible enough for broad adoption given the current limitations of Terraform.

I'm opening this issue to see if there is a better way. Or if we can come up with a way to improve conditions.

[morgante]

I think we should consider having separate node pool submodules for explicit management.

Hopefully, Terraform 0.12 will make count cases less problematic.

[brandonjbjelland]

Your point is 100% valid @trotttrotttrott but as it stands this is the best terraform can do. Separating out the resources, to me, doesn't seem to make sense as a cluster and a node pool are as intertwined as a hotdog and a bun 🌭 . If we separate them, in one of the cases, we'll likely just be packaging a single resource... which has negative value.

To spin this issue into perhaps something actionable, I think this module could use an operator's README. That is, common operations wouldn't seem so common, simple, or without unexpected side effects without some guidance. Even the act of adding a node pool, via the list of maps carries with it some assumptions that the end user engineer knows lists are ordered in state, and appending is the only way to reach the desired outcome without disruption. Node pool management is where most of this instruction is needed. These would fall along the lines of:

1. adding a node pool
2. removing a node pool (turn min/max to zero, leave list item in place - I doubt state mv is a good idea or possible for moving around list items with internal ids)
3. migrating workloads via taints
4. designing the set of nodepools upfront to best account for the inevitable operations above.

Terraform is definitely limited in its ability to handle these sorts of use cases but armed with some guardrails, I think this module becomes much more practical and useable to our audience. Thoughts?

[morgante]

Terraform is definitely limited in its ability to handle these sorts of use cases but armed with some guardrails, I think this module becomes much more practical and useable to our audience. Thoughts?

Yes, that could certainly be helpful though I wonder if 0.12 will come soon enough to obviate the need.

[brandonjbjelland]

Movement on the providers would seem to indicate 0.12 is near. Even still, I haven't peeked under the hood far enough to have an understanding of how/if internal data structures are stored differently. For someone who has the time, it'd be worth giving 0.12 beta a whirl and diving into the source to see where it stands today in its handling of lists in state and plans on those objects.

[morgante]

Ah, it looks like a resource for_each won't be included in 0.12 initial release anyways: https://www.hashicorp.com/blog/hashicorp-terraform-0-12-preview-for-and-for-each

@brandoconnor or @trotttrotttrott Would you mind replacing this issue with a new FR to document the operations necessary for node pool additions/edits?

[morgante]

Terraform v0.12.6 supports for_each: https://www.terraform.io/docs/configuration/resources.html#for_each-multiple-resource-instances-defined-by-a-map-or-set-of-strings

We should switch the node pool resources to use for_each.

[Dev25]

PR #257