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

[Daniel P. Berrange]

On Tue, Nov 25, 2008 at 10:16:35PM +0100, Guido G?nther wrote:
> 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.

e should avoid /proc because this is Linux specific, and QEMU is
perfectly capable of being used on Solaris / BSD too. I'm fine
with storing it explicitly in /var/run/libvirt/qemu/$DOMAN.monitor

> > 
> > 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.

Hmm, interesting idea - we already have a variant for parsing based
off an arbitrary XML node, rather than assuming the root, 

  virDomainDefPtr virDomainDefParseNode(virConnectPtr conn,
                                        virCapsPtr caps,
                                        xmlDocPtr doc,
                                        xmlNodePtr root);

So we could add a wrapper function which writes out a XML doc

   <domstate state="running" pid="1234">
     <monitor path="/dev/pts/4">
     <domain id='32'>
      ... normal domain XML config ...
     </domain>
   </domstate>

And so have a function

   virDomainObjPtr virDomainObjParse(const char *filename)

which'd parse the custom QEMU state, and then just invoke  the
virDomainDefParseNode for the nested <domain> tag.

Now, some things like VNC port, domain ID really are easily tracked
in the regular domain XML - rather than adding VIR_DOMAIN_XML_STATE
I've a slight alternative idea.

The formatXML methods already accept a flag VIR_DOMAIN_XML_INACTIVE
which when passed causes it to skip some attributes which only
make sense for running VMs - VNC port, ID, VIF name

The parseXML methods, just hardcode this and always skip these
attributes. We should fix this so the parsing only skips these
attrs if this same VIR_DOMAIN_XML_INACTIVE flag is passed. Then
just make sure all existing code is changed to set this flag
when parsing XML. The code for loading VM from disk can simply
not set this flag. Technically the LXC driver shold be doing
this already, precisely for the VIF name issue.


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 :|