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

rt: don't reserve a core for the admin runtime admin #613

Closed
wants to merge 3 commits into from

Conversation

hawkw
Copy link
Contributor

@hawkw hawkw commented Jul 31, 2020

As per @olix0r in #611 (comment):

I don't think we should dedicate an entire core to the admin thread...
That's a lot of resources for something we shouldn't be doing much
work on. The real goal of the separate admin runtime is to ensure we
can still interact with the proxy if, for instance, the main runtime
isn't healthy. I think we're best off keeping it simple and treating
it as if the admin thread needs no dedicated resources.

The admin runtime only performs observability work & certifies
identitities, so its load should be pretty light, and it doesn't need to
be actively using a CPU core all the time. The purpose of having a
separate admin runtime is to continue allowing access to observability
functions even if the rest of the proxy gets into a bad state, not to
take load off the main forwarding runtime; the admin thread no longer
performs service discovery lookups (and I think it hasn't for a while).

This branch changes the rt::build function in linkerd2-proxy so
that, when the multithreaded Tokio runtime feature is enabled, we give
the main forwarding runtime a thread for every available CPU core,
rather than every core minus one. Furthermore, if the feature flag is
enabled, we will now use the multithreaded Tokio scheduler if the
number of available CPUs is two or more, rather than three or more,
since we are no longer reserving a thread for the admin runtime.

In order to ensure that the admin runtime is out of the forwarding
path (so that multiple worker threads don't result in a corresponding
increase in admin load), I've moved the DNS resolver background task to
the main runtime. As a potential follow-up, we might want to simplify
the DNS code so that we just spawn resolution tasks directly, rather
than having a DNS daemon task. trust-dns no longer requires a
background task, so its purpose was just to ensure that DNS resolutions
were spawned on the admin thread.

hawkw added 3 commits July 31, 2020 09:03
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw hawkw requested review from olix0r and a team July 31, 2020 16:19
@olix0r
Copy link
Member

olix0r commented Jul 31, 2020

I've got most of this in #612

@hawkw
Copy link
Contributor Author

hawkw commented Jul 31, 2020

I've got most of this in #612

whoops, missed that! we should probably still move the DNS task, i can just cherry-pick that part out if you like?

@olix0r
Copy link
Member

olix0r commented Jul 31, 2020

@hawkw yeah do you want to change this PR to move the tasks onto the main thread? I think we should consider moving the destination & identity clients and the tap server onto the main runtime, too, leaving the admin thread to only be responsible for the admin server. Up to you whether these make sense as separate PRs etc.

@hawkw
Copy link
Contributor Author

hawkw commented Jul 31, 2020

Sounds good, I'll move the DNS and Destination clients next; still on the fence about where tap and identity belong. In particular, I think it might be worth being able to tap a proxy even if it's gone into a stuck state?

@hawkw hawkw closed this Jul 31, 2020
@olix0r olix0r deleted the eliza/no-admin-core branch May 25, 2021 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants