You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/01/15 10:46:33 UTC
[GitHub] [spark] chia7712 opened a new pull request #35215: [SPARK-37916][K8S] The ConfigMap is assigned to incorrect namespace
chia7712 opened a new pull request #35215:
URL: https://github.com/apache/spark/pull/35215
### What changes were proposed in this pull request?
#34983 added the namespace to ConfigMap. However, the confFileMap does not have namespace value, so it assigns the ConfigMap to "default" namespace. When we submit spark application in non-default namespace, it causes error since it can't find the configmap in non-default namespace
### Why are the changes needed?
The bug obstructs us from submitting spark application to non-default namespace. see following error message:
```
kubelet MountVolume.SetUp failed for volume "spark-conf-volume-driver" : configmap "spark-drv-2e1f4c7e54c2d4f3-conf-map" not found
```
The above error can be reproduced by following command.
```sh
export SPARK_VERSION=3.3.0-SNAPSHOT && ./bin/spark-submit \
--master k8s://https://k8s00:6443 \
--deploy-mode cluster \
--name sparkPi \
--class org.apache.spark.examples.SparkPi \
--conf spark.kubernetes.authenticate.driver.serviceAccountName=default \
--conf spark.kubernetes.namespace=yunikorn \
--conf spark.kubernetes.container.image=ghcr.io/chia7712/spark:${SPARK_VERSION} \
local:///opt/spark/examples/jars/spark-examples_2.12-${SPARK_VERSION}.jar
```
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Pass Integration Test and CIs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] chia7712 closed pull request #35215: [SPARK-37916][K8S] The ConfigMap is assigned to incorrect namespace
Posted by GitBox <gi...@apache.org>.
chia7712 closed pull request #35215:
URL: https://github.com/apache/spark/pull/35215
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Yikun commented on a change in pull request #35215: [SPARK-37916][K8S] The ConfigMap is assigned to incorrect namespace
Posted by GitBox <gi...@apache.org>.
Yikun commented on a change in pull request #35215:
URL: https://github.com/apache/spark/pull/35215#discussion_r790227632
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/KubernetesClusterSchedulerBackend.scala
##########
@@ -76,13 +76,12 @@ private[spark] class KubernetesClusterSchedulerBackend(
private def setUpExecutorConfigMap(driverPod: Option[Pod]): Unit = {
val configMapName = KubernetesClientUtils.configMapNameExecutor
- val resolvedExecutorProperties =
- Map(KUBERNETES_NAMESPACE.key -> conf.get(KUBERNETES_NAMESPACE))
val confFilesMap = KubernetesClientUtils
Review comment:
> driver confileMaps sets namespace in the same ways
Do you mean [the same way](https://github.com/apache/spark/blob/c4a9772f741836cdb399e35a45b3b5df48d9eea6/resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientUtils.scala#L92) is what was introduced in https://github.com/apache/spark/pull/34983 ? Actually this is the reason of why we need this PR (regression for driver side) and where the `confFileMap` mixup problem happened.
> aiming to keep confMaps up with sparkConf.
Aggree, this is right, and this PR also can keep this regression, you can see @chia7712 already added the test for driver and executor and also avoid `confFileMap` mixup problem.
> Second, spark.k8s.executor.configmap.namespace is not necessary.
I said "**if we need**", I updated it in my last comments to avoid misleading. And also it decribed how we set namespace according conf (even if the potential separate configure case in future) rather a indirect step confFileMap.
Of course, this is just my prefer, so I give my +1 on this PR. If the confiFilemap way can **solve the key mixup problem** and **also have regresson on driver side**, I'd also okay with it.
Anyway, both two way can also solve the problem of driver side, I think we need to get problem fixed first because this is a very basic case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dcoliversun commented on a change in pull request #35215: [SPARK-37916][K8S] The ConfigMap is assigned to incorrect namespace
Posted by GitBox <gi...@apache.org>.
dcoliversun commented on a change in pull request #35215:
URL: https://github.com/apache/spark/pull/35215#discussion_r790207979
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientUtils.scala
##########
@@ -88,9 +88,8 @@ private[spark] object KubernetesClientUtils extends Logging {
* on remote pods.
*/
def buildConfigMap(configMapName: String, confFileMap: Map[String, String],
- withLabels: Map[String, String] = Map()): ConfigMap = {
- val configMapNameSpace =
- confFileMap.getOrElse(KUBERNETES_NAMESPACE.key, KUBERNETES_NAMESPACE.defaultValueString)
+ configMapNameSpace: String,
Review comment:
Hi，I don't think it is a good idea to change this method signature. Could you fix namespace in conFileMap?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] chia7712 commented on pull request #35215: [SPARK-37916][K8S] The ConfigMap is assigned to incorrect namespace
Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #35215:
URL: https://github.com/apache/spark/pull/35215#issuecomment-1014889112
> If it's possible, would you mind also add a testcase to make sure the namespace is set correctly in executor/driver?
@Yikun thanks for your comments. will copy that later.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Yikun commented on a change in pull request #35215: [SPARK-37916][K8S] The ConfigMap is assigned to incorrect namespace
Posted by GitBox <gi...@apache.org>.
Yikun commented on a change in pull request #35215:
URL: https://github.com/apache/spark/pull/35215#discussion_r790218477
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/KubernetesClusterSchedulerBackend.scala
##########
@@ -76,13 +76,12 @@ private[spark] class KubernetesClusterSchedulerBackend(
private def setUpExecutorConfigMap(driverPod: Option[Pod]): Unit = {
val configMapName = KubernetesClientUtils.configMapNameExecutor
- val resolvedExecutorProperties =
- Map(KUBERNETES_NAMESPACE.key -> conf.get(KUBERNETES_NAMESPACE))
val confFilesMap = KubernetesClientUtils
Review comment:
Thanks for clarify, the Kubernetes allow any key as configmap key (even if the key name is 'namespace'), So I think any key put in configmap should be covert to the real configmap.key rather than do some hack and special process to occupy any user potential input.
If we just need to set executor separately, I guess what we real needed is introducing a new conf like `spark.k8s.executor.configmap.namespace` otherwise back to `spark.k8s.namespace` directly.
And I also didn't see any problem if we just use conf.namespace like this PR done. Could you explain some If you have any other concern?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Yikun commented on a change in pull request #35215: [SPARK-37916][K8S] The ConfigMap is assigned to incorrect namespace
Posted by GitBox <gi...@apache.org>.
Yikun commented on a change in pull request #35215:
URL: https://github.com/apache/spark/pull/35215#discussion_r790218477
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/KubernetesClusterSchedulerBackend.scala
##########
@@ -76,13 +76,12 @@ private[spark] class KubernetesClusterSchedulerBackend(
private def setUpExecutorConfigMap(driverPod: Option[Pod]): Unit = {
val configMapName = KubernetesClientUtils.configMapNameExecutor
- val resolvedExecutorProperties =
- Map(KUBERNETES_NAMESPACE.key -> conf.get(KUBERNETES_NAMESPACE))
val confFilesMap = KubernetesClientUtils
Review comment:
Thanks for clarify, the Kubernetes allow any key as configmap key (even if the key name is 'namespace'), So I think any key put in configmap should be covert to the real configmap.key rather than do some hack and special process to occupy any user potential input.
**If we need to set executor separately(although I don't think it's a real use case now)**, I guess what we real needed is introducing a new conf like `spark.k8s.executor.configmap.namespace` otherwise back to `spark.k8s.namespace` directly.
And I also didn't see any problem if we just use conf.namespace like this PR done. Could you explain some If you have any other concern?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Yikun commented on a change in pull request #35215: [SPARK-37916][K8S] The ConfigMap is assigned to incorrect namespace
Posted by GitBox <gi...@apache.org>.
Yikun commented on a change in pull request #35215:
URL: https://github.com/apache/spark/pull/35215#discussion_r786341158
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/KubernetesClusterSchedulerBackend.scala
##########
@@ -76,13 +76,12 @@ private[spark] class KubernetesClusterSchedulerBackend(
private def setUpExecutorConfigMap(driverPod: Option[Pod]): Unit = {
val configMapName = KubernetesClientUtils.configMapNameExecutor
- val resolvedExecutorProperties =
- Map(KUBERNETES_NAMESPACE.key -> conf.get(KUBERNETES_NAMESPACE))
val confFilesMap = KubernetesClientUtils
- .buildSparkConfDirFilesMap(configMapName, conf, resolvedExecutorProperties)
+ .buildSparkConfDirFilesMap(configMapName, conf, Map.empty)
val labels =
Map(SPARK_APP_ID_LABEL -> applicationId(), SPARK_ROLE_LABEL -> SPARK_POD_EXECUTOR_ROLE)
- val configMap = KubernetesClientUtils.buildConfigMap(configMapName, confFilesMap, labels)
+ val configMap = KubernetesClientUtils.buildConfigMap(configMapName, confFilesMap,
+ conf.get(KUBERNETES_NAMESPACE), labels)
Review comment:
Ah, yes, just ignore this comment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Yikun commented on a change in pull request #35215: [SPARK-37916][K8S] The ConfigMap is assigned to incorrect namespace
Posted by GitBox <gi...@apache.org>.
Yikun commented on a change in pull request #35215:
URL: https://github.com/apache/spark/pull/35215#discussion_r790227632
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/KubernetesClusterSchedulerBackend.scala
##########
@@ -76,13 +76,12 @@ private[spark] class KubernetesClusterSchedulerBackend(
private def setUpExecutorConfigMap(driverPod: Option[Pod]): Unit = {
val configMapName = KubernetesClientUtils.configMapNameExecutor
- val resolvedExecutorProperties =
- Map(KUBERNETES_NAMESPACE.key -> conf.get(KUBERNETES_NAMESPACE))
val confFilesMap = KubernetesClientUtils
Review comment:
> driver confileMaps sets namespace in the same ways
Do you mean [the same way](https://github.com/apache/spark/blob/c4a9772f741836cdb399e35a45b3b5df48d9eea6/resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientUtils.scala#L92) is what was introduced in https://github.com/apache/spark/pull/34983 ? Actually this is the reason of why we need this PR (regression for driver side) and where the `confFileMap` mixup problem happened.
> aiming to keep confMaps up with sparkConf.
Aggree, this is right, and this PR also can keep this regression, you can see @chia7712 already added the test for driver and executor and also avoid `confFileMap` mixup problem.
> Second, spark.k8s.executor.configmap.namespace is not necessary.
I said "**if we need**", I updated it in my last comments to avoid misleading. And also it decribed how we set namespace according conf (even if the potential separate configure case in future) rather a indirect step confFileMap.
Of course, this is just my prefer, so I give my +1 on this PR. If the confiFilemap way could solve the key mixup problem and also have regresson on driver side, I'd also okay with it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] chia7712 commented on pull request #35215: [SPARK-37916][K8S] The ConfigMap is assigned to incorrect namespace
Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #35215:
URL: https://github.com/apache/spark/pull/35215#issuecomment-1015618238
> If it's possible, would you mind also add a testcase to make sure the namespace is set correctly in executor/driver?
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] chia7712 commented on a change in pull request #35215: [SPARK-37916][K8S] The ConfigMap is assigned to incorrect namespace
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #35215:
URL: https://github.com/apache/spark/pull/35215#discussion_r786288709
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/KubernetesClusterSchedulerBackend.scala
##########
@@ -76,13 +76,12 @@ private[spark] class KubernetesClusterSchedulerBackend(
private def setUpExecutorConfigMap(driverPod: Option[Pod]): Unit = {
val configMapName = KubernetesClientUtils.configMapNameExecutor
- val resolvedExecutorProperties =
- Map(KUBERNETES_NAMESPACE.key -> conf.get(KUBERNETES_NAMESPACE))
val confFilesMap = KubernetesClientUtils
- .buildSparkConfDirFilesMap(configMapName, conf, resolvedExecutorProperties)
+ .buildSparkConfDirFilesMap(configMapName, conf, Map.empty)
val labels =
Map(SPARK_APP_ID_LABEL -> applicationId(), SPARK_ROLE_LABEL -> SPARK_POD_EXECUTOR_ROLE)
- val configMap = KubernetesClientUtils.buildConfigMap(configMapName, confFilesMap, labels)
+ val configMap = KubernetesClientUtils.buildConfigMap(configMapName, confFilesMap,
+ conf.get(KUBERNETES_NAMESPACE), labels)
Review comment:
The type of `conf` is `SparkConf` rather than `KubernetesConf`, so it can't call the method `namespace`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dcoliversun commented on a change in pull request #35215: [SPARK-37916][K8S] The ConfigMap is assigned to incorrect namespace
Posted by GitBox <gi...@apache.org>.
dcoliversun commented on a change in pull request #35215:
URL: https://github.com/apache/spark/pull/35215#discussion_r790219806
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/KubernetesClusterSchedulerBackend.scala
##########
@@ -76,13 +76,12 @@ private[spark] class KubernetesClusterSchedulerBackend(
private def setUpExecutorConfigMap(driverPod: Option[Pod]): Unit = {
val configMapName = KubernetesClientUtils.configMapNameExecutor
- val resolvedExecutorProperties =
- Map(KUBERNETES_NAMESPACE.key -> conf.get(KUBERNETES_NAMESPACE))
val confFilesMap = KubernetesClientUtils
Review comment:
I understood but I don't agree with that. First, driver confileMaps sets namespace in the same ways, aiming to keep confMaps up with sparkConf. Second, `spark.k8s.executor.configmap.namespace` is not necessary. Driver/executor pod and related resource should keep in same namespace.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dcoliversun commented on a change in pull request #35215: [SPARK-37916][K8S] The ConfigMap is assigned to incorrect namespace
Posted by GitBox <gi...@apache.org>.
dcoliversun commented on a change in pull request #35215:
URL: https://github.com/apache/spark/pull/35215#discussion_r790243553
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/KubernetesClusterSchedulerBackend.scala
##########
@@ -76,13 +76,12 @@ private[spark] class KubernetesClusterSchedulerBackend(
private def setUpExecutorConfigMap(driverPod: Option[Pod]): Unit = {
val configMapName = KubernetesClientUtils.configMapNameExecutor
- val resolvedExecutorProperties =
- Map(KUBERNETES_NAMESPACE.key -> conf.get(KUBERNETES_NAMESPACE))
val confFilesMap = KubernetesClientUtils
Review comment:
>
Thanks for your analysis. Driver conFileMaps sets namespace in the [same ways](https://github.com/apache/spark/blob/ddc77fb906cb3ce1567d277c2d0850104c89ac25/resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientApplication.scala#L104-L108)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Yikun commented on a change in pull request #35215: [SPARK-37916][K8S] The ConfigMap is assigned to incorrect namespace
Posted by GitBox <gi...@apache.org>.
Yikun commented on a change in pull request #35215:
URL: https://github.com/apache/spark/pull/35215#discussion_r790218477
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/KubernetesClusterSchedulerBackend.scala
##########
@@ -76,13 +76,12 @@ private[spark] class KubernetesClusterSchedulerBackend(
private def setUpExecutorConfigMap(driverPod: Option[Pod]): Unit = {
val configMapName = KubernetesClientUtils.configMapNameExecutor
- val resolvedExecutorProperties =
- Map(KUBERNETES_NAMESPACE.key -> conf.get(KUBERNETES_NAMESPACE))
val confFilesMap = KubernetesClientUtils
Review comment:
Thanks for clarify, the Kubernetes allow any key as configmap key (even if the key name is 'namespace'), So I think any key put in configmap should be covert to the real configmap.key rather than do some hack and special process to occupy any user potential input.
**If we need to set executor separately**, I guess what we real needed is introducing a new conf like `spark.k8s.executor.configmap.namespace` otherwise back to `spark.k8s.namespace` directly.
And I also didn't see any problem if we just use conf.namespace like this PR done. Could you explain some If you have any other concern?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Yikun commented on a change in pull request #35215: [SPARK-37916][K8S] The ConfigMap is assigned to incorrect namespace
Posted by GitBox <gi...@apache.org>.
Yikun commented on a change in pull request #35215:
URL: https://github.com/apache/spark/pull/35215#discussion_r790227632
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/KubernetesClusterSchedulerBackend.scala
##########
@@ -76,13 +76,12 @@ private[spark] class KubernetesClusterSchedulerBackend(
private def setUpExecutorConfigMap(driverPod: Option[Pod]): Unit = {
val configMapName = KubernetesClientUtils.configMapNameExecutor
- val resolvedExecutorProperties =
- Map(KUBERNETES_NAMESPACE.key -> conf.get(KUBERNETES_NAMESPACE))
val confFilesMap = KubernetesClientUtils
Review comment:
> driver confileMaps sets namespace in the same ways
Do you mean [this](https://github.com/apache/spark/blob/c4a9772f741836cdb399e35a45b3b5df48d9eea6/resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientUtils.scala#L92) is what was introduced in https://github.com/apache/spark/pull/34983 ? Actually this is the reason of why we need this PR (regression for driver side) and where `confFileMap` mixup problem happened.
> aiming to keep confMaps up with sparkConf.
Aggree, this is right, and this PR also can keep this regression, you can see @chia7712 already added the test for driver and executor and also avoid `confFileMap` mixup problem.
> Second, spark.k8s.executor.configmap.namespace is not necessary.
I said **if we need**, I updated it in my last comments to avoid misleading. And also it decribed how we set namespace according conf (even if the potential separate configure case in future) rather a indirect step confFileMap.
Of course, this is just my prefer, so I give my +1 on this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Yikun commented on a change in pull request #35215: [SPARK-37916][K8S] The ConfigMap is assigned to incorrect namespace
Posted by GitBox <gi...@apache.org>.
Yikun commented on a change in pull request #35215:
URL: https://github.com/apache/spark/pull/35215#discussion_r790218477
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/KubernetesClusterSchedulerBackend.scala
##########
@@ -76,13 +76,12 @@ private[spark] class KubernetesClusterSchedulerBackend(
private def setUpExecutorConfigMap(driverPod: Option[Pod]): Unit = {
val configMapName = KubernetesClientUtils.configMapNameExecutor
- val resolvedExecutorProperties =
- Map(KUBERNETES_NAMESPACE.key -> conf.get(KUBERNETES_NAMESPACE))
val confFilesMap = KubernetesClientUtils
Review comment:
Thanks for clarify, but the Kubernetes allow any key as configmap key (even if the key name is 'namespace'), we should't break any behavior due to spark implemention or workaround.
So I think any key put in configmap should be covert to the real configmap.key rather than do some hack and special process to occupy any user potential input.
If we just need to set executor separately, I guess what we real needed is introducing a new conf like spark.k8s.executor.configmap.namespace otherwise back to spark.k8s.namespace directly.
And I also didn't see any problem if we just use conf.namespace like this PR done. Could you explain some If you have any other concern?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] chia7712 commented on a change in pull request #35215: [SPARK-37916][K8S] The ConfigMap is assigned to incorrect namespace
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #35215:
URL: https://github.com/apache/spark/pull/35215#discussion_r790260807
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/KubernetesClusterSchedulerBackend.scala
##########
@@ -76,13 +76,12 @@ private[spark] class KubernetesClusterSchedulerBackend(
private def setUpExecutorConfigMap(driverPod: Option[Pod]): Unit = {
val configMapName = KubernetesClientUtils.configMapNameExecutor
- val resolvedExecutorProperties =
- Map(KUBERNETES_NAMESPACE.key -> conf.get(KUBERNETES_NAMESPACE))
val confFilesMap = KubernetesClientUtils
Review comment:
@dcoliversun @Yikun thanks for all your comments.
> Driver conFileMaps sets namespace in the same ways
It seems to me that it does not propagate namespace configured by ‘ spark.kubernetes.namespace’ to buildCpnfigMap. We don’t explicitly assign the namespace of configmap before #34983, so it is fine. After we assign the namespace of configmap explicit, it is possible to cause error (on my local) when the true namespace is not propagated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Yikun commented on a change in pull request #35215: [SPARK-37916][K8S] The ConfigMap is assigned to incorrect namespace
Posted by GitBox <gi...@apache.org>.
Yikun commented on a change in pull request #35215:
URL: https://github.com/apache/spark/pull/35215#discussion_r787343899
##########
File path: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/submit/ClientSuite.scala
##########
@@ -337,4 +337,32 @@ class ClientSuite extends SparkFunSuite with BeforeAndAfter {
submissionClient.run()
verify(loggingPodStatusWatcher).watchOrStop(kconf.namespace + ":driver")
}
+
+ test("driver's confimap.metadata.namespace set to conf.namespace otherwise default") {
+ val submissionClient = new Client(
+ kconf,
+ driverBuilder,
+ kubernetesClient,
+ loggingPodStatusWatcher)
+ assert(submissionClient.createConfigs()._3.getMetadata.getNamespace === "default")
Review comment:
```suggestion
assert(submissionClient.createConfigMap(BUILT_KUBERNETES_SPEC)._2.getMetadata.getNamespace === "default")
```
The test will also simple
##########
File path: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/scheduler/cluster/k8s/KubernetesClusterSchedulerBackendSuite.scala
##########
@@ -131,6 +131,22 @@ class KubernetesClusterSchedulerBackendSuite extends SparkFunSuite with BeforeAn
pollEvents)
}
+ test("executor's configmap.metadata.namespace set to conf.namespace otherwise default") {
+ assert(schedulerBackendUnderTest.createConfigMap().getMetadata.getNamespace === "default")
+
+ sparkConf.set(KUBERNETES_NAMESPACE, "namespace")
Review comment:
better to create new sparkConf to avoid the impact on potential followup testcase
##########
File path: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/submit/ClientSuite.scala
##########
@@ -337,4 +337,32 @@ class ClientSuite extends SparkFunSuite with BeforeAndAfter {
submissionClient.run()
verify(loggingPodStatusWatcher).watchOrStop(kconf.namespace + ":driver")
}
+
+ test("driver's confimap.metadata.namespace set to conf.namespace otherwise default") {
Review comment:
nit: test("SPARK-37916: driver's confimap.metadata.namespace set to conf.namespace otherwise default") {
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientApplication.scala
##########
@@ -100,12 +100,18 @@ private[spark] class Client(
kubernetesClient: KubernetesClient,
watcher: LoggingPodStatusWatcher) extends Logging {
- def run(): Unit = {
+ private[spark] def createConfigs(): (KubernetesDriverSpec, Map[String, String], ConfigMap) = {
Review comment:
I guess this refeactor is for test, we'd better only separate configMap part, and then, still keep resolvedDriverSpec in run, like this
```suggestion
private[spark] def createConfigMap(
driverSpec: KubernetesDriverSpec): (Map[String, String], ConfigMap) = {
val configMapName = KubernetesClientUtils.configMapNameDriver
val confFilesMap = KubernetesClientUtils.buildSparkConfDirFilesMap(configMapName,
conf.sparkConf, driverSpec.systemProperties)
val configMap = KubernetesClientUtils
.buildConfigMap(configMapName, confFilesMap, conf.namespace)
(confFilesMap, configMap)
}
def run(): Unit = {
val resolvedDriverSpec = builder.buildFromFeatures(conf, kubernetesClient)
val configMapName = KubernetesClientUtils.configMapNameDriver
val (confFilesMap, configMap) = createConfigMap(resolvedDriverSpec)
```
##########
File path: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/submit/ClientSuite.scala
##########
@@ -337,4 +337,32 @@ class ClientSuite extends SparkFunSuite with BeforeAndAfter {
submissionClient.run()
verify(loggingPodStatusWatcher).watchOrStop(kconf.namespace + ":driver")
}
+
+ test("driver's confimap.metadata.namespace set to conf.namespace otherwise default") {
+ val submissionClient = new Client(
+ kconf,
+ driverBuilder,
+ kubernetesClient,
+ loggingPodStatusWatcher)
+ assert(submissionClient.createConfigs()._3.getMetadata.getNamespace === "default")
+
+ val sparkConf = new SparkConf(false)
+ .set(Config.KUBERNETES_NAMESPACE, "namespace")
+
+ val kconf2 = KubernetesTestConf.createDriverConf(
+ sparkConf = sparkConf,
+ resourceNamePrefix = Some(KUBERNETES_RESOURCE_PREFIX)
Review comment:
I guess `resourceNamePrefix` is redundant.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Yikun commented on a change in pull request #35215: [SPARK-37916][K8S] The ConfigMap is assigned to incorrect namespace
Posted by GitBox <gi...@apache.org>.
Yikun commented on a change in pull request #35215:
URL: https://github.com/apache/spark/pull/35215#discussion_r790227632
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/KubernetesClusterSchedulerBackend.scala
##########
@@ -76,13 +76,12 @@ private[spark] class KubernetesClusterSchedulerBackend(
private def setUpExecutorConfigMap(driverPod: Option[Pod]): Unit = {
val configMapName = KubernetesClientUtils.configMapNameExecutor
- val resolvedExecutorProperties =
- Map(KUBERNETES_NAMESPACE.key -> conf.get(KUBERNETES_NAMESPACE))
val confFilesMap = KubernetesClientUtils
Review comment:
> driver confileMaps sets namespace in the same ways
Do you mean [the same way](https://github.com/apache/spark/blob/c4a9772f741836cdb399e35a45b3b5df48d9eea6/resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientUtils.scala#L92) is what was introduced in https://github.com/apache/spark/pull/34983 ? Actually this is the reason of why we need this PR (regression for driver side) and where the `confFileMap` mixup problem happened.
> aiming to keep confMaps up with sparkConf.
Aggree, this is right, and this PR also can keep this regression, you can see @chia7712 already added the test for driver and executor and also avoid `confFileMap` mixup problem.
> Second, spark.k8s.executor.configmap.namespace is not necessary.
I said **if we need**, I updated it in my last comments to avoid misleading. And also it decribed how we set namespace according conf (even if the potential separate configure case in future) rather a indirect step confFileMap.
Of course, this is just my prefer, so I give my +1 on this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Yikun commented on a change in pull request #35215: [SPARK-37916][K8S] The ConfigMap is assigned to incorrect namespace
Posted by GitBox <gi...@apache.org>.
Yikun commented on a change in pull request #35215:
URL: https://github.com/apache/spark/pull/35215#discussion_r790227632
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/KubernetesClusterSchedulerBackend.scala
##########
@@ -76,13 +76,12 @@ private[spark] class KubernetesClusterSchedulerBackend(
private def setUpExecutorConfigMap(driverPod: Option[Pod]): Unit = {
val configMapName = KubernetesClientUtils.configMapNameExecutor
- val resolvedExecutorProperties =
- Map(KUBERNETES_NAMESPACE.key -> conf.get(KUBERNETES_NAMESPACE))
val confFilesMap = KubernetesClientUtils
Review comment:
> driver confileMaps sets namespace in the same ways
Do you mean [the same way](https://github.com/apache/spark/blob/c4a9772f741836cdb399e35a45b3b5df48d9eea6/resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientUtils.scala#L92) is what was introduced in https://github.com/apache/spark/pull/34983 ? Actually this is the reason of why we need this PR (regression for driver side) and where the `confFileMap` mixup problem happened.
> aiming to keep confMaps up with sparkConf.
Aggree, this is right, and this PR also can keep this regression, you can see @chia7712 already added the test for driver and executor and also avoid `confFileMap` mixup problem.
> Second, spark.k8s.executor.configmap.namespace is not necessary.
I said "**if we need**", I updated it in my last comments to avoid misleading. And also it decribed how we set namespace according conf (even if the potential separate configure case in future) rather a indirect step confFileMap.
Of course, this is just my prefer, so I give my +1 on this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Yikun commented on a change in pull request #35215: [SPARK-37916][K8S] The ConfigMap is assigned to incorrect namespace
Posted by GitBox <gi...@apache.org>.
Yikun commented on a change in pull request #35215:
URL: https://github.com/apache/spark/pull/35215#discussion_r790218477
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/KubernetesClusterSchedulerBackend.scala
##########
@@ -76,13 +76,12 @@ private[spark] class KubernetesClusterSchedulerBackend(
private def setUpExecutorConfigMap(driverPod: Option[Pod]): Unit = {
val configMapName = KubernetesClientUtils.configMapNameExecutor
- val resolvedExecutorProperties =
- Map(KUBERNETES_NAMESPACE.key -> conf.get(KUBERNETES_NAMESPACE))
val confFilesMap = KubernetesClientUtils
Review comment:
Thanks for clarify, the Kubernetes allow any key as configmap key (even if the key name is 'namespace'), So I think any key put in configmap should be covert to the real configmap.key rather than do some hack and special process to occupy any user potential input.
**If we need to set executor separately(although I didn't it's a real use case now)**, I guess what we real needed is introducing a new conf like `spark.k8s.executor.configmap.namespace` otherwise back to `spark.k8s.namespace` directly.
And I also didn't see any problem if we just use conf.namespace like this PR done. Could you explain some If you have any other concern?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Yikun commented on a change in pull request #35215: [SPARK-37916][K8S] The ConfigMap is assigned to incorrect namespace
Posted by GitBox <gi...@apache.org>.
Yikun commented on a change in pull request #35215:
URL: https://github.com/apache/spark/pull/35215#discussion_r790231820
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/KubernetesClusterSchedulerBackend.scala
##########
@@ -76,13 +76,12 @@ private[spark] class KubernetesClusterSchedulerBackend(
private def setUpExecutorConfigMap(driverPod: Option[Pod]): Unit = {
val configMapName = KubernetesClientUtils.configMapNameExecutor
- val resolvedExecutorProperties =
- Map(KUBERNETES_NAMESPACE.key -> conf.get(KUBERNETES_NAMESPACE))
val confFilesMap = KubernetesClientUtils
Review comment:
Of course, this is just my prefer, so I give my +1 on this PR. If the confiFilemap way can **solve the key mixup problem** (but looks like it didn't) and **also have regresson on driver side**, I'd also okay with it.
Anyway, both two way (configfilemap way and this PR way) can also solve the problem of driver side, I think we need to get problem fixed first because this is a very basic case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Yikun commented on a change in pull request #35215: [SPARK-37916][K8S] The ConfigMap is assigned to incorrect namespace
Posted by GitBox <gi...@apache.org>.
Yikun commented on a change in pull request #35215:
URL: https://github.com/apache/spark/pull/35215#discussion_r785707395
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/KubernetesClusterSchedulerBackend.scala
##########
@@ -76,13 +76,12 @@ private[spark] class KubernetesClusterSchedulerBackend(
private def setUpExecutorConfigMap(driverPod: Option[Pod]): Unit = {
val configMapName = KubernetesClientUtils.configMapNameExecutor
- val resolvedExecutorProperties =
- Map(KUBERNETES_NAMESPACE.key -> conf.get(KUBERNETES_NAMESPACE))
val confFilesMap = KubernetesClientUtils
Review comment:
Yep, It's reasonable in here, because namespace is in `configmap.metadata`, we couldn't get any namespace from confFilesMap, and also should not force put the namespaces into confFilesMap to avoid occupy any `configmap.data` key(such like the `namespace` in here).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dcoliversun commented on a change in pull request #35215: [SPARK-37916][K8S] The ConfigMap is assigned to incorrect namespace
Posted by GitBox <gi...@apache.org>.
dcoliversun commented on a change in pull request #35215:
URL: https://github.com/apache/spark/pull/35215#discussion_r790208592
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/KubernetesClusterSchedulerBackend.scala
##########
@@ -76,13 +76,12 @@ private[spark] class KubernetesClusterSchedulerBackend(
private def setUpExecutorConfigMap(driverPod: Option[Pod]): Unit = {
val configMapName = KubernetesClientUtils.configMapNameExecutor
- val resolvedExecutorProperties =
- Map(KUBERNETES_NAMESPACE.key -> conf.get(KUBERNETES_NAMESPACE))
val confFilesMap = KubernetesClientUtils
Review comment:
Hi, Yikun. Thanks for you commnet. I think `configmap.namespace` should keep up with `spark.kubernetes.namespace`, which means the namespace that will be used for running the driver and executor pods. And, executor configmap never set namespace, so I put the namespace into confFilesMap.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Yikun commented on a change in pull request #35215: [SPARK-37916][K8S] The ConfigMap is assigned to incorrect namespace
Posted by GitBox <gi...@apache.org>.
Yikun commented on a change in pull request #35215:
URL: https://github.com/apache/spark/pull/35215#discussion_r790227632
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/KubernetesClusterSchedulerBackend.scala
##########
@@ -76,13 +76,12 @@ private[spark] class KubernetesClusterSchedulerBackend(
private def setUpExecutorConfigMap(driverPod: Option[Pod]): Unit = {
val configMapName = KubernetesClientUtils.configMapNameExecutor
- val resolvedExecutorProperties =
- Map(KUBERNETES_NAMESPACE.key -> conf.get(KUBERNETES_NAMESPACE))
val confFilesMap = KubernetesClientUtils
Review comment:
> driver confileMaps sets namespace in the same ways
Do you mean [the same way](https://github.com/apache/spark/blob/c4a9772f741836cdb399e35a45b3b5df48d9eea6/resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientUtils.scala#L92) is what was introduced in https://github.com/apache/spark/pull/34983 ? Actually this is the reason of why we need this PR (regression for driver side) and where `confFileMap` mixup problem happened.
> aiming to keep confMaps up with sparkConf.
Aggree, this is right, and this PR also can keep this regression, you can see @chia7712 already added the test for driver and executor and also avoid `confFileMap` mixup problem.
> Second, spark.k8s.executor.configmap.namespace is not necessary.
I said **if we need**, I updated it in my last comments to avoid misleading. And also it decribed how we set namespace according conf (even if the potential separate configure case in future) rather a indirect step confFileMap.
Of course, this is just my prefer, so I give my +1 on this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Yikun commented on a change in pull request #35215: [SPARK-37916][K8S] The ConfigMap is assigned to incorrect namespace
Posted by GitBox <gi...@apache.org>.
Yikun commented on a change in pull request #35215:
URL: https://github.com/apache/spark/pull/35215#discussion_r790227632
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/KubernetesClusterSchedulerBackend.scala
##########
@@ -76,13 +76,12 @@ private[spark] class KubernetesClusterSchedulerBackend(
private def setUpExecutorConfigMap(driverPod: Option[Pod]): Unit = {
val configMapName = KubernetesClientUtils.configMapNameExecutor
- val resolvedExecutorProperties =
- Map(KUBERNETES_NAMESPACE.key -> conf.get(KUBERNETES_NAMESPACE))
val confFilesMap = KubernetesClientUtils
Review comment:
> driver confileMaps sets namespace in the same ways
Do you mean [this](https://github.com/apache/spark/blob/c4a9772f741836cdb399e35a45b3b5df48d9eea6/resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientUtils.scala#L92) which was introduced in https://github.com/apache/spark/pull/34983 ? Actually this is the reason of why we need this PR (regression for driver side) and where `confFileMap` mixup problem happened.
> aiming to keep confMaps up with sparkConf.
Aggree, this is right, and this PR also can keep this regression, you can see @chia7712 already added the test for driver and executor and also avoid `confFileMap` mixup problem.
> Second, spark.k8s.executor.configmap.namespace is not necessary.
I said **if we need**, I updated it in my last comments to avoid misleading. And also it decribed how we set namespace according conf (even if the potential separate configure case in future) rather a indirect step confFileMap.
Of course, this is just my prefer, so I give my +1 on this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Yikun commented on a change in pull request #35215: [SPARK-37916][K8S] The ConfigMap is assigned to incorrect namespace
Posted by GitBox <gi...@apache.org>.
Yikun commented on a change in pull request #35215:
URL: https://github.com/apache/spark/pull/35215#discussion_r790218477
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/KubernetesClusterSchedulerBackend.scala
##########
@@ -76,13 +76,12 @@ private[spark] class KubernetesClusterSchedulerBackend(
private def setUpExecutorConfigMap(driverPod: Option[Pod]): Unit = {
val configMapName = KubernetesClientUtils.configMapNameExecutor
- val resolvedExecutorProperties =
- Map(KUBERNETES_NAMESPACE.key -> conf.get(KUBERNETES_NAMESPACE))
val confFilesMap = KubernetesClientUtils
Review comment:
Thanks for clarify, the Kubernetes allow any key as configmap key (even if the key name is 'namespace'), So I think any key put in configmap should be covert to the real configmap.key rather than do some hack and special process to occupy any user potential input.
**If we need to set executor separately **, I guess what we real needed is introducing a new conf like `spark.k8s.executor.configmap.namespace` otherwise back to `spark.k8s.namespace` directly.
And I also didn't see any problem if we just use conf.namespace like this PR done. Could you explain some If you have any other concern?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Yikun commented on a change in pull request #35215: [SPARK-37916][K8S] The ConfigMap is assigned to incorrect namespace
Posted by GitBox <gi...@apache.org>.
Yikun commented on a change in pull request #35215:
URL: https://github.com/apache/spark/pull/35215#discussion_r790218614
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientUtils.scala
##########
@@ -88,9 +88,8 @@ private[spark] object KubernetesClientUtils extends Logging {
* on remote pods.
*/
def buildConfigMap(configMapName: String, confFileMap: Map[String, String],
- withLabels: Map[String, String] = Map()): ConfigMap = {
- val configMapNameSpace =
- confFileMap.getOrElse(KUBERNETES_NAMESPACE.key, KUBERNETES_NAMESPACE.defaultValueString)
+ configMapNameSpace: String,
Review comment:
See my comments on https://github.com/apache/spark/pull/35215#discussion_r790218477
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Yikun commented on pull request #35215: [SPARK-37916][K8S] The ConfigMap is assigned to incorrect namespace
Posted by GitBox <gi...@apache.org>.
Yikun commented on pull request #35215:
URL: https://github.com/apache/spark/pull/35215#issuecomment-1014246663
cc @dongjoon-hyun
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] chia7712 commented on pull request #35215: [SPARK-37916][K8S] The ConfigMap is assigned to incorrect namespace
Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #35215:
URL: https://github.com/apache/spark/pull/35215#issuecomment-1018255636
rebase and ping for more reviews :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Yikun commented on a change in pull request #35215: [SPARK-37916][K8S] The ConfigMap is assigned to incorrect namespace
Posted by GitBox <gi...@apache.org>.
Yikun commented on a change in pull request #35215:
URL: https://github.com/apache/spark/pull/35215#discussion_r785718346
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/KubernetesClusterSchedulerBackend.scala
##########
@@ -76,13 +76,12 @@ private[spark] class KubernetesClusterSchedulerBackend(
private def setUpExecutorConfigMap(driverPod: Option[Pod]): Unit = {
val configMapName = KubernetesClientUtils.configMapNameExecutor
- val resolvedExecutorProperties =
- Map(KUBERNETES_NAMESPACE.key -> conf.get(KUBERNETES_NAMESPACE))
val confFilesMap = KubernetesClientUtils
- .buildSparkConfDirFilesMap(configMapName, conf, resolvedExecutorProperties)
+ .buildSparkConfDirFilesMap(configMapName, conf, Map.empty)
val labels =
Map(SPARK_APP_ID_LABEL -> applicationId(), SPARK_ROLE_LABEL -> SPARK_POD_EXECUTOR_ROLE)
- val configMap = KubernetesClientUtils.buildConfigMap(configMapName, confFilesMap, labels)
+ val configMap = KubernetesClientUtils.buildConfigMap(configMapName, confFilesMap,
+ conf.get(KUBERNETES_NAMESPACE), labels)
Review comment:
We can use conf.namespace in here.
https://github.com/apache/spark/blob/17aaf8ed29e24516ca460d92b481a0b2f0545ddf/resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesConf.scala#L50
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Yikun commented on a change in pull request #35215: [SPARK-37916][K8S] The ConfigMap is assigned to incorrect namespace
Posted by GitBox <gi...@apache.org>.
Yikun commented on a change in pull request #35215:
URL: https://github.com/apache/spark/pull/35215#discussion_r790231820
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/KubernetesClusterSchedulerBackend.scala
##########
@@ -76,13 +76,12 @@ private[spark] class KubernetesClusterSchedulerBackend(
private def setUpExecutorConfigMap(driverPod: Option[Pod]): Unit = {
val configMapName = KubernetesClientUtils.configMapNameExecutor
- val resolvedExecutorProperties =
- Map(KUBERNETES_NAMESPACE.key -> conf.get(KUBERNETES_NAMESPACE))
val confFilesMap = KubernetesClientUtils
Review comment:
Of course, this is just my prefer, so I give my +1 on this PR. If the confiFilemap way can **solve the key mixup problem** (but looks like it didn't) and **also have regresson on driver side**, I'd also okay with it.
Anyway, both two way can also solve the problem of driver side, I think we need to get problem fixed first because this is a very basic case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Yikun commented on a change in pull request #35215: [SPARK-37916][K8S] The ConfigMap is assigned to incorrect namespace
Posted by GitBox <gi...@apache.org>.
Yikun commented on a change in pull request #35215:
URL: https://github.com/apache/spark/pull/35215#discussion_r790218477
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/KubernetesClusterSchedulerBackend.scala
##########
@@ -76,13 +76,12 @@ private[spark] class KubernetesClusterSchedulerBackend(
private def setUpExecutorConfigMap(driverPod: Option[Pod]): Unit = {
val configMapName = KubernetesClientUtils.configMapNameExecutor
- val resolvedExecutorProperties =
- Map(KUBERNETES_NAMESPACE.key -> conf.get(KUBERNETES_NAMESPACE))
val confFilesMap = KubernetesClientUtils
Review comment:
Thanks for clarify, but the Kubernetes allow any key as configmap key (even if the key name is 'namespace'), we should't break any behavior due to spark implemention or workaround.
So I think any key put in configmap should be covert to the real configmap.key rather than do some hack and special process to occupy any user potential input.
If we just need to set executor separately, I guess what we real needed is introducing a new conf like `spark.k8s.executor.configmap.namespace` otherwise back to `spark.k8s.namespace` directly.
And I also didn't see any problem if we just use conf.namespace like this PR done. Could you explain some If you have any other concern?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Yikun commented on a change in pull request #35215: [SPARK-37916][K8S] The ConfigMap is assigned to incorrect namespace
Posted by GitBox <gi...@apache.org>.
Yikun commented on a change in pull request #35215:
URL: https://github.com/apache/spark/pull/35215#discussion_r790218477
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/KubernetesClusterSchedulerBackend.scala
##########
@@ -76,13 +76,12 @@ private[spark] class KubernetesClusterSchedulerBackend(
private def setUpExecutorConfigMap(driverPod: Option[Pod]): Unit = {
val configMapName = KubernetesClientUtils.configMapNameExecutor
- val resolvedExecutorProperties =
- Map(KUBERNETES_NAMESPACE.key -> conf.get(KUBERNETES_NAMESPACE))
val confFilesMap = KubernetesClientUtils
Review comment:
Thanks for clarify, but the Kubernetes allow any key as configmap key (even if the key name is 'namespace'), So I think any key put in configmap should be covert to the real configmap.key rather than do some hack and special process to occupy any user potential input.
If we just need to set executor separately, I guess what we real needed is introducing a new conf like `spark.k8s.executor.configmap.namespace` otherwise back to `spark.k8s.namespace` directly.
And I also didn't see any problem if we just use conf.namespace like this PR done. Could you explain some If you have any other concern?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Yikun commented on a change in pull request #35215: [SPARK-37916][K8S] The ConfigMap is assigned to incorrect namespace
Posted by GitBox <gi...@apache.org>.
Yikun commented on a change in pull request #35215:
URL: https://github.com/apache/spark/pull/35215#discussion_r790227632
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/KubernetesClusterSchedulerBackend.scala
##########
@@ -76,13 +76,12 @@ private[spark] class KubernetesClusterSchedulerBackend(
private def setUpExecutorConfigMap(driverPod: Option[Pod]): Unit = {
val configMapName = KubernetesClientUtils.configMapNameExecutor
- val resolvedExecutorProperties =
- Map(KUBERNETES_NAMESPACE.key -> conf.get(KUBERNETES_NAMESPACE))
val confFilesMap = KubernetesClientUtils
Review comment:
> driver confileMaps sets namespace in the same ways
Do you mean [the same way](https://github.com/apache/spark/blob/c4a9772f741836cdb399e35a45b3b5df48d9eea6/resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientUtils.scala#L92) is what was introduced in https://github.com/apache/spark/pull/34983 ? Actually this is the reason of why we need this PR (regression for driver side) and where the `confFileMap` mixup problem happened.
> aiming to keep confMaps up with sparkConf.
Aggree, this is right, and this PR also can keep this regression, you can see @chia7712 already added the test for driver and executor and also avoid `confFileMap` mixup problem.
> Second, spark.k8s.executor.configmap.namespace is not necessary.
I said "**if we need**", I updated it in my last comments to avoid misleading. And also it decribed how we set namespace according conf (even if the potential separate configure case in future) rather a indirect step confFileMap.
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/KubernetesClusterSchedulerBackend.scala
##########
@@ -76,13 +76,12 @@ private[spark] class KubernetesClusterSchedulerBackend(
private def setUpExecutorConfigMap(driverPod: Option[Pod]): Unit = {
val configMapName = KubernetesClientUtils.configMapNameExecutor
- val resolvedExecutorProperties =
- Map(KUBERNETES_NAMESPACE.key -> conf.get(KUBERNETES_NAMESPACE))
val confFilesMap = KubernetesClientUtils
Review comment:
Of course, this is just my prefer, so I give my +1 on this PR. If the confiFilemap way can **solve the key mixup problem** and **also have regresson on driver side**, I'd also okay with it.
Anyway, both two way can also solve the problem of driver side, I think we need to get problem fixed first because this is a very basic case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] chia7712 commented on pull request #35215: [SPARK-37916][K8S] The ConfigMap is assigned to incorrect namespace
Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #35215:
URL: https://github.com/apache/spark/pull/35215#issuecomment-1024869090
See #35359
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #35215: [SPARK-37916][K8S] The ConfigMap is assigned to incorrect namespace
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #35215:
URL: https://github.com/apache/spark/pull/35215#issuecomment-1013769317
Can one of the admins verify this patch?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org