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

Removing container CPU resource limit is undetected by Terraform #754

Closed
coolbananas118 opened this issue Feb 3, 2020 · 14 comments
Closed
Labels
acknowledged Issue has undergone initial review and is in our work queue. bug needs investigation size/M

Comments

@coolbananas118
Copy link

coolbananas118 commented Feb 3, 2020

Using Terraform 0.11.14 (not the latest version but I feel this issue is a provider issue not a Terraform version issue).

provider "kubernetes" { version = "~> 1.10" }
Was previously on 1.7 and upgrading to 1.10 made no difference.

Expected Behaviour

Terraform should detect that the cpu limit has been removed and therefore remove the limit.

Actual Behaviour

If removal of the CPU limit is the only change then Terraform reports
Apply complete! Resources: 0 added, 0 changed, 0 destroyed.

If I make another change e.g. the image version then Terraform changes the version but the CPU limit is still set.

Only a "destroy" followed by an "apply" works but this is of course unacceptable.

Steps to Reproduce

Take note of the line which contains cpu = "${var.limits_cpu}"

`resource "kubernetes_deployment" "main" {
spec {
replicas = "${var.replicas}"

strategy = {
  type = "RollingUpdate"
  rolling_update {
    max_surge       = "${var.max_surge}"
    max_unavailable = "${var.max_unavailable}"
  }
}

template {
  spec {
    container = [
      {
        image = "XXXXX:${var.version}"
        name = "XXXXXX"
        liveness_probe {
          initial_delay_seconds = 60
          tcp_socket {
            port = 8080
          }
          timeout_seconds = 5
          period_seconds  = 5
        }
        readiness_probe {
          initial_delay_seconds = 30
          http_get {
            path = "/health"
            port = 8080
          }
          timeout_seconds = 5
          period_seconds  = 5
        }
        lifecycle {
          pre_stop {
            exec {
              command = ["/bin/sh", "-c", "sleep 10"]
            }
          }
        }
        resources {
          limits {
            cpu    = "${var.limits_cpu}"
            memory = "${var.limits_memory}"
          }...`

If I remove this cpu limit above it doesn't get detected.

@syntastical
Copy link

Just hit this today. Even setting the "cpu = null" doesn't do anything. The documentation does that the value cannot be updated but when I change it, it applies fine, it just cannot be removed.

@robinkb
Copy link
Contributor

robinkb commented Feb 18, 2020

This seems to be a problem with the entire resources block, not just the CPU-related fields.

@dak1n1 dak1n1 added size/M acknowledged Issue has undergone initial review and is in our work queue. labels Jun 18, 2020
@dak1n1
Copy link
Contributor

dak1n1 commented Jul 2, 2020

I reproduced this using latest versions and confirmed it's still an issue. I could modify resources.limits, but when I try to remove the whole block, it failed to make any changes to the existing deployment.

Terraform v0.12.28
+ provider.kubernetes v1.11.3

I have a reproducer here, in case it helps whoever picks up this issue:

resource "kubernetes_deployment" "example" {
  metadata {
    name = "terraform-example"
    labels = {
      test = "MyExampleApp"
    }
  }
  spec {
    replicas = 1
    selector {
      match_labels = {
        test = "MyExampleApp"
      }
    }
    template {
      metadata {
        labels = {
          test = "MyExampleApp"
        }
      }
      spec {
        container {
          name  = "example"
          image = "bash"
          command = ["/bin/sleep"]
          args = ["1200s"]

          resources {
            limits {
              cpu    = "500m"
              memory = "200Mi"
            }
            requests {
              cpu    = "250m"
              memory = "50Mi"
            }
          }
        }
      }
    }
  }
}

@graywolf-at-work
Copy link

Hit this today as well.

@dak1n1
Copy link
Contributor

dak1n1 commented Aug 18, 2020

We might be able to add some granularity to the resources attribute in the container spec, to allow for updates in certain cases. Here is an example that might be useful for beginning investigation. https://github.com/dak1n1/terraform-provider-kubernetes/blob/c245d421be2e2070e8cb61a03a6cc4f026d5c07c/kubernetes/resource_kubernetes_persistent_volume_claim.go#L48

@L3tum
Copy link

L3tum commented Sep 7, 2020

We've just run into this as well. I hope this gets some priority soon since it's already been more than half a year since this has been reported.

@ryan-cahill
Copy link

For anyone looking for a workaround here, my team solved the problem by setting cpu and memory to '0', which the provider does register as a change. In doing so, the Kubernetes QoS will be set back to BestEffort and the resources will become unbounded.

@jcreager
Copy link

jcreager commented Oct 27, 2020

I have also encountered this. I used the workaround suggested by @ryan-cahill, however, this is only a partial workaround. While the effect of setting the requests and limits to 0 will cause a pod to behave like no limits are set, the limits still exist. See this deployment snippet after applying:

        resources:
          limits:
            cpu: "0"
            memory: 256Mi
          requests:
            cpu: "0"
            memory: 256Mi

The problem with this is that one cannot specify the request only without destroying and re-applying the resource. Setting only the request value while setting the limit to 0 will result in the error below.

Failed to update deployment: Deployment.apps "<deployment-name>" is invalid: spec.template.spec.containers[0].resources.requests: Invalid value: "250m": must be less than or equal to cpu limit

Something to be aware of when employing this workaround. My understanding is that requests can be set without specifying limits. If this applies to your use-case the workaround described will not work.

@ryan-cahill
Copy link

Thanks for the extra details @jcreager, I should have been more specific in that I was only working with limits.

@jcreager
Copy link

No worries @ryan-cahill. The workaround was still valuable to me. I just wanted to make others aware of the limitations in the event they originally set both requests and limits as I had. In my case I set both to 0. Its not ideal but its workable in the short term.

@mantoine96
Copy link

Hey,

Just chiming in to note that this seems to be resolved since: a43d8ad

This change hasn't been released yet, but I was able to set/unset limits without issues with a provider compiled from master, or even from this specific commit.

This fix seems to be a side-effect from the original commit though! 🤔

@renatomjr
Copy link

renatomjr commented Jan 21, 2021

@thehunt33r Any plans to release a new version with this fix?

@dak1n1
Copy link
Contributor

dak1n1 commented Jan 21, 2021

Version 2 was released today, which contains the fix for this issue. https://github.com/hashicorp/terraform-provider-kubernetes/releases/tag/v2.0.0

@dak1n1 dak1n1 closed this as completed Jan 21, 2021
@ghost
Copy link

ghost commented Feb 21, 2021

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!

@ghost ghost locked as resolved and limited conversation to collaborators Feb 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
acknowledged Issue has undergone initial review and is in our work queue. bug needs investigation size/M
Projects
None yet
Development

No branches or pull requests