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

Start using autoscaling/v2 for HorizontalPodAutoscaler #414

Conversation

GuyKomari
Copy link
Contributor

@GuyKomari GuyKomari commented Dec 5, 2022

What this PR does / why we need it:

Problem: chart-testing is failing for:
"autoscaling/v2beta2 HorizontalPodAutoscaler is deprecated in v1.23+, unavailable in v1.26+; use autoscaling/v2"

https://github.com/cortexproject/cortex-helm-chart/actions/runs/3606103570/jobs/6077123598

Solution: update charts to start using autoscaler/v2

Signed-off-by: guykomari guykomari@gmail.com

@GuyKomari GuyKomari force-pushed the guykomari/update-autoscaling-apiversion branch from c58d0d4 to 4c04611 Compare December 5, 2022 21:16
Copy link
Collaborator

@nschad nschad left a comment

Choose a reason for hiding this comment

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

We should do something like this until 1.26+. To keep functionality with older clusters.

{/*
Determine the policy api version
*/}}
{{- define "cortex.pdbVersion" -}}
{{- if or (.Capabilities.APIVersions.Has "policy/v1/PodDisruptionBudget") (semverCompare ">=1.21" .Capabilities.KubeVersion.Version) -}}
policy/v1
{{- else -}}
policy/v1beta1
{{- end -}}
{{- end -}}

Copy link
Collaborator

@nschad nschad 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, however please put the code in templates/_helpers.tpl global helpers file to avoid unnecessary code duplication just like with the pdbversion.

Then you can refer to it like this

apiVersion: {{ include "cortex.hpaVersion" . }}

On a side note: We should also check if something changed between v1beta1 and v1 (but I guess it shouldn't have?)

@GuyKomari GuyKomari force-pushed the guykomari/update-autoscaling-apiversion branch from 7ad269b to 44bbbb2 Compare December 6, 2022 15:12
templates/_helpers.tpl Outdated Show resolved Hide resolved
@GuyKomari
Copy link
Contributor Author

Thank you, however please put the code in templates/_helpers.tpl global helpers file to avoid unnecessary code duplication just like with the pdbversion.

Then you can refer to it like this

apiVersion: {{ include "cortex.hpaVersion" . }}

On a side note: We should also check if something changed between v1beta1 and v1 (but I guess it shouldn't have?)

by the docs, it seems that nothing has changed.

@GuyKomari GuyKomari requested a review from nschad December 6, 2022 17:30
Get cortex hpa version by k8s version
*/}}
{{- define "cortex.hpaVersion" -}}
{{- if or (.Capabilities.APIVersions.Has "autoscaling/v2") (semverCompare ">=1.23" .Capabilities.KubeVersion.Version) -}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
{{- if or (.Capabilities.APIVersions.Has "autoscaling/v2") (semverCompare ">=1.23" .Capabilities.KubeVersion.Version) -}}
{{- if or (.Capabilities.APIVersions.Has "autoscaling/v2/HorizontalPodAutoscaler") (semverCompare ">=1.23" .Capabilities.KubeVersion.Version) -}}

Should be this

@GuyKomari GuyKomari force-pushed the guykomari/update-autoscaling-apiversion branch from 8a1d286 to 58b3fe8 Compare December 7, 2022 08:07
Copy link
Collaborator

@nschad nschad left a comment

Choose a reason for hiding this comment

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

lgtm

@nschad nschad requested a review from kd7lxl December 7, 2022 08:08
@nschad nschad added the enhancement New feature or request label Dec 7, 2022
@nschad
Copy link
Collaborator

nschad commented Dec 7, 2022

@GuyKomari Check the Lint Test Job for error logs. There is some error evaluating the helm-chart.

Also please update the CHANGELOG.md

  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@GuyKomari GuyKomari force-pushed the guykomari/update-autoscaling-apiversion branch from 58b3fe8 to 53440c4 Compare December 7, 2022 08:59
@GuyKomari
Copy link
Contributor Author

@GuyKomari Check the Lint Test Job for error logs. There is some error evaluating the helm-chart.

Also please update the CHANGELOG.md

  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

I ran the ct lint locally and it passes now, so the github action should pass now.

What this Commit does / why we need it:

Problem: chart-testing is failing for:
"autoscaling/v2beta2 HorizontalPodAutoscaler is deprecated in v1.23+,
unavailable in v1.26+; use autoscaling/v2"

https://github.com/cortexproject/cortex-helm-chart/actions/runs/3606103570/jobs/6077123598

Solution: update charts to start using autoscaler/v2 in v1.23+

Signed-off-by: guykomari <guykomari@gmail.com>
@GuyKomari GuyKomari force-pushed the guykomari/update-autoscaling-apiversion branch from 53440c4 to 3b22074 Compare December 7, 2022 09:07
@GuyKomari GuyKomari requested review from nschad and removed request for kd7lxl December 7, 2022 09:11
@GuyKomari
Copy link
Contributor Author

GuyKomari commented Dec 7, 2022

@nschad once we merge this pr, when do u think we can also merge #413
and create a new release?

@nschad nschad requested a review from kd7lxl December 7, 2022 13:32
Copy link
Collaborator

@kd7lxl kd7lxl left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@nschad nschad merged commit 1385145 into cortexproject:master Dec 8, 2022
@nschad nschad mentioned this pull request Dec 15, 2022
1 task
SuperMatt pushed a commit to SuperMatt/cortex-helm-chart that referenced this pull request Jan 6, 2023
…texproject#414)

What this Commit does / why we need it:

Problem: chart-testing is failing for:
"autoscaling/v2beta2 HorizontalPodAutoscaler is deprecated in v1.23+,
unavailable in v1.26+; use autoscaling/v2"

https://github.com/cortexproject/cortex-helm-chart/actions/runs/3606103570/jobs/6077123598

Solution: update charts to start using autoscaler/v2 in v1.23+

Signed-off-by: guykomari <guykomari@gmail.com>

Signed-off-by: guykomari <guykomari@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants