Collect inventory concurrently within each collection phase - #11022
Collect inventory concurrently within each collection phase#11022smklein wants to merge 2 commits into
Conversation
Sled agent, timesync, internal DNS generation, and Clickhouse keeper collection previously issued requests to their targets one at a time. A single unresponsive sled agent could stall collection for its full 60-second timeout before the next sled was even contacted. Each of these phases now fans out requests via ParallelTaskSet with bounded concurrency (MAX_CONCURRENT_INVENTORY_REQUESTS = 8). Tasks return per-target results, which are merged into the CollectionBuilder on the calling task in enumeration order, so the collection contents and the order of recorded errors are unchanged from serial collection. The phases themselves still run one at a time (timesync and DNS derive their target lists from sled agent inventory). MGS collection remains serial because it deduplicates work across MGS clients as it goes, which assumes ordered processing; parallelizing it is left for a follow-up. CockroachDB collection was already concurrent internally.
| // of this in parallel. But this code path is not remotely | ||
| // latency-sensitive. And there's real risk of overloading our | ||
| // downstream services. So we just do one step at a time. This also | ||
| // keeps the code simpler. |
There was a problem hiding this comment.
Without this PR, I'm observing dogfood take ~55 - 65 seconds to collect inventory. This seems "fine", but I figured I'd put this PR up because:
- I think the code remains pretty simple
- I've had some discussions with folks, where we feared adding stuff to inventory because it "could get too slow'. I think this is a reasonable fear! we don't want unnecessary bloat. but this forced serialization seems like it's too restrictive?
Anyway, MAX_CONCURRENT_INVENTORY_REQUESTS sets the bound of how much concurrency we want to allow. I picked an arbitrary number, which seems very unlikely to risk "overloading our downstream services".
hawkw
left a comment
hawkw
left a comment
There was a problem hiding this comment.
neat! i'd be interested to see whether the MGS bit can also be parallelized a bit more than it is presently, but that would probably require some bigger changes to the way we de-duplicate things...
Thanks for the feedback! yeah, I agree - happy to do this, but wanted to do the "as simple as possible conversions to paralleltaskset first". |
I interpreted that issue to be directional. i.e., "do a bunch more concurrency here". I don't think it makes sense to interpret it as "do everything as concurrently as possible". 🤷 we can leave it open if we want but I don't think we'd prioritize going further unless it became an actual problem. |
|
Yeah, that's fair! |
3 participants
Inventory collection was previously a fully serial operation. This PR changes that - it now uses (bounded) concurrency.
The sled agent, timesync, internal DNS generation, and Clickhouse keeper phases now fan out requests via
ParallelTaskSetwith bounded concurrency (MAX_CONCURRENT_INVENTORY_REQUESTS = 8). Each task returns a per-target result, and results are merged into the builder on the calling task in enumeration order. This keeps the collection contents and the order of recorded errors identical to serial collection and checked by the collector's expectorate tests. Those tests pass with no golden changes.The phases themselves still run one at a time: timesync and DNS collection derive their target lists from sled agent inventory. MGS collection remains serial because it deduplicates work across MGS clients as it goes (the
found_*_already()checks), which assumes ordered processing. CockroachDB collection was already concurrent internally.If we wanted to interleave collection phases, we could (this would result in slightly snappier inventory collections) but IMO would be marginally more complex. I figured I'd go this route first, because it's pretty simple.
fixes #6818 and #4753.