[SPARK-35956][K8S] Support auto assigning labels to decommissioning pods #33270

holdenk · 2021-07-08T17:35:01Z

What changes were proposed in this pull request?

Add a new configuration flag to allow Spark to provide hints to the scheduler when we are decommissioning or exiting a pod that this pod will have the least impact for a pre-emption event.

Why are the changes needed?

Kubernetes added the concepts of pod disruption budgets (which can have selectors based on labels) as well pod deletion for providing hints to the scheduler as to what we would prefer to have pre-empted.

Does this PR introduce any user-facing change?

New configuration flag

How was this patch tested?

The deletion unit test was extended.

SparkQA · 2021-07-08T18:26:52Z

Test build #140814 has finished for PR 33270 at commit e6e0ae4.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2021-07-08T18:53:46Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/45325/

SparkQA · 2021-07-08T19:25:38Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/45325/

Yikun · 2021-07-09T17:07:37Z

resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/Config.scala

@@ -284,6 +284,24 @@ private[spark] object Config extends Logging {
      .toSequence
      .createWithDefault(Nil)

+  val KUBERNETES_EXECUTOR_POD_DECOMMISSION_LABEL =


doc updating for new config: docs/running-on-kubernetes.md.

Good call :)

Yikun · 2021-07-09T17:11:36Z

resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/Config.scala

+  val KUBERNETES_EXECUTOR_POD_DECOMMISSION_LABEL_VALUE =
+    ConfigBuilder("spark.kubernetes.executor.pod.decommmissionLabelValue")
+      .doc("Label value to apply to a pod which is being decommissioned." +
+        " Designed for use with pod disruption budgets and similar mechanism " +


ditto, redundant space

Yikun · 2021-07-09T17:13:45Z

resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/Config.scala

+  val KUBERNETES_EXECUTOR_POD_DECOMMISSION_LABEL =
+    ConfigBuilder("spark.kubernetes.executor.pod.decommmissionLabel")
+      .doc("Label to apply to a pod which is being decommissioned." +
+        " Designed for use with pod disruption budgets and similar mechanism " +


nit: redundant space in line end.

Yikun · 2021-07-12T14:41:13Z

resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/Config.scala

+      .doc("Label value to apply to a pod which is being decommissioned." +
+        " Designed for use with pod disruption budgets and similar mechanism " +
+        " such as pod-deletion-cost.")
+      .version("3.2.0")


And maybe 3.3.0 for now?

Yeah good point

dongjoon-hyun · 2021-07-15T07:15:32Z

Sorry for being late, @holdenk . I was on vacation last week.

dongjoon-hyun · 2021-07-15T07:20:31Z

...rc/main/scala/org/apache/spark/scheduler/cluster/k8s/KubernetesClusterSchedulerBackend.scala

@@ -181,7 +184,51 @@ private[spark] class KubernetesClusterSchedulerBackend(
    super.getExecutorIds()
  }

+  private def labelLowPriorityExecs(execIds: Seq[String]) = {


The naming labelLowPriorityExecs looks misleading to me because the pods already have PriorityClass.

Good point. We could say labelExecsForDecommissioning?

Sounds good to me.

dongjoon-hyun · 2021-07-15T07:30:33Z

resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/Config.scala

+        " such as pod-deletion-cost.")
+      .version("3.2.0")
+      .stringConf
+      .createWithDefault("")


Shall we use createOptional?

So my thought was that for a labels value we'd use an empty string if the user doesn't specify a value, but I'm down for something else.

Yes, we can still attach LABEL without values with createOptional conf.

So I don't have strong feelings about this one, but the label does need a value (albiet the empty string is a valid value), so I've switched to use optional and then we do a getOrElse on it but I can switch it back.

...st/scala/org/apache/spark/scheduler/cluster/k8s/KubernetesClusterSchedulerBackendSuite.scala

dongjoon-hyun

Could you address the review comments from @Yikun ? I also added some comments.

cc @attilapiros

...rc/main/scala/org/apache/spark/scheduler/cluster/k8s/KubernetesClusterSchedulerBackend.scala

SparkQA · 2021-07-15T17:52:05Z

Test build #141090 has finished for PR 33270 at commit 937732e.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2021-07-15T18:44:11Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/45604/

SparkQA · 2021-07-15T19:22:52Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/45604/

dongjoon-hyun · 2021-07-15T20:43:02Z

Thank you for updating, @holdenk .

attilapiros

I think we could extend the integration tests of DecommissionSuite as well with the decommmissionLabel and decommmissionLabelValue configs and at the end of the tests we could check whether it is set. WDYT?

...st/scala/org/apache/spark/scheduler/cluster/k8s/KubernetesClusterSchedulerBackendSuite.scala

holdenk · 2021-07-20T17:22:21Z

Yeah I think we can extend that test suite to pick it up as well so we aren't just depending on mocks/unit tests. I'll give that shot.

holdenk · 2021-07-20T17:56:52Z

Ah now I'm remembering why I didn't change the DecommissionSuite it's because we do K8s side decommissioning by triggering a delete event instead of Spark side decommissioning. But we do trigger a Spark side decom in one sub test so I'll add it there.

SparkQA · 2021-07-20T18:16:23Z

Test build #141353 has finished for PR 33270 at commit 83be9be.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2021-07-20T18:53:00Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/45867/

SparkQA · 2021-07-20T19:27:26Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/45867/

SparkQA · 2021-07-21T01:18:39Z

Test build #141368 has finished for PR 33270 at commit e460e52.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2021-07-21T01:57:05Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/45884/

SparkQA · 2021-07-21T02:37:29Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/45884/

dongjoon-hyun · 2021-07-21T17:17:44Z

Could you take a look at K8s IT failure? You may want to re-trigger it simply.

- Test decommissioning with dynamic allocation & shuffle cleanups *** FAILED ***
  The code passed to eventually never returned normally. Attempted 70 times over 2.0080748925166665 minutes. Last failure message: Map("spark-app-locator" -> "5131e7f3300942dba80c225eb409f5f1", "spark-app-selector" -> "spark-d1019d7e577b40adbab20a965952b5d0", "spark-exec-id" -> "1", "spark-exec-resourceprofile-id" -> "0", "spark-role" -> "executor") did not contain key "solong". (DecommissionSuite.scala:120)

SparkQA · 2021-07-21T18:27:40Z

Test build #141434 has finished for PR 33270 at commit bd9ae50.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

Increase timeouts for running in CI.

Bump up timeout

…L_VALUE as suggested by dongjoon.

SparkQA · 2021-07-22T18:17:00Z

Test build #141511 has finished for PR 33270 at commit bb52a4c.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun

Like the existing Spark label configuration spark.kubernetes.executor.label., shall we use spark.kubernetes.executor.decommmissionLabel instead of adding a new namespace .pod.?

SparkQA · 2021-07-22T18:47:05Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/46028/

dongjoon-hyun · 2021-07-22T18:47:33Z

docs/running-on-kubernetes.md

+</tr>
+<tr>
+  <td><code>spark.kubernetes.executor.pod.decommmissionLabelValue<code></td>
+  <td>""</td>


"" -> (none).

Thanks good catch.

SparkQA · 2021-07-22T19:20:46Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/46028/

dongjoon-hyun

+1, LGTM (with two minor comments). K8s IT passed, too.

Avoiding .pod. namespace from the conf naming.
Fixing the default value for decommmissionLabelValue.

Thank you, @holdenk . After revising them, feel free to merge.

SparkQA · 2021-07-23T01:20:39Z

Test build #141525 has finished for PR 33270 at commit 15d32ba.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2021-07-23T01:48:57Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/46043/

SparkQA · 2021-07-23T02:32:34Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/46043/

SparkQA · 2021-07-23T03:23:08Z

Test build #141533 has finished for PR 33270 at commit f7d85fa.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2021-07-23T03:54:13Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/46051/

SparkQA · 2021-07-23T04:28:45Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/46051/

attilapiros

I went through it again and I haven't found any issue.

LGTM

holdenk · 2021-07-23T22:23:07Z

Merged to the current master.

### What changes were proposed in this pull request? This is a follow-up of #33270 to fix typos in config names ```scala - ConfigBuilder("spark.kubernetes.executor.decommmissionLabel") + ConfigBuilder("spark.kubernetes.executor.decommissionLabel") - ConfigBuilder("spark.kubernetes.executor.decommmissionLabelValue") + ConfigBuilder("spark.kubernetes.executor.decommissionLabelValue") ``` ### Why are the changes needed? To fix them before Apache Spark 3.3 branch cut. ### Does this PR introduce _any_ user-facing change? No, this is not released yet. ### How was this patch tested? Pass the CIs. Closes #35767 from dongjoon-hyun/SPARK-35956. Authored-by: Dongjoon Hyun <dongjoon@apache.org> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

github-actions bot added the KUBERNETES label Jul 8, 2021

holdenk requested a review from dongjoon-hyun July 9, 2021 16:55

Yikun reviewed Jul 9, 2021

View reviewed changes

Yikun reviewed Jul 12, 2021

View reviewed changes

dongjoon-hyun reviewed Jul 15, 2021

View reviewed changes

...st/scala/org/apache/spark/scheduler/cluster/k8s/KubernetesClusterSchedulerBackendSuite.scala Outdated Show resolved Hide resolved

dongjoon-hyun requested changes Jul 15, 2021

View reviewed changes

attilapiros reviewed Jul 15, 2021

View reviewed changes

...rc/main/scala/org/apache/spark/scheduler/cluster/k8s/KubernetesClusterSchedulerBackend.scala Outdated Show resolved Hide resolved

github-actions bot added the DOCS label Jul 15, 2021

attilapiros reviewed Jul 16, 2021

View reviewed changes

...st/scala/org/apache/spark/scheduler/cluster/k8s/KubernetesClusterSchedulerBackendSuite.scala Outdated Show resolved Hide resolved

...st/scala/org/apache/spark/scheduler/cluster/k8s/KubernetesClusterSchedulerBackendSuite.scala Show resolved Hide resolved

holdenk force-pushed the SPARK-35956-support-auto-assigning-labels-to-decommissioning-pods branch from 937732e to 83be9be Compare July 20, 2021 17:39

holdenk and others added 8 commits July 22, 2021 10:26

Add the new configs to the kube docs

509469e

CR feedback (imports instead of bare names rename function)

ceefcb9

Remove extra blank line

3073b82

Add an integration test for checking the label

403fc40

Fix the integration test to use matchers and pass

11df5c5

Update DecommissionSuite.scala

b9c001f

Increase timeouts for running in CI.

Update DecommissionSuite.scala

77a55aa

Bump up timeout

Switch to createOptional on KUBERNETES_EXECUTOR_POD_DECOMMISSION_LABE…

bb52a4c

…L_VALUE as suggested by dongjoon.

holdenk force-pushed the SPARK-35956-support-auto-assigning-labels-to-decommissioning-pods branch from e21d2c7 to bb52a4c Compare July 22, 2021 17:26

dongjoon-hyun reviewed Jul 22, 2021

View reviewed changes

dongjoon-hyun approved these changes Jul 22, 2021

View reviewed changes

CR feedback drop pod and fix conf def value to be none

15d32ba

Update DecommissionSuite.scala

f7d85fa

dongjoon-hyun approved these changes Jul 23, 2021

View reviewed changes

attilapiros approved these changes Jul 23, 2021

View reviewed changes

asfgit closed this in bee2799 Jul 23, 2021

dongjoon-hyun mentioned this pull request Mar 8, 2022

[SPARK-35956][K8S][FOLLOWP] Fix typos in config names #35767

Closed

[SPARK-35956][K8S] Support auto assigning labels to decommissioning pods #33270

[SPARK-35956][K8S] Support auto assigning labels to decommissioning pods #33270

Conversation

holdenk commented Jul 8, 2021

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

SparkQA commented Jul 8, 2021

SparkQA commented Jul 8, 2021

SparkQA commented Jul 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dongjoon-hyun commented Jul 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dongjoon-hyun left a comment

Choose a reason for hiding this comment

SparkQA commented Jul 15, 2021

SparkQA commented Jul 15, 2021

SparkQA commented Jul 15, 2021

dongjoon-hyun commented Jul 15, 2021

attilapiros left a comment

Choose a reason for hiding this comment

holdenk commented Jul 20, 2021

holdenk commented Jul 20, 2021

SparkQA commented Jul 20, 2021

SparkQA commented Jul 20, 2021

SparkQA commented Jul 20, 2021

SparkQA commented Jul 21, 2021

SparkQA commented Jul 21, 2021

SparkQA commented Jul 21, 2021

dongjoon-hyun commented Jul 21, 2021

SparkQA commented Jul 21, 2021

SparkQA commented Jul 22, 2021

dongjoon-hyun left a comment

Choose a reason for hiding this comment

SparkQA commented Jul 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Jul 22, 2021

dongjoon-hyun left a comment

Choose a reason for hiding this comment

SparkQA commented Jul 23, 2021

SparkQA commented Jul 23, 2021

SparkQA commented Jul 23, 2021

SparkQA commented Jul 23, 2021

SparkQA commented Jul 23, 2021

SparkQA commented Jul 23, 2021

attilapiros left a comment

Choose a reason for hiding this comment

holdenk commented Jul 23, 2021