Repository navigation
Conversation
|
Made the pool configurable up front rather than asking, since the same question came up on #7838 and the answer there applies unchanged: how much concurrency a forge will take is an operator's knowledge, not something CDS can see.
Same treatment as The pool is swapped wholesale rather than mutated, so a request already choosing a lane cannot observe a half-built one. One thing I did decide and would like your view on: the knobs sit at the VCS service level ( Question 3 in #7847 is struck through accordingly; the other three still stand. |
fed6f09 to
e756153
Compare
A provider client is built per request -- every handler in engine/vcs calls GetAuthorizedClient -- and those clients carry a nil Transport, so all of them share http.DefaultTransport. Against an HTTP/2 forge, Go's h2 client keeps one connection per host and multiplexes onto it, so a repository analysis reading its entity files concurrently serialises behind a single connection's flow control however wide the fan-out is. Measured on one instance: netstat inside a live pod during a three-repository analysis burst showed exactly one ESTABLISHED connection to the forge, against a peak of 760 in-flight requests in a ten-minute window. A single curl to the same endpoint from that pod, with the same basic auth and the same commit ref, returns in 60ms; p50 under load was 1237ms (n=637, p90 2295ms, max 4422ms) with cds-vcs at 5m of CPU and no throttling. The transports now live in a package that outlives a request, since the per-request part is the credentials rather than the connection pool, and several of them are kept so that concurrent requests get real concurrency. MaxConnsPerHost does not do this: it bounds how many connections a cold dial race may produce, not whether a warm, healthy connection gets siblings. Forcing HTTP/1.1 does, but only by giving up multiplexing and header compression. Two tests, both mutation-checked. The rotation is asserted directly. The reason for it is asserted against a real HTTP/2 httptest server: one transport answers a warmed twenty-four way burst on one connection, the pool answers it on several. The first of those two assertions is deliberately written as a claim about Go rather than about this package -- if it ever stops holding, this package has no reason to exist and the test should say so. Only the Forgejo provider is wired here. See the RFC issue. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Gregoire Salingue <gregoire.salingue@brefwiz.com>
Same per-request client construction as Forgejo, so the same one
connection carries the whole fan-out. The Gitea SDK is also constructed
with http.Client{} and nothing else: no transport, and no timeout at
all, so a forge that stops answering hangs the handler until something
else gives up.
Not fixed here, deliberately: gg.NewClient probes /api/v1/version
behind a sync.Once held on the client instance, and CDS builds a client
per request, so every Gitea VCS call costs an extra round trip to the
forge before the real one -- verified against the vendored SDK, ten API
calls produce ten /version probes. Both ways to remove it trade
something. SetGiteaVersion("") drops the SDK's >= 1.11 guard; caching
the client per server keeps it but changes the lifetime of an
authenticated object. That is a decision for maintainers, so it is in
the RFC issue rather than in this diff.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Gregoire Salingue <gregoire.salingue@brefwiz.com>
The right number of connections to a forge is a property of the forge and of whatever fronts it, neither of which CDS can see: a hosted forge may rate limit simultaneous requests, a self-hosted one behind a single reverse proxy has a different ceiling again. That is an operator's knowledge, so it belongs in configuration rather than in a constant here. vcs.forge.connectionPoolSize (default 8) and vcs.forge.maxIdleConnsPerHost (default 32), applied in ApplyConfiguration before any handler can build a provider client. The defaults are applied where the values are read, not only where they are configured. A pool sized straight from an absent value would hold no transports at all, and the first forge request of a service that had simply not set the knob would panic on an empty slice. An unset value means the default; it does not mean none. The pool is swapped wholesale rather than mutated, so a request already choosing a lane cannot observe a half-built one. Test covers both halves and is mutation-checked: the configured size is the one used and the rotation follows it, and an unset or negative value falls back to the default rather than to an empty pool. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Gregoire Salingue <gregoire.salingue@brefwiz.com>
e756153 to
d776803
Compare
|
Streams on an h2 connection are multiplexed, and the server announces how many it accepts per connection: 250 for a Go server such as Forgejo. Below that limit, the forge receives every request at the same time on that one connection, and handles each stream in its own goroutine. Above it, the default Go client opens a second connection on its own. So "serialises behind a single connection" only holds at the TCP level (bandwidth, retransmission), which for small YAML files should not be visible, and the 5m CPU on cds-vcs says the client read loop is not the bottleneck either. Checked on Go 1.26 with a plain cloned Your netstat showing exactly one established connection therefore means the forge's stream limit was never reached, so every in-flight request was already reaching the forge concurrently. |
Proposed for discussion rather than merge as-is — the open questions are in #7847.
A VCS provider client is built per request: every handler in
engine/vcscallsGetAuthorizedClient. Those clients carry a nilTransport, so all of them sharehttp.DefaultTransport, and against an HTTP/2 forge Go's h2 client keeps one connection per host and multiplexes onto it. A repository analysis reading its entity files concurrently therefore serialises behind a single connection however wide the fan-out is.Measured on our instance, Forgejo forge, repositories carrying up to ~130
.cdsentity files:This adds
engine/vcs/transport, holding a small pool of transports that outlive a request, handed out in rotation — the per-request part of a client is its credentials, not its connection pool.Four things that look like the fix and are not, since they will come up in review:
MaxIdleConnsPerHost(2 onDefaultTransport) matters for an HTTP/1.1-only forge and not once h2 is negotiated; giving each client its ownTransportis strictly worse, because the client is per request so nothing is pooled at all;MaxConnsPerHostbounds how many connections a cold dial race may produce, not whether a warm healthy connection gets siblings; forcing HTTP/1.1 does work, by giving up multiplexing and header compression.The second commit wires the Gitea provider the same way and gives it a timeout — the SDK builds
&http.Client{}with neither. It deliberately does not remove that provider's/api/v1/versionprobe, which fires on every request because the SDK'ssync.Oncelives on a client CDS rebuilds each time. Both ways to remove it trade something away; the choice is in #7847.github,gitlab,bitbucketcloudandbitbucketserverhave the same per-request construction and are untouched: we have measured only Forgejo, and did not want to change six providers on one instance's evidence.Part of #7847
Explains the p50 discussed in #7834
Two in
engine/vcs/transport/transport_test.go, both mutation-checked against the change they cover:TestPooledRotates— the pool must hand out every transport it holds. Pinned directly, because without the rotation the pool is decoration: every request would go through one transport, which is the state this package exists to leave. Returningpool[0]turns it red.TestPooledOpensSeveralConnectionsToAnHTTP2Host— the reason the package exists, against a real HTTP/2httptestserver counting distinct connections under a warmed twenty-four way burst. One transport answers it on one connection; the pool answers it on several.The lanes are warmed before the burst on purpose: a transport dialling a cold host does open one connection per concurrent request, but that is a dial race rather than steady state, and a long-running service is never in it. Without warming the test would pass for the wrong reason.
The first assertion is deliberately written as a claim about Go rather than about this package. If it ever stops holding, this package has no reason to exist, and the test should be the thing that says so.
go build ./engine/...andgo test ./engine/vcs/transport/pass.go vet ./engine/vcs/...reports one pre-existing unkeyed-fields warning ingitea/client_branch.go, unchanged by this PR.@ovh/cds