Size active queried series worker pool from GOMAXPROCS#7671
Open
anxkhn wants to merge 1 commit into
Open
Conversation
The active queried series service sized its worker pool with runtime.NumCPU(), which returns the host's logical CPU count and ignores the CPU cgroup quota. Cortex imports go.uber.org/automaxprocs so that GOMAXPROCS matches the container's CPU limit, and reads container-aware parallelism via runtime.GOMAXPROCS(0) elsewhere (for example the resource monitor's CPU limit and the engine's decoding concurrency default). This one call site was inconsistent: in a CPU-limited container on a many-core host it sized the pool off the host cores instead of the CPU budget. Use runtime.GOMAXPROCS(0) so the pool follows the container CPU quota, matching the rest of the codebase. The min(..., 4) cap is unchanged. Add a regression test asserting the worker count follows GOMAXPROCS. Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
runtime.NumCPU()returns the number of logical CPUs on the host and does notaccount for the CPU cgroup quota. Cortex imports
go.uber.org/automaxprocs(
cmd/cortex/main.go) precisely so thatGOMAXPROCSreflects the container'sCPU limit, and elsewhere it reads container-aware parallelism via
runtime.GOMAXPROCS(0), for example the resource monitor's CPU limit(
pkg/cortex/modules.go) and the engine's decoding-concurrency default, whichis documented as
GOMAXPROCS / 2(pkg/engine/config.go).This one call site is inconsistent with that convention. On a many-core host
with the ingester restricted to a small CPU quota via cgroups, it sizes the
pool off the host core count rather than the CPU budget (for example, on a
64-core host limited to 2 CPUs it computes
min(64/2, 4) = 4workers insteadof the
min(GOMAXPROCS/2, 4)the rest of the code would use).This PR switches the call site to
runtime.GOMAXPROCS(0)so the pool followsthe container CPU quota, matching the rest of the codebase. The
min(..., 4)cap is unchanged, so the blast radius is small. A table-driven regression test
asserts the worker count follows
GOMAXPROCS.Which issue(s) this PR fixes:
No open issue; this is a small self-contained correctness fix. Happy to open a
tracking issue first if maintainers prefer.
Checklist
CHANGELOG.mdupdateddocs/configuration/v1-guarantees.mdupdated if this PR introduces experimental flags (n/a: no flags)This change was prepared with AI assistance (per the project's GENAI policy);
I reviewed every line, confirmed the behavior against the codebase's existing
automaxprocs/GOMAXPROCS(0)usage, and verified the regression test is redagainst
runtime.NumCPU()and green with the fix.