Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SPARK-24434][K8S] pod template files #22146

Closed
wants to merge 57 commits into from
Closed
Show file tree
Hide file tree
Changes from 34 commits
Commits
Show all changes
57 commits
Select commit Hold shift + click to select a range
e2e7223
add spec template configs
onursatici Aug 17, 2018
ea4dde6
start from template for driver
onursatici Aug 17, 2018
4f088db
start from template for executor
onursatici Aug 17, 2018
f2f9a44
wip
onursatici Aug 17, 2018
368d0a4
volume executor podspec template
onursatici Aug 17, 2018
0005ea5
move logic to apply functions
onursatici Aug 17, 2018
dda5cc9
find containers
onursatici Aug 17, 2018
d0f41aa
style
onursatici Aug 17, 2018
c4c1231
remove import
onursatici Aug 17, 2018
74de0e5
compiles
yifeih Aug 21, 2018
205ddd3
tests pass
yifeih Aug 21, 2018
4ae6fc6
adding TemplateVolumeStepSuite
yifeih Aug 21, 2018
c0bcfea
DriverBuilder test
yifeih Aug 21, 2018
b9e4263
WIP trying to write tests for KubernetesDriverBuilder constructor
yifeih Aug 21, 2018
c5e1ea0
fix test
yifeih Aug 21, 2018
56a6b32
fix test, and move loading logic to util method
yifeih Aug 22, 2018
7d0d928
validate that the executor pod template is good in the driver
yifeih Aug 22, 2018
1da79a8
cleaning
yifeih Aug 22, 2018
8ef756e
Merge branch 'apache/master' into yh/pod-template
yifeih Aug 22, 2018
7f3cb04
redo mounting file
yifeih Aug 23, 2018
cc8d3f8
rename to TemplateConfigMapStep
yifeih Aug 23, 2018
4119899
Pass initialPod constructor instead of Spec constructor
yifeih Aug 23, 2018
1d0a8fa
make driver and executor container names configurable
yifeih Aug 23, 2018
81e5a66
create temp file correctly?
yifeih Aug 23, 2018
7f4ff5a
executor initial pod test
yifeih Aug 23, 2018
3097aef
add docs
yifeih Aug 23, 2018
9b1418a
addressing some comments
yifeih Aug 23, 2018
ebacc96
integration tests attempt 1
yifeih Aug 24, 2018
98acd29
fix up docs
yifeih Aug 24, 2018
95f8b8b
rename a variable
yifeih Aug 24, 2018
da5dff5
fix style?
yifeih Aug 24, 2018
7fb76c7
fix docs to remove container name conf and further clarify
yifeih Aug 25, 2018
d86bc75
actually add the pod template test
yifeih Aug 25, 2018
f2720a5
remove containerName confs
yifeih Aug 25, 2018
4b3950d
test tag and indent
onursatici Aug 27, 2018
3813fcb
extension
onursatici Aug 28, 2018
ec04323
use resources for integartion tests templates
onursatici Aug 28, 2018
f3b6082
rat
onursatici Aug 28, 2018
fd503db
fix path
onursatici Aug 29, 2018
eeb2492
prevent having duplicate containers
onursatici Aug 29, 2018
4801e8e
Merge remote-tracking branch 'origin/master' into pod-template
onursatici Aug 29, 2018
36a70ad
do not use broken removeContainer
onursatici Aug 29, 2018
ece7a7c
nits
onursatici Aug 29, 2018
8b8aa48
inline integration test methods, add volume to executor builder unit …
onursatici Aug 30, 2018
1ed95ab
do not raise twice on template parse failuer
onursatici Aug 31, 2018
a4fde0c
add comprehensive test for supported template features
onursatici Aug 31, 2018
140e89c
generalize tests to cover both driver and executor builders
onursatici Aug 31, 2018
838c2bd
docs
onursatici Sep 4, 2018
5faea62
Merge remote-tracking branch 'origin/master' into pod-template
onursatici Oct 29, 2018
9e6a4b2
fix tests, templates does not support changing executor pod names
onursatici Oct 29, 2018
c8077dc
config to select spark containers in pod templates
onursatici Oct 29, 2018
3d6ff3b
more readable select container logic
onursatici Oct 29, 2018
83087eb
fix integration tests
onursatici Oct 29, 2018
a46b885
Merge remote-tracking branch 'origin/master' into pod-template
onursatici Oct 29, 2018
80b56c1
address comments
onursatici Oct 29, 2018
8f7f571
rename pod template volume name
onursatici Oct 30, 2018
3707e6a
imports
onursatici Oct 30, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
179 changes: 179 additions & 0 deletions docs/running-on-kubernetes.md
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,21 @@ To use a secret through an environment variable use the following options to the
--conf spark.kubernetes.executor.secretKeyRef.ENV_NAME=name:key
```

## Pod Template
Kubernetes allows defining pods from [template files](https://kubernetes.io/docs/concepts/workloads/pods/pod-overview/#pod-templates).
Spark users can similarly use template files to define the driver or executor pod configurations that Spark configurations do not support.
To do so, specify the spark properties `spark.kubernetes.driver.podTemplateFile` and `spark.kubernetes.executor.podTemplateFile`
to point to local files accessible to the `spark-submit` process. To allow the driver pod access the executor pod template
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you considered a single config map created at submission time, from which both driver and executors pull their appropriate templates?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't think we considered this, but, an interesting proposal. I think that can be a follow up feature if requested.

file, the file will be automatically mounted onto a volume in the driver pod when it's created.

It is important to note that Spark is opinionated about certain pod configurations so there are values in the
pod template that will always be overwritten by Spark. Therefore, users of this feature should note that specifying
the pod template file only lets Spark start with a template pod instead of an empty pod during the pod-building process.
For details, see the [full list](#pod-template-properties) of pod template values that will be overwritten by spark.

Pod template files can also define multiple containers. In such cases, Spark will always assume that the first container in
the list will be the driver or executor container.
Copy link
Contributor

@skonto skonto Aug 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to use only extra containers and not Spark specific with this approach? Could we have a naming convention or a less error prone convention?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR originally had an extra spark conf for container names, but we have decided to use the first container in the template instead. Users can have an empty first container in the pod spec template if they only want to add containers without changing Spark's executor or driver container

Copy link
Contributor

@skonto skonto Aug 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having a first container empty looks redundant to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@skonto True, but this prevents the addition of a new spark conf for the container name


## Introspection and Debugging

These are the different ways in which you can investigate a running/completed Spark application, monitor progress, and
Expand Down Expand Up @@ -775,4 +790,168 @@ specific to Spark on Kubernetes.
This sets the major Python version of the docker image used to run the driver and executor containers. Can either be 2 or 3.
</td>
</tr>
<tr>
<td><code>spark.kubernetes.driver.podTemplateFile</code></td>
<td>(none)</td>
<td>
Specify the local file that contains the driver [pod template](#pod-template). For example
<code>spark.kubernetes.driver.podTemplateFile=/path/to/driver-pod-template.yaml`</code>
</td>
</tr>
<tr>
<td><code>spark.kubernetes.executor.podTemplateFile</code></td>
<td>(none)</td>
<td>
Specify the local file that contains the executor [pod template](#pod-template). For example
<code>spark.kubernetes.executor.podTemplateFile=/path/to/executor-pod-template.yaml`</code>
</td>
</tr>
</table>

#### Pod template properties

See the below table for the full list of pod specifications that will be overwritten by spark.

### Pod Metadata

<table class="table">
<tr><th>Pod metadata key</th><th>Modified value</th><th>Description</th></tr>
<tr>
<td>name</td>
<td>Value of <code>spark.kubernetes.driver.pod.name</code></td>
<td>
The driver pod name will be overwritten with either the configured or default value of
<code>spark.kubernetes.driver.pod.name</code>. The executor pod names will be unaffected.
</td>
</tr>
<tr>
<td>namespace</td>
<td>Value of <code>spark.kubernetes.namespace</code></td>
<td>
Spark makes strong assumptions about the driver and executor namespaces. Both driver and executor namespaces will
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the spark conf value for namespace isn't set, can spark use the template setting, or will spark's conf default also override the template?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will also be replaced with the spark conf's default value. I'll update this description to match that of name right above this

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be the default namespace.

be replaced by either the configured or default spark conf value.
</td>
</tr>
<tr>
<td>labels</td>
<td>Adds the labels from <code>spark.kubernetes.{driver,executor}.label.*</code></td>
<td>
Spark will add additional labels specified by the spark configuration.
</td>
</tr>
<tr>
<td>annotations</td>
<td>Adds the annotations from <code>spark.kubernetes.{driver,executor}.annotation.*</code></td>
<td>
Spark will add additional labels specified by the spark configuration.
</td>
</tr>
</table>

### Pod Spec

<table class="table">
<tr><th>Pod spec key</th><th>Modified value</th><th>Description</th></tr>
<tr>
<td>imagePullSecrets</td>
<td>Adds image pull secrets from <code>spark.kubernetes.container.image.pullSecrets</code></td>
<td>
Additional pull secrets will be added from the spark configuration to both executor pods.
</td>
</tr>
<tr>
<td>nodeSelector</td>
<td>Adds node selectors from <code>spark.kubernetes.node.selector.*</code></td>
<td>
Additional node selectors will be added from the spark configuration to both executor pods.
</td>
</tr>
<tr>
<td>restartPolicy</td>
<td><code>"never"</code></td>
<td>
Spark assumes that both drivers and executors never restart.
</td>
</tr>
<tr>
<td>serviceAccount</td>
<td>Value of <code>spark.kubernetes.authenticate.driver.serviceAccountName</code></td>
<td>
Spark will override <code>serviceAccount</code> with the value of the spark configuration for only
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similar Q to namespace: if no spark-conf is set, will spark's conf default override here as well?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

driver pods, and only if the spark configuration is specified. Executor pods will remain unaffected.
</td>
</tr>
<tr>
<td>serviceAccountName</td>
<td>Value of <code>spark.kubernetes.authenticate.driver.serviceAccountName</code></td>
<td>
Spark will override <code>serviceAccountName</code> with the value of the spark configuration for only
driver pods, and only if the spark configuration is specified. Executor pods will remain unaffected.
</td>
</tr>
<tr>
<td>volumes</td>
<td>Adds volumes from <code>spark.kubernetes.{driver,executor}.volumes.[VolumeType].[VolumeName].mount.path</code></td>
<td>
Spark will add volumes as specified by the spark conf, as well as additional volumes necessary for passing
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should also document the volume names (and optionally mount points) for all the "internal" spark volumes (pod download, config map, etc).
As people will use the pod template to also mount their own volumes, we should document the existing names so we can avoid name clashes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

As I've commented elsewhere it is easily possible to create invalid specs with this feature because Spark will create certain volumes, config maps etc with known name patterns that users need to avoid

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have done exactly that even w/o this feature - I was trying to use the spark operator to mount a config map and accidentally hit upon the spark config volume on my first try :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1. As I mentioned before we need to know the implications of whatever property we expose.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One way we can avoid conflicting volumes entirely is by randomizing the name of the volumes added by features, e.g. appending some UUID or at least some large integer. I think keeping running documentation on all volumes we add from features is too much overhead. If we run into these conflicts often then we can do this, but I think it's fine not to block merging on that.

Either way though I think again, the validation piece can be done separately from this PR. I wouldn't consider that documentation as blocking on this merging. Thoughts?

spark conf and pod template files.
</td>
</tr>
</table>

### Container spec

The following affect the driver and executor containers. All other containers in the pod spec will be unaffected.

<table class="table">
<tr><th>Container spec key</th><th>Modified value</th><th>Description</th></tr>
<tr>
<td>env</td>
<td>Adds env variables from <code>spark.kubernetes.driverEnv.[EnvironmentVariableName]</code></td>
<td>
Spark will add driver env variables from <code>spark.kubernetes.driverEnv.[EnvironmentVariableName]</code>, and
executor env variables from <code>spark.executorEnv.[EnvironmentVariableName]</code>.
</td>
</tr>
<tr>
<td>image</td>
<td>Value of <code>spark.kubernetes.{driver,executor}.container.image</code></td>
<td>
The image will be defined by the spark configurations.
</td>
</tr>
<tr>
<td>imagePullPolicy</td>
<td>Value of <code>spark.kubernetes.container.image.pullPolicy</code></td>
<td>
Spark will override the pull policy for both driver and executors.
</td>
</tr>
<tr>
<td>name</td>
<td>See description.</code></td>
<td>
The container name will be assigned by spark ("spark-kubernetes-driver" for the driver container, and
"executor" for each executor container) if not defined by the pod template. If the container is defined by the
template, the template's name will be used.
</td>
</tr>
<tr>
<td>resources</td>
<td>See description</td>
<td>
The cpu limits are set by <code>spark.kubernetes.{driver,executor}.limit.cores</code>. The cpu is set by
<code>spark.{driver,executor}.cores</code>. The memory request and limit are set by summing the values of
<code>spark.{driver,executor}.memory</code> and <code>spark.{driver,executor}.memoryOverhead</code>.

</td>
</tr>
<tr>
<td>volumeMounts</td>
<td>Add volumes from <code>spark.kubernetes.driver.volumes.[VolumeType].[VolumeName].mount.{path,readOnly}</code></td>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just checking, is it add or replace? I'm hoping one could use this to mount unsupported volume types, like config maps or secrets, in addition to those managed by spark.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aditanase that was the purpose of the design doc: https://docs.google.com/document/d/1pcyH5f610X2jyJW9WbWHnj8jktQPLlbbmmUwdeK4fJk capture what behavior we want for each case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Volumes should be additive. If names are duplicated though I'd expect K8s to throw an error.

<td>
Spark will add volumes as specified by the spark conf, as well as additional volumes necessary for passing
spark conf and pod template files.
</td>
</tr>
</table>
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,18 @@ private[spark] object Config extends Logging {
"Ensure that major Python version is either Python2 or Python3")
.createWithDefault("2")

val KUBERNETES_DRIVER_PODTEMPLATE_FILE =
ConfigBuilder("spark.kubernetes.driver.podTemplateFile")
.doc("File containing a template pod spec for the driver")
.stringConf
.createOptional

val KUBERNETES_EXECUTOR_PODTEMPLATE_FILE =
ConfigBuilder("spark.kubernetes.executor.podTemplateFile")
.doc("File containing a template pod spec for executors")
.stringConf
.createOptional

val KUBERNETES_AUTH_SUBMISSION_CONF_PREFIX =
"spark.kubernetes.authenticate.submission"

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,16 @@ private[spark] object Constants {
val ENV_R_PRIMARY = "R_PRIMARY"
val ENV_R_ARGS = "R_APP_ARGS"

// Pod spec templates
val EXECUTOR_POD_SPEC_TEMPLATE_FILE_NAME = "pod-spec-template.yml"
val EXECUTOR_POD_SPEC_TEMPLATE_MOUNTHPATH = "/opt/spark/pod-template"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spelling, think we want EXECUTOR_POD_SPEC_TEMPLATE_MOUNTPATH?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick ping here

val POD_TEMPLATE_VOLUME = "podspec-volume"
Copy link
Contributor

@skonto skonto Aug 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: s/podspec-volume/pod-template-volume
You are passing the whole template right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ping here

val POD_TEMPLATE_CONFIGMAP = "podspec-configmap"
val POD_TEMPLATE_KEY = "podspec-configmap-key"

// Miscellaneous
val KUBERNETES_MASTER_INTERNAL_URL = "https://kubernetes.default.svc"
val DRIVER_CONTAINER_NAME = "spark-kubernetes-driver"
val DEFAULT_DRIVER_CONTAINER_NAME = "spark-kubernetes-driver"
val DEFAULT_EXECUTOR_CONTAINER_NAME = "executor"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/executor/spark-kubernetes-executor

val MEMORY_OVERHEAD_MIN_MIB = 384L
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,9 @@ private[spark] case class KubernetesDriverSpec(
systemProperties: Map[String, String])

private[spark] object KubernetesDriverSpec {
def initialSpec(initialProps: Map[String, String]): KubernetesDriverSpec = KubernetesDriverSpec(
SparkPod.initialPod(),
Seq.empty,
initialProps)
def initialSpec(initialConf: KubernetesConf[KubernetesDriverSpecificConf]): KubernetesDriverSpec =
KubernetesDriverSpec(
SparkPod.initialPod(),
Seq.empty,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: For clarity can you write as:

    KubernetesDriverSpec(
      SparkPod.initialPod(),
      driverKubernetesResources = Seq.empty,
      initialConf.sparkConf.getAll.toMap)

initialConf.sparkConf.getAll.toMap)
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,17 @@
*/
package org.apache.spark.deploy.k8s

import org.apache.spark.SparkConf
import java.io.File

import io.fabric8.kubernetes.api.model.ContainerBuilder
import io.fabric8.kubernetes.client.KubernetesClient
import scala.collection.JavaConverters._

import org.apache.spark.{SparkConf, SparkException}
import org.apache.spark.internal.Logging
import org.apache.spark.util.Utils

private[spark] object KubernetesUtils {
private[spark] object KubernetesUtils extends Logging {

/**
* Extract and parse Spark configuration properties with a given name prefix and
Expand Down Expand Up @@ -59,5 +66,23 @@ private[spark] object KubernetesUtils {
}
}

def loadPodFromTemplate(
kubernetesClient: KubernetesClient,
templateFile: File): SparkPod = {
try {
val pod = kubernetesClient.pods().load(templateFile).get()
val containers = pod.getSpec.getContainers.asScala
if (containers.nonEmpty) {
SparkPod(pod, new ContainerBuilder().build())
}
SparkPod(pod, containers.head)
} catch {
case e: Exception =>
logError(
s"Encountered exception while attempting to load initial pod spec from file", e)
throw new SparkException("Could not load driver pod from template file.", e)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error message is misleading, it throws when both executor and driver pod failed to load from its own template. Either remove "driver" or be more specific that its executor or driver.

}
}

def parseMasterUrl(url: String): String = url.substring("k8s://".length)
}
Original file line number Diff line number Diff line change
Expand Up @@ -31,4 +31,15 @@ private[spark] object SparkPod {
.build(),
new ContainerBuilder().build())
}

def initialPodWithContainerName(name: String): SparkPod = {
SparkPod(
new PodBuilder()
.withNewMetadata()
.endMetadata()
.withNewSpec()
.endSpec()
.build(),
new ContainerBuilder().withName(name).build())
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,10 @@ private[spark] class BasicDriverFeatureStep(
DEFAULT_BLOCKMANAGER_PORT
)
val driverUIPort = SparkUI.getUIPort(conf.sparkConf)
val driverContainerName = if (pod.container.getName == null) DEFAULT_DRIVER_CONTAINER_NAME
else pod.container.getName
val driverContainer = new ContainerBuilder(pod.container)
.withName(DRIVER_CONTAINER_NAME)
.withName(driverContainerName)
.withImage(driverContainerImage)
.withImagePullPolicy(conf.imagePullPolicy())
.addNewPort()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -128,8 +128,10 @@ private[spark] class BasicExecutorFeatureStep(
.build()
}

val executorContainerName = if (pod.container.getName == null) DEFAULT_EXECUTOR_CONTAINER_NAME
else pod.container.getName
val executorContainer = new ContainerBuilder(pod.container)
.withName("executor")
.withName(executorContainerName)
.withImage(executorContainerImage)
.withImagePullPolicy(kubernetesConf.imagePullPolicy())
.withNewResources()
Expand Down Expand Up @@ -163,8 +165,8 @@ private[spark] class BasicExecutorFeatureStep(
val executorPod = new PodBuilder(pod.pod)
.editOrNewMetadata()
.withName(name)
.withLabels(kubernetesConf.roleLabels.asJava)
.withAnnotations(kubernetesConf.roleAnnotations.asJava)
.addToLabels(kubernetesConf.roleLabels.asJava)
.addToAnnotations(kubernetesConf.roleAnnotations.asJava)
.addToOwnerReferences(ownerReference.toSeq: _*)
.endMetadata()
.editOrNewSpec()
Expand Down
Loading