tmpfs: stream file contents when writing a tar archive - #14222
Conversation
|
Thanks for submitting this! Have you looked at https://gvisor.dev/docs/user_guide/fs_snapshot/? That API already solves this issue by streaming pages to disk. In general is the more preferred and better supported API. We also support the GCS streaming option to restore such filesystem snapshots. Or is the TAR file output really useful for you? |
Hey @ayushr2! Thanks for the quick reply man. For my specific use-case here tar output is a better fit than fs checkpoints, mainly as I'd like the end result to be an OCI-spec image, but the current lock per gvisor version is the larger issue for me. I'm guessing that's mostly because |
|
@konstantin-s-bogom actually worked on making FS checkpoints backwards compatible in 8bd698e. So it should be backwards compatible after that change. We just don't have tests for backwards compatibility right now. But I hear you, the TAR usecase is legit too. I will try to review this soon. Folks at Modal also independently his this same OOM issue while using TAR upper-layer tool. |
|
@ayushr2 Honestly really good to hear they're now backwards compatible, I didn't realize that work had been in flight. I can look into possibly switching the way I'm thinking about snapshotting the FS, but currently the tar use-case is important for me here outside of the typical FS snapshot path (and I assume others at large but maybe this is more niche than anticipated?) as a more persistent / OCI compliant storage mechanism |
|
Hey @ayushr2 - will be maintaining a fork here for a little, but wanted to know if you all had any updates here on bandwidth to look at this ? No rush here. |
|
I will look at this today. I no longer work at Google, so don't have "merge this PR capability" but I am definitely happy to review this. Modal (my current company) also really wants this. |
| defer putRegularFileReadWriter(rw) | ||
| // Copy rather than read the file into one buffer, so that the copy does not | ||
| // allocate in proportion to the size of the file. | ||
| n, err := io.Copy(tw, io.LimitReader(safemem.ToIOReader{Reader: rw}, size)) |
There was a problem hiding this comment.
I suspect io.LimitReader is redundant, since ReadToBlocks already caps at the file size and the size can't change under inode.mu.
| for i := range content { | ||
| content[i] = byte(i % 251) | ||
| } |
There was a problem hiding this comment.
We never verify content. Fill with random data instead.
There was a problem hiding this comment.
switched to seeded math/rand/v2 for the file contents, and the round-trip test now compares the full bytes that come back out of the archive against what went in
| vfsObj := &vfs.VirtualFilesystem{} | ||
| if err := vfsObj.Init(ctx); err != nil { | ||
| t.Fatalf("VFS init: %v", err) | ||
| } | ||
| vfsObj.MustRegisterFilesystemType("tmpfs", FilesystemType{}, &vfs.RegisterFilesystemTypeOptions{ | ||
| AllowUserMount: true, | ||
| }) | ||
| mntns, err := vfsObj.NewMountNamespace(ctx, creds, "", "tmpfs", &vfs.MountOptions{ | ||
| GetFilesystemOptions: vfs.GetFilesystemOptions{ | ||
| InternalData: FilesystemOpts{ | ||
| SourceTar: io.NopCloser(&buf), | ||
| }, | ||
| }, | ||
| }, nil) | ||
| if err != nil { | ||
| t.Fatalf("NewMountNamespace: %v", err) | ||
| } | ||
| defer mntns.DecRef(ctx) | ||
|
|
||
| root := mntns.Root(ctx) | ||
| defer root.DecRef(ctx) | ||
| fs, ok := root.Mount().Filesystem().Impl().(*filesystem) | ||
| if !ok { | ||
| t.Fatalf("root filesystem is %T, want *tmpfs.filesystem", root.Mount().Filesystem().Impl()) | ||
| } |
There was a problem hiding this comment.
A lot of duplicate code here with other 2 tests. Please extract mountFromTarTestOnly(t, ctx, src) *filesystem (with t.Cleanup for the DecRefs) and a tarWithFile(name, size) helper.
There was a problem hiding this comment.
Now there's a mountFromTarTestOnly(t, ctx, src) with the DecRefs going through t.Cleanup. tarWithFile(t, name, content) that sits on top of a small tarWithEntries helper, and readFileFromTar pulls a single entry back out
The symlink reg test uses the same helpers, so its explicit DecRef moved into that cleanup as well
| if allocated > fileSize/2 { | ||
| t.Errorf("archiving a %d byte file allocated %d bytes, want at most %d", fileSize, allocated, fileSize/2) | ||
| } |
There was a problem hiding this comment.
Probably should tighten this. Maybe assert an absolute bound of 1 MiB and reduce the file size to 4 MiB.
There was a problem hiding this comment.
Changed this - now a 4 MiB file, absolute bound of 1 MiB
I also split the measurement so the restore and the archive write are each checked on their own. Ideally, a regression on either side would be obv from the failure msg
For ref, against master the restore side allocates ~8.4 MiB for that file (that's bytes.Buffer doubling) and the archive side ~4.2 MiB. w/ this change it's ~56 KiB and ~34 KiB
|
|
||
| // TestTarUpperLayerLargeFile checks that a file larger than the buffer it is | ||
| // copied through is written to the archive intact. | ||
| func TestTarUpperLayerLargeFile(t *testing.T) { |
There was a problem hiding this comment.
This test should run over multiple sizes. io.Copy uses a 32KiB copy buffer. So you could straddle the file sizes across that -- {0, 1, 32<<10 - 1, 32<<10, 32<<10 + 1, 4<<20 + 1234}.
There was a problem hiding this comment.
Now do 0 through 4M+1234 as subtests to straddle the 32K copy buf (each w/ random contents)
There was a problem hiding this comment.
This only fixes the OOM issue on the checkpoint writing side. But on restore, we have the same bug. tarDefaultReaderCallbacks.regularFileRead copies into bytes.Buffer per file. Then this is later copied into file via mknodFromTar() => regularFileSetContents(). Restoring a rootfs tar therefore allocates the sum of all regular file sizes in the sentry. And also whatever extra capacity bytes.Buffer needs since it doubles its size. We need to fix that too.
There was a problem hiding this comment.
restore side streams now too - added a basically identical writer function to mirror the safemem.ToIOReader func, and added a unit test there
There's a little bit of complexity here, in that the tar stream is sequential, so the content of the stream has to be consumed while the reader is sitting on that entry
What I did is create the regular file inode so it's detached w/ no parent yet, and then copy the entry straight into it through a new safemem.ToIOWriter. After that, we can just have mknodFromTar attach the pre-created inode once all the directories exist, which has the same ordering as before
One simplification we get out of this is we can then collapse th reader callback interface to just 1 regularFileRead(ctx, hdr, tr, rf)
Updated the allocation test so that it covers the restore path now, with that same 1 MiB bound on a 4 MiB file as we talked about above
3a0fb82 to
3acd8f3
Compare
|
Hey @ayushr2 - thanks for getting back to me here. Made some changes here based on your comments + implemented this on the restore side as well (and rebased off main to fix some failing PR checks). If you know anyone still on the gvisor team I'd love to get this in front of them :) Appreciate it! |
3acd8f3 to
387e9b1
Compare
387e9b1 to
88b8a25
Compare
Summary
Hey team - running into some issues when committing a gVisor upper layer. Seems like we read each file into a single alloc of the file's full size in
regularFileWrite:This is on default settings - my config doesn't pass
--overlay2, so as I understand it the overlay isroot:selfand the upper layer lives in a filestore on the host. That leaves the buffer above as the only anonymous alloc, and because the sentry runs inside the container's memory cgroup, archiving a file near that cgroup's limit OOM-kills it.Fs.TarRootfsUpperLayerthen fails withurpc method "Fs.TarRootfsUpperLayer" failed: EOFand the container is gone, which also means we lose the FS.I tested this against a gVisor container with a 512Mi memory limit running
runsc tar rootfs-upper. In this case, a 500 MiB file was successfully archived, while (and these are relatively arbitrary but just larger sizes) 800 MiB, 1000 MiB and 1536 MiB files each kill the container in about 20 seconds.The node reports
Memory cgroup out of memory: Killed process (exe) ... anon-rss:570584kB. If I bump the gVisor container to 2Gi, that same 1000 MiB file archives fine. We also avoid this entirely if we have a bunch of smaller files add up to an arbitrary size, as long as each file doesn't exceed the limit of the cgroup.Made a change here which copies the file into the archive through
safemem.ToIOReaderinstead of allocating the file's full size, so the mem an archive needs is no longer directly dependent on the size of the files in it.I did have Claude help me find where this logic is in gVisor, but have tested this independently and verified the logic myself. It could also be the case I'm missing something here, just let me know.
Thanks y'all!
Test plan
TestTarUpperLayerLargeFileAllocationasserts that archiving a 32 MiB file allocates less than half its size. On the current implementation it reports 33556016 bytes and fails; with this change it reports 34368 bytes.TestTarUpperLayerLargeFileround-trips a file larger than the copy buffer, and not a multiple of it, to check the contents survive.bazel test //pkg/sentry/fsimpl/tmpfs/...Assisted-by: Claude Code