Skip to content
This repository has been archived by the owner on Oct 9, 2023. It is now read-only.

Add non-interruptible node selector requirement to spark driver if set #346

Merged
merged 2 commits into from
Apr 25, 2023

Conversation

jeevb
Copy link
Contributor

@jeevb jeevb commented Apr 25, 2023

TL;DR

Spark driver pods should always run as non-interruptible. However, we do not apply the non-interruptible-node-selector-requirement node selector if set. As such, these pods may be scheduled incorrectly on interruptible nodes.

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Complete description

Always apply node selector requirements for non-interruptible to spark driver pods.

@jeevb jeevb requested a review from hamersaw April 25, 2023 05:06
@jeevb jeevb marked this pull request as draft April 25, 2023 05:10
@codecov
Copy link

codecov bot commented Apr 25, 2023

Codecov Report

Merging #346 (7d2b06c) into master (1f39163) will increase coverage by 1.43%.
The diff coverage is 100.00%.

❗ Current head 7d2b06c differs from pull request most recent head 3c886d1. Consider uploading reports for the commit 3c886d1 to get more accurate results

@@            Coverage Diff             @@
##           master     #346      +/-   ##
==========================================
+ Coverage   62.64%   64.08%   +1.43%     
==========================================
  Files         148      148              
  Lines       12397    10073    -2324     
==========================================
- Hits         7766     6455    -1311     
+ Misses       4036     3023    -1013     
  Partials      595      595              
Flag Coverage Δ
unittests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...o/tasks/plugins/k8s/kfoperators/pytorch/pytorch.go 80.00% <100.00%> (+7.65%) ⬆️
go/tasks/plugins/k8s/spark/spark.go 79.37% <100.00%> (+2.05%) ⬆️

... and 129 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@@ -236,6 +236,9 @@ func (sparkResourceHandler) BuildResource(ctx context.Context, taskCtx pluginsCo
j.Spec.MainClass = &sparkJob.MainClass
}

// Add non-interruptible node selector requirements to driver pod
Copy link
Contributor

@hamersaw hamersaw Apr 25, 2023

Choose a reason for hiding this comment

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

I would just update the comment to specify that driver pods should never be interruptible so we hardcode to non-interruptible to apply relevant node selectors if they exist.

Signed-off-by: Jeev B <[email protected]>
@jeevb jeevb force-pushed the jeev/spark-non-interruptible-driver branch from bd6d12d to 3c886d1 Compare April 25, 2023 16:05
@jeevb jeevb marked this pull request as ready for review April 25, 2023 16:49
@jeevb jeevb requested a review from hamersaw April 25, 2023 16:50
@jeevb jeevb merged commit 63e1e45 into master Apr 25, 2023
@jeevb jeevb deleted the jeev/spark-non-interruptible-driver branch April 25, 2023 17:50
eapolinario pushed a commit that referenced this pull request Sep 6, 2023
#346)

* Add non-interruptible node selector requirement to spark driver if set

Signed-off-by: Jeev B <[email protected]>

* comment

Signed-off-by: Jeev B <[email protected]>

---------

Signed-off-by: Jeev B <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants