fix(server): own the vuls2 db for the process instead of per request - #2628
fix(server): own the vuls2 db for the process instead of per request#2628MaineK00n wants to merge 1 commit into
Conversation
|
Do you have a built image with this I can use to test ? |
e0431f8 to
e84162c
Compare
|
I had a look at your That branch has this PR at the base with #2617 and #2618 rebased on top. All three rewrite This PR is meant to replace #2617 and #2618 rather than sit on top of them, so please build the branch on its own: I confirmed that from a fresh clone. I have also rebased the branch onto master, which incidentally drops the duplicate I don't have a published image to hand you — the upstream To set expectations: I haven't verified this against a real deployment yet — I don't have an environment that reproduces #2613/#2615. It builds and the unit tests pass, but that's all I can vouch for so far, which is exactly why your testing would help a lot. |
|
So, I tested your image. The database load at startup works as expected ! Hope it helps |
e84162c to
72a765c
Compare
|
You were right, and it was the cache. Thank you for testing — this was exactly the kind of report I couldn't have produced myself. I reproduced it locally against a real ~11 GB db with a 4873-CVE scan result. Almost all of it is in enrichment:
The cause was a design decision of mine. I had the process hold one open db handle that every request shared, and a shared session can't carry vuls2's read cache — the cache never evicts, so one that outlives a request grows until the process is OOM-killed. Without the cache, What I hadn't checked was what sharing the handle was buying. It turns out: nothing. Opening the db read-only is an mmap of a file the page cache already holds — ~50µs on the same ~11 GB db. I traded a 60x slowdown in enrichment for 50 microseconds. So the shared handle is gone. Two things worth flagging while you're set up:
Branch is force-pushed. Same build recipe as before: Would you be willing to give it one more run? |
…quest Server mode created a vuls2 db session per request, so every request decided for itself whether the db was due for a download and fetched it. A pod that came up with no db on disk therefore had each arriving request start its own multi-gigabyte fetch into the same directory, several at a time, each slow enough to trip the registry's stream limits and start over. Hoist the db's lifecycle out of the request path: - SharedDB downloads the db and keeps it current in the background, and hands each request a session with SkipUpdate forced on. Only the goroutine that prepares and refreshes fetches, so a fetch is single-flight and never blocks a request, and a request can never become another thing that pulls a db. /health reports 503 until there is a db to serve and /vuls refuses rather than fetching one of its own. Point a readiness probe at /health: a liveness probe would restart the process partway through the first fetch and start it over. - Each request opens its own handle and carries its own read cache. Sharing one open handle was tried and reverted: opening the db is an mmap of a file the page cache already holds, ~50us on a full-size ~11 GB db, while a session shared across requests cannot carry vuls2's read cache, since that cache never evicts and one outliving a request would grow until the process is OOM-killed. Going without it makes enrichment re-read and re-unmarshal the same advisory and vulnerability records once per root that references them: enriching a 4873-CVE result measured 2.4s with a cache and 150s without. - Startup adopts whatever usable db is already on disk before it considers downloading one, so a process that has a db serves right away rather than after a full fetch. A refresh that fails leaves the working db in place. - A refresh resolves the repository manifest and skips the download when its digest matches the one recorded in the local db. Going by timestamps alone, a nightly db past the staleness window looks due on every check for as long as it lives, which re-fetched gigabytes hourly even when the tag had not moved. - Record the db digest on the global config from SharedDB alone rather than from every session open. Concurrent requests wrote it while detector.DetectPkgCves read the same global to stamp the result, which was a data race. - Size detection workers by GOMAXPROCS rather than NumCPU, which reports the machine's CPU count even when a cgroup quota lets far fewer of them run. - Bound concurrent detections with -max-concurrency (default GOMAXPROCS). One detection holds every CVE it finds plus a read cache that measured 0.4-0.8 GB on heavy servers, so an unbounded number of them oversubscribes memory badly enough to stall the server. Requests queue on it rather than being rejected, and a slot is released only after the session it admitted has been closed. Refs #2613 Refs #2615 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
72a765c to
c67655a
Compare
2 participants
What did you implement:
Server mode created a vuls2 db session per request, so every request decided for itself whether the db was due for a download and fetched it. That is the root cause behind #2613: a pod that came up with no db on disk had each arriving request start its own multi-gigabyte fetch into the same directory, several at a time, all competing for bandwidth and disk until they tripped the registry's stream limits and started over.
This PR hoists the db's lifecycle out of the request path, while leaving the db handle itself per-request.
SharedDBowns fetching and readinessSharedDBdownloads the db and keeps it current in the background, and hands each request a session withSkipUpdateforced on. Only the goroutine that prepares and refreshes ever fetches, so a fetch is single-flight by construction and a request can never become another thing that pulls a db. Until there is a db to serve,/healthanswers 503 and/vulsrefuses.Important
Point a readiness probe at
/health. A liveness probe would restart the process partway through the first fetch and start the download over from nothing.Each request keeps its own handle and its own read cache
An earlier revision of this PR shared one open db handle across requests. That was wrong, and @maxenced caught it by testing a build — detection went from seconds to over 300s. Measured on a full-size (~11 GB) db with a 4873-CVE result:
The reason is that a session shared across requests cannot carry vuls2's read cache: the cache never evicts, so one outliving a request grows until the process is OOM-killed. Without it,
GetVulnerabilityDatare-reads and re-unmarshals the same advisory and vulnerability records once per root that references them, and enrichment walks every CVE.Sharing the handle bought nothing to pay for that. Opening the db read-only is an mmap of a file the page cache already holds — ~50µs on the same ~11 GB db, next to nothing beside the seconds a detection takes, and the pages are shared by the OS either way. So each request opens its own handle, carries its own cache, and drops both when it ends.
SharedDBshrank accordingly: no generations, no refcounting, no hot-swap.Startup serves from disk before considering a download
A db on disk that is merely due for a refresh is still worth serving, so a process that has one comes up in the time it takes to check a file instead of after a full fetch. Bringing it up to date is then the refresher's job, with requests served the whole time. A refresh that fails leaves the working db in place.
Refreshes compare the repository digest first
shouldDownloadgoes by timestamps, and the nightly db'sLastModifiedis the night it was built — so a db past the staleness window looks due on every check for as long as it lives, which would re-fetch gigabytes hourly.hasNewerRemoteresolves the repository manifest and compares its digest against the onefetchrecorded in the local db's metadata, so a download only happens when the tag has actually moved.Three adjacent fixes
SharedDBalone rather than by every session open. Concurrent requests wroteconfig.Conf.Vuls2.Digestwhiledetector.DetectPkgCvesread the same global to stamp the result — a data race that exists on master today.GOMAXPROCSrather thanruntime.NumCPU(), which reports the machine's CPU count even when a cgroup quota lets far fewer of them run. On a 2-vCPU pod scheduled on an 8-core host, every request was spawning 8 workers for CPUs it would never get.-max-concurrency(defaultGOMAXPROCS) bounds concurrent detections. One detection holds every CVE it finds plus a read cache that measured 0.4–0.8 GB on heavy servers (4873–7466 CVEs), so this is what puts a ceiling on the process: peak ≈max-concurrency× that. Master has no such bound at all. Requests queue rather than being rejected — clients with tight HTTP timeouts may see a timeout where they previously saw a slow response — and a slot is released only after the session it admitted has been closed, so the cache is freed before the next request is let in.Relation to #2617 / #2618
@maxenced opened #2617 and #2618 against discussion 2613, and the analysis in both is theirs — including the shape of the startup fix (fetch at startup, keep the server out of rotation until it completes). This PR arrives at the same behaviour for
/healthand the same single-flight guarantee.Where it differs is scope: #2617/#2618 keep the per-request fetch decision and coordinate it, while this PR moves the decision out of the request path entirely, which is also what makes the digest precheck and the concurrency bound natural to add.
@maxenced also tested a build of the earlier revision and found the regression described above, which is why the shared-handle design is gone. Thank you — that was the useful kind of review.
Type of change
Behaviour changes
/healthand/vuls503, one fetch, retried every minute-max-concurrency, queuedNumCPU(ignores cgroup quota)GOMAXPROCS/health200 ok503until there is a db to serveHow Has This Been Tested?
make fmt,go build ./...,go vet ./...go test -race ./detector/...New tests in
detector/vuls2/shared_test.go(all run under-race):TestSharedDB_NotReadyBeforeOpen— a db on disk is not one this process has validatedTestSharedDB_OpenLocal/TestSharedDB_Prepare— a stale db (SkipUpdate: false,LastModifiedold enough for a refresh to be due) is adopted without reaching the registry, which is what proves the staleness rule no longer blocks startupTestSharedDB_OpenLocalWithoutDB— refuses rather than downloadingTestSharedDB_AcquireCannotFetch— the guarantee behind 2613: whatever the configured policy, a request's session is barred from fetching, and still serves the stale dbTestSharedDB_AcquireCarriesCache— the regression test for what this revision fixesTestSharedDB_AcquireIsIndependent— one request closing does not take the db from another, and does not leak its own handleTestSharedDB_PrepareCancelled— gives up on ctx rather than blocking a shutdownTestSharedDB_Reload— a reload with nothing due is a no-opTestSharedDB_ConcurrentAcquire— 8 concurrent requests opening/reading/closing their own handles against 4 readers ofReadyTest_hasNewerRemoteindetector/vuls2/db_test.gocovers the branches reachable offline (no digest recorded, no db file, unparsable repository). The digest-match path needs a registry and is not covered.The timing and memory figures above were measured against a real
ghcr.io/vulsio/vuls-nightly-dbdb with real scan results, not fixtures. Still not verified against a real deployment — I don't have an environment that reproduces the discussion's symptoms at scale.Checklist:
/healthmust be a readiness probe, and-max-concurrencyqueues rather than rejects and is what bounds memory. Not written yet.make fmtmake testKnown gaps
Neither this PR nor #2617/#2618 addresses the fetch failure itself. In discussion 2613 the download dies at ~5 GB with
stream error: PROTOCOL_ERROR; received from peer, andfetch.Fetchreads the layer as one stream with no retry and no ranged resume, so it starts over from zero every time. Fixing that belongs inMaineK00n/vuls2, along with the same digest precheck sovuls2 db fetchbenefits too.Until then, the reliable deployment is
SkipUpdate = truewith the db provided by an initContainer or a PVC — with this PR that path also gives the fastest startup.Discussion 2615 asks for the per-request bolt opens to go away. They do not, and the measurement above is why: the open is ~50µs, and removing it costs the read cache, which is worth far more. What that discussion was feeling is most likely the unbounded concurrency, which
-max-concurrencynow bounds.Is this ready for review?: NO