storage: delete image metadata batched - #1107
Conversation
DeleteImage loop rewrites layers.json twice for each layer. This creates a rewrite storm that was debilitating our cluster. Instead mark every layer incomplete in one save and remove them all and save again. This is what load()'s loop already does. A 10-layer image rewrites layers.json twice instead of twenty times. On a 43k-layer store (our cluster numbers) removing a 20-layer image takes 2s instead of 16s. This also clears a partial failure behavior. Previously layers that are not referenced are unreachable by GarbageCollect and leaked permanently. This is also resolved. Signed-off-by: Jonathan Siegel <248302+usiegj00@users.noreply.github.com>
As in, timed out, or hanged completely? Hanging would be very unexpected.
Wait, are you saying that there was a single image with 40k layers? That’s impossible with overlay. Also this
seems to suggest that the 40k layers are not a part of a single deletion operation. (Also, (31.138-1.333)/(40*2-2) = 0,382 seconds for a write+ |
mtrmac
left a comment
mtrmac
left a comment
There was a problem hiding this comment.
Given that DeleteImage already holds a lock across the whole operation, this PR does not degrade interactivity any further; fine. So far I’m unconvinced that all of this is necessary, but the code complexity is small enough, and arguably writing the metadata only once is a nice optimization even if not strictly necessary.
But where I think this breaks is if we crash immediately after the first save in deferredDeleteMultiple. load will then try to delete all incomplete layers, but at that point we lost the parent/child ordering; and it might (well, given layers.json is written parent-first, probably will) try deleting parents before children. That’s breaks the semantic model of the driver, and might be impossible on snapshot-based drivers (and when deleting layers fails, load fails and the whole store is unusable). Fixing that to delete children first is not impossible but at that point I’d really want to have a very good reason for taking on that complexity.
Partial failure layers are now flagged incomplete and reaped on the next open.
what is that exactly?
The code comments are a very brief initial look, almost certainly not exhaustive.
| // Events are drained by a concurrent reader because inotify coalesces | ||
| // identical successive events that have not yet been read; counting after the | ||
| // fact would collapse every save into one. | ||
| func countLayersJSONReplacements(t *testing.T, path string, fn func()) int { |
There was a problem hiding this comment.
I think this is more intrusive / race-prone (and large, and hard-coding implementation details with separetly-implemented JSON parsers) than makes sense for a test.
There was a problem hiding this comment.
Replaced with a smaller test that asserts the recovery deletion order.
| return nil, fmt.Errorf("not allowed to delete layers at %q: %w", r.layerdir, ErrStoreIsReadOnly) | ||
| } | ||
| layers := make([]*Layer, 0, len(ids)) | ||
| var marked layerLocations |
There was a problem hiding this comment.
When internalDelete already does this marking, this needs a comment, otherwise someone will remove it to “optimize”.
| var deleted layerLocations | ||
| var firstErr error | ||
| for _, layer := range layers { | ||
| cf, err := r.internalDelete(layer.ID) |
There was a problem hiding this comment.
Really internalDelete should get a *Layer, not an id; every caller holds a valid layer.
Timed out after 600 seconds.
No. That was my bleary-eyed mistake. Layer counts per image (measured in our actually degraded environment) are median 11 layers / max 58. We had 43,222 layers in total in our observed node. I've a test harness to recreate the symptoms and it generates an 8MB JSON file (our cluster was 20MB). The timing for a test 58 layer image (116 writes of a ~8MB JSON file +
Yes. The 43,222 is the store-wide layer count not a single deletion. Deletion is per-image and the two multiply because each save rewrites the entire file so every save is O(store size). Also yes, the 382ms was on a VM used to recreate and it was backed with slow durable storage. On the affected bare metal node, at 44,000 records / 8.05 MB it's 47.7ms marshal + 16.5ms write+fsync+rename = 64.2ms per save (from actual bare metal cluster benchmark). |
You are right. I've added a test to reconstruct deletion order from
It's nine lines (push inbound) to sort the incomplete layers by
Sorry, that was my note. The project already handled layers in this way. I extracted that functionality into a helper and re-used it. |
Batching the incomplete-layer marks means load() can find a whole chain flagged at once, where before at most one layer was flagged at a time and recovery order did not matter. Deleting a parent that still has a child contradicts the driver's model and can fail on snapshot-based drivers, so sort by Created descending -- a child is always created after its parent, so newest-first is child-first. Wipe() orders bulk deletion the same way. Also from review: internalDelete now takes a *Layer rather than an id, since every caller already holds one; the pre-marking in deferredDeleteMultiple says why it is not redundant with internalDelete's own marking; and the save-count test is replaced by one asserting the recovery deletion order. Signed-off-by: Jonathan Siegel <248302+usiegj00@users.noreply.github.com>
Thanks, good idea. I was thinking we might need to do a topological sort. Given that, ACK to the general design. I still need to read the PR carefully. |
2 participants
Motivation
On a production cri-o 1.33.8 node, 208 days old with high container churn, the layer store had grown to 43,222 layers / ~20 MB of layers.json. In that state
crictl imagestimed out after 600 s andcrictl rmifailed with DeadlineExceeded after 5m10s.What is happening
DeleteImagedeletes an image's layers in a loop (deferredDelete), and each iteration rewrites layers.json twice: once to flag the layer incomplete, once after it is removed. An image with L layers therefore performs 2L rewrites, and each rewrite is O(store size) because the entire file is marshalled and replaced. The store-wide layer count and the per-image layer count multiply.Layer counts per image on the affected node were median 11, max 58 — the amplification does not need a deep image, only a large store.
What this changes
deferredDeleteMultipleflags every layer, saves once, deletes them, then saves once more: 2 saves instead of 2L. No public API changes.load()'s incomplete-layer recovery loop already batched deletions this way; this makesDeleteImageconsistent with it.Because a whole chain can now be flagged at once, recovery has to delete children before parents: removing a parent that still has a child contradicts the driver's model and can fail on snapshot-based drivers, and a failed deletion there fails
load(). The recovery loop therefore sorts byCreateddescending — a child is always created after its parent — asWipe()already does.This also follows #2325, which moved physical file removal outside global locks; this is the metadata half of the same concern.
Measured
AMD EPYC 7713, 256 cores, ext4 on LVM, vfs driver, as root, best-of-3. Benchmark and raw output: https://gist.github.com/usiegj00/8fdb6527e4d7b4216e6faa43eefc8b59
DeleteImagewall clock, 44,000-layer store:The first row matters: single-layer callers are unaffected, so the four other
deferredDeletecall sites keep their current behaviour.One metadata save at 44,000 records / 8.05 MB decomposes as 47.7 ms marshal + 16.5 ms write+fsync+rename = 64.2 ms. The cost is marshal-dominated, so it is CPU-bound serialisation of the whole record set rather than fsync latency.
Testing
A new test reproduces the state an interrupted deletion leaves behind — a whole chain flagged incomplete — and asserts that reopening the store deletes it children-first. It fails without the ordering fix. Full suite passes on vfs and overlay.
Signed-off-by: Jonathan Siegel 248302+usiegj00@users.noreply.github.com