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

Fix metrics hpa diffbug #2286

Closed
wants to merge 2 commits into from
Closed

Fix metrics hpa diffbug #2286

wants to merge 2 commits into from

Conversation

BBBmau
Copy link
Contributor

@BBBmau BBBmau commented Sep 13, 2023

Description

This PR changes the attribute type of metrics in horizontal pod autoscaler to Set in order to prevent Diffs from appearing despite their not being any change in the tfconfig. (Diff comes from out of order metrics list, using a set type for metrics would prevent this.)

Fixes #1188

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_basic"

...

Release Note

Release note for CHANGELOG:

`kubernetes/resource_horizontal_pod_autoscaler.go`: Change Lists to Set in metrics to prevent out of order metrics Diff

References

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

@BBBmau BBBmau requested a review from a team as a code owner September 13, 2023 22:14
@BBBmau
Copy link
Contributor Author

BBBmau commented Sep 13, 2023

I'll need some advice on how to handle a bug when changing from Lists to Sets

It looks to solve the problem however I noticed that when attempting to patch the resource, it produces an empty metric value in the set. Resulting in

 Error: Failed to update horizontal pod autoscaler: HorizontalPodAutoscaler.autoscaling "my-app-hpa" is invalid: [spec.metrics[0].type: Required value: must specify a metric source type, spec.metrics[0].type: Unsupported value: "": supported values: "ContainerResource", "External", "Object", "Pods", "Resource"]

due to their being an empty metric block being added to the set despite not their being one in the config. This can be seen when applying the configs from TestAccKubernetesHorizontalPodAutoscalerV2Beta2_basic

  1. Running basic config
  2. Running modified config

will result in the Error.

I'm not sure where exactly the empty metric block is being added in the Update function. I'll need some input on this. @jrhouston @alexsomesan

@BBBmau
Copy link
Contributor Author

BBBmau commented Sep 14, 2023

This is currently Blocked after discussing this issue further with @alexsomesan. We discovered that the extra empty metric value comes from d.Get("metrics").(*Schema.Set) which comes from the terraform-plugin-sdk repo. I'll be making an issue in that repo bringing up the block that I'm facing for fixing the metrics HPA Diff Bug

@jrhouston
Copy link
Collaborator

Instead of changing the type here, couldn't we just use a DiffSuppressFunc on this attribute, sort the old and new values and if they're the same then suppress the diff?

@BBBmau
Copy link
Contributor Author

BBBmau commented Aug 13, 2024

Closing this since I don't intend to work on this PR. In the future I'd want to start over and go at it in a different way.

@BBBmau BBBmau closed this Aug 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Is it intentional that order of metrics matter on HPAs?
2 participants