[PATCH 0/4] qemu: Report stats for backing images
Management tools such as oVirt need to monitor the 'allocation' of the backup job scratch file or disk copy target. Add the stats to the bulk stats. The stats can be queried by: $ virsh domstats $VM --block --backing Patch 1 fixes a bug in the status XML formatter where we'd not store the private data for the scratch file of a backup job, which resulted in a failure of getting the stats after restart of libvirtd. Peter Krempa (4): virDomainBackupDefFormat: Propagate private data callbacks qemustatusxml2xmldata: backup-pull: Add private data for scratch image qemuMonitorJSONQueryBlockstats: query stats for helper images qemuDomainGetStatsBlockExportDisk: Report stats also for helper images src/conf/backup_conf.c | 10 +-- src/conf/backup_conf.h | 3 +- src/qemu/qemu_backup.c | 4 +- src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_driver.c | 61 +++++++++++++++++++ src/qemu/qemu_monitor.c | 4 +- src/qemu/qemu_monitor_json.c | 20 +++--- src/qemu/qemu_monitor_json.h | 3 +- tests/genericxml2xmltest.c | 2 +- .../qemustatusxml2xmldata/backup-pull-in.xml | 9 ++- 10 files changed, 95 insertions(+), 23 deletions(-) -- 2.31.1
The formatter for the backup job data didn't pass the virDomainXMLOption struct to the disk formatter which meant that the private data of the disk source were not formatted. This didn't pose a problem for now as the blockjob list remembered the nodenames for the jobs, but the backup source lost them. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/backup_conf.c | 10 ++++++---- src/conf/backup_conf.h | 3 ++- src/qemu/qemu_backup.c | 4 +++- src/qemu/qemu_domain.c | 2 +- tests/genericxml2xmltest.c | 2 +- 5 files changed, 13 insertions(+), 8 deletions(-) diff --git a/src/conf/backup_conf.c b/src/conf/backup_conf.c index 694553a544..2a7fa95e0c 100644 --- a/src/conf/backup_conf.c +++ b/src/conf/backup_conf.c @@ -318,7 +318,8 @@ static int virDomainBackupDiskDefFormat(virBuffer *buf, virDomainBackupDiskDef *disk, bool push, - bool internal) + bool internal, + virDomainXMLOption *xmlopt) { g_auto(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER; g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf); @@ -358,7 +359,7 @@ virDomainBackupDiskDefFormat(virBuffer *buf, if (virDomainDiskSourceFormat(&childBuf, disk->store, sourcename, 0, false, storageSourceFormatFlags, - false, false, NULL) < 0) + false, false, xmlopt) < 0) return -1; } @@ -390,7 +391,8 @@ virDomainBackupDefFormatPrivate(virBuffer *buf, int virDomainBackupDefFormat(virBuffer *buf, virDomainBackupDef *def, - bool internal) + bool internal, + virDomainXMLOption *xmlopt) { g_auto(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER; g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf); @@ -418,7 +420,7 @@ virDomainBackupDefFormat(virBuffer *buf, for (i = 0; i < def->ndisks; i++) { if (virDomainBackupDiskDefFormat(&disksChildBuf, &def->disks[i], def->type == VIR_DOMAIN_BACKUP_TYPE_PUSH, - internal) < 0) + internal, xmlopt) < 0) return -1; } diff --git a/src/conf/backup_conf.h b/src/conf/backup_conf.h index b682da1c95..dc66b75892 100644 --- a/src/conf/backup_conf.h +++ b/src/conf/backup_conf.h @@ -123,7 +123,8 @@ G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainBackupDef, virDomainBackupDefFree); int virDomainBackupDefFormat(virBuffer *buf, virDomainBackupDef *def, - bool internal); + bool internal, + virDomainXMLOption *xmlopt); int virDomainBackupAlignDisks(virDomainBackupDef *backup, virDomainDef *dom, diff --git a/src/qemu/qemu_backup.c b/src/qemu/qemu_backup.c index abbfcf3682..9fa8d2f02e 100644 --- a/src/qemu/qemu_backup.c +++ b/src/qemu/qemu_backup.c @@ -954,6 +954,8 @@ char * qemuBackupGetXMLDesc(virDomainObj *vm, unsigned int flags) { + qemuDomainObjPrivate *priv = vm->privateData; + g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; virDomainBackupDef *backup; @@ -962,7 +964,7 @@ qemuBackupGetXMLDesc(virDomainObj *vm, if (!(backup = qemuDomainGetBackup(vm))) return NULL; - if (virDomainBackupDefFormat(&buf, backup, false) < 0) + if (virDomainBackupDefFormat(&buf, backup, false, priv->driver->xmlopt) < 0) return NULL; return virBufferContentAndReset(&buf); diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 209337404a..fb203bc830 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2152,7 +2152,7 @@ qemuDomainObjPrivateXMLFormatBackups(virBuffer *buf, g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf); if (priv->backup && - virDomainBackupDefFormat(&childBuf, priv->backup, true) < 0) + virDomainBackupDefFormat(&childBuf, priv->backup, true, priv->driver->xmlopt) < 0) return -1; virXMLFormatElement(buf, "backups", &attrBuf, &childBuf); diff --git a/tests/genericxml2xmltest.c b/tests/genericxml2xmltest.c index a6f974e758..34ccaff615 100644 --- a/tests/genericxml2xmltest.c +++ b/tests/genericxml2xmltest.c @@ -114,7 +114,7 @@ testCompareBackupXML(const void *opaque) return -1; } - if (virDomainBackupDefFormat(&buf, backup, data->internal) < 0) { + if (virDomainBackupDefFormat(&buf, backup, data->internal, NULL) < 0) { VIR_TEST_VERBOSE("failed to format backup def '%s'", file_in); return -1; } -- 2.31.1
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemustatusxml2xmldata/backup-pull-in.xml | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/tests/qemustatusxml2xmldata/backup-pull-in.xml b/tests/qemustatusxml2xmldata/backup-pull-in.xml index 95afd3a51f..59c934d4f7 100644 --- a/tests/qemustatusxml2xmldata/backup-pull-in.xml +++ b/tests/qemustatusxml2xmldata/backup-pull-in.xml @@ -257,7 +257,14 @@ <server transport='tcp' tls='yes' name='localhost' port='10809'/> <disks> <disk name='vda' backup='yes' state='running' type='file' index='123'> - <scratch file='/path/to/file/'/> + <scratch file='/path/to/file/'> + <privateData> + <nodenames> + <nodename type='storage' name='libvirt-1337-storage'/> + <nodename type='format' name='libvirt-1337-format'/> + </nodenames> + </privateData> + </scratch> </disk> </disks> <privateData> -- 2.31.1
On Mon, Nov 1, 2021 at 4:48 PM Peter Krempa <pkrempa@redhat.com> wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemustatusxml2xmldata/backup-pull-in.xml | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/tests/qemustatusxml2xmldata/backup-pull-in.xml b/tests/qemustatusxml2xmldata/backup-pull-in.xml index 95afd3a51f..59c934d4f7 100644 --- a/tests/qemustatusxml2xmldata/backup-pull-in.xml +++ b/tests/qemustatusxml2xmldata/backup-pull-in.xml @@ -257,7 +257,14 @@ <server transport='tcp' tls='yes' name='localhost' port='10809'/> <disks> <disk name='vda' backup='yes' state='running' type='file' index='123'> - <scratch file='/path/to/file/'/> + <scratch file='/path/to/file/'> + <privateData> + <nodenames> + <nodename type='storage' name='libvirt-1337-storage'/> + <nodename type='format' name='libvirt-1337-format'/>
Shouldn't this be libvirt-123-*, matching the index=? Since we have the index entry, and libvirt knows that it has libvirt-N-storage and libvirt-N-format for every disk, why do we need to keep this info? Finally, is this exposed in backup-dumpxml?
+ </nodenames> + </privateData> + </scratch> </disk> </disks> <privateData> -- 2.31.1
On Mon, Nov 01, 2021 at 18:32:19 +0200, Nir Soffer wrote:
On Mon, Nov 1, 2021 at 4:48 PM Peter Krempa <pkrempa@redhat.com> wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemustatusxml2xmldata/backup-pull-in.xml | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/tests/qemustatusxml2xmldata/backup-pull-in.xml b/tests/qemustatusxml2xmldata/backup-pull-in.xml index 95afd3a51f..59c934d4f7 100644 --- a/tests/qemustatusxml2xmldata/backup-pull-in.xml +++ b/tests/qemustatusxml2xmldata/backup-pull-in.xml @@ -257,7 +257,14 @@ <server transport='tcp' tls='yes' name='localhost' port='10809'/> <disks> <disk name='vda' backup='yes' state='running' type='file' index='123'> - <scratch file='/path/to/file/'/> + <scratch file='/path/to/file/'> + <privateData> + <nodenames> + <nodename type='storage' name='libvirt-1337-storage'/> + <nodename type='format' name='libvirt-1337-format'/>
Shouldn't this be libvirt-123-*, matching the index=?
It does normally, but here the job data (above out of context) have dummy nodenames. But that doesn't matter that much.
Since we have the index entry, and libvirt knows that it has libvirt-N-storage and libvirt-N-format for every disk, why do we need to keep this info?
Storing nodenames explicitly allows us to see which objects are in use actually. E.g. I'm removing the use of the 'raw' driver if it's just dummy to improve performance, and it comes in handy with upgrades. But yes, it is actually redundant.
Finally, is this exposed in backup-dumpxml?
The index is. The rest of the private data is not, that's just for libvirt state storage in the status XML file which is not to be used by anybody else.
+ </nodenames> + </privateData> + </scratch> </disk> </disks> <privateData> -- 2.31.1
Use the 'query-nodes' flag to return all stats. The flag was introduced prior to qemu-2.11 so we can always use it, but we invoke it only when querying stats. The other invocation is used for detecting the nodenames which is fragile code. The images without a frontend don't have the device field so the extraction code checks need to be relaxed. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_monitor.c | 4 ++-- src/qemu/qemu_monitor_json.c | 20 +++++++++----------- src/qemu/qemu_monitor_json.h | 3 ++- 3 files changed, 13 insertions(+), 14 deletions(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 908ee0d302..1fbf1eb5c7 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2038,14 +2038,14 @@ qemuMonitorGetBlockInfo(qemuMonitor *mon) * qemuMonitorQueryBlockstats: * @mon: monitor object * - * Returns data from a call to 'query-blockstats'. + * Returns data from a call to 'query-blockstats' without using 'query-nodes' */ virJSONValue * qemuMonitorQueryBlockstats(qemuMonitor *mon) { QEMU_CHECK_MONITOR_NULL(mon); - return qemuMonitorJSONQueryBlockstats(mon); + return qemuMonitorJSONQueryBlockstats(mon, false); } diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index e9be9bdabd..7f5d9f7cad 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -2435,12 +2435,15 @@ qemuMonitorJSONGetOneBlockStatsInfo(virJSONValue *dev, virJSONValue * -qemuMonitorJSONQueryBlockstats(qemuMonitor *mon) +qemuMonitorJSONQueryBlockstats(qemuMonitor *mon, + bool queryNodes) { g_autoptr(virJSONValue) cmd = NULL; g_autoptr(virJSONValue) reply = NULL; - if (!(cmd = qemuMonitorJSONMakeCommand("query-blockstats", NULL))) + if (!(cmd = qemuMonitorJSONMakeCommand("query-blockstats", + "B:query-nodes", queryNodes, + NULL))) return NULL; if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) @@ -2462,7 +2465,7 @@ qemuMonitorJSONGetAllBlockStatsInfo(qemuMonitor *mon, size_t i; g_autoptr(virJSONValue) devices = NULL; - if (!(devices = qemuMonitorJSONQueryBlockstats(mon))) + if (!(devices = qemuMonitorJSONQueryBlockstats(mon, true))) return -1; for (i = 0; i < virJSONValueArraySize(devices); i++) { @@ -2476,16 +2479,11 @@ qemuMonitorJSONGetAllBlockStatsInfo(qemuMonitor *mon, return -1; } - if (!(dev_name = virJSONValueObjectGetString(dev, "device"))) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("blockstats device entry was not " - "in expected format")); - return -1; + if ((dev_name = virJSONValueObjectGetString(dev, "device"))) { + if (*dev_name == '\0') + dev_name = NULL; } - if (*dev_name == '\0') - dev_name = NULL; - rc = qemuMonitorJSONGetOneBlockStatsInfo(dev, dev_name, 0, hash); if (rc < 0) diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index f9e01e5bf5..aa70d35bfb 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -75,7 +75,8 @@ int qemuMonitorJSONSetMemoryStatsPeriod(qemuMonitor *mon, int qemuMonitorJSONGetBlockInfo(qemuMonitor *mon, GHashTable *table); -virJSONValue *qemuMonitorJSONQueryBlockstats(qemuMonitor *mon); +virJSONValue *qemuMonitorJSONQueryBlockstats(qemuMonitor *mon, + bool queryNodes); int qemuMonitorJSONGetAllBlockStatsInfo(qemuMonitor *mon, GHashTable *hash); int qemuMonitorJSONBlockStatsUpdateCapacity(qemuMonitor *mon, -- 2.31.1
Add stat entries also for the mirror destination and the backup job scratch/target file. This is possible with '-blockdev' as we use unique index for the entries. The stats are reported when the VIR_CONNECT_GET_ALL_DOMAINS_STATS_BACKING is used. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2017928 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 61 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 70b5f37e6b..7d13ae9754 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -113,6 +113,7 @@ #include "virdomaincheckpointobjlist.h" #include "virsocket.h" #include "virutil.h" +#include "backup_conf.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -18453,6 +18454,66 @@ qemuDomainGetStatsBlockExportDisk(virDomainDiskDef *disk, break; } + /* in blockdev mode where we can properly and uniquely identify images we + * can also report stats for the mirror target or the scratch image or target + * of a backup operation */ + if (visitBacking && blockdev) { + qemuDomainObjPrivate *priv = dom->privateData; + + if (disk->mirror && + disk->mirrorJob == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY) { + if (qemuDomainGetStatsBlockExportHeader(disk, disk->mirror, *recordnr, params) < 0) + return -1; + + if (qemuDomainGetStatsOneBlock(driver, cfg, dom, params, + disk->mirror->nodeformat, + disk->mirror, + *recordnr, + stats) < 0) + return -1; + + if (qemuDomainGetStatsBlockExportBackendStorage(disk->mirror->nodestorage, + stats, *recordnr, + params) < 0) + return -1; + + (*recordnr)++; + } + + if (priv->backup) { + size_t i; + + for (i = 0; i < priv->backup->ndisks; i++) { + virDomainBackupDiskDef *backupdisk = priv->backup->disks + i; + + if (STRNEQ(disk->dst, priv->backup->disks[i].name)) + continue; + + if (backupdisk->store) { + if (qemuDomainGetStatsBlockExportHeader(disk, backupdisk->store, + *recordnr, params) < 0) + return -1; + + if (qemuDomainGetStatsOneBlock(driver, cfg, dom, params, + backupdisk->store->nodeformat, + backupdisk->store, + *recordnr, + stats) < 0) + return -1; + + if (qemuDomainGetStatsBlockExportBackendStorage(backupdisk->store->nodestorage, + stats, *recordnr, + params) < 0) + return -1; + + (*recordnr)++; + } + + break; + } + } + } + return 0; } -- 2.31.1
On Mon, Nov 1, 2021 at 4:48 PM Peter Krempa <pkrempa@redhat.com> wrote:
Add stat entries also for the mirror destination and the backup job scratch/target file. This is possible with '-blockdev' as we use unique index for the entries.
I wanted to ask about getting the allocation for mirror target disk and I see you already handle it, thanks!
The stats are reported when the VIR_CONNECT_GET_ALL_DOMAINS_STATS_BACKING is used.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2017928 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 61 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 70b5f37e6b..7d13ae9754 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -113,6 +113,7 @@ #include "virdomaincheckpointobjlist.h" #include "virsocket.h" #include "virutil.h" +#include "backup_conf.h"
#define VIR_FROM_THIS VIR_FROM_QEMU
@@ -18453,6 +18454,66 @@ qemuDomainGetStatsBlockExportDisk(virDomainDiskDef *disk, break; }
+ /* in blockdev mode where we can properly and uniquely identify images we + * can also report stats for the mirror target or the scratch image or target + * of a backup operation */ + if (visitBacking && blockdev) { + qemuDomainObjPrivate *priv = dom->privateData; + + if (disk->mirror && + disk->mirrorJob == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY) { + if (qemuDomainGetStatsBlockExportHeader(disk, disk->mirror, *recordnr, params) < 0) + return -1; + + if (qemuDomainGetStatsOneBlock(driver, cfg, dom, params, + disk->mirror->nodeformat, + disk->mirror, + *recordnr, + stats) < 0) + return -1; + + if (qemuDomainGetStatsBlockExportBackendStorage(disk->mirror->nodestorage, + stats, *recordnr, + params) < 0) + return -1; + + (*recordnr)++; + } + + if (priv->backup) { + size_t i; + + for (i = 0; i < priv->backup->ndisks; i++) { + virDomainBackupDiskDef *backupdisk = priv->backup->disks + i; + + if (STRNEQ(disk->dst, priv->backup->disks[i].name)) + continue; + + if (backupdisk->store) { + if (qemuDomainGetStatsBlockExportHeader(disk, backupdisk->store, + *recordnr, params) < 0) + return -1; + + if (qemuDomainGetStatsOneBlock(driver, cfg, dom, params, + backupdisk->store->nodeformat, + backupdisk->store, + *recordnr, + stats) < 0) + return -1; + + if (qemuDomainGetStatsBlockExportBackendStorage(backupdisk->store->nodestorage, + stats, *recordnr, + params) < 0) + return -1; + + (*recordnr)++; + } + + break; + } + } + } + return 0; }
-- 2.31.1
On a Monday in 2021, Peter Krempa wrote:
Management tools such as oVirt need to monitor the 'allocation' of the backup job scratch file or disk copy target. Add the stats to the bulk stats.
The stats can be queried by:
$ virsh domstats $VM --block --backing
Patch 1 fixes a bug in the status XML formatter where we'd not store the private data for the scratch file of a backup job, which resulted in a failure of getting the stats after restart of libvirtd.
Peter Krempa (4): virDomainBackupDefFormat: Propagate private data callbacks qemustatusxml2xmldata: backup-pull: Add private data for scratch image qemuMonitorJSONQueryBlockstats: query stats for helper images qemuDomainGetStatsBlockExportDisk: Report stats also for helper images
src/conf/backup_conf.c | 10 +-- src/conf/backup_conf.h | 3 +- src/qemu/qemu_backup.c | 4 +- src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_driver.c | 61 +++++++++++++++++++ src/qemu/qemu_monitor.c | 4 +- src/qemu/qemu_monitor_json.c | 20 +++--- src/qemu/qemu_monitor_json.h | 3 +- tests/genericxml2xmltest.c | 2 +- .../qemustatusxml2xmldata/backup-pull-in.xml | 9 ++- 10 files changed, 95 insertions(+), 23 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (3)
-
Ján Tomko -
Nir Soffer -
Peter Krempa