fix(content/oci): make the local OCI store survive a partial write - #1301
fix(content/oci): make the local OCI store survive a partial write#1301cwedgwood wants to merge 2 commits into
Conversation
58339e4 to
fd8e0a9
Compare
|
Suggestion applied and pushed — this is ready for another look.
One note in case it saves you a broken build: the inline suggestion still shows as applicable, but it is anchored to what is now the closing brace of |
dd38d3a to
a022b71
Compare
|
@TerryHowe — this has your suggestion applied and is rebased on |
|
I'm seeing some performance issues in testing, but I need to investigate the finding |
|
Confirmed, and thank you for catching it. I reproduced it, and it is worse than I would have The cause is that Measured on ext4 on an enterprise SSD (
The ratio gets worse on faster storage, because the baseline shrinks faster than the The two fsync calls are not equally worth their cost I would like to drop the directory The directory The one on the file is doing more work. Without it there is no guarantee that the contents reach The frequency may matter more than the per-call cost The overhead scales with how often
So a caller doing bulk work can already pay one That leaves 1.79x on the default path. My instinct is that the safe default is the right one here, Happy to push whichever shape you prefer. Worth knowing that the checks here have never run — they |
a022b71 to
ea7295b
Compare
`index.json` and `oci-layout` are written with `os.WriteFile`, which opens the file with `O_TRUNC`. The truncation succeeds even when the file system is full, so a write that fails part-way replaces a good file with an empty one, or with only the part of the content that was written before the failure. A store whose `index.json` has been emptied this way fails to open with "failed to decode index file: EOF" for as long as the file is there, and nothing rebuilds it, so a transient disk-full condition becomes permanent. Write both files to a temporary file in the same directory and rename that file over the target, which is what the package already does for blobs in `content/oci/storage.go`, and what `registry/remote/config` does for the configuration file. The content is flushed before the rename: with delayed allocation a write can be accepted against space that is never allocated, and the resulting failure is reported neither by the write nor by `Close`, so a rename without a flush can still publish content that never reached the disk. The directory entry is flushed afterwards on a best-effort basis, since by then the rename has taken effect and reporting a failure would describe an operation that did happen. Treat a zero-length `index.json` or `oci-layout` as a missing one and rewrite it. The store already recovers from a missing index, and an empty file carries no information and describes the same state. The recovery is deliberately limited to zero-length files: one that is malformed but not empty still fails, since discarding it would silently lose the tags of the store and hide a problem that is not a partial write. Recovering the index does lose the tags it held, which is documented on New. The permission that `os.WriteFile` produced is preserved. A file being created is created with 0666, so the umask applies to it as before, and the permission of an existing file is restored onto the temporary file before the rename. Replacing a file rather than writing through it differs from `os.WriteFile` in ways that are documented on the function. Signed-off-by: Chris Wedgwood <cw@f00f.org>
`ingest` names its return values so that the deferred cleanup can remove
the ingest file when a write fails, but its error paths return with
`return "", err`, which clears the named return `path` before the deferred
function runs. The cleanup therefore calls `os.Remove("")` and the ingest
file is left behind, so every failed push costs the disk space of the
content written before the failure. Set the named error and return, as
`ioutil.Ingest` has done since oras-project#1185.
Nothing reclaimed those files afterwards either. `GC` sweeps `blobs/` only,
so the residue of interrupted blob and metadata writes accumulates for the
life of the store, which makes the store a contributor to the full file
system that produced the residue in the first place. Remove them in `GC`,
where the other unreachable content of the store is already collected.
Only entries that have not been modified for an hour are reclaimed. A write
removes its own temporary file when it fails, so anything still present is
either the residue of a process that died mid-write or a write that another
`Store` on the same directory is performing right now, which the lock of
this `Store` does not serialize against. The modification time of a write in
progress keeps advancing, so a live write is never eligible while the
residue of a dead one always becomes eligible.
The two directories are otherwise treated differently on purpose. `ingest/`
is created and owned by this package, so any stale entry in it is residue.
The root of the store may hold files that belong to whoever put them there,
which the image layout specification permits, so only the names a temporary
file of this package can actually have are considered there.
Signed-off-by: Chris Wedgwood <cw@f00f.org>
|
Hi Terry — just checking back when you have a chance. We’ve rebased onto current |
ea7295b to
8212f10
Compare
2 participants
index.jsonandoci-layoutare written withos.WriteFile, which opens the fileO_TRUNC. The truncation succeeds even when the file system is full, so a write that failspart-way leaves the file empty — and
loadIndexFiletreats a zero-length file as a decodeerror rather than the missing-file state it already recovers from. The store then fails to
open for as long as the file is there, and nothing rebuilds it:
This is an edge case, but we have hit it several times in production. A node fills up,
index.jsonis left at 0 bytes, and because the cache directory outlives the container,restarting does not help.
blobs/is unaffected, since blobs already go throughingest-then-rename.
Commit 1 writes both metadata files to a temporary file in the same directory, flushes
it, and renames it over the target — the same thing
content/oci/storage.goalready doesfor blobs and
registry/remote/configdoes for the config file. The flush is load-bearing:with delayed allocation a write is accepted against space that is never allocated and
Closedoes not report it, so rename alone would not fix theENOSPCcase. A zero-lengthmetadata file is now treated as missing and rewritten; malformed-but-not-empty still fails,
since that may be real corruption whose tags are worth keeping for a human to look at.
Commit 2 fixes
ingestreturningreturn "", err, which clears the named returnpathbefore the deferred cleanup runs, so it calls
os.Remove("")and leaks the ingest file —the same bug #1185 fixed in
registry/remote/internal/ioutil.GCnow also reclaims thatresidue, which nothing did before. Happy to split this commit into its own PR.
Worth knowing, all documented on
writeFileAtomic: replacing a file rather than writingthrough it means symlinks and hard links are replaced, and a
0444or0000target isreplaced where
os.WriteFilereturnedEACCES, though its mode is preserved. Ownership,ACLs and xattrs are not carried over. Permissions are otherwise unchanged —
os.CreateTempignores the umask, so the temporary file is created with
O_EXCLat 0666, or at thetarget's own mode when replacing, and set exactly before the rename. The directory flush
after the rename is best-effort and returns nothing, since by then the rename has already
taken effect.
Five of the new tests fail against current
main, includingTestStore_EmptyIndexFilewiththe exact error above and
TestStorage_BadPush_NoIngestFileLeftBehindwithlen(ingest entries) = 1, want 0.make testpasses under both matrix Go versions (1.25and 1.26), coverage 85.3%, and
TestStore_BadIndexandTestStore_BadLayoutstill pass.Not in scope: the blob write is not flushed either, and its directory entry is not synced
after the rename. Both are worth fixing, but separately from this.
We still depend on the
v2line in a number of places, so we would value a backport if thisis accepted. The two commits cherry-pick onto
v2cleanly and the suite passes there; happyto open that PR whenever you want it.