Skip to content
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

core: add 50 hop limit to rpc forwarding #16577

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

schmichael
Copy link
Member

If I add an infinite loop due to client RPCs as in #16517, I now get a hideous error message like this in 21ms:

$ nomad node meta read -node-id 36
Error reading dynamic node metadata: Unexpected response code: 500 (rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: rpc error: exceeded hop limit)

I think this is probably a good failsafe, although making the error friendlier would be nice.

I should be able to test it by adding a custom broken handler to a Server.

I think the maximum number of hops if everything goes well is 3 as HTTP->RPC isn't counted, and Client RPCs mostly aren't counted (although could be? 🤔)

  1. Intra-region stale read max hops (1): Server
  2. Intra-region write max hops (2): Follower -> Leader
  3. Inter-region stale read max hops (2): Region 1 Server -> R2 Server
  4. Inter-region write max hops (3): Region 1 Server -> R2 Follower -> R2 Leader

That being said requests across leadership elections may get forwarded more than once, and I'd hate to set this so low it unexpectedly broke some future feature. 50 hops in 21ms (locally).

@schmichael schmichael requested a review from lgfa29 March 21, 2023 00:03
@tgross tgross added the stage/needs-rebase This PR needs to be rebased on main before it can be backported to pick up new BPA workflows label May 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stage/needs-rebase This PR needs to be rebased on main before it can be backported to pick up new BPA workflows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants