Allow customizing external URI provision + External URI can be set via annotations #147

mccheah · 2017-02-24T02:23:32Z

Closes #140.

kimoonkim · 2017-02-24T23:25:40Z

@mccheah FYI, the Travis build failed not because of unit tests but because of style issues:

error file=/home/travis/build/apache-spark-on-k8s/spark/resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/kubernetes/constants.scala message=File line length exceeds 100 characters line=68
Saving to outputFile=/home/travis/build/apache-spark-on-k8s/spark/resource-managers/kubernetes/core/target/scalastyle-output.xml
Processed 19 file(s)
Found 1 errors
Found 0 warnings
Found 0 infos
Finished in 1617 ms
[ERROR] Failed to execute goal org.scalastyle:scalastyle-maven-plugin:0.8.0:check (default) on project spark-kubernetes_2.11: Failed during scalastyle execution: You have 1 Scalastyle violation(s). -> [Help 1]

mccheah · 2017-02-24T23:59:30Z

docs/running-on-kubernetes.md

This could perhaps be more concise or clear. Would appreciate anyone's thoughts on how this could be better explained.

mccheah · 2017-02-25T00:00:52Z

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

@ash211 I swapped the order here of the tryWithResource and try-finally blocks mainly because we want to clean up the secrets and the shutdown hooks after the pod has been launched but also before the application itself completes.

why do we need to do switch to shutdown hooks? those accumulate this global state and you start ending up having ordering issues (like you had with priorities) that I think is best avoided. Can we keep as try-finally blocks instead?

try-finally doesn't catch the case when the JVM is stopped via a kill signal. I suppose the big question mark is if we care about such scenarios. The rest of the Spark ecosystem relies on shutdown hooks greatly to clear up all kinds of temporary state.

mccheah · 2017-02-25T00:02:36Z

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

It may be the case that the external URI providers take a little longer to be ready. In testing #70 I noticed that the Ingress can take awhile to set up in the background.

mccheah · 2017-02-25T00:06:20Z

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

Although I think this highlights that we have an API discrepancy here. We document that the driverServiceManager will build the service that will end up being used, but the code still manipulates the created service after the fact to avoid exposing the NodePort. There's also the extra configuration parameter spark.kubernetes.driver.service.exposeUiPort. There should be a way to reconcile all of these configurations.

ash211 · 2017-02-25T00:43:25Z

docs/running-on-kubernetes.md

To use a manually-specified external IP instead of a Kubelet's IP, ...

ash211 · 2017-02-25T00:44:37Z

docs/running-on-kubernetes.md

This will create a service with the annotation XYZ that routes to the driver pod

ash211 · 2017-02-25T00:45:24Z

docs/running-on-kubernetes.md

provide an example URI, e.g. https://example.com:8080/my-job

ash211 · 2017-02-25T00:47:21Z

docs/running-on-kubernetes.md

the service-loaded class should be the value of that setting? e.g. spark.kubernetes.driver.serviceManagerType =com.mycompany.spark.MyCustomDriverServiceManager ?

It is not - the value here matches what is returned by DriverServiceManager#getType.

The advantage of this is that the configuration value becomes easier to set for the things we bundle into it - NodePort vs. ExternalAnnotation and eventually ApiServerProxy is more elegant to remember than full class names.

ash211 · 2017-02-25T00:48:58Z

docs/running-on-kubernetes.md

we should also provide an implementation of this that uploads through the apiserver proxy

Think this can be left for future work.

ash211 · 2017-02-25T01:07:42Z

...rc/main/scala/org/apache/spark/deploy/rest/kubernetes/NodePortUrisDriverServiceManager.scala

put this method at the end so the flow is top to bottom. getServiceManagerType is called first, then customizeDriverService, then getDriverServiceSubmissionServerUris

ash211 · 2017-02-25T01:12:56Z

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

I think this service cleanup after the job is submitted should be part of the plugin point, rather than the core always opinionatedly deleting the service after the job has submitted.

Alternatively the contract could be that any created k8s resources need to have owner references to the service so that when Spark deletes the service the dependent resources are cleaned up too

It's different here because the service isn't deleted, but rather it is manipulated such that its type becomes ClusterIP and it no longer exposes the submission server over the Node Port. Regardless there is still the question of whether or not the plugin should be the component that makes this decision or if it's universal.

ash211 · 2017-02-25T01:15:01Z

docs/running-on-kubernetes.md

provide a list of bundled implementations in a bullet list here -- right now this is just NodePort and ExternalAnnotation but I also think we should do a ServerApi also

Didn't want to try to embed a bullet-list in the table - but added the valid values in prose form.

ash211 · 2017-02-25T01:18:20Z

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

why do we need to do switch to shutdown hooks? those accumulate this global state and you start ending up having ordering issues (like you had with priorities) that I think is best avoided. Can we keep as try-finally blocks instead?

ash211 · 2017-02-25T01:19:30Z

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

these comments got disconnected from the LoggingPodStatusWatcher code

foxish · 2017-02-25T01:26:18Z

Haven't done a full review yet of the code.

On the docs front, one broad comment:
The kubelet is the process that runs on each node. The kubelet has to do with bookkeeping and master communication. That name is not used to refer to the node. The node is referred to as "Kubernetes node", or simply "node". For example, NodePort opens a port on the node IP address and listens to it.

foxish · 2017-02-25T01:27:05Z

Also, this PR should be targeted towards k8s-support-alternate-incremental? It is pointing at the ssl branch as of now.

mccheah · 2017-02-25T02:35:26Z

@ash211 @foxish thanks for reviewing. I changed the base branch to k8s-support-alternate-incremental and also addressed all the comments so far.

mccheah · 2017-02-28T20:53:32Z

@foxish any more thoughts on this PR?

ash211

Some minor docs suggestions

ash211 · 2017-02-28T22:53:18Z

docs/running-on-kubernetes.md

+
+There may be cases where the nodes cannot be reached by the submission client. For example, the cluster may
+only be reachable through an external load balancer. The user may provide their own external IP for Spark driver
+services. To use a your own external IP instead of a node's IP, first set


external IP -> external URI

ash211 · 2017-02-28T22:55:05Z

docs/running-on-kubernetes.md

+services. To use a your own external IP instead of a node's IP, first set
+`spark.kubernetes.driver.serviceManagerType` to `ExternalAnnotation`. A service will be created with the annotation
+`spark-job.alpha.apache.org/provideExternalUri`, and this service routes to the driver pod. You will need to run a
+process that watches the API server for services that are created with this annotation in the application's namespace


separate process

ash211 · 2017-02-28T22:56:00Z

docs/running-on-kubernetes.md

+`spark.kubernetes.driver.serviceManagerType` to `ExternalAnnotation`. A service will be created with the annotation
+`spark-job.alpha.apache.org/provideExternalUri`, and this service routes to the driver pod. You will need to run a
+process that watches the API server for services that are created with this annotation in the application's namespace
+(set by `spark.kubernetes.namespace`). The process should determine a URI that routes to this service, and patch the


... that routes to this service (potentially configuring infrastructure to handle the URI behind the scenes), and patch ...

ash211 · 2017-02-28T22:58:36Z

docs/running-on-kubernetes.md

+URI that your process has provided (e.g. `https://example.com:8080/my-job`).
+
+Note that if the URI provided by the annotation also provides a base path, the base path should be removed when the
+request is forwarded to the back end pod.


I'm not following this sentence. If the Spark code ignores the base path, then let's throw when the external URI provider gives a URI with a base path? Alternatively probably we should support submitting through a base path since some external URI providers may route based on base-path and require the base path for traffic to reach the underlying spark submit service

The external URI can include a base path, just that when the request reaches the back end pod, the back end pod should be contacted at an empty base path. In the case that you describe where the external URI provider requires a base path, the URI provider should also rewrite the URI when forwarding to the back end to remove the base path. I'm not sure of the best way to phrase this.

Ah, maybe something like:

Note that the URI provided in the annotation needs to route traffic to the appropriate destination on the pod, which has a fixed path portion of the URI. This means the external URI provider will likely need to rewrite the path from the external URI to the destination on the pod, e.g. https://example.com:8080/spark-app-1/submit will need to route traffic to https://<pod_ip>:<service_port>/path/to/submit. Note that the paths of these two URLs are different.

I'm not sure what the destination /path/to/submit actually is in Spark -- could use some help there.

Done - the path is empty on the back end.

ash211 · 2017-02-28T22:59:24Z

docs/running-on-kubernetes.md

+
+If the above is confusing, keep in mind that this functionality is only necessary if the submitter cannot reach any of
+the nodes at the driver's node port. It is recommended to use the default configuration with the node port service
+whenever possible.


mention that there's a way to serviceload implementations of this here

I don't think we should include the service loading note here - it's a pretty advanced use case and is already covered in the configuration table below.

Mostly because this section is confusing as it is - wouldn't want to add to the confusion here.

Ah right, missed that it's mentioned in the config table.

ash211 · 2017-02-28T23:00:18Z

docs/running-on-kubernetes.md

+  <td>
+    A tag indicating which class to use for creating the Kubernetes service and determining its URI for the submission
+    client. Valid values are currently <code>NodePort</code> and <code>ExternalAnnotation</code>. By default, a service
+    is created with the NodePort type, and the driver will be contacted at one of the kubelet nodes at the port that the


since NodePort is a config value in this paragraph, make sure it's always wrapped in code tags

It's not exactly a config value when stating "a service is created with the NodePort type" - NodePort is the type of the service in this case. Granted, NodePort is a term that is overloaded in this case. Marked it with code tags anyways.

foxish · 2017-03-01T17:22:50Z

LGTM

ash211

Looks good to me, though there's a merge conflict now

…te-incremental' into external-uri-provider

ash211 · 2017-03-02T22:31:56Z

One of the travis checks failed and it looked like a flake (couldn't find mvn?) so I retriggered it.

ash211 · 2017-03-03T00:22:48Z

Passed this time -- merging!

…a annotations (#147) * Listen for annotations that provide external URIs. * FIx scalstyle * Address comments * Fix doc style * Docs updates * Clearly explain path rewrites

…a annotations (apache-spark-on-k8s#147) * Listen for annotations that provide external URIs. * FIx scalstyle * Address comments * Fix doc style * Docs updates * Clearly explain path rewrites (cherry picked from commit f0a40b8)

…a annotations (apache-spark-on-k8s#147) * Listen for annotations that provide external URIs. * FIx scalstyle * Address comments * Fix doc style * Docs updates * Clearly explain path rewrites

ash211 mentioned this pull request Feb 24, 2017

Support providing the driver URI from an external controller #140

Closed

mccheah commented Feb 24, 2017

View reviewed changes

mccheah commented Feb 25, 2017

View reviewed changes

mccheah mentioned this pull request Feb 25, 2017

Pull component readiness logic to another class #124

Closed

ash211 suggested changes Feb 25, 2017

View reviewed changes

mccheah added 2 commits February 24, 2017 18:02

Listen for annotations that provide external URIs.

5955607

FIx scalstyle

ba50538

mccheah force-pushed the external-uri-provider branch from 3e34edb to ba50538 Compare February 25, 2017 02:04

mccheah changed the base branch from refactor-ssl to k8s-support-alternate-incremental February 25, 2017 02:04

Address comments

b848ee1

Fix doc style

95c54a9

mccheah changed the title ~~Listen for annotations that provide external URIs.~~ Allow customizing external URI provision + External URI can be set via annotations Feb 28, 2017

ash211 reviewed Feb 28, 2017

View reviewed changes

mccheah added 2 commits February 28, 2017 15:28

Docs updates

664d82f

Clearly explain path rewrites

343ab54

foxish approved these changes Mar 1, 2017

View reviewed changes

ash211 approved these changes Mar 2, 2017

View reviewed changes

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

0705d3e

…te-incremental' into external-uri-provider

ash211 merged commit f0a40b8 into k8s-support-alternate-incremental Mar 3, 2017

ash211 deleted the external-uri-provider branch March 3, 2017 00:22

Allow customizing external URI provision + External URI can be set via annotations #147

Allow customizing external URI provision + External URI can be set via annotations #147

Uh oh!

Conversation

mccheah commented Feb 24, 2017

Uh oh!

kimoonkim commented Feb 24, 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!

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!

foxish commented Feb 25, 2017

Uh oh!

foxish commented Feb 25, 2017

Uh oh!

mccheah commented Feb 25, 2017

Uh oh!

mccheah commented Feb 28, 2017

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