Re: [libvirt PATCH v3 3/4] qemu_migration: get migration blockers before hardcoded checks
On Wed, Jul 20, 2022 at 4:01 PM Jiri Denemark <jdenemar(a)redhat.com> wrote:
On Wed, Jul 20, 2022 at 14:15:57 +0200, Eugenio Pérez wrote:
> since qemu 6.0, if migration is blocked for some reason, 'query-migrate'
> will return an array of error strings describing the migration blockers.
> This can be used to check whether there are any devices blocking
> migration, etc.
>
> Enable qemuMigrationSrcIsAllowed to query it.
>
> Signed-off-by: Eugenio Pérez <eperezma(a)redhat.com>
> ---
> v3:
> * Report message with a colon.
> * Report all blockers instead of only the first.
> ---
> src/qemu/qemu_migration.c | 34 ++++++++++++++++++++++++++++++++++
> 1 file changed, 34 insertions(+)
>
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index b12cb518ee..6ac4ef150b 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -1414,6 +1414,20 @@ qemuMigrationSrcIsAllowedHostdev(const virDomainDef *def)
> return true;
> }
>
> +static int
> +qemuDomainGetMigrationBlockers(virQEMUDriver *driver,
> + virDomainObj *vm,
> + char ***blockers)
> +{
> + qemuDomainObjPrivate *priv = vm->privateData;
> + int rc;
> +
> + qemuDomainObjEnterMonitor(driver, vm);
> + rc = qemuMonitorGetMigrationBlockers(priv->mon, blockers);
> + qemuDomainObjExitMonitor(vm);
> +
> + return rc;
> +}
>
> /**
> * qemuMigrationSrcIsAllowed:
> @@ -1439,6 +1453,26 @@ qemuMigrationSrcIsAllowed(virQEMUDriver *driver,
> int nsnapshots;
> int pauseReason;
> size_t i;
> + int r;
> +
> + /* Ask qemu if it have a migration blocker */
> + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_BLOCKED_REASONS)) {
> + g_auto(GStrv) blockers = NULL;
> + r = qemuDomainGetMigrationBlockers(driver, vm, &blockers);
> + if (r != 0) {
> + virReportError(VIR_ERR_OPERATION_INVALID,
> + _("cannot migrate domain: %s"),
> + _("error getting blockers"));
> + return false;
> + }
As mentioned in v2 review the virReportError call should be dropped as
it overwrites the error reported by qemuDomainGetMigrationBlockers. That
is, you can just
if (qemuDomainGetMigrationBlockers(driver, vm, &blockers) < 0)
return false;
But there are a few conditions that don't report an error, like a bad
JSON answer. For example, if "blockers" is not an array parsing the
response JSON, libvirt would not print any error, isn't it?
> +
> + if (blockers && blockers[0]) {
> + g_autofree char *reasons = g_strjoinv(", ", blockers);
In the following patch you change ", " to "; ". I don't mind that
much
either way, but it should be done in this patch :-)
> + virReportError(VIR_ERR_OPERATION_INVALID,
> + _("cannot migrate domain: %s"), reasons);
> + return false;
> + }
> + }
>
> /* perform these checks only when migrating to remote hosts */
> if (remote) {
Hmm, easy but not trivial changes so I guess v4 would be better.
Jirka