Improve container engine / node pool handling

[drzero42]

Terraform Version

v0.10.0

Affected Resource(s)

• google_container_cluster
• google_container_node_pool

During my work with trying to bring our Google Cloud Platform infrastructure under Terraform administration, I ran into what seems like an artificial limitation in the provider. We run Container Engine clusters with multiple node pools, which have different node configurations. Currently the google_container_cluster resource only allows me to set names and initial_node_count for the node pools I want to create, which will all be created with the same node config. I see that google_container_node_pool resources in the current master branch actually support seperate node configs, even though the documentation on terraform.io doesn't reflect it yet. This is a step in the right direction, as I can let google_container_cluster create the default pool and add a second one next to it. This, however, still makes it difficult to make container_cluster resources importable, as the get/describe call will return all of the node pools attached to a cluster.

I am not experienced enough with Terraform or GCP yet, to suggest a perfect solution for this, but my (maybe naive) thought for handling this would be to have a node_pools argument for google_container_cluster resources, which is a list of hash references to google_container_node_pool resources. I think this leaves us with a chicken/egg or circular dependency problem, where there doesn't exist a hash ref for the google_container_node_pool resources until they are created, but they can't be created until a cluster exists. The API expects the call to create a cluster to include at least one node pool (or node config, which will be used to created a pool called default-pool). This leads me to think that maybe node pools shouldn't be independent resources, but rather compound arguments on google_container_cluster resources. This would simplify import, but would require additional code to handle changes to node pools.

Maybe this ticket could be a discussion where a consensus for how to handle this could be found?

[danawillow]

Adding/removing node pools is really complicated with regards to the default node pool. Let's say I have only the default node pool in my cluster, and I want to add another one (assume we supported update on node pools in clusters for now). But our terraform config doesn't have the default node pool in it, so terraform would want to delete it, which isn't our intent. This means that everyone that wants to make changes to node pools would then have to add the default one back into their config, which is kind of gross. If we removed node pools entirely from clusters and left them as separate resources, then we lose the ability to create a node pool that isn't the default one on cluster creation.

So no solution from me right now either, but I'll be thinking about this since it's important. Happy to have this conversation though, it's something I've been meaning to get to for a while :)

[drzero42]

Okay, here are my thoughts, after more study of the API available and the current code we have.
In my mind it makes no sense to have node pools as seperate resources, when at least one has to exist for a cluster, and no pool can exist without a cluster. Because of how Terraform states work, I don't see how we can have clusters and node pools seperated. Instead I would suggest a node_pools list attribute on cluster resources, and don't allow initial_node_count and node_config on the top of the cluster resource, but only on entries in the node_pools list. It requires a bit more coding in update and create functions, but should be doable. Most of the attributes, which currently forces a recreate upon changes, can actually be updated without recreating the resource. I have tried writing down a very basic breakdown of the logic involved with CRUD for container_cluster resources with embedded node_pools. These are just my thoughts on it, but could maybe be expanded into a design document. My knowledge of Terraform internals are still limited, so I apologize if my approach is incorrect or weird for some operations.

We create a schemaNodePool in the same spirit as schemaNodeConfig.

For create:
Complete Cluster object can be sent in one.
initialNodeCount and nodePool should not be used.
nodePools[] with at least one should be used instead.
Instead of node_version the name version is probably better.
Use version to set initialClusterVersion in the create request.

For get:
projects.zones.clusters.get outputs a Cluster object which should map nicely to the state with embedded node_pools with just a few exceptions.
One exception is to map currentMasterVersion to version.

For update:
The current implementation for the provider marks a lot of attributes as forcing a recreate. It seems to me, this does not need to happen for a lot of them.
If version has changed, update with projects.zones.clusters.master.
If zone or additional_zones has changed, build a locations[] and update with projects.zones.clusters.locations - does not need to ForceNew.
If logging_service has changed, set with projects.zones.clusters.logging - does not need to ForceNew.
If monitoring_service has changed, set with projects.zones.clusters.monitoring - does not need to ForceNew.
If addons_config has changed, set with projects.zones.clusters.addons - does not need to ForceNew.

Loop through nodePools[] from returned Cluster object
Based on nodePool names:

• If any exists remote, which does not exist locally, delete with projects.zones.clusters.nodePools.delete
• If any exists locally, but not remote, create with projects.zones.clusters.nodePools.create

If machine_type for a nodePool has changed: forceNew.
If autoscaling for a nodePool has changed, set with projects.zones.clusters.nodePools.autoscaling.
If management for a nodePool has changed, set with projects.zones.clusters.nodePools.setManagement.
If version for a nodePool has changed, update with projects.zones.clusters.nodePools.update.
Limitation in API: We can't get current number of nodes in a pool, only the initial count.

For delete:
Looks like a call to projects.zones.clusters.delete cleans up nicely by also deleting node pool.

Looking forward to hearing where my logic is wrong, or any arguments for/against this approach :)

[danawillow]

Let's table the conversation about Update for now, since there's not much of a design decision to happen there- as time has gone on, GKE has allowed updating of more fields and they just haven't made it into Terraform yet because we've been more focused on other things people have asked for :) (feel free to file a separate issue about it though!)

With regards to node pools- your approach is pretty similar to the one that I came up with when I started thinking about this. I had actually planned on deprecating the separate node pools resource and then got pulled on to other things and never swung back to finish it up.

I actually do like the idea of insisting the user create a node pool rather than allowing the API to create a default one, since the user is still specifying the same attributes they would with the default one- it just appears elsewhere. And then we don't run into diffs because the number of node pools server-side is different from what's in our config file.

[drebes]

#299 is going in that direction.

[drzero42]

Regarding update - I will create separate issues for the fields that don't need to force recreation anymore, and maybe also take a stab at some PRs to fix those issues :)

Sounds good to me, to deprecate separate node pools and insist the user specify them in the cluster resource :)

Can we break this down in to smaller steps, or does it need to be one big patch?
What would be the best way for me to contribute towards this?

[danawillow]

You can probably break it down into smaller steps- give it a try and see how it goes :)

Here are some docs about contributing, let me know if you have questions or need help: https://github.com/hashicorp/terraform/blob/master/.github/CONTRIBUTING.md

[danawillow]

Actually, let's back up a step to make sure we need to do this. What exactly are you trying to do with the current infrastructure? It sounds like you want to make container cluster importable and you're running into issues there because node pools on clusters in terraform don't accept a node config. That's going to be fixed in #299. Once that's fixed, what's the next problem that leads container clusters to not be importable? Or am I misunderstanding somewhere?

[drzero42]

Mostly my purpose is to be able to manage our GCP infrastructure with Terraform. There used to be a bunch of problems where the Google provider made assumptions about how people use GCP, which did not follow how we had setup our infrastructure. But the Provider seems to have been vastly improved over the last couple of months (kudos for that!), along with us having changed our setup a bit, means that we are down to mostly not being able to import certain of our resources, most important of them being Container Engine Clusters / Node Pools.
With the Node Configs being added to Node Pools, and the google_container_cluster resource being importable, I think the real show stoppers are fixed. After that, I think we are down to mostly annoyances like the fields that don't need to force recreation, but those are going to be seperate issues.
So to sum it up, my main concern is close to being addressed (importability being the next/last step). I do however still believe that it would be better to deprecate separate google_container_node_pool resources along with initial_node_count and node_config attributes on the google_container_cluster resource, to avoid confusion. I am all for flexibility, but I think it will create frustration for a lot of users if they start out with initial_node_count and node_config and then want to change to defining node_pools inside the google_container_cluster resource. In my opinion it is better to have a "one true way" from the beginning.

[drzero42]

One more thing that I guess this ticket might also address - changes to node_pools should not need recreate the entire cluster :)

[danawillow]

So it seems like there are a few different things getting conflated here.

On the topic of Terraform not having certain functionality that the API exposes, that's almost always the case of "nobody got around to implementing it yet" vs us assuming you won't use certain features. Likewise, certain things get added to the API later (like all the properties of container cluster that are now updatable), which can easily lead to resources that are out-of-date solely due to the passage of time and not because of any decisions explicitly made to keep them that way.

So now in terms of deprecating the node pool resource, is there a scenario that having both makes impossible? If so, it'll be much easier to just leave things the way they are so users don't have to change their existing configs. Having node pools as a separate resource also allows users to easily add/remove node pools after the default one has been created. There have been a lot of situations in other resources where we've implemented similar behavior (allowing two different ways of specifying the same thing) in order to handle different use cases (see #171 for a good example). We can certainly update our docs to make sure people are using the right resource from the beginning, but if there's not broken behavior I'd rather not have us spend the effort on this.

As for removing ForceNew from NodePools, that's certainly a valid issue, feel free to file it :)

[drebes]

I think the current pair of resources to manage a GKE cluster makes things confusing. The default pool config can only be configured through google_container_cluster while the non-defaut pools can only be configured through google_container_node_pool (until #299 gets merged.)

Personally I prefer to manage everything through google_container_cluster as it leaves the lifecycle of node pools from the same cluster more coupled. While updating node pools is a nice feature (although a bit difficult as different parameters of a node pool need to be updated with different API endpoints), I'm at first more concerned that node pools can be replaced in one go without creating an intermediate configuration with two node pools.

I mean, upgrading node pools with

version 1:

resource "google_container_node_pool" "old_pool" {
  ...
}

version 2:

resource "google_container_node_pool" "old_pool" {
  ...
}
resource "google_container_node_pool" "new_pool" {
  ...
}

version 3:

resource "google_container_node_pool" "new_pool" {
  ...
}

could be achieved with google_container_cluster through:
version 1:

resource "google_container_cluster" "cluster" {
  ...
  node_pool {
    name "pool-v1"
    node_config {
      ...
    }
  }
}

version 2:

resource "google_container_cluster" "cluster" {
  ...
  node_pool {
    name "pool-v2"
    node_config {
      ...
    }
  }
}

as long as the resource creates the new node pools before destroying the old ones during the delta. (Pods will be rescheduled on the new node pool assuming the scheduling constraints are met, assuming the capacity of pool-v2 is at least the same as pool-v1.)

As a second step, we can change the resource to update the node pools when possible (changing node versions is a different operation than changing node pool size, so updates to the two cannot be done at the same type. The resource needs to detect these and warn the user.

[paddycarver]

Hey everyone, this looks like it's getting lengthy and confusing. Let's see if we can't all get on the same page here.

@drzero42, it sounds like you're looking for GKE clusters and node pools to be importable. Is that accurate? It doesn't really matter what steps we take to get there, you're happy as long as the cluster and node pools are importable, right? (Assuming we don't lose current functionality, etc.)

There seems to be a separate, parallel conversation here about whether it makes sense to have a google_container_cluster resource and a google_container_node_pool resource. I can shed some light on that. This idea doesn't make sense until you think about Terraform's concept of ownership. Terraform "owns" the resources defined in its config, and will coerce your environment to match, or throw an error if it can't. By defining a google_container_cluster resource, you're putting the entire cluster under Terraform's control. Any node pools that exist in the cluster that aren't configured in Terraform are aberrant and should be removed, in Terraform's view. They're deviations to be corrected. For some users, this isn't what they actually want. They just want to say "at least these node pools should exist, but also, some other management tool will be adding its own node pools, or creating the cluster and I just want to add node pools to it". That's what google_container_node_pool is for; it limits the scope of Terraform's control to just the defined node pool, not the entire cluster.

Finally, there seems to be a conversation around this, as well:

The default pool config can only be configured through google_container_cluster while the non-defaut pools can only be configured through google_container_node_pool (until #299 gets merged.)

Why can't the default pool config be managed through google_container_node_pool by importing it after cluster creation? It's not nice, and it's definitely not ideal, but it is a workaround for the moment.

Managing only non-default pools through google_container_node_pool (until #299 gets merged) does seem to be the case, and is unfortunate. But there seems to be a straightforward fix for that pending.

So at the end of the day, I'm considering this issue to be mostly about "how do we get google_container_cluster and google_container_node_pool importable?" If I'm missing something, please let me know.

My thoughts on that are pretty straightforward: I think we can make them importable just as we would any other resource. Users shouldn't be using google_container_cluster and google_container_node_pool on the same cluster (perhaps updating the docs to make this clearer would be good?) because that doesn't make an awful lot of sense to me, ownership-wise: which resource owns the node pools? Which is the source of truth?

Of course, that comes with some problems. The biggest I can see right now is (e.g.) one team creating a cluster using Terraform, specifying no node pools, and another team creating a node pool under that cluster, using Terraform. The first team clearly owns the cluster and its administration, but wants to delegate the node pool ownership to the second team.

One way around this limitation would be for the cluster to set lifecycle.ignore_changes on the node_pools field, which would basically relinquish its ownership claim on that field.

This is, admittedly, all pretty confusing, and we should probably do a better job of documenting how these resources relate to each other.

Hopefully that clears some things up, and doesn't add to the confusion. And hopefully I actually have an accurate understanding of what's being discussed here. Please correct me if I don't. :)

[drzero42]

First, I should apologize for my latency - life called and demanded my attendence :)

@danawillow Yes, missing functionality definitely related to the API evolving, and nobody having gotten around to implement it yet. I didn't open this ticket to complain - I just wanted to get the train rolling on getting these things fixed and added :)

Wrt the separate node pool resources debate, if we can let the default pool be handled by the cluster resource and then add node pools with separate node pool resources, I don't have any issues with that. I just have a feeling it will be problematic to implement, since a describe/get call to the API for a cluster, will return a node pools list, which includes all node pools in the cluster. How does the code actually map that to resources, if those node pools could be either specified inside the cluster resource or as separate node pools? The way I see it, it would seem to be quite error-prone if we don't mandate either one or the other approach. I have no particular feelings towards one or the other - I just want to be able to import my current setups and start managing everything through Terraform ;)

Wrt changing node pool size, there is a limitation in the API that makes it difficult for us. I haven't found a way to actually retrieve the current number of nodes in a pool, only the initial count. Does anybody here have a way to contact the Container Engine team and request this information be added to the API? :)

@paddycarver Yes, this is getting lengthy and a bit confusing, but I think a lot of very good points have come from this debate so far :)
My objective is absolutely to get everything related to Container Engine to be importable and manageable in Terraform. I am happy no matter which way the provider chooses to implement the logic, as long is it doesn't limit my ability to utilize the service :)
I think you hit the nail on the head - ownership of the resources are definitely an issue, and needs to be thought through.

[danawillow]

For changing node pool size, I'm already working on it- the issue for that is #25 (please claim issues if you're working on them so we don't duplicate work!)

[coreypobrien]

Users shouldn't be using google_container_cluster and google_container_node_pool on the same cluster (perhaps updating the docs to make this clearer would be good?) because that doesn't make an awful lot of sense to me, ownership-wise: which resource owns the node pools? Which is the source of truth?

I'm not sure it makes much sense to only manage some of the infrastructure. Is the pattern you're suggesting that users create clusters manually and then use terraform to manage node pools? The expectation should be that terraform manages both because it should be able to manage an entire infrastructure.

I think the confusion is that gcloud and the console UI both do something automatic that isn't intuitive: they create a node pool. In my opinion, terraform should attempt to avoid that kind of automatic/hidden behavior.

The least surprising, most declarative implementation is that google_container_cluster should require the node_pool attribute to be a list of google_container_node_pool ids. Additionally, we should remove initial_node_count and node_config from google_container_cluster because they are only used to create that automatic/hidden node pool and are mutually exclusive with node_pool in the API.

[drzero42]

@coreypobrien I kind of agree, but if google_container_cluster resources reference google_container_node_pool ids, you create a dependency, which would have Terraform try to create node pools before the cluster exists. Because of the way Container Engine / the API works, node pools are "children" of container clusters, and as such can not exist without a cluster, which again can not exist without at least one node pool. In my opinion, this co-dependency makes it better to have just one resource type (google_container_cluster) with one or more node pools defined as subsections. I vote that the google_container_node_pool resource type be deprecated.

[paddycarver]

I'm not sure it makes much sense to only manage some of the infrastructure. Is the pattern you're suggesting that users create clusters manually and then use terraform to manage node pools? The expectation should be that terraform manages both because it should be able to manage an entire infrastructure.

I'm not so much suggesting a pattern, to be honest. I think more in terms of what users are trying to do:

• I just want a cluster, I'm going to manage it all in Terraform and all in one module and don't mind if changing the node pools destroys the cluster: I'd suggest google_container_cluster.
• I just want to let a module set up a node pool on a cluster ID I give it, but not control the whole cluster: I'd suggest google_container_node_pool.
• I want a cluster, and I want to be able to add/remove node pools without destroying and recreating the whole cluster: I'd suggest google_container_cluster with lifecycle.ignore_changes set to node_pools, and google_container_node_pool defining the pools.

If there's something people want that's not covered by that, I definitely want to hear about it, and we'll want to work out a solution for it. I also know that last suggestion isn't very pretty, and we're working on ways around it, but the ideal way is currently held up pending some core improvements.

I vote that the google_container_node_pool resource type be deprecated.

Could you help me understand why? What could you do if it was that you cannot do today?

My objective is absolutely to get everything related to Container Engine to be importable and manageable in Terraform. I am happy no matter which way the provider chooses to implement the logic, as long is it doesn't limit my ability to utilize the service :)

This, to me, is the crux of what this issue is asking for. And as far as I can tell, we can make google_container_cluster and google_container_node_pool importable without any of these changes being discussed.

Apologies, I think I'm just confused about why these changes are being proposed in the first place and what the goals are. I'd love to have more info on that.

[coreypobrien]

My use case is

• I want Terraform to completely manage the cluster and the node pools. I want to be able to add and remove node pools and without recreating the cluster. I want to be able to upgrade the version for the masters and change the list of add-ons without recreating the cluster.

Right now that isn't possible because of this idea that you can either manage node pools and ignore the cluster state, or managed the cluster but ignore the node pools.

In my opinion, designing Terraform to only partially manage infrastructure is counter-intuitive. The default for Terraform providers should be to expect to manage everything. Anything that isn't managed is an exception to the default.

It doesn't matter to me whether google_container_node_pool stays a separate resource or whether it is merged into an attribute of google_container_cluster as long as Terraform can manage all of the infrastructure and not either/or.

[drzero42]

@paddycarver The only reason I would vote to remove google_container_node_pool is to avoid ambiguity and confusion. I am completely with @coreypobrien in that I don't really care if it stays a seperate resource or gets merged into an attribute of google_container_cluster. As long as I can manage everything in Terraform, I am happy.

[rochdev]

I feel like keeping google_container_node_pool and removing the default node pool from google_container_cluster is the most flexible since it enables adding node pools dynamically from other modules. I think it's also much cleaner to have several node pool resources instead of a huge cluster definition.

I agree with @drzero42 however that any of the 2 approaches is better than the current broken behaviour.

[drebes]

I understand that's not possible: one can't create a container_cluster without the default node_pool, due to how the API works.

[coreypobrien]

According to the API docs i don't think you have to have the default node pool. I think the key is that if you specify the nodePool and do not specify nodeConfig or initialNodeCount you should get the specific node pool and not the default one. If you do specify the nodeConfig or initialNodeCount the default pool is created based on those.

[drebes]

You're right, I misunderstood @rochdev wanted to create a cluster without any pool, and manage the pools through google_container_node_pool.

[drzero42]

I just made #473, which might be yet another reason to change the current way of doing it. The node pools created by google_container_cluster don't have the same set of options available as the ones created by google_container_node_pool resources - in this case autoscaling. Seems to me it would be less confusing for everybody (including devs who want to add new features to node pools) to just choose one way of doing it - maintain all node pools for a cluster as proper google_container_node_pool resources (might be problematic due to the way Terraform works?) or maintain full config for all node pools inside of the google_container_cluster resource they are a part of.

[danawillow]

@drzero42, I just want to clarify something at a high level. When a resource in Terraform doesn't support a certain set of features, the reason is almost always going to be that we just haven't gotten around to it yet. Please give us the benefit of the doubt, and be assured that it is much easier to add features than it is to deprecate large portions of our codebase that likely already have users that would need to migrate 😃

[madmod]

I second what @drzero42 has said. I think having both node_pool and the default node_pool-like attributes at the container_cluster level is confusing and is causing inconsistencies in the API. (For example node_pool does not support labels while container_cluster does for the default node pool, node_pool only supports a single zone, and container_cluster does not support autoscaling (min/max) for the default node pool.) It also leaves some ambiguity such as does the initial_node_count for the cluster affect the initial_node_count for a node_pool? (It doesn't of course, but it isn't immediately obvious why you would need to specify that at a cluster level and node pool level.) What about master node count?

I think that the container_cluster.node_config and initial_node_count attributes should be deprecated in favor of requiring a node_pool resource and either having a required default_node_pool_name attribute and/or setting the first node_pool attribute to be the default.

I also think that keeping node_pool as a separate resource has the advantage of simplifying collecting outputs from the node_pool.

edit
Another thing I just noticed is that initial_node_count is required for both the node_pool resource and when referencing it in container_cluster.node_pool which is confusing and redundant.

edit 2 😄
I just realized that container_cluster.node_pool is another way of defining the node pool. I think that it should also be removed in favor of requiring separate node_pool resources because they are much more configurable.

[paddycarver]

Hi folks,

My gosh, this issue has gotten long and gone on a long time. I'm starting to have trouble following what exactly people are asking for, and I think it's because a lot of related, but different things are being asked for in this thread. In an effort to make sure everyone's feedback is taken into account and to make sure we can track actionable chunks of work, I'm going to close this issue and split it into several related issues, each tracking a more granularly-scoped problem.

Here's a breakdown of the new issues:

• If you want to manage all the node pools on your cluster in a single Terraform state file, and make sure none outside what you define in your config file are created, but your cluster is being destroyed and recreated when you don't think it needs to be, head to google_container_cluster fields recreate the cluster unnecesarily #474, which is tracking making the google_container_cluster resource's fields updatable instead of forcing a new resource on change.
• If you find the distinction between when to use google_container_cluster and when to use google_container_node_pool confusing, head to Unclear when to use google_container_cluster or google_container_node_pool #475, which is tracking how to better document that distinction.
• If you want to manage node pools using google_container_cluster but are missing options (either found in google_container_node_pool or missing from both) head to autoscaling options not available for default nodepool in google_container_cluster resource #473, which is tracking adding those options.

If there's a problem you're having that I didn't cover, feel free to open an issue for it. As you do so, keep in mind that it's definitely easiest for us to get on the same page if you tell us about the problem you're having. Things like "I want to be able to XXX but I can't because YYY" are incredibly helpful in this regard.

Finally, I notice a lot of proposals and votes to deprecate resources in this issue. I'm not going to comment on any specific resources, or any specific comments, but I think it's worth mentioning that our goal has always been to support the infrastructure design that makes sense to each org, not declare one right way to do it. We value everyone's feedback and ideas, and are listening carefully, but do also keep in mind that there are a lot of diverse organizations that use Terraform, and we want to meet everyone's needs the best we can.

[drzero42]

@danawillow I always give you the benefit of the doubt - I didn't mean anything disrespectful. My point was more that it seems like extra work to maintain the same kinds of features in multiple places :)
@paddycarver Thanks for the moderation - this issue was getting out of hand :)

[ghost]

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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!