[libvirt] [PATCH 0/4] Various bugfixes for cancelled VM migrations
This patch series contains fixes for several bugs I encountered while deliberately forcing a VM migration to abort by killing the libvirt client. My VM has storage on local disk, which needs to be mirrored during the migration, and this gives ample time for this abort to take place. The particular bug I encountered depended on precisely which phase the migration had made it to (e.g. whether disk mirroring had actually commenced). Patch 1 fixes a crash on the destination libvirt daemon due to a use-after-free of the domain object. Patches 2 and 4 fix some bugs related to the close callback handling on the source libvirt side. Patch 3 ensures that the VM on the source libvirt does not get into an invalid state if its migration is aborted during disk mirroring. All patches are independent of one another and can be applied separately. Michael Chapman (4): qemu: fix crash in qemuProcessAutoDestroy qemu: fix error propagation in qemuMigrationBegin qemu: fix race between disk mirror fail and cancel util: fix removal of callbacks in virCloseCallbacksRun src/qemu/qemu_domain.c | 5 +++++ src/qemu/qemu_migration.c | 12 +++++++++++- src/qemu/qemu_process.c | 4 +++- src/util/virclosecallbacks.c | 10 ++++++---- 4 files changed, 25 insertions(+), 6 deletions(-) -- 2.1.0
The destination libvirt daemon in a migration may segfault if the client disconnects immediately after the migration has begun: # virsh -c qemu+tls://remote/system list --all Id Name State ---------------------------------------------------- ... # timeout --signal KILL 1 \ virsh migrate example qemu+tls://remote/system \ --verbose --compressed --live --auto-converge \ --abort-on-error --unsafe --persistent \ --undefinesource --copy-storage-all --xml example.xml Killed # virsh -c qemu+tls://remote/system list --all error: failed to connect to the hypervisor error: unable to connect to server at 'remote:16514': Connection refused The crash is in: 1531 void 1532 qemuDomainObjEndJob(virQEMUDriverPtr driver, virDomainObjPtr obj) 1533 { 1534 qemuDomainObjPrivatePtr priv = obj->privateData; 1535 qemuDomainJob job = priv->job.active; 1536 1537 priv->jobs_queued--; Backtrace: #0 at qemuDomainObjEndJob at qemu/qemu_domain.c:1537 #1 in qemuDomainRemoveInactive at qemu/qemu_domain.c:2497 #2 in qemuProcessAutoDestroy at qemu/qemu_process.c:5646 #3 in virCloseCallbacksRun at util/virclosecallbacks.c:350 #4 in qemuConnectClose at qemu/qemu_driver.c:1154 ... qemuDomainRemoveInactive calls virDomainObjListRemove, which in this case is holding the last remaining reference to the domain. qemuDomainRemoveInactive then calls qemuDomainObjEndJob, but the domain object has been freed and poisoned by then. This patch bumps the domain's refcount until qemuDomainRemoveInactive has completed. We also ensure qemuProcessAutoDestroy does not return the domain to virCloseCallbacksRun to be unlocked in this case. There is similar logic in bhyveProcessAutoDestroy and lxcProcessAutoDestroy (which call virDomainObjListRemove directly). Signed-off-by: Michael Chapman <mike@very.puzzling.org> --- src/qemu/qemu_domain.c | 5 +++++ src/qemu/qemu_process.c | 4 +++- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index ac5ca74..d433cba 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2424,11 +2424,16 @@ qemuDomainRemoveInactive(virQEMUDriverPtr driver, VIR_WARN("unable to remove snapshot directory %s", snapDir); VIR_FREE(snapDir); } + + virObjectRef(vm); + virDomainObjListRemove(driver->domains, vm); virObjectUnref(cfg); if (haveJob) qemuDomainObjEndJob(driver, vm); + + virObjectUnref(vm); } void diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 515402e..554399e 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5689,8 +5689,10 @@ qemuProcessAutoDestroy(virDomainObjPtr dom, qemuDomainObjEndJob(driver, dom); - if (!dom->persistent) + if (!dom->persistent) { qemuDomainRemoveInactive(driver, dom); + dom = NULL; + } if (event) qemuDomainEventQueue(driver, event); -- 2.1.0
If virCloseCallbacksSet fails, qemuMigrationBegin must return NULL to indicate an error occurred. Signed-off-by: Michael Chapman <mike@very.puzzling.org> --- src/qemu/qemu_migration.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 9458606..21432c0 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2745,8 +2745,10 @@ qemuMigrationBegin(virConnectPtr conn, * place. */ if (virCloseCallbacksSet(driver->closeCallbacks, vm, conn, - qemuMigrationCleanup) < 0) + qemuMigrationCleanup) < 0) { + VIR_FREE(xml); goto endjob; + } qemuMigrationJobContinue(vm); } else { goto endjob; -- 2.1.0
If a VM migration is aborted, a disk mirror may be failed by QEMU before libvirt has a chance to cancel it. The disk->mirrorState remains at _ABORT in this case, and this breaks subsequent mirrorings of that disk. We should instead check the mirrorState directly and transition to _NONE if it is already aborted. Do the check *after* aborting the block job in QEMU to avoid a race. Signed-off-by: Michael Chapman <mike@very.puzzling.org> --- src/qemu/qemu_migration.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 21432c0..39ca37c 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1852,6 +1852,10 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver, } else { VIR_WARN("Unable to enter monitor. No block job cancelled"); } + + /* If disk mirror is already aborted, clear the mirror state now */ + if (disk->mirrorState == VIR_DOMAIN_DISK_MIRROR_STATE_ABORT) + disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE; } if (err) virSetError(err); @@ -1920,6 +1924,10 @@ qemuMigrationCancelDriveMirror(qemuMigrationCookiePtr mig, ret = -1; goto cleanup; } + + /* If disk mirror is already aborted, clear the mirror state now */ + if (disk->mirrorState == VIR_DOMAIN_DISK_MIRROR_STATE_ABORT) + disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE; } cleanup: -- 2.1.0
The close callbacks hash are keyed by a UUID-string, but virCloseCallbacksRun was attempting to remove them by raw UUID. This patch ensures the callback entries are removed by UUID-string as well. This bug could cause problems when guest migrations were abnormally aborted: # timeout --signal KILL 1 \ virsh migrate example qemu+tls://remote/system \ --verbose --compressed --live --auto-converge \ --abort-on-error --unsafe --persistent \ --undefinesource --copy-storage-all --xml example.xml Killed # virsh migrate example qemu+tls://remote/system \ --verbose --compressed --live --auto-converge \ --abort-on-error --unsafe --persistent \ --undefinesource --copy-storage-all --xml example.xml error: Requested operation is not valid: domain 'example' is not being migrated Signed-off-by: Michael Chapman <mike@very.puzzling.org> --- src/util/virclosecallbacks.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/util/virclosecallbacks.c b/src/util/virclosecallbacks.c index 4128057..b0a1a9e 100644 --- a/src/util/virclosecallbacks.c +++ b/src/util/virclosecallbacks.c @@ -236,6 +236,7 @@ typedef struct _virCloseCallbacksListEntry virCloseCallbacksListEntry; typedef virCloseCallbacksListEntry *virCloseCallbacksListEntryPtr; struct _virCloseCallbacksListEntry { unsigned char uuid[VIR_UUID_BUFLEN]; + char uuidstr[VIR_UUID_STRING_BUFLEN]; virCloseCallback callback; }; @@ -279,6 +280,8 @@ virCloseCallbacksGetOne(void *payload, memcpy(data->list->entries[data->list->nentries - 1].uuid, uuid, VIR_UUID_BUFLEN); + memcpy(data->list->entries[data->list->nentries - 1].uuidstr, + uuidstr, VIR_UUID_STRING_BUFLEN); data->list->entries[data->list->nentries - 1].callback = closeDef->cb; } @@ -332,7 +335,7 @@ virCloseCallbacksRun(virCloseCallbacksPtr closeCallbacks, for (i = 0; i < list->nentries; i++) { virHashRemoveEntry(closeCallbacks->list, - list->entries[i].uuid); + list->entries[i].uuidstr); } virObjectUnlock(closeCallbacks); @@ -341,9 +344,8 @@ virCloseCallbacksRun(virCloseCallbacksPtr closeCallbacks, if (!(vm = virDomainObjListFindByUUID(domains, list->entries[i].uuid))) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(list->entries[i].uuid, uuidstr); - VIR_DEBUG("No domain object with UUID %s", uuidstr); + VIR_DEBUG("No domain object with UUID %s", + list->entries[i].uuidstr); continue; } -- 2.1.0
On Mon, Mar 30, 2015 at 01:41:01PM +1100, Michael Chapman wrote:
The close callbacks hash are keyed by a UUID-string, but virCloseCallbacksRun was attempting to remove them by raw UUID. This patch ensures the callback entries are removed by UUID-string as well.
This bug could cause problems when guest migrations were abnormally aborted:
# timeout --signal KILL 1 \ virsh migrate example qemu+tls://remote/system \ --verbose --compressed --live --auto-converge \ --abort-on-error --unsafe --persistent \ --undefinesource --copy-storage-all --xml example.xml Killed
# virsh migrate example qemu+tls://remote/system \ --verbose --compressed --live --auto-converge \ --abort-on-error --unsafe --persistent \ --undefinesource --copy-storage-all --xml example.xml error: Requested operation is not valid: domain 'example' is not being migrated
Signed-off-by: Michael Chapman <mike@very.puzzling.org> --- src/util/virclosecallbacks.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/src/util/virclosecallbacks.c b/src/util/virclosecallbacks.c index 4128057..b0a1a9e 100644 --- a/src/util/virclosecallbacks.c +++ b/src/util/virclosecallbacks.c @@ -236,6 +236,7 @@ typedef struct _virCloseCallbacksListEntry virCloseCallbacksListEntry; typedef virCloseCallbacksListEntry *virCloseCallbacksListEntryPtr; struct _virCloseCallbacksListEntry { unsigned char uuid[VIR_UUID_BUFLEN]; + char uuidstr[VIR_UUID_STRING_BUFLEN];
We shouldn't be storing the uuid twice.
virCloseCallback callback; };
@@ -279,6 +280,8 @@ virCloseCallbacksGetOne(void *payload,
memcpy(data->list->entries[data->list->nentries - 1].uuid, uuid, VIR_UUID_BUFLEN); + memcpy(data->list->entries[data->list->nentries - 1].uuidstr, + uuidstr, VIR_UUID_STRING_BUFLEN); data->list->entries[data->list->nentries - 1].callback = closeDef->cb; }
@@ -332,7 +335,7 @@ virCloseCallbacksRun(virCloseCallbacksPtr closeCallbacks,
for (i = 0; i < list->nentries; i++) { virHashRemoveEntry(closeCallbacks->list, - list->entries[i].uuid); + list->entries[i].uuidstr);
I think the CPU time wasted by doing virUUIDFormat here again is negligible and easier than writing a Hash function that works on VIR_UUID buffers :) Jan
On Tue, 7 Apr 2015, Ján Tomko wrote:
On Mon, Mar 30, 2015 at 01:41:01PM +1100, Michael Chapman wrote:
The close callbacks hash are keyed by a UUID-string, but virCloseCallbacksRun was attempting to remove them by raw UUID. This patch ensures the callback entries are removed by UUID-string as well.
This bug could cause problems when guest migrations were abnormally aborted:
# timeout --signal KILL 1 \ virsh migrate example qemu+tls://remote/system \ --verbose --compressed --live --auto-converge \ --abort-on-error --unsafe --persistent \ --undefinesource --copy-storage-all --xml example.xml Killed
# virsh migrate example qemu+tls://remote/system \ --verbose --compressed --live --auto-converge \ --abort-on-error --unsafe --persistent \ --undefinesource --copy-storage-all --xml example.xml error: Requested operation is not valid: domain 'example' is not being migrated
Signed-off-by: Michael Chapman <mike@very.puzzling.org> --- src/util/virclosecallbacks.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/src/util/virclosecallbacks.c b/src/util/virclosecallbacks.c index 4128057..b0a1a9e 100644 --- a/src/util/virclosecallbacks.c +++ b/src/util/virclosecallbacks.c @@ -236,6 +236,7 @@ typedef struct _virCloseCallbacksListEntry virCloseCallbacksListEntry; typedef virCloseCallbacksListEntry *virCloseCallbacksListEntryPtr; struct _virCloseCallbacksListEntry { unsigned char uuid[VIR_UUID_BUFLEN]; + char uuidstr[VIR_UUID_STRING_BUFLEN];
We shouldn't be storing the uuid twice.
OK.
virCloseCallback callback; };
@@ -279,6 +280,8 @@ virCloseCallbacksGetOne(void *payload,
memcpy(data->list->entries[data->list->nentries - 1].uuid, uuid, VIR_UUID_BUFLEN); + memcpy(data->list->entries[data->list->nentries - 1].uuidstr, + uuidstr, VIR_UUID_STRING_BUFLEN); data->list->entries[data->list->nentries - 1].callback = closeDef->cb; }
@@ -332,7 +335,7 @@ virCloseCallbacksRun(virCloseCallbacksPtr closeCallbacks,
for (i = 0; i < list->nentries; i++) { virHashRemoveEntry(closeCallbacks->list, - list->entries[i].uuid); + list->entries[i].uuidstr);
I think the CPU time wasted by doing virUUIDFormat here again is negligible and easier than writing a Hash function that works on VIR_UUID buffers :)
Jan
Yeah, sounds reasonable. I'll send through a v2 patch shortly. I've also got a few extra patches lined up, but I'll start a new series for those. Thanks for the review! - Michael
The close callbacks hash are keyed by a UUID-string, but virCloseCallbacksRun was attempting to remove them by raw UUID. This patch ensures the callback entries are removed by UUID-string as well. This bug caused problems when guest migrations were abnormally aborted: # timeout --signal KILL 1 \ virsh migrate example qemu+tls://remote/system \ --verbose --compressed --live --auto-converge \ --abort-on-error --unsafe --persistent \ --undefinesource --copy-storage-all --xml example.xml Killed # virsh migrate example qemu+tls://remote/system \ --verbose --compressed --live --auto-converge \ --abort-on-error --unsafe --persistent \ --undefinesource --copy-storage-all --xml example.xml error: Requested operation is not valid: domain 'example' is not being migrated Signed-off-by: Michael Chapman <mike@very.puzzling.org> --- src/util/virclosecallbacks.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/util/virclosecallbacks.c b/src/util/virclosecallbacks.c index 4128057..9006d36 100644 --- a/src/util/virclosecallbacks.c +++ b/src/util/virclosecallbacks.c @@ -331,8 +331,9 @@ virCloseCallbacksRun(virCloseCallbacksPtr closeCallbacks, return; for (i = 0; i < list->nentries; i++) { - virHashRemoveEntry(closeCallbacks->list, - list->entries[i].uuid); + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(list->entries[i].uuid, uuidstr); + virHashRemoveEntry(closeCallbacks->list, uuidstr); } virObjectUnlock(closeCallbacks); -- 2.1.0
On Wed, Apr 08, 2015 at 01:22:39PM +1000, Michael Chapman wrote:
The close callbacks hash are keyed by a UUID-string, but virCloseCallbacksRun was attempting to remove them by raw UUID. This patch ensures the callback entries are removed by UUID-string as well.
This bug caused problems when guest migrations were abnormally aborted:
# timeout --signal KILL 1 \ virsh migrate example qemu+tls://remote/system \ --verbose --compressed --live --auto-converge \ --abort-on-error --unsafe --persistent \ --undefinesource --copy-storage-all --xml example.xml Killed
# virsh migrate example qemu+tls://remote/system \ --verbose --compressed --live --auto-converge \ --abort-on-error --unsafe --persistent \ --undefinesource --copy-storage-all --xml example.xml error: Requested operation is not valid: domain 'example' is not being migrated
Signed-off-by: Michael Chapman <mike@very.puzzling.org> --- src/util/virclosecallbacks.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
ACK I have pushed the series Jan.
On Mon, Mar 30, 2015 at 01:40:57PM +1100, Michael Chapman wrote:
This patch series contains fixes for several bugs I encountered while deliberately forcing a VM migration to abort by killing the libvirt client. My VM has storage on local disk, which needs to be mirrored during the migration, and this gives ample time for this abort to take place. The particular bug I encountered depended on precisely which phase the migration had made it to (e.g. whether disk mirroring had actually commenced).
Patch 1 fixes a crash on the destination libvirt daemon due to a use-after-free of the domain object. Patches 2 and 4 fix some bugs related to the close callback handling on the source libvirt side. Patch 3 ensures that the VM on the source libvirt does not get into an invalid state if its migration is aborted during disk mirroring.
All patches are independent of one another and can be applied separately.
Michael Chapman (4): qemu: fix crash in qemuProcessAutoDestroy qemu: fix error propagation in qemuMigrationBegin qemu: fix race between disk mirror fail and cancel util: fix removal of callbacks in virCloseCallbacksRun
src/qemu/qemu_domain.c | 5 +++++ src/qemu/qemu_migration.c | 12 +++++++++++- src/qemu/qemu_process.c | 4 +++- src/util/virclosecallbacks.c | 10 ++++++---- 4 files changed, 25 insertions(+), 6 deletions(-)
ACK to patches 1 - 3. I will wait a while before pushing to give others a chance to take a look. Jan
participants (2)
-
Ján Tomko -
Michael Chapman