-
Notifications
You must be signed in to change notification settings - Fork 2k
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
fsm: one-time token expiration should be deterministic #13737
Conversation
When applying a raft log to expire ACL tokens, we need to use a timestamp provided by the leader so that the result is deterministic across servers.
4bf50f5
to
3efb883
Compare
@@ -12077,6 +12077,7 @@ type OneTimeTokenDeleteRequest struct { | |||
|
|||
// OneTimeTokenExpireRequest is a request to delete all expired one-time tokens | |||
type OneTimeTokenExpireRequest struct { | |||
Timestamp time.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.
Doesn't this need to get set in CoreScheduler.expiredOneTimeTokenGC
? I don't see where this gets set, and core_sched.go
is where the other GC operations compute their threshold based on the timetable/local clock before submitting the deletions via raft.
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 should be set on the ACL.ExpireOneTimeTokens
RPC, which I missed b/c the tests passed.
Most of the core jobs use the timetable block specifically because they don't submit the deletions via raft, but via the RPC. While the leader creates the GC eval, it can be processed on any worker (ref worker.go#L565-L569
).
So when I wrote the expire OTT job originally, I hit the same "quiet clusters will have wonky timestamps" problem @jrasell discovered (and I wanted to make transaction logic simple). So I wanted to use the wall clock on the leader to get a more accurate expiration. But I messed that up by not having the timestamp set in the leader's RPC, but in the FSM instead. So we should be setting that on the leader's handler for the RPC and we'll be all set.
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.
Fixed in 9d8138c
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
Fixes #13725 for one-time tokens.
When applying a raft log to expire ACL tokens, we need to use a
timestamp provided by the leader so that the result is deterministic
across servers.