peter-toth commented on code in PR #57285:
URL: https://github.com/apache/spark/pull/57285#discussion_r3639196967
##########
core/src/main/scala/org/apache/spark/deploy/security/HadoopDelegationTokenManager.scala:
##########
@@ -226,24 +261,35 @@ private[spark] class HadoopDelegationTokenManager(
*
* @return Credentials containing the new tokens.
*/
- private def obtainTokensAndScheduleRenewal(ugi: UserGroupInformation):
Credentials = {
- ugi.doAs(new PrivilegedExceptionAction[Credentials]() {
- override def run(): Credentials = {
- val (creds, nextRenewal) = obtainDelegationTokens()
-
- // Calculate the time when new credentials should be created, based on
the configured
- // ratio.
- val now = System.currentTimeMillis
- val ratio = sparkConf.get(CREDENTIALS_RENEWAL_INTERVAL_RATIO)
- val delay = (ratio * (nextRenewal - now)).toLong
- logInfo(log"Calculated delay on renewal is ${MDC(LogKeys.DELAY,
delay)}," +
- log" based on next renewal ${MDC(LogKeys.NEXT_RENEWAL_TIME,
nextRenewal)}" +
- log" and the ratio ${MDC(LogKeys.CREDENTIALS_RENEWAL_INTERVAL_RATIO,
ratio)}," +
- log" and current time ${MDC(LogKeys.CURRENT_TIME, now)}")
- scheduleRenewal(delay)
- creds
+ private def obtainTokensAndScheduleRenewal(): Credentials = {
+ val (creds, nextRenewal) = if (hasKerberosCredentials) {
+ val freshUGI = doLogin()
+ val currentUser = UserGroupInformation.getCurrentUser()
+ val result = freshUGI.doAs(new PrivilegedExceptionAction[(Credentials,
Long)]() {
+ override def run(): (Credentials, Long) = {
+ obtainDelegationTokens()
+ }
+ })
+ if (!currentUser.equals(freshUGI)) {
+ FileSystem.closeAllForUGI(freshUGI)
}
- })
+ result
+ } else if (sparkConf.get(CREDENTIALS_DIRECT_PROVIDERS_ENABLED)) {
+ obtainDelegationTokens(isolateFailures = true)
Review Comment:
**Finding 7.** On this renewal path, `obtainDelegationTokens(isolateFailures
= true)` catches every provider exception (line 199) and returns `None` for the
failed provider. If *all* providers fail (e.g. the IdP is transiently
unreachable at startup), `nextRenewal` folds to `Long.MaxValue`, so the `delay`
computed just below (line 286) is effectively infinite and `scheduleRenewal`
won't re-run for ~centuries. And since nothing throws, `updateTokensTask`'s
`catch` — which would otherwise reschedule after
`CREDENTIALS_RENEWAL_RETRY_WAIT` — never fires. Net: executors receive empty
credentials and there's no recovery short of a driver restart. The Kerberos
path avoids this because it doesn't isolate, so a failure propagates and gets
retried.
Note `nextRenewal == Long.MaxValue` is also the legitimate "providers
succeeded but reported no expiry" case, so keying a retry off the delay alone
would over-fire. Cleaner to have the isolate path signal that a failure
actually occurred (e.g. count the caught exceptions) and, when that count is >
0, schedule `CREDENTIALS_RENEWAL_RETRY_WAIT` instead of the computed delay.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
