vmm: fix migration sender hang on worker failure - #192
Closed
Coffeeri wants to merge 1894 commits into
Closed
Conversation
Signed-off-by: Bo Chen <bchen@crusoe.ai>
This reverts commit ced3762. This change lead to a serious memory regression when not using hugepages or shared=on. `MAP_PRIVATE` creates an anonymous memory allocation for every page written when the backing store is a file. This CoW behaviour is useful but leads to double allocations when the backing store is an empty file created by `memfd_create()`. When the page is written to, the CoW semantics require a real page to be created in the memory for the memfd (previously before the page was touched they would all point to the zero page). This real page is filled with zeroes because in theory this page would be accessible via read/write syscalls on the FD even though in our implementation it is only ever `mmap()`ed. The intention of the commit was to enable `fallocate()` to be used to punch holes but that would only affect the inaccessible backing page and the page in the CoW anonymous memory would be unaffected. Leading it likely not to have the desired effect. Fixes: cloud-hypervisor#8211 Signed-off-by: Rob Bradford <rbradford@meta.com>
Reordering commands or adding commands in-between is breaking the migration protocol. By using explicit numbers, we can increase the attention required when touching this code. On-behalf-of: SAP philipp.schuster@sap.com Signed-off-by: Philipp Schuster <philipp.schuster@cyberus-technology.de>
This increases debugability. On-behalf-of: SAP philipp.schuster@sap.com Signed-off-by: Philipp Schuster <philipp.schuster@cyberus-technology.de>
Storing the snapshot causes issues when needing to do a subsequent hotplug instead just pass it through on all the methods that need it making the lifecycle cleaner. Assisted-by: Claude:Opus-4.6 Signed-off-by: Rob Bradford <rbradford@meta.com>
ReadVolatile already provides a default read_volatile_exact() implementation, and WriteVolatile a default write_volatile_exact() implementation. Overriding these functions adds no behavioral value, but duplicates logic and needs to be updated whenever SocketStream gains or changes a variant. On-behalf-of: SAP sebastian.eydam@sap.com Signed-off-by: Sebastian Eydam <sebastian.eydam@cyberus-technology.de>
The trait is not used and thus can be removed. On-behalf-of: SAP sebastian.eydam@sap.com Signed-off-by: Sebastian Eydam <sebastian.eydam@cyberus-technology.de>
TLS connections have a TLS server (listens for incoming connections) and a TLS client (initiates the connection). This commit adds the code for the client side, which is the sender of a migration On-behalf-of: SAP sebastian.eydam@sap.com Signed-off-by: Sebastian Eydam <sebastian.eydam@cyberus-technology.de>
Code for the TLS server, i.e. the receiver of a live migration. On-behalf-of: SAP sebastian.eydam@sap.com Signed-off-by: Sebastian Eydam <sebastian.eydam@cyberus-technology.de>
Teach the migration transport to handle TLS-backed streams alongside plain TCP and UNIX sockets. Introduce a Tls variant in SocketStream and implement the necessary traits. Also updates the local-migration error path to reject any non-UNIX transport, which now includes TLS-wrapped TCP connections. On-behalf-of: SAP sebastian.eydam@sap.com Signed-off-by: Sebastian Eydam <sebastian.eydam@cyberus-technology.de>
Extend ReceiveListener with a TLS-backed listener variant for migration receivers. Store the TCP listener together with the server TLS configuration, wrap accepted sockets in TlsStream::new_server(), and preserver the existing listener cloning and fd polling behavior so receive-side migration code can treat TLS listeners like the existing TCP and UNIX cases. On-behalf-of: SAP sebastian.eydam@sap.com Signed-off-by: Sebastian Eydam <sebastian.eydam@cyberus-technology.de>
For TLS we have to parse the hostname from the given migration URL. For that we have to make a few assumptions about the URL (e.g. it always has a port). To catch problems early, we tighten the URL validation. On-behalf-of: SAP sebastian.eydam@sap.com Signed-off-by: Sebastian Eydam <sebastian.eydam@cyberus-technology.de>
To enable TLS, the caller has to provide a path to a directory that contains the necessary files. On-behalf-of: SAP sebastian.eydam@sap.com Signed-off-by: Sebastian Eydam <sebastian.eydam@cyberus-technology.de>
As we now have more than one parameter for the receive migration call, this commit also adds parsing and validation for those parameters. We maintain backwards compatibility by also correctly parsing the case where the caller only provides a URL. On-behalf-of: SAP sebastian.eydam@sap.com Signed-off-by: Sebastian Eydam <sebastian.eydam@cyberus-technology.de>
Wire in the code paths that activate the TLS encrypting if the necessary API arguments are provided. On-behalf-of: SAP sebastian.eydam@sap.com Signed-off-by: Sebastian Eydam <sebastian.eydam@cyberus-technology.de>
On-behalf-of: SAP sebastian.eydam@sap.com Signed-off-by: Sebastian Eydam <sebastian.eydam@cyberus-technology.de>
Signed-off-by: Philipp Schuster <philipp.schuster@cyberus-technology.de> On-behalf-of: SAP philipp.schuster@sap.com
On-behalf-of: SAP philipp.schuster@sap.com Signed-off-by: Philipp Schuster <philipp.schuster@cyberus-technology.de>
To check gitlint locally, one can run: gitlint --commits "HEAD~2..HEAD" which for example checks the last two commits. Although this is just our kinda private (but public) fork, people might cherry-pick commits from us for whatever reason. So we should have proper commit style. On-behalf-of: SAP philipp.schuster@sap.com Signed-off-by: Philipp Schuster <philipp.schuster@cyberus-technology.de>
Remove irrelevant/annoying CI here to accelerate development. Further, we don't have the runners to run the integration tests, but at least we want to run the unit tests, clippy, etc. Signed-off-by: Philipp Schuster <philipp.schuster@cyberus-technology.de> On-behalf-of: SAP philipp.schuster@sap.com
Adds a flake configuration that enables building Cloud Hypervisor directly from this repository using Nix. This makes it possible to deploy and test Cloud Hypervisor on NixOS systems in real environments. On-behalf-of: SAP philipp.schuster@sap.com Signed-off-by: Philipp Schuster <philipp.schuster@cyberus-technology.de>
Sets the `CH_EXTRA_VERSION` env var during compilation to add the git revision to the version output. On-behalf-of: SAP julian.schindel@sap.com Signed-off-by: Julian Schindel <julian.schindel@cyberus-technology.de>
On-behalf-of: SAP philipp.schuster@sap.com Signed-off-by: Philipp Schuster <philipp.schuster@cyberus-technology.de>
TL;DR: Fix for long rebuilds locally when testing things. The release profile is optimized for maximum performance, sacrificing build speed. As local development and testing requires frequent rebuilds, but the dev profile is way too slow for "real testing", this profile is a sweet spot and helps to investigate things. Instead of `cargo run --release`, one can now run `cargo run --profile optimized-dev`. # Measurements Measurements were done using `$ [cargo clean;] time cargo build --profile release|optimized-dev` and rustc 1.89. I've used the `time`-builtin from zsh. Note that user time is much higher as we have more threads (codegen units) now. The total time is much shorter, tho. ## Clean Build Speedup of 56%. - `$ time cargo clean --release`: `109,67s user 13,64s system 211% cpu 58,343 total` - `$ time cargo clean --profile optimized-dev`: `185,41s user 14,92s system 528% cpu 37,876 total` ## Incremental Build Speedup of 153%. - `$ time cargo clean --release`: `37,58s user 1,53s system 117% cpu 33,356 total` - `$ time cargo clean --profile optimized-dev`: `47,62s user 1,71s system 373% cpu 13,220 total` Signed-off-by: Philipp Schuster <philipp.schuster@cyberus-technology.de> On-behalf-of: SAP philipp.schuster@sap.com
With debug symbols, we will get better backtraces and can improve our experience debugging. The only downside is larger binary size which is negligible in our case. There are no implications for the performance. Stripped: 3.9M Unstripped: 4.7M Signed-off-by: Philipp Schuster <philipp.schuster@cyberus-technology.de> On-behalf-of: SAP philipp.schuster@sap.com
This improves the quality of the logs when debugging issues. I've used the `jiff` time library as it is well-known time library of the ecosystem. Now, the first logging message (level info!) looks somewhat like this: ```text Cloud Hypervisor starting: build version: v51.1-203-g7f0f1f5cb-dirty, date: 2026-03-30T14:42:30.00730185+02:00 ``` On-behalf-of: SAP philipp.schuster@sap.com Signed-off-by: Philipp Schuster <philipp.schuster@cyberus-technology.de>
We've seen big VMs under massive load that regularly fired their
`"vCPU thread did not respond in {count}ms to signal - retrying` warning
message. So far, all such situations recovered themselves after ~600ms.
To be more fail-safe for the production environment under load, we
increase this timeout to 10s.
On-behalf-of: SAP philipp.schuster@sap.com
Signed-off-by: Philipp Schuster <philipp.schuster@cyberus-technology.de>
This is very helpful information in the field where libvirt just sets the info level. This retains the behavior that we already have at our customer. Further, our tests rely on that line to check the migration progresses. On-behalf-of: SAP philipp.schuster@sap.com Signed-off-by: Philipp Schuster <philipp.schuster@cyberus-technology.de>
This is a temporary measurement as upstream decided for a different name than we in our fork. On-behalf-of: SAP philipp.schuster@sap.com Signed-off-by: Philipp Schuster <philipp.schuster@cyberus-technology.de>
This includes the timezone again. On-behalf-of: SAP philipp.schuster@sap.com Signed-off-by: Philipp Schuster <philipp.schuster@cyberus-technology.de>
Inline the format argument rather than provide it as a parameter. Signed-off-by: Rob Bradford <rbradford@meta.com> (cherry picked from commit 85415cd)
Inline the format argument rather than provide it as a parameter. Signed-off-by: Rob Bradford <rbradford@meta.com> (cherry picked from commit 4d27640)
Replace use of redundant & (leading to &&) in format strings. Signed-off-by: Rob Bradford <rbradford@meta.com> (cherry picked from commit eca621c)
Replace format arguments with inlined versions. Signed-off-by: Rob Bradford <rbradford@meta.com> (cherry picked from commit 29f392f)
When formatting as hex the minimum format size is 4 not 2. On-behalf-of: SAP julian.schindel@sap.com Signed-off-by: Julian Schindel <julian.schindel@cyberus-technology.de>
Replace use of redundant & (leading to &&) in format strings. On-behalf-of: SAP julian.schindel@sap.com Signed-off-by: Julian Schindel <julian.schindel@cyberus-technology.de>
The function suffers from a TOCTOU problem. There is no guarantee that the locks set on the file at the time of the state check are the same as at the time of the locking attempt. On-behalf-of: SAP julian.schindel@sap.com Signed-off-by: Julian Schindel <julian.schindel@cyberus-technology.de>
Rust typically doesn't use `get_` prefixes. On-behalf-of: SAP julian.schindel@sap.com Signed-off-by: Julian Schindel <julian.schindel@cyberus-technology.de>
All functions take a `LockGranularity` already and making them part of `LockGranularity` makes it easier to add associated helpers. On-behalf-of: SAP julian.schindel@sap.com Signed-off-by: Julian Schindel <julian.schindel@cyberus-technology.de>
By extracting the full file and file range locking, it's easier to add file lock handling that's not following this pattern. On-behalf-of: SAP julian.schindel@sap.com Signed-off-by: Julian Schindel <julian.schindel@cyberus-technology.de>
Avoids repeating the retry/error logic when adding new users of the function. On-behalf-of: SAP julian.schindel@sap.com Signed-off-by: Julian Schindel <julian.schindel@cyberus-technology.de>
Reference to pointer coercion doesn't happen for variadic functions like `libc::fcntl`. While this doesn't pose a problem by itself, it removes the check whether a reference can be coerced to the appropriate pointer type or mutability. By explicitly casting to a pointer, we ensure refactors don't accidentally break the safety requirements. On-behalf-of: SAP julian.schindel@sap.com Signed-off-by: Julian Schindel <julian.schindel@cyberus-technology.de>
QEMU uses locks on specific bytes for modeling file lock permissions. This commit only implements what's needed to model the permissions cloud-hypervisor currently uses. This is needed to be compatible with storage management systems that expect QEMU compatible file locking like NetApp's NFS implementation. QEMU uses two offsets from the start of the file to separate permissions and "unshared" permissions. "unshared" permissions are permissions that cannot be shared between lock holders [0] and can be considered exclusive. Starting from each offset, locks are placed with a length of one byte. Each corresponds to a specific permission [1][2]. The locking is done by first signaling intent by locking the required marker bytes. Next any conflicts with existing locks are detected and in the failure case, the locks are rolled back. Locking is done via `F_RDLCK`, which allows other parties to set the same locks and check for conflicts with `F_WRLCK`. [0]: https://github.com/qemu/qemu/blob/30e8a06b64aa58a3990ba39cb5d09531e7d265e0/block/file-posix.c#L131-L134 [1]: https://github.com/qemu/qemu/blob/30e8a06b64aa58a3990ba39cb5d09531e7d265e0/include/block/block-common.h#L392-L437 [2]: https://github.com/qemu/qemu/blob/30e8a06b64aa58a3990ba39cb5d09531e7d265e0/block/file-posix.c#L868-L940 On-behalf-of: SAP julian.schindel@sap.com Signed-off-by: Julian Schindel <julian.schindel@cyberus-technology.de>
On-behalf-of: SAP julian.schindel@sap.com Signed-off-by: Julian Schindel <julian.schindel@cyberus-technology.de>
A PCI BAR stores its address in the upper bits and type information in the lower bits. To determine the BAR size, an operating system disables memory decoding, writes ones to the BAR's address bits, and reads the value back. Bits that remain zero reveal the required alignment and therefore the size. We currently recognize only 0xffffffff as a size probe. OpenBSD instead writes the memory BAR address mask 0xfffffff0, without setting the lower four non-address bits [0][1]. We misinterpret that probe as a real BAR relocation. Because OpenBSD has disabled memory space, the relocation remains pending. We apply it when OpenBSD enables memory space again, moving the virtio block BAR away from its mapped address and preventing the guest from booting. Recognize probes using the address mask for each BAR type and exclude them from BAR reprogramming. Add a test for the OpenBSD memory BAR probe. [0]: https://github.com/openbsd/src/blob/54a2bc1e5457a3dc555cb0cc642641a62266c778/sys/dev/pci/pci_map.c#L154-L180 [1]: https://github.com/openbsd/src/blob/54a2bc1e5457a3dc555cb0cc642641a62266c778/sys/dev/pci/pcireg.h#L429-L443 Co-authored-by: Thomas Prescher <thomas.prescher@cyberus-technology.de> On-behalf-of: SAP leander.kohler@sap.com Signed-off-by: Leander Kohler <leander.kohler@cyberus-technology.de>
This reverts commit f795caf. On-behalf-of: SAP julian.schindel@sap.com Signed-off-by: Julian Schindel <julian.schindel@cyberus-technology.de>
Add support for formatting logs following the popular glog crate style. In particular this can use local or UTC time along with a single character level. As an implementation detail of that implement finegrained date time log outputs including both local and UTC variants. Signed-off-by: Rob Bradford <rbradford@meta.com> Assisted-by: Claude:Opus-4.6 (cherry picked from commit e0c68db)
Local-time log fields called `jiff::Zoned::now()`, which resolves the system timezone on every record by reading `/etc/localtime`/`/etc/timezone` if it doesn't hit the cached version. This cache miss could then cause a seccomp violation depending on the thread it was run from. Avoid this by capturing the value in `Logger`. This avoids opening the seccomp filter for the whole process. Signed-off-by: Dylan Reid <dgreid@fb.com> (cherry picked from commit b88d5de)
This reverts commit 3caabf4. On-behalf-of: SAP julian.schindel@sap.com Signed-off-by: Julian Schindel <julian.schindel@cyberus-technology.de>
Co-authored-by: Philipp Schuster <philipp.schuster@cyberus-technology.de> On-behalf-of: SAP julian.schindel@sap.com Signed-off-by: Julian Schindel <julian.schindel@cyberus-technology.de>
Migration threads may be left orphaned and keep the socket bound after a migration has failed. Prevent this by signaling termination via the `terminate_fd` in `ReceiveAdditionalConnections`'s `Drop` impl. On-behalf-of: SAP julian.schindel@sap.com Signed-off-by: Julian Schindel <julian.schindel@cyberus-technology.de>
The ThrottleCommand does not need to be Clone (nor Copy) as of now and we will need to introduce a variant wrapping a type that is not Copy in a follow up commit. It is also debatable whether a message type should be Clone, as they are typically intended to be sent only once (and not used for other purposes). Signed-off-by: Oliver Anderson <oliver.anderson@cyberus-technology.de> On-behalf-of: SAP oliver.anderson@sap.com
In order to add a command for resetting the throttle thread with confirmation we have to add an additional variant that wraps a type that does not implement the PartialEq and Eq traits. We thus need to choose between manually implementing these traits for ThrottleCommand, or avoiding them. We choose the latter because that requires much less code. Signed-off-by: Oliver Anderson <oliver.anderson@cyberus-technology.de> On-behalf-of: SAP oliver.anderson@sap.com
It makes more sense for a command to use the imperative form and the enum does not need to be used to track the throttling thread's current state. This enables us to introduce a reset variant to the enum that asks the throttle thread to stop its throttle loop and inform us when it is back to waiting for the next incoming command. Signed-off-by: Oliver Anderson <oliver.anderson@cyberus-technology.de> On-behalf-of: SAP oliver.anderson@sap.com
The current behavior of joining the vCPU throttling thread towards the end of a live migration is problematic when the live migration fails because then auto-converge is no longer possible on a second attempt. We fix this by instead resetting the throttling thread to its initial state. The throttling thread is now instead gracefully stopped by the ThrottleThreadHandle's destructor which runs whenever the Vm instance goes out of scope. Signed-off-by: Oliver Anderson <oliver.anderson@cyberus-technology.de> On-behalf-of: SAP oliver.anderson@sap.com
When a live migration fails before all memory iterations have been sent we also need to reset the throttling thread, otherwise the VM continues existing on the migration source with throttled vCPUs. Signed-off-by: Oliver Anderson <oliver.anderson@cyberus-technology.de> On-behalf-of: SAP oliver.anderson@sap.com
A single failing parallel migration connection can deadlock the migration sender. When the bounded send-channel is full, `SendAdditionalConnections::cleanup()` silently drops the Disconnect messages via the non-blocking `try_send`. The surviving workers drain the channel and then block forever in `recv()`, while the main thread blocks forever in `join()`. The VM stays stuck in the "migrating" state. We fix this by sending the `Disconnect` messages with a blocking send. In addition, once one of the workers sets `worker_error`, the other workers stop sending the queued memory chunks and drain the channel, so the Disconnects are queued and all workers exit. On-behalf-of: SAP leander.kohler@sap.com Signed-off-by: Leander Kohler <leander.kohler@cyberus-technology.de>
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
12 participants
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.
A single failing parallel migration connection can deadlock the migration sender.
When the bounded send-channel is full,
SendAdditionalConnections::cleanup()silently drops the Disconnect messages via the non-blockingtry_send. The surviving workers drain the channel and then block forever inrecv(), while the main thread blocks forever injoin(). The VM stays stuck in the "migrating" state.We fix this by sending the
Disconnectmessages with a blocking send. In addition, once one of the workers setsworker_error, the other workers stop sending the queued memory chunks and drain the channel, so the Disconnects are queued and all workers exit.cc @tpressure @phip1611