[WIP] misc: pre-requisites for live-migration of virtio-net devices & network FDs #7152
Conversation
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
6 tasks
phip1611
force-pushed
the
network-fd-livemig-prerequisites
branch
5 times, most recently
from
1003f9c to
96df2a4
Compare
phip1611
commented
Jun 19, 2025
phip1611
force-pushed
the
network-fd-livemig-prerequisites
branch
2 times, most recently
from
748d6b1 to
a3567fb
Compare
phip1611
commented
Jun 20, 2025
phip1611
force-pushed
the
network-fd-livemig-prerequisites
branch
2 times, most recently
from
ad8fe5e to
0fca3b0
Compare
phip1611
commented
Jun 20, 2025
| /// Once these valid file descriptors are received here, we integrate the actual | ||
| /// FDs into the virtual machine configuration, allowing the virtio-net device to | ||
| /// function correctly with its backing network resources. | ||
| fn apply_new_fds_to_cfg<T>( |
Member
Author
There was a problem hiding this comment.
This is a very cool and helpful abstraction for #7150.
Prevents a lot of code duplication and unifies logging and error handling across handlers!
phip1611
force-pushed
the
network-fd-livemig-prerequisites
branch
2 times, most recently
from
ebb9a40 to
968e378
Compare
alyssais
reviewed
Jun 21, 2025
Member
alyssais
left a comment
alyssais
left a comment
There was a problem hiding this comment.
Support what you're trying to do here, but I have some comments :)
| serialize_with = "serialize_restorednetconfig_fds", | ||
| deserialize_with = "deserialize_restorednetconfig_fds" | ||
| )] | ||
| #[serde(default, deserialize_with = "deserialize_restorednetconfig_fds")] |
Member
There was a problem hiding this comment.
Doesn't this mean we end up serializing the file descriptor numbers? I don't think that makes sense — it would be better to not emit them at all, with a proper deprecation process if necessary.
Member
Author
There was a problem hiding this comment.
The commit message explained the why part. The reason here is that the "info" call (e.g. ch-remote --api-socket ... info returns something valid`.
I personally think this is fine. I don't have a strong opinion here.
phip1611
force-pushed
the
network-fd-livemig-prerequisites
branch
5 times, most recently
from
8705c22 to
015196f
Compare
phip1611
commented
Jun 23, 2025
phip1611
force-pushed
the
network-fd-livemig-prerequisites
branch
from
015196f to
a502c4a
Compare
alyssais
reviewed
Jun 23, 2025
phip1611
force-pushed
the
network-fd-livemig-prerequisites
branch
from
a502c4a to
45c0d6c
Compare
One can call `to_vec()` anyway if one needs an owned copy. This change further helps to prevent needless copies in upcoming changes. Signed-off-by: Philipp Schuster <[email protected]> On-behalf-of: SAP [email protected]
Signed-off-by: Philipp Schuster <[email protected]> On-behalf-of: SAP [email protected]
phip1611
force-pushed
the
network-fd-livemig-prerequisites
branch
from
45c0d6c to
6050b08
Compare
phip1611
force-pushed
the
network-fd-livemig-prerequisites
branch
2 times, most recently
from
ba81699 to
b191c44
Compare
To ease debugging of networking in the field, especially in context of libvirt, state save/resume, and live-migration, more logging helps to identify what happens behind the scenes in certain corner-cases as well as (apparently) normal operation. For example: There are multiple ways to create virtio-net devices: - with vhost-user backend - from provided network FDs - from provided interface name of Tap device is given - Tap device is created by CHV (fallback) To confirm that the expected behavior occurs—especially in the more complex case of network file descriptors—these logging statements provide valuable insight into the system's internal operations. Signed-off-by: Philipp Schuster <[email protected]> On-behalf-of: SAP [email protected]
Live migration, state save/resume, and hotplug are not trivial when it comes to virtio-net devices backed by network FDs. As the mechanism behind it can be considered as quite "multi-step magic" even for experienced programmers, it makes sense to thoroughly document this to ease debugging and to improve the mental model of developers working on this in the future. Signed-off-by: Philipp Schuster <[email protected]> On-behalf-of: SAP [email protected]
Signed-off-by: Philipp Schuster <[email protected]> On-behalf-of: SAP [email protected]
(1) The old messages are missing the "why" part. With this change, users
of Cloud Hypervisor have somehow more context and people looking at
the code perfectly know what's going on.
(2) Using warn! implies that the user should take action, but in this
case, there’s nothing the user can do. If the API is used correctly
and file descriptors are passed via an SCM_RIGHTS message over a
UNIX domain socket, everything works as intended. In that case,
there's no need to issue a warning — debug! is sufficient.
Signed-off-by: Philipp Schuster <[email protected]>
On-behalf-of: SAP [email protected]
Deserializing values as `-1` makes sense to prevent errors, so let's keep it. However, serializing them differently adds confusion. For example, a `ch-remote info` call should not report `-1` but the actual FDs. Signed-off-by: Philipp Schuster <[email protected]> On-behalf-of: SAP [email protected]
This unifies error handling, the implementation, and logging. Otherwise, we have code repetition, especially with the upcoming support for the VmReceiveMigration patch. Signed-off-by: Philipp Schuster <[email protected]> On-behalf-of: SAP [email protected]
phip1611
force-pushed
the
network-fd-livemig-prerequisites
branch
from
b191c44 to
c88c5cc
Compare
phip1611
changed the title
Member
Author
|
What started as a small PR has grown relatively large, so I'm closing it in favor of splitting it into multiple smaller, more manageable PRs for easier review. Please note that I'll be on vacation from July 10th to July 30th. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
This is a split-out from #7150 for better review-ability. It contains pre-requisites that I consider as crucial and/or helpful for the planned changes but also universally.
Please review this commit by commit.
Steps to Undraft
debug!logging is useful. I'm not at the end of the road yet, there might be more useful debugging spots in the code