[PATCH 0/3] migration: Fix attempt to fix use of VIR_MIGRATE_PARAM_DEST_XML with VIR_MIGRATE_PERSIST_DEST
Revert the code, document the quirks. Peter Krempa (3): Revert "qemu: migration: Improve handling of VIR_MIGRATE_PARAM_DEST_XML with VIR_MIGRATE_PERSIST_DEST" API: migration: Warn about use of VIR_MIGRATE_PERSIST_DEST with VIR_MIGRATE_PARAM_DEST_XML manpage: virsh: Add warning about 'migrate' with '--persistent' together with '--xml' docs/manpages/virsh.rst | 22 +++++++++++++-------- include/libvirt/libvirt-domain.h | 13 +++++++++++++ src/qemu/qemu_migration.c | 33 ++++++++++---------------------- 3 files changed, 37 insertions(+), 31 deletions(-) -- 2.47.0
The original intention was to improve the behaviour of the VIR_MIGRATE_PERSIST_DEST flag which makes the VM persistent after migration on the destination when used with VIR_MIGRATE_PARAM_DEST_XML. While it worked as intended with p2p migration where the migration is driven from the virtqemud instance on the source of the migration, which can distinguish between the user-provided input XML and the one fetched from the source of the migration, it's not easily possible to achieve the same behaviour with normal migration driven from the client library. The approach also still had corner cases (originally deemed worth changing) such as if the persistent definition was modified it would be overwritten. As there is no clear fix which would improve both styles of migrations with no corner cases revert the change. Upcoming commits will modify the documentation to add warning about the use of VIR_MIGRATE_PERSIST_DEST with VIR_MIGRATE_PARAM_DEST_XML/xmlin without using VIR_MIGRATE_PARAM_PERSIST_XML instead of a code fix. This reverts commit 6a385590926d01ab2f2137d1d0833ae797cd2839. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration.c | 33 ++++++++++----------------------- 1 file changed, 10 insertions(+), 23 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 832b2946e0..26a92d8ee2 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -4829,7 +4829,6 @@ qemuMigrationSrcCancel(virDomainObj *vm, static int qemuMigrationSrcRun(virQEMUDriver *driver, virDomainObj *vm, - const char *xmlin, const char *persist_xml, const char *cookiein, int cookieinlen, @@ -4902,15 +4901,6 @@ qemuMigrationSrcRun(virQEMUDriver *driver, persist_xml, NULL, NULL))) goto error; - } else if (xmlin) { - /* if input XML is provided, use that one as template for the - * persistent XML. Otherwise user's changes will be thrown away. - */ - if (!(persistDef = qemuMigrationAnyPrepareDef(driver, - priv->qemuCaps, - xmlin, - NULL, NULL))) - goto error; } else { virDomainDef *def = vm->newDef ? vm->newDef : vm->def; if (!(persistDef = qemuDomainDefCopy(driver, priv->qemuCaps, def, @@ -5267,7 +5257,6 @@ qemuMigrationSrcResume(virDomainObj *vm, static int qemuMigrationSrcPerformNative(virQEMUDriver *driver, virDomainObj *vm, - const char *xmlin, const char *persist_xml, const char *uri, const char *cookiein, @@ -5356,7 +5345,7 @@ qemuMigrationSrcPerformNative(virQEMUDriver *driver, ret = qemuMigrationSrcResume(vm, migParams, cookiein, cookieinlen, cookieout, cookieoutlen, &spec, dconn, flags); } else { - ret = qemuMigrationSrcRun(driver, vm, xmlin, persist_xml, cookiein, cookieinlen, + ret = qemuMigrationSrcRun(driver, vm, persist_xml, cookiein, cookieinlen, cookieout, cookieoutlen, flags, resource, &spec, dconn, graphicsuri, migrate_disks, migrate_disks_detect_zeroes, @@ -5374,7 +5363,6 @@ static int qemuMigrationSrcPerformTunnel(virQEMUDriver *driver, virDomainObj *vm, virStreamPtr st, - const char *xmlin, const char *persist_xml, const char *cookiein, int cookieinlen, @@ -5422,7 +5410,7 @@ qemuMigrationSrcPerformTunnel(virQEMUDriver *driver, /* Migration with NBD is not supported with _TUNNELLED, thus * 'migrate_disks_detect_zeroes' is NULL here */ - ret = qemuMigrationSrcRun(driver, vm, xmlin, persist_xml, cookiein, cookieinlen, + ret = qemuMigrationSrcRun(driver, vm, persist_xml, cookiein, cookieinlen, cookieout, cookieoutlen, flags, resource, &spec, dconn, graphicsuri, migrate_disks, NULL, migParams, NULL); @@ -5461,7 +5449,7 @@ qemuMigrationSrcPerformResume(virQEMUDriver *driver, virCloseCallbacksDomainRemove(vm, NULL, qemuMigrationAnyConnectionClosed); qemuDomainCleanupRemove(vm, qemuProcessCleanupMigrationJob); - ret = qemuMigrationSrcPerformNative(driver, vm, NULL, NULL, uri, + ret = qemuMigrationSrcPerformNative(driver, vm, NULL, uri, cookiein, cookieinlen, cookieout, cookieoutlen, flags, 0, NULL, NULL, NULL, NULL, migParams, NULL); @@ -5563,12 +5551,12 @@ qemuMigrationSrcPerformPeer2Peer2(virQEMUDriver *driver, VIR_DEBUG("Perform %p", sconn); ignore_value(qemuMigrationJobStartPhase(vm, QEMU_MIGRATION_PHASE_PERFORM2)); if (flags & VIR_MIGRATE_TUNNELLED) - ret = qemuMigrationSrcPerformTunnel(driver, vm, st, NULL, NULL, + ret = qemuMigrationSrcPerformTunnel(driver, vm, st, NULL, NULL, 0, NULL, NULL, flags, resource, dconn, NULL, NULL, migParams); else - ret = qemuMigrationSrcPerformNative(driver, vm, NULL, NULL, uri_out, + ret = qemuMigrationSrcPerformNative(driver, vm, NULL, uri_out, cookie, cookielen, NULL, NULL, /* No out cookie with v2 migration */ flags, resource, dconn, NULL, NULL, @@ -5838,14 +5826,14 @@ qemuMigrationSrcPerformPeer2Peer3(virQEMUDriver *driver, } else { ignore_value(qemuMigrationJobSetPhase(vm, QEMU_MIGRATION_PHASE_PERFORM3)); if (flags & VIR_MIGRATE_TUNNELLED) { - ret = qemuMigrationSrcPerformTunnel(driver, vm, st, xmlin, persist_xml, + ret = qemuMigrationSrcPerformTunnel(driver, vm, st, persist_xml, cookiein, cookieinlen, &cookieout, &cookieoutlen, flags, bandwidth, dconn, graphicsuri, migrate_disks, migParams); } else { - ret = qemuMigrationSrcPerformNative(driver, vm, xmlin, persist_xml, uri, + ret = qemuMigrationSrcPerformNative(driver, vm, persist_xml, uri, cookiein, cookieinlen, &cookieout, &cookieoutlen, flags, bandwidth, dconn, graphicsuri, @@ -6243,7 +6231,7 @@ qemuMigrationSrcPerformJob(virQEMUDriver *driver, if (qemuMigrationJobStartPhase(vm, QEMU_MIGRATION_PHASE_PERFORM2) < 0) goto endjob; - ret = qemuMigrationSrcPerformNative(driver, vm, xmlin, persist_xml, uri, cookiein, cookieinlen, + ret = qemuMigrationSrcPerformNative(driver, vm, persist_xml, uri, cookiein, cookieinlen, cookieout, cookieoutlen, flags, resource, NULL, NULL, NULL, NULL, migParams, nbdURI); @@ -6308,7 +6296,6 @@ static int qemuMigrationSrcPerformPhase(virQEMUDriver *driver, virConnectPtr conn, virDomainObj *vm, - const char *xmlin, const char *persist_xml, const char *uri, const char *graphicsuri, @@ -6346,7 +6333,7 @@ qemuMigrationSrcPerformPhase(virQEMUDriver *driver, virCloseCallbacksDomainRemove(vm, NULL, qemuMigrationAnyConnectionClosed); - if (qemuMigrationSrcPerformNative(driver, vm, xmlin, persist_xml, uri, cookiein, cookieinlen, + if (qemuMigrationSrcPerformNative(driver, vm, persist_xml, uri, cookiein, cookieinlen, cookieout, cookieoutlen, flags, resource, NULL, graphicsuri, migrate_disks, migrate_disks_detect_zeroes, @@ -6450,7 +6437,7 @@ qemuMigrationSrcPerform(virQEMUDriver *driver, } if (v3proto) { - return qemuMigrationSrcPerformPhase(driver, conn, vm, xmlin, persist_xml, uri, + return qemuMigrationSrcPerformPhase(driver, conn, vm, persist_xml, uri, graphicsuri, migrate_disks, migrate_disks_detect_zeroes, migParams, -- 2.47.0
When a VM is being migrated to a destination host it can be made persistent on the destination by using VIR_MIGRATE_PERSIST_DEST. That may not work as intended if VIR_MIGRATE_PARAM_DEST_XML or the 'xmlin' parameter is used as that allows overriding certain aspects of the VM xml, but does not involve the persistent definition. In most cases users will need to supply also VIR_MIGRATE_PARAM_PERSIST_XML with the same set of modification. Modify the man page to clarify the above so that users don't end up with broken VM after migrating and restarting it. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- include/libvirt/libvirt-domain.h | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 9232ce2e6b..d4f1573954 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -929,6 +929,19 @@ typedef enum { * VIR_MIGRATE_UNDEFINE_SOURCE is not used, it will end up persistent on * both hosts. * + * Note: If VIR_MIGRATE_PERSIST_DEST flag is used together with the + * VIR_MIGRATE_PARAM_DEST_XML migration parameter which supplies an + * updated definition for the destination host it's required to + * supply also VIR_MIGRATE_PARAM_PERSIST_XML updated the same way. + * Otherwise the persistent definition on the destination will not + * contain the updates. + * + * The VIR_MIGRATE_PERSIST_DEST flag should not be used with the + * "xmlin" parameter of older APIs as that way an updated persistent + * XML can't be supplied and thus the persistent definition will + * likely be incorrect as it will be based on the persistent definition + * on the source of the migration. + * * Since: 0.7.3 */ VIR_MIGRATE_PERSIST_DEST = (1 << 3), -- 2.47.0
When a VM is being migrated to a destination host it can be made persistent on the destination by using '--persistent'. That may not work as intended if '--xml' is used as well as that allows overriding certain aspects of the VM xml, but does not involve the persistent definition. In most cases users will need to supply also '--persistent-xml' with the same set of modification. Modify the man page to clarify the above so that users don't end up with broken VM after migrating and restarting it. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- docs/manpages/virsh.rst | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index 6776ea53d0..2e525d3fac 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -3409,7 +3409,8 @@ starting the domain on destination and without stopping it on source host. Offline migration may be used with inactive domains and it must be used with *--persistent* option. -*--persistent* leaves the domain persistent on destination host, +*--persistent* leaves the domain persistent on destination host (See below +for quirks when used together with *--xml*), *--undefinesource* undefines the domain on the source host, and *--suspend* leaves the domain paused on the destination host. @@ -3489,13 +3490,18 @@ such as GFS2 or GPFS. If you are sure the migration is safe or you just do not care, use *--unsafe* to force the migration. *dname* is used for renaming the domain to new name during migration, which -also usually can be omitted. Likewise, *--xml* ``file`` is usually -omitted, but can be used to supply an alternative XML file for use on -the destination to supply a larger set of changes to any host-specific -portions of the domain XML, such as accounting for naming differences -between source and destination in accessing underlying storage. -If *--persistent* is enabled, *--persistent-xml* ``file`` can be used to -supply an alternative XML file which will be used as the persistent guest +also usually can be omitted. + +*--xml* ``file``, while usually not required, can be used to supply an +alternative XML file for use on the destination to supply a larger set of +changes to any host-specific portions of the domain XML, such as accounting for +naming differences between source and destination in accessing underlying +storage. If *--xml* is used together with *--persistent* it's usually required +to provide a persistent XML definition via *--persistent-xml* (see below) which +is fixed the same way as the XML passed to *--file* was. + +If *--persistent* is enabled, *--persistent-xml* ``file`` can be used +to supply an alternative XML file which will be used as the persistent guest definition on the destination host. *--timeout* ``seconds`` tells virsh to run a specified action when live -- 2.47.0
On 11/18/24 14:59, Peter Krempa wrote:
Revert the code, document the quirks.
Peter Krempa (3): Revert "qemu: migration: Improve handling of VIR_MIGRATE_PARAM_DEST_XML with VIR_MIGRATE_PERSIST_DEST" API: migration: Warn about use of VIR_MIGRATE_PERSIST_DEST with VIR_MIGRATE_PARAM_DEST_XML manpage: virsh: Add warning about 'migrate' with '--persistent' together with '--xml'
docs/manpages/virsh.rst | 22 +++++++++++++-------- include/libvirt/libvirt-domain.h | 13 +++++++++++++ src/qemu/qemu_migration.c | 33 ++++++++++---------------------- 3 files changed, 37 insertions(+), 31 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (2)
-
Michal Prívozník -
Peter Krempa