[SPARK-11265] [YARN] YarnClient can't get tokens to talk to Hive 1.2.1 in a secure cluster #9232

steveloughran · 2015-10-22T21:18:31Z

This is a fix for SPARK-11265; the introspection code to get Hive delegation tokens failing on Spark 1.5.1+, due to changes in the Hive codebase

…luster. This is just the first "it compiles" stage of the patch: no tests or attempts to run things yet

… set doesn't follow the current rules precisely

dougb · 2015-10-22T21:34:01Z

yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala

@@ -1337,55 +1337,8 @@ object Client extends Logging {
      conf: Configuration,
      credentials: Credentials) {
    if (shouldGetTokens(sparkConf, "hive") && UserGroupInformation.isSecurityEnabled) {


Does it make sense to add a check for isClusterMode too ?
I believe we don't need a delegation token if we are running in client mode.
The driver will have access to the submitting user's kerberos tokens.

yeah, probably does. In fact, getting a delegation token there is potentially worse than useless, as that DT will eventually expire. Using the keytab direct will, provided the user keeps logging on, stay valid.

As @tgravescs points out, the tokens are needed throughout the cluster, and yes, must be obtained irrespective of deployment.

Actually I don't really think the delegation tokens are needed in client mode (for Hive only). Only the driver talks to the metastore.

SparkQA · 2015-10-22T21:49:05Z

Test build #44176 has finished for PR 9232 at commit 1fcb42e.

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

chesterxgchen · 2015-10-22T21:50:45Z

yarn/src/main/scala/org/apache/spark/deploy/yarn/YarnSparkHadoopUtil.scala

+      username: String): Option[Token[DelegationTokenIdentifier]] = {
+    val mirror = universe.runtimeMirror(getClass.getClassLoader)
+
+    // the hive configuration class is a subclass of Hadoop Configuration


the hive metadata uri should also in Hadoop Configuration, does it make sense directly check the metastore URI from conf instead trying to get it from HiveConf ? which could potentially get exception

…metastore, but goes all the way to trying set up a thrift RPC call, then handles the exception raised and verifies that handling is as expected. Guarantees that the reflection code does (and will contine to) work.

steveloughran · 2015-10-23T13:57:18Z

-just pushed up a version with @dougb's suggestion; the check for cluster mode is made before trying to obtain either hive or hbase tokens.

Note that the HBase token acquisition is very similar to the hive one, and could also be factored out, including sharing the same exception-handling function as the hive one. And again, testable, though that would need Hbase on the test classpath.

SparkQA · 2015-10-23T14:15:27Z

Test build #44222 has finished for PR 9232 at commit 1248656.

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

SparkQA · 2015-10-23T14:26:46Z

Test build #44223 has finished for PR 9232 at commit 9630a9d.

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

tgravescs · 2015-10-23T15:52:51Z

yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala

@@ -322,8 +322,10 @@ private[spark] class Client(
    // multiple times, YARN will fail to launch containers for the app with an internal
    // error.
    val distributedUris = new HashSet[String]
-    obtainTokenForHiveMetastore(sparkConf, hadoopConf, credentials)
-    obtainTokenForHBase(sparkConf, hadoopConf, credentials)
+    if (isClusterMode) {


why is this cluster mode only? I can run spark shell to access hive or hbase and this won't get tokens for those to ship to executors?

suggestion from @dougb , but yes, I didn't think that through. It's not for the driver, its for the nodes. Will fix

…luster mode" This reverts commit 9630a9d.

steveloughran · 2015-10-23T18:35:31Z

Patch with revert posted

For reviewers, here some things to consider

Which exceptions should trigger log & skip for token, vs which are critical enough to fail the process. The patch ignores no classpath found (NoClassDefFoundError and ClassNotFoundException) (i.e. hive not on the CP), but also NoSuchMethodException. Which means that even if the security settings require a token, if there's some method incompatibility, it will be ignored.
the Hadoop config now passed down is used to help define the hive configuration used in the binding. This is needed for testing. Previously, an empty HiveConf instance was created: enough to force in the hive-default.xml and hive-site.xml, but not able to pick on any custom options. This is a change: does it constitute a regression, or a feature?

Also, I've just realised the finalcloseCurrent() call must be in a finally(); will add

SparkQA · 2015-10-23T18:53:40Z

Test build #44244 has finished for PR 9232 at commit 5158afe.

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

…ny; always call closeCurrent() but only log that call's exceptions, rather than rethrow,

steveloughran · 2015-10-23T20:50:02Z

Updated patch

Moves all arg validation up before any hive class reflection
gets all hive class methods before attempting to call any, so fails fast if any are missing
calls Hive.closeCurrent() in a finally block (if it could be loaded); catches and logs exceptions @ warn

With this load-methods-first strategy, the closeCurrent variable will always contain a value prior to Hive.get() being invoked. As a result, Hive.closeCurrent() will always be invoked after Hive.get() method has been invoked (irrespective of outcome), so is guaranteed to trigger a cleanup once a Hive instance was created. Previously, any failure of Hive.getDelegationToken() would skip the cleanup.

SparkQA · 2015-10-23T21:10:51Z

Test build #44257 has finished for PR 9232 at commit 9713cf5.

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

…put in a val, and, as we know that it is valid from then on, called directly in the following try/finally block

…mports

steveloughran · 2015-10-23T21:46:25Z

I've tightened this up and am happy with the code now, except I'm not sure that NoSuchMethodExceptions should be downgraded. It will hide the situation of incompatible Hive version on the CP yet principal required to talk to the hive metastore. Provided the spark app doesn't want to talk to hive, it's not going to matter. But if the app does want't to talk to hive, there'll be no cue except for a log message at launch time, and failures in executors whenever they try to set up an RPC connection. Leaving it as is retains backwards compatibility though.

SparkQA · 2015-10-23T21:48:06Z

Test build #44260 has finished for PR 9232 at commit bc6ea26.

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

SparkQA · 2015-10-23T22:07:42Z

Test build #44264 has finished for PR 9232 at commit b417ca3.

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

vanzin · 2015-10-24T00:21:40Z

@steveloughran could you use PR titles following the convention described in the following document?
https://cwiki.apache.org/confluence/display/SPARK/Contributing+to+Spark#ContributingtoSpark-PullRequest

SparkQA · 2015-10-24T13:50:23Z

Test build #44296 has finished for PR 9232 at commit 89edfa6.

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

vanzin · 2015-10-24T23:20:04Z

yarn/src/main/scala/org/apache/spark/deploy/yarn/YarnSparkHadoopUtil.scala

+      case e: NoSuchMethodException =>
+        logInfo("Hive Method not found", e)
+        None
+      case e: ClassNotFoundException =>


I kinda feel that everything but this exception should be propagated. That's really the only valid error you might expect; someone running Spark without Hive classes, and even that might change (I think there's a bug tracking removal of the hive profile from the maven build).

Every other error means that either you have the wrong version of Hive in the classpath (user error) or that your configuration says you need delegation tokens, but you can't get them for some reason.

+1. I'd left it in there as it may have had a valid reason for being there, but i do things it's correct. Detecting config problems, that is something to throw up.

Note that Client.obtainTokenForHBase() has similar behaviour; this patch doesn't address it. When someone sits down to do it, the policy about how to react to failures could be converted into a wrapper around a closure which executes the token retrieval (here obtainTokenForHiveMetastoreInner), so there'd be no divergence.

vanzin · 2015-10-27T21:56:23Z

yarn/src/main/scala/org/apache/spark/deploy/yarn/YarnSparkHadoopUtil.scala

+      case e: ClassNotFoundException =>
+        logInfo(s"$service class not found $e")
+        logDebug("Hive Class not found", e)
+      case t: Throwable => {


really, you don't need this.

Why would you? All you're doing here is printing the stack trace to stderr, which will happen again when you re-throw the exception.

those exceptions aren't being rethrown though, are they? So its logging the full stack @ debug, and a one-liner for most.

I don't follow. You're catching an exception, logging it, and re-throwing it, which causes the exception to show up twice in the process output.

Instead, you can just delete your code, and let the exception propagate naturally. It will show up in the output the same way.

oh, we're at cross purposes. I was looking @ line 128, above. you were at 180. That's why I Was confused. Yes, I'll cut the lower

Hi @steveloughran ,

I think you're still not really getting what I'm saying. You can just delete this whole case. The exception will just propagate up the call stack.

(edit: actually I see why you need to re-throw. I'd just move the exception handling to the calling method. It's just simpler that way, you're not gaining anything from having this extra method.)

the reason I'd pulled it out was to have an isolated policy which could be both tested without mocking failures, and be re-used in the HBase token retrieval, which needs an identical set of clauses.

Given the policy has now been simplified so much, the method is now pretty much unused; I can pull it. But still the HBase token logic will need to be 100% in sync.

SparkQA · 2015-10-27T22:16:25Z

Test build #44469 has finished for PR 9232 at commit fbf0ecb.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):\n * logInfo(s\"$service class not found $e\")\n

SparkQA · 2015-10-27T23:49:22Z

Test build #44475 has finished for PR 9232 at commit 00cb5a7.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):\n * logInfo(s\"$service class not found $e\")\n

zhzhan · 2015-10-28T00:00:26Z

yarn/src/main/scala/org/apache/spark/deploy/yarn/YarnSparkHadoopUtil.scala

+        logInfo(s"$service class not found $e")
+        logDebug("Hive Class not found", e)
+      case t: Throwable => {
+        throw t


Here the exception is thrown. I know swallow the exception is bad, but what happen if the user does not want to access the hive metastore but want to use spark even if token cannot be acquired?

The user can (i) not give Spark a hive configuration, in which case there will be no metastore URIs, and this code will be skipped, or (ii) set spark.yarn.security.tokens.hive.enabled to false.

SparkQA · 2015-10-28T11:52:37Z

Test build #44522 has finished for PR 9232 at commit 217faba.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):\n * logInfo(s\"$service class not found $e\")\n * logDebug(\"$service class not found\", e)\n

steveloughran · 2015-10-29T10:26:33Z

yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala

@@ -1337,55 +1337,9 @@ object Client extends Logging {
      conf: Configuration,
      credentials: Credentials) {
    if (shouldGetTokens(sparkConf, "hive") && UserGroupInformation.isSecurityEnabled) {
-      val mirror = universe.runtimeMirror(getClass.getClassLoader)


this should go to utils.getContextOrSparkClassLoader(); notable that scalastyle doesn't pick up on this, even though it rejects Class.forName() since SPARK-8962

SparkQA · 2015-10-29T21:31:25Z

Test build #44637 has finished for PR 9232 at commit dd8dea9.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):\n * logInfo(s\"Hive class not found $e\")\n * logDebug(\"Hive class not found\", e)\n

vanzin · 2015-10-30T07:47:35Z

yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala

-        case e: Exception => { logError("Unexpected Exception " + e)
-          throw new RuntimeException("Unexpected exception", e)
-        }
+      val util = new YarnSparkHadoopUtil()


This should be YarnSparkHadoopUtil.get; you could also avoid the val altogether.

vanzin · 2015-10-30T07:53:27Z

Just some minor things left to clean up, otherwise looks ok.

SparkQA · 2015-10-30T11:06:12Z

Test build #44676 has finished for PR 9232 at commit ebb2b5a.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):\n * logInfo(s\"Hive class not found $e\")\n * logDebug(\"Hive class not found\", e)\n

vanzin · 2015-10-31T23:52:18Z

Latest patch LGTM.

vanzin · 2015-11-01T01:21:14Z

Merging to master.

vanzin · 2015-11-01T01:59:07Z

Hi @steveloughran, this patch doesn't merge cleanly to branch-1.5. If you want to apply it there, could you send a new pr for that branch? Thanks.

steveloughran · 2015-11-01T10:41:27Z

Will do. Same JIRA or a new backport one?

tedyu · 2015-11-01T14:24:50Z

yarn/src/main/scala/org/apache/spark/deploy/yarn/YarnSparkHadoopUtil.scala

+    } catch {
+      case e: ClassNotFoundException =>
+        logInfo(s"Hive class not found $e")
+        logDebug("Hive class not found", e)


Why double log the message ?

They're not exactly the same.

vanzin · 2015-11-01T21:14:49Z

Same JIRA or a new backport one?

You could use the same one; I marked it as resolved but it's ok to reopen it for the backport.

steveloughran added 2 commits October 22, 2015 22:08

SPARK-11265 YarnClient can't get tokens to talk to Hive in a secure c…

3bb1b92

…luster. This is just the first "it compiles" stage of the patch: no tests or attempts to run things yet

SPARK-11265 reduce size of patch on imports, even though the existing…

1fcb42e

… set doesn't follow the current rules precisely

dougb reviewed Oct 22, 2015
View reviewed changes

chesterxgchen reviewed Oct 22, 2015
View reviewed changes

steveloughran added 2 commits October 23, 2015 14:46

SPAK-11265 only get hive & HBase tokens if the client is in cluster mode

9630a9d

steveloughran changed the title ~~Stevel/patches/spark 11265 hive tokens~~ SPARK-11265 hive tokens Oct 23, 2015

tgravescs reviewed Oct 23, 2015
View reviewed changes

Revert "SPAK-11265 only get hive & HBase tokens if the client is in c…

5158afe

…luster mode" This reverts commit 9630a9d.

SPARK-11265 get all Hive class introspected methods before invoking a…

9713cf5

…ny; always call closeCurrent() but only log that call's exceptions, rather than rethrow,

steveloughran added 2 commits October 23, 2015 22:21

SPARK-11265 by asking for the closeCurrent() method first, it can be …

bc6ea26

…put in a val, and, as we know that it is valid from then on, called directly in the following try/finally block

SPARK-11265 tightened up: javadocs, remove unused sparkConf arg and i…

b417ca3

…mports

steveloughran changed the title ~~SPARK-11265 hive tokens~~ [SPARK-11265] [YARN] YarnClient can't get tokens to talk to Hive in a secure cluster Oct 24, 2015

[SPARK-11265] revert newline insertion at end of file

89edfa6

vanzin reviewed Oct 24, 2015
View reviewed changes

vanzin reviewed Oct 27, 2015
View reviewed changes

[SPARK-11265] don't log throwables

00cb5a7

zhzhan reviewed Oct 28, 2015
View reviewed changes

SPARK-11265 make sure string @ debug is consistent

217faba

steveloughran reviewed Oct 29, 2015
View reviewed changes

[SPARK-11265] merge exception handling into main method; cut test

dd8dea9

vanzin reviewed Oct 30, 2015
View reviewed changes

[SPARK-11265] address review comments of 29-oct-2015

ebb2b5a

asfgit closed this in 40d3c67 Nov 1, 2015

tedyu reviewed Nov 1, 2015
View reviewed changes

steveloughran deleted the stevel/patches/SPARK-11265-hive-tokens branch November 3, 2015 15:06

[SPARK-11265] [YARN] YarnClient can't get tokens to talk to Hive 1.2.1 in a secure cluster #9232

[SPARK-11265] [YARN] YarnClient can't get tokens to talk to Hive 1.2.1 in a secure cluster #9232

Conversation

steveloughran commented Oct 22, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Oct 22, 2015

Choose a reason for hiding this comment

steveloughran commented Oct 23, 2015

SparkQA commented Oct 23, 2015

SparkQA commented Oct 23, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steveloughran commented Oct 23, 2015

SparkQA commented Oct 23, 2015

steveloughran commented Oct 23, 2015

SparkQA commented Oct 23, 2015

steveloughran commented Oct 23, 2015

SparkQA commented Oct 23, 2015

SparkQA commented Oct 23, 2015

vanzin commented Oct 24, 2015

SparkQA commented Oct 24, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Oct 27, 2015

SparkQA commented Oct 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Oct 28, 2015

Choose a reason for hiding this comment

SparkQA commented Oct 29, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vanzin commented Oct 30, 2015

SparkQA commented Oct 30, 2015

vanzin commented Oct 31, 2015

vanzin commented Nov 1, 2015

vanzin commented Nov 1, 2015

steveloughran commented Nov 1, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vanzin commented Nov 1, 2015