[libvirt] [PATCH 7/7] qemu: snapshot: Correctly revert snapshots in PMSUSPENDED state

[Peter Krempa]

On 07/19/14 01:12, Eric Blake wrote:
> On 07/18/2014 10:11 AM, Peter Krempa wrote:
>> PMSUSPENDED state implies the qemu process running, so we should treat
>> it that way. This increases the possible state space of transitions that
>> may happen during the snapshot revert so this patch documents them as
>> well.
>>
>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1079162
>> ---
>>  src/qemu/qemu_driver.c | 67 ++++++++++++++++++++++++++++++++++++--------------
>>  1 file changed, 48 insertions(+), 19 deletions(-)
> 
> I think this needs some rework - current qemu treats it as reverting to
> a pm wakeup event, and the guest will start running again (unless the
> user requested to revert as paused).
> 
>>
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index 1f98f4a..f93e0fd 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -14052,22 +14052,29 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
>>      virDomainDefPtr config = NULL;
>>      virQEMUDriverConfigPtr cfg = NULL;
>>      virCapsPtr caps = NULL;
>> -    int oldState = vm->state.state;
>> +    int oldState;
>>
>>      virCheckFlags(VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING |
>>                    VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED |
>>                    VIR_DOMAIN_SNAPSHOT_REVERT_FORCE, -1);
>>
>>      /* We have the following transitions, which create the following events:
>> -     * 1. inactive -> inactive: none
>> -     * 2. inactive -> running:  EVENT_STARTED
>> -     * 3. inactive -> paused:   EVENT_STARTED, EVENT_PAUSED
>> -     * 4. running  -> inactive: EVENT_STOPPED
>> -     * 5. running  -> running:  none
>> -     * 6. running  -> paused:   EVENT_PAUSED
>> -     * 7. paused   -> inactive: EVENT_STOPPED
>> -     * 8. paused   -> running:  EVENT_RESUMED
>> -     * 9. paused   -> paused:   none
>> +     * 1.  inactive    -> inactive:    none
>> +     * 2.  inactive    -> running:     EVENT_STARTED
>> +     * 3.  inactive    -> paused:      EVENT_STARTED, EVENT_PAUSED
>> +     * 4.  running     -> inactive:    EVENT_STOPPED
>> +     * 5.  running     -> running:     none
>> +     * 6.  running     -> paused:      EVENT_PAUSED
>> +     * 7.  paused      -> inactive:    EVENT_STOPPED
>> +     * 8.  paused      -> running:     EVENT_RESUMED
>> +     * 9.  paused      -> paused:      none
>> +     * 10. pmsuspended -> pmsuspended: none
> 
> this state is not possible with current qemu
> 
>> +     * 11. pmsuspended -> inactive:    EVENT_STOPPED
>> +     * 12. pmsuspended -> running:     EVENT_RESUMED
>> +     * 13. pmsuspended -> paused:      EVENT_PAUSED
>> +     * 14. inactive    -> pmsuspended: EVENT_STARTED, EVENT_PMSUSPENDED
>> +     * 15. paused      -> pmsuspended: EVENT_PMSUSPENDED
>> +     * 16. running     -> pmsuspended: EVENT_PMSUSPENDED
> 
> and these three states are not possible
> 
>>       * Also, several transitions occur even if we fail partway through,
>>       * and use of FORCE can cause multiple transitions.
>>       */
>> @@ -14075,6 +14082,8 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
>>      if (!(vm = qemuDomObjFromSnapshot(snapshot)))
>>          return -1;
>>
>> +    oldState = vm->state.state;
>> +
>>      cfg = virQEMUDriverGetConfig(driver);
>>
>>      if (virDomainRevertToSnapshotEnsureACL(snapshot->domain->conn, vm->def) < 0)
>> @@ -14127,6 +14136,14 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
>>          }
>>      }
>>
>> +    if (snap->def->state == VIR_DOMAIN_PMSUSPENDED &&
>> +        (flags & VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING ||
>> +         flags & VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED)) {
>> +        virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
>> +                       _("snapshot of a VM in PMSUSPENDED state cannot be "
>> +                         "reverted to a running or paused state"));
> 
> actually, I think that we need to tackle it differently - we can't
> CREATE a snapshot in the pmsuspended state (just as we can't migrate in
> that state - the mere act of migration is a wakeup event).  So, if you
> eliminate all snapshots in the pmsuspended state as invalid, the set of
> state transitions is smaller.
> 
> 

I've posted a series that forbids snapshot of pmsuspended guests. My
initiative to post this full rework was so that libvirt would do the
right thing right after the problem in qemu is fixed. With forbidding of
the snapshot we'll need to re-visit this stuff once that qemu is fixed.

Peter


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 884 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140721/d516b995/attachment-0001.sig>