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

14 Jan 2009

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.

    

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

Mark McLoughlin