-
Notifications
You must be signed in to change notification settings - Fork 117
Generate the application ID label irrespective of app name. #331
Changes from all commits
2c7c786
4743c21
b617001
2dacf57
8c11cb5
b4a1b21
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,10 +19,12 @@ package org.apache.spark.deploy.kubernetes | |
| package object constants { | ||
| // Labels | ||
| private[spark] val SPARK_DRIVER_LABEL = "spark-driver" | ||
| private[spark] val SPARK_APP_ID_LABEL = "spark-app-id" | ||
| private[spark] val SPARK_APP_NAME_LABEL = "spark-app-name" | ||
| private[spark] val SPARK_APP_ID_LABEL = "spark-app-selector" | ||
| private[spark] val SPARK_EXECUTOR_ID_LABEL = "spark-exec-id" | ||
| private[spark] val SPARK_ROLE_LABEL = "spark-role" | ||
| private[spark] val SPARK_POD_DRIVER_ROLE = "driver" | ||
| private[spark] val SPARK_POD_EXECUTOR_ROLE = "executor" | ||
| private[spark] val SPARK_APP_NAME_ANNOTATION = "spark-app-name" | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should "reserved" identifiers like "spark-app-name" in annotation keys be documented?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They could be but the error message might be self-documenting enough, and I'm not sure if it's worth the space. Do you anticipate this being an annotation that people will want to set?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems more like a low-prob corner case. If it's a lot of doc, then might not be worth it. If it's low effort then I'd consider it worth doing
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mostly would be worried if there is potential for silent failures
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We send the notification here: https://github.com/apache-spark-on-k8s/spark/pull/331/files#diff-8c861c0709460ebb9571f0d44791b6beR92 |
||
|
|
||
| // Credentials secrets | ||
| private[spark] val DRIVER_CREDENTIALS_SECRETS_BASE_DIR = | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,13 +17,13 @@ | |
| package org.apache.spark.deploy.kubernetes.submit | ||
|
|
||
| import java.io.File | ||
| import java.util.Collections | ||
| import java.util.{Collections, UUID} | ||
|
|
||
| import io.fabric8.kubernetes.api.model.{ContainerBuilder, EnvVarBuilder, OwnerReferenceBuilder, PodBuilder, QuantityBuilder} | ||
| import io.fabric8.kubernetes.client.KubernetesClient | ||
| import scala.collection.JavaConverters._ | ||
|
|
||
| import org.apache.spark.{SparkConf, SparkException} | ||
| import org.apache.spark.SparkConf | ||
| import org.apache.spark.deploy.kubernetes.{ConfigurationUtils, SparkKubernetesClientFactory} | ||
| import org.apache.spark.deploy.kubernetes.config._ | ||
| import org.apache.spark.deploy.kubernetes.constants._ | ||
|
|
@@ -43,22 +43,21 @@ import org.apache.spark.util.Utils | |
| * where different steps of submission should be factored out into separate classes. | ||
| */ | ||
| private[spark] class Client( | ||
| appName: String, | ||
| kubernetesAppId: String, | ||
| mainClass: String, | ||
| sparkConf: SparkConf, | ||
| appArgs: Array[String], | ||
| sparkJars: Seq[String], | ||
| sparkFiles: Seq[String], | ||
| waitForAppCompletion: Boolean, | ||
| kubernetesClient: KubernetesClient, | ||
| initContainerComponentsProvider: DriverInitContainerComponentsProvider, | ||
| kubernetesCredentialsMounterProvider: DriverPodKubernetesCredentialsMounterProvider, | ||
| loggingPodStatusWatcher: LoggingPodStatusWatcher) | ||
| extends Logging { | ||
|
|
||
| appName: String, | ||
| kubernetesResourceNamePrefix: String, | ||
| kubernetesAppId: String, | ||
| mainClass: String, | ||
| sparkConf: SparkConf, | ||
| appArgs: Array[String], | ||
| sparkJars: Seq[String], | ||
| sparkFiles: Seq[String], | ||
| waitForAppCompletion: Boolean, | ||
| kubernetesClient: KubernetesClient, | ||
| initContainerComponentsProvider: DriverInitContainerComponentsProvider, | ||
| kubernetesCredentialsMounterProvider: DriverPodKubernetesCredentialsMounterProvider, | ||
| loggingPodStatusWatcher: LoggingPodStatusWatcher) extends Logging { | ||
| private val kubernetesDriverPodName = sparkConf.get(KUBERNETES_DRIVER_POD_NAME) | ||
| .getOrElse(kubernetesAppId) | ||
| .getOrElse(s"$kubernetesResourceNamePrefix-driver") | ||
| private val driverDockerImage = sparkConf.get(DRIVER_DOCKER_IMAGE) | ||
| private val dockerImagePullPolicy = sparkConf.get(DOCKER_IMAGE_PULL_POLICY) | ||
|
|
||
|
|
@@ -86,15 +85,16 @@ private[spark] class Client( | |
| val parsedCustomLabels = ConfigurationUtils.parseKeyValuePairs( | ||
| customLabels, KUBERNETES_DRIVER_LABELS.key, "labels") | ||
| require(!parsedCustomLabels.contains(SPARK_APP_ID_LABEL), s"Label with key " + | ||
| s" $SPARK_APP_ID_LABEL is not allowed as it is reserved for Spark bookkeeping operations.") | ||
| require(!parsedCustomLabels.contains(SPARK_APP_NAME_LABEL), s"Label with key" + | ||
| s" $SPARK_APP_NAME_LABEL is not allowed as it is reserved for Spark bookkeeping operations.") | ||
| s" $SPARK_APP_ID_LABEL is not allowed as it is reserved for Spark bookkeeping" + | ||
| s" operations.") | ||
| val parsedCustomAnnotations = ConfigurationUtils.parseKeyValuePairs( | ||
| customAnnotations, KUBERNETES_DRIVER_ANNOTATIONS.key, "annotations") | ||
| require(!parsedCustomAnnotations.contains(SPARK_APP_NAME_ANNOTATION), s"Annotation with key" + | ||
| s" $SPARK_APP_NAME_ANNOTATION is not allowed as it is reserved for Spark bookkeeping" + | ||
| s" operations.") | ||
| val allLabels = parsedCustomLabels ++ Map( | ||
| SPARK_APP_ID_LABEL -> kubernetesAppId, | ||
| SPARK_APP_NAME_LABEL -> appName, | ||
| SPARK_ROLE_LABEL -> "driver") | ||
| val parsedCustomAnnotations = ConfigurationUtils.parseKeyValuePairs( | ||
| customAnnotations, KUBERNETES_DRIVER_ANNOTATIONS.key, "annotations") | ||
| SPARK_ROLE_LABEL -> SPARK_POD_DRIVER_ROLE) | ||
|
|
||
| val driverExtraClasspathEnv = driverExtraClasspath.map { classPath => | ||
| new EnvVarBuilder() | ||
|
|
@@ -140,6 +140,7 @@ private[spark] class Client( | |
| .withName(kubernetesDriverPodName) | ||
| .addToLabels(allLabels.asJava) | ||
| .addToAnnotations(parsedCustomAnnotations.asJava) | ||
| .addToAnnotations(SPARK_APP_NAME_ANNOTATION, appName) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. annotations don't have the same restrictions labels do?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doesn't seem that way since we put in full init-container specs in the annotations. |
||
| .endMetadata() | ||
| .withNewSpec() | ||
| .withRestartPolicy("Never") | ||
|
|
@@ -186,6 +187,7 @@ private[spark] class Client( | |
| } | ||
| resolvedSparkConf.setIfMissing(KUBERNETES_DRIVER_POD_NAME, kubernetesDriverPodName) | ||
| resolvedSparkConf.set("spark.app.id", kubernetesAppId) | ||
| resolvedSparkConf.set(KUBERNETES_EXECUTOR_POD_NAME_PREFIX, kubernetesResourceNamePrefix) | ||
| // We don't need this anymore since we just set the JVM options on the environment | ||
| resolvedSparkConf.remove(org.apache.spark.internal.config.DRIVER_JAVA_OPTIONS) | ||
| val resolvedLocalClasspath = containerLocalizedFilesResolver | ||
|
|
@@ -234,11 +236,11 @@ private[spark] class Client( | |
| throw e | ||
| } | ||
| if (waitForAppCompletion) { | ||
| logInfo(s"Waiting for application $kubernetesAppId to finish...") | ||
| logInfo(s"Waiting for application $appName to finish...") | ||
| loggingPodStatusWatcher.awaitCompletion() | ||
| logInfo(s"Application $kubernetesAppId finished.") | ||
| logInfo(s"Application $appName finished.") | ||
| } else { | ||
| logInfo(s"Deployed Spark application $kubernetesAppId into Kubernetes.") | ||
| logInfo(s"Deployed Spark application $appName into Kubernetes.") | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -279,15 +281,21 @@ private[spark] object Client { | |
| val sparkFiles = sparkConf.getOption("spark.files") | ||
| .map(_.split(",")) | ||
| .getOrElse(Array.empty[String]) | ||
| val appName = sparkConf.getOption("spark.app.name") | ||
| .getOrElse("spark") | ||
| val kubernetesAppId = s"$appName-$launchTime".toLowerCase.replaceAll("\\.", "-") | ||
| val appName = sparkConf.getOption("spark.app.name").getOrElse("spark") | ||
| // The resource name prefix is derived from the application name, making it easy to connect the | ||
| // names of the Kubernetes resources from e.g. Kubectl or the Kubernetes dashboard to the | ||
| // application the user submitted. However, we can't use the application name in the label, as | ||
| // label values are considerably restrictive, e.g. must be no longer than 63 characters in | ||
| // length. So we generate a separate identifier for the app ID itself, and bookkeeping that | ||
| // requires finding "all pods for this application" should use the kubernetesAppId. | ||
| val kubernetesResourceNamePrefix = s"$appName-$launchTime".toLowerCase.replaceAll("\\.", "-") | ||
| val kubernetesAppId = s"spark-${UUID.randomUUID().toString.replaceAll("-", "")}" | ||
| val namespace = sparkConf.get(KUBERNETES_NAMESPACE) | ||
| val master = resolveK8sMaster(sparkConf.get("spark.master")) | ||
| val sslOptionsProvider = new ResourceStagingServerSslOptionsProviderImpl(sparkConf) | ||
| val initContainerComponentsProvider = new DriverInitContainerComponentsProviderImpl( | ||
| sparkConf, | ||
| kubernetesAppId, | ||
| kubernetesResourceNamePrefix, | ||
| namespace, | ||
| sparkJars, | ||
| sparkFiles, | ||
|
|
@@ -300,14 +308,16 @@ private[spark] object Client { | |
| None, | ||
| None)) { kubernetesClient => | ||
| val kubernetesCredentialsMounterProvider = | ||
| new DriverPodKubernetesCredentialsMounterProviderImpl(sparkConf, kubernetesAppId) | ||
| new DriverPodKubernetesCredentialsMounterProviderImpl( | ||
| sparkConf, kubernetesResourceNamePrefix) | ||
| val waitForAppCompletion = sparkConf.get(WAIT_FOR_APP_COMPLETION) | ||
| val loggingInterval = Option(sparkConf.get(REPORT_INTERVAL)) | ||
| .filter( _ => waitForAppCompletion) | ||
| val loggingPodStatusWatcher = new LoggingPodStatusWatcherImpl( | ||
| kubernetesAppId, loggingInterval) | ||
| kubernetesResourceNamePrefix, loggingInterval) | ||
| new Client( | ||
| appName, | ||
| kubernetesResourceNamePrefix, | ||
| kubernetesAppId, | ||
| mainClass, | ||
| sparkConf, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will this default ever be used? seems the value will be set before its ever accessed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's tricky because it's always set on the submission client, but in order to get the value directly we need to use
sparkConf.get(...). If the parameter is a config entry of type optional, then we need to call.geton the option, and require it to be present. I think it's ok if the contract is that it's always provided but there's still a default value here. At worst the executor names are incorrect but since we're doing all of our logic based on the ID label, it should not matter for correctness.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
compromise with
...getOrElse("spark")?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be more or less the same effect as this, so we might as well encode the default here - code in the config class is preferred over complexity in the scheduler backend class.