Allow submitting to the driver through an Ingress instead of NodePort #116

mccheah · 2017-02-15T00:58:24Z

Closes #70.

…te-incremental' into ingress-support

mccheah

@foxish The SparkUI changes are not intuitive. There might be a better way to go about fixing the UI.

Normally, when using any other deploy mode, the Spark UI is hosted on the driver with an empty base path (that is, /). However when using the Ingress, the full path that is sent to the proxy may also be what is requested of the driver service. This means that the driver service will always be contacted at a base path of http://<service-ip>:<service-port>/<spark-kubernetes-id>/<spark-ui>/. Unfortunately, the prior implementation of the Spark UI expects to only be contacted at http://<service-ip>:<service-port>.

There's generally some questions about how to handle all the various ways the Ingress controller may be set up in production. Think SSL, etc.

mccheah · 2017-02-15T01:01:12Z

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

        (null, SSLContext.getDefault)
      }
+    val maxRetriesPerServer = if (useIngress) {
+      SUBMISSION_CLIENT_RETRIES_INGRESS


@foxish is there perhaps a better way to do this? It turns out that the Ingress resource takes longer to become ready than the NodePort endpoints. Furthermore, the Watch on the Ingress doesn't necessarily catch whether or not the Ingress has added the endpoint.

mccheah · 2017-02-15T01:07:55Z

@ash211

mccheah · 2017-02-15T01:11:33Z

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

+    val defaultErrorDecoder = new ErrorDecoder.Default
+    val alwaysRetryErrorDecoder = new ErrorDecoder {
+      override def decode(methodKey: String, response: Response): Exception = {
+        defaultErrorDecoder.decode(methodKey, response) match {


The Ingress code path may return errors that are 404s when the load balancer (Nginx in the case of Minikube) has not set up the proxy yet. These kinds of errors aren't considered retryable by default.

foxish · 2017-02-15T01:44:24Z

docs/running-on-kubernetes.md

+  <td><code>spark.kubernetes.driver.exposeIngress</code></td>
+  <td>false</td>
+  <td>
+    When initially contacting the driver, use an Ingress when the submitting client passes application dependencies to


typo 'UI.me'

foxish · 2017-02-16T17:40:00Z

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

      "changeit")
    keyStoreFile = keyStore
    trustStoreFile = trustStore
+    minikubeKubernetesClient.services().inNamespace("kube-system").createOrReplaceWithNew()


We don't generally create non-system components in the kube-system namespace but I guess it is okay in the test.

Hm, is there a way for a service to proxy to pods that are not in its namespace?

foxish · 2017-02-16T20:08:17Z

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

        .endMetadata()
      .withNewSpec()
-        .withType("NodePort")
+        .withType(if (useIngress) "ClusterIP" else "NodePort")


This works with some but not all ingress. See https://github.com/kubernetes/ingress/blob/master/docs/faq/gce.md#i-created-an-ingress-and-nothing-happens-now-what, kubernetes/kubernetes#26508

I think we'd want the user to get a cluster-ip type here, with the default still being NodePort.

So to clarify, we should use NodePort to catch all of the use cases?

Yes, nodeport would definitely capture some use cases. But folks may have on-prem ingress controllers which use ClusterIP. I'm thinking we should support both of these.

Huh... how would we support both of these? Would we need to make this setting configurable?

Making the service type configurable on its own seems like a good idea, considering #129 also. That way, we could isolate the concerns of having/not having an ingress, and the service type being used. It would be up to the user to set a service type appropriately (nodeport/clusterIP) if they are using an ingress. Do you have ideas of how we can manage the complexity from the proliferation of commandline options configuring services? I'm not so sure that there is one setting that fits a majority of the use cases.

foxish · 2017-02-16T20:15:45Z

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

+      .booleanConf
+      .createWithDefault(false)
+
+  private[spark] val INGRESS_BASE_PATH =


Is this supposed to be the external hostname/IP address?

I believe so. I was wondering if we could rely on the LoadBalancerStatus we get back from querying API server for the Ingress status.

I think it should be possible to do that. I don't know how much we can count on different ingress controllers behaving the same way. I think it is very useful to have ingress support though. I'm swamped today, I'll get back to this item tomorrow.

Let's leave the base path as hardcoded and configurable then, at least for now?

…te-incremental' into ingress-support

lins05 · 2017-02-17T16:16:52Z

docs/running-on-kubernetes.md

+<tr>
+  <td><code>spark.kubernetes.driver.ingressBasePath</code></td>
+  <td>(none)</td>
+  <td>


Shall we give an example here? Otherwise the name ingressBasePath would make people tend to think it's only a path and not something like [host]:[port]

lins05 · 2017-02-17T16:32:26Z

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

    sparkConf.setIfMissing("spark.driver.port", DEFAULT_DRIVER_PORT.toString)
    sparkConf.setIfMissing("spark.blockmanager.port",
      DEFAULT_BLOCKMANAGER_PORT.toString)
+    sparkConf.set("spark.ui.basePath", s"/$kubernetesAppId/$UI_PATH_COMPONENT")


I guess this should only be set when exposeIngress is true, otherwise it would affect the current non-ingress deploy.

I just ran the integration tests and it failed at the first case which don't have exposeIngress set, and the test log shows the spark api returns 404.

I'm surprised the test failed - I changed KubernetesSuite#getSparkMetricsService in anticipation of the change in the base path. The tests pass locally for me.

The more I think about it the more I think the base path should be consistent between Ingress mode and non-Ingress mode. It would be nice to avoid a base path entirely, but unfortunately the Ingress will take the path "as-is" and forward that exact path to the backend by default. Some ingress controllers support path rewriting, but not all Ingress controllers would indeed support that so we have to go with the lowest common denominator, unfortunately.

I'm surprised the test failed - I changed KubernetesSuite#getSparkMetricsService in anticipation of the change in the base path. The tests pass locally for me.

OK, I'll take another test run later.

lins05 · 2017-02-17T16:56:06Z

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

  def validate(): KubernetesSparkRestServerArguments = {
    require(host.isDefined, "Hostname not set via --hostname.")
    require(port.isDefined, "Port not set via --port")
+    require(basePath.isDefined, "Base path not defined via --base-path")


--base-path should also be optional based on if ingress is used.

I think we want the base path to be consistent for both this and the Spark UI regardless if the Ingress is used or not. That reduces the number of end results that would come up depending on the configuration. For example someone may migrate their application to submit through Ingress instead of previously submitting through NodePort and they shouldn't have to rewrite the base path for their Spark UI.

mccheah · 2017-02-21T23:14:57Z

@foxish thoughts on the change overall, and some of my follow-up remarks?

foxish · 2017-02-21T23:39:05Z

I can't get this to work on GKE or GCE yet, because of reasons I'm still investigating. I think the expectation that spark-on-k8s will seamlessly work with ingress may lead to a lot of inconsistent experiences, mainly because of how ingress is handled a little differently in different clusters. I do not think the user should be providing the "base path" and we should derive it from the the resource.

A different issue: Setting the owner references also seems to not be happening, although the code does attempt it.

foxish · 2017-02-22T16:29:38Z

Another thing to note: The kubernetes.io/ingress.class needs to be set to target a particular ingress controller when there are multiple ingress controllers running.

mccheah · 2017-02-23T18:58:51Z

I'm closing this based on discussion of an alternate approach from #70.

…tream

mccheah added 2 commits February 14, 2017 16:40

Allow submitting with an Ingress

5971552

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

7b2d8e2

…te-incremental' into ingress-support

mccheah commented Feb 15, 2017

View reviewed changes

foxish mentioned this pull request Feb 15, 2017

Upgrade minikube #98

Closed

This was referenced Feb 16, 2017

Use a separate class to track components that need to be cleaned up #122

Merged

Run unit tests as part of CI builds #110

Closed

foxish suggested changes Feb 16, 2017

View reviewed changes

foxish reviewed Feb 16, 2017

View reviewed changes

mccheah added 6 commits February 16, 2017 14:38

Fix docs

28f5ec7

Merge branch 'k8s-support-alternate-incremental' into ingress-support

4fec8ac

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

a5100e7

…te-incremental' into ingress-support

Fix compiler error

bebeee9

Fix test compiler error

4332fa2

Fix test

c2251ee

lins05 reviewed Feb 17, 2017

View reviewed changes

mccheah mentioned this pull request Feb 21, 2017

Expose driver Web UI port as NodePort #129

Closed

foxish mentioned this pull request Feb 21, 2017

Optionally expose the driver UI port as NodePort #131

Merged

mccheah mentioned this pull request Feb 22, 2017

Support creating Ingress services for driver #70

Closed

mccheah closed this Feb 23, 2017

ash211 deleted the ingress-support branch February 23, 2017 21:20

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

Merge pull request apache-spark-on-k8s#116 from palantir/rk/merge-ups…

704fc2c

…tream

Allow submitting to the driver through an Ingress instead of NodePort #116

Allow submitting to the driver through an Ingress instead of NodePort #116

Uh oh!

Conversation

mccheah commented Feb 15, 2017

Uh oh!

mccheah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mccheah commented Feb 15, 2017

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!

foxish Feb 21, 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!

foxish 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!

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 21, 2017

Uh oh!

foxish commented Feb 21, 2017

Uh oh!

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

Uh oh!

Uh oh!

mccheah commented Feb 23, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

foxish Feb 21, 2017 •

edited

Loading

foxish Feb 16, 2017 •

edited

Loading

foxish commented Feb 22, 2017 •

edited

Loading