[libvirt] [PATCH v2 3/5] qemu: Track domain quiesced status

[Tomoki Sekiyama]

>On Wed, Mar 05, 2014 at 02:53:10PM -0500, Tomoki Sekiyama wrote:
>> Adds an quiesced flag into qemuDomainObjPrivate that tracks whether guest
>> filesystems of the domain is quiesced of not.
>> It also modify error code from qemuDomainSnapshotFSFreeze and
>> qemuDomainSnapshotFSThaw, so that a caller can know whether the command is
>> actually sent to the guest agent. If the error is caused before sending a
>> freeze command, a counterpart thaw command shouldn't be sent to avoid
>> confusing the tracking of quiesced status.
>> ---
>>  src/qemu/qemu_domain.h |    2 ++
>>  src/qemu/qemu_driver.c |   41 +++++++++++++++++++++++++++++++----------
>>  2 files changed, 33 insertions(+), 10 deletions(-)
>>
>> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
>> index 0bed50b..8a79a00 100644
>> --- a/src/qemu/qemu_domain.h
>> +++ b/src/qemu/qemu_domain.h
>> @@ -176,6 +176,8 @@ struct _qemuDomainObjPrivate {
>>      char **qemuDevices; /* NULL-terminated list of devices aliases known to QEMU */
>>
>>      bool hookRun;  /* true if there was a hook run over this domain */
>> +
v> +    bool quiesced; /* true if the domain filesystems are quiesced */
>>  };
>>
>>  typedef enum {
> 
> You will also want to update qemuDomainObjPrivateXMLFormat and
> qemuDomainObjPrivateXMLParse so that this new 'quiesced' attribute
> is preserved in the live XML state across libvirtd restarts.

OK, I will update them too.

>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index 9aad2dc..8109e46 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -12040,6 +12040,8 @@ cleanup:
>>  }
>>
>>
>> +/* Return -1 if request is not sent to agent due to misconfig, -2 on agent
>> + * failure, and number of frozen filesystems on success. */
>>  static int
>>  qemuDomainSnapshotFSFreeze(virDomainObjPtr vm) {
>>      qemuDomainObjPrivatePtr priv = vm->privateData;
>> @@ -12056,14 +12058,24 @@ qemuDomainSnapshotFSFreeze(virDomainObjPtr vm) {
>>                         _("QEMU guest agent is not configured"));
>>          return -1;
>>      }
>> +    if (priv->quiesced) {
>> +        virReportError(VIR_ERR_OPERATION_INVALID, "%s",
>> +                       _("domain is already quiesced"));
>> +        return -1;
>> +    }
>>
>>      qemuDomainObjEnterAgent(vm);
>>      freezed = qemuAgentFSFreeze(priv->agent);
>>      qemuDomainObjExitAgent(vm);
>>
>> -    return freezed;
>> +    if (freezed >= 0)
>> +        priv->quiesced = true;
>> +
>> +    return freezed < 0 ? -2 : freezed;
>>  }
>>
>> +/* Return -1 if request is not sent to agent due to misconfig, -2 on agent
>> + * failure, and number of thawed filesystems on success. */
>>  static int
>>  qemuDomainSnapshotFSThaw(virDomainObjPtr vm, bool report)
>>  {
>> @@ -12084,6 +12096,11 @@ qemuDomainSnapshotFSThaw(virDomainObjPtr vm, bool report)
>>                             _("QEMU guest agent is not configured"));
>>          return -1;
>>      }
>> +    if (!priv->quiesced && report) {
>> +        virReportError(VIR_ERR_OPERATION_INVALID, "%s",
>> +                       _("domain is not quiesced"));
>> +        return -1;
>> +    }
>>
>>      qemuDomainObjEnterAgent(vm);
>>      if (!report)
>> @@ -12093,8 +12110,11 @@ qemuDomainSnapshotFSThaw(virDomainObjPtr vm, bool report)
>>          virSetError(err);
>>      qemuDomainObjExitAgent(vm);
>>
>> +    if (thawed >= 0)
>> +        priv->quiesced = false;
>> +
>>      virFreeError(err);
>> -    return thawed;
>> +    return thawed < 0 ? -2 : thawed;
>>  }
>
> And in both these methods you want to call virDomainSaveStatus after changing
> the priv->quiesced flag, to save the status XML to disk

OK. Thanks for pointing this out.

>>
>>  /* The domain is expected to be locked and inactive. */
>> @@ -13069,17 +13089,18 @@ qemuDomainSnapshotCreateActiveExternal(virConnectPtr conn,
>>          goto cleanup;
>>
>>      /* If quiesce was requested, then issue a freeze command, and a
>> -     * counterpart thaw command, no matter what.  The command will
>> -     * fail if the guest is paused or the guest agent is not
>> -     * running.  */
>> +     * counterpart thaw command when the it is actually sent to agent.
>> +     * The command will fail if the guest is paused or the guest agent is not
>> +     * running, or is already quiesced.  */
>>      if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE) {
>> -        if (qemuDomainSnapshotFSFreeze(vm) < 0) {
>> -            /* helper reported the error */
>> -            thaw = -1;
>> +        int freeze = qemuDomainSnapshotFSFreeze(vm);
>> +        if (freeze < 0) {
>> +            /* the helper reported the error */
>> +            if (freeze != -1)
>> +                thaw = -1; /* the command is sent but agent failed */
>>              goto endjob;
>> -        } else {
>> -            thaw = 1;
>>          }
>> +        thaw = 1;
>>      }
>
> I'm not sure I understand why you're changing this ? Are you trying to
> deal with the case where the guest is already frozen, but someone has
> still supplied the VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE flag ?

Right.
Without this change, if VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE
flag is supplied for a frozen guest:
  1. the caller will get 'domain is already quiesced' error
  2. the guest is thawed by the error handling (because of 'thaw = 1;' )

1 is expected behavior (error reporting to the caller), but 2 would be hardly
expected from a user's viewpoint. The intention of this change is to avoid 2.

> I'd probably argue that such usage is a bug we should report to the
> caller as an error.


Thanks,
Tomoki Sekiyama