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

Add support for HPA Container Resource metrics #1647

Merged
merged 6 commits into from Mar 30, 2022

Conversation

arybolovlev
Copy link
Contributor

@arybolovlev arybolovlev commented Mar 16, 2022

Description

This PR adds support for HPA Container Resource metrics. More details in Kubernetes documentation.

Acceptance tests

  • Have you added an acceptance test for the functionality being added?
  • Have you run the acceptance tests on this branch?

Output from acceptance testing:

$ make testacc TESTARGS='-run ^TestAccKubernetesHorizontalPodAutoscalerV2Beta2*'

=== RUN   TestAccKubernetesHorizontalPodAutoscalerV2Beta2_basic
--- PASS: TestAccKubernetesHorizontalPodAutoscalerV2Beta2_basic (12.44s)
=== RUN   TestAccKubernetesHorizontalPodAutoscalerV2Beta2_containerResource
--- PASS: TestAccKubernetesHorizontalPodAutoscalerV2Beta2_containerResource (5.79s)
PASS
ok  	github.com/hashicorp/terraform-provider-kubernetes/kubernetes	18.748s

Release Note

Release note for CHANGELOG:

Add a new metric type `container_resource` in resource `kubernetes_horizontal_pod_autoscaler_v2beta2`.

References

#1637

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

@arybolovlev arybolovlev marked this pull request as ready for review March 18, 2022 11:44
@@ -53,6 +53,7 @@ func resourceKubernetesHorizontalPodAutoscalerV2Beta2() *schema.Resource {
"behavior": {
Type: schema.TypeList,
Description: "Behavior configures the scaling behavior of the target in both Up and Down directions (scale_up and scale_down fields respectively).",
Computed: true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How come this needs to be computed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I was working on this PR I noticed that terraform apply constantly produces a plan with changes related to the behavior attribute. Kubernetes uses default HPA Scaling rules if they are not set and the default values of these rules are part of the API response. That updates the state file with the behavior attribute after applying and causes the attribute deletion during the next plan.

Kubernetes documentation link: behavior ... If not set, the default HPAScalingRules for scale up and scale down are used.

You are right, it doesn't relate to the aim of this PR and shouldn't be here. Removed, thank you!

@jrhouston jrhouston changed the title Add support for HPA Container Resource metics Add support for HPA Container Resource metrics Mar 25, 2022
Copy link
Contributor

@jrhouston jrhouston left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work @arybolovlev, just a couple of things.

@@ -194,7 +225,7 @@ func metricSpecFields() *schema.Resource {
"type": {
Type: schema.TypeString,
Required: true,
Description: `type is the type of metric source. It should be one of "Object", "Pods", "External" or "Resource", each mapping to a matching field in the object.`,
Description: `type is the type of metric source. It should be one of "ContainerResource", "External", "Object", "Pods" or "Resource", each mapping to a matching field in the object. Note: "ContainerResource" type is available on when the feature-gate HPAContainerMetrics is enabled`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add validation to this attribute to check that it's one of the values documented here. You can use validation.StringInSlice

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @jrhouston! Since the current K8s API library supports only autoscaling/v2beta2, I have added it as it is. Once we bump up the version and autoscaling/v2 becomes available, I will update the code.

@@ -129,6 +129,48 @@ func TestAccKubernetesHorizontalPodAutoscalerV2Beta2_basic(t *testing.T) {
})
}

func TestAccKubernetesHorizontalPodAutoscalerV2Beta2_containerResource(t *testing.T) {
Copy link
Contributor

@jrhouston jrhouston Mar 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test will fail on clusters where the HPAContainerMetrics feature gate is not enabled. Right now there isn't a reliable way to check if a feature gate has been enabled programmatically. You have to check the --feature-gates args supplied to the apiserver, which we can't even do on EKS, GKE, et al.

For the moment I would add a t.Skip() to this test with a message explaining this. We can remove when the feature gate becomes defaulted to true.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added ErrorCheck with t.Skip(). In this case, the step will be executed and skipped only if it fails.

@arybolovlev arybolovlev merged commit bd0f076 into main Mar 30, 2022
@arybolovlev arybolovlev deleted the add-hpa-container-resource branch March 30, 2022 15:01
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

kubernetes_horizontal_pod_autoscaler_v2beta2: containerResource not support
2 participants