Unbounded concurrent Monitoring API requests exhaust memory and OOM-kill the container #537 - #536
Open
ntmspavan wants to merge 3 commits into
Open
Conversation
…currency Each scrape fetches time series for every metric descriptor of every configured project concurrently, with no limit. When google.projects.filter (or a long google.project-ids list) resolves to many projects, this can spawn far more concurrent Monitoring API requests/JSON decodes than a memory-constrained pod can handle, leading to OOM kills. Add monitoring.max-concurrency (default 0, unbounded) which caps concurrent TimeSeries.List requests via a single semaphore shared across all projects, so the limit holds regardless of how many projects are resolved. Signed-off-by: Pavan Nalam <pavan.nalam3693@gmail.com>
Move the concurrency test into monitoring_collector_test.go (the corresponding _test.go file for the changed code), extract semaphore construction into a small newRequestLimiter helper with its own table-driven test, and add direct unit tests for acquire/releaseRequestLimiter covering the nil (unbounded) and blocking-when-full cases. Also assert Config.MaxConcurrentRequests defaults correctly in TestNewConfigWithDefaults. Signed-off-by: Pavan Nalam <pavan.nalam3693@gmail.com>
…ription Signed-off-by: Pavan Nalam <pavan.nalam3693@gmail.com>
ntmspavan
changed the title
ntmspavan
changed the title
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
1 participant
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.
What this does
Each scrape currently fetches time series for every metric descriptor of
every configured project concurrently, with no limit (one goroutine +
HTTP request + JSON decode per descriptor, per project). When
google.projects.filter(or a longgoogle.project-idslist) resolves tomany projects, this can spawn far more concurrent Monitoring API requests
than a memory-constrained pod can service at once, and lead to OOM kills —
particularly in containers, where
GOMAXPROCSreports the host's vCPUsrather than the container's cgroup CPU limit, so far more goroutines get
scheduled than the pod's actual quota, and CPU throttling stalls many of
them mid-decode with their buffers unfreed.
This adds
monitoring.max-concurrency(default0, unbounded — nobehavior change for existing users) which caps concurrent
TimeSeries.Listrequests via a single semaphore shared across allresolved projects, so the limit holds regardless of how many projects are
matched.
Note: the semaphore intentionally only gates the per-descriptor
TimeSeries.Listcall, not the outerMetricDescriptors.Listcall — theouter call synchronously waits on the goroutines it spawns, so sharing one
pool between both sites risks a nested-semaphore deadlock once enough
prefixes are in flight to exhaust the pool with outer callers.
Testing
make test/go test ./...— all existing tests passcollectors/request_limiter_test.go, which spins up anhttptest.Serverand asserts concurrentTimeSeries.Listrequests neverexceed the configured limit
go vet ./...clean@SuperQ @kgeckhart could one of you take a look when you get a chance?