[libvirt] [PATCH 0/2] qemu: Remove stale transient def when migration fails

If a migration of a domain which is already defined on the destination host failed early (before we tried to start QEMU), we would forget to remove the incoming transient definition. Later on when someone starts the domain on the destination host, we will use the stale incoming definition and the persistent definition will just be ignored. https://bugzilla.redhat.com/show_bug.cgi?id=1368774 Jiri Denemark (2): Add helper for removing transient definition qemu: Remove stale transient def when migration fails src/conf/domain_conf.c | 19 +++++++++++++++++++ src/conf/domain_conf.h | 1 + src/libvirt_private.syms | 1 + src/libxl/libxl_domain.c | 8 +------- src/lxc/lxc_process.c | 7 +------ src/qemu/qemu_migration.c | 4 ++++ src/qemu/qemu_process.c | 7 +------ src/test/test_driver.c | 9 ++------- src/uml/uml_driver.c | 14 ++------------ 9 files changed, 32 insertions(+), 38 deletions(-) -- 2.10.0

The code for replacing domain's transient definition with the persistent one is repeated in several places and we'll need to add one more. Let's make a nice helper for it. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/conf/domain_conf.c | 19 +++++++++++++++++++ src/conf/domain_conf.h | 1 + src/libvirt_private.syms | 1 + src/libxl/libxl_domain.c | 8 +------- src/lxc/lxc_process.c | 7 +------ src/qemu/qemu_process.c | 7 +------ src/test/test_driver.c | 9 ++------- src/uml/uml_driver.c | 14 ++------------ 8 files changed, 28 insertions(+), 38 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c8c4f61..db030cc 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2981,6 +2981,25 @@ virDomainObjSetDefTransient(virCapsPtr caps, return ret; } + +/* + * Remove the running configuration and replace it with the persistent one. + * + * @param domain domain object pointer + */ +void +virDomainObjRemoveTransientDef(virDomainObjPtr domain) +{ + if (!domain->newDef) + return; + + virDomainDefFree(domain->def); + domain->def = domain->newDef; + domain->def->id = -1; + domain->newDef = NULL; +} + + /* * Return the persistent domain configuration. If domain is transient, * return the running config. diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 0fe4154..79dda1c 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2577,6 +2577,7 @@ void virDomainObjAssignDef(virDomainObjPtr domain, int virDomainObjSetDefTransient(virCapsPtr caps, virDomainXMLOptionPtr xmlopt, virDomainObjPtr domain); +void virDomainObjRemoveTransientDef(virDomainObjPtr domain); virDomainDefPtr virDomainObjGetPersistentDef(virCapsPtr caps, virDomainXMLOptionPtr xmlopt, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a5fa305..6a77e46 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -421,6 +421,7 @@ virDomainObjGetShortName; virDomainObjGetState; virDomainObjNew; virDomainObjParseNode; +virDomainObjRemoveTransientDef; virDomainObjSetDefTransient; virDomainObjSetMetadata; virDomainObjSetState; diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index a85dd75..43f4a7f 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -811,13 +811,7 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver, VIR_FREE(xml); } - if (vm->newDef) { - virDomainDefFree(vm->def); - vm->def = vm->newDef; - vm->def->id = -1; - vm->newDef = NULL; - } - + virDomainObjRemoveTransientDef(vm); virObjectUnref(cfg); } diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index 7703fe1..bce6a2f 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -246,12 +246,7 @@ static void virLXCProcessCleanup(virLXCDriverPtr driver, VIR_FREE(xml); } - if (vm->newDef) { - virDomainDefFree(vm->def); - vm->def = vm->newDef; - vm->def->id = -1; - vm->newDef = NULL; - } + virDomainObjRemoveTransientDef(vm); virObjectUnref(cfg); } diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index e6e91df..7596579 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -6039,12 +6039,7 @@ void qemuProcessStop(virQEMUDriverPtr driver, VIR_FREE(xml); } - if (vm->newDef) { - virDomainDefFree(vm->def); - vm->def = vm->newDef; - vm->def->id = -1; - vm->newDef = NULL; - } + virDomainObjRemoveTransientDef(vm); endjob: if (asyncJob != QEMU_ASYNC_JOB_NONE) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 53cfa3c..8dd7579 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -588,14 +588,9 @@ testDomainShutdownState(virDomainPtr domain, virDomainObjPtr privdom, virDomainShutoffReason reason) { - if (privdom->newDef) { - virDomainDefFree(privdom->def); - privdom->def = privdom->newDef; - privdom->newDef = NULL; - } - + virDomainObjRemoveTransientDef(privdom); virDomainObjSetState(privdom, VIR_DOMAIN_SHUTOFF, reason); - privdom->def->id = -1; + if (domain) domain->id = -1; } diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index b978453..4f25f76 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -1135,12 +1135,7 @@ static int umlStartVMDaemon(virConnectPtr conn, if (ret < 0) { virDomainConfVMNWFilterTeardown(vm); umlCleanupTapDevices(vm); - if (vm->newDef) { - virDomainDefFree(vm->def); - vm->def = vm->newDef; - vm->def->id = -1; - vm->newDef = NULL; - } + virDomainObjRemoveTransientDef(vm); } /* NB we don't mark it running here - we do that async @@ -1182,12 +1177,7 @@ static void umlShutdownVMDaemon(struct uml_driver *driver, /* Stop autodestroy in case guest is restarted */ umlProcessAutoDestroyRemove(driver, vm); - if (vm->newDef) { - virDomainDefFree(vm->def); - vm->def = vm->newDef; - vm->def->id = -1; - vm->newDef = NULL; - } + virDomainObjRemoveTransientDef(vm); driver->nactive--; if (!driver->nactive && driver->inhibitCallback) -- 2.10.0

On Thu, Sep 08, 2016 at 16:21:03 +0200, Jiri Denemark wrote:
The code for replacing domain's transient definition with the persistent one is repeated in several places and we'll need to add one more. Let's make a nice helper for it.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/conf/domain_conf.c | 19 +++++++++++++++++++ src/conf/domain_conf.h | 1 + src/libvirt_private.syms | 1 + src/libxl/libxl_domain.c | 8 +------- src/lxc/lxc_process.c | 7 +------ src/qemu/qemu_process.c | 7 +------ src/test/test_driver.c | 9 ++------- src/uml/uml_driver.c | 14 ++------------ 8 files changed, 28 insertions(+), 38 deletions(-)
ACK

If a migration of a domain which is already defined on the destination host failed early (before we tried to start QEMU), we would forget to remove the incoming transient definition. Later on when someone starts the domain on the destination host, we will use the stale incoming definition and the persistent definition will just be ignored. https://bugzilla.redhat.com/show_bug.cgi?id=1368774 Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_migration.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index e451ef6..6ef2396 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -3874,6 +3874,10 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, priv->nbdPort = 0; qemuDomainRemoveInactive(driver, vm); } + + if (ret < 0) + virDomainObjRemoveTransientDef(vm); + virDomainObjEndAPI(&vm); qemuDomainEventQueue(driver, event); qemuMigrationCookieFree(mig); -- 2.10.0

On Thu, Sep 08, 2016 at 16:21:04 +0200, Jiri Denemark wrote:
If a migration of a domain which is already defined on the destination host failed early (before we tried to start QEMU), we would forget to remove the incoming transient definition. Later on when someone starts the domain on the destination host, we will use the stale incoming definition and the persistent definition will just be ignored.
https://bugzilla.redhat.com/show_bug.cgi?id=1368774
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_migration.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index e451ef6..6ef2396 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -3874,6 +3874,10 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, priv->nbdPort = 0; qemuDomainRemoveInactive(driver, vm); } + + if (ret < 0) + virDomainObjRemoveTransientDef(vm);
This needs to go to the block above. 'priv' is set if the domain def was assigned (basically after virDomainObjListAdd) succeeded. Currently it would crash if the target VM would be running. 'priv' basically guarantees that 'vm' is valid. ACK if you move this into the block above.
participants (2)
-
Jiri Denemark
-
Peter Krempa