qemu: Don't set migration caps when changing postcopy bandwidth

[libvirt] [PATCH] qemu: Don't set migration caps when changing postcopy bandwidth

Posted by Jiri Denemark 5 years, 1 month ago

The qemuMigrationParamsApply internal API was designed to apply all
migration parameters and capabilities before we start to migrate a
domain. The API can also be called outside migration job, e.g., via a
call to qemuDomainMigrateSetMaxSpeed with
VIR_DOMAIN_MIGRATE_MAX_SPEED_POSTCOPY flag, but in that case it should
only apply migration parameters and ignore capabilities.

Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
---

Notes:
    Best viewed with --ignore-all-space

 src/qemu/qemu_migration_params.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/src/qemu/qemu_migration_params.c b/src/qemu/qemu_migration_params.c
index 24e5368783..623193cf6c 100644
--- a/src/qemu/qemu_migration_params.c
+++ b/src/qemu/qemu_migration_params.c
@@ -778,14 +778,23 @@ qemuMigrationParamsApply(virQEMUDriverPtr driver,
     if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
         return -1;
 
-    if (!(caps = qemuMigrationCapsToJSON(priv->migrationCaps, migParams->caps)))
-        goto cleanup;
-
-    if (virJSONValueArraySize(caps) > 0) {
-        rc = qemuMonitorSetMigrationCapabilities(priv->mon, caps);
-        caps = NULL;
-        if (rc < 0)
+    if (asyncJob == QEMU_ASYNC_JOB_NONE) {
+        if (!virBitmapIsAllClear(migParams->caps)) {
+            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                           _("Migration capabilities can only be set by "
+                             "a migration job"));
             goto cleanup;
+        }
+    } else {
+        if (!(caps = qemuMigrationCapsToJSON(priv->migrationCaps, migParams->caps)))
+            goto cleanup;
+
+        if (virJSONValueArraySize(caps) > 0) {
+            rc = qemuMonitorSetMigrationCapabilities(priv->mon, caps);
+            caps = NULL;
+            if (rc < 0)
+                goto cleanup;
+        }
     }
 
     /* If QEMU is too old to support xbzrle-cache-size migration parameter,
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu: Don't set migration caps when changing postcopy bandwidth

Posted by Ján Tomko 5 years, 1 month ago

On Wed, Mar 06, 2019 at 12:15:08PM +0100, Jiri Denemark wrote:
>The qemuMigrationParamsApply internal API was designed to apply all
>migration parameters and capabilities before we start to migrate a
>domain. The API can also be called outside migration job, e.g., via a
>call to qemuDomainMigrateSetMaxSpeed with
>VIR_DOMAIN_MIGRATE_MAX_SPEED_POSTCOPY flag, but in that case it should
>only apply migration parameters and ignore capabilities.
>

Is there a public bug you can link here?

>Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
>---
>
>Notes:
>    Best viewed with --ignore-all-space
>
> src/qemu/qemu_migration_params.c | 23 ++++++++++++++++-------
> 1 file changed, 16 insertions(+), 7 deletions(-)
>

Reviewed-by: Ján Tomko <jtomko@redhat.com>

Jano
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu: Don't set migration caps when changing postcopy bandwidth

Posted by Jiri Denemark 5 years, 1 month ago

On Wed, Mar 06, 2019 at 12:29:05 +0100, Ján Tomko wrote:
> On Wed, Mar 06, 2019 at 12:15:08PM +0100, Jiri Denemark wrote:
> >The qemuMigrationParamsApply internal API was designed to apply all
> >migration parameters and capabilities before we start to migrate a
> >domain. The API can also be called outside migration job, e.g., via a
> >call to qemuDomainMigrateSetMaxSpeed with
> >VIR_DOMAIN_MIGRATE_MAX_SPEED_POSTCOPY flag, but in that case it should
> >only apply migration parameters and ignore capabilities.
> >
> 
> Is there a public bug you can link here?

No.

Jirka

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu: Don't set migration caps when changing postcopy bandwidth

Posted by Peter Krempa 5 years, 1 month ago

On Wed, Mar 06, 2019 at 12:15:08 +0100, Jiri Denemark wrote:
> The qemuMigrationParamsApply internal API was designed to apply all
> migration parameters and capabilities before we start to migrate a
> domain. The API can also be called outside migration job, e.g., via a
> call to qemuDomainMigrateSetMaxSpeed with

Is it expected that if it's called inside the migration job and the caps
bitmap is empty the monitor command will be called with an empty array?

> VIR_DOMAIN_MIGRATE_MAX_SPEED_POSTCOPY flag, but in that case it should
> only apply migration parameters and ignore capabilities.

ACK, but the wording is weird. You don't really ignore any caps because
you explicitly reject it. I presume the problem is in the fact that if
migration is not running the API does not have efect.

Obviously v2 required if the problem is actually that the monitor
command is unhappy with empty array as the argument.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu: Don't set migration caps when changing postcopy bandwidth

Posted by Jiri Denemark 5 years, 1 month ago

On Wed, Mar 06, 2019 at 12:39:56 +0100, Peter Krempa wrote:
> On Wed, Mar 06, 2019 at 12:15:08 +0100, Jiri Denemark wrote:
> > The qemuMigrationParamsApply internal API was designed to apply all
> > migration parameters and capabilities before we start to migrate a
> > domain. The API can also be called outside migration job, e.g., via a
> > call to qemuDomainMigrateSetMaxSpeed with
> 
> Is it expected that if it's called inside the migration job and the caps
> bitmap is empty the monitor command will be called with an empty array?

The bitmap will always have QEMU_MIGRATION_CAP_LAST bits. They can all
be zeros, but that will only result in an empty json array if QEMU does
not support migration capabilities at all. All supported capabilities
will be explicitly disabled and thus the array won't be empty. Even if
it was empty, the monitor command is only called when
virJSONValueArraySize(caps) > 0.

> > VIR_DOMAIN_MIGRATE_MAX_SPEED_POSTCOPY flag, but in that case it should
> > only apply migration parameters and ignore capabilities.
> 
> ACK, but the wording is weird. You don't really ignore any caps because
> you explicitly reject it.

The code is ignoring capabilities when they are all disabled, which is
the default state after qemuMigrationParamsNew(). When the caller is not
a migration job and it tries to enable any capability, an error is
reported as this would be a bug in our code.

> I presume the problem is in the fact that if migration is not running
> the API does not have efect.
>
> Obviously v2 required if the problem is actually that the monitor
> command is unhappy with empty array as the argument.

The actual problem is that the migrate-set-capabilities QMP command can
only be called before migration starts and calling it while migration is
already running does not make (and it is actually rejected by QEMU).

So how about:

    The qemuMigrationParamsApply internal API was designed to apply all
    migration parameters and capabilities before we start to migrate a
    domain. While migration parameters are only passed to QEMU when we
    explicitly want to set a specific value, capabilities are always
    either enabled or disabled.

    Thus when this API is called outside migration job, e.g., via a call
    to qemuDomainMigrateSetMaxSpeed with
    VIR_DOMAIN_MIGRATE_MAX_SPEED_POSTCOPY flag, we would call
    migrate-set-capabilities and disable all capabilities. However,
    changing capabilities while migration is already running does not
    make sense and our code should never be trying to do so. In fact
    QEMU even reports an error if migrate-set-capabilities is called
    during migration and qemuDomainMigrateSetMaxSpeed would fail with:

        internal error: unable to execute QEMU command
        migrate-set-capabilities: There's a migration process in
        progress

    With this patch qemuMigrationParamsApply never tries to call
    migrate-set-capabilities outside of migration job. When the
    capabilities bitmap is all zeros (which is its initial value after
    qemuMigrationParamsNew), we just skip the command. But when any
    capability bit is set to 1 by a non-migration job, we report an
    error to highlight a bug in our code.

Jirka

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu: Don't set migration caps when changing postcopy bandwidth

Posted by Peter Krempa 5 years, 1 month ago

On Wed, Mar 06, 2019 at 13:31:14 +0100, Jiri Denemark wrote:
> On Wed, Mar 06, 2019 at 12:39:56 +0100, Peter Krempa wrote:
> > On Wed, Mar 06, 2019 at 12:15:08 +0100, Jiri Denemark wrote:

[...]

> So how about:
> 
>     The qemuMigrationParamsApply internal API was designed to apply all
>     migration parameters and capabilities before we start to migrate a
>     domain. While migration parameters are only passed to QEMU when we
>     explicitly want to set a specific value, capabilities are always
>     either enabled or disabled.
> 
>     Thus when this API is called outside migration job, e.g., via a call
>     to qemuDomainMigrateSetMaxSpeed with
>     VIR_DOMAIN_MIGRATE_MAX_SPEED_POSTCOPY flag, we would call
>     migrate-set-capabilities and disable all capabilities. However,
>     changing capabilities while migration is already running does not
>     make sense and our code should never be trying to do so. In fact
>     QEMU even reports an error if migrate-set-capabilities is called
>     during migration and qemuDomainMigrateSetMaxSpeed would fail with:
> 
>         internal error: unable to execute QEMU command
>         migrate-set-capabilities: There's a migration process in
>         progress
> 
>     With this patch qemuMigrationParamsApply never tries to call
>     migrate-set-capabilities outside of migration job. When the
>     capabilities bitmap is all zeros (which is its initial value after
>     qemuMigrationParamsNew), we just skip the command. But when any
>     capability bit is set to 1 by a non-migration job, we report an
>     error to highlight a bug in our code.

ACK, this explains it nicely.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list