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

feat: template scheduler configs #209

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

marwanad
Copy link
Contributor

This change templates the proxy and candidate scheduler configs in values. It's useful for cases when a non-default plugin config is required.

Copy link
Contributor

@adrienjt adrienjt left a comment

Choose a reason for hiding this comment

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

In general, templated values are sort of an anti-pattern actually, and making all of the KubeSchedulerConfiguration a value removes a lot of the abstraction provided by the chart. In practice, what are you trying to change specifically? Could the plugins section be the value?

charts/multicluster-scheduler/templates/cm.yaml Outdated Show resolved Hide resolved
@marwanad
Copy link
Contributor Author

@adrienjt the one thing I'm trying to change is appending a pluginConfig to adjust the scoringStrategy for NodeResourcesFit. The goal is to achieve as much binpacking as possible for the GPU resource.

I think we can probably do better, just by templating the pluginConfig section. I am planning on moving the hardcoded context durations to be part of the plugin arguments in another PR so this will play nicely with that change as well.

Copy link
Contributor

@adrienjt adrienjt left a comment

Choose a reason for hiding this comment

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

How would you test this? Can you for example set scoringStrategy in

h $i upgrade --install admiralty charts/multicluster-scheduler -n admiralty --create-namespace -f $VALUES \
--set controllerManager.image.repository=admiralty-agent \
--set scheduler.image.repository=admiralty-scheduler \
--set postDeleteJob.image.repository=admiralty-remove-finalizers \
--set restarter.image.repository=admiralty-restarter \
--set controllerManager.image.tag=$VERSION-amd64 \
--set scheduler.image.tag=$VERSION-amd64 \
--set postDeleteJob.image.tag=$VERSION-amd64 \
--set restarter.image.tag=$VERSION-amd64
k $i delete pod --all -n admiralty
and confirm that it works with an e2e test?

@marwanad marwanad force-pushed the helm-chart-restructure branch 13 times, most recently from 850af94 to 6aa1a3b Compare August 12, 2024 03:49
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.

None yet

2 participants