Re: [PATCH] Qemu: migration: Not bind RAM info with active migration status

On 7/15/20 3:18 AM, Keqian Zhu wrote: > For that Qemu supports returning incoming migration info since its commit > 65ace0604551 (migration: add postcopy total blocktime into query-migrate), It is worth saying that this QEMU commit that decoupled the RAM status from the active migration status is live since early 2018: $ git show 65ace0604551 commit 65ace060455122a461cdc9302238b914084bcd42 Author: Alexey Perevalov <a.perevalov(a)samsung.com&gt; Date: Thu Mar 22 21:17:27 2018 +0300 migration: add postcopy total blocktime into query-migrate $ git describe 65ace0604551 v2.12.0-6-g65ace06045 I am not sure if we care about removing a migration failure check for QEMU 2.12 when we're waiting for 5.1 to come out. My guess is that we do care, but not enough to demand a "if (QEMU <= 2.12)" in this logic. I'll also assume that the existing failure check is doing more harm than good nowadays, so:

Reviewed-by: Daniel Henrique Barboza <danielhb413(a)gmail.com&gt; > which may contains active status, but without RAM info. Drop this binding > relationship check in libvirt. > > Signed-off-by: Keqian Zhu <zhukeqian1(a)huawei.com&gt; > --- > src/qemu/qemu_monitor_json.c | 88 +++++++++++++++++------------------- > 1 file changed, 42 insertions(+), 46 deletions(-) > > diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c > index d808c4b55b..ba8e340742 100644 > --- a/src/qemu/qemu_monitor_json.c > +++ b/src/qemu/qemu_monitor_json.c > @@ -3547,56 +3547,52 @@ qemuMonitorJSONGetMigrationStatsReply(virJSONValuePtr reply, > case QEMU_MONITOR_MIGRATION_STATUS_PRE_SWITCHOVER: > case QEMU_MONITOR_MIGRATION_STATUS_DEVICE: > ram = virJSONValueObjectGetObject(ret, "ram"); > - if (!ram) { > - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > - _("migration was active, but no RAM info was set")); > - return -1; > - } > + if (ram) { > + if (virJSONValueObjectGetNumberUlong(ram, "transferred", > + &stats->ram_transferred) < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("migration was active, but RAM 'transferred' " > + "data was missing")); > + return -1; > + } > + if (virJSONValueObjectGetNumberUlong(ram, "remaining", > + &stats->ram_remaining) < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("migration was active, but RAM 'remaining' " > + "data was missing")); > + return -1; > + } > + if (virJSONValueObjectGetNumberUlong(ram, "total", > + &stats->ram_total) < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("migration was active, but RAM 'total' " > + "data was missing")); > + return -1; > + } > - if (virJSONValueObjectGetNumberUlong(ram, "transferred", > - &stats->ram_transferred) < 0) { > - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > - _("migration was active, but RAM 'transferred' " > - "data was missing")); > - return -1; > - } > - if (virJSONValueObjectGetNumberUlong(ram, "remaining", > - &stats->ram_remaining) < 0) { > - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > - _("migration was active, but RAM 'remaining' " > - "data was missing")); > - return -1; > - } > - if (virJSONValueObjectGetNumberUlong(ram, "total", > - &stats->ram_total) < 0) { > - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > - _("migration was active, but RAM 'total' " > - "data was missing")); > - return -1; > - } > + if (virJSONValueObjectGetNumberDouble(ram, "mbps", &mbps) == 0 && > + mbps > 0) { > + /* mpbs from QEMU reports Mbits/s (M as in 10^6 not Mi as 2^20) */ > + stats->ram_bps = mbps * (1000 * 1000 / 8); > + } > - if (virJSONValueObjectGetNumberDouble(ram, "mbps", &mbps) == 0 && > - mbps > 0) { > - /* mpbs from QEMU reports Mbits/s (M as in 10^6 not Mi as 2^20) */ > - stats->ram_bps = mbps * (1000 * 1000 / 8); > + if (virJSONValueObjectGetNumberUlong(ram, "duplicate", > + &stats->ram_duplicate) == 0) > + stats->ram_duplicate_set = true; > + ignore_value(virJSONValueObjectGetNumberUlong(ram, "normal", > + &stats->ram_normal)); > + ignore_value(virJSONValueObjectGetNumberUlong(ram, "normal-bytes", > + &stats->ram_normal_bytes)); > + ignore_value(virJSONValueObjectGetNumberUlong(ram, "dirty-pages-rate", > + &stats->ram_dirty_rate)); > + ignore_value(virJSONValueObjectGetNumberUlong(ram, "page-size", > + &stats->ram_page_size)); > + ignore_value(virJSONValueObjectGetNumberUlong(ram, "dirty-sync-count", > + &stats->ram_iteration)); > + ignore_value(virJSONValueObjectGetNumberUlong(ram, "postcopy-requests", > + &stats->ram_postcopy_reqs)); > } > - if (virJSONValueObjectGetNumberUlong(ram, "duplicate", > - &stats->ram_duplicate) == 0) > - stats->ram_duplicate_set = true; > - ignore_value(virJSONValueObjectGetNumberUlong(ram, "normal", > - &stats->ram_normal)); > - ignore_value(virJSONValueObjectGetNumberUlong(ram, "normal-bytes", > - &stats->ram_normal_bytes)); > - ignore_value(virJSONValueObjectGetNumberUlong(ram, "dirty-pages-rate", > - &stats->ram_dirty_rate)); > - ignore_value(virJSONValueObjectGetNumberUlong(ram, "page-size", > - &stats->ram_page_size)); > - ignore_value(virJSONValueObjectGetNumberUlong(ram, "dirty-sync-count", > - &stats->ram_iteration)); > - ignore_value(virJSONValueObjectGetNumberUlong(ram, "postcopy-requests", > - &stats->ram_postcopy_reqs)); > - > disk = virJSONValueObjectGetObject(ret, "disk"); > if (disk) { > rc = virJSONValueObjectGetNumberUlong(disk, "transferred", > .