Change the API contract for uploading local files #107

mccheah · 2017-02-11T02:10:29Z

This mirrors similarly to what YARN and Mesos expects.

…te-incremental' into change-add-files-format

mccheah · 2017-02-11T02:17:22Z

Use #104 instead.

EDIT: actually using this PR after all

ash211

This is looking great @mccheah ! The main question I have is about whether we need to distinguish between the driver container and the executor container. Right now I think the code requires jars/files are baked into the same place in both driver and executor images, and doesn't support baking jars/files into just the driver and using the existing http service to distribute them to executors.

ash211 · 2017-02-16T02:48:02Z

docs/running-on-kubernetes.md

+section for details. Note that files specified with the `local` scheme should be added to the container image of both
+the driver and the executors. Files without a scheme or with the scheme `file://` are treated as being on the disk of
+the submitting machine, and are uploaded to the driver running in Kubernetes before launching the application.
+


Why did you get rid of the examples? I think it would still be useful to have examples for:

everything located on driver container:

app jar

keystore

additional files (log4j ?)

additional library jars located on executors' containers

to prevent re-distributing large jars over the driver's http service

link to advanced dependency management is great though!

The examples would merely be re-hashing what is already stated in the advanced dependency management system. The additional library jars on executor containers are just files specified with local:// and those files also have to be on the driver container... which is also covered in the advanced dependency management section. Similarly with added local files.

The SSL configuration is interesting however - I don't think the docs have been updated to reflect that.

ash211 · 2017-02-16T02:50:04Z

...rnetes/core/src/main/scala/org/apache/spark/deploy/rest/kubernetes/KubernetesFileUtils.scala

+    uris.filter(uri => schemeFilter(Option(Utils.resolveURI(uri).getScheme).getOrElse("file")))
+  }
+
+  def getOnlyContainerLocalOrRemoteFiles(uris: Iterable[String]): Iterable[String] = {


getNonSubmitterLocalFiles ? It more accurately describes the applied filter

ash211 · 2017-02-16T02:56:30Z

...urce-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/kubernetes/Client.scala

+    val submitterLocalJars = KubernetesFileUtils.getOnlySubmitterLocalFiles(sparkJars)
+    (submitterLocalFiles ++ submitterLocalJars).foreach { file =>
+      if (!new File(Utils.resolveURI(file).getPath).isFile) {
+        throw new SparkException(s"File $file is not a file or is a directory.")


add "does not exist" to this list of things that could be wrong. Does this also mean that you can't submit a file by symlink?

Heh yeah "not a file" should be "does not exist".

ash211 · 2017-02-16T02:59:33Z

...urce-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/kubernetes/Client.scala

+      case "file" =>
+        val appFile = new File(mainResourceUri.getPath)
        if (!appFile.isFile) {
          throw new IllegalStateException("Provided local file path does not exist" +


this check is now redundant with the check in line 87? remove this one since the other one is earlier and failing fast is good

ash211 · 2017-02-16T03:04:52Z

...urce-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/kubernetes/Client.scala

-      (isProvidedKeyStoreLocal, Option.apply(keyStoreURI.getPath))
+      (KubernetesFileUtils.isUriLocalFile(keyStore),
+        Option.apply(Utils.resolveURI(keyStore).getPath))
    }).getOrElse((true, Option.empty[String]))


should this be false? if no key store is given not sure it makes sense to have isLocalKeyStore=true

We ignore the property anyways if no key store is given.

ash211 · 2017-02-16T03:15:49Z

.../core/src/main/scala/org/apache/spark/deploy/rest/kubernetes/KubernetesSparkRestServer.scala

          val resourceFile = new File(tempDir, resourceName)
          val resourceFilePath = resourceFile.getAbsolutePath
          if (resourceFile.createNewFile()) {
            val resourceContentsBytes = Base64.decodeBase64(resourceContentsBase64)


this holds another copy of the file in memory -- can we unBase64 and stream straight to disk without holding the bytes array?

ash211 · 2017-02-16T03:17:39Z

.../core/src/main/scala/org/apache/spark/deploy/rest/kubernetes/KubernetesSparkRestServer.scala

+    // ought to have in the spark.jars property. The two may be different because for non-local
+    // dependencies, we have to fetch the resource (if it is not "local") but still want to use
+    // the full URI in spark.jars.
+    private def resolvedAppResource(appResource: AppResource, tempDir: File): (String, String) = {


I think this description is big enough for javadoc and especially to clarify what the (String, String) return value means. First is local absolute path and second is the value to use in spark.jars on driver launch?

ash211 · 2017-02-16T03:18:40Z

.../core/src/main/scala/org/apache/spark/deploy/rest/kubernetes/KubernetesSparkRestServer.scala

              s" to $resourceFilePath")
          }
-        case ContainerAppResource(resource) => resource
+        case ContainerAppResource(resource) => (Utils.resolveURI(resource).getPath, resource)


are we distinguishing between resources located on just the driver container vs on both driver and executor containers? In the former you need to convert to a file:/// path so the Spark driver will transfer it to executors via http and in the latter you don't do any conversion so executors read from their in-container paths.

Files with the scheme local are expected to be pre-mounted everywhere. I believe the other deploy modes do not allow for a mode of operation where files can be shipped only to the driver but are expected to also be pre-mounted on the executors.

In other words - there is no case where a container resource is given a file that is pre-mounted on one container but not mounted on the other containers. Such resources can only be specified by spark.driver.extraClassPath and spark.executor.extraClassPath.

Yep this should be fine to not support that driver-only case. If you can bake something ahead of time into the driver you ought to be able to bake it into the executor image too. Let's not differentiate between just driver or just executor resources.

ash211 · 2017-02-16T03:22:43Z

...ests/src/test/scala/org/apache/spark/deploy/kubernetes/integrationtest/KubernetesSuite.scala

+      "--jars", HELPER_JAR,
+      "--files", TEST_EXISTENCE_FILE.getAbsolutePath,
      "--class", FILE_EXISTENCE_MAIN_CLASS,
      "--conf", "spark.ui.enabled=false",


do we need a test for:

--jars with a local:/// path

--files with a local:/// path

The more I think about it the more I'm not sure what having --files with a local:// path even means. If the file is local and the user wants access to it... if they knew the path to give in the --files parameter itself, would they not also be able to just use that path directly from the containers? So I don't think testing --files with local paths is altogether too relevant, really.

It's also not clear if we should be copying those files to the working directory.

Good point that --files with local:/// doesn't do anything more than copy the files to the working directory. Maybe we should explicitly block local:// URIs from --files.

I wouldn't block it as the other cluster managers probably support it, but there's probably no need for a test there.

ash211 · 2017-02-16T03:23:44Z

docs/running-on-kubernetes.md

+
 ### Setting Up SSL For Submitting the Driver

 When submitting to Kubernetes, a pod is started for the driver, and the pod starts an HTTP server. This HTTP server


need to update the container: references to local: below in these docs (lines 77 and 79)

mccheah · 2017-02-16T18:36:08Z

Right now I think the code requires jars/files are baked into the same place in both driver and executor images, and doesn't support baking jars/files into just the driver and using the existing http service to distribute them to executors.

This is correct, and mirrors what is expected on the other runtime modes.

…te-incremental' into change-add-files-format

ash211

Doing a final sanity check on my cluster then will merge.

ash211 · 2017-02-16T23:28:35Z

Looks all good, merging!

* Change the API contract for uploading local jars. This mirrors similarly to what YARN and Mesos expects. * Address comments * Fix test

…s#107) * Change the API contract for uploading local jars. This mirrors similarly to what YARN and Mesos expects. * Address comments * Fix test

mccheah added 2 commits February 10, 2017 18:06

Change the API contract for uploading local jars.

9411194

This mirrors similarly to what YARN and Mesos expects.

Merge remote-tracking branch 'apache-spark-on-k8s/k8s-support-alterna…

c916ae2

…te-incremental' into change-add-files-format

mccheah changed the title ~~Change the API contract for uploading local jars.~~ Change the API contract for uploading local files Feb 11, 2017

mccheah closed this Feb 11, 2017

mccheah deleted the change-add-files-format branch February 11, 2017 02:32

mccheah restored the change-add-files-format branch February 11, 2017 02:32

mccheah deleted the change-add-files-format branch February 11, 2017 02:32

ash211 restored the change-add-files-format branch February 16, 2017 01:27

ash211 reopened this Feb 16, 2017

ash211 mentioned this pull request Feb 16, 2017

[WIP] Use spark.jars and spark.files instead of k8s-specific config #104

Closed

ash211 reviewed Feb 16, 2017

View reviewed changes

mccheah added 3 commits February 16, 2017 13:51

Address comments

10f84e4

Merge remote-tracking branch 'apache-spark-on-k8s/k8s-support-alterna…

c4e06bb

…te-incremental' into change-add-files-format

Fix test

720036c

ash211 approved these changes Feb 16, 2017

View reviewed changes

ash211 merged commit 51df502 into k8s-support-alternate-incremental Feb 16, 2017

ash211 deleted the change-add-files-format branch February 16, 2017 23:28

ash211 mentioned this pull request Mar 2, 2017

Match YARN behavior for spark.jars and spark.files #92

Closed

ash211 pushed a commit that referenced this pull request Mar 8, 2017

Change the API contract for uploading local files (#107)

6a999ca

* Change the API contract for uploading local jars. This mirrors similarly to what YARN and Mesos expects. * Address comments * Fix test

foxish pushed a commit that referenced this pull request Jul 24, 2017

Change the API contract for uploading local files (#107)

81c6968

* Change the API contract for uploading local jars. This mirrors similarly to what YARN and Mesos expects. * Address comments * Fix test

ifilonenko pushed a commit to ifilonenko/spark that referenced this pull request Feb 25, 2019

use hadoop 2.8.0-palantir2 (apache-spark-on-k8s#107)

a22ccff


		### Setting Up SSL For Submitting the Driver

		When submitting to Kubernetes, a pod is started for the driver, and the pod starts an HTTP server. This HTTP server

Change the API contract for uploading local files #107

Change the API contract for uploading local files #107

Uh oh!

Conversation

mccheah commented Feb 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mccheah commented Feb 11, 2017 • edited by ash211 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ash211 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mccheah Feb 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mccheah commented Feb 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ash211 left a comment

Choose a reason for hiding this comment

Uh oh!

ash211 commented Feb 16, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mccheah commented Feb 11, 2017 •

edited

Loading

mccheah commented Feb 11, 2017 •

edited by ash211

Loading

mccheah Feb 16, 2017 •

edited

Loading

mccheah commented Feb 16, 2017 •

edited

Loading