terraform-provider-kubernetes icon indicating copy to clipboard operation
terraform-provider-kubernetes copied to clipboard

Is it intentional that order of metrics matter on HPAs?

Open marjorg opened this issue 3 years ago • 11 comments

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)

  • kubernetes_horizontal_pod_autoscaler
  • kubernetes_service
  • kubernetes_deployment

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

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment

marjorg avatar Mar 08 '21 14:03 marjorg

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.

jrhouston avatar May 05 '21 06:05 jrhouston

@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 avatar Jun 07 '21 23:06 joe-a-t

@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

joe-a-t avatar Aug 26 '21 23:08 joe-a-t

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

cohandv avatar Aug 27 '21 13:08 cohandv

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!

github-actions[bot] avatar Aug 28 '22 00:08 github-actions[bot]

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

joe-a-t avatar Aug 29 '22 14:08 joe-a-t

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!

github-actions[bot] avatar Aug 30 '23 00:08 github-actions[bot]

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

joe-a-t avatar Aug 30 '23 16:08 joe-a-t

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

BBBmau avatar Sep 14 '23 17:09 BBBmau