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.
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list