| Message ID | 20200702175754.2211821-3-berrange@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | migration: bring savevm/loadvm/delvm over to QMP | expand |
On 7/2/20 12:57 PM, Daniel P. Berrangé wrote: > savevm, loadvm and delvm are some of the few commands that have never > been converted to use QMP. The primary reason for this lack of > conversion is that they block execution of the thread for as long as > they run. > > Despite this downside, however, libvirt and applications using libvirt > has used these commands for as long as QMP has existed, via the > "human-monitor-command" passthrough command. IOW, while it is clearly > desirable to be able to fix the blocking problem, this is not an > immediate obstacle to real world usage. > > Meanwhile there is a need for other features which involve adding new > parameters to the commands. This is possible with HMP passthrough, but > it provides no reliable way for apps to introspect features, so using > QAPI modelling is highly desirable. > > This patch thus introduces trival savevm, loadvm, delvm commands trivial > to QMP that are functionally identical to the HMP counterpart, including > the blocking problem. Should we name them 'x-savevm', 'x-loadvm', 'x-delvm' to give ourselves room to change them when we DO solve the blocking issue? Or will the solution of the blocking issue introduce new QMP commands, at which point we can add QMP deprecation markers on these commands to eventually retire them? > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > --- > +++ b/qapi/migration.json > @@ -1621,3 +1621,79 @@ > ## > { 'event': 'UNPLUG_PRIMARY', > 'data': { 'device-id': 'str' } } > + > +## > +# @savevm: > +# > +# Save a VM snapshot > +# > +# @tag: name of the snapshot to create. If it already > +# exists it will be replaced. > +# > +# Note that execution of the VM will be paused during the time > +# it takes to save the snapshot > +# > +# Returns: nothing > +# > +# Example: > +# > +# -> { "execute": "savevm", > +# "data": { > +# "tag": "my-snap" > +# } > +# } > +# <- { "return": { } } > +# > +# Since: 5.2 I guess you are NOT trying to make 5.1 soft freeze next week?
On Thu, Jul 02, 2020 at 01:12:52PM -0500, Eric Blake wrote: > On 7/2/20 12:57 PM, Daniel P. Berrangé wrote: > > savevm, loadvm and delvm are some of the few commands that have never > > been converted to use QMP. The primary reason for this lack of > > conversion is that they block execution of the thread for as long as > > they run. > > > > Despite this downside, however, libvirt and applications using libvirt > > has used these commands for as long as QMP has existed, via the > > "human-monitor-command" passthrough command. IOW, while it is clearly > > desirable to be able to fix the blocking problem, this is not an > > immediate obstacle to real world usage. > > > > Meanwhile there is a need for other features which involve adding new > > parameters to the commands. This is possible with HMP passthrough, but > > it provides no reliable way for apps to introspect features, so using > > QAPI modelling is highly desirable. > > > > This patch thus introduces trival savevm, loadvm, delvm commands > > trivial > > > to QMP that are functionally identical to the HMP counterpart, including > > the blocking problem. > > Should we name them 'x-savevm', 'x-loadvm', 'x-delvm' to give ourselves room > to change them when we DO solve the blocking issue? Or will the solution of > the blocking issue introduce new QMP commands, at which point we can add QMP > deprecation markers on these commands to eventually retire them? I was in two minds about this, so I'm open to arguments either way. The primary goal is for libvirt to consume the APIs as soon as possible, and generally libvirt doesn't want todo this is they are declared experimental via a "x-" prefix. So that pushes me away from "x-". If we don't have an "x-" prefix and want to make changes, we can add extra parameters to trigger new behaviour in backwards compatible manner. Or we can simply deprecate these commands, deleting them 2 releases later, while adding completely new commands. If we think the prposed design will definitely need incompatible changes in a very short time frame though, that would push towards "x-". So IMHO the right answer largely depends on whether there is a credible strategy to implement the ideal non-blocking solution in a reasonable amount of time. I can't justify spending much time on this myself, but I'm willing to consider & try proposals for solving the blocking problem if they're not too complex / invasive. I just don't want to end up having a "x-savevm" command for another 10 years, waiting for a perfect solution that never arrives because people always have higher priority items, as apps are clearly able to accept the blocking problem if the alternative is no snapshots at all. > > + > > +## > > +# @savevm: > > +# > > +# Save a VM snapshot > > +# > > +# @tag: name of the snapshot to create. If it already > > +# exists it will be replaced. > > +# > > +# Note that execution of the VM will be paused during the time > > +# it takes to save the snapshot > > +# > > +# Returns: nothing > > +# > > +# Example: > > +# > > +# -> { "execute": "savevm", > > +# "data": { > > +# "tag": "my-snap" > > +# } > > +# } > > +# <- { "return": { } } > > +# > > +# Since: 5.2 > > I guess you are NOT trying to make 5.1 soft freeze next week? Correct. It is unrealistic to consider this for soft freeze. I'd really love to have a solution in 5.2 though, even if it doesn't solve all our problems. Something that can at least unblock apps that want to use OVMF with internal snapshots today. Regards, Daniel
* Daniel P. Berrangé (berrange@redhat.com) wrote: > On Thu, Jul 02, 2020 at 01:12:52PM -0500, Eric Blake wrote: > > On 7/2/20 12:57 PM, Daniel P. Berrangé wrote: > > > savevm, loadvm and delvm are some of the few commands that have never > > > been converted to use QMP. The primary reason for this lack of > > > conversion is that they block execution of the thread for as long as > > > they run. > > > > > > Despite this downside, however, libvirt and applications using libvirt > > > has used these commands for as long as QMP has existed, via the > > > "human-monitor-command" passthrough command. IOW, while it is clearly > > > desirable to be able to fix the blocking problem, this is not an > > > immediate obstacle to real world usage. > > > > > > Meanwhile there is a need for other features which involve adding new > > > parameters to the commands. This is possible with HMP passthrough, but > > > it provides no reliable way for apps to introspect features, so using > > > QAPI modelling is highly desirable. > > > > > > This patch thus introduces trival savevm, loadvm, delvm commands > > > > trivial > > > > > to QMP that are functionally identical to the HMP counterpart, including > > > the blocking problem. > > > > Should we name them 'x-savevm', 'x-loadvm', 'x-delvm' to give ourselves room > > to change them when we DO solve the blocking issue? Or will the solution of > > the blocking issue introduce new QMP commands, at which point we can add QMP > > deprecation markers on these commands to eventually retire them? > > I was in two minds about this, so I'm open to arguments either way. > > The primary goal is for libvirt to consume the APIs as soon as possible, > and generally libvirt doesn't want todo this is they are declared experimental > via a "x-" prefix. So that pushes me away from "x-". > > If we don't have an "x-" prefix and want to make changes, we can add extra > parameters to trigger new behaviour in backwards compatible manner. Or we can > simply deprecate these commands, deleting them 2 releases later, while adding > completely new commands. > > If we think the prposed design will definitely need incompatible changes in > a very short time frame though, that would push towards "x-". > > So IMHO the right answer largely depends on whether there is a credible > strategy to implement the ideal non-blocking solution in a reasonable amount > of time. I can't justify spending much time on this myself, but I'm willing > to consider & try proposals for solving the blocking problem if they're not > too complex / invasive. Remind me, what was the problem with just making a block: migration channel, and then we can migrate to it? Dave > I just don't want to end up having a "x-savevm" command for another 10 years, > waiting for a perfect solution that never arrives because people always have > higher priority items, as apps are clearly able to accept the blocking problem > if the alternative is no snapshots at all. > > > > > + > > > +## > > > +# @savevm: > > > +# > > > +# Save a VM snapshot > > > +# > > > +# @tag: name of the snapshot to create. If it already > > > +# exists it will be replaced. > > > +# > > > +# Note that execution of the VM will be paused during the time > > > +# it takes to save the snapshot > > > +# > > > +# Returns: nothing > > > +# > > > +# Example: > > > +# > > > +# -> { "execute": "savevm", > > > +# "data": { > > > +# "tag": "my-snap" > > > +# } > > > +# } > > > +# <- { "return": { } } > > > +# > > > +# Since: 5.2 > > > > I guess you are NOT trying to make 5.1 soft freeze next week? > > Correct. It is unrealistic to consider this for soft freeze. > > I'd really love to have a solution in 5.2 though, even if it doesn't > solve all our problems. Something that can at least unblock apps that > want to use OVMF with internal snapshots today. > > Regards, > Daniel > -- > |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o- https://fstop138.berrange.com :| > |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Fri, Jul 03, 2020 at 04:49:33PM +0100, Dr. David Alan Gilbert wrote: > * Daniel P. Berrangé (berrange@redhat.com) wrote: > > On Thu, Jul 02, 2020 at 01:12:52PM -0500, Eric Blake wrote: > > > On 7/2/20 12:57 PM, Daniel P. Berrangé wrote: > > > > savevm, loadvm and delvm are some of the few commands that have never > > > > been converted to use QMP. The primary reason for this lack of > > > > conversion is that they block execution of the thread for as long as > > > > they run. > > > > > > > > Despite this downside, however, libvirt and applications using libvirt > > > > has used these commands for as long as QMP has existed, via the > > > > "human-monitor-command" passthrough command. IOW, while it is clearly > > > > desirable to be able to fix the blocking problem, this is not an > > > > immediate obstacle to real world usage. > > > > > > > > Meanwhile there is a need for other features which involve adding new > > > > parameters to the commands. This is possible with HMP passthrough, but > > > > it provides no reliable way for apps to introspect features, so using > > > > QAPI modelling is highly desirable. > > > > > > > > This patch thus introduces trival savevm, loadvm, delvm commands > > > > > > trivial > > > > > > > to QMP that are functionally identical to the HMP counterpart, including > > > > the blocking problem. > > > > > > Should we name them 'x-savevm', 'x-loadvm', 'x-delvm' to give ourselves room > > > to change them when we DO solve the blocking issue? Or will the solution of > > > the blocking issue introduce new QMP commands, at which point we can add QMP > > > deprecation markers on these commands to eventually retire them? > > > > I was in two minds about this, so I'm open to arguments either way. > > > > The primary goal is for libvirt to consume the APIs as soon as possible, > > and generally libvirt doesn't want todo this is they are declared experimental > > via a "x-" prefix. So that pushes me away from "x-". > > > > If we don't have an "x-" prefix and want to make changes, we can add extra > > parameters to trigger new behaviour in backwards compatible manner. Or we can > > simply deprecate these commands, deleting them 2 releases later, while adding > > completely new commands. > > > > If we think the prposed design will definitely need incompatible changes in > > a very short time frame though, that would push towards "x-". > > > > So IMHO the right answer largely depends on whether there is a credible > > strategy to implement the ideal non-blocking solution in a reasonable amount > > of time. I can't justify spending much time on this myself, but I'm willing > > to consider & try proposals for solving the blocking problem if they're not > > too complex / invasive. > > Remind me, what was the problem with just making a block: migration > channel, and then we can migrate to it? migration only does vmstate, not disks. The current blockdev commands are all related to external snapshots, nothing for internal snapshots AFAIK. So we still need commands to load/save internal snapshots of the disk data in the qcow2 files. So we could look at loadvm/savevm conceptually as a syntax sugar above a migration transport that targets disk images, and blockdev QMP command that can do internal snapshots. Neither of these exist though and feel like a significantly larger amount of work than using existing functionality that is currently working. Regards, Daniel
* Daniel P. Berrangé (berrange@redhat.com) wrote: > On Fri, Jul 03, 2020 at 04:49:33PM +0100, Dr. David Alan Gilbert wrote: > > * Daniel P. Berrangé (berrange@redhat.com) wrote: > > > On Thu, Jul 02, 2020 at 01:12:52PM -0500, Eric Blake wrote: > > > > On 7/2/20 12:57 PM, Daniel P. Berrangé wrote: > > > > > savevm, loadvm and delvm are some of the few commands that have never > > > > > been converted to use QMP. The primary reason for this lack of > > > > > conversion is that they block execution of the thread for as long as > > > > > they run. > > > > > > > > > > Despite this downside, however, libvirt and applications using libvirt > > > > > has used these commands for as long as QMP has existed, via the > > > > > "human-monitor-command" passthrough command. IOW, while it is clearly > > > > > desirable to be able to fix the blocking problem, this is not an > > > > > immediate obstacle to real world usage. > > > > > > > > > > Meanwhile there is a need for other features which involve adding new > > > > > parameters to the commands. This is possible with HMP passthrough, but > > > > > it provides no reliable way for apps to introspect features, so using > > > > > QAPI modelling is highly desirable. > > > > > > > > > > This patch thus introduces trival savevm, loadvm, delvm commands > > > > > > > > trivial > > > > > > > > > to QMP that are functionally identical to the HMP counterpart, including > > > > > the blocking problem. > > > > > > > > Should we name them 'x-savevm', 'x-loadvm', 'x-delvm' to give ourselves room > > > > to change them when we DO solve the blocking issue? Or will the solution of > > > > the blocking issue introduce new QMP commands, at which point we can add QMP > > > > deprecation markers on these commands to eventually retire them? > > > > > > I was in two minds about this, so I'm open to arguments either way. > > > > > > The primary goal is for libvirt to consume the APIs as soon as possible, > > > and generally libvirt doesn't want todo this is they are declared experimental > > > via a "x-" prefix. So that pushes me away from "x-". > > > > > > If we don't have an "x-" prefix and want to make changes, we can add extra > > > parameters to trigger new behaviour in backwards compatible manner. Or we can > > > simply deprecate these commands, deleting them 2 releases later, while adding > > > completely new commands. > > > > > > If we think the prposed design will definitely need incompatible changes in > > > a very short time frame though, that would push towards "x-". > > > > > > So IMHO the right answer largely depends on whether there is a credible > > > strategy to implement the ideal non-blocking solution in a reasonable amount > > > of time. I can't justify spending much time on this myself, but I'm willing > > > to consider & try proposals for solving the blocking problem if they're not > > > too complex / invasive. > > > > Remind me, what was the problem with just making a block: migration > > channel, and then we can migrate to it? > > migration only does vmstate, not disks. The current blockdev commands > are all related to external snapshots, nothing for internal snapshots > AFAIK. So we still need commands to load/save internal snapshots of > the disk data in the qcow2 files. > > So we could look at loadvm/savevm conceptually as a syntax sugar above > a migration transport that targets disk images, and blockdev QMP command > that can do internal snapshots. Neither of these exist though and feel > like a significantly larger amount of work than using existing functionality > that is currently working. I think that's what we should aim for; adding this wrapper isn't gaining that much without moving a bit towards that; so I would stick with the x- for now. Dave > Regards, > Daniel > -- > |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o- https://fstop138.berrange.com :| > |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Fri, Jul 03, 2020 at 05:10:12PM +0100, Dr. David Alan Gilbert wrote: > * Daniel P. Berrangé (berrange@redhat.com) wrote: > > On Fri, Jul 03, 2020 at 04:49:33PM +0100, Dr. David Alan Gilbert wrote: > > > * Daniel P. Berrangé (berrange@redhat.com) wrote: > > > > On Thu, Jul 02, 2020 at 01:12:52PM -0500, Eric Blake wrote: > > > > > On 7/2/20 12:57 PM, Daniel P. Berrangé wrote: > > > > > > savevm, loadvm and delvm are some of the few commands that have never > > > > > > been converted to use QMP. The primary reason for this lack of > > > > > > conversion is that they block execution of the thread for as long as > > > > > > they run. > > > > > > > > > > > > Despite this downside, however, libvirt and applications using libvirt > > > > > > has used these commands for as long as QMP has existed, via the > > > > > > "human-monitor-command" passthrough command. IOW, while it is clearly > > > > > > desirable to be able to fix the blocking problem, this is not an > > > > > > immediate obstacle to real world usage. > > > > > > > > > > > > Meanwhile there is a need for other features which involve adding new > > > > > > parameters to the commands. This is possible with HMP passthrough, but > > > > > > it provides no reliable way for apps to introspect features, so using > > > > > > QAPI modelling is highly desirable. > > > > > > > > > > > > This patch thus introduces trival savevm, loadvm, delvm commands > > > > > > > > > > trivial > > > > > > > > > > > to QMP that are functionally identical to the HMP counterpart, including > > > > > > the blocking problem. > > > > > > > > > > Should we name them 'x-savevm', 'x-loadvm', 'x-delvm' to give ourselves room > > > > > to change them when we DO solve the blocking issue? Or will the solution of > > > > > the blocking issue introduce new QMP commands, at which point we can add QMP > > > > > deprecation markers on these commands to eventually retire them? > > > > > > > > I was in two minds about this, so I'm open to arguments either way. > > > > > > > > The primary goal is for libvirt to consume the APIs as soon as possible, > > > > and generally libvirt doesn't want todo this is they are declared experimental > > > > via a "x-" prefix. So that pushes me away from "x-". > > > > > > > > If we don't have an "x-" prefix and want to make changes, we can add extra > > > > parameters to trigger new behaviour in backwards compatible manner. Or we can > > > > simply deprecate these commands, deleting them 2 releases later, while adding > > > > completely new commands. > > > > > > > > If we think the prposed design will definitely need incompatible changes in > > > > a very short time frame though, that would push towards "x-". > > > > > > > > So IMHO the right answer largely depends on whether there is a credible > > > > strategy to implement the ideal non-blocking solution in a reasonable amount > > > > of time. I can't justify spending much time on this myself, but I'm willing > > > > to consider & try proposals for solving the blocking problem if they're not > > > > too complex / invasive. > > > > > > Remind me, what was the problem with just making a block: migration > > > channel, and then we can migrate to it? > > > > migration only does vmstate, not disks. The current blockdev commands > > are all related to external snapshots, nothing for internal snapshots > > AFAIK. So we still need commands to load/save internal snapshots of > > the disk data in the qcow2 files. > > > > So we could look at loadvm/savevm conceptually as a syntax sugar above > > a migration transport that targets disk images, and blockdev QMP command > > that can do internal snapshots. Neither of these exist though and feel > > like a significantly larger amount of work than using existing functionality > > that is currently working. > > I think that's what we should aim for; adding this wrapper isn't gaining > that much without moving a bit towards that; so I would stick with the > x- for now. The question is how much work that approach will be and whether anyone can realistically commit to doing that ? It looks like a much larger piece of work in both QEMU and libvirt side to do that. I don't want to see us stuck with a x-savevm for years because no one has resource to work on the perfect solution. If we did get a perfect solution at a point in future, we can still deprecate and then remove any "savevm" command we add to QMP. Regards, Daniel
* Daniel P. Berrangé (berrange@redhat.com) wrote: > On Fri, Jul 03, 2020 at 05:10:12PM +0100, Dr. David Alan Gilbert wrote: > > * Daniel P. Berrangé (berrange@redhat.com) wrote: > > > On Fri, Jul 03, 2020 at 04:49:33PM +0100, Dr. David Alan Gilbert wrote: > > > > * Daniel P. Berrangé (berrange@redhat.com) wrote: > > > > > On Thu, Jul 02, 2020 at 01:12:52PM -0500, Eric Blake wrote: > > > > > > On 7/2/20 12:57 PM, Daniel P. Berrangé wrote: > > > > > > > savevm, loadvm and delvm are some of the few commands that have never > > > > > > > been converted to use QMP. The primary reason for this lack of > > > > > > > conversion is that they block execution of the thread for as long as > > > > > > > they run. > > > > > > > > > > > > > > Despite this downside, however, libvirt and applications using libvirt > > > > > > > has used these commands for as long as QMP has existed, via the > > > > > > > "human-monitor-command" passthrough command. IOW, while it is clearly > > > > > > > desirable to be able to fix the blocking problem, this is not an > > > > > > > immediate obstacle to real world usage. > > > > > > > > > > > > > > Meanwhile there is a need for other features which involve adding new > > > > > > > parameters to the commands. This is possible with HMP passthrough, but > > > > > > > it provides no reliable way for apps to introspect features, so using > > > > > > > QAPI modelling is highly desirable. > > > > > > > > > > > > > > This patch thus introduces trival savevm, loadvm, delvm commands > > > > > > > > > > > > trivial > > > > > > > > > > > > > to QMP that are functionally identical to the HMP counterpart, including > > > > > > > the blocking problem. > > > > > > > > > > > > Should we name them 'x-savevm', 'x-loadvm', 'x-delvm' to give ourselves room > > > > > > to change them when we DO solve the blocking issue? Or will the solution of > > > > > > the blocking issue introduce new QMP commands, at which point we can add QMP > > > > > > deprecation markers on these commands to eventually retire them? > > > > > > > > > > I was in two minds about this, so I'm open to arguments either way. > > > > > > > > > > The primary goal is for libvirt to consume the APIs as soon as possible, > > > > > and generally libvirt doesn't want todo this is they are declared experimental > > > > > via a "x-" prefix. So that pushes me away from "x-". > > > > > > > > > > If we don't have an "x-" prefix and want to make changes, we can add extra > > > > > parameters to trigger new behaviour in backwards compatible manner. Or we can > > > > > simply deprecate these commands, deleting them 2 releases later, while adding > > > > > completely new commands. > > > > > > > > > > If we think the prposed design will definitely need incompatible changes in > > > > > a very short time frame though, that would push towards "x-". > > > > > > > > > > So IMHO the right answer largely depends on whether there is a credible > > > > > strategy to implement the ideal non-blocking solution in a reasonable amount > > > > > of time. I can't justify spending much time on this myself, but I'm willing > > > > > to consider & try proposals for solving the blocking problem if they're not > > > > > too complex / invasive. > > > > > > > > Remind me, what was the problem with just making a block: migration > > > > channel, and then we can migrate to it? > > > > > > migration only does vmstate, not disks. The current blockdev commands > > > are all related to external snapshots, nothing for internal snapshots > > > AFAIK. So we still need commands to load/save internal snapshots of > > > the disk data in the qcow2 files. > > > > > > So we could look at loadvm/savevm conceptually as a syntax sugar above > > > a migration transport that targets disk images, and blockdev QMP command > > > that can do internal snapshots. Neither of these exist though and feel > > > like a significantly larger amount of work than using existing functionality > > > that is currently working. > > > > I think that's what we should aim for; adding this wrapper isn't gaining > > that much without moving a bit towards that; so I would stick with the > > x- for now. > > The question is how much work that approach will be and whether anyone can > realistically commit to doing that ? It looks like a much larger piece of > work in both QEMU and libvirt side to do that. I don't want to see us stuck > with a x-savevm for years because no one has resource to work on the perfect > solution. If we did get a perfect solution at a point in future, we can > still deprecate and then remove any "savevm" command we add to QMP. I'd at least like to understand that we've got a worklist for it though. We've already got qemu_fopen_bdrv - what's actually wrong with that - is that enough to do the migrate to a stream (given a tiny amount of syntax) ? Dave > Regards, > Daniel > -- > |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o- https://fstop138.berrange.com :| > |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Fri, Jul 03, 2020 at 17:10:12 +0100, Dr. David Alan Gilbert wrote: > * Daniel P. Berrangé (berrange@redhat.com) wrote: > > On Fri, Jul 03, 2020 at 04:49:33PM +0100, Dr. David Alan Gilbert wrote: > > > * Daniel P. Berrangé (berrange@redhat.com) wrote: > > > > On Thu, Jul 02, 2020 at 01:12:52PM -0500, Eric Blake wrote: > > > > > On 7/2/20 12:57 PM, Daniel P. Berrangé wrote: [...] > > > Remind me, what was the problem with just making a block: migration > > > channel, and then we can migrate to it? > > > > migration only does vmstate, not disks. The current blockdev commands > > are all related to external snapshots, nothing for internal snapshots > > AFAIK. So we still need commands to load/save internal snapshots of > > the disk data in the qcow2 files. > > > > So we could look at loadvm/savevm conceptually as a syntax sugar above > > a migration transport that targets disk images, and blockdev QMP command > > that can do internal snapshots. Neither of these exist though and feel > > like a significantly larger amount of work than using existing functionality > > that is currently working. > > I think that's what we should aim for; adding this wrapper isn't gaining > that much without moving a bit towards that; so I would stick with the > x- for now. Relying on the HMP variants is IMO even worse. Error handling is terrible there. I'd vote even for a straight wrapper without any logic at this point. IMO it's just necessary to document that it's an intermediate solution which WILL be deprecated and removed as soon as a suitable replacement is in place. Not doing anything is the argument we hear for multiple years now and savevm/delvm/loadvm are now the only 3 commands used via the HMP wrapper in libvirt. Since deprecation is now a thing I think we can add a disposable inteface. In the end HMP will or will not need to remain anyways and the deprecation there is IMO less clear.
* Peter Krempa (pkrempa@redhat.com) wrote: > On Fri, Jul 03, 2020 at 17:10:12 +0100, Dr. David Alan Gilbert wrote: > > * Daniel P. Berrangé (berrange@redhat.com) wrote: > > > On Fri, Jul 03, 2020 at 04:49:33PM +0100, Dr. David Alan Gilbert wrote: > > > > * Daniel P. Berrangé (berrange@redhat.com) wrote: > > > > > On Thu, Jul 02, 2020 at 01:12:52PM -0500, Eric Blake wrote: > > > > > > On 7/2/20 12:57 PM, Daniel P. Berrangé wrote: > > [...] > > > > > Remind me, what was the problem with just making a block: migration > > > > channel, and then we can migrate to it? > > > > > > migration only does vmstate, not disks. The current blockdev commands > > > are all related to external snapshots, nothing for internal snapshots > > > AFAIK. So we still need commands to load/save internal snapshots of > > > the disk data in the qcow2 files. > > > > > > So we could look at loadvm/savevm conceptually as a syntax sugar above > > > a migration transport that targets disk images, and blockdev QMP command > > > that can do internal snapshots. Neither of these exist though and feel > > > like a significantly larger amount of work than using existing functionality > > > that is currently working. > > > > I think that's what we should aim for; adding this wrapper isn't gaining > > that much without moving a bit towards that; so I would stick with the > > x- for now. > > Relying on the HMP variants is IMO even worse. Error handling is > terrible there. I'd vote even for a straight wrapper without any logic > at this point. IMO it's just necessary to document that it's an > intermediate solution which WILL be deprecated and removed as soon as a > suitable replacement is in place. > > Not doing anything is the argument we hear for multiple years now and > savevm/delvm/loadvm are now the only 3 commands used via the HMP wrapper > in libvirt. > > Since deprecation is now a thing I think we can add a disposable > inteface. In the end HMP will or will not need to remain anyways and the > deprecation there is IMO less clear. Only if we come up with a list of what we actually need to do to properly fix it; I'm not suggesting we actually need to do the work, but we should figure out what we need to do. Dave -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Fri, Jul 03, 2020 at 05:22:46PM +0100, Dr. David Alan Gilbert wrote: > * Daniel P. Berrangé (berrange@redhat.com) wrote: > > On Fri, Jul 03, 2020 at 05:10:12PM +0100, Dr. David Alan Gilbert wrote: > > > * Daniel P. Berrangé (berrange@redhat.com) wrote: > > > > On Fri, Jul 03, 2020 at 04:49:33PM +0100, Dr. David Alan Gilbert wrote: > > > > > * Daniel P. Berrangé (berrange@redhat.com) wrote: > > > > > > On Thu, Jul 02, 2020 at 01:12:52PM -0500, Eric Blake wrote: > > > > > > > On 7/2/20 12:57 PM, Daniel P. Berrangé wrote: > > > > > > > > savevm, loadvm and delvm are some of the few commands that have never > > > > > > > > been converted to use QMP. The primary reason for this lack of > > > > > > > > conversion is that they block execution of the thread for as long as > > > > > > > > they run. > > > > > > > > > > > > > > > > Despite this downside, however, libvirt and applications using libvirt > > > > > > > > has used these commands for as long as QMP has existed, via the > > > > > > > > "human-monitor-command" passthrough command. IOW, while it is clearly > > > > > > > > desirable to be able to fix the blocking problem, this is not an > > > > > > > > immediate obstacle to real world usage. > > > > > > > > > > > > > > > > Meanwhile there is a need for other features which involve adding new > > > > > > > > parameters to the commands. This is possible with HMP passthrough, but > > > > > > > > it provides no reliable way for apps to introspect features, so using > > > > > > > > QAPI modelling is highly desirable. > > > > > > > > > > > > > > > > This patch thus introduces trival savevm, loadvm, delvm commands > > > > > > > > > > > > > > trivial > > > > > > > > > > > > > > > to QMP that are functionally identical to the HMP counterpart, including > > > > > > > > the blocking problem. > > > > > > > > > > > > > > Should we name them 'x-savevm', 'x-loadvm', 'x-delvm' to give ourselves room > > > > > > > to change them when we DO solve the blocking issue? Or will the solution of > > > > > > > the blocking issue introduce new QMP commands, at which point we can add QMP > > > > > > > deprecation markers on these commands to eventually retire them? > > > > > > > > > > > > I was in two minds about this, so I'm open to arguments either way. > > > > > > > > > > > > The primary goal is for libvirt to consume the APIs as soon as possible, > > > > > > and generally libvirt doesn't want todo this is they are declared experimental > > > > > > via a "x-" prefix. So that pushes me away from "x-". > > > > > > > > > > > > If we don't have an "x-" prefix and want to make changes, we can add extra > > > > > > parameters to trigger new behaviour in backwards compatible manner. Or we can > > > > > > simply deprecate these commands, deleting them 2 releases later, while adding > > > > > > completely new commands. > > > > > > > > > > > > If we think the prposed design will definitely need incompatible changes in > > > > > > a very short time frame though, that would push towards "x-". > > > > > > > > > > > > So IMHO the right answer largely depends on whether there is a credible > > > > > > strategy to implement the ideal non-blocking solution in a reasonable amount > > > > > > of time. I can't justify spending much time on this myself, but I'm willing > > > > > > to consider & try proposals for solving the blocking problem if they're not > > > > > > too complex / invasive. > > > > > > > > > > Remind me, what was the problem with just making a block: migration > > > > > channel, and then we can migrate to it? > > > > > > > > migration only does vmstate, not disks. The current blockdev commands > > > > are all related to external snapshots, nothing for internal snapshots > > > > AFAIK. So we still need commands to load/save internal snapshots of > > > > the disk data in the qcow2 files. > > > > > > > > So we could look at loadvm/savevm conceptually as a syntax sugar above > > > > a migration transport that targets disk images, and blockdev QMP command > > > > that can do internal snapshots. Neither of these exist though and feel > > > > like a significantly larger amount of work than using existing functionality > > > > that is currently working. > > > > > > I think that's what we should aim for; adding this wrapper isn't gaining > > > that much without moving a bit towards that; so I would stick with the > > > x- for now. > > > > The question is how much work that approach will be and whether anyone can > > realistically commit to doing that ? It looks like a much larger piece of > > work in both QEMU and libvirt side to do that. I don't want to see us stuck > > with a x-savevm for years because no one has resource to work on the perfect > > solution. If we did get a perfect solution at a point in future, we can > > still deprecate and then remove any "savevm" command we add to QMP. > > I'd at least like to understand that we've got a worklist for it though. > We've already got qemu_fopen_bdrv - what's actually wrong with that - is > that enough to do the migrate to a stream (given a tiny amount of > syntax) ? It is close. The migration code works with the QEMUFile layer, but in terms of the monitor commands the current framework expects a QIOChannel based QEMUFile. It would be possible to add new helpers to work with the bdrv backed QEMUFile. The ideal would be to create a QIOChannel impl that is backed by a block device though. At that point there would only be a single QEMUFile impl based on QIOChannel. That would be another step closer to unlocking the ability to eliminate the QEMUFile wrapper entirely. QEMUFile does I/O buffering too for performance, that could be done in a QIOChannel layer too, as that's a concept useful for other QIOChannel users too. That's still only the vmstate part though, and a solution is needed for the internal snapshot handling. Regards, Daniel
* Daniel P. Berrangé (berrange@redhat.com) wrote: > On Fri, Jul 03, 2020 at 05:22:46PM +0100, Dr. David Alan Gilbert wrote: > > * Daniel P. BerrangÃÆé (berrange@redhat.com) wrote: > > > On Fri, Jul 03, 2020 at 05:10:12PM +0100, Dr. David Alan Gilbert wrote: > > > > * Daniel P. BerrangÃÆé (berrange@redhat.com) wrote: > > > > > On Fri, Jul 03, 2020 at 04:49:33PM +0100, Dr. David Alan Gilbert wrote: > > > > > > * Daniel P. BerrangÃÆé (berrange@redhat.com) wrote: > > > > > > > On Thu, Jul 02, 2020 at 01:12:52PM -0500, Eric Blake wrote: > > > > > > > > On 7/2/20 12:57 PM, Daniel P. BerrangÃÆé wrote: > > > > > > > > > savevm, loadvm and delvm are some of the few commands that have never > > > > > > > > > been converted to use QMP. The primary reason for this lack of > > > > > > > > > conversion is that they block execution of the thread for as long as > > > > > > > > > they run. > > > > > > > > > > > > > > > > > > Despite this downside, however, libvirt and applications using libvirt > > > > > > > > > has used these commands for as long as QMP has existed, via the > > > > > > > > > "human-monitor-command" passthrough command. IOW, while it is clearly > > > > > > > > > desirable to be able to fix the blocking problem, this is not an > > > > > > > > > immediate obstacle to real world usage. > > > > > > > > > > > > > > > > > > Meanwhile there is a need for other features which involve adding new > > > > > > > > > parameters to the commands. This is possible with HMP passthrough, but > > > > > > > > > it provides no reliable way for apps to introspect features, so using > > > > > > > > > QAPI modelling is highly desirable. > > > > > > > > > > > > > > > > > > This patch thus introduces trival savevm, loadvm, delvm commands > > > > > > > > > > > > > > > > trivial > > > > > > > > > > > > > > > > > to QMP that are functionally identical to the HMP counterpart, including > > > > > > > > > the blocking problem. > > > > > > > > > > > > > > > > Should we name them 'x-savevm', 'x-loadvm', 'x-delvm' to give ourselves room > > > > > > > > to change them when we DO solve the blocking issue? Or will the solution of > > > > > > > > the blocking issue introduce new QMP commands, at which point we can add QMP > > > > > > > > deprecation markers on these commands to eventually retire them? > > > > > > > > > > > > > > I was in two minds about this, so I'm open to arguments either way. > > > > > > > > > > > > > > The primary goal is for libvirt to consume the APIs as soon as possible, > > > > > > > and generally libvirt doesn't want todo this is they are declared experimental > > > > > > > via a "x-" prefix. So that pushes me away from "x-". > > > > > > > > > > > > > > If we don't have an "x-" prefix and want to make changes, we can add extra > > > > > > > parameters to trigger new behaviour in backwards compatible manner. Or we can > > > > > > > simply deprecate these commands, deleting them 2 releases later, while adding > > > > > > > completely new commands. > > > > > > > > > > > > > > If we think the prposed design will definitely need incompatible changes in > > > > > > > a very short time frame though, that would push towards "x-". > > > > > > > > > > > > > > So IMHO the right answer largely depends on whether there is a credible > > > > > > > strategy to implement the ideal non-blocking solution in a reasonable amount > > > > > > > of time. I can't justify spending much time on this myself, but I'm willing > > > > > > > to consider & try proposals for solving the blocking problem if they're not > > > > > > > too complex / invasive. > > > > > > > > > > > > Remind me, what was the problem with just making a block: migration > > > > > > channel, and then we can migrate to it? > > > > > > > > > > migration only does vmstate, not disks. The current blockdev commands > > > > > are all related to external snapshots, nothing for internal snapshots > > > > > AFAIK. So we still need commands to load/save internal snapshots of > > > > > the disk data in the qcow2 files. > > > > > > > > > > So we could look at loadvm/savevm conceptually as a syntax sugar above > > > > > a migration transport that targets disk images, and blockdev QMP command > > > > > that can do internal snapshots. Neither of these exist though and feel > > > > > like a significantly larger amount of work than using existing functionality > > > > > that is currently working. > > > > > > > > I think that's what we should aim for; adding this wrapper isn't gaining > > > > that much without moving a bit towards that; so I would stick with the > > > > x- for now. > > > > > > The question is how much work that approach will be and whether anyone can > > > realistically commit to doing that ? It looks like a much larger piece of > > > work in both QEMU and libvirt side to do that. I don't want to see us stuck > > > with a x-savevm for years because no one has resource to work on the perfect > > > solution. If we did get a perfect solution at a point in future, we can > > > still deprecate and then remove any "savevm" command we add to QMP. > > > > I'd at least like to understand that we've got a worklist for it though. > > We've already got qemu_fopen_bdrv - what's actually wrong with that - is > > that enough to do the migrate to a stream (given a tiny amount of > > syntax) ? > > It is close. The migration code works with the QEMUFile layer, but in terms > of the monitor commands the current framework expects a QIOChannel based > QEMUFile. It would be possible to add new helpers to work with the bdrv > backed QEMUFile. The ideal would be to create a QIOChannel impl that is > backed by a block device though. At that point there would only be a single > QEMUFile impl based on QIOChannel. > > That would be another step closer to unlocking the ability to eliminate the > QEMUFile wrapper entirely. QEMUFile does I/O buffering too for performance, > that could be done in a QIOChannel layer too, as that's a concept useful > for other QIOChannel users too. There's some separate patches on list to do buffering in bdrv for vmsave because apparently the qemufile ones don't play well with it. > That's still only the vmstate part though, and a solution is needed for > the internal snapshot handling. Which bit is the bit that makes it block? Dave > Regards, > Daniel > -- > |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o- https://fstop138.berrange.com :| > |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Fri, Jul 03, 2020 at 06:00:50PM +0100, Dr. David Alan Gilbert wrote: > * Daniel P. Berrangé (berrange@redhat.com) wrote: > > On Fri, Jul 03, 2020 at 05:22:46PM +0100, Dr. David Alan Gilbert wrote: > > > * Daniel P. Berrangé (berrange@redhat.com) wrote: > > > > On Fri, Jul 03, 2020 at 05:10:12PM +0100, Dr. David Alan Gilbert wrote: > > > > > * Daniel P. Berrangé (berrange@redhat.com) wrote: > > > > > > On Fri, Jul 03, 2020 at 04:49:33PM +0100, Dr. David Alan Gilbert wrote: > > > > > > > * Daniel P. Berrangé (berrange@redhat.com) wrote: > > > > > > > > On Thu, Jul 02, 2020 at 01:12:52PM -0500, Eric Blake wrote: > > > > > > > > > On 7/2/20 12:57 PM, Daniel P. Berrangé wrote: > > > > > > > > > > savevm, loadvm and delvm are some of the few commands that have never > > > > > > > > > > been converted to use QMP. The primary reason for this lack of > > > > > > > > > > conversion is that they block execution of the thread for as long as > > > > > > > > > > they run. > > > > > > > > > > > > > > > > > > > > Despite this downside, however, libvirt and applications using libvirt > > > > > > > > > > has used these commands for as long as QMP has existed, via the > > > > > > > > > > "human-monitor-command" passthrough command. IOW, while it is clearly > > > > > > > > > > desirable to be able to fix the blocking problem, this is not an > > > > > > > > > > immediate obstacle to real world usage. > > > > > > > > > > > > > > > > > > > > Meanwhile there is a need for other features which involve adding new > > > > > > > > > > parameters to the commands. This is possible with HMP passthrough, but > > > > > > > > > > it provides no reliable way for apps to introspect features, so using > > > > > > > > > > QAPI modelling is highly desirable. > > > > > > > > > > > > > > > > > > > > This patch thus introduces trival savevm, loadvm, delvm commands > > > > > > > > > > > > > > > > > > trivial > > > > > > > > > > > > > > > > > > > to QMP that are functionally identical to the HMP counterpart, including > > > > > > > > > > the blocking problem. > > > > > > > > > > > > > > > > > > Should we name them 'x-savevm', 'x-loadvm', 'x-delvm' to give ourselves room > > > > > > > > > to change them when we DO solve the blocking issue? Or will the solution of > > > > > > > > > the blocking issue introduce new QMP commands, at which point we can add QMP > > > > > > > > > deprecation markers on these commands to eventually retire them? > > > > > > > > > > > > > > > > I was in two minds about this, so I'm open to arguments either way. > > > > > > > > > > > > > > > > The primary goal is for libvirt to consume the APIs as soon as possible, > > > > > > > > and generally libvirt doesn't want todo this is they are declared experimental > > > > > > > > via a "x-" prefix. So that pushes me away from "x-". > > > > > > > > > > > > > > > > If we don't have an "x-" prefix and want to make changes, we can add extra > > > > > > > > parameters to trigger new behaviour in backwards compatible manner. Or we can > > > > > > > > simply deprecate these commands, deleting them 2 releases later, while adding > > > > > > > > completely new commands. > > > > > > > > > > > > > > > > If we think the prposed design will definitely need incompatible changes in > > > > > > > > a very short time frame though, that would push towards "x-". > > > > > > > > > > > > > > > > So IMHO the right answer largely depends on whether there is a credible > > > > > > > > strategy to implement the ideal non-blocking solution in a reasonable amount > > > > > > > > of time. I can't justify spending much time on this myself, but I'm willing > > > > > > > > to consider & try proposals for solving the blocking problem if they're not > > > > > > > > too complex / invasive. > > > > > > > > > > > > > > Remind me, what was the problem with just making a block: migration > > > > > > > channel, and then we can migrate to it? > > > > > > > > > > > > migration only does vmstate, not disks. The current blockdev commands > > > > > > are all related to external snapshots, nothing for internal snapshots > > > > > > AFAIK. So we still need commands to load/save internal snapshots of > > > > > > the disk data in the qcow2 files. > > > > > > > > > > > > So we could look at loadvm/savevm conceptually as a syntax sugar above > > > > > > a migration transport that targets disk images, and blockdev QMP command > > > > > > that can do internal snapshots. Neither of these exist though and feel > > > > > > like a significantly larger amount of work than using existing functionality > > > > > > that is currently working. > > > > > > > > > > I think that's what we should aim for; adding this wrapper isn't gaining > > > > > that much without moving a bit towards that; so I would stick with the > > > > > x- for now. > > > > > > > > The question is how much work that approach will be and whether anyone can > > > > realistically commit to doing that ? It looks like a much larger piece of > > > > work in both QEMU and libvirt side to do that. I don't want to see us stuck > > > > with a x-savevm for years because no one has resource to work on the perfect > > > > solution. If we did get a perfect solution at a point in future, we can > > > > still deprecate and then remove any "savevm" command we add to QMP. > > > > > > I'd at least like to understand that we've got a worklist for it though. > > > We've already got qemu_fopen_bdrv - what's actually wrong with that - is > > > that enough to do the migrate to a stream (given a tiny amount of > > > syntax) ? > > > > It is close. The migration code works with the QEMUFile layer, but in terms > > of the monitor commands the current framework expects a QIOChannel based > > QEMUFile. It would be possible to add new helpers to work with the bdrv > > backed QEMUFile. The ideal would be to create a QIOChannel impl that is > > backed by a block device though. At that point there would only be a single > > QEMUFile impl based on QIOChannel. > > > > That would be another step closer to unlocking the ability to eliminate the > > QEMUFile wrapper entirely. QEMUFile does I/O buffering too for performance, > > that could be done in a QIOChannel layer too, as that's a concept useful > > for other QIOChannel users too. > > There's some separate patches on list to do buffering in bdrv for > vmsave because apparently the qemufile ones don't play well with it. > > > That's still only the vmstate part though, and a solution is needed for > > the internal snapshot handling. > > Which bit is the bit that makes it block? The entire save_snapshot / load_snapshot methods really. They doing I/O operations throughout and this executes in context of the thread running the monitor, so their execution time is proportional to I/O time. Regards, Daniel
On 7/2/20 8:57 PM, Daniel P. Berrangé wrote: > savevm, loadvm and delvm are some of the few commands that have never > been converted to use QMP. The primary reason for this lack of > conversion is that they block execution of the thread for as long as > they run. > > Despite this downside, however, libvirt and applications using libvirt > has used these commands for as long as QMP has existed, via the > "human-monitor-command" passthrough command. IOW, while it is clearly > desirable to be able to fix the blocking problem, this is not an > immediate obstacle to real world usage. > > Meanwhile there is a need for other features which involve adding new > parameters to the commands. This is possible with HMP passthrough, but > it provides no reliable way for apps to introspect features, so using > QAPI modelling is highly desirable. > > This patch thus introduces trival savevm, loadvm, delvm commands > to QMP that are functionally identical to the HMP counterpart, including > the blocking problem. > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > --- > migration/savevm.c | 27 ++++++++++++++++ > monitor/hmp-cmds.c | 18 ++--------- > qapi/migration.json | 76 +++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 106 insertions(+), 15 deletions(-) > > diff --git a/migration/savevm.c b/migration/savevm.c > index 72dbad95ed..53586a6406 100644 > --- a/migration/savevm.c > +++ b/migration/savevm.c > @@ -2943,3 +2943,30 @@ bool vmstate_check_only_migratable(const VMStateDescription *vmsd) > > return !(vmsd && vmsd->unmigratable); > } > + > +void qmp_savevm(const char *tag, Error **errp) > +{ > + save_snapshot(tag, errp); > +} > + > +void qmp_loadvm(const char *tag, Error **errp) > +{ > + int saved_vm_running = runstate_is_running(); > + > + vm_stop(RUN_STATE_RESTORE_VM); > + > + if (load_snapshot(tag, errp) == 0 && saved_vm_running) { > + vm_start(); > + } > +} > + > +void qmp_delvm(const char *tag, Error **errp) > +{ > + BlockDriverState *bs; > + > + if (bdrv_all_delete_snapshot(tag, &bs, errp) < 0) { > + error_prepend(errp, > + "deleting snapshot on device '%s': ", > + bdrv_get_device_or_node_name(bs)); > + } > +} > diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c > index 2b0b58a336..26a5a1a701 100644 > --- a/monitor/hmp-cmds.c > +++ b/monitor/hmp-cmds.c > @@ -1089,15 +1089,9 @@ void hmp_balloon(Monitor *mon, const QDict *qdict) > > void hmp_loadvm(Monitor *mon, const QDict *qdict) > { > - int saved_vm_running = runstate_is_running(); > - const char *name = qdict_get_str(qdict, "name"); > Error *err = NULL; > > - vm_stop(RUN_STATE_RESTORE_VM); > - > - if (load_snapshot(name, &err) == 0 && saved_vm_running) { > - vm_start(); > - } > + qmp_loadvm(qdict_get_str(qdict, "name"), &err); > hmp_handle_error(mon, err); > } > > @@ -1105,21 +1099,15 @@ void hmp_savevm(Monitor *mon, const QDict *qdict) > { > Error *err = NULL; > > - save_snapshot(qdict_get_try_str(qdict, "name"), &err); > + qmp_savevm(qdict_get_try_str(qdict, "name"), &err); > hmp_handle_error(mon, err); > } > > void hmp_delvm(Monitor *mon, const QDict *qdict) > { > - BlockDriverState *bs; > Error *err = NULL; > - const char *name = qdict_get_str(qdict, "name"); > > - if (bdrv_all_delete_snapshot(name, &bs, &err) < 0) { > - error_prepend(&err, > - "deleting snapshot on device '%s': ", > - bdrv_get_device_name(bs)); > - } > + qmp_delvm(qdict_get_str(qdict, "name"), &err); > hmp_handle_error(mon, err); > } > > diff --git a/qapi/migration.json b/qapi/migration.json > index d5000558c6..849de38fb0 100644 > --- a/qapi/migration.json > +++ b/qapi/migration.json > @@ -1621,3 +1621,79 @@ > ## > { 'event': 'UNPLUG_PRIMARY', > 'data': { 'device-id': 'str' } } > + > +## > +# @savevm: > +# > +# Save a VM snapshot > +# > +# @tag: name of the snapshot to create. If it already > +# exists it will be replaced. > +# > +# Note that execution of the VM will be paused during the time > +# it takes to save the snapshot > +# > +# Returns: nothing > +# > +# Example: > +# > +# -> { "execute": "savevm", > +# "data": { > +# "tag": "my-snap" > +# } > +# } > +# <- { "return": { } } > +# > +# Since: 5.2 > +## > +{ 'command': 'savevm', > + 'data': { 'tag': 'str' } } > + > +## > +# @loadvm: > +# > +# Load a VM snapshot > +# > +# @tag: name of the snapshot to load. > +# > +# Returns: nothing > +# > +# Example: > +# > +# -> { "execute": "loadvm", > +# "data": { > +# "tag": "my-snap" > +# } > +# } > +# <- { "return": { } } > +# > +# Since: 5.2 > +## > +{ 'command': 'loadvm', > + 'data': { 'tag': 'str' } } > + > +## > +# @delvm: > +# > +# Delete a VM snapshot > +# > +# @tag: name of the snapshot to delete. > +# > +# Note that execution of the VM will be paused during the time > +# it takes to delete the snapshot > +# > +# Returns: nothing > +# > +# Example: > +# > +# -> { "execute": "delvm", > +# "data": { > +# "tag": "my-snap" > +# } > +# } > +# <- { "return": { } } > +# > +# Since: 5.2 > +## > +{ 'command': 'delvm', > + 'data': { 'tag': 'str' } } The interface proposed is not good from the interface point of view. The problem is that savevm and loadvm are VERY lengthy operations especially for VM with big amount of RAM. Thus they can run for hours f.e. with 1 Tb RAM. This "feature" was available in the old legacy HMP interface, but this should be fixed in the new and clear one. Den
* Daniel P. Berrangé (berrange@redhat.com) wrote: > On Fri, Jul 03, 2020 at 06:00:50PM +0100, Dr. David Alan Gilbert wrote: > > * Daniel P. Berrangé (berrange@redhat.com) wrote: > > > On Fri, Jul 03, 2020 at 05:22:46PM +0100, Dr. David Alan Gilbert wrote: > > > > * Daniel P. BerrangÃÆé (berrange@redhat.com) wrote: > > > > > On Fri, Jul 03, 2020 at 05:10:12PM +0100, Dr. David Alan Gilbert wrote: > > > > > > * Daniel P. BerrangÃÆé (berrange@redhat.com) wrote: > > > > > > > On Fri, Jul 03, 2020 at 04:49:33PM +0100, Dr. David Alan Gilbert wrote: > > > > > > > > * Daniel P. BerrangÃÆé (berrange@redhat.com) wrote: > > > > > > > > > On Thu, Jul 02, 2020 at 01:12:52PM -0500, Eric Blake wrote: > > > > > > > > > > On 7/2/20 12:57 PM, Daniel P. BerrangÃÆé wrote: > > > > > > > > > > > savevm, loadvm and delvm are some of the few commands that have never > > > > > > > > > > > been converted to use QMP. The primary reason for this lack of > > > > > > > > > > > conversion is that they block execution of the thread for as long as > > > > > > > > > > > they run. > > > > > > > > > > > > > > > > > > > > > > Despite this downside, however, libvirt and applications using libvirt > > > > > > > > > > > has used these commands for as long as QMP has existed, via the > > > > > > > > > > > "human-monitor-command" passthrough command. IOW, while it is clearly > > > > > > > > > > > desirable to be able to fix the blocking problem, this is not an > > > > > > > > > > > immediate obstacle to real world usage. > > > > > > > > > > > > > > > > > > > > > > Meanwhile there is a need for other features which involve adding new > > > > > > > > > > > parameters to the commands. This is possible with HMP passthrough, but > > > > > > > > > > > it provides no reliable way for apps to introspect features, so using > > > > > > > > > > > QAPI modelling is highly desirable. > > > > > > > > > > > > > > > > > > > > > > This patch thus introduces trival savevm, loadvm, delvm commands > > > > > > > > > > > > > > > > > > > > trivial > > > > > > > > > > > > > > > > > > > > > to QMP that are functionally identical to the HMP counterpart, including > > > > > > > > > > > the blocking problem. > > > > > > > > > > > > > > > > > > > > Should we name them 'x-savevm', 'x-loadvm', 'x-delvm' to give ourselves room > > > > > > > > > > to change them when we DO solve the blocking issue? Or will the solution of > > > > > > > > > > the blocking issue introduce new QMP commands, at which point we can add QMP > > > > > > > > > > deprecation markers on these commands to eventually retire them? > > > > > > > > > > > > > > > > > > I was in two minds about this, so I'm open to arguments either way. > > > > > > > > > > > > > > > > > > The primary goal is for libvirt to consume the APIs as soon as possible, > > > > > > > > > and generally libvirt doesn't want todo this is they are declared experimental > > > > > > > > > via a "x-" prefix. So that pushes me away from "x-". > > > > > > > > > > > > > > > > > > If we don't have an "x-" prefix and want to make changes, we can add extra > > > > > > > > > parameters to trigger new behaviour in backwards compatible manner. Or we can > > > > > > > > > simply deprecate these commands, deleting them 2 releases later, while adding > > > > > > > > > completely new commands. > > > > > > > > > > > > > > > > > > If we think the prposed design will definitely need incompatible changes in > > > > > > > > > a very short time frame though, that would push towards "x-". > > > > > > > > > > > > > > > > > > So IMHO the right answer largely depends on whether there is a credible > > > > > > > > > strategy to implement the ideal non-blocking solution in a reasonable amount > > > > > > > > > of time. I can't justify spending much time on this myself, but I'm willing > > > > > > > > > to consider & try proposals for solving the blocking problem if they're not > > > > > > > > > too complex / invasive. > > > > > > > > > > > > > > > > Remind me, what was the problem with just making a block: migration > > > > > > > > channel, and then we can migrate to it? > > > > > > > > > > > > > > migration only does vmstate, not disks. The current blockdev commands > > > > > > > are all related to external snapshots, nothing for internal snapshots > > > > > > > AFAIK. So we still need commands to load/save internal snapshots of > > > > > > > the disk data in the qcow2 files. > > > > > > > > > > > > > > So we could look at loadvm/savevm conceptually as a syntax sugar above > > > > > > > a migration transport that targets disk images, and blockdev QMP command > > > > > > > that can do internal snapshots. Neither of these exist though and feel > > > > > > > like a significantly larger amount of work than using existing functionality > > > > > > > that is currently working. > > > > > > > > > > > > I think that's what we should aim for; adding this wrapper isn't gaining > > > > > > that much without moving a bit towards that; so I would stick with the > > > > > > x- for now. > > > > > > > > > > The question is how much work that approach will be and whether anyone can > > > > > realistically commit to doing that ? It looks like a much larger piece of > > > > > work in both QEMU and libvirt side to do that. I don't want to see us stuck > > > > > with a x-savevm for years because no one has resource to work on the perfect > > > > > solution. If we did get a perfect solution at a point in future, we can > > > > > still deprecate and then remove any "savevm" command we add to QMP. > > > > > > > > I'd at least like to understand that we've got a worklist for it though. > > > > We've already got qemu_fopen_bdrv - what's actually wrong with that - is > > > > that enough to do the migrate to a stream (given a tiny amount of > > > > syntax) ? > > > > > > It is close. The migration code works with the QEMUFile layer, but in terms > > > of the monitor commands the current framework expects a QIOChannel based > > > QEMUFile. It would be possible to add new helpers to work with the bdrv > > > backed QEMUFile. The ideal would be to create a QIOChannel impl that is > > > backed by a block device though. At that point there would only be a single > > > QEMUFile impl based on QIOChannel. > > > > > > That would be another step closer to unlocking the ability to eliminate the > > > QEMUFile wrapper entirely. QEMUFile does I/O buffering too for performance, > > > that could be done in a QIOChannel layer too, as that's a concept useful > > > for other QIOChannel users too. > > > > There's some separate patches on list to do buffering in bdrv for > > vmsave because apparently the qemufile ones don't play well with it. > > > > > That's still only the vmstate part though, and a solution is needed for > > > the internal snapshot handling. > > > > Which bit is the bit that makes it block? > > The entire save_snapshot / load_snapshot methods really. They doing I/O > operations throughout and this executes in context of the thread running > the monitor, so their execution time is proportional to I/O time. Is there any reason for the migrationy bit to run synchronously then? Could those snapshotting things be done with any of the block copy processes that run asynchronously? Dave > > Regards, > Daniel > -- > |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o- https://fstop138.berrange.com :| > |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Am 03.07.2020 um 18:02 hat Daniel P. Berrangé geschrieben: > On Fri, Jul 03, 2020 at 04:49:33PM +0100, Dr. David Alan Gilbert wrote: > > * Daniel P. Berrangé (berrange@redhat.com) wrote: > > > On Thu, Jul 02, 2020 at 01:12:52PM -0500, Eric Blake wrote: > > > > On 7/2/20 12:57 PM, Daniel P. Berrangé wrote: > > > > > savevm, loadvm and delvm are some of the few commands that have never > > > > > been converted to use QMP. The primary reason for this lack of > > > > > conversion is that they block execution of the thread for as long as > > > > > they run. > > > > > > > > > > Despite this downside, however, libvirt and applications using libvirt > > > > > has used these commands for as long as QMP has existed, via the > > > > > "human-monitor-command" passthrough command. IOW, while it is clearly > > > > > desirable to be able to fix the blocking problem, this is not an > > > > > immediate obstacle to real world usage. > > > > > > > > > > Meanwhile there is a need for other features which involve adding new > > > > > parameters to the commands. This is possible with HMP passthrough, but > > > > > it provides no reliable way for apps to introspect features, so using > > > > > QAPI modelling is highly desirable. > > > > > > > > > > This patch thus introduces trival savevm, loadvm, delvm commands > > > > > > > > trivial > > > > > > > > > to QMP that are functionally identical to the HMP counterpart, including > > > > > the blocking problem. > > > > > > > > Should we name them 'x-savevm', 'x-loadvm', 'x-delvm' to give ourselves room > > > > to change them when we DO solve the blocking issue? Or will the solution of > > > > the blocking issue introduce new QMP commands, at which point we can add QMP > > > > deprecation markers on these commands to eventually retire them? > > > > > > I was in two minds about this, so I'm open to arguments either way. > > > > > > The primary goal is for libvirt to consume the APIs as soon as possible, > > > and generally libvirt doesn't want todo this is they are declared experimental > > > via a "x-" prefix. So that pushes me away from "x-". > > > > > > If we don't have an "x-" prefix and want to make changes, we can add extra > > > parameters to trigger new behaviour in backwards compatible manner. Or we can > > > simply deprecate these commands, deleting them 2 releases later, while adding > > > completely new commands. > > > > > > If we think the prposed design will definitely need incompatible changes in > > > a very short time frame though, that would push towards "x-". > > > > > > So IMHO the right answer largely depends on whether there is a credible > > > strategy to implement the ideal non-blocking solution in a reasonable amount > > > of time. I can't justify spending much time on this myself, but I'm willing > > > to consider & try proposals for solving the blocking problem if they're not > > > too complex / invasive. > > > > Remind me, what was the problem with just making a block: migration > > channel, and then we can migrate to it? For normal migration, the VM is still running and changing its state, so you would have to wait until migration completes and then snapshot the disk. This is possible, but it would store redundant data and the snapshot would be at an arbitrary point after actually receiving the snapshot command, which is probably not what users expect. So what you really want is some kind of postcopy migration inside the same process. Virtuozzo had a look at this some years ago and when the discussion comes up again, they say they are still kind of interested, though it's not their priority. I have never seen the actual code, but I imagine it's not trivial (unless the migration code already supports something like this today, but I'm not aware of another use case for it, so probably not?) > migration only does vmstate, not disks. The current blockdev commands > are all related to external snapshots, nothing for internal snapshots > AFAIK. So we still need commands to load/save internal snapshots of > the disk data in the qcow2 files. > > So we could look at loadvm/savevm conceptually as a syntax sugar above > a migration transport that targets disk images, and blockdev QMP command > that can do internal snapshots. Neither of these exist though and feel > like a significantly larger amount of work than using existing functionality > that is currently working. There is blockdev-snapshot-internal-sync, which does a disk-only snapshot of a single node. A snapshot of multiple nodes can be taken by putting multiple blockdev-snapshot-internal-sync inside a 'transaction' command. If we want to build on top of this, we'd have to implement a transactionable command for storing only the VM state to a specific node. This would probably still be a long-running job. So some of it does exist, but I'm torn on whether separating things into different commands is actually a good idea or not. Kevin
On Mon, Jul 06, 2020 at 18:15:55 +0200, Kevin Wolf wrote: > Am 03.07.2020 um 18:02 hat Daniel P. Berrangé geschrieben: > > On Fri, Jul 03, 2020 at 04:49:33PM +0100, Dr. David Alan Gilbert wrote: > > > * Daniel P. Berrangé (berrange@redhat.com) wrote: > > > > On Thu, Jul 02, 2020 at 01:12:52PM -0500, Eric Blake wrote: > > > > > On 7/2/20 12:57 PM, Daniel P. Berrangé wrote: [...] > > migration only does vmstate, not disks. The current blockdev commands > > are all related to external snapshots, nothing for internal snapshots > > AFAIK. So we still need commands to load/save internal snapshots of > > the disk data in the qcow2 files. > > > > So we could look at loadvm/savevm conceptually as a syntax sugar above > > a migration transport that targets disk images, and blockdev QMP command > > that can do internal snapshots. Neither of these exist though and feel > > like a significantly larger amount of work than using existing functionality > > that is currently working. > > There is blockdev-snapshot-internal-sync, which does a disk-only > snapshot of a single node. A snapshot of multiple nodes can be taken by > putting multiple blockdev-snapshot-internal-sync inside a 'transaction' > command. Libvirt never implemented support for disk-only internal snapshots as I didn't think they are worth it. We also made a mistake by using the VIR_DOMAIN_SNAPSHOT_DISK_ONLY to switch to an external snapshot, so while the XML can modify the snapshot to be internal it's not very clear nor user-friendly to force an internal disk only snapshot. > If we want to build on top of this, we'd have to implement a > transactionable command for storing only the VM state to a specific > node. This would probably still be a long-running job. IMO we really want this also for external snapshots. Driving the migration as standard migration is really suboptimal especially when the user wants minimal downtime. Transactioning a post-copy style copy-on write migration would simplify this a lot. I agree though that this is for a different conversation.
Am 07.07.2020 um 08:38 hat Peter Krempa geschrieben: > On Mon, Jul 06, 2020 at 18:15:55 +0200, Kevin Wolf wrote: > > Am 03.07.2020 um 18:02 hat Daniel P. Berrangé geschrieben: > > > On Fri, Jul 03, 2020 at 04:49:33PM +0100, Dr. David Alan Gilbert wrote: > > > > * Daniel P. Berrangé (berrange@redhat.com) wrote: > > > > > On Thu, Jul 02, 2020 at 01:12:52PM -0500, Eric Blake wrote: > > > > > > On 7/2/20 12:57 PM, Daniel P. Berrangé wrote: > > [...] > > > > migration only does vmstate, not disks. The current blockdev commands > > > are all related to external snapshots, nothing for internal snapshots > > > AFAIK. So we still need commands to load/save internal snapshots of > > > the disk data in the qcow2 files. > > > > > > So we could look at loadvm/savevm conceptually as a syntax sugar above > > > a migration transport that targets disk images, and blockdev QMP command > > > that can do internal snapshots. Neither of these exist though and feel > > > like a significantly larger amount of work than using existing functionality > > > that is currently working. > > > > There is blockdev-snapshot-internal-sync, which does a disk-only > > snapshot of a single node. A snapshot of multiple nodes can be taken by > > putting multiple blockdev-snapshot-internal-sync inside a 'transaction' > > command. > > Libvirt never implemented support for disk-only internal snapshots as I > didn't think they are worth it. We also made a mistake by using the > VIR_DOMAIN_SNAPSHOT_DISK_ONLY to switch to an external snapshot, so > while the XML can modify the snapshot to be internal it's not very clear > nor user-friendly to force an internal disk only snapshot. > > > If we want to build on top of this, we'd have to implement a > > transactionable command for storing only the VM state to a specific > > node. This would probably still be a long-running job. > > IMO we really want this also for external snapshots. Driving the > migration as standard migration is really suboptimal especially when the > user wants minimal downtime. Transactioning a post-copy style copy-on > write migration would simplify this a lot. I agree though that this is > for a different conversation. This is an interesting point actually. And while the implementation of the post-copy style live snapshotting is for a different conversation, I think the implications it has on the API are relevant for us now. But even if we have an all-in-one snapshot job instead of a transaction to group all the individual operations together, I think you could still represent that by just specifying an empty list of nodes to be snapshotted. (I feel this is another argument for passing the nodes to include rather than nodes to exclude from the disk snapshot.) Kevin
On Tue, Jul 07, 2020 at 12:33:31 +0200, Kevin Wolf wrote: > Am 07.07.2020 um 08:38 hat Peter Krempa geschrieben: > > On Mon, Jul 06, 2020 at 18:15:55 +0200, Kevin Wolf wrote: > > > Am 03.07.2020 um 18:02 hat Daniel P. Berrangé geschrieben: [...] > > IMO we really want this also for external snapshots. Driving the > > migration as standard migration is really suboptimal especially when the > > user wants minimal downtime. Transactioning a post-copy style copy-on > > write migration would simplify this a lot. I agree though that this is > > for a different conversation. > > This is an interesting point actually. And while the implementation of > the post-copy style live snapshotting is for a different conversation, I > think the implications it has on the API are relevant for us now. > > But even if we have an all-in-one snapshot job instead of a transaction > to group all the individual operations together, I think you could still > represent that by just specifying an empty list of nodes to be > snapshotted. (I feel this is another argument for passing the nodes to > include rather than nodes to exclude from the disk snapshot.) Definitely. From libvirt's POV it's IMO simpler and more future-proof to enumerate everything rather than keep a database of what to skip.
diff --git a/migration/savevm.c b/migration/savevm.c index 72dbad95ed..53586a6406 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -2943,3 +2943,30 @@ bool vmstate_check_only_migratable(const VMStateDescription *vmsd) return !(vmsd && vmsd->unmigratable); } + +void qmp_savevm(const char *tag, Error **errp) +{ + save_snapshot(tag, errp); +} + +void qmp_loadvm(const char *tag, Error **errp) +{ + int saved_vm_running = runstate_is_running(); + + vm_stop(RUN_STATE_RESTORE_VM); + + if (load_snapshot(tag, errp) == 0 && saved_vm_running) { + vm_start(); + } +} + +void qmp_delvm(const char *tag, Error **errp) +{ + BlockDriverState *bs; + + if (bdrv_all_delete_snapshot(tag, &bs, errp) < 0) { + error_prepend(errp, + "deleting snapshot on device '%s': ", + bdrv_get_device_or_node_name(bs)); + } +} diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c index 2b0b58a336..26a5a1a701 100644 --- a/monitor/hmp-cmds.c +++ b/monitor/hmp-cmds.c @@ -1089,15 +1089,9 @@ void hmp_balloon(Monitor *mon, const QDict *qdict) void hmp_loadvm(Monitor *mon, const QDict *qdict) { - int saved_vm_running = runstate_is_running(); - const char *name = qdict_get_str(qdict, "name"); Error *err = NULL; - vm_stop(RUN_STATE_RESTORE_VM); - - if (load_snapshot(name, &err) == 0 && saved_vm_running) { - vm_start(); - } + qmp_loadvm(qdict_get_str(qdict, "name"), &err); hmp_handle_error(mon, err); } @@ -1105,21 +1099,15 @@ void hmp_savevm(Monitor *mon, const QDict *qdict) { Error *err = NULL; - save_snapshot(qdict_get_try_str(qdict, "name"), &err); + qmp_savevm(qdict_get_try_str(qdict, "name"), &err); hmp_handle_error(mon, err); } void hmp_delvm(Monitor *mon, const QDict *qdict) { - BlockDriverState *bs; Error *err = NULL; - const char *name = qdict_get_str(qdict, "name"); - if (bdrv_all_delete_snapshot(name, &bs, &err) < 0) { - error_prepend(&err, - "deleting snapshot on device '%s': ", - bdrv_get_device_name(bs)); - } + qmp_delvm(qdict_get_str(qdict, "name"), &err); hmp_handle_error(mon, err); } diff --git a/qapi/migration.json b/qapi/migration.json index d5000558c6..849de38fb0 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -1621,3 +1621,79 @@ ## { 'event': 'UNPLUG_PRIMARY', 'data': { 'device-id': 'str' } } + +## +# @savevm: +# +# Save a VM snapshot +# +# @tag: name of the snapshot to create. If it already +# exists it will be replaced. +# +# Note that execution of the VM will be paused during the time +# it takes to save the snapshot +# +# Returns: nothing +# +# Example: +# +# -> { "execute": "savevm", +# "data": { +# "tag": "my-snap" +# } +# } +# <- { "return": { } } +# +# Since: 5.2 +## +{ 'command': 'savevm', + 'data': { 'tag': 'str' } } + +## +# @loadvm: +# +# Load a VM snapshot +# +# @tag: name of the snapshot to load. +# +# Returns: nothing +# +# Example: +# +# -> { "execute": "loadvm", +# "data": { +# "tag": "my-snap" +# } +# } +# <- { "return": { } } +# +# Since: 5.2 +## +{ 'command': 'loadvm', + 'data': { 'tag': 'str' } } + +## +# @delvm: +# +# Delete a VM snapshot +# +# @tag: name of the snapshot to delete. +# +# Note that execution of the VM will be paused during the time +# it takes to delete the snapshot +# +# Returns: nothing +# +# Example: +# +# -> { "execute": "delvm", +# "data": { +# "tag": "my-snap" +# } +# } +# <- { "return": { } } +# +# Since: 5.2 +## +{ 'command': 'delvm', + 'data': { 'tag': 'str' } }
savevm, loadvm and delvm are some of the few commands that have never been converted to use QMP. The primary reason for this lack of conversion is that they block execution of the thread for as long as they run. Despite this downside, however, libvirt and applications using libvirt has used these commands for as long as QMP has existed, via the "human-monitor-command" passthrough command. IOW, while it is clearly desirable to be able to fix the blocking problem, this is not an immediate obstacle to real world usage. Meanwhile there is a need for other features which involve adding new parameters to the commands. This is possible with HMP passthrough, but it provides no reliable way for apps to introspect features, so using QAPI modelling is highly desirable. This patch thus introduces trival savevm, loadvm, delvm commands to QMP that are functionally identical to the HMP counterpart, including the blocking problem. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- migration/savevm.c | 27 ++++++++++++++++ monitor/hmp-cmds.c | 18 ++--------- qapi/migration.json | 76 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 106 insertions(+), 15 deletions(-)