[libvirt] [PATCH 0/3] qemu: backup: Allow configuring and reading back of export an backup bitmap name
These were already posted before. oVirt folks would like to be able to read the export and bitmap names. Given that I already wrote support for configuring it and it's rather simple I'm re-posting these as they were before. Peter Krempa (3): conf: backup: Allow configuration of names exported via NBD qemu: backup: Implement support for backup disk export name configuration qemu: backup: Implement support for backup disk bitmap name configuration docs/formatbackup.html.in | 9 +++++++++ docs/schemas/domainbackup.rng | 8 ++++++++ src/conf/backup_conf.c | 11 +++++++++++ src/conf/backup_conf.h | 2 ++ src/qemu/qemu_backup.c | 14 ++++++++++++-- .../domainbackupxml2xmlin/backup-pull-seclabel.xml | 2 +- .../backup-pull-seclabel.xml | 2 +- 7 files changed, 44 insertions(+), 4 deletions(-) -- 2.24.1
If users wish to use different name for exported disks or bitmaps the new fields allow to do so. Additionally they also document the current settings. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- docs/formatbackup.html.in | 9 +++++++++ docs/schemas/domainbackup.rng | 8 ++++++++ src/conf/backup_conf.c | 11 +++++++++++ src/conf/backup_conf.h | 2 ++ tests/domainbackupxml2xmlin/backup-pull-seclabel.xml | 2 +- tests/domainbackupxml2xmlout/backup-pull-seclabel.xml | 2 +- 6 files changed, 32 insertions(+), 2 deletions(-) diff --git a/docs/formatbackup.html.in b/docs/formatbackup.html.in index 1c690901c7..7d2c6f1599 100644 --- a/docs/formatbackup.html.in +++ b/docs/formatbackup.html.in @@ -85,6 +85,15 @@ <dd>Setting this attribute to <code>yes</code>(default) specifies that the disk should take part in the backup and using <code>no</code> excludes the disk from the backup.</dd> + <dt><code>exportname</code></dt> + <dd>Allows to modify the NBD export name for the given disk. + By default equal to disk target. + Valid only for pull mode backups. </dd> + <dt><code>exportbitmap</code></dt> + <dd>Allows to modify the name of the bitmap describing dirty + blocks for an incremental backup exported via NBD export name + for the given disk. + Valid only for pull mode backups. </dd> <dt><code>type</code></dt> <dd>A mandatory attribute to describe the type of the disk, except when <code>backup='no'</code> is diff --git a/docs/schemas/domainbackup.rng b/docs/schemas/domainbackup.rng index c1e4d80302..395ea841f9 100644 --- a/docs/schemas/domainbackup.rng +++ b/docs/schemas/domainbackup.rng @@ -165,6 +165,14 @@ <attribute name='name'> <ref name='diskTarget'/> </attribute> + <optional> + <attribute name='exportname'> + <text/> + </attribute> + <attribute name='exportbitmap'> + <text/> + </attribute> + </optional> <choice> <group> <attribute name='backup'> diff --git a/src/conf/backup_conf.c b/src/conf/backup_conf.c index aa11967d2a..a4b87baa55 100644 --- a/src/conf/backup_conf.c +++ b/src/conf/backup_conf.c @@ -71,6 +71,8 @@ virDomainBackupDefFree(virDomainBackupDefPtr def) virDomainBackupDiskDefPtr disk = def->disks + i; g_free(disk->name); + g_free(disk->exportname); + g_free(disk->exportbitmap); virObjectUnref(disk->store); } @@ -124,6 +126,11 @@ virDomainBackupDiskDefParseXML(xmlNodePtr node, if (def->backup == VIR_TRISTATE_BOOL_NO) return 0; + if (!push) { + def->exportname = virXMLPropString(node, "exportname"); + def->exportbitmap = virXMLPropString(node, "exportbitmap"); + } + if (internal) { if (!(state = virXMLPropString(node, "state")) || (tmp = virDomainBackupDiskStateTypeFromString(state)) < 0) { @@ -165,6 +172,7 @@ virDomainBackupDiskDefParseXML(xmlNodePtr node, storageSourceParseFlags, xmlopt) < 0) return -1; + if ((driver = virXPathString("string(./driver/@type)", ctxt))) { def->store->format = virStorageFileFormatTypeFromString(driver); if (def->store->format <= 0) { @@ -333,6 +341,9 @@ virDomainBackupDiskDefFormat(virBufferPtr buf, if (disk->backup == VIR_TRISTATE_BOOL_YES) { virBufferAsprintf(&attrBuf, " type='%s'", virStorageTypeToString(disk->store->type)); + virBufferEscapeString(&attrBuf, " exportname='%s'", disk->exportname); + virBufferEscapeString(&attrBuf, " exportbitmap='%s'", disk->exportbitmap); + if (disk->store->format > 0) virBufferEscapeString(&childBuf, "<driver type='%s'/>\n", virStorageFileFormatTypeToString(disk->store->format)); diff --git a/src/conf/backup_conf.h b/src/conf/backup_conf.h index 7cf44245d4..672fd52ee7 100644 --- a/src/conf/backup_conf.h +++ b/src/conf/backup_conf.h @@ -51,6 +51,8 @@ typedef virDomainBackupDiskDef *virDomainBackupDiskDefPtr; struct _virDomainBackupDiskDef { char *name; /* name matching the <target dev='...' of the domain */ virTristateBool backup; /* whether backup is requested */ + char *exportname; /* name of the NBD export for pull mode backup */ + char *exportbitmap; /* name of the bitmap exposed in NBD for pull mode backup */ /* details of target for push-mode, or of the scratch file for pull-mode */ virStorageSourcePtr store; diff --git a/tests/domainbackupxml2xmlin/backup-pull-seclabel.xml b/tests/domainbackupxml2xmlin/backup-pull-seclabel.xml index a00d8758bb..4e6e602c19 100644 --- a/tests/domainbackupxml2xmlin/backup-pull-seclabel.xml +++ b/tests/domainbackupxml2xmlin/backup-pull-seclabel.xml @@ -2,7 +2,7 @@ <incremental>1525889631</incremental> <server transport='tcp' name='localhost' port='10809'/> <disks> - <disk name='vda' type='file'> + <disk name='vda' type='file' exportname='test-vda' exportbitmap='blah'> <driver type='qcow2'/> <scratch file='/path/to/file'> <seclabel model='dac' relabel='no'/> diff --git a/tests/domainbackupxml2xmlout/backup-pull-seclabel.xml b/tests/domainbackupxml2xmlout/backup-pull-seclabel.xml index c631c9b979..450f007d3a 100644 --- a/tests/domainbackupxml2xmlout/backup-pull-seclabel.xml +++ b/tests/domainbackupxml2xmlout/backup-pull-seclabel.xml @@ -2,7 +2,7 @@ <incremental>1525889631</incremental> <server transport='tcp' name='localhost' port='10809'/> <disks> - <disk name='vda' backup='yes' type='file'> + <disk name='vda' backup='yes' type='file' exportname='test-vda' exportbitmap='blah'> <driver type='qcow2'/> <scratch file='/path/to/file'> <seclabel model='dac' relabel='no'/> -- 2.24.1
On 1/9/20 3:31 PM, Peter Krempa wrote:
If users wish to use different name for exported disks or bitmaps the new fields allow to do so. Additionally they also document the current settings.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- docs/formatbackup.html.in | 9 +++++++++ docs/schemas/domainbackup.rng | 8 ++++++++ src/conf/backup_conf.c | 11 +++++++++++ src/conf/backup_conf.h | 2 ++ tests/domainbackupxml2xmlin/backup-pull-seclabel.xml | 2 +- tests/domainbackupxml2xmlout/backup-pull-seclabel.xml | 2 +- 6 files changed, 32 insertions(+), 2 deletions(-)
diff --git a/docs/formatbackup.html.in b/docs/formatbackup.html.in index 1c690901c7..7d2c6f1599 100644 --- a/docs/formatbackup.html.in +++ b/docs/formatbackup.html.in @@ -85,6 +85,15 @@ <dd>Setting this attribute to <code>yes</code>(default) specifies that the disk should take part in the backup and using <code>no</code> excludes the disk from the backup.</dd> + <dt><code>exportname</code></dt> + <dd>Allows to modify the NBD export name for the given disk. + By default equal to disk target. + Valid only for pull mode backups. </dd> + <dt><code>exportbitmap</code></dt> + <dd>Allows to modify the name of the bitmap describing dirty + blocks for an incremental backup exported via NBD export name + for the given disk. + Valid only for pull mode backups. </dd> <dt><code>type</code></dt> <dd>A mandatory attribute to describe the type of the disk, except when <code>backup='no'</code> is diff --git a/docs/schemas/domainbackup.rng b/docs/schemas/domainbackup.rng index c1e4d80302..395ea841f9 100644 --- a/docs/schemas/domainbackup.rng +++ b/docs/schemas/domainbackup.rng @@ -165,6 +165,14 @@ <attribute name='name'> <ref name='diskTarget'/> </attribute> + <optional> + <attribute name='exportname'> + <text/> + </attribute> + <attribute name='exportbitmap'> + <text/> + </attribute> + </optional> <choice> <group> <attribute name='backup'> diff --git a/src/conf/backup_conf.c b/src/conf/backup_conf.c index aa11967d2a..a4b87baa55 100644 --- a/src/conf/backup_conf.c +++ b/src/conf/backup_conf.c @@ -71,6 +71,8 @@ virDomainBackupDefFree(virDomainBackupDefPtr def) virDomainBackupDiskDefPtr disk = def->disks + i;
g_free(disk->name); + g_free(disk->exportname); + g_free(disk->exportbitmap); virObjectUnref(disk->store); }
@@ -124,6 +126,11 @@ virDomainBackupDiskDefParseXML(xmlNodePtr node, if (def->backup == VIR_TRISTATE_BOOL_NO) return 0;
+ if (!push) { + def->exportname = virXMLPropString(node, "exportname"); + def->exportbitmap = virXMLPropString(node, "exportbitmap"); + } + if (internal) { if (!(state = virXMLPropString(node, "state")) || (tmp = virDomainBackupDiskStateTypeFromString(state)) < 0) { @@ -165,6 +172,7 @@ virDomainBackupDiskDefParseXML(xmlNodePtr node, storageSourceParseFlags, xmlopt) < 0) return -1;
+
Is this an unintended newline? Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
if ((driver = virXPathString("string(./driver/@type)", ctxt))) { def->store->format = virStorageFileFormatTypeFromString(driver); if (def->store->format <= 0) { @@ -333,6 +341,9 @@ virDomainBackupDiskDefFormat(virBufferPtr buf, if (disk->backup == VIR_TRISTATE_BOOL_YES) { virBufferAsprintf(&attrBuf, " type='%s'", virStorageTypeToString(disk->store->type));
+ virBufferEscapeString(&attrBuf, " exportname='%s'", disk->exportname); + virBufferEscapeString(&attrBuf, " exportbitmap='%s'", disk->exportbitmap); + if (disk->store->format > 0) virBufferEscapeString(&childBuf, "<driver type='%s'/>\n", virStorageFileFormatTypeToString(disk->store->format));
On 1/9/20 12:31 PM, Peter Krempa wrote:
If users wish to use different name for exported disks or bitmaps the new fields allow to do so. Additionally they also document the current settings.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- docs/formatbackup.html.in | 9 +++++++++ docs/schemas/domainbackup.rng | 8 ++++++++ src/conf/backup_conf.c | 11 +++++++++++ src/conf/backup_conf.h | 2 ++ tests/domainbackupxml2xmlin/backup-pull-seclabel.xml | 2 +- tests/domainbackupxml2xmlout/backup-pull-seclabel.xml | 2 +- 6 files changed, 32 insertions(+), 2 deletions(-)
diff --git a/docs/formatbackup.html.in b/docs/formatbackup.html.in index 1c690901c7..7d2c6f1599 100644 --- a/docs/formatbackup.html.in +++ b/docs/formatbackup.html.in @@ -85,6 +85,15 @@ <dd>Setting this attribute to <code>yes</code>(default) specifies that the disk should take part in the backup and using <code>no</code> excludes the disk from the backup.</dd> + <dt><code>exportname</code></dt> + <dd>Allows to modify the NBD export name for the given disk.
Allows modification of the NBD export name
+ By default equal to disk target. + Valid only for pull mode backups. </dd>
Is the space after '.' needed?
+ <dt><code>exportbitmap</code></dt> + <dd>Allows to modify the name of the bitmap describing dirty
Allows modification of the name
+ blocks for an incremental backup exported via NBD export name + for the given disk. + Valid only for pull mode backups. </dd>
Another trailing space Otherwise looks fine, once you also fix Daniel's nit. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Pass the exportname as configured when exporting the image via NBD and fill it with the default if it's not configured. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_backup.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_backup.c b/src/qemu/qemu_backup.c index c47de2f4a8..2cc0e6ab07 100644 --- a/src/qemu/qemu_backup.c +++ b/src/qemu/qemu_backup.c @@ -548,9 +548,12 @@ qemuBackupBeginPullExportDisks(virDomainObjPtr vm, for (i = 0; i < ndisks; i++) { struct qemuBackupDiskData *dd = disks + i; + if (!dd->backupdisk->exportname) + dd->backupdisk->exportname = g_strdup(dd->domdisk->dst); + if (qemuMonitorNBDServerAdd(priv->mon, dd->store->nodeformat, - dd->domdisk->dst, + dd->backupdisk->exportname, false, dd->incrementalBitmap) < 0) return -1; -- 2.24.1
On 1/9/20 3:31 PM, Peter Krempa wrote:
Pass the exportname as configured when exporting the image via NBD and fill it with the default if it's not configured.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_backup.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
On 1/9/20 12:31 PM, Peter Krempa wrote:
Pass the exportname as configured when exporting the image via NBD and fill it with the default if it's not configured.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_backup.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
Reviewed-by: Eric Blake <eblake@redhat.com>
diff --git a/src/qemu/qemu_backup.c b/src/qemu/qemu_backup.c index c47de2f4a8..2cc0e6ab07 100644 --- a/src/qemu/qemu_backup.c +++ b/src/qemu/qemu_backup.c @@ -548,9 +548,12 @@ qemuBackupBeginPullExportDisks(virDomainObjPtr vm, for (i = 0; i < ndisks; i++) { struct qemuBackupDiskData *dd = disks + i;
+ if (!dd->backupdisk->exportname) + dd->backupdisk->exportname = g_strdup(dd->domdisk->dst); + if (qemuMonitorNBDServerAdd(priv->mon, dd->store->nodeformat, - dd->domdisk->dst, + dd->backupdisk->exportname, false, dd->incrementalBitmap) < 0) return -1;
-- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Use the user-configured name of the bitmap when merging the appropriate bitmaps for an incremental backup so that the user can see it as configured. Additionally expose the default bitmap name if nothing is configured. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_backup.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_backup.c b/src/qemu/qemu_backup.c index 2cc0e6ab07..23518a5d40 100644 --- a/src/qemu/qemu_backup.c +++ b/src/qemu/qemu_backup.c @@ -322,7 +322,10 @@ qemuBackupDiskPrepareDataOne(virDomainObjPtr vm, return -1; if (incremental) { - dd->incrementalBitmap = g_strdup_printf("backup-%s", dd->domdisk->dst); + if (dd->backupdisk->exportbitmap) + dd->incrementalBitmap = g_strdup(dd->backupdisk->exportbitmap); + else + dd->incrementalBitmap = g_strdup_printf("backup-%s", dd->domdisk->dst); if (qemuBackupDiskPrepareOneBitmaps(dd, actions, incremental, blockNamedNodeData) < 0) @@ -368,6 +371,10 @@ static int qemuBackupDiskPrepareDataOnePull(virJSONValuePtr actions, struct qemuBackupDiskData *dd) { + if (!dd->backupdisk->exportbitmap && + dd->incrementalBitmap) + dd->backupdisk->exportbitmap = g_strdup(dd->incrementalBitmap); + if (qemuMonitorTransactionBackup(actions, dd->domdisk->src->nodeformat, dd->blockjob->name, -- 2.24.1
On 1/9/20 3:31 PM, Peter Krempa wrote:
Use the user-configured name of the bitmap when merging the appropriate bitmaps for an incremental backup so that the user can see it as configured. Additionally expose the default bitmap name if nothing is configured.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_backup.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
On 1/9/20 12:31 PM, Peter Krempa wrote:
Use the user-configured name of the bitmap when merging the appropriate bitmaps for an incremental backup so that the user can see it as configured. Additionally expose the default bitmap name if nothing is configured.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_backup.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_backup.c b/src/qemu/qemu_backup.c index 2cc0e6ab07..23518a5d40 100644 --- a/src/qemu/qemu_backup.c +++ b/src/qemu/qemu_backup.c @@ -322,7 +322,10 @@ qemuBackupDiskPrepareDataOne(virDomainObjPtr vm, return -1;
if (incremental) { - dd->incrementalBitmap = g_strdup_printf("backup-%s", dd->domdisk->dst); + if (dd->backupdisk->exportbitmap) + dd->incrementalBitmap = g_strdup(dd->backupdisk->exportbitmap);
Do we need to worry about the user requesting a name that would cause conflicts with existing bitmaps in the qcow2 file? I'm worried this can open the door for odd failures if the user accidentally stomps on names that libvirt thought were available for its own use. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On Tue, Jan 14, 2020 at 08:50:55 -0600, Eric Blake wrote:
On 1/9/20 12:31 PM, Peter Krempa wrote:
Use the user-configured name of the bitmap when merging the appropriate bitmaps for an incremental backup so that the user can see it as configured. Additionally expose the default bitmap name if nothing is configured.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_backup.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_backup.c b/src/qemu/qemu_backup.c index 2cc0e6ab07..23518a5d40 100644 --- a/src/qemu/qemu_backup.c +++ b/src/qemu/qemu_backup.c @@ -322,7 +322,10 @@ qemuBackupDiskPrepareDataOne(virDomainObjPtr vm, return -1;
if (incremental) { - dd->incrementalBitmap = g_strdup_printf("backup-%s", dd->domdisk->dst); + if (dd->backupdisk->exportbitmap) + dd->incrementalBitmap = g_strdup(dd->backupdisk->exportbitmap);
Do we need to worry about the user requesting a name that would cause conflicts with existing bitmaps in the qcow2 file? I'm worried this can open the door for odd failures if the user accidentally stomps on names that libvirt thought were available for its own use.
This name will be passed to a transactioned 'block-dirty-bitmap-add' so existing bitmaps should not be overwritten. I'm not sure whether it's worth forbidding. Users actually might want to pick a different bitmap name when they accidentally used the one libvirt would pick for the temporary bitmap when left blank in which case the code would fail without any way to fix it.
On 1/21/20 6:55 AM, Peter Krempa wrote:
On Tue, Jan 14, 2020 at 08:50:55 -0600, Eric Blake wrote:
On 1/9/20 12:31 PM, Peter Krempa wrote:
Use the user-configured name of the bitmap when merging the appropriate bitmaps for an incremental backup so that the user can see it as configured. Additionally expose the default bitmap name if nothing is configured.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_backup.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_backup.c b/src/qemu/qemu_backup.c index 2cc0e6ab07..23518a5d40 100644 --- a/src/qemu/qemu_backup.c +++ b/src/qemu/qemu_backup.c @@ -322,7 +322,10 @@ qemuBackupDiskPrepareDataOne(virDomainObjPtr vm, return -1;
if (incremental) { - dd->incrementalBitmap = g_strdup_printf("backup-%s", dd->domdisk->dst); + if (dd->backupdisk->exportbitmap) + dd->incrementalBitmap = g_strdup(dd->backupdisk->exportbitmap);
Do we need to worry about the user requesting a name that would cause conflicts with existing bitmaps in the qcow2 file? I'm worried this can open the door for odd failures if the user accidentally stomps on names that libvirt thought were available for its own use.
This name will be passed to a transactioned 'block-dirty-bitmap-add' so existing bitmaps should not be overwritten.
I'm not sure whether it's worth forbidding. Users actually might want to pick a different bitmap name when they accidentally used the one libvirt would pick for the temporary bitmap when left blank in which case the code would fail without any way to fix it.
Okay, it sounds like even if the user picks a problematic name, the transaction will fail and they will at least get a useful error message that hopefully points them to pick a better name. In that case, no objection from me. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
participants (3)
-
Daniel Henrique Barboza -
Eric Blake -
Peter Krempa