-
Notifications
You must be signed in to change notification settings - Fork 28.4k
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-22646] [Submission] Spark on Kubernetes - basic submission client #19717
Changes from all commits
dcaac45
27c67ff
6d597d0
5b9fa39
5ccadb5
12f2797
c35fe48
faa2849
347ed69
0e8ca01
3a0b8e3
83d0b9c
44c40b1
67bc847
7d2b303
caf2206
2e7810b
cbcd30e
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 |
---|---|---|
|
@@ -2744,6 +2744,42 @@ private[spark] object Utils extends Logging { | |
} | ||
} | ||
|
||
/** | ||
* Check the validity of the given Kubernetes master URL and return the resolved URL. Prefix | ||
* "k8s:" is appended to the resolved URL as the prefix is used by KubernetesClusterManager | ||
* in canCreate to determine if the KubernetesClusterManager should be used. | ||
*/ | ||
def checkAndGetK8sMasterUrl(rawMasterURL: String): String = { | ||
require(rawMasterURL.startsWith("k8s://"), | ||
"Kubernetes master URL must start with k8s://.") | ||
val masterWithoutK8sPrefix = rawMasterURL.substring("k8s://".length) | ||
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. Can we change this String representation to 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. Done. 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. With the changes, URLs like 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. All of the following should be supported and resolved as follows:
Think we just need to use whatever code that is necessary to get to this state. 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. Are those URIs set in stone? This seems more readable to me:
It also allows parsing using the 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. This decision is to match how Mesos works with Zookeeper, as these Strings are valid IIRC: 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. 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. OK, did some changes to make all supported formats work. See 51844cc. |
||
|
||
// To handle master URLs, e.g., k8s://host:port. | ||
if (!masterWithoutK8sPrefix.contains("://")) { | ||
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. startsWith instead of contains? 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. This is used to differentiate |
||
val resolvedURL = s"https://$masterWithoutK8sPrefix" | ||
logInfo("No scheme specified for kubernetes master URL, so defaulting to https. Resolved " + | ||
s"URL is $resolvedURL.") | ||
return s"k8s:$resolvedURL" | ||
} | ||
|
||
val masterScheme = new URI(masterWithoutK8sPrefix).getScheme | ||
val resolvedURL = masterScheme.toLowerCase match { | ||
case "https" => | ||
masterWithoutK8sPrefix | ||
case "http" => | ||
logWarning("Kubernetes master URL uses HTTP instead of HTTPS.") | ||
masterWithoutK8sPrefix | ||
case null => | ||
val resolvedURL = s"https://$masterWithoutK8sPrefix" | ||
logInfo("No scheme specified for kubernetes master URL, so defaulting to https. Resolved " + | ||
s"URL is $resolvedURL.") | ||
resolvedURL | ||
case _ => | ||
throw new IllegalArgumentException("Invalid Kubernetes master scheme: " + masterScheme) | ||
} | ||
|
||
return s"k8s:$resolvedURL" | ||
} | ||
} | ||
|
||
private[util] object CallerContext extends Logging { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -388,6 +388,33 @@ class SparkSubmitSuite | |
conf.get("spark.ui.enabled") should be ("false") | ||
} | ||
|
||
test("handles k8s cluster mode") { | ||
val clArgs = Seq( | ||
"--deploy-mode", "cluster", | ||
"--master", "k8s://host:port", | ||
"--executor-memory", "5g", | ||
"--class", "org.SomeClass", | ||
"--driver-memory", "4g", | ||
"--conf", "spark.kubernetes.namespace=spark", | ||
"--conf", "spark.kubernetes.driver.docker.image=bar", | ||
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 we also test the arg "--kubernetes-namespace"? 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.
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. 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. Done. |
||
"/home/thejar.jar", | ||
"arg1") | ||
val appArgs = new SparkSubmitArguments(clArgs) | ||
val (childArgs, classpath, conf, mainClass) = prepareSubmitEnvironment(appArgs) | ||
|
||
val childArgsMap = childArgs.grouped(2).map(a => a(0) -> a(1)).toMap | ||
childArgsMap.get("--primary-java-resource") should be (Some("file:/home/thejar.jar")) | ||
childArgsMap.get("--main-class") should be (Some("org.SomeClass")) | ||
childArgsMap.get("--arg") should be (Some("arg1")) | ||
mainClass should be (KUBERNETES_CLUSTER_SUBMIT_CLASS) | ||
classpath should have length (0) | ||
conf.get("spark.master") should be ("k8s:https://host:port") | ||
conf.get("spark.executor.memory") should be ("5g") | ||
conf.get("spark.driver.memory") should be ("4g") | ||
conf.get("spark.kubernetes.namespace") should be ("spark") | ||
conf.get("spark.kubernetes.driver.docker.image") should be ("bar") | ||
} | ||
|
||
test("handles confs with flag equivalents") { | ||
val clArgs = Seq( | ||
"--deploy-mode", "cluster", | ||
|
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.
nit: should also add doc for this config here.
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.
Yes, it's added under
spark.driver.memory
. See https://github.com/apache-spark-on-k8s/spark/blob/0e8ca012dc6d5dfd5645f45b6d1bb84e1e9e72a5/docs/configuration.md.