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.

Sign up for GitHub

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

Jump to bottom

topology: add support for pod topology constraints #312

Merged
merged 1 commit into from
Sep 1, 2022
Merged

topology: add support for pod topology constraints #312

merged 1 commit into from
Sep 1, 2022

Conversation

erikkn
Copy link
Contributor

@erikkn erikkn commented Aug 29, 2022 •
edited by tomkerkhove

This PR adds support for Pod Topology Constraints for the keda-operator Pod.

Checklist

  • Commits are signed with Developer Certificate of Origin (DCO - learn more)
  • A PR is opened to update KEDA core (repo) (if applicable, ie. when deployment manifests are modified)
  • README is updated with new configuration values (if applicable)

Fixes #

@erikkn erikkn requested a review from a team as a code owner August 29, 2022 14:14
@erikkn erikkn force-pushed the operator-topology-constraints branch from b8ddae2 to 449c8b5 Compare August 29, 2022 14:16
@tomkerkhove
Copy link
Member

Any reason why we are not adding this for metric server as well?

@zroubalik Do you want to have this in CORE as well?

@erikkn
Copy link
Contributor Author

erikkn commented Aug 29, 2022

@tomkerkhove correct me if I am wrong, but based on the documentation, my understanding is that currently, only one replica of the keda-metrics-server is supported; in that case Pod Topology Constraints for this Pod is a bit useless, or do you disagree entirely :)?

@tomkerkhove
Copy link
Member

That is true, but same goes for the operator :) But it's not 100% the same limitation so it's fine, thanks for the PR!

@erikkn
Copy link
Contributor Author

erikkn commented Aug 29, 2022

The operator supports multiple (>=2) replicas that also does leader-election.

Please let me know if you want me to add support for the keda-metrics-server as well, very happy to add it.

@tomkerkhove
Copy link
Member

It's OK, we can leave metric server out

@JorTurFer
Copy link
Member

We can have more than 1 instance in the metrics server. Even the traffic could be routed to a single instance (upstream limitation), recently we added support to specify more than 1 instance in metrics server.
I said because maybe we should extend this PR to include metrics server

@erikkn
Copy link
Contributor Author

erikkn commented Aug 29, 2022

We can have more than 1 instance in the metrics server. Even the traffic could be routed to a single instance (upstream limitation), recently we added support to specify more than 1 instance in metrics server. I said because maybe we should extend this PR to include metrics server

Alrighty, I will add support for that later tonight/tomorrow, thanks!

@erikkn erikkn force-pushed the operator-topology-constraints branch 2 times, most recently from 890c3b8 to 512759e Compare August 30, 2022 14:29
@erikkn
Copy link
Contributor Author

erikkn commented Aug 30, 2022

@JorTurFer I added support for the metricsServer as well, you mind reviewing this PR :)?

@erikkn
topology: add support for pod topology constraints
440d619 
Signed-off-by: Erik Nobel <erik@nobel.info>

add constraint to metricsServer and change logic
@erikkn erikkn force-pushed the operator-topology-constraints branch from 512759e to 440d619 Compare August 30, 2022 14:51
tomkerkhove
tomkerkhove approved these changes Aug 30, 2022
View reviewed changes
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.

LGTM, but we need to update the README

keda/README.md
@@ -106,6 +106,7 @@ their default values.
| `resources.metricServer` | Manage resource request & limits of KEDA metrics apiserver pod ([docs](https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/)) | `` |
| `nodeSelector` | Node selector for pod scheduling ([docs](https://kubernetes.io/docs/concepts/scheduling-eviction/assign-pod-node/)) | `{}` |
| `tolerations` | Tolerations for pod scheduling ([docs](https://kubernetes.io/docs/concepts/scheduling-eviction/taint-and-toleration/)) | `{}` |
| topologySpreadConstraints | object | `{}` | Pod Topology Constraints https://kubernetes.io/docs/concepts/workloads/pods/pod-topology-spread-constraints/ |
Copy link
Member

Choose a reason for hiding this comment

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

This is not correct as the correct fields are topologySpreadConstraints.operator & topologySpreadConstraints.metricsServer

@JorTurFer JorTurFer merged commit 8778582 into kedacore:main Sep 1, 2022
@erikkn erikkn deleted the operator-topology-constraints branch September 1, 2022 14:21
@tomkerkhove tomkerkhove added this to the KEDA Core vNext milestone Sep 1, 2022
@JorTurFer JorTurFer changed the title topology: add support for pod topology constraints to operator topology: add support for pod topology constraints Sep 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomkerkhove tomkerkhove tomkerkhove approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
KEDA Core v2.8.2
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@erikkn @tomkerkhove @JorTurFer