Re: [libvirt PATCH 4/4] qemu: Enable migration events only for fresh QEMU process

On Thu, May 12, 2022 at 10:55:47 +0200, Jiri Denemark wrote: > Every running QEMU process we are willing to reconnect (i.e., at least > 3.1.0) supports migration events and we can assume the capability is > already enabled since last time libvirt daemon connected to its monitor. > > Well, it's not guaranteed though. If libvirt 1.2.17 or older was used to > start QEMU 3.1.0 or newer, migration events would not be enabled. And if > the user decides to upgrade libvirt from 1.2.17 to 8.4.0 while the QEMU > process is still running, they would not be able to migrate the domain But doesn't the function below actually enable the events? Or is there something else that needs to be done? Such that we simply could enable the events and be done with it?

> because of disabled migration events. I think we do not really need to > worry about this scenario as libvirt 1.2.17 is 7 years old while QEMU > 3.1.0 was released only 3.5 years ago. Thus a chance someone would be > running such configuration should be fairly small and a combination with > upgrading 1.2.17 to 8.4.0 (or newer) with running domains should get it > pretty much to zero. The issue would disappear ff the ancient libvirt is > first upgraded to something older than 8.4.0 and then to the current > libvirt. > > Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; > --- > > Notes: > I was hoping we could do this without any downside, but it appeared this > was not possible. In case someone still thinks this would be an issue, I > can take an alternative solution and check whether migration events are > enabled when reconnecting to QEMU monitor. > > src/qemu/qemu_migration_params.c | 26 ++++++++++++++------------ > src/qemu/qemu_migration_params.h | 3 ++- > src/qemu/qemu_process.c | 14 +++++++++----- > 3 files changed, 25 insertions(+), 18 deletions(-) > > diff --git a/src/qemu/qemu_migration_params.c b/src/qemu/qemu_migration_params.c > index 26754f03f8..95fd773645 100644 > --- a/src/qemu/qemu_migration_params.c > +++ b/src/qemu/qemu_migration_params.c > @@ -1385,10 +1385,10 @@ qemuMigrationParamsParse(xmlXPathContextPtr ctxt, > int > qemuMigrationCapsCheck(virQEMUDriver *driver, > virDomainObj *vm, > - int asyncJob) > + int asyncJob, > + bool reconnect) > { > qemuDomainObjPrivate *priv = vm->privateData; > - g_autoptr(virBitmap) migEvent = NULL; > g_autoptr(virJSONValue) json = NULL; > g_auto(GStrv) caps = NULL; > char **capStr; > @@ -1419,22 +1419,24 @@ qemuMigrationCapsCheck(virQEMUDriver *driver, > } > } > > - migEvent = virBitmapNew(QEMU_MIGRATION_CAP_LAST); > + if (!reconnect) { > + g_autoptr(virBitmap) migEvent = virBitmapNew(QEMU_MIGRATION_CAP_LAST); > > - ignore_value(virBitmapSetBit(migEvent, QEMU_MIGRATION_CAP_EVENTS)); > + ignore_value(virBitmapSetBit(migEvent, QEMU_MIGRATION_CAP_EVENTS)); Here you assert the flag ... > > - if (!(json = qemuMigrationCapsToJSON(migEvent, migEvent))) > - return -1; > + if (!(json = qemuMigrationCapsToJSON(migEvent, migEvent))) > + return -1; > > - if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) > - return -1; > + if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) > + return -1; > > - rc = qemuMonitorSetMigrationCapabilities(priv->mon, &json); > + rc = qemuMonitorSetMigrationCapabilities(priv->mon, &json); ... and ask the monitor to enable it. To me that looks like we're good even with older qemus, right?