-
-
Notifications
You must be signed in to change notification settings - Fork 262
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
feat: allow user to scale more precisely with ContainerResource in hpa #725
base: master
Are you sure you want to change the base?
Conversation
@@ -20,17 +21,34 @@ spec: | |||
maxReplicas: {{ .Values.deployment.autoscaling.maxReplicas }} | |||
metrics: | |||
{{- with .Values.deployment.autoscaling.targetMemory }} | |||
{{- if and (and (not (empty $kubeVersion)) (semverCompare ">=1.30-0" $kubeVersion) (not (empty $.Values.deployment.autoscaling.extraMetrics))) }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to avoid breaking change for people already using extraContainers, we switch to containerResource only when it's available in kubernetes > 1.30 and Values.deployment.autoscaling.extraMetrics is defined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea, but not 100% sold on the implementation 🤔
I was wondering if we should not proceed on a cleaner path:
- define a new template for hpa, that is for k8s 1.30+ where we use only ContainerResources, as from my pov we should scale on the base of the core ory apps, and not sidecars
- then at 1.3x deprecate the older file and leave only ContainerResource scaling
- also, regardless of which path we choose, i would prefer to have it across all charts, and not only oathkeeper ;)
wdyt?
Related Issue or Design Document
Add possibility to define extra containers metrics in hpa to scale independently and more precisely when multiple containers are declared.
Checklist
If this pull request addresses a security vulnerability,
I confirm that I got approval (please contact [email protected]) from the maintainers to push the changes.
Further comments