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 vpa crs for keda-operator and metrics-server #562

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

QuantumEnigmaa
Copy link

This PR adds VPA CRs for both keda-operator and metrics-server.

Checklist

  • I have verified that my change is according to the deprecations & breaking changes policy
  • Commits are signed with Developer Certificate of Origin (DCO - learn more)
  • README is updated with new configuration values (if applicable) learn more
  • A PR is opened to update KEDA core (repo) (if applicable, ie. when deployment manifests are modified)

Fixes #

@QuantumEnigmaa QuantumEnigmaa requested a review from a team as a code owner November 9, 2023 08:57
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
docs/index.yaml Outdated Show resolved Hide resolved
keda/Chart.yaml Outdated Show resolved Hide resolved
keda/values.yaml Outdated Show resolved Hide resolved
@QuantumEnigmaa
Copy link
Author

I just noticed I forgot to sign my 2 previous commits :/

Copy link
Member

@tomkerkhove tomkerkhove left a comment

Choose a reason for hiding this comment

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

Can you remove the new artifacts please?

keda/values.yaml Outdated Show resolved Hide resolved
keda/README.md Outdated Show resolved Hide resolved
Signed-off-by: QuantumEnigmaa <[email protected]>
@QuantumEnigmaa
Copy link
Author

@tomkerkhove I reverted the changes on the artefacts

@@ -109,6 +109,11 @@ their default values.

| Parameter | Type | Default | Description |
|-----------|------|---------|-------------|
| `autoscaling.verticalPodAutoscaler.keda.cpu.maxAllowed` | int | `2` | |
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

Do you mean I can get rid of this change in the PR as it will be automatically be run by the ci jobs ?

@JorTurFer
Copy link
Member

I think that this is fascinating, but I'm not sure if it works correctly before the live pod resizing is in GA. I mean, restarting the operator for updating the pod size could produce scaling downtimes and we should clarify it IMHO. This also could generate impact on k8s api server on huge clusters if the VPA scales the pod so often because all the scalers cache has to be rebuilt on each restart, recovering all the needed info from the cluster

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