[SPARK-37713][K8S] Assign namespace to executor configmap #34983

dcoliversun · 2021-12-22T09:16:43Z

What changes were proposed in this pull request?

Since Spark 3.X, Executor pod needs separate executor configmap. But, no namespace is assigned in configmap while building it. K8s views configmap without namespace as global resource. Once pod access is restricted (global resources cannot be read), and executor cannot obtain configmap itself.

Why are the changes needed?

Fix executor pod authorization error in K8S. When executor pod cannot read global resources, executor configmap is not available for executor.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Pass Integration Test and CIs.

AmplabJenkins · 2021-12-22T09:48:53Z

Can one of the admins verify this patch?

dcoliversun · 2021-12-23T01:49:37Z

@dongjoon-hyun Would you have review for these code? Thanks :)

dongjoon-hyun

Could you add a test case, @dcoliversun ?

dongjoon-hyun · 2021-12-27T22:16:55Z

Gentle ping, @dcoliversun .

dcoliversun · 2021-12-29T03:03:14Z

Sorry so late to reply. I've been working on it for days. I couldn't add test case because k8s allows configmap without .metadata.namespce. Request to APIServer will put namespace to body. But, .metadata.namespace is still null when request to APIServer. I think it's a good action. User will see this configmap through dashboard. The action that namespace of executor configmap yaml is null will confuse user.

dongjoon-hyun

Why is it a reason not to add a test case? You need to add a test case for your code, not K8s code itself.

I couldn't add test case because k8s allows configmap without .metadata.namespce.

It sounds like you did try to make K8s itself fail, but we know that Apache Spark works well with the previous config without any issue.

dongjoon-hyun · 2021-12-29T19:21:15Z

...ubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientUtils.scala

+  def buildConfigMap(
+     configMapName: String,
+     confFileMap: Map[String, String],
+     configMapNameSpace: String,


Please try to get namespace via confFileMap instead changing this method signature.

Done, buildConfigMap method could get namespace via confFileMap

dongjoon-hyun

I made two requests for this PR.

Provide a test coverage for newly added code path.
Avoid a method signature change as much as possible when you can do.

For the other code, it looks reasonable, @dcoliversun .

dcoliversun · 2021-12-30T01:08:09Z

OK，I understand. I am working on it

dongjoon-hyun · 2021-12-30T05:46:37Z

...etes/core/src/test/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientUtilsSuite.scala

+
+  test("verify that configmap built as expected") {
+    val configMapName = "testConfigMapName"
+    val properties = Map(Config.KUBERNETES_NAMESPACE.key -> "default")


If you don't mind, could you use a random name here? Although the previous code doesn't attache the namespace, this default is too generic to be in the test case.

OK, I change it.

dongjoon-hyun · 2021-12-30T06:30:57Z

...etes/core/src/test/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientUtilsSuite.scala

@@ -82,8 +81,9 @@ class KubernetesClientUtilsSuite extends SparkFunSuite with BeforeAndAfter {
  }

  test("verify that configmap built as expected") {
-    val configMapName = "testConfigMapName"
-    val properties = Map(Config.KUBERNETES_NAMESPACE.key -> "default")
+    val configMapName = java.util.UUID.randomUUID.toString


import java.util.UUID?

It works without "import java.util.UUID". Is this a code import style limitation? If true, I can import :)

@dcoliversun . Apache Spark recommends not to hide your import in the code. Especially, you are using it twice at line 84 and 85. Your code is verbose due to the repeatability and not good at the readability too.

@dongjoon-hyun Thanks for your suggestion. I import java.util.UUID and add name/namespace prefix to improve readability. Please reivew it 😊

dcoliversun · 2021-12-30T06:45:06Z

Sorry, CI has some error. I'm fixing it.

dongjoon-hyun · 2021-12-31T00:05:42Z

...etes/core/src/test/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientUtilsSuite.scala

@@ -82,8 +81,9 @@ class KubernetesClientUtilsSuite extends SparkFunSuite with BeforeAndAfter {
  }

  test("verify that configmap built as expected") {
-    val configMapName = "testConfigMapName"
-    val properties = Map(Config.KUBERNETES_NAMESPACE.key -> "default")
+    val configMapName = java.util.UUID.randomUUID.toString


@dcoliversun . Apache Spark recommends not to hide your import in the code. Especially, you are using it twice at line 84 and 85. Your code is verbose due to the repeatability and not good at the readability too.

dongjoon-hyun

+1, LGTM. Thank you, @dcoliversun .

dongjoon-hyun · 2021-12-31T01:50:05Z

cc @williamhyun

dcoliversun · 2021-12-31T01:50:42Z

+1, LGTM. Thank you, @dcoliversun .

@dongjoon-hyun Thanks for your review, which helps me a lot. HAPPY NEW YEAR

dongjoon-hyun · 2021-12-31T01:51:33Z

Happy New Year, @dcoliversun ! :)

dongjoon-hyun · 2021-12-31T02:28:35Z

K8s tests passed. Merged to master for Apache Spark 3.3.

dongjoon-hyun · 2021-12-31T02:31:05Z

If you need this in next Apache Spark 3.2.1, please make a backporting PR against branch-3.2.

Yikun · 2022-01-17T08:16:27Z

FYI, this PR breaks the case of using ConfigMap with namespace specified (driver side), see #35215 .

You could using below cmd to revert this commits manually before above PR merged:

git revert c4a9772f741836cdb399e35a45b3b5df48d9eea6

GabeChurch · 2022-01-27T05:59:41Z

Noticing the same behavior as Yikun with this pull breaking ConfigMap usage in non "default" namespace(s).

### What changes were proposed in this pull request? This is a follow-up of SPARK-37713 (#34983) to fix insufficient namespace propagation at configMap creation. ### Why are the changes needed? This will recover the behavior on non-`default` namespace Spark jobs like the following. ``` NAMESPACE NAME DATA AGE 10e32ae0f2304236be9c687edf8a06a4 kube-root-ca.crt 1 8s 10e32ae0f2304236be9c687edf8a06a4 spark-drv-fa3f387ea3871bbd-conf-map 2 5s 10e32ae0f2304236be9c687edf8a06a4 spark-exec-da5ea37ea38727f1-conf-map 2 3s ``` ### Does this PR introduce _any_ user-facing change? No. This is a bug fix of the unreleased patch. ### How was this patch tested? Pass the CIs and manually run the integration tests which was broken since the original PR. ``` $ mvn package -Pkubernetes -DskipTests $ resource-managers/kubernetes/integration-tests/dev/dev-run-integration-tests.sh --deploy-mode docker-for-desktop --exclude-tags minikube,r ... KubernetesSuite: - Run SparkPi with no resources - Run SparkPi with no resources & statefulset allocation - Run SparkPi with a very long application name. - Use SparkLauncher.NO_RESOURCE - Run SparkPi with a master URL without a scheme. - Run SparkPi with an argument. - Run SparkPi with custom labels, annotations, and environment variables. - All pods have the same service account by default - Run extraJVMOptions check on driver - Run SparkRemoteFileTest using a remote data file - Verify logging configuration is picked from the provided SPARK_CONF_DIR/log4j2.properties - Run SparkPi with env and mount secrets. - Run PySpark on simple pi.py example - Run PySpark to test a pyfiles example - Run PySpark with memory customization - Run in client mode. - Start pod creation from template - Test basic decommissioning - Test basic decommissioning with shuffle cleanup - Test decommissioning with dynamic allocation & shuffle cleanups - Test decommissioning timeouts - SPARK-37576: Rolling decommissioning Run completed in 10 minutes, 34 seconds. Total number of tests run: 22 Suites: completed 2, aborted 0 Tests: succeeded 22, failed 0, canceled 0, ignored 0, pending 0 All tests passed. ``` Closes #35299 Closes #35359 from dongjoon-hyun/SPARK-37713. Authored-by: Dongjoon Hyun <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

github-actions bot added the KUBERNETES label Dec 22, 2021

dongjoon-hyun reviewed Dec 23, 2021

View reviewed changes

dongjoon-hyun reviewed Dec 29, 2021

View reviewed changes

dongjoon-hyun requested changes Dec 29, 2021

View reviewed changes

dcoliversun force-pushed the SPARK-37713 branch from 1768f1b to 4f8ee74 Compare December 30, 2021 05:39

dongjoon-hyun reviewed Dec 30, 2021

View reviewed changes

[SPARK-37713][K8S] assign namespace to executor configmap

42bbf93

dcoliversun force-pushed the SPARK-37713 branch from acb9b34 to 42bbf93 Compare December 30, 2021 08:29

dcoliversun requested a review from dongjoon-hyun December 30, 2021 10:31

use default value other than hard-code value

f9053c4

dcoliversun force-pushed the SPARK-37713 branch from 3051461 to f9053c4 Compare December 30, 2021 12:22

dongjoon-hyun requested changes Dec 31, 2021

View reviewed changes

add UUID import and prefix to configmap name/namespace for readability

730a61c

dongjoon-hyun changed the title ~~[SPARK-37713][K8S] assign namespace to executor configmap~~ [SPARK-37713][K8S] Assign namespace to executor configmap Dec 31, 2021

dongjoon-hyun approved these changes Dec 31, 2021

View reviewed changes

dongjoon-hyun closed this in c4a9772 Dec 31, 2021

dcoliversun deleted the SPARK-37713 branch December 31, 2021 06:15

chia7712 mentioned this pull request Jan 15, 2022

[SPARK-37916][K8S] The ConfigMap is assigned to incorrect namespace #35215

Closed

Yikun mentioned this pull request Jan 24, 2022

[SPARK-37916][SPARK-37713][K8S] Revert "Assign namespace to executor configmap" #35299

Closed

dongjoon-hyun mentioned this pull request Jan 29, 2022

[SPARK-37713][K8S][FOLLOWUP] Fix insufficient namespace propagation #35359

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-37713][K8S] Assign namespace to executor configmap #34983

[SPARK-37713][K8S] Assign namespace to executor configmap #34983

dcoliversun commented Dec 22, 2021

AmplabJenkins commented Dec 22, 2021

dcoliversun commented Dec 23, 2021

dongjoon-hyun left a comment

dongjoon-hyun commented Dec 27, 2021

dcoliversun commented Dec 29, 2021

dongjoon-hyun left a comment

dongjoon-hyun Dec 29, 2021

dcoliversun Dec 30, 2021

dongjoon-hyun left a comment

dcoliversun commented Dec 30, 2021

dongjoon-hyun Dec 30, 2021

dcoliversun Dec 30, 2021

dongjoon-hyun Dec 30, 2021

dcoliversun Dec 30, 2021

dongjoon-hyun Dec 31, 2021 •

edited

Loading

dcoliversun Dec 31, 2021

dcoliversun commented Dec 30, 2021

dongjoon-hyun Dec 31, 2021 •

edited

Loading

dongjoon-hyun left a comment

dongjoon-hyun commented Dec 31, 2021

dcoliversun commented Dec 31, 2021

dongjoon-hyun commented Dec 31, 2021

dongjoon-hyun commented Dec 31, 2021 •

edited

Loading

dongjoon-hyun commented Dec 31, 2021

Yikun commented Jan 17, 2022 •

edited

Loading

GabeChurch commented Jan 27, 2022

[SPARK-37713][K8S] Assign namespace to executor configmap #34983

[SPARK-37713][K8S] Assign namespace to executor configmap #34983

Conversation

dcoliversun commented Dec 22, 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?

AmplabJenkins commented Dec 22, 2021

dcoliversun commented Dec 23, 2021

dongjoon-hyun left a comment

Choose a reason for hiding this comment

dongjoon-hyun commented Dec 27, 2021

dcoliversun commented Dec 29, 2021

dongjoon-hyun left a comment

Choose a reason for hiding this comment

dongjoon-hyun Dec 29, 2021

Choose a reason for hiding this comment

dcoliversun Dec 30, 2021

Choose a reason for hiding this comment

dongjoon-hyun left a comment

Choose a reason for hiding this comment

dcoliversun commented Dec 30, 2021

dongjoon-hyun Dec 30, 2021

Choose a reason for hiding this comment

dcoliversun Dec 30, 2021

Choose a reason for hiding this comment

dongjoon-hyun Dec 30, 2021

Choose a reason for hiding this comment

dcoliversun Dec 30, 2021

Choose a reason for hiding this comment

dongjoon-hyun Dec 31, 2021 • edited Loading

Choose a reason for hiding this comment

dcoliversun Dec 31, 2021

Choose a reason for hiding this comment

dcoliversun commented Dec 30, 2021

dongjoon-hyun Dec 31, 2021 • edited Loading

Choose a reason for hiding this comment

dongjoon-hyun left a comment

Choose a reason for hiding this comment

dongjoon-hyun commented Dec 31, 2021

dcoliversun commented Dec 31, 2021

dongjoon-hyun commented Dec 31, 2021

dongjoon-hyun commented Dec 31, 2021 • edited Loading

dongjoon-hyun commented Dec 31, 2021

Yikun commented Jan 17, 2022 • edited Loading

GabeChurch commented Jan 27, 2022

dongjoon-hyun Dec 31, 2021 •

edited

Loading

dongjoon-hyun Dec 31, 2021 •

edited

Loading

dongjoon-hyun commented Dec 31, 2021 •

edited

Loading

Yikun commented Jan 17, 2022 •

edited

Loading