-
Notifications
You must be signed in to change notification settings - Fork 662
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
Implement Spark pod template overrides #4183
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #4183 +/- ##
==========================================
- Coverage 59.46% 59.26% -0.20%
==========================================
Files 638 552 -86
Lines 54375 39769 -14606
==========================================
- Hits 32336 23571 -8765
+ Misses 19490 13867 -5623
+ Partials 2549 2331 -218
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
ad57897
to
4fe1ae8
Compare
Signed-off-by: Andrew Dye <[email protected]>
4fe1ae8
to
b326791
Compare
flyteplugins/go/tasks/pluginmachinery/flytek8s/non_interruptible.go
Outdated
Show resolved
Hide resolved
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.
A few nit questions, otherwise looks good. Thanks!
Signed-off-by: Andrew Dye <[email protected]>
cf09a48
to
c5303f0
Compare
Signed-off-by: Andrew Dye <[email protected]>
c5303f0
to
6fc00ea
Compare
Signed-off-by: Andrew Dye <[email protected]>
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.
LGTM! Looks like this plugin might now be primed for GPU (and GPU affinity) support. A task for a later PR. Thanks!
Signed-off-by: Andrew Dye <[email protected]>
Signed-off-by: Andrew Dye <[email protected]>
…-pod-templates-plugins Signed-off-by: Andrew Dye <[email protected]>
Tracking issue
#4105
Describe your changes
Create driver, executor, and spark application spec from optional pod template. Uses ToK8sPodSpec as a shared solution for merging defaults with container or pod template target overrides. Driver and executor specs will share the same pod template values. See note below on this choice.
Why use the same pod template for driver and executor?
As part of #4179 we had a discussion about whether or not to use the task-level pod template, or introduce role-specific pod templates for driver and executor. For now this change adopts the task-level pod template pattern used by other k8s plugins. We may end up moving this and others to role-specific templates in the future.
Test plan
Unit tests
I refactored, extended, and renamed the existing container based Spark unit test (
TestBuildResourceContainer
). I added a newTestBuildResourcePodTemplate
test to cover most of the interface.Sanity check diff of driver and executor pods locally
spark
to enabled-plugins to the config following this guide$HOME/.flyte/cluster-resource-templates/
Verify behavior of specifying
@task(pod_template=...)
locallyModified the
my_spark
example to use a pod templateAnd verified that the resulting pod had tolerations set
Check all the applicable boxes