atom feed11 messages in com.redhat.et-mgmt-tools[et-mgmt-tools] Re: [PATCH/RFC] kvm/q...
FromSent OnAttachments
Eric DudaSep 26, 2008 2:38 am 
Daniel P. BerrangeSep 26, 2008 2:42 am 
Eric DudaSep 26, 2008 3:47 am 
Cole RobinsonSep 26, 2008 6:51 am 
Guido GüntherSep 26, 2008 8:41 am 
Daniel P. BerrangeSep 26, 2008 8:47 am 
Eric DudaSep 26, 2008 11:00 am 
Guido GüntherOct 5, 2008 5:16 am 
Daniel P. BerrangeOct 5, 2008 9:05 am 
Guido GüntherOct 7, 2008 9:12 am.patch
Daniel P. BerrangeOct 7, 2008 9:37 am 
Subject:[et-mgmt-tools] Re: [PATCH/RFC] kvm/qemu vs libvirtd restarts
From:Daniel P. Berrange (berr@redhat.com)
Date:Oct 7, 2008 9:37:52 am
List:com.redhat.et-mgmt-tools

Moving thread to libvir-list....

On Tue, Oct 07, 2008 at 06:12:33PM +0200, Guido G?nther wrote:

On Fri, Sep 26, 2008 at 04:47:05PM +0100, Daniel P. Berrange wrote:

On Fri, Sep 26, 2008 at 05:41:02PM +0200, Guido G?nther wrote:

We've got this problem sorted in the 'lxc' driver and need to apply the same approach to the QEMU driver. The problem was always finding the Pseduo-TTY/PID for the monitor after restart, and associating the live config with it. We've "solved" that in LXC driver by savingt the live XML & PID to /var/run/libvirt/lxc/. It basically needs the same approach to be applied to the QEMU daemons. In addition the DNSmasq deamon needs adapting for simiarl reasons.

Attached is a very early patch. It keeps the qemu instances running and writes out their states upon shutdown. Reconnecting (and therefore sending monitor commands, etc.) works but since stdout/stderr don't get picked up again yet we don't handle virEventAddhandle. I'll grab them from /proc/<pid>/fd/{1,2} later. Network state is missing too, but since this is moving into a separate file it's becoming a different matter anyway.

First off, thanks for working on this! A few general comments, and then I'll put rest inline..

For stdout/err, it need to be setup so that the logfile is dup()'d onto the QEMU process' file descriptors directly, and get libvirt outof the business of I/O forwarding. This avoids any complications with re-attaching to stdout/err, and loosing any log data while libvirt isn't running.

This also needs to be implemented with the assumption that the daemon can & will crash any time. So putting the state-saving hooks into the qemudShutdown() method won't be sufficient. We need to write out the state we need when initially starting the VM, and then update it any time we hot-plug/unplug devices.

Ignoring network state is fine - we can address that separately..

Subject: [PATCH] let qemu/kvm survive libvirtd restarts

--- src/domain_conf.c | 1 + src/domain_conf.h | 1 + src/qemu_conf.h | 1 + src/qemu_driver.c | 264 +++++++++++++++++++++++++++++++++++++++++++++++++---- src/util.c | 4 +- src/util.h | 1 + 6 files changed, 254 insertions(+), 18 deletions(-)

diff --git a/src/domain_conf.c b/src/domain_conf.c index 6a35064..aa102f6 100644 --- a/src/domain_conf.c +++ b/src/domain_conf.c @@ -420,6 +420,7 @@ void virDomainObjFree(virDomainObjPtr dom) virDomainDefFree(dom->def); virDomainDefFree(dom->newDef);

+ VIR_FREE(dom->monitorpath); VIR_FREE(dom->vcpupids);

VIR_FREE(dom); diff --git a/src/domain_conf.h b/src/domain_conf.h index 632e5ad..1ac1561 100644 --- a/src/domain_conf.h +++ b/src/domain_conf.h @@ -448,6 +448,7 @@ struct _virDomainObj { int stdout_fd; int stderr_fd; int monitor; + char *monitorpath;

I think we can avoid needing this. If we write out the staste the moment the VM is started, we don't need to carry this around in memory. Alternatively, since we're writing all stdout/err data to the logfile, we could simply re-parse the log to extract the monitor path upon restart.

int logfile; int pid; int state; diff --git a/src/qemu_conf.h b/src/qemu_conf.h index 88dfade..f47582e 100644 --- a/src/qemu_conf.h +++ b/src/qemu_conf.h @@ -62,6 +62,7 @@ struct qemud_driver { char *networkConfigDir; char *networkAutostartDir; char *logDir; + char *stateDir; unsigned int vncTLS : 1; unsigned int vncTLSx509verify : 1; char *vncTLSx509certdir; diff --git a/src/qemu_driver.c b/src/qemu_driver.c index 806608d..87789a9 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -172,6 +172,181 @@ void qemudAutostartConfigs(struct qemud_driver *driver) { }

#ifdef WITH_LIBVIRTD + +static int +qemudFileWriteMonitor(const char *dir, + const char *name, + const char *path) +{ + int rc; + int fd; + FILE *file = NULL; + char *monitorfile = NULL; + + if ((rc = virFileMakePath(dir))) + goto cleanup; + + if (asprintf(&monitorfile, "%s/%s.monitor", dir, name) < 0) { + rc = ENOMEM; + goto cleanup; + } + + if ((fd = open(monitorfile, + O_WRONLY | O_CREAT | O_TRUNC, + S_IRUSR | S_IWUSR)) < 0) { + rc = errno; + goto cleanup; + } + + if (!(file = fdopen(fd, "w"))) { + rc = errno; + close(fd); + goto cleanup; + } + + if (fprintf(file, "%s", path) < 0) { + rc = errno; + goto cleanup; + } + + rc = 0; + +cleanup: + if (file && + fclose(file) < 0) { + rc = errno; + } + + VIR_FREE(monitorfile); + return rc; +} + +static int +qemudFileReadMonitor(const char *dir, + const char *name, + char **path) +{ + int rc; + FILE *file; + char *monitorfile = NULL; + + if (asprintf(&monitorfile, "%s/%s.monitor", dir, name) < 0) { + rc = ENOMEM; + goto cleanup; + } + + if (!(file = fopen(monitorfile, "r"))) { + rc = errno; + goto cleanup; + } + + if (fscanf(file, "%as", path) != 1) { + rc = EINVAL; + goto cleanup; + } + + if (fclose(file) < 0) { + rc = errno; + goto cleanup; + } + + rc = 0; + + cleanup: + VIR_FREE(monitorfile); + return rc; +}

If we re-parse the logfile to extract the monitiro PTY path, this is unneccessary. If not, this can be simplied by just calling the convenient virFileReadAll() method.

+ +static int +qemudFileDeleteMonitor(const char *dir, + const char *name) +{ + int rc = 0; + char *pidfile = NULL; + + if (asprintf(&pidfile, "%s/%s.monitor", dir, name) < 0) { + rc = errno; + goto cleanup; + } + + if (unlink(pidfile) < 0 && errno != ENOENT) + rc = errno; + +cleanup: + VIR_FREE(pidfile); + return rc; +} + + +static int qemudOpenMonitor(virConnectPtr conn, + struct qemud_driver *driver, + virDomainObjPtr vm, + const char *monitor, + int reconnect); +/** + * qemudReconnectVMs + * + * Reconnect running vms to the daemon process + */ +static int +qemudReconnectVMs(struct qemud_driver *driver) +{ + virDomainObjPtr vm = driver->domains; + + while (vm) { + virDomainDefPtr tmp; + char *config = NULL; + char *monitor = NULL; + int rc; + + /* Read pid */ + if ((rc = virFileReadPid(driver->stateDir, vm->def->name, &vm->pid)) ==
0) { + virFileDeletePid(driver->stateDir, vm->def->name); + DEBUG("Found pid %d for '%s'", vm->pid, vm->def->name); + } else + goto next; + + if ((config = virDomainConfigFile(NULL, + driver->stateDir, + vm->def->name)) == NULL) { + qemudLog(QEMUD_ERR, _("Failed to read back domain config for
%s\n"), + vm->def->name); + goto next; + } + + /* Try and load the config */ + tmp = virDomainDefParseFile(NULL, driver->caps, config); + unlink(config); + if (tmp) { + vm->newDef = vm->def; + vm->def = tmp; + } + + /* read back monitor path */ + if ((rc = qemudFileReadMonitor(driver->stateDir, vm->def->name,
&monitor)) != 0) { + qemudLog(QEMUD_ERR, _("Failed to read back monitor path for %s:
%s\n"), + vm->def->name, strerror(rc)); + goto next; + } + qemudFileDeleteMonitor(driver->stateDir, vm->def->name); + + /* FIXME: reconnect stdout/stderr via /proc/pid/fd/ */ + + if ((rc = qemudOpenMonitor(NULL, driver, vm, monitor, 1)) != 0) { + qemudLog(QEMUD_ERR, _("Failed to reconnect to monitor for %s:
%d\n"), + vm->def->name, rc); + goto next; + } + vm->def->id = driver->nextvmid++;

The ID of a vm must never change for until it is rebooted. Simply don't overwrite the existing vm->def->id that we jjust loaded off disk. And instead adjust the nextvmid field, eg

if (vm->def->id >= driver->nextvmid) driver->nextvmid = vm->def->id + 1;

+ vm->state = VIR_DOMAIN_RUNNING; +next: + VIR_FREE(config); + VIR_FREE(monitor); + vm = vm->next; + } + return 0; +} + /** * qemudStartup: * @@ -197,6 +372,10 @@ qemudStartup(void) {

if ((base = strdup (SYSCONF_DIR "/libvirt")) == NULL) goto out_of_memory; + + if (asprintf (&qemu_driver->stateDir, + "%s/run/libvirt/qemu/", LOCAL_STATE_DIR) == -1) + goto out_of_memory; } else { if (!(pw = getpwuid(uid))) { qemudLog(QEMUD_ERR, _("Failed to find user record for uid '%d':
%s\n"), @@ -210,6 +389,10 @@ qemudStartup(void) {

if (asprintf (&base, "%s/.libvirt", pw->pw_dir) == -1) goto out_of_memory; + + if (asprintf (&qemu_driver->stateDir, + "%s/run/libvirt/qemu/", base) == -1) + goto out_of_memory; }

/* Configuration paths are either ~/.libvirt/qemu/... (session) or @@ -250,6 +433,8 @@ qemudStartup(void) { qemudShutdown(); return -1; } + qemudReconnectVMs(qemu_driver); + if (virNetworkLoadAllConfigs(NULL, &qemu_driver->networks, qemu_driver->networkConfigDir, @@ -329,6 +514,34 @@ qemudActive(void) { return 0; }

+/** + * qemudSaveDomainState + * + * Save the full state of a running domain to statedir + * + * Returns 0 on success + */ +static int +qemudSaveDomainState(struct qemud_driver * driver, virDomainObjPtr vm) { + int ret; + + if ((ret = virFileWritePid(driver->stateDir, vm->def->name, vm->pid)) != 0)
{ + qemudLog(QEMUD_ERR, _("Unable to safe pidfile for %s: %s\n"), + vm->def->name, strerror(ret)); + return ret; + } + if ((ret = virDomainSaveConfig(NULL, driver->stateDir, vm->def))) { + qemudLog(QEMUD_ERR, _("Unable to save domain %s\n"), vm->def->name); + return ret; + } + if ((ret = qemudFileWriteMonitor(driver->stateDir, vm->def->name,
vm->monitorpath)) != 0) { + qemudLog(QEMUD_ERR, _("Unable to monitor file for %s: %s\n"), + vm->def->name, strerror(ret)); + return ret; + } + return 0; +} +

This will need to be called at time of VM creation, and the XSL config will need updating whether live config changes.

/** * qemudShutdown: * @@ -349,10 +562,14 @@ qemudShutdown(void) { while (vm) { virDomainObjPtr next = vm->next; if (virDomainIsActive(vm)) +#if 1 + qemudSaveDomainState(qemu_driver, vm); +#else qemudShutdownVMDaemon(NULL, qemu_driver, vm); if (!vm->persistent) virDomainRemoveInactive(&qemu_driver->domains, vm); +#endif vm = next; }

@@ -389,6 +606,7 @@ qemudShutdown(void) { VIR_FREE(qemu_driver->networkConfigDir); VIR_FREE(qemu_driver->networkAutostartDir); VIR_FREE(qemu_driver->vncTLSx509certdir); + VIR_FREE(qemu_driver->stateDir);

if (qemu_driver->brctl) brShutdown(qemu_driver->brctl); @@ -499,7 +717,7 @@ qemudCheckMonitorPrompt(virConnectPtr conn ATTRIBUTE_UNUSED, static int qemudOpenMonitor(virConnectPtr conn, struct qemud_driver *driver, virDomainObjPtr vm, - const char *monitor) { + const char *monitor, int reconnect) { int monfd; char buf[1024]; int ret = -1; @@ -520,11 +738,26 @@ static int qemudOpenMonitor(virConnectPtr conn, goto error; }

- ret = qemudReadMonitorOutput(conn, - driver, vm, monfd, - buf, sizeof(buf), - qemudCheckMonitorPrompt, - "monitor"); + if (!reconnect) { + ret = qemudReadMonitorOutput(conn, + driver, vm, monfd, + buf, sizeof(buf), + qemudCheckMonitorPrompt, + "monitor"); + + } else { + vm->monitor = monfd; + ret = 0; + } + + if (ret != 0) + goto error; + + if (!(vm->monitorpath = strdup(monitor))) { + qemudReportError(conn, NULL, NULL, VIR_ERR_NO_MEMORY, + "%s", _("failed to allocate space for monitor path")); + goto error; + }

/* Keep monitor open upon success */ if (ret == 0) @@ -623,7 +856,7 @@ qemudFindCharDevicePTYs(virConnectPtr conn, }

/* Got them all, so now open the monitor console */ - ret = qemudOpenMonitor(conn, driver, vm, monitor); + ret = qemudOpenMonitor(conn, driver, vm, monitor, 0);

cleanup: VIR_FREE(monitor); @@ -966,12 +1199,11 @@ static int qemudStartVMDaemon(virConnectPtr conn,

ret = virExec(conn, argv, NULL, &keepfd, &vm->pid, vm->stdin_fd, &vm->stdout_fd, &vm->stderr_fd, - VIR_EXEC_NONBLOCK); + VIR_EXEC_NONBLOCK | VIR_EXEC_SETSID); if (ret == 0) { vm->def->id = driver->nextvmid++; vm->state = migrateFrom ? VIR_DOMAIN_PAUSED : VIR_DOMAIN_RUNNING; } - for (i = 0 ; argv[i] ; i++) VIR_FREE(argv[i]); VIR_FREE(argv); @@ -1037,7 +1269,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); @@ -1057,13 +1291,9 @@ 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 if off for sure */ + kill(vm->pid, SIGKILL); + virFileDeletePid(driver->stateDir, vm->def->name);

vm->pid = -1; vm->def->id = -1; diff --git a/src/util.c b/src/util.c index ca14be1..442c810 100644 --- a/src/util.c +++ b/src/util.c @@ -299,14 +299,16 @@ virExec(virConnectPtr conn, !FD_ISSET(i, keepfd))) close(i);

- if (flags & VIR_EXEC_DAEMON) { + if (flags & VIR_EXEC_DAEMON || flags & VIR_EXEC_SETSID) { if (setsid() < 0) { ReportError(conn, VIR_ERR_INTERNAL_ERROR, _("cannot become session leader: %s"), strerror(errno)); _exit(1); } + }

+ if (flags & VIR_EXEC_DAEMON) { if (chdir("/") < 0) { ReportError(conn, VIR_ERR_INTERNAL_ERROR, _("cannot change to root directory: %s"), diff --git a/src/util.h b/src/util.h index 093ef46..8fbe2cd 100644 --- a/src/util.h +++ b/src/util.h @@ -32,6 +32,7 @@ enum { VIR_EXEC_NONE = 0, VIR_EXEC_NONBLOCK = (1 << 0), VIR_EXEC_DAEMON = (1 << 1), + VIR_EXEC_SETSID = (1 << 2), };

Shouldn't we simply be starting all the QEMU VMs with VIR_EXEC_DAEMON so they totally disassociate themselves from libvirtd right away. They will be disassociated anyway if the libvirtd is ever restarted, so best to daemonize from time they are started, to avoid any surprising changes in behaviour

Regards, Daniel