hashicorp / terraform-provider-kubernetes

Terraform Kubernetes provider
https://www.terraform.io/docs/providers/kubernetes/
Mozilla Public License 2.0
1.59k stars 971 forks source link

Is it intentional that order of metrics matter on HPAs? #1188

Open marjorg opened 3 years ago

marjorg commented 3 years ago

Terraform Version, Provider Version and Kubernetes Version

Terraform version: v0.14.7
Kubernetes provider version: v2.0.2
Kubernetes version:  v1.19

Affected Resource(s)

Using this with AWS, not sure if that matters.

Terraform Configuration Files


resource "kubernetes_deployment" "my_app_deployment" {
    metadata {
        name = "my-app-deployment"
        labels = {
            app = "my-app"
        }
    }

    spec {
        revision_history_limit = 3

        selector {
            match_labels = {
                app = "my-app"
            }
        }

        template {
            metadata {
                labels = {
                    app = "my-app"
                }
            }

            spec {
                container {
                    name = "my-app"
                    image = "IMAGE_URL"
                    image_pull_policy = "Always"

                    port {
                        container_port = 80
                    }

                    resources {
                        requests = {
                            cpu = "200m"
                            memory = "200Mi"
                        }
                        limits = {
                            cpu    = "600m"
                            memory = "600Mi"
                        }
                    }
                }
            }
        }
    }
}

resource "kubernetes_service" "my_app_service" {
    metadata {
        name = "my-app-service"
    }

    spec {
        type = "ClusterIP"
        selector = {
            app = "my-app"
        }

        port {
            protocol = "TCP"
            port = 80
            target_port = 80
        }
    }
}

resource "kubernetes_horizontal_pod_autoscaler" "my_app_hpa" {
    metadata {
        name = "my-app-hpa"
    }

    spec {
        min_replicas = 1
        max_replicas = 5

        scale_target_ref {
            api_version = "apps/v1"
            kind = "Deployment"
            name = "my-app-deployment"
        }

        metric {
            type = "Resource"

            resource {
                name = "cpu"

                target {
                    type = "Utilization"
                    average_utilization = 60
                }
            }
        }

        metric {
            type = "Resource"

            resource {
                name = "memory"

                target {
                    type = "Utilization"
                    average_utilization = 60
                }
            }
        }

    }
}

Steps to Reproduce

  1. terraform apply (type yes)
  2. Run terraform apply again without changing config

Expected Behavior

No changes should be listed as the config is unchanged.

Actual Behavior

Even though config is unchanged the actions below are listed on every terraform apply. If i change the order of metrics in the above config the changes to list memory above cpu, this does not happen.

  # kubernetes_horizontal_pod_autoscaler.my_app_deployment will be updated in-place
  ~ resource "kubernetes_horizontal_pod_autoscaler" "my_app_hpa" {
        id = "default/myapp-hpa"

      ~ spec {
            # (3 unchanged attributes hidden)

          ~ metric {
                # (1 unchanged attribute hidden)

              ~ resource {
                  ~ name = "memory" -> "cpu"

                    # (1 unchanged block hidden)
                }
            }
          ~ metric {
                # (1 unchanged attribute hidden)

              ~ resource {
                  ~ name = "cpu" -> "memory"

                    # (1 unchanged block hidden)
                }
            }

            # (1 unchanged block hidden)
        }
        # (1 unchanged block hidden)
    }
Plan: 0 to add, 1 to change, 0 to destroy.

Community Note

jrhouston commented 3 years ago

It looks like the metrics do come back from the API in a different sort order – so we're missing a diff suppress function here.

Workaround is to reverse the order in the Terraform config.

joe-a-t commented 3 years ago

@jrhouston it doesn't look like reversing the order in the Terraform config so that memory is listed first and cpu is listed second makes a difference for me. One possible difference is that I am calling the kubernetes_horizontal_pod_autoscaler through a module and have dynamic metric blocks instead of hard coded. Is there any timeline or priority for this since it's pretty noisy and the workaround does not appear to work for people using dynamic metric blocks?

joe-a-t commented 3 years ago

@jrhouston is there any update on this? We are still experiencing the noise. Is this something where we could open a PR ourselves and get it reviewed? Is it literally just changing https://github.com/hashicorp/terraform-provider-kubernetes/blob/main/kubernetes/resource_kubernetes_horizontal_pod_autoscaler.go#L40 from TypeList to TypeSet? I can't tell if community PRs are accepted or not since there are currently 50 open PRs

cohandv commented 3 years ago

I also noticed there is no way to put the api_version that suppports multiple metrics

github-actions[bot] commented 2 years ago

Marking this issue as stale due to inactivity. If this issue receives no comments in the next 30 days it will automatically be closed. If this issue was automatically closed and you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. This helps our maintainers find and focus on the active issues. Maintainers may also remove the stale label at their discretion. Thank you!

joe-a-t commented 2 years ago

This issue is still a big problem, please address remove the stale label and address @jrhouston

github-actions[bot] commented 1 year ago

Marking this issue as stale due to inactivity. If this issue receives no comments in the next 30 days it will automatically be closed. If this issue was automatically closed and you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. This helps our maintainers find and focus on the active issues. Maintainers may also remove the stale label at their discretion. Thank you!

joe-a-t commented 1 year ago

This issue is still a big problem, please address remove the stale label and address @jrhouston

BBBmau commented 1 year ago

This is currently Blocked after discussing this issue further with @alexsomesan.

I attempted to solve this issue by switching to using Set instead of Lists, however this introduced another problem where an empty metric value gets added when attempting to Patch.

We discovered that the extra empty metric value comes from d.Get("metrics").(*Schema.Set) which comes from the terraform-plugin-sdk repo. Several issues are open that sound similar to the one being seen here, we'll come back to this issue once it has been addressed there.

https://github.com/hashicorp/terraform-plugin-sdk/issues/588 https://github.com/hashicorp/terraform-plugin-sdk/issues/1197

github-actions[bot] commented 1 month ago

Marking this issue as stale due to inactivity. If this issue receives no comments in the next 30 days it will automatically be closed. If this issue was automatically closed and you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. This helps our maintainers find and focus on the active issues. Maintainers may also remove the stale label at their discretion. Thank you!

tom-reinders commented 3 weeks ago

I don't have the ability to check this at the moment, but I can't find anything about it in the change logs so I assume this is still an issue.