Use a headless service to give a hostname to the driver. #483

mccheah · 2017-09-07T00:20:41Z

Required since SPARK-21642 was added upstream. Closes #482.

Required since SPARK-21642 was added upstream.

mccheah · 2017-09-07T00:21:41Z

...a/org/apache/spark/deploy/kubernetes/submit/submitsteps/DriverAddressConfigurationStep.scala

+            s" managed via a Kubernetes service.")
+
+    val preferredServiceName = s"$kubernetesResourceNamePrefix$DRIVER_SVC_POSTFIX"
+    val resolvedServiceName = if (preferredServiceName.length <= MAX_SERVICE_NAME_LENGTH) {


Because the name prefix is derived from the app name, but we don't want the app name to be constrained because of service name limits. Nevertheless this still is far from ideal and I would appreciate suggestions on how we can do this better.

because the service name isn't nearly as visible to users as pod names are, I don't think naming is all that important here. What you have seems fine to me!

We might expose the driver UI using the DNS name now. So, I think it would be good to make sure it's intelligible.

There isn't a great way to shorten the name given a long app name though. It should be fine to make the name less intelligible in the corner cases where the given app name is long.

ash211 · 2017-09-07T00:22:15Z

We should cherry pick SPARK-21642 into this PR to confirm this works with it

mccheah · 2017-09-07T00:23:33Z

I wouldn't consider that a requirement for merging this into branch-2.2-kubernetes. We can double check when we propose to upstream master.

ash211 · 2017-09-07T00:25:34Z

If we're doing this change so that this works with that commit, we should test against it at the same time as we make the fix. Otherwise we're not sure this works and we'll have to figure this out again in the future, but with context lost.

What's the harm of cherry picking it in?

mccheah · 2017-09-07T00:28:58Z

It's fine because we're hard-setting spark.driver.host here. SPARK-21642 only changes what the default value of spark.driver.host is. Since we now force spark.driver.host to a particular value, our implementation is now not opinionated towards SPARK-21642.

mccheah · 2017-09-07T00:31:41Z

Speaking of which - I just thought of another way to do this, which is to just have KubernetesClusterSchedulerBackend override what the default value is for spark.driver.host so that it reverts to looking at the IP address of the driver pod. That would remove the need for using a headless service at all.

But I like the idea of having a FQDN associated with every driver pod, and using a FQDN based approach fits the spirit of what SPARK-21642 was trying to accomplish in the first place, anyways. The tradeoffs are worth considering though.

foxish · 2017-09-07T16:15:47Z

The upstream JIRA had FQDN to solve an SSL cert issue. Maybe we conditionally create the DNS name IF we have SSL turned on? I think that would be reasonable. In the other cases, spark.driver.host override that you suggested might be okay?

mccheah · 2017-09-07T18:02:57Z

How much overhead is it to create the headless service? I would rather not fork the code paths if possible to reduce the complexity.

mccheah · 2017-09-07T18:04:36Z

...a/org/apache/spark/deploy/kubernetes/submit/submitsteps/DriverAddressConfigurationStep.scala

+      .build()
+
+    val namespace = submissionSparkConf.get(KUBERNETES_NAMESPACE)
+    val driverHostname = s"${driverService.getMetadata.getName}.$namespace.svc.cluster.local"


Does this entire string need to be <= 63 characters?

just each component between the dots

foxish · 2017-09-07T18:14:45Z

How much overhead is it to create the headless service? I would rather not fork the code paths if possible to reduce the complexity.

Yeah, it wouldn't be too bad. I think we should just create the service then.
The other good thing is that provided apiserver proxy can understand the DNS name, we get a prettier URL for the driver UI.

kimoonkim · 2017-09-07T18:33:23Z

It is possible for kube-dns to be momentarily unstable. (I saw this first-hand a few times on our dev K8s cluster) Then this could affect the executor-to-driver communication and may lead to failure. Having this as an option may make jobs more resilient in case SSL is not used.

mccheah · 2017-09-07T18:38:12Z

Is the instability significant and frequent enough in most clusters to warrant the extra complexity here? Basically I don't think the cases that we would optimize for with a forked approach to use the IP address vs. the headless service, would be worthwhile if the headless service would work for us most of the time. I also foresee needing a service in general to expose the Spark UI in the not so distant future.

…er-headless-service

kimoonkim · 2017-09-07T21:27:01Z

Is the instability significant and frequent enough in most clusters to warrant the extra complexity here?

I don't know the answer. I used to see it fail more often in kube 1.5. And kube 1.6 made it better, it seems. It will really depend on how much resource kube-dns has. Or, maybe it has HA that will mitigate the risk often enough. @foxish Do we know how well kube-dns perform in general?

foxish · 2017-09-08T16:55:06Z

@kimoonkim I haven't seen kube-dns perform badly in production clusters. It may occasionally take a bit longer to resolve DNS names, but that's not common. Let's add the headless service anyway, and keep our eyes peeled for issues reported because of it; at that point, we can consider forking the code paths and overriding spark.driver.host like @mccheah said in #483 (comment). We should be good with this fix for now.

kimoonkim · 2017-09-08T16:56:18Z

SGTM.

ash211

I'm taking that as a +1 from @kimoonkim and @foxish so will merge this when builds are green.

Thanks @mccheah !

ifilonenko · 2017-10-16T18:41:10Z

I think this should be re-examined in reference to #523. As this is affecting some PRS (i.e. #514) from passing despite correctness. @mccheah @foxish @ash211

…k-on-k8s#483) * Use a headless service to give a hostname to the driver. Required since SPARK-21642 was added upstream. * Fix scalastyle. * Add back import * Fix conflict properly. * Fix orchestrator test.

Use a headless service to give a hostname to the driver.

07a1911

Required since SPARK-21642 was added upstream.

mccheah commented Sep 7, 2017

View reviewed changes

mccheah added 2 commits September 6, 2017 17:24

Merge branch 'branch-2.2-kubernetes' into driver-headless-service

03cc4eb

Fix scalastyle.

a1e5e84

mccheah mentioned this pull request Sep 7, 2017

Upstreaming and pull request strategy for Spark on Kubernetes #441

Open

mccheah commented Sep 7, 2017

View reviewed changes

mccheah added 2 commits September 7, 2017 13:49

Merge remote-tracking branch 'origin/branch-2.2-kubernetes' into driv…

e662d26

…er-headless-service

Add back import

a3e6699

mccheah added 2 commits September 7, 2017 15:08

Fix conflict properly.

98f8b61

Fix orchestrator test.

5d42d0e

Merge branch 'branch-2.2-kubernetes' into driver-headless-service

ad44711

ash211 approved these changes Sep 8, 2017

View reviewed changes

ash211 merged commit 6cebfb6 into branch-2.2-kubernetes Sep 8, 2017

ash211 deleted the driver-headless-service branch September 8, 2017 19:25

mccheah mentioned this pull request Sep 26, 2017

[NOSQUASH] Resync with Kubernetes palantir/spark#269

Merged

ash211 mentioned this pull request Sep 26, 2017

Propose config flag upstream for SPARK-21642 palantir/spark#256

Closed

liyinan926 mentioned this pull request Oct 5, 2017

Kubernetes resources created for an application should be deleted when the application finishes #519

Open

ifilonenko mentioned this pull request Oct 12, 2017

SparkPi Example: java.nio.channels.UnresolvedAddressException #523

Closed

ash211 mentioned this pull request Oct 26, 2017

[SPARK-18278] [Scheduler] Spark on Kubernetes - Basic Scheduler Backend apache/spark#19468

Closed

liyinan926 mentioned this pull request Dec 14, 2017

In-cluster client mode #456

Open

Use a headless service to give a hostname to the driver. #483

Use a headless service to give a hostname to the driver. #483

Uh oh!

Conversation

mccheah commented Sep 7, 2017

Uh oh!

mccheah Sep 7, 2017

Choose a reason for hiding this comment

Uh oh!

ash211 Sep 7, 2017

Choose a reason for hiding this comment

Uh oh!

foxish Sep 7, 2017

Choose a reason for hiding this comment

Uh oh!

mccheah Sep 7, 2017

Choose a reason for hiding this comment

Uh oh!

ash211 commented Sep 7, 2017

Uh oh!

mccheah commented Sep 7, 2017

Uh oh!

ash211 commented Sep 7, 2017

Uh oh!

mccheah commented Sep 7, 2017

Uh oh!

mccheah commented Sep 7, 2017

Uh oh!

foxish commented Sep 7, 2017

Uh oh!

mccheah commented Sep 7, 2017

Uh oh!

mccheah Sep 7, 2017

Choose a reason for hiding this comment

Uh oh!

ash211 Sep 7, 2017

Choose a reason for hiding this comment

Uh oh!

foxish commented Sep 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kimoonkim commented Sep 7, 2017

Uh oh!

mccheah commented Sep 7, 2017

Uh oh!

kimoonkim commented Sep 7, 2017

Uh oh!

foxish commented Sep 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kimoonkim commented Sep 8, 2017

Uh oh!

ash211 left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

foxish commented Sep 7, 2017 •

edited

Loading

foxish commented Sep 8, 2017 •

edited

Loading

ifilonenko commented Oct 16, 2017 •

edited

Loading