erikerlandson commented on a change in pull request #29844:
URL: https://github.com/apache/spark/pull/29844#discussion_r494423864
##########
File path:
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/Config.scala
##########
@@ -81,6 +80,13 @@ private[spark] object Config extends Logging {
.stringConf
.createOptional
+ val KUBERNETES_EXECUTOR_SERVICE_ACCOUNT_NAME =
+ ConfigBuilder(s"$KUBERNETES_AUTH_EXECUTOR_CONF_PREFIX.serviceAccountName")
+ .doc("Service account that is used when running the executor pod." +
+ "If this parameter is not setup, the fallback logic will use the
driver's service account.")
Review comment:
It would be more backward compatible to default the executor SA to "none"
##########
File path:
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/Config.scala
##########
@@ -89,9 +95,9 @@ private[spark] object Config extends Logging {
val KUBERNETES_DRIVER_SUBMIT_CHECK =
ConfigBuilder("spark.kubernetes.submitInDriver")
- .internal()
- .booleanConf
- .createWithDefault(false)
+ .internal()
Review comment:
this is a nit, but pure formatting changes are not good PR practice.
##########
File path:
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesUtils.scala
##########
@@ -60,4 +63,15 @@ private[spark] object KubernetesUtils {
}
def parseMasterUrl(url: String): String = url.substring("k8s://".length)
+
+ def buildPodWithServiceAccount(serviceAccount: Option[String], pod:
SparkPod): Option[Pod] = {
Review comment:
nit: would call this operation "add service account"
##########
File path:
resource-managers/kubernetes/integration-tests/src/test/scala/org/apache/spark/deploy/k8s/integrationtest/BasicTestsSuite.scala
##########
@@ -84,6 +84,13 @@ private[spark] trait BasicTestsSuite { k8sSuite:
KubernetesSuite =>
})
}
+ test("All pods have the same service account by default", k8sTestTag) {
Review comment:
should discuss whether desired behavior is "executor SA defaults to
driver" or "defaults to none"
##########
File path:
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesUtils.scala
##########
@@ -60,4 +63,15 @@ private[spark] object KubernetesUtils {
}
def parseMasterUrl(url: String): String = url.substring("k8s://".length)
+
+ def buildPodWithServiceAccount(serviceAccount: Option[String], pod:
SparkPod): Option[Pod] = {
Review comment:
Can the `pod` parameter here be null, or empty?
##########
File path:
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesUtils.scala
##########
@@ -60,4 +63,15 @@ private[spark] object KubernetesUtils {
}
def parseMasterUrl(url: String): String = url.substring("k8s://".length)
+
+ def buildPodWithServiceAccount(serviceAccount: Option[String], pod:
SparkPod): Option[Pod] = {
Review comment:
sorry, I forgot that this was being pulled back to 2.4 as a bug-fix.
Should probably keep it consistent w/ 3.0
##########
File path:
resource-managers/kubernetes/integration-tests/src/test/scala/org/apache/spark/deploy/k8s/integrationtest/BasicTestsSuite.scala
##########
@@ -84,6 +84,13 @@ private[spark] trait BasicTestsSuite { k8sSuite:
KubernetesSuite =>
})
}
+ test("All pods have the same service account by default", k8sTestTag) {
Review comment:
ignore this, since it is being pulled back from 3.0
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
