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

[Mark McLoughlin]

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.

...
> diff --git a/src/bridge.c b/src/bridge.c
> --- a/src/bridge.c
> +++ b/src/bridge.c
> @@ -163,6 +163,43 @@ int brAddBridge (brControl *ctl ATTRIBUT
>  }
>  #endif
>  
> +#ifdef SIOCBRDELBR
> +int
> +brHasBridge(brControl *ctl,
> +            const char *name)
> +{
> +    struct ifreq ifr;
> +    int len;
> +
> +    if (!ctl || !name) {
> +        errno = EINVAL;
> +        return -1;
> +    }
> +
> +    if ((len = strlen(name)) >=  BR_IFNAME_MAXLEN) {
                                  ^^

Two spaces! Oh noes! Yay for nitpicks!

> +        errno = EINVAL;
> +        return -1;
> +    }
> +
> +    memset(&ifr, 0, sizeof(struct ifreq));
> +
> +    strncpy(ifr.ifr_name, name, len);
> +    ifr.ifr_name[len] = '\0';
> +
> +    if (ioctl(ctl->fd, SIOCGIFMTU, &ifr))
> +        return -1;

I'd probably use SIOCGIFFLAGS for this.

(Alternative is to use SIOCGIFBR to iterate over bridge names, but the
simpler approach is better)

...
> diff --git a/src/bridge.h b/src/bridge.h
> --- a/src/bridge.h
> +++ b/src/bridge.h
> @@ -50,6 +50,8 @@ int     brAddBridge             (brContr
>                                   char **name);
>  int     brDeleteBridge          (brControl *ctl,
>                                   const char *name);
> +int     brHasBridge             (brControl *ctl,
> +                                 const char *name);
>  
>  int     brAddInterface          (brControl *ctl,
>                                   const char *bridge,
> @@ -58,6 +60,7 @@ int     brDeleteInterface       (brContr
>                                   const char *bridge,
>                                   const char *iface);
>  
> +

Spurious.

... 
> @@ -695,48 +671,64 @@ int virNetworkSaveConfig(virConnectPtr c
>      if (safewrite(fd, xml, towrite) < 0) {
>          virReportSystemError(conn, errno,
>                               _("cannot write config file '%s'"),
> -                             net->configFile);
> +                             configFile);
>          goto cleanup;
>      }
>  
>      if (close(fd) < 0) {
>          virReportSystemError(conn, errno,
>                               _("cannot save config file '%s'"),
> -                             net->configFile);
> +                             configFile);
>          goto cleanup;
>      }
>  
>      ret = 0;
>  
>   cleanup:
> -    VIR_FREE(xml);
>      if (fd != -1)
>          close(fd);
>  
> +    VIR_FREE(configFile);
> +
>      return ret;
>  }
>  
> +int virNetworkSaveConfig(virConnectPtr conn,
> +                         const char *configDir,
> +                         virNetworkDefPtr def)
> +{
> +    int ret = -1;
> +    char *xml;
> +
> +    if (!(xml = virNetworkDefFormat(conn,
> +                                    def)))

Odd formatting.

...
> diff --git a/src/network_driver.c b/src/network_driver.c
> --- a/src/network_driver.c
> +++ b/src/network_driver.c
> @@ -57,6 +57,8 @@
>  #include "iptables.h"
>  #include "bridge.h"
>  
> +#define NETWORK_PID_DIR LOCAL_STATE_DIR "/run/libvirt/network"
> +#define NETWORK_LIB_DIR LOCAL_STATE_DIR "/lib/libvirt/network"

s/LIB_DIR/STATE_DIR/ perhaps?

Also, I'd prefer to see dirs like this created by "make install" and/or
RPM install.

e.g. with read-only root if you wanted /var/lib/libvirt to be read-only
and bind-mount a tmpfs onto /var/lib/libvirt/network

>  #define VIR_FROM_THIS VIR_FROM_NETWORK
>  
> @@ -106,6 +108,64 @@ static struct network_driver *driverStat
>  
> 
>  static void
> +networkFindActiveConfigs(struct network_driver *driver) {
> +    unsigned int i;
> +
> +    for (i = 0 ; i < driver->networks.count ; i++) {
> +        virNetworkObjPtr obj = driver->networks.objs[i];
> +        virNetworkDefPtr tmp;
> +        char *config;
> +
> +        virNetworkObjLock(obj);
> +
> +        if ((config = virNetworkConfigFile(NULL,
> +                                           NETWORK_LIB_DIR,
> +                                           obj->def->name)) == NULL) {
> +            virNetworkObjUnlock(obj);
> +            continue;
> +        }
> +
> +        if (access(config, R_OK) < 0) {
> +            VIR_FREE(config);
> +            virNetworkObjUnlock(obj);
> +            continue;
> +        }
> +
> +        /* Try and load the live config */
> +        tmp = virNetworkDefParseFile(NULL,config);
                                             ^
Missing whitespace.

...
> @@ -377,15 +443,6 @@ networkBuildDnsmasqArgv(virConnectPtr co
>      APPEND_ARG(*argv, i++, "--except-interface");
>      APPEND_ARG(*argv, i++, "lo");
>  
> -    /*
> -     * NB, dnsmasq command line arg bug means we need to
> -     * use a single arg '--dhcp-leasefile=path' rather than
> -     * two separate args in '--dhcp-leasefile path' style
> -     */

Worth keeping this comment - e.g. someone could come along and change it
do s/=/ /, check that it works with newer dnsmasq and then 6 months
later get a report that it doesn't work with older dnsmasq.

> -    snprintf(buf, sizeof(buf), "--dhcp-leasefile=%s/lib/libvirt/dhcp-%s.leases",
> -             LOCAL_STATE_DIR, network->def->name);
> -    APPEND_ARG(*argv, i++, buf);

--dhcp-leasefile not needed? Worth a comment in the commit log.

...
> @@ -442,13 +502,49 @@ dhcpStartDhcpDaemon(virConnectPtr conn,
>          return -1;
>      }
>  
> +    if ((err = virFileMakePath(NETWORK_PID_DIR)) < 0) {
> +        virReportSystemError(conn, err,
> +                             _("cannot create directory %s"),
> +                             NETWORK_PID_DIR);
> +        return -1;
> +    }
> +    if ((err = virFileMakePath(NETWORK_LIB_DIR)) < 0) {
> +        virReportSystemError(conn, err,
> +                             _("cannot create directory %s"),
> +                             NETWORK_LIB_DIR);
> +        return -1;
> +    }
> +
> +    if (!(pidfile = virFilePid(NETWORK_PID_DIR, network->def->name))) {
> +        virReportOOMError(conn);
> +        return -1;
> +    }
> +
>      argv = NULL;
> -    if (networkBuildDnsmasqArgv(conn, network, &argv) < 0)
> +    if (networkBuildDnsmasqArgv(conn, network, pidfile, &argv) < 0) {
> +        VIR_FREE(pidfile);
>          return -1;
> +    }
>  
> -    ret = virExec(conn, argv, NULL, NULL,
> -                  &network->dnsmasqPid, -1, NULL, NULL, VIR_EXEC_NONBLOCK);
> +    if (virRun(conn, argv, NULL) < 0)
> +        goto cleanup;
>  
> +    /*
> +     * There really is no race here - when dnsmasq daemonizes,
> +     * its leader process stays around until its child has
> +     * actually written its pidfile. So by time virRun exits
> +     * it has waitpid'd and guarenteed the proess has started

guaranteed

> +     * and writtena pid

written a

... 
> @@ -798,16 +892,24 @@ static int networkStartNetworkDaemon(vir
>  }
>  
> 
> -static int networkShutdownNetworkDaemon(virConnectPtr conn ATTRIBUTE_UNUSED,
> -                                      struct network_driver *driver,
> -                                      virNetworkObjPtr network) {
> +static int networkShutdownNetworkDaemon(virConnectPtr conn,
> +                                        struct network_driver *driver,
> +                                        virNetworkObjPtr network) {
>      int err;
> +    char *configFile;
>  
>      networkLog(NETWORK_INFO, _("Shutting down network '%s'\n"), network->def->name);
>  
>      if (!virNetworkIsActive(network))
>          return 0;
>  
> +    configFile = virNetworkConfigFile(conn, NETWORK_LIB_DIR, network->def->name);
> +    if (!configFile)
> +        return -1;
> +
> +    unlink(configFile);
> +    VIR_FREE(configFile);

Perhaps rename this to stateFile - I got confused for a second thinking
you were deleting the actual config file here.

>      if (network->dnsmasqPid > 0)
>          kill(network->dnsmasqPid, SIGTERM);
>  
> @@ -824,13 +926,10 @@ static int networkShutdownNetworkDaemon(
>                   network->def->bridge, strerror(err));
>      }
>  
> +    /* See if its still alive and really really kill it */
>      if (network->dnsmasqPid > 0 &&
> -        waitpid(network->dnsmasqPid, NULL, WNOHANG) != network->dnsmasqPid) {
> +        (kill(network->dnsmasqPid, 0) == 0))
>          kill(network->dnsmasqPid, SIGKILL);
> -        if (waitpid(network->dnsmasqPid, NULL, 0) != network->dnsmasqPid)
> -            networkLog(NETWORK_WARN,
> -                     "%s", _("Got unexpected pid for dnsmasq\n"));
> -    }

Why no more waitpid? Zombies, no?

...

Cheers,
Mark.