Terraform-provider-digitalocean: Do not delete / recreate cluster when changing node pool

[gibiansky]

When you create a Kubernetes cluster with one node pool, and then afterward change the node pool properties (e.g. size), it will delete the entire cluster.

This is similar to this issue. If you believe this is a duplicate issue, feel free to close this issue.

This behaviour is problematic because it messes up any authentication you might have, among other things.

It would be great if this would _add a node pool_, then delete the previous node pool, without deleting the cluster. This is possible via the DigitalOcean UI, though I don't know if there's a technical reason this behaviour isn't implemented or hard to implement.

Thanks!

[andrewsomething]

Just dropping some notes here in case anyone else takes a look at this one...

• The DigitalOcean API does not support resizing Droplets used in a node pool. As mentioned above, a new pool would need to be created and the old one then deleted. It must be done in that order as a cluster must have at least on node pool at all times.

• The ForceNew behavior for the cluster is inherited from the shared node pool schema. That can be change by doing:

--- a/digitalocean/resource_digitalocean_kubernetes_node_pool.go
+++ b/digitalocean/resource_digitalocean_kubernetes_node_pool.go
@@ -43,6 +43,9 @@ func nodePoolResourceSchema() map[string]*schema.Schema {
                ForceNew:     true,
        }

+       // Size should force a new node pool but not a new cluster
+       s["size"].ForceNew = true
+
        // remove the id when this is used in a specific resource
        // not as a child
        delete(s, "id")
@@ -65,7 +68,6 @@ func nodePoolSchema() map[string]*schema.Schema {
                "size": {
                        Type:         schema.TypeString,
                        Required:     true,
-                       ForceNew:     true,
                        ValidateFunc: validation.NoZeroValues,
                },

• With that done, the create and replace logic would need to be added to resourceDigitalOceanKubernetesClusterUpdate. Unfortunately, doing so here means you are outside of Terraform normal state management. The UX is arguably much worse than the current situation as it is very misleading. It will look like an in-place upgrade of the size attribute and not sufficiently convey that your nodes will be replaced completely. For example:

"Do you want to perform these actions?"
  # digitalocean_kubernetes_cluster.foo will be updated in-place
  ~ resource "digitalocean_kubernetes_cluster" "test" {

   # snip...  

      ~ node_pool {
            actual_node_count = 2
            auto_scale        = false
            id                = "348c6394-44d5-41e2-914a-721dfe759ed6"
            labels            = {}
            max_nodes         = 0
            min_nodes         = 0
            name              = "default"
            node_count        = 2
            nodes             = [
                {
                    created_at = "2020-06-25 18:19:23 +0000 UTC"
                    droplet_id = "197500730"
                    id         = "73a3aa23-d48f-4833-8e00-68e6ed9a908d"
                    name       = "default-3ypbn"
                    status     = "running"
                    updated_at = "2020-06-25 18:24:31 +0000 UTC"
                },
                {
                    created_at = "2020-06-25 18:19:23 +0000 UTC"
                    droplet_id = "197500732"
                    id         = "66f40ab6-ca25-4eb6-80be-145a91bb88cd"
                    name       = "default-3ypb3"
                    status     = "running"
                    updated_at = "2020-06-25 18:24:31 +0000 UTC"
                },
            ]
          ~ size              = "s-2vcpu-2gb" -> "s-1vcpu-2gb"
            tags              = []
        }

Nodes are first drained before being deleted. So in theory, depending on the work load, this might not cause down time if the new pool is up first. Though we shouldn't make assumptions about what is running in the cluster.

• This would seemingly be a good use case for SetNewComputed (directly or via ComputedIf). Using a CustomizeDiff, we could set both node_pool.0.id and node_pool.0.nodes to NewComputed to indicate they will change. Unfortunately, those will not work on nested attributes.

  See: https://github.com/hashicorp/terraform-plugin-sdk/issues/459 and https://github.com/hashicorp/terraform-plugin-sdk/commit/1e08e982731eb0c0a35fc00dcc3878bb1f790f70#diff-5572654f34bded06e0d3e5eb9ed7d1bf

  You can't set individual items in lists, you can only set the list as a whole, so we shouldn't allow sub-blocks to SetNew or SetNewComputed.

Using SetNewComputed or SetNew on the entire node_pool does not work as not all of the attributes are computed.

This will also complicates https://github.com/terraform-providers/terraform-provider-digitalocean/issues/303

[CapCap]

This is also critical for our use case as well :-)

Would it simplify anything to make the node_pool optional, and require there to be an associated digitalocean_kubernetes_node_pool resource?

[morgangrubb]

I have just tested and the digitalocean_kubernetes_node_pool resource behaves exactly the same way when changing the node size - that is, it destroys the pool and then creates a new one.

[tomjohnburton]

Would love this feature as well