Re: [libvirt] [PATCH v5 1/4] blockcopy: add more XML for state tracking
On 07/29/2014 05:20 AM, Peter Krempa wrote:
> On 07/29/14 06:20, Eric Blake wrote:
>> Doing a blockcopy operation across a libvirtd restart is not very
>> robust at the moment. In particular, we are clearing the
>> element prior to telling qemu to finish the job; and thanks to the
>> ability to request async completion, the user can easily regain
>> control prior to qemu actually finishing the effort, and they should
>> be able to poll the domain XML to see if the job is still going.
>>
>>
>> +typedef enum {
>> +VIR_DOMAIN_DISK_MIRROR_DEFAULT = 0, /* No job, or job still not synced
>> */
>
> I'd go with _NONE here and ...
>
>> +VIR_DOMAIN_DISK_MIRROR_READY, /* Job in second phase */
>> +VIR_DOMAIN_DISK_MIRROR_ABORT, /* Job aborted, waiting for event */
>> +VIR_DOMAIN_DISK_MIRROR_PIVOT, /* Job pivoted, waiting for event */
>> +
>> +VIR_DOMAIN_DISK_MIRROR_LAST
>> +} virDomainDiskMirror;
>
> ... as this describes the ready state of the mirroring operation I'd go
> with virDomainDiskMirrorState and rename those fields accordingly.
> Without that it's not clear what the enum describes.
>
> Otherwise looks good. ACK if you change the enum name and default value
> name.
I'm squashing this in, and pushing.
diff --git i/src/conf/domain_conf.c w/src/conf/domain_conf.c
index 1c8f8cf..c2a81f1 100644
--- i/src/conf/domain_conf.c
+++ w/src/conf/domain_conf.c
@@ -747,8 +747,8 @@ VIR_ENUM_IMPL(virDomainDiskDiscard,
VIR_DOMAIN_DISK_DISCARD_LAST,
"unmap",
"ignore")
-VIR_ENUM_IMPL(virDomainDiskMirror, VIR_DOMAIN_DISK_MIRROR_LAST,
- "default",
+VIR_ENUM_IMPL(virDomainDiskMirrorState, VIR_DOMAIN_DISK_MIRROR_STATE_LAST,
+ "none",
"yes",
"abort",
"pivot")
@@ -5488,8 +5488,8 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
}
ready = virXMLPropString(cur, "ready");
if (ready) {
-def->mirrorState =
virDomainDiskMirrorTypeFromString(ready);
-if (def->mirrorState < 0) {
+if ((def->mirrorState =
+ virDomainDiskMirrorStateTypeFromString(ready))
< 0) {
virReportError(VIR_ERR_XML_ERROR,
_("unknown mirror ready state %s"),
ready);
@@ -15289,7 +15289,7 @@ virDomainDiskDefFormat(virBufferPtr buf,
if (def->mirrorState) {
const char *mirror;
-mirror = virDomainDiskMirrorTypeToString(def->mirrorState);
+mirror =
virDomainDiskMirrorStateTypeToString(def->mirrorState);
virBufferEscapeString(buf, " ready='%s'", mirror);
}
virBufferAddLit(buf, ">\n");
diff --git i/src/conf/domain_conf.h w/src/conf/domain_conf.h
index bc8966d..2df3ae2 100644
--- i/src/conf/domain_conf.h
+++ w/src/conf/domain_conf.h
@@ -611,13 +611,13 @@ typedef virDomainBlockIoTuneInfo
*virDomainBlockIoTuneInfoPtr;
typedef enum {
-VIR_DOMAIN_DISK_MIRROR_DEFAULT = 0, /* No job, or job still not
synced */
-VIR_DOMAIN_DISK_MIRROR_READY, /* Job in second phase */
-VIR_DOMAIN_DISK_MIRROR_ABORT, /* Job aborted, waiting for event */
-VIR_DOMAIN_DISK_MIRROR_PIVOT, /* Job pivoted, waiting for event */
+VIR_DOMAIN_DISK_MIRROR_STATE_NONE = 0, /* No job, or job still not
synced */
+VIR_DOMAIN_DISK_MIRROR_STATE_READY, /* Job in second phase */
+VIR_DOMAIN_DISK_MIRROR_STATE_ABORT, /* Job aborted, waiting for
event */
+VIR_DOMAIN_DISK_MIRROR_STATE_PIVOT, /* Job pivoted, waiting for
event */
-VIR_DOMAIN_DISK_MIRROR_LAST
-} virDomainDiskMirror;
+VIR_DOMAIN_DISK_MIRROR_STATE_LAST
+} virDomainDiskMirrorState;
/* Stores the virtual disk configuration */
@@ -631,7 +631,7 @@ struct _virDomainDiskDef {
int removable; /* enum virTristateSwitch */
virStorageSourcePtr mirror;
-int mirrorState; /* enum virDomainDiskMirror */
+int mirrorState; /* enum virDomainDiskMirrorState */
struct {
unsigned int cylinders;
@@ -2566,7 +2566,7 @@ VIR_ENUM_DECL(virDomainDiskIo)
VIR_ENUM_DECL(virDomainDeviceSGIO)
VIR_ENUM_DECL(virDomainDiskTray)
VIR_ENUM_DECL(virDomainDiskDiscard)
-VIR_ENUM_DECL(virDomainDiskMirror)
+VIR_ENUM_DECL(virDomainDiskMirrorState)
VIR_ENUM_DECL(virDomainController)
VIR_ENUM_DECL(virDomainControllerModelPCI)
VIR_ENUM_DECL(virDomainControllerModelSCSI)
diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c
index acc9ef0..836adba 100644
--- i/src/qemu/qemu_driver.c
+++ w/src/qemu/qemu_driver.c
@@ -14860,10 +14860,10 @@ qemuDomainBlockPivot(virConnectPtr conn,
}
if (rc == 1 && info.cur == info.end &&
info.type == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY)
-disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_READY;
+disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_READY;
}
-if (disk->mirrorState != VIR_DOMAIN_DISK_MIR
Re: [libvirt] [PATCH v5 1/4] blockcopy: add more XML for state tracking
On 07/29/14 06:20, Eric Blake wrote:
> Doing a blockcopy operation across a libvirtd restart is not very
> robust at the moment. In particular, we are clearing the
> element prior to telling qemu to finish the job; and thanks to the
> ability to request async completion, the user can easily regain
> control prior to qemu actually finishing the effort, and they should
> be able to poll the domain XML to see if the job is still going.
>
> A future patch will fix things to actually wait until qemu is done
> before modifying the XML to reflect the job completion. But since
> qemu issues identical BLOCK_JOB_COMPLETE events regardless of whether
> the job was cancelled (kept the original disk) or completed (pivoted
> to the new disk), we have to track which of the two operations were
> used to end the job. Furthermore, we'd like to avoid attempts to
> end a job where we are already waiting on an earlier request to qemu
> to end the job. Likewise, if we miss the qemu event (perhaps because
> it arrived during a libvirtd restart), we still need enough state
> recorded to be able to determine how to modify the domain XML once
> we reconnect to qemu and manually learn whether the job still exists.
>
> Although this patch doesn't actually fix the problem, it is a
> preliminary step that makes it possible to track whether a job
> has already begun steps towards completion.
>
> * src/conf/domain_conf.h (virDomainDiskMirror): New enum.
> (_virDomainDiskDef): Convert bool mirroring to new enum.
> * src/conf/domain_conf.c (virDomainDiskDefParseXML)
> (virDomainDiskDefFormat): Handle new values.
> * src/qemu/qemu_process.c (qemuProcessHandleBlockJob): Adjust
> client.
> * src/qemu/qemu_driver.c (qemuDomainBlockPivot)
> (qemuDomainBlockJobImpl): Likewise.
> * docs/schemas/domaincommon.rng (diskMirror): Expose new values.
> * docs/formatdomain.html.in (elementsDisks): Document it.
> * tests/qemuxml2argvdata/qemuxml2argv-disk-mirror.xml: Test it.
>
> Signed-off-by: Eric Blake
> ---
> docs/formatdomain.html.in | 10 +++---
> docs/schemas/domaincommon.rng | 6 +-
> src/conf/domain_conf.c | 23
> +++---
> src/conf/domain_conf.h | 13 +++-
> src/qemu/qemu_driver.c | 12 +--
> src/qemu/qemu_process.c| 4 ++--
> .../qemuxml2argvdata/qemuxml2argv-disk-mirror.xml | 4
> 7 files changed, 56 insertions(+), 16 deletions(-)
>
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index f6f697c..389c8c9 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -4284,7 +4284,11 @@
>
>
>
> - yes
> +
> +yes
> +abort
Seems reasonable to store the state of the final step here.
> +pivot
> +
>
>
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 910f6e2..1c8f8cf 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -747,6 +747,12 @@ VIR_ENUM_IMPL(virDomainDiskDiscard,
> VIR_DOMAIN_DISK_DISCARD_LAST,
>"unmap",
>"ignore")
>
> +VIR_ENUM_IMPL(virDomainDiskMirror, VIR_DOMAIN_DISK_MIRROR_LAST,
> + "default",
> + "yes",
> + "abort",
> + "pivot")
> +
> #define VIR_DOMAIN_XML_WRITE_FLAGS VIR_DOMAIN_XML_SECURE
> #define VIR_DOMAIN_XML_READ_FLAGS VIR_DOMAIN_XML_INACTIVE
>
> @@ -5482,7 +5488,14 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
> }
> ready = virXMLPropString(cur, "ready");
> if (ready) {
> -def->mirroring = true;
> +def->mirrorState =
> virDomainDiskMirrorTypeFromString(ready);
> +if (def->mirrorState < 0) {
> +virReportError(VIR_ERR_XML_ERROR,
> + _("unknown mirror ready state %s"),
> + ready);
> +VIR_FREE(ready);
> +goto error;
> +}
> VIR_FREE(ready);
> }
> } else if (xmlStrEqual(cur->name, BAD_CAST "auth")) {
> @@ -15273,8 +15286,12 @@ virDomainDiskDefFormat(virBufferPtr buf,
> virBufferEscapeString(buf, " file='%s'", def->mirror->path);
> virBufferEscapeString(buf, " format='%s'", formatStr);
> }
> -if (def->mirroring)
> -virBufferAddLit(buf, " ready='yes'");
> +if (def->mirrorState) {
> +const char *mirror;
> +
> +mirror = virDomainDiskMirrorTypeToString(def->mirrorState);
> +virBufferEscapeString(buf, " ready='%s'", mirror);
> +}
> virBufferAddLit(buf, ">\n");
> vi
[libvirt] [PATCH v5 1/4] blockcopy: add more XML for state tracking
Doing a blockcopy operation across a libvirtd restart is not very
robust at the moment. In particular, we are clearing the
element prior to telling qemu to finish the job; and thanks to the
ability to request async completion, the user can easily regain
control prior to qemu actually finishing the effort, and they should
be able to poll the domain XML to see if the job is still going.
A future patch will fix things to actually wait until qemu is done
before modifying the XML to reflect the job completion. But since
qemu issues identical BLOCK_JOB_COMPLETE events regardless of whether
the job was cancelled (kept the original disk) or completed (pivoted
to the new disk), we have to track which of the two operations were
used to end the job. Furthermore, we'd like to avoid attempts to
end a job where we are already waiting on an earlier request to qemu
to end the job. Likewise, if we miss the qemu event (perhaps because
it arrived during a libvirtd restart), we still need enough state
recorded to be able to determine how to modify the domain XML once
we reconnect to qemu and manually learn whether the job still exists.
Although this patch doesn't actually fix the problem, it is a
preliminary step that makes it possible to track whether a job
has already begun steps towards completion.
* src/conf/domain_conf.h (virDomainDiskMirror): New enum.
(_virDomainDiskDef): Convert bool mirroring to new enum.
* src/conf/domain_conf.c (virDomainDiskDefParseXML)
(virDomainDiskDefFormat): Handle new values.
* src/qemu/qemu_process.c (qemuProcessHandleBlockJob): Adjust
client.
* src/qemu/qemu_driver.c (qemuDomainBlockPivot)
(qemuDomainBlockJobImpl): Likewise.
* docs/schemas/domaincommon.rng (diskMirror): Expose new values.
* docs/formatdomain.html.in (elementsDisks): Document it.
* tests/qemuxml2argvdata/qemuxml2argv-disk-mirror.xml: Test it.
Signed-off-by: Eric Blake
---
docs/formatdomain.html.in | 10 +++---
docs/schemas/domaincommon.rng | 6 +-
src/conf/domain_conf.c | 23 +++---
src/conf/domain_conf.h | 13 +++-
src/qemu/qemu_driver.c | 12 +--
src/qemu/qemu_process.c| 4 ++--
.../qemuxml2argvdata/qemuxml2argv-disk-mirror.xml | 4
7 files changed, 56 insertions(+), 16 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 8950959..4cb4289 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -1907,9 +1907,13 @@
format of the source). The details of the source
sub-element are determined by the type attribute
of the mirror, similar to what is done for the
-overall disk device element. If
-attribute ready is present, then it is known the
-disk is ready to pivot; otherwise, the disk is probably still
+overall disk device element. The
+attribute ready, if present, tracks progress of
+the job: yes if the disk is known to be ready to
+pivot, or, since
+1.2.7, abort or pivot if the
+job is in the process of completing. If ready is
+not present, the disk is probably still
copying. For now, this element only valid in output; it is
ignored on input. The source sub-element exists
for all two-phase jobs since 1.2.6.
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index f6f697c..389c8c9 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -4284,7 +4284,11 @@
- yes
+
+yes
+abort
+pivot
+
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 910f6e2..1c8f8cf 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -747,6 +747,12 @@ VIR_ENUM_IMPL(virDomainDiskDiscard,
VIR_DOMAIN_DISK_DISCARD_LAST,
"unmap",
"ignore")
+VIR_ENUM_IMPL(virDomainDiskMirror, VIR_DOMAIN_DISK_MIRROR_LAST,
+ "default",
+ "yes",
+ "abort",
+ "pivot")
+
#define VIR_DOMAIN_XML_WRITE_FLAGS VIR_DOMAIN_XML_SECURE
#define VIR_DOMAIN_XML_READ_FLAGS VIR_DOMAIN_XML_INACTIVE
@@ -5482,7 +5488,14 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
}
ready = virXMLPropString(cur, "ready");
if (ready) {
-def->mirroring = true;
+def->mirrorState =
virDomainDiskMirrorTypeFromString(ready);
+if (def->mirrorState < 0) {
+virReportError(VIR_ERR_XML_ERROR,
+ _("unknown mirror ready state %s"),
+ ready);
+VIR_FREE(ready);
+goto error;
+
