You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by skonto <gi...@git.apache.org> on 2018/11/25 20:50:41 UTC
[GitHub] spark pull request #23136: [SPARK-25515][K8s] Adds a config option to keep e...
GitHub user skonto opened a pull request:
https://github.com/apache/spark/pull/23136
[SPARK-25515][K8s] Adds a config option to keep executor pods for debuging
## What changes were proposed in this pull request?
Keeps K8s executor resources present if case of failure or normal termination.
Introduces a new boolean config option: `spark.kubernetes.deleteExecutors`, with default value set to true.
The idea is to update Spark K8s backend structures but leave the resources around.
The assumption is that since entries are not removed from the `removedExecutorsCache` we are immune to updates that refer to the the executor resources previously removed.
The only delete operation not touched is the one in the `doKillExecutors` method.
Reason is right now we dont support [blacklisting](https://issues.apache.org/jira/browse/SPARK-23485) and dynamic allocation with Spark on K8s. In both cases in the future we might want to handle these scenarios although its more complicated.
## How was this patch tested?
Manually by running a Spark job and verifying pods are not deleted.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/skonto/spark keep_pods
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/23136.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #23136
----
commit 5cdca3167e2f8acb84a23f9c64e5c3bc524f04ac
Author: Stavros Kontopoulos <st...@...>
Date: 2018-11-25T20:18:42Z
add config option to keep executor pods
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23136: [SPARK-25515][K8s] Adds a config option to keep executor...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/23136
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-make-spark-distribution-unified/5466/
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23136: [SPARK-25515][K8s] Adds a config option to keep executor...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/23136
**[Test build #99619 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/99619/testReport)** for PR 23136 at commit [`6857f5e`](https://github.com/apache/spark/commit/6857f5eba6671016d7c2277902cb9f9488835565).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23136: [SPARK-25515][K8s] Adds a config option to keep executor...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/23136
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/99343/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23136: [SPARK-25515][K8s] Adds a config option to keep executor...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/23136
**[Test build #99343 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/99343/testReport)** for PR 23136 at commit [`3be89cc`](https://github.com/apache/spark/commit/3be89ccf7bcb06e4dbf7248ebdce9312545c56c4).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23136: [SPARK-25515][K8s] Adds a config option to keep executor...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/23136
**[Test build #99619 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/99619/testReport)** for PR 23136 at commit [`6857f5e`](https://github.com/apache/spark/commit/6857f5eba6671016d7c2277902cb9f9488835565).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23136: [SPARK-25515][K8s] Adds a config option to keep executor...
Posted by liyinan926 <gi...@git.apache.org>.
Github user liyinan926 commented on the issue:
https://github.com/apache/spark/pull/23136
LGTM. Will merge to master shortly.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23136: [SPARK-25515][K8s] Adds a config option to keep executor...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/23136
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23136: [SPARK-25515][K8s] Adds a config option to keep executor...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/23136
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/5420/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23136: [SPARK-25515][K8s] Adds a config option to keep executor...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/23136
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/5414/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #23136: [SPARK-25515][K8s] Adds a config option to keep e...
Posted by skonto <gi...@git.apache.org>.
Github user skonto commented on a diff in the pull request:
https://github.com/apache/spark/pull/23136#discussion_r236456591
--- Diff: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorPodsAllocator.scala ---
@@ -86,11 +88,14 @@ private[spark] class ExecutorPodsAllocator(
s" cluster after $podCreationTimeout milliseconds despite the fact that a" +
" previous allocation attempt tried to create it. The executor may have been" +
" deleted but the application missed the deletion event.")
- Utils.tryLogNonFatalError {
- kubernetesClient
- .pods()
- .withLabel(SPARK_EXECUTOR_ID_LABEL, execId.toString)
- .delete()
+
+ if (shouldDeleteExecutors) {
+ Utils.tryLogNonFatalError {
+ kubernetesClient
+ .pods()
+ .withLabel(SPARK_EXECUTOR_ID_LABEL, execId.toString)
--- End diff --
Ok let's make that configurable too.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23136: [SPARK-25515][K8s] Adds a config option to keep executor...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/23136
**[Test build #99392 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/99392/testReport)** for PR 23136 at commit [`3be89cc`](https://github.com/apache/spark/commit/3be89ccf7bcb06e4dbf7248ebdce9312545c56c4).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23136: [SPARK-25515][K8s] Adds a config option to keep executor...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/23136
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/5334/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23136: [SPARK-25515][K8s] Adds a config option to keep executor...
Posted by skonto <gi...@git.apache.org>.
Github user skonto commented on the issue:
https://github.com/apache/spark/pull/23136
tests failed but not due to this PR.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23136: [SPARK-25515][K8s] Adds a config option to keep executor...
Posted by skonto <gi...@git.apache.org>.
Github user skonto commented on the issue:
https://github.com/apache/spark/pull/23136
@liyinan926 updated the PR,here is the [jira](https://issues.apache.org/jira/browse/SPARK-26256) for the bug wil create a PR for it shortly.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23136: [SPARK-25515][K8s] Adds a config option to keep executor...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/23136
Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-make-spark-distribution-unified/5414/
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23136: [SPARK-25515][K8s] Adds a config option to keep executor...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/23136
**[Test build #99336 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/99336/testReport)** for PR 23136 at commit [`3e77d53`](https://github.com/apache/spark/commit/3e77d53a53a53025e1271e2aa5c0597297fac994).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #23136: [SPARK-25515][K8s] Adds a config option to keep e...
Posted by liyinan926 <gi...@git.apache.org>.
Github user liyinan926 commented on a diff in the pull request:
https://github.com/apache/spark/pull/23136#discussion_r236455846
--- Diff: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorPodsAllocator.scala ---
@@ -86,11 +88,14 @@ private[spark] class ExecutorPodsAllocator(
s" cluster after $podCreationTimeout milliseconds despite the fact that a" +
" previous allocation attempt tried to create it. The executor may have been" +
" deleted but the application missed the deletion event.")
- Utils.tryLogNonFatalError {
- kubernetesClient
- .pods()
- .withLabel(SPARK_EXECUTOR_ID_LABEL, execId.toString)
- .delete()
+
+ if (shouldDeleteExecutors) {
+ Utils.tryLogNonFatalError {
+ kubernetesClient
+ .pods()
+ .withLabel(SPARK_EXECUTOR_ID_LABEL, execId.toString)
--- End diff --
We should also add labels `SPARK_APP_ID_LABEL` and `SPARK_ROLE_LABEL`.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23136: [SPARK-25515][K8s] Adds a config option to keep executor...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/23136
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23136: [SPARK-25515][K8s] Adds a config option to keep executor...
Posted by liyinan926 <gi...@git.apache.org>.
Github user liyinan926 commented on the issue:
https://github.com/apache/spark/pull/23136
retest this please
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23136: [SPARK-25515][K8s] Adds a config option to keep executor...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/23136
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23136: [SPARK-25515][K8s] Adds a config option to keep executor...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/23136
**[Test build #99343 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/99343/testReport)** for PR 23136 at commit [`3be89cc`](https://github.com/apache/spark/commit/3be89ccf7bcb06e4dbf7248ebdce9312545c56c4).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #23136: [SPARK-25515][K8s] Adds a config option to keep e...
Posted by skonto <gi...@git.apache.org>.
Github user skonto commented on a diff in the pull request:
https://github.com/apache/spark/pull/23136#discussion_r237643173
--- Diff: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorPodsAllocator.scala ---
@@ -86,11 +88,14 @@ private[spark] class ExecutorPodsAllocator(
s" cluster after $podCreationTimeout milliseconds despite the fact that a" +
" previous allocation attempt tried to create it. The executor may have been" +
" deleted but the application missed the deletion event.")
- Utils.tryLogNonFatalError {
- kubernetesClient
- .pods()
- .withLabel(SPARK_EXECUTOR_ID_LABEL, execId.toString)
- .delete()
+
+ if (shouldDeleteExecutors) {
+ Utils.tryLogNonFatalError {
+ kubernetesClient
+ .pods()
+ .withLabel(SPARK_EXECUTOR_ID_LABEL, execId.toString)
--- End diff --
@liyinan926 ok. In general it is not a big issue but for tracking purposes it would help.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23136: [SPARK-25515][K8s] Adds a config option to keep executor...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/23136
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/99336/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #23136: [SPARK-25515][K8s] Adds a config option to keep e...
Posted by skonto <gi...@git.apache.org>.
Github user skonto commented on a diff in the pull request:
https://github.com/apache/spark/pull/23136#discussion_r236089683
--- Diff: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/KubernetesClusterSchedulerBackend.scala ---
@@ -82,11 +85,13 @@ private[spark] class KubernetesClusterSchedulerBackend(
pollEvents.stop()
}
- Utils.tryLogNonFatalError {
- kubernetesClient.pods()
- .withLabel(SPARK_APP_ID_LABEL, applicationId())
- .withLabel(SPARK_ROLE_LABEL, SPARK_POD_EXECUTOR_ROLE)
- .delete()
+ if (shouldDeleteExecutors) {
+ Utils.tryLogNonFatalError {
+ kubernetesClient.pods()
+ .withLabel(SPARK_APP_ID_LABEL, applicationId())
+ .withLabel(SPARK_ROLE_LABEL, SPARK_POD_EXECUTOR_ROLE)
+ .delete()
+ }
--- End diff --
safest scenario as the backend stops all executors before pod deletion.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #23136: [SPARK-25515][K8s] Adds a config option to keep e...
Posted by skonto <gi...@git.apache.org>.
Github user skonto commented on a diff in the pull request:
https://github.com/apache/spark/pull/23136#discussion_r236488306
--- Diff: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorPodsAllocator.scala ---
@@ -86,11 +88,14 @@ private[spark] class ExecutorPodsAllocator(
s" cluster after $podCreationTimeout milliseconds despite the fact that a" +
" previous allocation attempt tried to create it. The executor may have been" +
" deleted but the application missed the deletion event.")
- Utils.tryLogNonFatalError {
- kubernetesClient
- .pods()
- .withLabel(SPARK_EXECUTOR_ID_LABEL, execId.toString)
- .delete()
+
+ if (shouldDeleteExecutors) {
+ Utils.tryLogNonFatalError {
+ kubernetesClient
+ .pods()
+ .withLabel(SPARK_EXECUTOR_ID_LABEL, execId.toString)
--- End diff --
I just run multiple jobs in parallel:
```
NAME READY STATUS RESTARTS AGE
spark-pi-1543281035622-driver 1/1 Running 0 2m
spark-pi-1543281035622-exec-1 1/1 Running 0 1m
spark-pi-1543281098418-driver 0/1 Completed 0 1m
spark-pi-1543281107591-driver 0/1 Completed 0 57s
```
The long running was not terminated...
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23136: [SPARK-25515][K8s] Adds a config option to keep executor...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/23136
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23136: [SPARK-25515][K8s] Adds a config option to keep executor...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/23136
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/99392/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23136: [SPARK-25515][K8s] Adds a config option to keep executor...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/23136
Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-make-spark-distribution-unified/5466/
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23136: [SPARK-25515][K8s] Adds a config option to keep executor...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/23136
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-make-spark-distribution-unified/5334/
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23136: [SPARK-25515][K8s] Adds a config option to keep executor...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/23136
**[Test build #99245 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/99245/testReport)** for PR 23136 at commit [`5cdca31`](https://github.com/apache/spark/commit/5cdca3167e2f8acb84a23f9c64e5c3bc524f04ac).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #23136: [SPARK-25515][K8s] Adds a config option to keep e...
Posted by liyinan926 <gi...@git.apache.org>.
Github user liyinan926 commented on a diff in the pull request:
https://github.com/apache/spark/pull/23136#discussion_r237626777
--- Diff: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorPodsAllocator.scala ---
@@ -86,11 +88,14 @@ private[spark] class ExecutorPodsAllocator(
s" cluster after $podCreationTimeout milliseconds despite the fact that a" +
" previous allocation attempt tried to create it. The executor may have been" +
" deleted but the application missed the deletion event.")
- Utils.tryLogNonFatalError {
- kubernetesClient
- .pods()
- .withLabel(SPARK_EXECUTOR_ID_LABEL, execId.toString)
- .delete()
+
+ if (shouldDeleteExecutors) {
+ Utils.tryLogNonFatalError {
+ kubernetesClient
+ .pods()
+ .withLabel(SPARK_EXECUTOR_ID_LABEL, execId.toString)
--- End diff --
Agreed this bug fix should be in a separate PR and should be backport to 2.4. @skonto can you create a separate PR with a Jira for the bug fix? Thanks!
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #23136: [SPARK-25515][K8s] Adds a config option to keep e...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/23136#discussion_r236431997
--- Diff: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorPodsLifecycleManagerSuite.scala ---
@@ -100,6 +101,18 @@ class ExecutorPodsLifecycleManagerSuite extends SparkFunSuite with BeforeAndAfte
verify(schedulerBackend).doRemoveExecutor("1", expectedLossReason)
}
+ test("When an executor reaches error states immediately and the user has set the flag" +
--- End diff --
could you write shorter test names? that makes it easier to actually differentiate tests.
e.g. "keep executor pods in k8s if configured" says the same thing in a lot less words.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23136: [SPARK-25515][K8s] Adds a config option to keep executor...
Posted by liyinan926 <gi...@git.apache.org>.
Github user liyinan926 commented on the issue:
https://github.com/apache/spark/pull/23136
@mccheah do you have any comment?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #23136: [SPARK-25515][K8s] Adds a config option to keep e...
Posted by onursatici <gi...@git.apache.org>.
Github user onursatici commented on a diff in the pull request:
https://github.com/apache/spark/pull/23136#discussion_r237534100
--- Diff: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorPodsAllocator.scala ---
@@ -86,11 +88,14 @@ private[spark] class ExecutorPodsAllocator(
s" cluster after $podCreationTimeout milliseconds despite the fact that a" +
" previous allocation attempt tried to create it. The executor may have been" +
" deleted but the application missed the deletion event.")
- Utils.tryLogNonFatalError {
- kubernetesClient
- .pods()
- .withLabel(SPARK_EXECUTOR_ID_LABEL, execId.toString)
- .delete()
+
+ if (shouldDeleteExecutors) {
+ Utils.tryLogNonFatalError {
+ kubernetesClient
+ .pods()
+ .withLabel(SPARK_EXECUTOR_ID_LABEL, execId.toString)
--- End diff --
It seems like this change fixes an existing bug. Should this be a separate PR?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23136: [SPARK-25515][K8s] Adds a config option to keep executor...
Posted by skonto <gi...@git.apache.org>.
Github user skonto commented on the issue:
https://github.com/apache/spark/pull/23136
Fixed that failing test.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23136: [SPARK-25515][K8s] Adds a config option to keep executor...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/23136
**[Test build #99245 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/99245/testReport)** for PR 23136 at commit [`5cdca31`](https://github.com/apache/spark/commit/5cdca3167e2f8acb84a23f9c64e5c3bc524f04ac).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #23136: [SPARK-25515][K8s] Adds a config option to keep e...
Posted by onursatici <gi...@git.apache.org>.
Github user onursatici commented on a diff in the pull request:
https://github.com/apache/spark/pull/23136#discussion_r237534125
--- Diff: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorPodsLifecycleManager.scala ---
@@ -30,7 +30,7 @@ import org.apache.spark.scheduler.ExecutorExited
import org.apache.spark.util.Utils
private[spark] class ExecutorPodsLifecycleManager(
- conf: SparkConf,
+ val conf: SparkConf,
--- End diff --
is exposing this fine? maybe we can create a new `eventHandlerUnderTest` in the new test below as a workaround
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23136: [SPARK-25515][K8s] Adds a config option to keep executor...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/23136
**[Test build #99392 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/99392/testReport)** for PR 23136 at commit [`3be89cc`](https://github.com/apache/spark/commit/3be89ccf7bcb06e4dbf7248ebdce9312545c56c4).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23136: [SPARK-25515][K8s] Adds a config option to keep executor...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/23136
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/5466/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23136: [SPARK-25515][K8s] Adds a config option to keep executor...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/23136
Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-make-spark-distribution-unified/5420/
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23136: [SPARK-25515][K8s] Adds a config option to keep executor...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/23136
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-make-spark-distribution-unified/5674/
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #23136: [SPARK-25515][K8s] Adds a config option to keep e...
Posted by skonto <gi...@git.apache.org>.
Github user skonto commented on a diff in the pull request:
https://github.com/apache/spark/pull/23136#discussion_r236485835
--- Diff: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorPodsAllocator.scala ---
@@ -86,11 +88,14 @@ private[spark] class ExecutorPodsAllocator(
s" cluster after $podCreationTimeout milliseconds despite the fact that a" +
" previous allocation attempt tried to create it. The executor may have been" +
" deleted but the application missed the deletion event.")
- Utils.tryLogNonFatalError {
- kubernetesClient
- .pods()
- .withLabel(SPARK_EXECUTOR_ID_LABEL, execId.toString)
- .delete()
+
+ if (shouldDeleteExecutors) {
+ Utils.tryLogNonFatalError {
+ kubernetesClient
+ .pods()
+ .withLabel(SPARK_EXECUTOR_ID_LABEL, execId.toString)
--- End diff --
Correct here is a sample output:
```Labels: spark-app-selector=spark-application-1543242814683
spark-exec-id=2
spark-role=executor
```
Weird that didnt see one job interfering with the other before.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23136: [SPARK-25515][K8s] Adds a config option to keep executor...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/23136
**[Test build #99336 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/99336/testReport)** for PR 23136 at commit [`3e77d53`](https://github.com/apache/spark/commit/3e77d53a53a53025e1271e2aa5c0597297fac994).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23136: [SPARK-25515][K8s] Adds a config option to keep executor...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/23136
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-make-spark-distribution-unified/5420/
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23136: [SPARK-25515][K8s] Adds a config option to keep executor...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/23136
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23136: [SPARK-25515][K8s] Adds a config option to keep executor...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/23136
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/5674/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23136: [SPARK-25515][K8s] Adds a config option to keep executor...
Posted by skonto <gi...@git.apache.org>.
Github user skonto commented on the issue:
https://github.com/apache/spark/pull/23136
@mccheah gentle ping.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #23136: [SPARK-25515][K8s] Adds a config option to keep e...
Posted by liyinan926 <gi...@git.apache.org>.
Github user liyinan926 commented on a diff in the pull request:
https://github.com/apache/spark/pull/23136#discussion_r236455466
--- Diff: docs/running-on-kubernetes.md ---
@@ -926,6 +926,13 @@ specific to Spark on Kubernetes.
<code>spark.kubernetes.executor.podTemplateFile=/path/to/executor-pod-template.yaml`</code>
</td>
</tr>
+<tr>
+ <td><code>spark.kubernetes.deleteExecutors</code></td>
--- End diff --
Should this be named `spark.kubernetes.executor.deleteOnTermination` so the style is consistent with other executor-related options?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23136: [SPARK-25515][K8s] Adds a config option to keep executor...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/23136
Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-make-spark-distribution-unified/5334/
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23136: [SPARK-25515][K8s] Adds a config option to keep executor...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/23136
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #23136: [SPARK-25515][K8s] Adds a config option to keep e...
Posted by skonto <gi...@git.apache.org>.
Github user skonto commented on a diff in the pull request:
https://github.com/apache/spark/pull/23136#discussion_r236432458
--- Diff: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorPodsLifecycleManagerSuite.scala ---
@@ -100,6 +101,18 @@ class ExecutorPodsLifecycleManagerSuite extends SparkFunSuite with BeforeAndAfte
verify(schedulerBackend).doRemoveExecutor("1", expectedLossReason)
}
+ test("When an executor reaches error states immediately and the user has set the flag" +
--- End diff --
Yeah that was a draft name, I can change it.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23136: [SPARK-25515][K8s] Adds a config option to keep executor...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/23136
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #23136: [SPARK-25515][K8s] Adds a config option to keep e...
Posted by skonto <gi...@git.apache.org>.
Github user skonto commented on a diff in the pull request:
https://github.com/apache/spark/pull/23136#discussion_r237642388
--- Diff: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorPodsLifecycleManager.scala ---
@@ -30,7 +30,7 @@ import org.apache.spark.scheduler.ExecutorExited
import org.apache.spark.util.Utils
private[spark] class ExecutorPodsLifecycleManager(
- conf: SparkConf,
+ val conf: SparkConf,
--- End diff --
@onursatici I think its a small bug fix that can be include dhere, I can update the PR description if that helps. I thought of creating a new one but exposing that value was easy for testing purposes.
I don't expect that to be a problem unless someone tries to modify SparkConf at the backend when he shouldnt. I dont thinkg it is not a big concern.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23136: [SPARK-25515][K8s] Adds a config option to keep executor...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/23136
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-make-spark-distribution-unified/5414/
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23136: [SPARK-25515][K8s] Adds a config option to keep executor...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/23136
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/99619/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23136: [SPARK-25515][K8s] Adds a config option to keep executor...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/23136
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #23136: [SPARK-25515][K8s] Adds a config option to keep e...
Posted by liyinan926 <gi...@git.apache.org>.
Github user liyinan926 commented on a diff in the pull request:
https://github.com/apache/spark/pull/23136#discussion_r236461893
--- Diff: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorPodsAllocator.scala ---
@@ -86,11 +88,14 @@ private[spark] class ExecutorPodsAllocator(
s" cluster after $podCreationTimeout milliseconds despite the fact that a" +
" previous allocation attempt tried to create it. The executor may have been" +
" deleted but the application missed the deletion event.")
- Utils.tryLogNonFatalError {
- kubernetesClient
- .pods()
- .withLabel(SPARK_EXECUTOR_ID_LABEL, execId.toString)
- .delete()
+
+ if (shouldDeleteExecutors) {
+ Utils.tryLogNonFatalError {
+ kubernetesClient
+ .pods()
+ .withLabel(SPARK_EXECUTOR_ID_LABEL, execId.toString)
--- End diff --
I think it's fine to do this later, i.e., making sure that we use consistent sets of labels when listing and deleting pods.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #23136: [SPARK-25515][K8s] Adds a config option to keep e...
Posted by skonto <gi...@git.apache.org>.
Github user skonto commented on a diff in the pull request:
https://github.com/apache/spark/pull/23136#discussion_r236457049
--- Diff: docs/running-on-kubernetes.md ---
@@ -926,6 +926,13 @@ specific to Spark on Kubernetes.
<code>spark.kubernetes.executor.podTemplateFile=/path/to/executor-pod-template.yaml`</code>
</td>
</tr>
+<tr>
+ <td><code>spark.kubernetes.deleteExecutors</code></td>
--- End diff --
No problem I can add change to that seems more readable if we remove driver pods as well.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23136: [SPARK-25515][K8s] Adds a config option to keep executor...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/23136
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/99245/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #23136: [SPARK-25515][K8s] Adds a config option to keep e...
Posted by liyinan926 <gi...@git.apache.org>.
Github user liyinan926 commented on a diff in the pull request:
https://github.com/apache/spark/pull/23136#discussion_r236463330
--- Diff: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorPodsAllocator.scala ---
@@ -86,11 +88,14 @@ private[spark] class ExecutorPodsAllocator(
s" cluster after $podCreationTimeout milliseconds despite the fact that a" +
" previous allocation attempt tried to create it. The executor may have been" +
" deleted but the application missed the deletion event.")
- Utils.tryLogNonFatalError {
- kubernetesClient
- .pods()
- .withLabel(SPARK_EXECUTOR_ID_LABEL, execId.toString)
- .delete()
+
+ if (shouldDeleteExecutors) {
+ Utils.tryLogNonFatalError {
+ kubernetesClient
+ .pods()
+ .withLabel(SPARK_EXECUTOR_ID_LABEL, execId.toString)
--- End diff --
Actually just realized that `execId` is just an integer incremented whenever a new executor pod gets created. We definitely need to add the two labels to make sure we don't accidentally delete executor pods with the same `execId` from a different application.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23136: [SPARK-25515][K8s] Adds a config option to keep executor...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/23136
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23136: [SPARK-25515][K8s] Adds a config option to keep executor...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/23136
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #23136: [SPARK-25515][K8s] Adds a config option to keep executor...
Posted by skonto <gi...@git.apache.org>.
Github user skonto commented on the issue:
https://github.com/apache/spark/pull/23136
@erikerlandson @mccheah pls review.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #23136: [SPARK-25515][K8s] Adds a config option to keep e...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/23136
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org