[libvirt] PATCH: Allow virtual networks to survive daemon restarts

[Daniel Veillard]

On Wed, Jan 14, 2009 at 12:44:50PM +0000, Daniel P. Berrange wrote:
> On Wed, Jan 14, 2009 at 08:55:46AM +0000, Mark McLoughlin wrote:
> > On Tue, 2009-01-13 at 20:49 +0000, Daniel P. Berrange wrote:
> > > Currently when we shutdown  the virtual networks are all shutdown too.
> > > This is less than useful if we're letting guest VMs hang around post
> > > shutdown of libvirtd, because it means we're tearing their network
> > > connection out from under them. This patch fixes that allowing networks
> > > to survive restarts, and be re-detected next time around.
> > > 
> > > When starting a virtual network we write the live config into
> > > 
> > >   /var/lib/libvirt/network/$NAME.xml
> > > 
> > > This is because the bridge device name is potentially auto-generated
> > > and we need to keep track of that
> > > 
> > > We change dnsmasq args to include an explicit pidfile location
> > > 
> > >   /var/run/libvirt/network/$NAME.pid
> > > 
> > > and also tell it to put itself into the background - ie daemonize. This
> > > is because we want dnsmasq to survive the daemon.
> > > 
> > > Now, when libvirtd starts up it
> > > 
> > >  - Looks for the live config, and if found loads it. 
> > >  - Calls a new method brHasBridge() to see if its desired bridge
> > >    actually exists (and thus whether the network is running).
> > >    If it exists,the network is marked active
> > >  - If DHCP is configured, then reads the dnsmasq PIDfile, and sends
> > >    kill($PID, 0) to check if the process is actually alive
> > > 
> > > In addition I cleanup the network code to remove the configFile and
> > > autostartLink fields in virNetworkObjPtr, so it matches virDomaiObjPtr
> > > usage.
> > > 
> > > With all this applied you can now restart the daemon, and virbr0 is
> > > left happily running.
> > 
> > It all sounds and looks good to me. Some comments below, but nothing
> > major.
> 
> Here's an updated patch with all your comments incorporated.

  Sounds a real user improvement, patch looks fine but I'm concerned
about this:

> +            /* Finally try and read dnsmasq pid if any DHCP ranges are set */
> +            if (obj->def->nranges &&
> +                virFileReadPid(NETWORK_PID_DIR, obj->def->name,
> +                               &obj->dnsmasqPid) == 0) {
> +
> +                /* Check its still alive */
> +                if (kill(obj->dnsmasqPid, 0) != 0)
> +                    obj->dnsmasqPid = -1;
> +
> +                /* XXX ideally we'd check this was actually
> +                 * the dnsmasq process, not a stale pid file
> +                 * with someone else's process. But how ?
> +                 */

   OS specific but check in configure about the location of dnsmasq
export it as DNSMASQ_PATH, then compare to file pointed by /proc/pid/exe

#ifdef __linux__
    char *pidpath;

    virAsprintf(&pidpath, "/proc/%d/exe", obj->dnsmasqPid);
    if (virFileLinkPointsTo(pidpath, DNSMASQ_PATH) == 0))
        obj->dnsmasqPid = -1;
    VIR_FREE(pidpath);
#endif

  I'm sure one can find a Solaris equivalent.


  Patch looks fine to me,

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/