6:57 a.m.
Introduce a virProcessKill function that can be safely called
even when the job mutex is held. This allows virDomainDestroy
to kill any VM even if it is asleep in a monitor job. The PID
will die and the thread asleep on the monitor will then wake
up releasing the job mutex.
* src/qemu/qemu_driver.c: Kill process before using qemuProcessStop
to ensure job is released
* src/qemu/qemu_process.c: Add virProcessKill for killing off
QEMU processes
---
src/qemu/qemu_driver.c | 7 +++++++
src/qemu/qemu_process.c | 39 +++++++++++++++++++++++++++++++--------
src/qemu/qemu_process.h | 2 ++
3 files changed, 40 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index b3f9e00..6d6fb51 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1482,6 +1482,13 @@ static int qemudDomainDestroy(virDomainPtr dom) {
goto cleanup;
}
+ /* Although qemuProcessStop does this already, there may
+ * be an outstanding job active. We want to make sure we
+ * can kill the process even if a job is active. Killing
+ * it now, means the job will be released
+ */
+ qemuProcessKill(vm);
+
if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0)
goto cleanup;
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index a6c0dc8..c60c51f 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -2369,6 +2369,36 @@ cleanup:
}
+void qemuProcessKill(virDomainObjPtr vm)
+{
+ int i;
+ int rc;
+ VIR_DEBUG("vm=%s pid=%d", vm->def->name, vm->pid);
+
+ if (!virDomainObjIsActive(vm)) {
+ VIR_DEBUG("VM '%s' not active", vm->def->name);
+ return;
+ }
+
+ for (i = 0 ; i < 15 ; i++) {
+ int signum;
+ if (i == 0)
+ signum = SIGTERM;
+ else if (i == 8)
+ signum = SIGKILL;
+ else
+ signum = 0; /* Just check for existance */
+
+ rc = virKillProcess(vm->pid, signum);
+ VIR_DEBUG("Iteration %d rc=%d", i, rc);
+ if (rc < 0)
+ break;
+
+ usleep(200 * 1000);
+ }
+}
+
+
void qemuProcessStop(struct qemud_driver *driver,
virDomainObjPtr vm,
int migrated)
@@ -2436,13 +2466,6 @@ void qemuProcessStop(struct qemud_driver *driver,
}
}
- /* This will safely handle a non-running guest with pid=0 or pid=-1*/
- if (virKillProcess(vm->pid, 0) == 0 &&
- virKillProcess(vm->pid, SIGTERM) < 0)
- virReportSystemError(errno,
- _("Failed to send SIGTERM to %s (%d)"),
- vm->def->name, vm->pid);
-
if (priv->mon)
qemuMonitorClose(priv->mon);
@@ -2454,7 +2477,7 @@ void qemuProcessStop(struct qemud_driver *driver,
}
/* shut it off for sure */
- virKillProcess(vm->pid, SIGKILL);
+ qemuProcessKill(vm);
/* now that we know it's stopped call the hook if present */
if (virHookPresent(VIR_HOOK_DRIVER_QEMU)) {
diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h
index f1ab599..d8afab0 100644
--- a/src/qemu/qemu_process.h
+++ b/src/qemu/qemu_process.h
@@ -49,4 +49,6 @@ void qemuProcessStop(struct qemud_driver *driver,
virDomainObjPtr vm,
int migrated);
+void qemuProcessKill(virDomainObjPtr vm);
+
#endif /* __QEMU_PROCESS_H__ */
--
1.7.4.4
9:34 a.m.
On 05/06/2011 05:57 AM, Daniel P. Berrange wrote:
Introduce a virProcessKill function that can be safely called
even when the job mutex is held. This allows virDomainDestroy
to kill any VM even if it is asleep in a monitor job. The PID
will die and the thread asleep on the monitor will then wake
up releasing the job mutex.
Very nice. This will help issues where if you kill a migration at the
source, then the destination is left with a stale VM with the job lock
held waiting for the incoming migration to complete. With this, you now
have the means to forcibly kill the leftover VM on the destination,
rather than waiting for a network timeout.
+++ b/src/qemu/qemu_driver.c
@@ -1482,6 +1482,13 @@ static int qemudDomainDestroy(virDomainPtr dom) {
goto cleanup;
}
+ /* Although qemuProcessStop does this already, there may
+ * be an outstanding job active. We want to make sure we
+ * can kill the process even if a job is active. Killing
+ * it now, means the job will be released
s/now,/now/
+ */
+ qemuProcessKill(vm);
+
if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0)
goto cleanup;
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index a6c0dc8..c60c51f 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -2369,6 +2369,36 @@ cleanup:
}
+void qemuProcessKill(virDomainObjPtr vm)
Do you want this to return -1 on failure to kill, such as if waitpid()
fails? On the other hand, I guess that best effort is okay; you already
did VIR_DEBUG logging of failures, and the caller can't do much even if
they were to check for a failure.
+{
+ int i;
+ int rc;
+ VIR_DEBUG("vm=%s pid=%d", vm->def->name, vm->pid);
+
+ if (!virDomainObjIsActive(vm)) {
+ VIR_DEBUG("VM '%s' not active", vm->def->name);
+ return;
+ }
+
+ for (i = 0 ; i < 15 ; i++) {
+ int signum;
+ if (i == 0)
+ signum = SIGTERM;
+ else if (i == 8)
+ signum = SIGKILL;
+ else
+ signum = 0; /* Just check for existance */
s/existance/existence/
It took me three reads to figure out what this loop is doing; a comment
would be helpful:
/* Try SIGTERM, then wait 1.6 seconds, then try SIGKILL, all while
probing every 200ms to see if the process is gone */
+
+ rc = virKillProcess(vm->pid, signum);
+ VIR_DEBUG("Iteration %d rc=%d", i, rc);
+ if (rc < 0)
+ break;
When errno==ESRCH, this breaks the loop because the child is dead. But
for any other errno, should you log (at least at VIR_DEBUG) the errno
that causes failure, since that means we didn't exit the loop normally?
+
+ usleep(200 * 1000);
+ }
+}
+
ACK with those nits addressed.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
4:13 a.m.
On Fri, May 06, 2011 at 08:34:43AM -0600, Eric Blake wrote:
On 05/06/2011 05:57 AM, Daniel P. Berrange wrote:
> Introduce a virProcessKill function that can be safely called
> even when the job mutex is held. This allows virDomainDestroy
> to kill any VM even if it is asleep in a monitor job. The PID
> will die and the thread asleep on the monitor will then wake
> up releasing the job mutex.
Very nice. This will help issues where if you kill a migration at the
source, then the destination is left with a stale VM with the job lock
held waiting for the incoming migration to complete. With this, you now
have the means to forcibly kill the leftover VM on the destination,
rather than waiting for a network timeout.
Actually this is the one use case it won't help with. THe migration
thing is a bit of a nasty hack / abuse. 'Prepare' obtains the job
mutex, and 'Finish' releases it. Neither of them are actually
monitoring the QEMU process, so it won't get released. I'll need
some other special case code for that.
This patch will help for any case where a API command is stuck
waiting for a monitor command reply.
> +++ b/src/qemu/qemu_driver.c
> @@ -1482,6 +1482,13 @@ static int qemudDomainDestroy(virDomainPtr dom) {
> goto cleanup;
> }
>
> + /* Although qemuProcessStop does this already, there may
> + * be an outstanding job active. We want to make sure we
> + * can kill the process even if a job is active. Killing
> + * it now, means the job will be released
s/now,/now/
> + */
> + qemuProcessKill(vm);
> +
> if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0)
> goto cleanup;
>
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index a6c0dc8..c60c51f 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -2369,6 +2369,36 @@ cleanup:
> }
>
>
> +void qemuProcessKill(virDomainObjPtr vm)
Do you want this to return -1 on failure to kill, such as if waitpid()
fails? On the other hand, I guess that best effort is okay; you already
did VIR_DEBUG logging of failures, and the caller can't do much even if
they were to check for a failure.
I didn't bother to return any value, because all the callers are
going to ignore it anyway.
> +{
> + int i;
> + int rc;
> + VIR_DEBUG("vm=%s pid=%d", vm->def->name, vm->pid);
> +
> + if (!virDomainObjIsActive(vm)) {
> + VIR_DEBUG("VM '%s' not active", vm->def->name);
> + return;
> + }
> +
> + for (i = 0 ; i < 15 ; i++) {
> + int signum;
> + if (i == 0)
> + signum = SIGTERM;
> + else if (i == 8)
> + signum = SIGKILL;
> + else
> + signum = 0; /* Just check for existance */
s/existance/existence/
It took me three reads to figure out what this loop is doing; a comment
would be helpful:
/* Try SIGTERM, then wait 1.6 seconds, then try SIGKILL, all while
probing every 200ms to see if the process is gone */
> +
> + rc = virKillProcess(vm->pid, signum);
> + VIR_DEBUG("Iteration %d rc=%d", i, rc);
> + if (rc < 0)
> + break;
When errno==ESRCH, this breaks the loop because the child is dead. But
for any other errno, should you log (at least at VIR_DEBUG) the errno
that causes failure, since that means we didn't exit the loop normally?
Yeah i guess so.
> +
> + usleep(200 * 1000);
> + }
> +}
> +
ACK with those nits addressed.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
4946
days inactive
4949
days old
2 comments
2 participants
participants (2)
-
Daniel P. Berrange -
Eric Blake