Repository navigation
fix(metrics): bound the instance metrics Collect call - #1177
Open
abd-ulbasit wants to merge 1 commit into
Open
abd-ulbasit wants to merge 1 commit into
abd-ulbasit wants to merge 1 commit into
Conversation
The instance manager calls the plugin Collect RPC while it serves the /metrics endpoint of the instance, and it does not set a deadline. If Collect never returns, the whole endpoint hangs, not only the plugin metrics. Give Collect its own 5 second deadline, so it returns an error instead. The instance manager then logs the error, counts it in cnpg_collector_collection_errors_total, and still serves the other metrics. The deadline alone is not enough. The caching client held its lock for the whole request to the API server, and a mutex does not honour a context. One slow Secret or ObjectStore read blocked every other cached read, including the one in Collect. Release the lock while the request is in flight. Two lookups of the same object can now run at the same time. Stamp each cache entry with the time its request was sent, and do not replace an entry that was fetched later, so a slow request cannot overwrite fresher data or keep old data past the TTL. Refs cloudnative-pg#1045 Assisted-by: Claude Opus 5.5 Signed-off-by: abd-ulbasit <basit@basit.engineer>
This branch has not been deployed
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.
Refs #1045
This change puts a deadline on plugin metrics collection and removes the global client lock from API-server requests.
What changed
Collectnow has its own 5-second deadline. If the client doesn't respond in time, collection returnscontext.DeadlineExceeded. The instance manager can record the collection error and continue serving the remaining metrics.ExtendedClientno longer holds its mutex while waiting for the API server. The cache is checked under the lock, the request runs without it, and the result is written back under the lock.Why
A single slow API-server read shouldn't be able to stall the plugin's metrics indefinitely or block unrelated cached reads. The goal is to limit the impact to the plugin's own metrics for that scrape, rather than holding up the instance's entire
/metricsendpoint.Scope
This PR addresses the plugin-side collection and cache behavior only. It doesn't add a deadline to the core
pluginCollector.Collectcall; that belongs incloudnative-pg/cloudnative-pg. It also doesn't add a retention-cycle timeout, which needsexec.CommandContextsupport in barman-cloud first (#1044).Cache behavior
Allowing concurrent cache misses means two requests for the same entry can reach the API server at the same time. Each entry is timestamped using the time its request was sent, and an older request completing late must not overwrite a newer cache entry. Entries also remain subject to the existing 10-second TTL measured from the request's start time.
Tests
CollectreturnsDeadlineExceededwhen the client never responds.Getisn't blocked by anotherGetstuck on the API server.go test(non-e2e): passed.go test -raceoninternal/cnpgi/instance/...: passed.golangci-lintv2.15.0: passed.Disclosure: parts of this change and its tests were written with Claude Code (Claude Opus 5.5), as noted in the
Assisted-bycommit trailer.