[SPARK-25262][K8S] Better support configurability of Spark scratch space when using Kubernetes #22256
Conversation
- Skip creating an emptyDir volume if the pod spec already defines an appropriate volume. This change is in preparation for SPARK-24434 changes - Provide the ability to specify that local dirs on K8S should be backed by tmpfs
Adds unit tests to the LocalDirsFeatureStepSuite to account for changes made to make local dirs configurable. Also adjusts parts of the logic in LocalDirsFeatureStep to make sure that we appropriately mount pre-defined volumes.
- Avoid creating volume mounts if pod template already defines them - Refuse to create pod if template has conflicting volume mount definitions present - Unit tests for the above - Scalastyle corrections
Adds documentation of how K8S uses local storage and how to configure it for different environments.
rvesse
changed the title
|
@skonto Here is the PR for the I have minimised config to adding a single new setting for the simple case of just wanting to use |
|
Cool @rvesse! This should the size limit right? I just noticed that: kubernetes/kubernetes#63126 |
| .addToVolumes(localDirVolumes: _*) | ||
| // Don't want to re-add volumes that already existed in the incoming spec | ||
| // as duplicate definitions will lead to K8S API errors | ||
| .addToVolumes(localDirVolumes.filter(v => !hasVolume(pod, v.getName)): _*) |
There was a problem hiding this comment.
Checking current volumes in a feature step isn't consistent with the additive design of the feature builder pattern. @mccheah to comment
There was a problem hiding this comment.
All of this conflicting volume mount and conflicting volumes seems out of place here. If we're anticipating using the pod template file, keep in mind that the pod template feature is specifically not designed to do any validation. What kinds of errors are we hoping to avoid by doing the deduplication here?
There was a problem hiding this comment.
The implementation is still additive in that it will add to existing elements in the pod spec as needed but respect what is already present.
If your pod spec contains duplicate volumes/volume mounts then K8S will reject it as invalid e.g.
The Pod "rvesse-test" is invalid: spec.volumes[1].name: Duplicate value: "spark-local-dirs-1"
Therefore it is necessary to explicitly avoid duplicating things already present in the template
If the aim is to replace adding further config options with the pod template feature then the existing builders do need to be more intelligent in what they do to avoid generating invalid pod specs. This is regardless of whether the template feature is opinionated about validation, even if the template feature doesn't do validation, Spark code itself should be ensuring that it generates valid specs as far as it is able to. Obviously it can't detect every possible invalid spec that it might generate if the templates aren't being validated but it can avoid introducing easily avoidable invalid specs.
There was a problem hiding this comment.
This is regardless of whether the template feature is opinionated about validation, even if the template feature doesn't do validation, Spark code itself should be ensuring that it generates valid specs as far as it is able to.
This is a stance that as far I'm aware, we specifically chose not to take in the pod template feature. If one is using the pod template feature then Spark won't provide any guarantees that the pod it makes will be well-formed. When spark submit deploys the pod to the cluster the API will return a clear enough error informing the user to make the appropriate corrections to their pod template.
@onursatici I just checked the pod template files PR, I didn't see this specifically called out - should this be documented?
There was a problem hiding this comment.
@mccheah yeap we should document that, will add
There was a problem hiding this comment.
This is a stance that as far I'm aware, we specifically chose not to take in the pod template feature. If one is using the pod template feature then Spark won't provide any guarantees that the pod it makes will be well-formed. When spark submit deploys the pod to the cluster the API will return a clear enough error informing the user to make the appropriate corrections to their pod template.
Sure, but we still need to be realistic about how the template feature will be used. It is supposed to enable power users to customise the pods for their environments. If there is an area like this where there is a clear use case to allow customisation we should be enabling that rather than saying sorry we're going to generate invalid pods regardless. Obviously the power user is assuming the risk of creating a pod template that meaningfully combines with Sparks generated pod to yield a valid runtime environment.
Clearly my stance here is controversial and likely needs a broader discussion on the dev list.
I can reduce this PR to just the config to enable tmpfs backed emptyDir volumes if that is preferred?
There was a problem hiding this comment.
Yeah, that might be better @rvesse
There was a problem hiding this comment.
Ok, will do that Monday
FYI I notice @onursatici has now made some similar tweaks in his latest commit - a4fde0c - notice several feature steps there now have editOrNewX() or addToX() so that they combine with rather than overriding the template
There was a problem hiding this comment.
This is different in that we're looking for specific volumes that have been set up by previous feature steps or outside logic. Preferably every step is self-contained in that it doesn't have to look up specific values set by previous steps.
For example this logic would break if we applied the templating after this step, or if a different step after this one added the volumes that are being looked up here.
Whereas editOrNew and addTo... at worst only change the ordering on some of the fields depending on when the step is invoked in the sequence.
There was a problem hiding this comment.
@mccheah @ifilonenko OK, I have opened PR #22323 with just the tmpfs enabling changes
|
@skonto I haven't done anything specific for the size limit ATM. From the K8S docs From the discussion you linked you can explicitly set a size limit for the volume but I wanted to avoid adding multiple configuration options if possible. Since this PR allows for template defined volumes to be used you could define a volume in your template with the |
|
Test build #4308 has finished for PR 22256 at commit
|
|
Can one of the admins verify this patch? |
## What changes were proposed in this pull request? The default behaviour of Spark on K8S currently is to create `emptyDir` volumes to back `SPARK_LOCAL_DIRS`. In some environments e.g. diskless compute nodes this may actually hurt performance because these are backed by the Kubelet's node storage which on a diskless node will typically be some remote network storage. Even if this is enterprise grade storage connected via a high speed interconnect the way Spark uses these directories as scratch space (lots of relatively small short lived files) has been observed to cause serious performance degradation. Therefore we would like to provide the option to use K8S's ability to instead back these `emptyDir` volumes with `tmpfs`. Therefore this PR adds a configuration option that enables `SPARK_LOCAL_DIRS` to be backed by Memory backed `emptyDir` volumes rather than the default. Documentation is added to describe both the default behaviour plus this new option and its implications. One of which is that scratch space then counts towards your pods memory limits and therefore users will need to adjust their memory requests accordingly. *NB* - This is an alternative version of PR #22256 reduced to just the `tmpfs` piece ## How was this patch tested? Ran with this option in our diskless compute environments to verify functionality Author: Rob Vesse <rvesse@dotnetrdf.org> Closes #22323 from rvesse/SPARK-25262-tmpfs.
|
Closed in favour of #22323 which has been merged |
The default behaviour of Spark on K8S currently is to create `emptyDir` volumes to back `SPARK_LOCAL_DIRS`. In some environments e.g. diskless compute nodes this may actually hurt performance because these are backed by the Kubelet's node storage which on a diskless node will typically be some remote network storage. Even if this is enterprise grade storage connected via a high speed interconnect the way Spark uses these directories as scratch space (lots of relatively small short lived files) has been observed to cause serious performance degradation. Therefore we would like to provide the option to use K8S's ability to instead back these `emptyDir` volumes with `tmpfs`. Therefore this PR adds a configuration option that enables `SPARK_LOCAL_DIRS` to be backed by Memory backed `emptyDir` volumes rather than the default. Documentation is added to describe both the default behaviour plus this new option and its implications. One of which is that scratch space then counts towards your pods memory limits and therefore users will need to adjust their memory requests accordingly. *NB* - This is an alternative version of PR apache#22256 reduced to just the `tmpfs` piece Ran with this option in our diskless compute environments to verify functionality Author: Rob Vesse <rvesse@dotnetrdf.org> Closes apache#22323 from rvesse/SPARK-25262-tmpfs.
The default behaviour of Spark on K8S currently is to create `emptyDir` volumes to back `SPARK_LOCAL_DIRS`. In some environments e.g. diskless compute nodes this may actually hurt performance because these are backed by the Kubelet's node storage which on a diskless node will typically be some remote network storage. Even if this is enterprise grade storage connected via a high speed interconnect the way Spark uses these directories as scratch space (lots of relatively small short lived files) has been observed to cause serious performance degradation. Therefore we would like to provide the option to use K8S's ability to instead back these `emptyDir` volumes with `tmpfs`. Therefore this PR adds a configuration option that enables `SPARK_LOCAL_DIRS` to be backed by Memory backed `emptyDir` volumes rather than the default. Documentation is added to describe both the default behaviour plus this new option and its implications. One of which is that scratch space then counts towards your pods memory limits and therefore users will need to adjust their memory requests accordingly. *NB* - This is an alternative version of PR apache#22256 reduced to just the `tmpfs` piece Ran with this option in our diskless compute environments to verify functionality Author: Rob Vesse <rvesse@dotnetrdf.org> Closes apache#22323 from rvesse/SPARK-25262-tmpfs.
What changes were proposed in this pull request?
This change improves how Spark on Kubernetes creates the local directories used for Spark scratch space i.e.
SPARK_LOCAL_DIRS/spark.local.dirsCurrently Spark on Kubernetes creates each defined local directory, or a single default directory if none defined, as a Kubernetes
emptyDirvolume mounted into the containers. The problem is thatemptyDirdirectories are backed by the node storage and so for some compute environments e.g. diskless any "local" storage is actually provided by some remote file system that may actually harm performance when jobs use it heavily.Kubernetes provides the option to have
emptyDirvolumes backed bytmpfsi.e. RAM on the nodes so we introduce a booleanspark.kubernetes.local.dirs.tmpfsoption that when true causes the createdemptyDirvolumes to use memory.A second related problem is that because Spark on Kubernetes always generates
emptyDirvolumes users have no way to use alternative volume types that may be available in their cluster.No new options specific to this problem are introduced but the code is modified to detect when the pod spec already defines an appropriately named volume and to avoid creating
emptyDirvolumes in this case. This uses the convention of the existing code that volumes for scratch space are namedspark-local-dirs-Nnumbered from 1-N based on the number of entries defined in theSPARK_LOCAL_DIRS/spark.local.dirssetting. This is done in anticipation of the pod template feature form SPARK-24434 (PR #22146) being merged since that will allow users to define custom volumes more easily.Tasks:
tmpfsvolumesHow was this patch tested?
Unit tests added to the relevant feature step to exercise the new configuration option and to check that pre-existing volumes are used. Plan to add further unit tests to check some other corner cases.