[SPARK-19038][YARN] Avoid overwriting keytab configuration in yarn-client #16923

jerryshao · 2017-02-14T09:12:06Z

What changes were proposed in this pull request?

Because yarn#client will reset the spark.yarn.keytab configuration to point to the location in distributed file, so if user still uses the old SparkConf to create SparkSession with Hive enabled, it will read keytab from the path in distributed cached. This is OK for yarn cluster mode, but in yarn client mode where driver is running out of container, it will be failed to fetch the keytab.

So here we should avoid reseting this configuration in the yarn#client and only overwriting it for AM, so using spark.yarn.keytab could get correct keytab path no matter running in client (keytab in local fs) or cluster (keytab in distributed cache) mode.

How was this patch tested?

Verified in security cluster.

SparkQA · 2017-02-14T10:49:58Z

Test build #72865 has finished for PR 16923 at commit 7ed7c6c.

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

jerryshao · 2017-02-15T00:55:39Z

@tgravescs @mridulm would you please help to review, thanks a lot.

tgravescs

mostly looks good just a small request on the comment.

tgravescs · 2017-02-15T15:26:11Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/client/HiveClientImpl.scala

this seems odd, the comment seems to say that it was creating new instance of SparkConf but I don't see that in the code you removed so I guess this comment was just wrong...

It looks like this must have gotten broke when we went to spark 2.x, original version actually was creating the new SparkConf:
https://github.com/apache/spark/pull/9272/files

yes, looks like this is a regression in 2.x.

tgravescs · 2017-02-15T15:33:48Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/client/HiveClientImpl.scala

I'm not sure I understand the comment here. This doesn't have anything to do with the user creating an old SparkConf? Isn't this just in yarn client mode it has to use the original SparkConf where yarn.Client didn't update the location of the keytab file? If that is the case perhaps just update the comment to say something to that affect.

Yes, it only affects yarn client mode, in which we should get original key tab path for driver (not the one updated by yarn.Client).

SparkQA · 2017-02-16T04:13:20Z

Test build #72976 has finished for PR 16923 at commit b0990e3.

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

jerryshao · 2017-02-17T00:58:18Z

@vanzin , would you mind helping to review this PR, thanks a lot.

IIUC the issue was introduced in #11510 .

vanzin · 2017-02-17T03:18:59Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/client/HiveClientImpl.scala

I probably should look at the history of this file, but I'm a little puzzled about why is this login necessary.

SparkSubmit.scala already logs in the user with the provided keytab, before YARN's Client.scala has a chance to mess with it. So it seems to me like this code is redundant?

Not clearly sure why here login from keytab is required. Is it the behavior required by Hive?

Perhaps the reason is described in here.

That makes sense. Wish there was a different solution instead of logging in again, but let's leave that for a separate discussion...

Instead of this change, how about making Client.scala store the AM location for the keytab in a different key? As far as I can see AMCredentialRenewer is the only place where it's used. I think that would be a better change. The current change relies on spark.yarn.keytab being set as a system property so that new SparkConf() picks it up.

SparkQA · 2017-02-20T07:11:42Z

Test build #73146 has finished for PR 16923 at commit 57060e3.

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

vanzin · 2017-02-21T17:55:54Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/client/HiveClientImpl.scala

The idea of having the new conf was to avoid the changes in this file. Why are they still needed?

AFAIK, I think we still need to check different configurations for yarn client and cluster mode. In cluster mode, checking "spark.yarn.keytab" should be failed now, since it pointed to original file which doesn't exist in AM side.

I see. Man this is messy. Is there a better way to check where this code is running so that you don't have to to fs checks to see if the files exist? There's a very unlikely possibility that on the AM side "spark.yarn.keytab" might point at a real file that is not the desired keytab...

I also noticed that in the client case, it doesn't seem like delegation token renewal will work. I'm not sure how well that works in general in client mode anyway, but might be something worth tracking in a separate bug.

One sort of hacky way is to have ApplicationMaster.scala override the value of KEYTAB in the configuration with the value of AM_KEYTAB. Then this code just needs to look at "spark.yarn.keytab" to cover both cases.

So your suggestion is that we don't modify this HiveClientImpl code and all the changes just put into yarn module?

Yeah that would be my preference. Also my last suggestion would mean that the value of "spark.yarn.keytab" always points to the path of the keytab defined by the user, regardless of where the code is running.

Ok, I think I can see a cleaner solution that what I proposed. Sorry for flip-flopping on this.

Instead of overwriting the KEYTAB value in Client.scala, instead, how about:

keep the keytab name in an instance variable

don't update SparkConf, and use the instance variable in the distribute call (around L470)

when writing the AM conf, overwrite KEYTAB.key in the properties instance (around L708)

That avoids the second config, and keeps all the code to handle the different locations in one place (Client.scala), without having to change anywhere else.

OK, thanks for suggestion, let me update the code.

Change-Id: Ic0dd74361171de05dd8369919762455895e672c1

SparkQA · 2017-02-23T04:01:57Z

Test build #73318 has finished for PR 16923 at commit 11bfb4f.

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

Change-Id: Ia77a7a6a694d01afea9a867ed11fb5d3326455f4

SparkQA · 2017-02-23T06:56:49Z

Test build #73326 has finished for PR 16923 at commit 08d53d2.

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

vanzin · 2017-02-23T17:44:09Z

resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala

      sparkConf.getAll.foreach { case (k, v) => props.setProperty(k, v) }
+      // Override spark.yarn.key to point to the location in distributed cache which will be used
+      // by AM.
+      Option(amKeytabFileName).foreach(k => props.setProperty(KEYTAB.key, k))


nit: either just a good old if, or .foreach { k => ... }

vanzin · 2017-02-23T17:45:00Z

Can you update the title and summary to match what the change is doing?

Change-Id: I57452186389651c347d6560be94c68f92249e38c

SparkQA · 2017-02-24T03:38:53Z

Test build #73386 has finished for PR 16923 at commit c8390e6.

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

vanzin · 2017-02-24T17:31:26Z

Merging to master (2.1 and 2.0 if no conflicts).

…ient ## What changes were proposed in this pull request? Because yarn#client will reset the `spark.yarn.keytab` configuration to point to the location in distributed file, so if user still uses the old `SparkConf` to create `SparkSession` with Hive enabled, it will read keytab from the path in distributed cached. This is OK for yarn cluster mode, but in yarn client mode where driver is running out of container, it will be failed to fetch the keytab. So here we should avoid reseting this configuration in the `yarn#client` and only overwriting it for AM, so using `spark.yarn.keytab` could get correct keytab path no matter running in client (keytab in local fs) or cluster (keytab in distributed cache) mode. ## How was this patch tested? Verified in security cluster. Author: jerryshao <[email protected]> Closes #16923 from jerryshao/SPARK-19038. (cherry picked from commit a920a43) Signed-off-by: Marcelo Vanzin <[email protected]>

…ient ## What changes were proposed in this pull request? Because yarn#client will reset the `spark.yarn.keytab` configuration to point to the location in distributed file, so if user still uses the old `SparkConf` to create `SparkSession` with Hive enabled, it will read keytab from the path in distributed cached. This is OK for yarn cluster mode, but in yarn client mode where driver is running out of container, it will be failed to fetch the keytab. So here we should avoid reseting this configuration in the `yarn#client` and only overwriting it for AM, so using `spark.yarn.keytab` could get correct keytab path no matter running in client (keytab in local fs) or cluster (keytab in distributed cache) mode. ## How was this patch tested? Verified in security cluster. Author: jerryshao <[email protected]> Closes apache#16923 from jerryshao/SPARK-19038.

tgravescs requested changes Feb 15, 2017

View reviewed changes

vanzin reviewed Feb 17, 2017

View reviewed changes

vanzin reviewed Feb 21, 2017

View reviewed changes

Change to not override spark.yarn.keytab in local

11bfb4f

Change-Id: Ic0dd74361171de05dd8369919762455895e672c1

jerryshao force-pushed the SPARK-19038 branch from 57060e3 to 11bfb4f Compare February 23, 2017 03:41

Fix NPE issue

08d53d2

Change-Id: Ia77a7a6a694d01afea9a867ed11fb5d3326455f4

vanzin approved these changes Feb 23, 2017

View reviewed changes

jerryshao changed the title ~~[SPARK-19038][Hive][YARN] Correctly figure out keytab file name in yarn client mode~~ [SPARK-19038][YARN] Avoid overwriting keytab configuration in the driver side Feb 24, 2017

jerryshao changed the title ~~[SPARK-19038][YARN] Avoid overwriting keytab configuration in the driver side~~ [SPARK-19038][YARN] Avoid overwriting keytab configuration in yarn-client Feb 24, 2017

Style change

c8390e6

Change-Id: I57452186389651c347d6560be94c68f92249e38c

asfgit closed this in a920a43 Feb 24, 2017

[SPARK-19038][YARN] Avoid overwriting keytab configuration in yarn-client #16923

[SPARK-19038][YARN] Avoid overwriting keytab configuration in yarn-client #16923

Uh oh!

Conversation

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

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Feb 14, 2017

Uh oh!

jerryshao commented Feb 15, 2017

Uh oh!

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

SparkQA commented Feb 16, 2017

Uh oh!

jerryshao commented Feb 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jerryshao Feb 17, 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!

vanzin Feb 17, 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!

SparkQA commented Feb 20, 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!

SparkQA commented Feb 23, 2017

Uh oh!

SparkQA commented Feb 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vanzin commented Feb 23, 2017

Uh oh!

SparkQA commented Feb 24, 2017

Uh oh!

vanzin commented Feb 24, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jerryshao commented Feb 14, 2017 •

edited

Loading

jerryshao Feb 17, 2017 •

edited

Loading

vanzin Feb 17, 2017 •

edited

Loading