-
Notifications
You must be signed in to change notification settings - Fork 2
Post Migration Announce #66
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: gardenlinux
Are you sure you want to change the base?
Conversation
…ncements Some devices can do announcements after a live migration, e.g. a network device announces its new location in a network. With this change a device can return an announcer to do that. On-behalf-of: SAP [email protected] Signed-off-by: Sebastian Eydam <[email protected]>
To announce the new location in a network, virtio-net devices can send reverse ARP packets. On-behalf-of: SAP [email protected] Signed-off-by: Sebastian Eydam <[email protected]>
After this change switches in a network should find the new location of a VM quicker. On-behalf-of: SAP [email protected] Signed-off-by: Sebastian Eydam <[email protected]>
|
Could you please rephrase your PR title, the commits, and the corresponding internals to something more descriptive than "post migration announce"? Something like "Post-migration ARP Advertisment Packets"? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! Before reviewing this further, we should discuss if the proposed design is something we want to go with. I prefer to use the existing resume() path. Bonus point: it would also work after a cold migration.
Are there any implications I'm missing here by chance?
PS: You also need to do the same for the Neighbor Discovery Protocol (ARP equivalent in IPv6).
|
|
||
| /// Some devices can announce their location after a live migration to | ||
| /// speed up normal execution. | ||
| fn post_migration_announcer(&mut self) -> std::option::Option<Box<dyn PostMigrationAnnouncer>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use Option and not the fully qualified path
| fn build_rarp_announce(&self) -> [u8; 60] { | ||
| const ETH_P_RARP: u16 = 0x8035; // Ethertype RARP | ||
| const ARP_HTYPE_ETH: u16 = 0x1; // Hardware type Ethernet | ||
| const ARP_PTYPE_IP: u16 = 0x0800; // Protocol type IPv4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need to do the same for the ARP equivalent of IPv6?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I know this works for IPv4 and IPv6. We never put any IP addresses into the packet, only MAC addresses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But IPv6 doesn't use ARP AFAICT. ipv6 uses the Neighbor Discovery Protocol (NDP).
I am not 100% with that, but we should clarify and make sure your PR reaches your desired network discovery speedup if guest VMs only uses IPv6!
| // The unwrap is safe, because the state machine makes sure we called | ||
| // vm_receive_state before, which creates the VM. | ||
| let vm = self.vm.vm_mut().unwrap(); | ||
| vm.announce(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am very surprised about this fully-blown internal abstraction. I think this can be build on the existing resume() mechanism that Cloud Hypervisor already has?
Before I review this further, we should discuss this fundamental design discussion.
This PR adds a post migration announce to Cloud Hypervisor. After a successful live migration, Cloud Hypervisor now sends reverse ARP packets on all virtio-net devices. That way switches should learn the new location of the VM faster.