12:26 p.m.
Make sure vms don't get killed when the libvirtd quits unexpectedly.
Needs the previous patch since it looks at the pid file.
---
src/qemu_driver.c | 36 ++++++++++++++++++++++++------------
1 files changed, 24 insertions(+), 12 deletions(-)
diff --git a/src/qemu_driver.c b/src/qemu_driver.c
index a2e573e..7804094 100644
--- a/src/qemu_driver.c
+++ b/src/qemu_driver.c
@@ -858,6 +858,7 @@ static int qemudStartVMDaemon(virConnectPtr conn,
const char *emulator;
uid_t uid = geteuid();
mode_t logmode;
+ pid_t child;
FD_ZERO(&keepfd);
@@ -988,12 +989,26 @@ static int qemudStartVMDaemon(virConnectPtr conn,
for (i = 0 ; i < ntapfds ; i++)
FD_SET(tapfds[i], &keepfd);
- ret = virExec(conn, argv, progenv, &keepfd, &vm->pid,
+ ret = virExec(conn, argv, progenv, &keepfd, &child,
vm->stdin_fd, &vm->stdout_fd, &vm->stderr_fd,
- VIR_EXEC_NONBLOCK);
- if (ret == 0)
+ VIR_EXEC_NONBLOCK | VIR_EXEC_DAEMON);
+
+ /* wait for qemu process to to show up */
+ if (ret == 0) {
+ int retries = 100;
+ while (retries) {
+ if ((ret = virFileReadPid(driver->stateDir, vm->def->name,
&vm->pid)) == 0)
+ break;
+ usleep(10*1000);
+ retries--;
+ }
+ if (ret)
+ qemudLog(QEMUD_WARN, _("Domain %s didn't show up\n"),
vm->def->name);
+ }
+
+ if (ret == 0) {
vm->state = migrateFrom ? VIR_DOMAIN_PAUSED : VIR_DOMAIN_RUNNING;
- else
+ } else
vm->def->id = -1;
for (i = 0 ; argv[i] ; i++)
@@ -1069,7 +1084,9 @@ static void qemudShutdownVMDaemon(virConnectPtr conn
ATTRIBUTE_UNUSED,
qemudLog(QEMUD_INFO, _("Shutting down VM '%s'\n"),
vm->def->name);
- kill(vm->pid, SIGTERM);
+ if (kill(vm->pid, SIGTERM) < 0)
+ qemudLog(QEMUD_ERROR, _("Failed to send SIGTERM to %s (%d): %s\n"),
+ vm->def->name, vm->pid, strerror(errno));
qemudVMData(driver, vm, vm->stdout_fd);
qemudVMData(driver, vm, vm->stderr_fd);
@@ -1089,13 +1106,8 @@ static void qemudShutdownVMDaemon(virConnectPtr conn
ATTRIBUTE_UNUSED,
vm->stderr_fd = -1;
vm->monitor = -1;
- if (waitpid(vm->pid, NULL, WNOHANG) != vm->pid) {
- kill(vm->pid, SIGKILL);
- if (waitpid(vm->pid, NULL, 0) != vm->pid) {
- qemudLog(QEMUD_WARN,
- "%s", _("Got unexpected pid, damn\n"));
- }
- }
+ /* shut it off for sure */
+ kill(vm->pid, SIGKILL);
vm->pid = -1;
vm->def->id = -1;
--
1.6.0.2
3:19 a.m.
On Fri, Dec 12, 2008 at 07:26:32PM +0100, Guido Günther wrote:
Make sure vms don't get killed when the libvirtd quits
unexpectedly.
Needs the previous patch since it looks at the pid file.
[...]
+ /* wait for qemu process to to show up */
+ if (ret == 0) {
+ int retries = 100;
+ while (retries) {
+ if ((ret = virFileReadPid(driver->stateDir, vm->def->name,
&vm->pid)) == 0)
+ break;
+ usleep(10*1000);
+ retries--;
+ }
+ if (ret)
+ qemudLog(QEMUD_WARN, _("Domain %s didn't show up\n"),
vm->def->name);
+ }
so we are waiting at most one second and waking up the process 100
times, I would suggest to relax that a bit, give it a bit more time like
10 seconds in case the system is trashing a bit 1 second may really be
too short. And maybe wake up a bit less ...
but patch looks good to me, it's just unfortunate we need to do some
tuning there.
thanks !
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
8:48 a.m.
On Mon, Dec 15, 2008 at 10:19:28AM +0100, Daniel Veillard wrote:
On Fri, Dec 12, 2008 at 07:26:32PM +0100, Guido Günther wrote:
> Make sure vms don't get killed when the libvirtd quits unexpectedly.
> Needs the previous patch since it looks at the pid file.
[...]
> + /* wait for qemu process to to show up */
> + if (ret == 0) {
> + int retries = 100;
> + while (retries) {
> + if ((ret = virFileReadPid(driver->stateDir, vm->def->name,
&vm->pid)) == 0)
> + break;
> + usleep(10*1000);
> + retries--;
> + }
> + if (ret)
> + qemudLog(QEMUD_WARN, _("Domain %s didn't show up\n"),
vm->def->name);
> + }
so we are waiting at most one second and waking up the process 100
times, I would suggest to relax that a bit, give it a bit more time like
10 seconds in case the system is trashing a bit 1 second may really be
too short. And maybe wake up a bit less ...
but patch looks good to me, it's just unfortunate we need to do some
tuning there.
I've put an updated version of this and the follow up patch to
reconnect
to running vms at:
http://honk.sigxcpu.org/projects/libvirt/daemon-restart/0001-daemonize-qe...
http://honk.sigxcpu.org/projects/libvirt/daemon-restart/0002-read-saved-v...
Just in case somebody else wants to try them.
Cheers,
-- Guido
5:21 a.m.
On Fri, Dec 12, 2008 at 07:26:32PM +0100, Guido G?nther wrote:
Make sure vms don't get killed when the libvirtd quits
unexpectedly.
Needs the previous patch since it looks at the pid file.
---
src/qemu_driver.c | 36 ++++++++++++++++++++++++------------
1 files changed, 24 insertions(+), 12 deletions(-)
diff --git a/src/qemu_driver.c b/src/qemu_driver.c
index a2e573e..7804094 100644
--- a/src/qemu_driver.c
+++ b/src/qemu_driver.c
@@ -858,6 +858,7 @@ static int qemudStartVMDaemon(virConnectPtr conn,
const char *emulator;
uid_t uid = geteuid();
mode_t logmode;
+ pid_t child;
FD_ZERO(&keepfd);
@@ -988,12 +989,26 @@ static int qemudStartVMDaemon(virConnectPtr conn,
for (i = 0 ; i < ntapfds ; i++)
FD_SET(tapfds[i], &keepfd);
- ret = virExec(conn, argv, progenv, &keepfd, &vm->pid,
+ ret = virExec(conn, argv, progenv, &keepfd, &child,
vm->stdin_fd, &vm->stdout_fd, &vm->stderr_fd,
- VIR_EXEC_NONBLOCK);
- if (ret == 0)
+ VIR_EXEC_NONBLOCK | VIR_EXEC_DAEMON);
+
+ /* wait for qemu process to to show up */
+ if (ret == 0) {
+ int retries = 100;
+ while (retries) {
+ if ((ret = virFileReadPid(driver->stateDir, vm->def->name,
&vm->pid)) == 0)
+ break;
If we want to support versions of oQEMU without -pidfile, we'll have to conditionally
run them without the _DAEMON flag.
+ usleep(10*1000);
+ retries--;
+ }
+ if (ret)
+ qemudLog(QEMUD_WARN, _("Domain %s didn't show up\n"),
vm->def->name);
+ }
+
+ if (ret == 0) {
vm->state = migrateFrom ? VIR_DOMAIN_PAUSED : VIR_DOMAIN_RUNNING;
- else
+ } else
vm->def->id = -1;
for (i = 0 ; argv[i] ; i++)
@@ -1069,7 +1084,9 @@ static void qemudShutdownVMDaemon(virConnectPtr conn
ATTRIBUTE_UNUSED,
qemudLog(QEMUD_INFO, _("Shutting down VM '%s'\n"),
vm->def->name);
- kill(vm->pid, SIGTERM);
+ if (kill(vm->pid, SIGTERM) < 0)
+ qemudLog(QEMUD_ERROR, _("Failed to send SIGTERM to %s (%d): %s\n"),
+ vm->def->name, vm->pid, strerror(errno));
We should add a guard around all the kill() statements for
if (vm->pid > 1)
kill..
One of the bugs I found when doing the LXC/UML drivers is that when relying
on an external PID file, some things can go wrong in unexpected ways that
just don't happen when getting the PID straight back from fork. This often
ended up with vm->pid being -1, 0 with horrific results -
"If pid equals -1, then sig is sent to every process for which the calling
process has permission to send signals, except for process 1 (init)"
Yes, I killed every single process on my dev machine several times before
discovering this isues :-)
qemudVMData(driver, vm, vm->stdout_fd);
qemudVMData(driver, vm, vm->stderr_fd);
@@ -1089,13 +1106,8 @@ static void qemudShutdownVMDaemon(virConnectPtr conn
ATTRIBUTE_UNUSED,
vm->stderr_fd = -1;
vm->monitor = -1;
- if (waitpid(vm->pid, NULL, WNOHANG) != vm->pid) {
- kill(vm->pid, SIGKILL);
- if (waitpid(vm->pid, NULL, 0) != vm->pid) {
- qemudLog(QEMUD_WARN,
- "%s", _("Got unexpected pid, damn\n"));
- }
- }
+ /* shut it off for sure */
+ kill(vm->pid, SIGKILL);
Same here - we should add a guard for pid > 1
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
7:22 a.m.
On Mon, Dec 15, 2008 at 11:21:20AM +0000, Daniel P. Berrange wrote:
On Fri, Dec 12, 2008 at 07:26:32PM +0100, Guido G?nther wrote:
> + if (kill(vm->pid, SIGTERM) < 0)
> + qemudLog(QEMUD_ERROR, _("Failed to send SIGTERM to %s (%d):
%s\n"),
> + vm->def->name, vm->pid, strerror(errno));
We should add a guard around all the kill() statements for
if (vm->pid > 1)
kill..
One of the bugs I found when doing the LXC/UML drivers is that when relying
on an external PID file, some things can go wrong in unexpected ways that
just don't happen when getting the PID straight back from fork. This often
ended up with vm->pid being -1, 0 with horrific results -
"If pid equals -1, then sig is sent to every process for which the calling
process has permission to send signals, except for process 1 (init)"
Yes, I killed every single process on my dev machine several times before
discovering this isues :-)
This call for an utility function
int virKillProcess(pid_t pid, int sig)
call checking for this, could also help on debugging/logging
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
3:42 a.m.
On Mon, Dec 15, 2008 at 02:22:42PM +0100, Daniel Veillard wrote:
On Mon, Dec 15, 2008 at 11:21:20AM +0000, Daniel P. Berrange wrote:
> On Fri, Dec 12, 2008 at 07:26:32PM +0100, Guido G?nther wrote:
> > + if (kill(vm->pid, SIGTERM) < 0)
> > + qemudLog(QEMUD_ERROR, _("Failed to send SIGTERM to %s (%d):
%s\n"),
> > + vm->def->name, vm->pid, strerror(errno));
>
> We should add a guard around all the kill() statements for
>
> if (vm->pid > 1)
> kill..
>
> One of the bugs I found when doing the LXC/UML drivers is that when relying
> on an external PID file, some things can go wrong in unexpected ways that
> just don't happen when getting the PID straight back from fork. This often
> ended up with vm->pid being -1, 0 with horrific results -
>
> "If pid equals -1, then sig is sent to every process for which the calling
> process has permission to send signals, except for process 1 (init)"
>
> Yes, I killed every single process on my dev machine several times before
> discovering this isues :-)
This call for an utility function
int virKillProcess(pid_t pid, int sig)
call checking for this, could also help on debugging/logging
I'll add that.
I'd like to apply the other 3 out of 5 patches that
monitor the state of the qemu instances nevertheless since they don't
change any behaviour but only add the necessary status tracking to fork
of the qemu/kvm processes later. This way we can easily check if the
status file is in sync with the running qemu by comapring it to "virsh
dumpxml".
Cheers,
-- Guido
8:36 a.m.
New subject: [PATCH] virKillProcess [was Re: [libvirt] [PATCH 2/5] daemonize qemu processes]
On Mon, Dec 15, 2008 at 02:22:42PM +0100, Daniel Veillard wrote:
On Mon, Dec 15, 2008 at 11:21:20AM +0000, Daniel P. Berrange wrote:
> On Fri, Dec 12, 2008 at 07:26:32PM +0100, Guido G?nther wrote:
> > + if (kill(vm->pid, SIGTERM) < 0)
> > + qemudLog(QEMUD_ERROR, _("Failed to send SIGTERM to %s (%d):
%s\n"),
> > + vm->def->name, vm->pid, strerror(errno));
>
> We should add a guard around all the kill() statements for
>
> if (vm->pid > 1)
> kill..
>
> One of the bugs I found when doing the LXC/UML drivers is that when relying
> on an external PID file, some things can go wrong in unexpected ways that
> just don't happen when getting the PID straight back from fork. This often
> ended up with vm->pid being -1, 0 with horrific results -
>
> "If pid equals -1, then sig is sent to every process for which the calling
> process has permission to send signals, except for process 1 (init)"
>
> Yes, I killed every single process on my dev machine several times before
> discovering this isues :-)
This call for an utility function
int virKillProcess(pid_t pid, int sig)
call checking for this, could also help on debugging/logging
O.k. to apply the
attached version of virKillProcess? I'm already using
this in the libvirtd restart patches.
-- Guido
3:58 a.m.
New subject: [PATCH] virKillProcess [was Re: [libvirt] [PATCH 2/5] daemonize qemu processes]
On Mon, Dec 29, 2008 at 03:36:15PM +0100, Guido G?nther wrote:
On Mon, Dec 15, 2008 at 02:22:42PM +0100, Daniel Veillard wrote:
> On Mon, Dec 15, 2008 at 11:21:20AM +0000, Daniel P. Berrange wrote:
> > On Fri, Dec 12, 2008 at 07:26:32PM +0100, Guido G?nther wrote:
> > > + if (kill(vm->pid, SIGTERM) < 0)
> > > + qemudLog(QEMUD_ERROR, _("Failed to send SIGTERM to %s (%d):
%s\n"),
> > > + vm->def->name, vm->pid, strerror(errno));
> >
> > We should add a guard around all the kill() statements for
> >
> > if (vm->pid > 1)
> > kill..
> >
> > One of the bugs I found when doing the LXC/UML drivers is that when relying
> > on an external PID file, some things can go wrong in unexpected ways that
> > just don't happen when getting the PID straight back from fork. This often
> > ended up with vm->pid being -1, 0 with horrific results -
> >
> > "If pid equals -1, then sig is sent to every process for which the
calling
> > process has permission to send signals, except for process 1 (init)"
> >
> > Yes, I killed every single process on my dev machine several times before
> > discovering this isues :-)
>
> This call for an utility function
> int virKillProcess(pid_t pid, int sig)
> call checking for this, could also help on debugging/logging
O.k. to apply the attached version of virKillProcess? I'm already using
this in the libvirtd restart patches.
ACK
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
11:47 a.m.
New subject: [PATCH] virKillProcess [was Re: [libvirt] [PATCH 2/5] daemonize qemu processes]
On Mon, Jan 05, 2009 at 09:58:06AM +0000, Daniel P. Berrange wrote:
On Mon, Dec 29, 2008 at 03:36:15PM +0100, Guido G?nther wrote:
> On Mon, Dec 15, 2008 at 02:22:42PM +0100, Daniel Veillard wrote:
> > On Mon, Dec 15, 2008 at 11:21:20AM +0000, Daniel P. Berrange wrote:
> > > On Fri, Dec 12, 2008 at 07:26:32PM +0100, Guido G?nther wrote:
> > > > + if (kill(vm->pid, SIGTERM) < 0)
> > > > + qemudLog(QEMUD_ERROR, _("Failed to send SIGTERM to %s
(%d): %s\n"),
> > > > + vm->def->name, vm->pid, strerror(errno));
> > >
> > > We should add a guard around all the kill() statements for
> > >
> > > if (vm->pid > 1)
> > > kill..
> > >
> > > One of the bugs I found when doing the LXC/UML drivers is that when
relying
> > > on an external PID file, some things can go wrong in unexpected ways that
> > > just don't happen when getting the PID straight back from fork. This
often
> > > ended up with vm->pid being -1, 0 with horrific results -
> > >
> > > "If pid equals -1, then sig is sent to every process for which
the calling
> > > process has permission to send signals, except for process 1
(init)"
> > >
> > > Yes, I killed every single process on my dev machine several times before
> > > discovering this isues :-)
> >
> > This call for an utility function
> > int virKillProcess(pid_t pid, int sig)
> > call checking for this, could also help on debugging/logging
> O.k. to apply the attached version of virKillProcess? I'm already using
> this in the libvirtd restart patches.
ACK
Applied.
-- Guido
5799
days inactive
5824
days old
8 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
Guido Günther