Repository navigation
imagecache: Pin a pull's layers so eviction cannot retire them mid-pull - #1509
Conversation
|
Can you clean up the PR description? It's very difficult to follow. |
8c113ee to
db9b3aa
Compare
bad54ac to
9618ddd
Compare
177a278 to
7d468f0
Compare
7d468f0 to
f759658
Compare
ba7a5da to
82b9951
Compare
Benjamin Elder (BenTheElder)
left a comment
There was a problem hiding this comment.
AI assisted.
With my human eyeballs: the pinMu seems like it could be improved, but I'd rather deflake CI and follow-up.
82b9951 to
8a851d3
Compare
|
Rebased onto main with the nits addressed. #2279 is stacked on this and rebased too. |
8a851d3 to
979f917
Compare
|
igooch Good morning, can you please make sure this PR gets merged today? Thanks! |
ensureLayer and retireLayer shared one singleflight key per layer, and that sharing was the retire/reuse interlock: reuse refreshed the layer's mtime inside the flight, retirement re-checked it inside the same flight. But a caller whose Do joins a flight rather than leading one runs no closure and learns nothing about what it joined. When the flight was a retirement, ensureLayer returned the layer path anyway, which the rename had just moved aside. The pull recorded a layer with nothing behind it, and its final re-verify failed a healthy pull with "layer dir vanished during pull". The window is microseconds wide, so it surfaced only as a rare failure under load. Protect a pull's layers the way a cache shrinker protects objects in use, with a count of current users rather than a lock. A pull pins each of its layers before any layer work and unpins when it returns. retireLayer checks the pin and the mtime and renames under the same mutex, and never retires a pinned layer: either the pin lands first and the retirement is vetoed, or the rename lands first and the pull finds the dir missing and unpacks it again. Retirement no longer touches the layer singleflight, so every flight a pull can join is another pull's ensure, whose result it can trust. The pin map holds only layers in use by in-flight pulls and drops entries at zero. The pin is taken per pull, outside the flight, so a pull that joins another's flight is covered by its own pin once the leader's lapses. This changes what happens to a wedged pull. Its record still ages out after min-age, but its finished layers now stay until the pull returns, instead of being retired while the download is still running and failing the pull at its re-verify.
Closes the gaps left when pins replaced the shared layer flight. pull's re-check under the image flight returned layer dirs through cachedImage, without touching the record, so that one path handed out dirs with neither a pin nor the min-age cover every other hit gets. It now goes through cachedImageHit like the hit path. retireLayer logged under pinMu, the lock every pull takes to pin, so a slow log handler stalled every pull on the node. The critical section now does only the pin check, a re-stat and the rename, and the caller logs after it returns. dryRunRetire re-implemented the stat and cutoff rule and checked pins before existence, so for a pinned layer that had not landed yet the dry run kept a record the real pass evicted. It now calls retirable in the same order retireLayer does. A pull took pinMu once per layer; pinLayers pins the whole set under one acquisition. The unpin's idempotence guard had no caller and is gone. TestPullPinsLayersAgainstEviction waited for the record mtime to reach the layer dir's, which coarse filesystem timestamps satisfy before the pull's touch, so the backdate could race the touch and skip the record as fresh. It now backdates and runs the passes until the record reaches retirement; the touch happens once, so a pass it refreshed changed nothing.
78582a9 to
7fe236c
Compare
Benjamin Elder (BenTheElder)
left a comment
There was a problem hiding this comment.
Sorry for the review delay, lot of PRs in flight and this package has gotten pretty convoluted in main, whee agents :^)
I think we could merge this now, but the nit on retireLayer might be worth fixing first.
A pull pins every layer before any of them is unpacked, so a pinned layer with no dir under its final name is one still unpacking, not one already removed. retireLayer reported it gone, so while every layer only a record referenced was still downloading, an eviction pass deleted that record, and if the pull then failed the layers it had finished had no record until the next pull of that image or the startup orphan sweep. Such a layer is now vetoed in retireLayer and dryRunRetire, which keeps the record for the pull the same way a landed pinned layer does. The pin check for a missing layer reads the map under pinMu; the fresh-layer fast path still stays off the lock.
| if st, mtime, err := retirable(dir, cutoff); st != retireRetired { | ||
| if st == retireGone && s.pinned(hex) { | ||
| return "", retireVetoed, nil | ||
| } | ||
| if st == retireVetoed && err == nil { | ||
| slog.Info(logMsgLayerRetireVetoed, slog.String("diffid", hex), slog.Time("last_used", mtime)) | ||
| } | ||
| return "", st, err | ||
| } |
There was a problem hiding this comment.
🤖 nit 🟢 – My earlier suggestion left a narrow gap here. If the layer lands and the pull then fails on another layer and unpins, all between the stat and pinned(), this returns gone for a layer that is now on disk with no record. dryRunRetire checks the pin first, so it doesn't have this gap.
Letting a missing layer fall through to renameAsideUnlessPinned closes it, since that checks the pin and then re-stats under pinMu. The veto then logs like a landed pinned layer does, and pinned() goes back to dry-run only, as its comment says. The window is tiny, so this can be a follow-up.
| if st, mtime, err := retirable(dir, cutoff); st != retireRetired { | |
| if st == retireGone && s.pinned(hex) { | |
| return "", retireVetoed, nil | |
| } | |
| if st == retireVetoed && err == nil { | |
| slog.Info(logMsgLayerRetireVetoed, slog.String("diffid", hex), slog.Time("last_used", mtime)) | |
| } | |
| return "", st, err | |
| } | |
| if st, mtime, err := retirable(dir, cutoff); st == retireVetoed { | |
| if err == nil { | |
| slog.Info(logMsgLayerRetireVetoed, slog.String("diffid", hex), slog.Time("last_used", mtime)) | |
| } | |
| return "", st, err | |
| } |
There was a problem hiding this comment.
[I'm choosing to take this as a follow-up, because narrowing the race is desirable to keep CI moving]
…r a failed pull (agent-substrate#2989) Follow-up to agent-substrate#1509 (comment). `retireLayer` stats a layer before checking its pin. If the layer lands, and its pull then fails and unpins in between, eviction reports the layer gone and drops the image record, so the unpacked layer has no record until the image is pulled again. A missing layer now goes through `renameAsideUnlessPinned`, which checks the pin and stats again under `pinMu`. `dryRunRetire` already checks the pin first. There is no new test, since the window can't be hit without a hook between the two checks. `TestRetireLayerStatuses` still covers the missing and missing+pinned cases. AI use: AI-assisted, I reviewed the change. - [x] Tests pass
Fixes #1076.
The bug
ensureLayerandretireLayershared one singleflight key per layer, and that shared flight was the retire/reuse interlock. A caller that joins a flight rather than leading it runs no code and learns nothing about what the flight did. When the flight was a retirement,ensureLayerreturned the layer path anyway, just after the rename had moved it aside, and the pull failed its final check withlayer dir vanished during pull. The window is microseconds wide, so it shows up as a rare CI flake.The fix
Count a pull's in-flight use of each layer instead of using the flight as a lock.
retireLayernever retires a pinned layer. The pin check, a re-stat and the rename run under one mutex, so either the pin lands first and retirement is vetoed, or the rename lands first and the pull unpacks the layer again. A missing or fresh layer is ruled out before the mutex is taken.Tradeoff
A wedged pull holds its layers until it returns, bounded by the pull timeout. Under disk pressure that holds space longer than before. In exchange, a slow pull can no longer lose layers it already finished.