-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-16742] Mesos Kerberos Support #17665
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Test build #75872 has finished for PR 17665 at commit
|
|
Test build #75873 has finished for PR 17665 at commit
|
4014b99 to
a47c9c0
Compare
|
Test build #75874 has finished for PR 17665 at commit
|
|
Skimmed through the PR - the way it has been currently designed, I dont think renewal can be added in future. I am hoping I missed something; can you point me to how it is going to be approached ? Some doc/proposal or notes ? |
mridulm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a few queries I had at a high level.
| val tokensBuf = new java.io.ByteArrayInputStream(tokens) | ||
| creds.readTokenStorageStream(new java.io.DataInputStream(tokensBuf)) | ||
| UserGroupInformation.getCurrentUser.addCredentials(creds) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is relying on an immutable initial credential populated in the driver conf.
How is renewal scenario expected to be handled here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my pending renewal PR, I've implemented it to use a new UpdateDelegationToken RPC call. This is just used for the initial transmission. I guess it makes sense to consolidate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be great, thanks - I am trying to get a sense of how renewal works in this case.
There some ongoing work for having different ways to update credentials; and I was hoping this (from what I understand, not using hdfs but direct rpc) would be another way to do it - allowing for multiple common implementations which can be leveraged across all schedulers depending on requirements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To elaborate; one potential use of it is to do token acquisition and token distribution without needing to provide principal/keytab to spark application (other than keeping track of last credential update to account for AM failures).
This is WIP though, but if mesos approach is different from yarn - that would be a great way to iterate on the latter aspect (token distribution); and ensure it is extensible enough for future requirements/implementations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I believe @jerryshao is working on a standard method of delegating tokens. We can plug that in once it exists.
| package org.apache.spark.deploy.security | ||
|
|
||
| import org.apache.hadoop.conf.Configuration | ||
| import org.apache.hadoop.security.{Credentials, UserGroupInformation} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this move not break all existing ServiceCredentialProvider's ? (since the service discovery via META-INF/services will get affected now).
@jerryshao and @vanzin can comment more since they wrote/reviewed this IIRC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code in the "yarn" module is sort of considered "private", so anyone extending those classes is walking on thin ice... the internal services files need to be moved / updated, but I don't think we need to worry about external implementations.
This also means we need to be more careful here when making classes / traits public. Maybe add @InterfaceStability.Evolving annotations if they haven't been added, or something.
(Haven't looked at the rest of the code yet, kinda waiting for the PR builder to be happy first.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've moved all the ServiceCredentialProvider subclasses to core, and I've updated the references in yarn's META-INF.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vanzin Since this is documented in running-on-yarn.md, I dont think it can be considered internal implementation detail anymore; but it is part of our exposed contract.
@mgummelt I did see that all spark impl have been moved - but since this is an exposed contract, there can be (and are) external implementations which rely on it.
If we are breaking api compatibility, we should be very explicit about it : I wanted to make sure it is called out, and evaluated accordingly (and not an internal implementation detail within yarn/core).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... that's unfortunate, not just because of this change, but because yarn/ is not included in mima checks, so if the class changed in incompatible ways we wouldn't notice.
I really doubt anyone has custom implementations of that interface at this point. And making it a proper API in core is better going forward, IMO. If you really think that it's important to keep backwards compatibility, we could have the old interface present, extending the new one, and some compatibility code to keep both working. But that sounds like a lot of work for something I doubt anyone is currently using...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vanzin Would be good to keep backward compatibility since there are users of the interface.
I did not realize yarn was excluded from MIMA - that is unfortunate.
Cursory look at ServiceLoader seemed to indicate it might not be very involved to provide backward compatibility (yarn interface extends from core, load for both interfaces; and use both as core impl will do ? or did I miss something here ?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For backwards compat you'd have to call ServiceLoader twice, to load implementations of the two interfaces. That should be most of it, really, assuming the interface itself is not changing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK I'll try to unbreak backwards compatibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I:
- Readded
deploy.yarn.security.ServiceCredentialProvider, extended fromdeploy.security.ServiceCredentialProvider, and deprecated it. - Added a new
YARNConfigurableCredentialProvider, which extends fromConfigurableCredentialProvider, with the added behavior that it loads providers fromdeploy.yarn.security.ServiceCredentialProvideras well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not quite sure this is correct though. As it is, if a user has a service listed in the resources entry for both deploy.security.ServiceCredentialProvider and deploy.yarn.security.ServiceCredentialProvider, then they will fetch tokens twice for each service. This should only occur if the user's workflow is to drop a duplicated, but modified version of the deploy.yarn.security.ServiceCredentialProvider resources file into META-INF. If they have a fork of Spark that they merge, or if they have an entry in some other jar for just their customized service, there should be no duplication problem.
Thoughts?
| val numTokensBefore = userCreds.numberOfTokens | ||
| val hadoopConf = SparkHadoopUtil.get.newConfiguration(conf) | ||
| val credentialManager = new ConfigurableCredentialManager(conf, hadoopConf) | ||
| credentialManager.obtainCredentials(hadoopConf, userCreds) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is potentially buggy - and will work only the first time for some credential providers - hbase (for example).
Token acquisition will fail for hbase when (prev) TOKEN is already present in the credentials - irrespective of whether kerberos credentials are present or not.
Acquiring credentials in context of ugi returned via UserGroupInformation.loginUserFromKeytabAndReturnUGI avoids this issue.
@jerryshao might have a better reference for the hbase behavior - since IIRC he worked a bit on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fetching tokens multiple times isn't supported yet. That will come in the renewal PR.
When you say it will fail, do you mean it will throw an exception, or just fail to fetch a token? Looking at the code, I don't see how either would happen: https://github.com/mesosphere/spark/blob/SPARK-16742-kerberos/core/src/main/scala/org/apache/spark/deploy/security/HBaseCredentialProvider.scala#L46 It looks like it fetches a token indepedent of the current credentials, then overwrites any current HBase token via creds.addToken
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mgummelt It might be a bit involved, and has to do with how hbase issues tokens at its side (not in client library, but at the server) - if it sees a token in the incoming request, it will not issue new tokens.
@jerryshao is the right person to detail it if required, but bottomline is, if we add acquired tokens to ugi.currentUser, then subsequently we cannot use ugi.currentUser to acquire tokens for hbase. This actually comes from semantics of token, and might be applicable to other token providers as well (hbase is one case I know where it does this for sure).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regardless, it shouldn't be an issue until we add renewal, right? Without renewal, we only ever fetch a single token.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that is why I said this is potentially buggy :-)
As it currently stands in the current happy path - since there is only a single invocation of the method, it will work.
But as soon as another invocation of the method happens in the same jvm (through any means); it will fail to acquire tokens.
…r for backwards compatibility
|
Test build #75916 has finished for PR 17665 at commit
|
|
Test build #75918 has finished for PR 17665 at commit
|
| @@ -1 +0,0 @@ | |||
| org.apache.spark.deploy.yarn.security.TestCredentialProvider | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vanzin Do you know the purpose of this test service provider? I don't see this class anywhere in the code base.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is for unit test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which unit test uses it? Since the class doesn't exist anywhere, what effect does it have?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ConfigurableCredentialManagerSuite. It is loaded by ServiceLoader implicitly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean "it does not exist anywhere"?
It's declared in ConfigurableCredentialManagerSuite.scala in my tree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, whoops. I was searching for the file with the same name. Thanks.
| // must trick it into thinking we're YARN. | ||
| if (clusterManager == MESOS && UserGroupInformation.isSecurityEnabled) { | ||
| val shortUserName = UserGroupInformation.getCurrentUser.getShortUserName | ||
| val key = s"spark.hadoop.${YarnConfiguration.RM_PRINCIPAL}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it work in user impersonation scenario? Here shortUserName is a real user, while HadoopRDD may execute as a proxy user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it? It looks like getCurrentUser returns the subject from the current AccessControllerContext, which is set by the doAs when --proxy-user is set.
Regardless, the renewer specified here actually has no effect, since we aren't renewing yet. Once I add renewal, I'll need to revisit this to make sure it's consistent with the renewal we're going to do in MesosSecurity. I'm only setting this now to avoid an Exception that gets thrown in the hadoop library if the master principal is not configured. See the JIRA in the above code comment for details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The renewer for delegation tokens should be the user that created them (if the service is not doing it for the user, like in the YARN case); only the logged in user can create tokens, so this has to be the current user (not the proxy user, which happens later).
The tokens should be created in the proxy user's name (so user = "proxy" renewer = "real user"), when you care to support that.
Once I add renewal
So do you mean that currently this only works until the delegation tokens need renewal (which is 1 day by default - a lot shorter than the max life time)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So do you mean that currently this only works until the delegation tokens need renewal (which is 1 day by default - a lot shorter than the max life time)?
Yes
| private[yarn] final class ConfigurableCredentialManager( | ||
| private[spark] class ConfigurableCredentialManager( | ||
| sparkConf: SparkConf, hadoopConf: Configuration) extends Logging { | ||
| private val deprecatedProviderEnabledConfig = "spark.yarn.security.tokens.%s.enabled" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we also rename the configurations, currently it is "spark.yarn.security.*"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, these are deprecated configs which need to keep the old names for compatibility.
|
Test build #75945 has finished for PR 17665 at commit
|
|
retest this please |
|
Test build #75946 has finished for PR 17665 at commit
|
vanzin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this PR would benefit from being broken in two: one to deal with the refactoring of the existing code, without adding anything that is required by Mesos, and a second one to integrate the refactored code with Mesos. The refactoring has some parts in it that are a bit tricky to handle, and it would benefit from having a more focused PR that didn't have new features added into the mix.
| import org.apache.commons.lang3.StringUtils | ||
| import org.apache.hadoop.fs.Path | ||
| import org.apache.hadoop.security.UserGroupInformation | ||
| import org.apache.hadoop.yarn.conf.YarnConfiguration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... you're adding a YARN dependency in core, which should be able to build without YARN... but you're not actually adding a new dependency to the POM, so I guess the dependency is already there indirectly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea it looks like this should require hadoop-yarn-api. I'll add this dependency to core unless you object, in which case I suppose I could just use the raw string instead of YarnConfiguration.RM_PRINCIPAL, but that seems hacky.
FYI, we originally talked about placing the code below into the Mesos scheduler, but that seems to be too late. The SparkContext constructor creates a copy of the configuration, so we need to set the required YARN property before SparkContext is created, which means before user code gets run, which probably means somewhere in SparkSubmit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you need the explicit dependency (otherwise this would not be compiling). This is probably being brought by hadoop-client already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to explicitly declare a dependency rather than rely on transitivity, right?
| // must trick it into thinking we're YARN. | ||
| if (clusterManager == MESOS && UserGroupInformation.isSecurityEnabled) { | ||
| val shortUserName = UserGroupInformation.getCurrentUser.getShortUserName | ||
| val key = s"spark.hadoop.${YarnConfiguration.RM_PRINCIPAL}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The renewer for delegation tokens should be the user that created them (if the service is not doing it for the user, like in the YARN case); only the logged in user can create tokens, so this has to be the current user (not the proxy user, which happens later).
The tokens should be created in the proxy user's name (so user = "proxy" renewer = "real user"), when you care to support that.
Once I add renewal
So do you mean that currently this only works until the delegation tokens need renewal (which is 1 day by default - a lot shorter than the max life time)?
| private[yarn] final class ConfigurableCredentialManager( | ||
| private[spark] class ConfigurableCredentialManager( | ||
| sparkConf: SparkConf, hadoopConf: Configuration) extends Logging { | ||
| private val deprecatedProviderEnabledConfig = "spark.yarn.security.tokens.%s.enabled" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, these are deprecated configs which need to keep the old names for compatibility.
| private[spark] class ConfigurableCredentialManager( | ||
| sparkConf: SparkConf, hadoopConf: Configuration) extends Logging { | ||
| private val deprecatedProviderEnabledConfig = "spark.yarn.security.tokens.%s.enabled" | ||
| private val providerEnabledConfig = "spark.yarn.security.credentials.%s.enabled" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now these configs probably should be updated to the new namespace, with code added for backwards compatibility...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| * A credential provider for a service. User must implement this if they need to access a | ||
| * secure service from Spark. | ||
| */ | ||
| trait ServiceCredentialProvider { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No annotation means "Unstable" IIRC, but might as well be explicit here and add a proper InterfaceStability annotation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does InterfaceStability relate to scala language level access control (public, private, etc.)? Every public interface must maintain backwards compatibility during a major version, right? So does it make sense to have a public trait that's unstable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not a Scala thing, it's a Spark API thing. When adding a public API that might change, we annotate it appropriately so that people can know that it's not really stable. Flip side, if you change an API that is tagged with a "Stable" annotation, you'll be yelled at.
It also shows up in docs:
http://spark.apache.org/docs/latest/api/scala/index.html#org.apache.spark.sql.util.QueryExecutionListener
(Basically this is the new version of @DeveloperApi and @Experimental.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When adding a public API that might change
Ah, thanks. I didn't know this was allowed at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| private[spark] object CoarseGrainedExecutorBackend extends Logging { | ||
|
|
||
| private def addMesosDelegationTokens(driverConf: SparkConf) { | ||
| val value = driverConf.get("spark.mesos.kerberos.userCredentials") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You really should be using the config constant you create here.
But a bigger issue is that this value will be written to event logs and shown in the UI. Instead of using the config for this, would it be too hard to add this as a field in SparkAppConfig (see CoarseGrainedClusterMessage.scala)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, I'll go ahead and change this to an RPC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| .*\.sql | ||
| .Rbuildignore | ||
| org.apache.spark.deploy.yarn.security.ServiceCredentialProvider | ||
| org.apache.spark.deploy.security.ServiceCredentialProvider |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a change I have internally I changed this to:
META-INF/services/*
If that works (haven't tested yet) I think that's better than having to keep changing / adding these files here...
| def setUGITokens(conf: SparkConf): Unit = { | ||
| val userCreds = getDelegationTokens(conf) | ||
|
|
||
| val byteStream = new java.io.ByteArrayOutputStream(1024 * 1024) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it necessary to create a large buffer up front? Credentials will be much smaller than that in most situations.
Also, why not just import these types?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No good reason. Fixed both.
| sparkConfVars: Map[String, String] = null, | ||
| home: String = "/path"): Unit = { | ||
| sparkConf = (new SparkConf) | ||
| home: String = "/path"): SparkConf = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: parameters need an extra level of indent.
| @@ -0,0 +1,3 @@ | |||
| org.apache.spark.deploy.yarn.security.YARNHadoopFSCredentialProvider | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This worried me a little bit. Because I'm pretty sure ServiceLoader will look at both files (this one and the one in core), and when running in YARN you'll end up with way too many providers.
So you should verify that behavior, and the best way to do it is to write a unit test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yes. This is a problem. I'll write the unit test, and move the core META-INF to Mesos. I was hoping for a general set of registered providers, but I guess each scheduler will need to register their own.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move the core META-INF to Mesos
That's not enough. Because in a real app (not unit tests), the mesos and yarn jars will both still exist, so both files will still be loaded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yea, I guess the standard Spark distributions are compiled with both profiles. I created a separate YARNHadoopFSCredentialProvider to handle YARN's specific functionality (renewer, extra filesystems, etc.), but I guess I could do that through parameterization rather than a separate class, since it seems we can't have a YARN specific class through service loading. How does that sound?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but I guess I could do that through parameterization
That would probably be better. You can see if using SparkHadoopUtil / YarnSparkHadoopUtil to handle the differences can help too.
|
@vanzin How important is it to you for this to be two PRs? I already factored out the renewal logic to simplify this PR, and I'd rather not have to decouple again. But I can if it's a blocker. |
|
The thing that triggered my comment is the fact that I'm pretty sure you're breaking the YARN side with this change (loading too many credential providers, including one that probably won't work with YARN). Having the refactoring separately would make it easier to make sure YARN keeps working as before without having to include all the new code that deals with Mesos in the mix. |
| --> | ||
| <dependency> | ||
| <groupId>${hive.group}</groupId> | ||
| <artifactId>hive-exec</artifactId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a problem here that I could use some help on.
hive-exec has a dependency on pentaho that doesn't exist in maven central anymore. This is why we have an entry for it in spark-parent's dependencyManagement section that excludes pentaho via calcite-core.
But the scope in spark-parent's dependencyManagement entry for hive-exec is compile, so if I make it test here, which is what it was in YARN, then it fails to match that entry anymore, and thus doesn't inherit the excludes. This shouldn't be a problem, because dependencies in the test scope aren't transitive, so we should never be trying to fetch the missing pentaho dependency. But sbt builds don't seem to recognize this transitivity, so if I add the test scope, I get this error:
[warn] Note: Unresolved dependencies path:
[warn] org.pentaho:pentaho-aggdesigner-algorithm:5.1.5-jhyde
[warn] +- org.apache.calcite:calcite-core:1.2.0-incubating ((com.typesafe.sbt.pom.MavenHelper) MavenHelper.scala#L76)
[warn] +- org.spark-project.hive:hive-exec:1.2.1.spark2 ((com.typesafe.sbt.pom.MavenHelper) MavenHelper.scala#L76)
[warn] +- org.apache.spark:spark-core_2.11:2.2.0-SNAPSHOT ((com.typesafe.sbt.pom.MavenHelper) MavenHelper.scala#L76)
[warn] +- org.apache.spark:spark-catalyst_2.11:2.2.0-SNAPSHOT ((com.typesafe.sbt.pom.MavenHelper) MavenHelper.scala#L76)
[warn] +- org.apache.spark:spark-sql_2.11:2.2.0-SNAPSHOT ((com.typesafe.sbt.pom.MavenHelper) MavenHelper.scala#L76)
[warn] +- org.apache.spark:spark-hive_2.11:2.2.0-SNAPSHOT
What I can't understand is how this wasn't a problem when these dependencies were in YARN. For some reason sbt dependencyTree on master shows that calcite-core is excluded from the hive-exec dependency tree, but the same command on this PR tries to resolve calcite-core.
Let me know if you have any ideas here. My best idea right now is to just try to duplicate the excludes in spark-parent.
|
Test build #75956 has finished for PR 17665 at commit
|
|
@vanzin OK, I'll split it up. |
|
I've addressed all review comments, and as per @vanzin's request, I'll now close this out and resubmit a PR with just the YARN refactoring. |
|
Test build #76008 has finished for PR 17665 at commit
|
| timeOfNextTokenRenewal = (currTime - currTime % tokenRenewalInterval) + tokenRenewalInterval | ||
|
|
||
| Some(timeOfNextTokenRenewal) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you leave comments for each deleted test case to explain what are the new location?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. I'll come back to all of your requests once #17723 gets merged.
| sparkProperties: Seq[(String, String)], | ||
| ioEncryptionKey: Option[Array[Byte]]) | ||
| ioEncryptionKey: Option[Array[Byte]], | ||
| ugiTokens: Option[Array[Byte]]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add descriptions what is ugiTokens?
| private def addDelegationTokens(tokenBytes: Array[Byte], driverConf: SparkConf) { | ||
| val creds = new CredentialsSerializer().deserializeTokens(tokenBytes) | ||
|
|
||
| logInfo(s"Adding ${creds.numberOfTokens()} tokens and ${creds.numberOfSecretKeys()} secret" + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: secret" -> secret "
cc @skonto @vanzin @ArtRand
What changes were proposed in this pull request?
Mesos Kerberos Support. This does not yet include support for cluster mode, or for delegation token renewal. That will come in a future PR.
The delegation tokens are passed to the executors via RPC. The driver sets the tokens as a configuration property, which the executors then read by issuing the
RetrieveSparkAppConfigRPC call (I did not add this call. It already existed).ConfigurableCredentialProviderand all of theCredentialProviderclasses have been moved from theyarntocoremodule so that they may be shared by Mesos.How was this patch tested?
Manually against a kerberized HDFS cluster. Verified that the job failed w/o
kinitand succeeded w/kinit