Warm Migration feature for bhyve [Part 1]

[mihaiburcea15_gmail.com]

Summary

This series of tickets is based on https://reviews.freebsd.org/D28270 and https://reviews.freebsd.org/D30954, but with multiple modifications and broken down into smaller pieces. The patch was applied over 0e04dd3b66c053422b90c387f7bbd82a0921bda0.

The first 5 represent the implementation for the warm migration feature for bhyve. It is a wrapper over the save/restore feature and it allows sending the virtual machine state (memory, kernel structures, emulated devices) from a source host to a destination host, over the network.

The features works only for IPv4 guests with similar hardware specifications.
More information regarding the implementation can be found here:

• Migrating a bhyve guest (BSDCan 2019)
• Testing and Profiling Warm And Live Migration In bhyve

The usage of the feature is detailed here: Virtual Machine Migration using bhyve.

This first part adds command line parameters and parsing for both warm and live migration.

[markj]

lib/libvmmapi/vmmapi.h

262 ↗

(On Diff #104390)

Spurious whitespace change.

usr.sbin/bhyve/bhyverun.c

1326

Please document the flag in bhyve.8.

1592

1594

usr.sbin/bhyve/snapshot.c

35

No need to add these to new code, they are useless after the switch away from Subversion.

usr.sbin/bhyve/snapshot.h

47 ↗

(On Diff #104390)

There is MAXHOSTNAMELEN in sys/param.h, any reason not to use that? I guess there's an ABI concern if this is used to size a buffer passed over the wire.

usr.sbin/bhyvectl/bhyvectl.c

91

Similarly, it should be documented in bhyvectl.8.

[markj]

usr.sbin/bhyve/snapshot.c

1448

Out of paranoia, I think we should check for truncation here.

[mihaiburcea15_gmail.com]

Addressed the comments from @markj for this and subsequent diffs as well as some other small fixes.

[corvink]

usr.sbin/bhyve/bhyverun.c

250

Other tools like ssh or qemu or bhyve's vnc option are using the syntax host:port. Is there any advantage of using host,port?

1260

Suggestion: receive_migration sounds like a bool. migration_host as used by bhyvectl seems to fit better.

usr.sbin/bhyve/migration.c

35

This is not required for new files.

71

79

99

Wouldn't it be better to check for rc <= 0?

102

Should we return EINVAL instead?

114

Should we return EINVAL instead?

120

Wrong indentation.

usr.sbin/bhyve/migration.h

3

12–31

I'm not really familiar with such license information. Afaik, this isn't required as it's already covered by the SPDX identifier.

34

I prefer a #pragma once

usr.sbin/bhyve/snapshot.c

1452

1460

Shouldn't this be stdout?

1460

1461

usr.sbin/bhyvectl/bhyvectl.c

1770

Wouldn't it be better to check for rc <= 0?

1773

EINVAL?

[corvink]

usr.sbin/bhyve/migration.c

89

Just a suggestion.

[imp]

Just looked at copyright / legal stuff. My comments to migration.h also apply to migration.c.
Corvink had most of the stuff, but I thought I'd add the detail or two that was missing

usr.sbin/bhyve/migration.h

3

Yes, the -FreeBSD variant is obsolete.

8

You likely don't want 'All Rights Reserved' in your copyright notices.

12–31

It's usually better to just have the SPDX identifier unless there's a specific reason to include the boilerplate like it's different materially from the SPDX version. But in that case, you shouldn't use the SPDX stuff since the two should match.

[mihaiburcea15_gmail.com]

We're preparing an update for the patches to a current version of FreeBSD. We'll address @corvink's and @imp's comments there. Thank you for those!

[mihaiburcea15_gmail.com]

Updated the patches to a newer version of FreeBSD.
Addressed @corvink's and @imp's comments.

[corvink]

usr.sbin/bhyve/bhyverun.c

1545

Nit: Sry, didn't saw that the \r was already there. Leaving it as is, makes this patch a bit smaller and cleaner. Nevertheless, I don't have a preference. So, you can decide if you like to change it or not.

usr.sbin/bhyve/migration.h

22–25

Nit: Other places are using __packed.

28

Nit: Not sure if it's required to have a newline at end of file. This one is missing it.

usr.sbin/bhyvectl/bhyvectl.c

68

Just on thought for your patch stack: It might speed up review and merging if you create two patch stacks. One for bhyve related changes and one for bhyvectl related changes.

1760

Is there any reason of not using the same pattern as bhyve?

[mihaiburcea15_gmail.com]

usr.sbin/bhyve/bhyverun.c

1545

Let's change it then.

usr.sbin/bhyve/migration.h

22–25

This one has no reason to be __packed, but I changed it for the newer ones.

usr.sbin/bhyvectl/bhyvectl.c

68

This is actually the only diff that has bhyvectl changes. But I can split it if you think it's a good idea.

[afedorov]

@corvink , I think this commit is too early. Because we haven't finished supporting snapshots, but this commit already fixes the user interface for live migration, which is not finished at all.

Sorry for the late comment, but maybe this commit should be rolled back?

[rew]

In D34717#925210, @afedorov wrote:

@corvink , I think this commit is too early. Because we haven't finished supporting snapshots, but this commit already fixes the user interface for live migration, which is not finished at all.

Sorry for the late comment, but maybe this commit should be rolled back?

This was more of a drive-by commit than anything else - it contains dead code, duplicated code, and code that's been commented out.

[mgrooms_shrew.net]

In D34717#925255, @rew wrote:

In D34717#925210, @afedorov wrote:

@corvink , I think this commit is too early. Because we haven't finished supporting snapshots, but this commit already fixes the user interface for live migration, which is not finished at all.

Sorry for the late comment, but maybe this commit should be rolled back?

This was more of a drive-by commit than anything else - it contains dead code, duplicated code, and code that's been commented out.

I can't tell you how disappointing it is to see comments like this after a review sits in public review for months. You could have easily voiced any of these concerns during the time leading up to the commit instead of withholding them until after it was made. What exactly is your goal hear? A team of students have spent countless hours trying to improve this aspect of FreeBSD with very little help from the community. Corvin is doing his best to help move this work forward. If this is rolled back, will you commit your time to reviewing the code and providing feedback, or do you only plan to offer disparaging comments after commits have been made?

[elenamihailescu22_gmail.com]

In D34717#925210, @afedorov wrote:

@corvink , I think this commit is too early. Because we haven't finished supporting snapshots, but this commit already fixes the user interface for live migration, which is not finished at all.

Sorry for the late comment, but maybe this commit should be rolled back?

I agree that we must get rid of the BHYVE_SNAPSHOT guard. However, I don't see why we cannot have a proper review for the migration part. We updated our code to keep up with the current bhyve / bhyve snapshot implementation (including the CAPSICUM/Casper part). Also, we are willing to improve our code to keep it updated with the changes from the snapshot part (i.e., if the nvlist implementation will be added to the snapshot code, we will add it as well for the migration code, as Corvin suggested). Keep in mind that this feature has been split into multiple parts for ease of review.

Moreover, the feedback we received until now is much appreciated. We do not want this work to be wasted.

[afedorov]

@elenamihailescu22_gmail.com , I really respect your work and what you have done.

But now we have release 14 coming out, and I don't want to see fixed API/ABI (userspace interaction), which then we have to maintain. Simply because users will expect the -R option to be available, but the functionality itself has not yet been implemented. So I want to rollback the commit.

Of course, we need to continue the discussion, but not in this release cycle.

[rew]

In D34717#925263, @mgrooms_shrew.net wrote:

I can't tell you how disappointing it is to see comments like this after a review sits in public review for months. You could have easily voiced any of these concerns during the time leading up to the commit instead of withholding them until after it was made. What exactly is your goal hear?

I have voiced my concerns, via mailing list (https://lists.freebsd.org/archives/freebsd-virtualization/2023-May/001263.html), to which they were simply ignored. Of course, I understand my suggestion is not the one you want to hear - that the warm/live migration patch stack should be rebased after the snapshot feature is compiled in by default.

A team of students have spent countless hours trying to improve this aspect of FreeBSD with very little help from the community.

I have also volunteered my time to improving the snapshot code in an effort to remove the BHYVE_SNAPSHOT #ifdef guards, see the git log for my efforts.

Corvin is doing his best to help move this work forward.

committing questionable code is not helping.

If this is rolled back, will you commit your time to reviewing the code and providing feedback

I'd be willing to review this patch stack when the snapshot feature is compiled in by default.

or do you only plan to offer disparaging comments after commits have been made?

I gave no disparaging comments, I simply described the state of the code as it was committed.

Last thing, and my reason for saying that this patch stack should be rebased after the snapshot feature is compiled in by default:

Committing the live/warm migration patch stack will complicate (i.e., increase developer time) efforts in removing the #ifdef BHYVE_SNAPSHOT guards.

@gusev.vitaliy_gmail.com is actively making efforts to pull this off - I think it's a poor decision to slow that process down by adding experimental features on top of the code that is actively being worked on.

[mgrooms_shrew.net]

It's pretty simple: The most constructive way to help a review process is to provide feedback on the proposed code changes.

A team of students have spent countless hours trying to improve this aspect of FreeBSD with very little help from the community.

I have also volunteered my time to improving the snapshot code in an effort to remove the BHYVE_SNAPSHOT #ifdef guards, see the git log for my efforts.

Excellent. Thank you.

If this is rolled back, will you commit your time to reviewing the code and providing feedback

I'd be willing to review this patch stack when the snapshot feature is compiled in by default.

Any additional review of the code would be welcome. Thank you.

Last thing, and my reason for saying that this patch stack should be rebased after the snapshot feature is compiled in by default:

Committing the live/warm migration patch stack will complicate (i.e., increase developer time) efforts in removing the #ifdef BHYVE_SNAPSHOT guards.

This sounds reasonable. And if the 14 freeze is imminent, it may be the only option. Since you've voiced a strong opinion on this subject, what changes do you consider necessary to have the #ifdef's removed?

@gusev.vitaliy_gmail.com is actively making efforts to pull this off - I think it's a poor decision to slow that process down by adding experimental features on top of the code that is actively being worked on.

Any effort to review and/or improve the code would be very much appreciated.

[corvink]

Would be really nice to receive some feedback before it's committed ...
Just a short comment would be enough.

As far as I can see, this patch series uses the same internal functions to receive snapshot data like snapshot/restore. However, that's mostly independent of the snapshot/restore format series. Therefore, I don't see why we can't make progress here.

I do agree, that this patch series does things in the wrong order. It would be better to add the cmdline parameter and especially the manpage entry as last step. So, I'm going to revert this commit.

@mihaiburcea15_gmail.com It might be a good idea to send a proposal to [e-mail] to explain shortly how this feature is intended to work, how data is transferred and which API/ABI you would propose. I know there are many papers/presentations. However, it might be a good idea to resend this to start a new discussion on a better place. @afedorov, @rew and others can share there feedback in that thread.

[mihaiburcea15_gmail.com]

In D34717#925881, @corvink wrote:

I do agree, that this patch series does things in the wrong order. It would be better to add the cmdline parameter and especially the manpage entry as last step. So, I'm going to revert this commit.

Sounds fair. I can move the parameters and manpage stuff to the last parts. Would you prefer splitting the warm and live migration parameters in different parts as well?

@mihaiburcea15_gmail.com It might be a good idea to send a proposal to [e-mail] to explain shortly how this feature is intended to work, how data is transferred and which API/ABI you would propose. I know there are many papers/presentations. However, it might be a good idea to resend this to start a new discussion on a better place. @afedorov, @rew and others can share there feedback in that thread.

Will do. Thank you.

[corvink]

In D34717#926534, @mihaiburcea15_gmail.com wrote:

In D34717#925881, @corvink wrote:

I do agree, that this patch series does things in the wrong order. It would be better to add the cmdline parameter and especially the manpage entry as last step. So, I'm going to revert this commit.

Sounds fair. I can move the parameters and manpage stuff to the last parts. Would you prefer splitting the warm and live migration parameters in different parts as well?

In general, you should split your patch as much as possible (all commits have to be compileable!). This simplifies reviewing and merging. So, ideally it speeds up upstreaming.

Imagine what happens when warm migration is merged and live migration not. We couldn't merge your parameter and manpage stuff at that point if it includes both.