-
-
Notifications
You must be signed in to change notification settings - Fork 44
Add inftyai-scheduler support and config updates #447
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
Conversation
5aa2b0f to
719ccd4
Compare
Signed-off-by: carlory <[email protected]>
719ccd4 to
c6f907a
Compare
Makefile
Outdated
| helm: manifests kustomize helmify | ||
| $(KUSTOMIZE) build config/default | $(HELMIFY) -crd-dir | ||
| $(KUSTOMIZE) build config/default \ | ||
| | yq 'select(.kind and (.kind != "ConfigMap" or .metadata.name != "llmaz-global-config"))' \ |
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.
Better not to add too much logic to the makefile command, what's the advantages over the current one?
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.
Because the default value is default-scheduler in config/default/configmap.yaml and it can not be easily changed by user via helm install --set command.
I can not edit chart/template/global-config.yaml because it is generated by helmify. I want users to be able to easily use the chart without extra configuration.
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.
But it's not a general solution, we have a lot of configurations.
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'm ok to let user configure the configmaps via global.values.yaml. It's also not difficult.
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.
Can we switch the default scheduler to inftyai-scheduler in config/default/configmap.yaml? And add make install-inftyai-scheduler and make uninstall-inftyai-scheduler to the Makefile.
install-inftyai-scheduler:
kubectl apply --server-side -k config/inftyai-scheduler
.PHONY: uninstall-prometheus
uninstall-inftyai-scheduler:
kubectl delete -k config/inftyai-scheduler
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.
If not, we can not enable the inftyai-scheduler in chart by default due to the immutability of the configmap which is generated by helmify.
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'm ok to let user configure the configmaps via global.values.yaml. It's also not difficult.
This pr has supported it.
chart/templates/global-config.yaml
Outdated
| data: | ||
| config.data: {{ .Values.globalConfig.configData | toYaml | indent 1 }} | ||
| config.data: |- | ||
| {{- $base := deepCopy .Values.globalConfig.configData }} |
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.
What I mean is we may have several configurations in globalconfig, so this is not a general solution, I would suggest revert to the original configurations, and people can modify the chart/values.global.yaml before installation/upgrade, does that workaround?
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.
It works. Howerver, if an user want to use the scheduler, he have to do the following:
-
Get values from chart via
helm show values oci://registry-1.docker.io/inftyai/llmaz > /t mp/values.yaml -
Modify the values.yaml to enable the scheduler like this:
kube-scheduler:
enabled: true
globalConfig:
configData: |-
scheduler-name: inftyai-scheduler
# init-container-image: inftyai/model-loader:v0.0.10
- Install the chart with the modified values.yaml.
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.
We can not add this config to values.global.yaml in codebase because make helm-package appends (not merge) the file to values.yaml.
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.
Another drawback is that users need to provide the whole content of the configData when they want to change only one field because the globalConfig.configData is a string, not a map.
|
let's merge for now and optimize it later. Thanks @carlory |
What this PR does / why we need it
Add inftyai-scheduler support and config updates
Test it with a custom scheduler name.
Which issue(s) this PR fixes
Fixes #
Special notes for your reviewer
Does this PR introduce a user-facing change?