YARN-11112 Avoid renewing delegation token when app is first submitte… #4198
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
💔 -1 overall
This message was automatically generated. |
Contributor
steveloughran
left a comment
steveloughran
left a comment
There was a problem hiding this comment.
Someone who understands YARN properly will need to review this ... not me
It does seem excessive to renew tokens immediately. But it will be good to understand why that decision was made.
Does the Git history of the file provide any information?
Looking at the patch, the process must renew tokens just before they expire rather than waiting until they have actually expired. And we can't assume that every hosts clocks in perfect sync.
| import java.util.concurrent.locks.ReadWriteLock; | ||
| import java.util.concurrent.locks.ReentrantReadWriteLock; | ||
|
|
||
| import org.apache.hadoop.security.token.TokenIdentifier; |
Contributor
There was a problem hiding this comment.
nit: should go in to the other org.apache. imports
| try { | ||
| renewToken(dttr); | ||
| // if expire date is not greater than now, renew token. | ||
| if (expirationDate <= now) { |
Contributor
There was a problem hiding this comment.
that's going to break if the clocks are out of sync; tokens also need to be renewed if they are going to expire soon
|
💔 -1 overall
This message was automatically generated. |
Member
Author
|
@steveloughran Thanks for your comment. Let's say a user submit an app,
The Step 6 is not very necesarry as the expire time does not vary a lot (only several minutes) . Renewing token will be a heavy opertion in a very busy cluster(in our case, there are nearly 40, 000 apps an hour), as it uses namesystem write lock. --> And we can't assume that every hosts clocks in perfect sync |
Hexiaoqiao
reviewed
Apr 21, 2022
Contributor
Hexiaoqiao
left a comment
Hexiaoqiao
left a comment
There was a problem hiding this comment.
This is good improvement to me. leave comment inline, FYI.
| dttr = new DelegationTokenToRenew(Arrays.asList(applicationId), token, | ||
| tokenConf, now, shouldCancelAtEnd, evt.getUser()); | ||
| // decode identify to get maxDate. | ||
| TokenIdentifier tokenIdentifier = token.decodeIdentifier(); |
Contributor
There was a problem hiding this comment.
I think this is good improvement, especially for big and busy production cluster. IMO this changes is safe for HDFS, but I am not sure if it is proper for other system.
Another way, just suggest to update org.apache.hadoop.yarn.server.resourcemanager.security.DelegationTokenRenewer#skipTokenRenewal rather than here to avoid double decode token. Also it is reasonable and readable.
added 2 commits
April 21, 2022 18:09
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
zuston
reviewed
Apr 25, 2022
Member
zuston
left a comment
zuston
left a comment
There was a problem hiding this comment.
Nice improvement. @yuanboliu
Thanks for your effort on this ticket which is also useful to us.
| hasHdfsToken = true; | ||
| } | ||
| if (skipTokenRenewal(token)) { | ||
| if (skipTokenRenewal(token, tokenExpiredTime)) { |
Member
There was a problem hiding this comment.
The var of tokenExpiredTime will be changed in the method of skipTokenRenewal, however we can't found it according to its method name directly. Maybe skipTokenRenewal should be renamed to skipTokenRenewalAndUpdateExpiredTime.
...main/java/org/apache/hadoop/yarn/server/resourcemanager/security/DelegationTokenRenewer.java
Outdated
Show resolved
Hide resolved
| if (identifier == null) { | ||
| return false; | ||
| } | ||
| expiredTime.set(identifier.getMaxDate()); |
Member
There was a problem hiding this comment.
Do we just to reset the expireTime only when the result of skipTokenRenewal is false?
Member
|
@dineshchitlangia @9uapaw @brumi1024 If u have time, could u help review it? I think it's a nice improvement. |
Member
Author
|
@zuston thanks for your comment. We're also working on HDFS-16558 to change the write lock of delegation token to read lock |
|
🎊 +1 overall
This message was automatically generated. |
steveloughran
requested changes
May 16, 2022
| //to cause this one to be set for renew in 2 secs | ||
| Renewer.tokenToRenewIn2Sec = token1; | ||
| AbstractDelegationTokenIdentifier identifier1 = token1.decodeIdentifier(); | ||
| identifier1.setMaxDate(System.currentTimeMillis() + 2 * 1000); |
Contributor
There was a problem hiding this comment.
nit, use 2_000 for representing 2 seconds
| import java.util.concurrent.atomic.AtomicBoolean; | ||
| import java.util.concurrent.atomic.AtomicInteger; | ||
|
|
||
| import org.apache.hadoop.security.token.delegation.AbstractDelegationTokenIdentifier; |
Contributor
There was a problem hiding this comment.
nit, should go in the right place in the org.apache imports.
| + applicationId + ", user=" + evt.getUser(), ioe); | ||
| requestNewHdfsDelegationTokenAsProxyUser( | ||
| Arrays.asList(applicationId), evt.getUser(), | ||
| Arrays.asList(applicationId), evt.getUser(), |
Contributor
There was a problem hiding this comment.
roll this change back just to keep the diff smaller
| import org.apache.hadoop.yarn.server.resourcemanager.rmapp.RMAppEvent; | ||
| import org.apache.hadoop.yarn.server.resourcemanager.rmapp.RMAppEventType; | ||
| import org.apache.hadoop.yarn.server.utils.YarnServerBuilderUtils; | ||
| import org.slf4j.Logger; |
Contributor
There was a problem hiding this comment.
revert moving these...they were in the right place.
imports are where much needless backport/cherrypick issues surface, so minimising changes here is really important.
| try { | ||
| renewToken(dttr); | ||
| // if expire date is not greater than now, renew token. | ||
| if (tokenExpiredTime.get() <= now) { |
Contributor
There was a problem hiding this comment.
this is too brittle. it should allow for some clock skew. maybe renew if within 10 minutes of expiry.
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Member
Author
|
For those who is interesting in this patch, we've applied it in our product env, and it works as expected. |
Hexiaoqiao
reviewed
May 22, 2022
Contributor
Hexiaoqiao
left a comment
Hexiaoqiao
left a comment
There was a problem hiding this comment.
@yuanboliu Thanks for involving me here. Thanks for your good catch here. Leave some nit comment inline. Thanks again.
| renewToken(dttr); | ||
| // if expire date is not greater than now, renew token. | ||
| // add extra time in case of clock skew | ||
| if (tokenExpiredTime.get() <= now + clockSkewExtraTime) { |
Contributor
There was a problem hiding this comment.
Sorry I didn't get why add clock skew condition check here. I think we should file another JIRA to fix if it is issue indeed.
| } | ||
| expiredTime.set(identifier.getMaxDate()); | ||
| Text renewer = identifier.getRenewer(); | ||
| return (renewer != null && renewer.toString().equals("")); |
Contributor
There was a problem hiding this comment.
Just suggest to check if delegation token need to renew while app is just submitted.
Contributor
|
Thanks for the patch @yuanboliu. I was not involved in the implementation of this class, however, according to: https://blog.cloudera.com/hadoop-delegation-tokens-explained/ the immediate renewal is necessary due to "YARN RM verifies the Delegation Tokens are valid by immediately renewing them." What is you opinion about it @steveloughran @Hexiaoqiao? |
Contributor
|
@9uapaw @Hexiaoqiao @yuanboliu I would like to ask, RM_DELEGATIONTOKEN, this TOKEN is generated by RM, but I don't see the logic to verify this TOKEN，can you give some advice？ |
Member
Author
|
Sorry for the late reply |
Member
Author
|
@slfan1989 As for the verification, you can refer it to RMDelegationTokenSecretManager which extends from AbstractDelegationTokenSecretManager checkToken. |
Member
Author
|
For those who's interesting in this patch: |
zeekling
suggested changes
Apr 2, 2025
| if (identifier == null) { | ||
| return false; | ||
| } | ||
| expiredTime.set(identifier.getMaxDate()); |
Contributor
There was a problem hiding this comment.
This should be identifier.getIssueDate()
Contributor
|
We're closing this stale PR because it has been open for 100 days with no activity. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…d to RM
Description of PR
When auth is enabled by NameNode, then delegation token is required if application needs to acess files/directoies. We find that when app is first submitted to RM, RM renewer will renew app token no matter whether token is expired or not. Renewing token is a bit heavy since it uses global write lock. Here is the result when delegation token is required in a very busy cluster.
How was this patch tested?
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?