[SPARK-24960][K8S] explicitly expose ports on driver container #21884

adelbertc · 2018-07-26T17:45:19Z

https://issues.apache.org/jira/browse/SPARK-24960

What changes were proposed in this pull request?

Expose ports explicitly in the driver container. The driver Service created expects to reach the driver Pod at specific ports which before this change, were not explicitly exposed and would likely cause connection issues (see apache-spark-on-k8s#617).

This is a port of the original PR created in the now-deprecated Kubernetes fork: apache-spark-on-k8s#618

How was this patch tested?

Failure in apache-spark-on-k8s#617 reproduced on Kubernetes 1.6.x and 1.8.x. Built the driver image with this patch and observed fixed apache-spark-on-k8s#617 on Kubernetes 1.6.x.

felixcheung · 2018-07-27T06:25:11Z

can you update the format of the title and description as described here
"Pull Request" in https://spark.apache.org/contributing.html

adelbertc · 2018-07-27T17:16:58Z

@felixcheung Done I think, is that OK?

mccheah · 2018-07-27T20:27:41Z

This needs a Spark ticket. I also don't think pods specifically have to expose ports. My understanding was that the ports field on the pod was only to assign ports names, so that the ports can be referenced by name and not by number on service objects.

adelbertc · 2018-07-28T19:34:54Z

@mccheah Done https://issues.apache.org/jira/browse/SPARK-24960

Re: Exposing ports, my understanding of Kubernetes' networking model is it is possible for a cluster to be setup such that Pod ports are closed, perhaps as a security measure. At least in our Kubernetes cluster (tested both on 1.6.x and 1.8.x) the SparkPi example failed with a connection error without this change, and succeeded after. (I've also added this information to the JIRA)

liyinan926 · 2018-07-30T22:07:23Z

...rnetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/BasicDriverFeatureStep.scala

    val driverContainer = new ContainerBuilder(pod.container)
      .withName(DRIVER_CONTAINER_NAME)
      .withImage(driverContainerImage)
      .withImagePullPolicy(conf.imagePullPolicy())
+      .addNewPort()


The driver UI port should also be explicitly exposed.

mccheah · 2018-07-30T23:00:02Z

At least in our Kubernetes cluster (tested both on 1.6.x and 1.8.x) the SparkPi example failed with a connection error without this change, and succeeded after. (I've also added this information to the JIRA)

@adelbertc Just to clarify, by "succeeded later", do you mean after this change was applied? Or do you mean "succeeded later" as in a retry worked with the same code?

adelbertc · 2018-07-31T00:07:36Z

@mccheah After the change was applied

liyinan926

LGTM.

mccheah · 2018-07-31T19:27:32Z

Can you quickly change [Kubernetes] to [K8S] in the PR title? Will merge after that is done.

mccheah · 2018-07-31T19:28:15Z

Actually can you also update the unit test? Thanks!

adelbertc · 2018-07-31T21:59:31Z

@mccheah Done and done

mccheah · 2018-07-31T22:07:03Z

...s/core/src/test/scala/org/apache/spark/deploy/k8s/features/BasicDriverFeatureStepSuite.scala

@@ -87,6 +87,10 @@ class BasicDriverFeatureStepSuite extends SparkFunSuite {
    assert(configuredPod.container.getImage === "spark-driver:latest")
    assert(configuredPod.container.getImagePullPolicy === CONTAINER_IMAGE_PULL_POLICY)

+    val expectedPortNames = Set(DRIVER_PORT_NAME, BLOCK_MANAGER_PORT_NAME, UI_PORT_NAME)
+    val foundPortNames = configuredPod.container.getPorts.asScala.map(port => port.getName).toSet


Better to be safer here and check the protocol and the target port numbers. You can just compare against a list of expected ContainerPort objects - Kubernetes objects in our Java API have been good about giving us sane equals implementations.

adelbertc · 2018-08-01T03:17:23Z

@mccheah Done - unfortunately I seem to be having some issues running tests locally, I assume there is some CI checking this as well?

mccheah · 2018-08-01T16:28:14Z

ok to test

liyinan926 · 2018-08-01T16:32:15Z

...s/core/src/test/scala/org/apache/spark/deploy/k8s/features/BasicDriverFeatureStepSuite.scala

+
+  def containerPort(name: String, portNumber: Int): ContainerPort = {
+    val port = new ContainerPort()
+    port.setName(name)


Looks like the indention here is incorrect.

@liyinan926 Is it? It looks OK on my end I think unless I'm missing something

Sorry my bad, I misread it as something like:

return new ContainerPort() .setName(name) ...

SparkQA · 2018-08-01T16:43:03Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-make-spark-distribution-unified/1565/

SparkQA · 2018-08-01T16:54:13Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-make-spark-distribution-unified/1565/

mccheah · 2018-08-01T18:50:03Z

jenkins, test this please

mccheah · 2018-08-01T18:50:19Z

(Not sure why the unit test build didn't run just now, only the integration tests did)

SparkQA · 2018-08-01T19:02:24Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-make-spark-distribution-unified/1575/

SparkQA · 2018-08-01T19:10:33Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-make-spark-distribution-unified/1575/

mccheah · 2018-08-01T19:12:43Z

@shaneknapp the unit test build doesn't seem to be triggering on this PR, any idea as to what's going on?

mccheah · 2018-08-01T19:13:13Z

...s/core/src/test/scala/org/apache/spark/deploy/k8s/features/BasicDriverFeatureStepSuite.scala

@@ -203,4 +212,12 @@ class BasicDriverFeatureStepSuite extends SparkFunSuite {
      "spark.files" -> "https://localhost:9000/file1.txt,/opt/spark/file2.txt")
    assert(additionalProperties === expectedSparkConf)
  }
+
+  def containerPort(name: String, portNumber: Int): ContainerPort = {
+    val port = new ContainerPort()


Use ContainerPortBuilder

shaneknapp · 2018-08-01T20:26:05Z

jenkins test this please

shaneknapp · 2018-08-01T20:30:17Z

@mccheah the tests didn't run because you're not an admin for the regular (non-k8s) pull request builder.

i added you to that list and you'll be able to trigger tests now.

SparkQA · 2018-08-01T20:36:56Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-make-spark-distribution-unified/1581/

SparkQA · 2018-08-01T20:44:53Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-make-spark-distribution-unified/1581/

SparkQA · 2018-08-01T20:45:04Z

Test build #93901 has finished for PR 21884 at commit 4215fc2.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

mccheah · 2018-08-01T20:56:16Z

Thanks @shaneknapp, I'll merge this now.

mccheah · 2018-08-01T21:00:00Z

Thanks @adelbertc for the contribution!

SparkQA · 2018-08-01T21:03:39Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-make-spark-distribution-unified/1583/

SparkQA · 2018-08-01T21:12:13Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-make-spark-distribution-unified/1583/

adelbertc force-pushed the k8s-expose-driver-ports branch from 48d07c5 to c537779 Compare July 27, 2018 17:15

adelbertc changed the title ~~k8s: explicitly expose ports on driver container~~ [Scheduler][K8s]: explicitly expose ports on driver container Jul 27, 2018

adelbertc force-pushed the k8s-expose-driver-ports branch from c537779 to a2f2667 Compare July 28, 2018 19:33

adelbertc changed the title ~~[Scheduler][K8s]: explicitly expose ports on driver container~~ [SPARK-24960][Kubernetes] explicitly expose ports on driver container Jul 28, 2018

liyinan926 reviewed Jul 30, 2018

View reviewed changes

adelbertc force-pushed the k8s-expose-driver-ports branch from a2f2667 to 1fc6faa Compare July 31, 2018 00:07

liyinan926 approved these changes Jul 31, 2018

View reviewed changes

adelbertc force-pushed the k8s-expose-driver-ports branch from 1fc6faa to 9000a2f Compare July 31, 2018 21:58

adelbertc changed the title ~~[SPARK-24960][Kubernetes] explicitly expose ports on driver container~~ [SPARK-24960][K8S] explicitly expose ports on driver container Jul 31, 2018

mccheah reviewed Jul 31, 2018

View reviewed changes

adelbertc force-pushed the k8s-expose-driver-ports branch from 9000a2f to 4215fc2 Compare August 1, 2018 03:16

liyinan926 reviewed Aug 1, 2018

View reviewed changes

mccheah reviewed Aug 1, 2018

View reviewed changes

[SPARK-24960][K8S] explicitly expose ports on driver container

66d1e62

adelbertc force-pushed the k8s-expose-driver-ports branch from 4215fc2 to 66d1e62 Compare August 1, 2018 20:48

asfgit closed this in f5113ea Aug 1, 2018

adelbertc mentioned this pull request Jan 29, 2019

Explicitly expose ports on driver container, fixes #617 apache-spark-on-k8s/spark#618

Closed

[SPARK-24960][K8S] explicitly expose ports on driver container #21884

[SPARK-24960][K8S] explicitly expose ports on driver container #21884

Conversation

adelbertc commented Jul 26, 2018 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

felixcheung commented Jul 27, 2018

adelbertc commented Jul 27, 2018

mccheah commented Jul 27, 2018

adelbertc commented Jul 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mccheah commented Jul 30, 2018

adelbertc commented Jul 31, 2018

liyinan926 left a comment

Choose a reason for hiding this comment

mccheah commented Jul 31, 2018

mccheah commented Jul 31, 2018

adelbertc commented Jul 31, 2018

Choose a reason for hiding this comment

adelbertc commented Aug 1, 2018

mccheah commented Aug 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Aug 1, 2018

SparkQA commented Aug 1, 2018

mccheah commented Aug 1, 2018

mccheah commented Aug 1, 2018

SparkQA commented Aug 1, 2018

SparkQA commented Aug 1, 2018

mccheah commented Aug 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shaneknapp commented Aug 1, 2018

shaneknapp commented Aug 1, 2018

SparkQA commented Aug 1, 2018

SparkQA commented Aug 1, 2018

SparkQA commented Aug 1, 2018

mccheah commented Aug 1, 2018

mccheah commented Aug 1, 2018

SparkQA commented Aug 1, 2018

SparkQA commented Aug 1, 2018

adelbertc commented Jul 26, 2018 •

edited

Loading