Hold the NBD export to what it tells a client, and let it keep its blocks somewhere other than a file - #895
Open
MayCXC wants to merge 12 commits into
Open
Hold the NBD export to what it tells a client, and let it keep its blocks somewhere other than a file#895MayCXC wants to merge 12 commits into
MayCXC wants to merge 12 commits into
Conversation
The reaper that runs at a concurrency of one deletes everything in the test directory except the unpacked rootfs it means to preserve. It held that rootfs path as FileManager reports it, under /var, and compared it against the entries of contentsOfDirectory, which reports them under /private/var, so the preserved path never matched any entry and the rootfs was deleted along with the per-test files. The unpack coordinator still held it as unpacked, so the next test opened a rootfs that was no longer there and failed with a missing file error. Resolve both sides with resolvingSymlinksInPathWithPrivate, which exists for this difference between the two views of the same directory.
Tests guarded for Linux are compiled out on macOS, so `make test` reports success without having run them, and nothing says so. The target that does run them is not mentioned anywhere outside the makefile.
The default kernel the tests fetch had drifted from the one users run. container installs kata 3.28.0 and its 6.18.15 kernel, while these tests fetched 3.17.0, so the suite exercised a guest with a different feature set to the one it is meant to represent. Nested runtimes are the visible case: 3.17.0 was built without nf_tables, so a docker daemon inside a container fails there and works on what ships. Kata moved from xz to zstd between those releases, so the archive is no longer named for its compression and tar is left to recognise it rather than being told, which also holds if the format changes again.
A test's rootfs clones, init block, and writable layers land in a directory the runner creates for it and deletes when it finishes, so a test holds disk only while it runs at any concurrency. The run-shared directory holds the unpacked images every bootstrap clones from, and is removed when the run ends.
A container's storage is its rootfs, an optional writable layer, and its mounts; a machine's is its containers' plus the volumes they share. One generic shape describes both the Mount values a machine is configured with and the AttachedFilesystem values it reports once attached, so converting between the two is a map over the structure and the roles cannot drift between configuration and attachment. Device addresses are allocated walking the same sorted order the devices are created in, on both backends. Consumers read roles instead of list positions: the spec builders take a container's mounts without prefix arithmetic, pod volumes mount by name, and a cloud-hypervisor volume's virtiofsd is held by a machine-lifetime reference alongside its containers' reference counts.
A container can be given a swap area, so a workload whose memory exceeds its limit is reclaimed rather than killed. The area is a raw file on the host attached as a block device, which is how a guest gets swap it can write to when its own root is read only, and vminitd formats and enables it while mounting the container's filesystems. The area is enabled with discard, and the device backing it is marked non rotational first. virtio block devices are rotational by default, and the kernel only tracks a swap area in clusters when its device is non rotational, so without that the discard flags are accepted and no discard is ever issued. Together they let the sparse file that backs the area release the blocks the guest stops using, rather than holding the area's high water mark for as long as the container runs.
The server answers reads, writes and flushes against whatever holds the export's blocks, so where they are kept is a choice made when the server is built rather than the one thing it can do. A file is one such place. Memory is another, and it differs in where the blocks end up when the host runs short: memory a process holds is pageable, so those blocks go to the host's own swap and share the pool the rest of the system draws on, while a file takes space of its own. Only the chunks written are held, so an export costs nothing until something is stored in it. Every connection to an export now serves the one store behind it, which is what lets a client read back what another wrote, so a client going away no longer closes it.
A swap area named by an export's URL rather than a path reaches the guest the same way, so what it pushes out is held by a host process instead of a file. That is where the two differ: memory a process holds is pageable, so those pages reach the host's own swap and share the pool the rest of the system draws on, rather than taking storage of their own. Enabling an area writes a header to it, so the test asks for more than a guest that swapped nothing could have put there.
An export that never says it takes trims is never sent one, because the protocol forbids a client from asking otherwise, so a store grew with every block ever written to it and gave nothing back however much the guest had finished with. A swap area is rewritten constantly and never shrinks on its own, so what a guest has done with is most of what an area holds. Saying so and acting on it lets a file punch the hole out and a store in memory drop the chunk, both of which read back afterwards as the zeroes the protocol calls for. https://github.com/NetworkBlockDevice/nbd/blob/master/doc/proto.md
The flags a client sends with a command sit in the two bytes after the request magic, which were read past, so an export that said it took force unit access replied to one before what it wrote was durable. They are read now, and a command carrying that flag waits. An export that only reads says so and turns writes away, which the protocol requires of it and which the pod volume API already offers to ask for. A request reaching past the end is refused as invalid rather than left to fail somewhere in the store, and the errors the protocol names are all spelled out rather than the three that were in use. Writing zeroes and caching are what a client reaches for beside them: zeroes are punched out unless the client asks for the range to stay written through, and a cache hint has nothing to prepare here, so it is accepted rather than refused. https://github.com/NetworkBlockDevice/nbd/blob/master/doc/proto.md
A client that asks for replies carrying their own framing gets them, and a read answered that way names the offset it covers and carries a message with any error rather than a bare number. That framing is what lets an export be asked how it is laid out, so the context describing that is offered and answered: a store in memory knows which chunks it holds and a file is asked through the same seeks a sparse copy uses, so the holes a client is told about are the ones really there. Listing the exports names the one on offer. Every connection serves the one store behind the export, so a flush on any of them covers what was written on the others, which is the condition for saying that a client may spread its work across several. https://github.com/NetworkBlockDevice/nbd/blob/master/doc/proto.md
The memory backed test asserts the release as well as the high water mark: the filler exits while the guest is still up, freeing its swap slots, and a freed cluster is discarded, so the export drops to little more than the swap header. The discards trail the exit, so the test watches the export while the machine is still up rather than sampling it once.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Five places where the NBD server tells a client one thing and does another, read against the protocol document, plus a pluggable block store and one use for it. https://github.com/NetworkBlockDevice/nbd/blob/master/doc/proto.md
The command flags were read past. They sit in the two bytes after the request magic, which were skipped, so an export that said it took force unit access replied to a command carrying it before what it wrote was durable. They are read now, and a command carrying that flag waits.
A read-only export now says so and turns writes away, which the protocol requires of it and which the pod volume API already offers to ask for. A request reaching past the end is refused as invalid rather than served.
Structured replies are offered and answered. A client that asks for replies carrying their own framing gets them, and a read answered that way names the offset it covers and carries a message with any error rather than a bare number. That framing is what lets an export be asked how it is laid out, so the block-descriptor context is offered and answered: a store in memory knows which chunks it holds and a file is asked through the same seeks a sparse copy uses, so the holes a client is told about are the ones really there.
Listing the exports names the one on offer. Every connection serves the one store behind the export, so a flush on any of them covers what was written on the others, which is the condition for saying that a client may spread its work across several.
The store behind an export is chosen rather than fixed to a file, which is what makes a memory-backed store possible: a container's swap area held in host memory rather than on disk.
Dependency and Merge Order
This targets stock
apple/containerization:main. Merge order: #863, #872, #878, then this. The head carries their commits because all of them target stockmain; after they merge, this branch takes currentmainand its remaining diff is the six commits above.Motivation and Context
Discussion: #883, where I ask whether the memory-backed swap area is something you want at all, or whether the useful half here is the protocol fixes plus the pluggable store. The protocol fixes stand on their own and I am glad to send them separately.
Testing
make containerization,make check, andswift testclean.make integration: the memory-backed test asserts the release as well as the high water mark. The filler exits while the guest is still up, freeing its swap slots, and a freed cluster is discarded, so the export drops to little more than the swap header. The discards trail the exit, so the test watches the export while the machine is still up rather than sampling it once; sampling after shutdown measures nothing.Type of Change