[libvirt] Re: [PATCH/RFC] kvm/qemu vs libvirtd restarts

[Guido Günther]

Finally got around to have another look at qemu/kvm surviving libvirt
restarts:

On Tue, Oct 07, 2008 at 05:37:52PM +0100, Daniel P. Berrange wrote:
> > 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.
I'm not sure reparsing the log is that good. Maybe the log got rotated
in the meantime or the admin moved it away. So I'd rather keep this in
/var/run/ too. We don't need that extra monitorpath variable we though,
we can simply pick it from /proc/<libvirtpid>/fd/<monitorfd> when
writing out the state information.

[..snip..] 
> > +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.
We have several things that need to be (re)stored. The id (if we don't
switch over to using PIDs), the monitor path, the domain state (running,
paused, ...), and the acutally used vncport are things that come to
mind. Some of the information is already in the domain.xml but not
getting reparsed properly (e.g. def->id is always set to -1 and the
vncport isn't reread if "autport=yes").
Therefore I introduced a flag VIR_DOMAIN_XML_STATE that tells the
virDomain*DefParse functions to set those values when reading back "state"
not "config". This somewhat mixes config with state which I don't like
too much. It would probably be better to add an extra set of functions
that takes the virDomainObjPtr instead of the virDomainDefPtr? This way
we could also fold in the monitor path and the domain state easily like:

<domstate state="running">
<monitor path="/dev/pts/4"/>
<domstate/>

Does this make sense? For now I use an extra file for the monitor path.

[..snip..] 
> > +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.
O.k. I'm currently only calling this when forking qemu, I'll add the
calls to the places where the config changes soonish.

[..snip..] 
> > 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
Done.
 -- Guido
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-let-qemu-kvm-survive-libvirtd-restarts.patch
Type: text/x-diff
Size: 26769 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20081125/147be644/attachment-0001.bin>