-
Notifications
You must be signed in to change notification settings - Fork 50
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 Tolerations and Affinity support #204
Add Tolerations and Affinity support #204
Conversation
charts/kubeai/values.yaml
Outdated
operator: "Equal" | ||
value: "present" | ||
effect: "NoSchedule" | ||
affinity: |
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 try to keep our default resource profile cloud agnostic. So I would prefer to remove the affinity rule
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.
Agreed - we can keep the tolerations though
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.
Ah yeah true. Let's keep the toleration.
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 think we can accomplish a resource profile that works across most popular cluster types by leveraging how affinity nodeSelectorTerms
are OR'd together instead of AND'd. Will open a new issue to track this. We should still remove the affinity & node selector in here for now.
Thanks for the PR. We do need this since adding taints for GPU nodes is common. I did an initial review but let's wait for @nstogner to review too. |
The quickstart e2e test CI is failing because kubeAI scaled down to 0:
|
@vlameiras Thanks for the PR! We were missing unit tests for the modified function so I added a PR into your branch to add those in as well as update the integration tests: vlameiras#1 |
Thanks for the quick replies and for the detailed comments! |
Please ignore my previous comment about e2e quickstart CI failing. It was flaky and I have fixed the flakiness here: #205 |
Simplify equality check and add tests
I think we are good to merge once the changes to the default |
Hi there 👋
We were testing the project internally and noticed including Tolerations and Affinity support would be nice.
Hopefully this PR will do the trick.
Thanks for the great project!