Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Do not delete / recreate cluster when changing node pool #424

Open
gibiansky opened this issue Apr 28, 2020 · 15 comments
Open

Do not delete / recreate cluster when changing node pool #424

gibiansky opened this issue Apr 28, 2020 · 15 comments
Labels
do-api Depends on changes to the DigitalOcean API

Comments

@gibiansky
Copy link

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
Copy link
Member

andrewsomething commented Jun 26, 2020

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: hashicorp/terraform-plugin-sdk#459 and hashicorp/terraform-plugin-sdk@1e08e98#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
Copy link

CapCap commented Jun 27, 2020

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
Copy link

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
Copy link

Would love this feature as well

@loarca
Copy link

loarca commented Nov 18, 2021

I'd LOVE this change, for real

@dvcrn
Copy link

dvcrn commented Jan 11, 2022

Scaling the default node pool in general is a bit funky. If I change the size of the default node pool I am not getting a full recreation, but a lot of these

dial tcp 127.0.0.1:80: connect: connection refused

Since it can't connect to the node pool anymore. I tried creating a separate node pool with digitalocean_kubernetes_node_pool, then changing the default node pool name to be identical to the created one in the hopes that it wouldn't break, but same connection refused error.

I don't mind doing manual steps like manually adding the new nodepool then removing the old one, but I couldn't find a proper way to tell the provider that this new node pool should be the default one

@mkjmdski
Copy link
Contributor

mkjmdski commented Mar 3, 2022

This is a real buzz killer that you need to recreate control plane to resize default node pool. I'd like to be able to create kubernetes cluster without default node pool, there is no way to perform any stable updates with current setup... Seems like this issue here is for very long can someone from @digitalocean take a look into that ?

@lemeurherve
Copy link

As a workaround, for Jenkins Infrastructure we went on a cluster with a minimal node pool just for keeping our cluster safe, and another autoscaled node pool which can be scaled and modified at ease.
It burns a little bit more resources but it's the only way we found to be able to tune our cluster nodes.
Allowing node pool to scale to zero would be greatly appreciated for this second autoscaled pool too, but that's another topic ^^

@mkjmdski
Copy link
Contributor

mkjmdski commented Mar 3, 2022

If somebody doesn't like to burn resources I've figured out how to do it :). According to the docs (https://registry.terraform.io/providers/digitalocean/digitalocean/latest/docs/resources/kubernetes_node_pool)

Note: If the node pool has the terraform:default-node-pool tag, then it is a default node pool for an existing cluster. 
The provider will refuse to import the node pool in that case because the node pool is managed by the digitalocean_kubernetes_cluster resource and not by this digitalocean_kubernetes_node_pool resource.

So I ended up writting code like that

resource "digitalocean_kubernetes_cluster" "cluster" {
  name          = var.name
  region        = var.region
  auto_upgrade  = true
  version       = data.digitalocean_kubernetes_versions.version.latest_version
  vpc_uuid      = var.vpc_uuid
  surge_upgrade = false

  node_pool {
    name       = format("%s-hacky", local.node_pool_name)
    size       = "s-1vcpu-1gb"
    node_count = 1
  }

  maintenance_policy {
    start_time = "04:00"
    day        = "monday"
  }

  lifecycle {
    ignore_changes = [
      node_pool.0
    ]
  }
}

resource "null_resource" "remove_cluster_node_pool" {
  triggers = {
    cluster_id = digitalocean_kubernetes_cluster.cluster.id
  }
  provisioner "local-exec" {
    command = format("%s/remove-default-node-pool.sh %s", path.module, digitalocean_kubernetes_cluster.cluster.id)
  }
}

resource "digitalocean_kubernetes_node_pool" "default_node_pool" {
  cluster_id = digitalocean_kubernetes_cluster.cluster.id
  name       = local.node_pool_name
  size       = var.size
  auto_scale = true
  min_nodes = 1
  max_nodes = 1
  tags       = local.tags
}

which creates node pool together with default node pool and after that null resource runs a script

#!/bin/bash
DOCTL_VERSION="1.70.0"
cluster_name="$1"
wget "https://github.com/digitalocean/doctl/releases/download/v${DOCTL_VERSION}/doctl-${DOCTL_VERSION}-linux-amd64.tar.gz"
tar xf "doctl-${DOCTL_VERSION}-linux-amd64.tar.gz"
if ./doctl --access-token $DIGITALOCEAN_TOKEN kubernetes cluster node-pool list "${cluster_name}" | grep -v 'hacky'; then
    node_pool_id=$(./doctl --access-token $DIGITALOCEAN_TOKEN kubernetes cluster node-pool list "${cluster_name}" | grep 'hacky' | awk '{print $1}')
    ./doctl --access-token $DIGITALOCEAN_TOKEN kubernetes cluster node-pool "${cluster_name}" "${node_pool_id}" --force
fi

it's just imporant that your local.tags contains terraform:default-node-pool. Now you need to manually remove cluster from state and reimport it. You should have now node pool separated from main cluster. This is super hacky but achieves the goal :)

@lemeurherve
Copy link

Interesting, thank very much for the detailed information @mkjmdski !

@darzanebor
Copy link

up

@flyte
Copy link
Contributor

flyte commented Feb 22, 2023

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

This would be preferable, as we may create a K8s cluster with some small shared-CPU instances, then need to scale up to general purpose nodes.
If we could simply create a new node pool, apply, then remove/scale to zero the original one, then this wouldn't really be a problem.
It's only such a big issue because we can't get rid of that default node pool completely and we're stuck with whatever node size we chose for the lifetime of the K8s cluster.

@flyte
Copy link
Contributor

flyte commented Feb 22, 2023

I was able to work around this manually by:

  1. Add new worker pool in DO UI
  2. Wait for nodes to become Ready in kubectl get nodes
  3. Remove old worker pool in DO UI
  4. Add the terraform:default-node-pool tag to the new worker pool in DO UI
  5. Update the worker size and pool name in the digitalocean_kubernetes_cluster resource's node_pool block
  6. Remove the k8s cluster from the Terraform state with terraform state rm digitalocean_kubernetes_cluster.k8s
  7. Import the k8s cluster with terraform import digitalocean_kubernetes_cluster.k8s <my k8s cluster id> using the cluster's ID that you can get from the DO UI
  8. Run terraform apply to make sure there are no further changes

I had some issues with the node_pool size being 0 but it may be because I renamed my node pool after creating it.

@ChiefMateStarbuck ChiefMateStarbuck added the do-api Depends on changes to the DigitalOcean API label Mar 28, 2023
@MatsKeWorks
Copy link

@ChiefMateStarbuck what is the status of this at the moment? There are currently a lot of issues regarding this problem.

@ChiefMateStarbuck
Copy link
Contributor

CC @andrewsomething @danaelhe

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-api Depends on changes to the DigitalOcean API
Projects
None yet
Development

No branches or pull requests