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

[prometheus-statsd-exporter] - add conditional for hpa apiVersion #3635

Merged
merged 4 commits into from
Aug 4, 2023

Conversation

tylenwells
Copy link
Contributor

What this PR does / why we need it

autoscaling/v2beta1 api for horizontalPodAutoscalers was removed in k8s 1.25
This PR adds a conditional to use autoscaling/v2 API version if the k8s version of your cluster is 1.25 or greater.

Which issue this PR fixes

(optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged)

  • fixes #

Special notes for your reviewer

@scDisorder - @-mentioning you per the instructions above. Please review when you have the time, thank you!

Checklist

  • DCO signed
  • Chart Version bumped
  • Title of the PR starts with chart name (e.g. [prometheus-couchdb-exporter])

Copy link
Member

@zeritti zeritti left a comment

Choose a reason for hiding this comment

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

Thank you, @tylenwells, for your PR. Please, see my comment below.

charts/prometheus-statsd-exporter/templates/hpa.yaml Outdated Show resolved Hide resolved
@oshaughnessy
Copy link

i'm running into issues with autoscaling/v2beta1 -- i see warnings in my K8S 1.25 clusters and failures in 1.26. i'd love to see a solution merged in, whether or not the code is optimal. not ideal, perhaps, but how about approving & merging as-is and updating with better code as a separate PR? :-)

@zeritti zeritti requested a review from zanhsieh August 3, 2023 12:05
@oshaughnessy
Copy link

@zeritti, that's a nice change. thanks for giving this some attention!

@zeritti zeritti merged commit 7932a53 into prometheus-community:main Aug 4, 2023
@tylenwells
Copy link
Contributor Author

Leaving a note here for anyone who comes across this:

This change allows for the updated apiVersion to be used, but doesn't account for the change in spec formatting for the new apiVersion. #3681 takes care of that.

old ver:

metrics:
   - type: Resource
      resource:
        name: cpu
        targetAverageUtilization:  {{ .Values.autoscaling.targetMemoryUtilizationPercentage }}

new ver:

metrics:
    - type: Resource
      resource:
        name: memory
        target:
          type: Utilization
          averageUtilization: {{ .Values.autoscaling.targetMemoryUtilizationPercentage }}

Matiasmct pushed a commit to giffgaff/prometheus-charts-backup that referenced this pull request Aug 25, 2023
…ometheus-community#3635)

* add k8s verison condiitonal to hpa template apiVerison mapping

Signed-off-by: Tylen Wells <[email protected]>

* bump chart version

Signed-off-by: Tylen Wells <[email protected]>

* Add template for setting HPA API version

Signed-off-by: zeritti <[email protected]>

---------

Signed-off-by: Tylen Wells <[email protected]>
Signed-off-by: zeritti <[email protected]>
Co-authored-by: Tylen Wells <[email protected]>
Co-authored-by: zeritti <[email protected]>
Co-authored-by: MH <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants