[SPARK-25088][CORE][MESOS][DOCS] Update Rest Server docs & defaults.

[squito]

What changes were proposed in this pull request?

(a) disabled rest submission server by default in standalone mode
(b) fails the standalone master if rest server enabled & authentication secret set
(c) fails the mesos cluster dispatcher if authentication secret set
(d) doc updates
(e) when submitting a standalone app, only try the rest submission first if spark.master.rest.enabled=true

otherwise you'd see a 10 second pause like
18/08/09 08:13:22 INFO RestSubmissionClient: Submitting a request to launch an application in spark://...
18/08/09 08:13:33 WARN RestSubmissionClient: Unable to connect to server spark://...

I also made sure the mesos cluster dispatcher failed with the secret enabled, though I had to do that on slightly different code as I don't have mesos native libs around.

How was this patch tested?

I ran the tests in the mesos module & in core for org.apache.spark.deploy.*

I ran a test on a cluster with standalone master to make sure I could still start with the right configs, and would fail the right way too.

[SparkQA]

Test build #94572 has finished for PR 22071 at commit b4ca224.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #4241 has started for PR 22071 at commit b4ca224.

[felixcheung]

@tnachen

[SparkQA]

Test build #4244 has finished for PR 22071 at commit b4ca224.

• This patch fails Spark unit tests.
• This patch does not merge cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #4245 has finished for PR 22071 at commit b4ca224.

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

[tnachen]

I think it might be better to place this in the MesosRestServer code, since it's not really about the framework (MesosClusterDispatcher) but the RestServer receiving requests.

[squito]

I thought about this too, and originally wrote it that way ... but then I figured the way it works now, its really the same thing. If I put it in the MesosRestServer, it might seem like you could run the ClusterDispatcher without the RestServer somehow -- but maybe the exception itself is clear enough?

anyway, don't feel particularly strongly either way.

[tnachen]

Got it, my reasoning is that it could be harder for someone looking at the code to figure out why this is not allowed, since we don't really mention about the rest server which is really the one requiring security to be turned off. Another reason it will be beneficial to have the check in the MesosRestServer is that the MesosClusterDispatcher framework could technically be decoupled from the MesosRestServer and allow another way to receive requests, so to increase flexibility and avoid someone forgetting about why we put this here, my suggestion is to move the check closer to where it's being required will help maintain this a bit better.

[tgravescs]

Personally the way I read this, I agree with @squito and it should be here. From my understanding from reading the docs on running on mesos, the user doesn't start the rest server, they start the dispatcher. The dispatcher doesn't support any sort of authentication. The fact that it uses the rest server is an implementation detail the user doesn't necessarily care about. If you change the dispatcher to have another way then it should support authentication as well or have this same error. Someone adding another way other then rest server should be made aware of this, so it being here accomplishes that.

Perhaps just adding a comment in the code about the rest server would help to clarify to developers? Or please correct me if my understanding of running the dispatcher is wrong.

[tgravescs]

Note if someone add another way to that supports authentication we just move it at that point.

[squito]

MesosClusterDispatcher framework could technically be decoupled from the MesosRestServer

I actually think that is a reason to keep it here. If somebody adds another way, then this check is still in place for the new way -- unless they consciously think about making it work, and then they'd move the check to a more appropriate spot.

I can add a comment in the code: // This doesn't support authentication because the RestSubmissionServer doesn't support it.

[tgravescs]

lgtm. +1

[SparkQA]

Test build #94706 has finished for PR 22071 at commit 897b587.

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

[tnachen]

LGTM as well

[jerryshao]

LGTM.

[tgravescs]

+1

[srowen]

Merged to master

[squito]

any objections about putting this in prior branches as well?

[srowen]

Docs -- obviously OK to backport. I wasn't as sure about the behavior change. Normally we'd never do that for a maintenance branch if it can be helped. I could see an argument here, but did think we were just going to change the behavior for 2.4. 2.3.2 -- I could see.

[felixcheung]

in this case maybe ok. perhaps just rel note this iff there's another 2.2.x or 2.1.x releases?

[akirillov]

Hi @squito, if I may, I have a few comments to share regarding this change and probably you can help me to understand it in better details.

From Spark Security Doc page it looks like spark.master.rest.enabled property belongs to Spark Standalone Master only. IMO, It makes sense, because it's the server side who is responsible for enabling and disabling REST endpoint according to the source code.

Now, from the comment in the code of SparkSubmit.scala it looks like REST-based gateway introduced in Spark 1.3 is the default way of communication with Spark Standalone Master with a fallback to legacy RPC.

And here's the problem: now spark.master.rest.enabled became a client-side property which defaults to false for both Mesos and Spark Standalone and breaks all spark-submit calls to Mesos Dispatcher if this property is not provided explicitly. Now users have to provide it every time for Mesos cluster mode while REST is the only mode supported by the dispatcher. Looks like a breaking change.

Another note is regarding spark.authenticate.secret assertion in MesosClusterDispatcher. Dispatcher will be unable to start if spark.authenticate.secret is set. It looks like not so great behaviour if it doesn't allow to run a program at all if some flag is set. Which also looks like a breaking change.

Suggestions:

• it looks natural for spark.master.rest.enabled to be handled in SparkSubmit only when used with Spark Standalone Master. It can be completely ignored with Mesos Dispatcher because REST interface is the only endpoint it exposes
• defaulting Spark Standalone Master's spark.master.rest.enabled to false switches the communication to 'legacy' RPC protocol which is marked as a fallback-only since Spark 1.3. It's not clear how this protocol is going to be supported in the future. It looks safer to document how to disable REST endpoint for security purposes instead while preserving REST communications by default
• instead of throwing an assertion error - log a WARN message in Mesos Dispatcher if spark.authenticate.secret is provided in configuration

Please let me know what you think. Thanks.