[libvirt] [PATCH 0/5] network: fix active status & usage of inactive networks
All 5 of these patches are required to address https://bugzilla.redhat.com/show_bug.cgi?id=880483 The reported problem is that an interface using a currently-inactive macvtap/hostdev network can be attached to a domain (addressed by 5/5). In order to make fixing that problem less of a problem, the automatic inactivation of all macvtap/hostdev networks any time libvirtd is restart must be addressed, and that is done in 1/5-4/5. Part of fixing the latter problem is changing the network driver to use /var/run for its status XML rather than /var/lib (2/5), which causes problems during upgrade (addressed in 3/5) and in a more limited sense downgrade (see the comments in 3/5 for why I haven't addressed those problems)
This directory should be created when the network driver is first started up, not just when a dhcp daemon is run. This hasn't posed a problem in the past, because the directory has always been pre-existing. --- src/network/bridge_driver.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index eb276cd..fdddc9a 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -487,6 +487,13 @@ networkStateInitialize(bool privileged, } } + if (virFileMakePath(driverState->stateDir) < 0) { + virReportSystemError(errno, + _("cannot create directory %s"), + driverState->stateDir); + goto cleanup; + } + /* if this fails now, it will be retried later with dnsmasqCapsRefresh() */ driverState->dnsmasqCaps = dnsmasqCapsNewFromBinary(DNSMASQ); @@ -1171,12 +1178,6 @@ networkStartDhcpDaemon(virNetworkDriverStatePtr driver, driverState->pidDir); goto cleanup; } - if (virFileMakePath(driverState->stateDir) < 0) { - virReportSystemError(errno, - _("cannot create directory %s"), - driverState->stateDir); - goto cleanup; - } if (!(pidfile = virPidFileBuildPath(driverState->pidDir, network->def->name))) -- 1.9.0
On 04/10/2014 09:19 AM, Laine Stump wrote:
This directory should be created when the network driver is first started up, not just when a dhcp daemon is run. This hasn't posed a problem in the past, because the directory has always been pre-existing. --- src/network/bridge_driver.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)
Moving out of the Refresh, Restart, and StartNetworkVirtual to earlier in the process - seems reasonable, so ACK John
For some reason these have been stored in /var/lib, although other drivers store their state files in /var/run. It's much nicer to store them in /var/run because they are automatically cleared out when the system reboots. This can be a useful way of learning whether or not a particular network is active. Note that this change by itself will cause problems in the case of an upgrade from an older libvirt that uses /var/lib, and also in the case of a downgrade from a newer libvirt using /var/run to an older version using /var/lib. This compatibility problem will be addressed in the next patch. --- src/network/bridge_driver.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index fdddc9a..a027b47 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -446,10 +446,11 @@ networkStateInitialize(bool privileged, * ~/.config/libvirt/... (session/unprivileged) * /etc/libvirt/... && /var/(run|lib)/libvirt/... (system/privileged). * - * NB: The qemu driver puts its domain state in /var/run, and I - * think the network driver should have used /var/run too (instead - * of /var/lib), but it's been this way for a long time, and we - * probably shouldn't change it now. + * NB: The network driver previously stored its status xml in + * /var/lib/libvirt/network instead of + * /var/run/libvirt/network. An upgrade downgrade between libvirt + * versions using different state directories may or may not cause + * problems. */ if (privileged) { if (VIR_STRDUP(driverState->networkConfigDir, @@ -457,7 +458,7 @@ networkStateInitialize(bool privileged, VIR_STRDUP(driverState->networkAutostartDir, SYSCONFDIR "/libvirt/qemu/networks/autostart") < 0 || VIR_STRDUP(driverState->stateDir, - LOCALSTATEDIR "/lib/libvirt/network") < 0 || + LOCALSTATEDIR "/run/libvirt/network") < 0 || VIR_STRDUP(driverState->pidDir, LOCALSTATEDIR "/run/libvirt/network") < 0 || VIR_STRDUP(driverState->dnsmasqStateDir, -- 1.9.0
On 04/10/2014 09:19 AM, Laine Stump wrote:
For some reason these have been stored in /var/lib, although other drivers store their state files in /var/run.
It's much nicer to store them in /var/run because they are automatically cleared out when the system reboots. This can be a useful way of learning whether or not a particular network is active.
Note that this change by itself will cause problems in the case of an upgrade from an older libvirt that uses /var/lib, and also in the case of a downgrade from a newer libvirt using /var/run to an older version using /var/lib. This compatibility problem will be addressed in the next patch. --- src/network/bridge_driver.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index fdddc9a..a027b47 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -446,10 +446,11 @@ networkStateInitialize(bool privileged, * ~/.config/libvirt/... (session/unprivileged) * /etc/libvirt/... && /var/(run|lib)/libvirt/... (system/privileged). * - * NB: The qemu driver puts its domain state in /var/run, and I - * think the network driver should have used /var/run too (instead - * of /var/lib), but it's been this way for a long time, and we - * probably shouldn't change it now. + * NB: The network driver previously stored its status xml in + * /var/lib/libvirt/network instead of + * /var/run/libvirt/network. An upgrade downgrade between libvirt + * versions using different state directories may or may not cause + * problems. */ if (privileged) { if (VIR_STRDUP(driverState->networkConfigDir, @@ -457,7 +458,7 @@ networkStateInitialize(bool privileged, VIR_STRDUP(driverState->networkAutostartDir, SYSCONFDIR "/libvirt/qemu/networks/autostart") < 0 || VIR_STRDUP(driverState->stateDir, - LOCALSTATEDIR "/lib/libvirt/network") < 0 || + LOCALSTATEDIR "/run/libvirt/network") < 0 || VIR_STRDUP(driverState->pidDir, LOCALSTATEDIR "/run/libvirt/network") < 0 || VIR_STRDUP(driverState->dnsmasqStateDir,
This is only being done for the /var/... files and not the non-privileged case, right? I guess part of me wonders why not follow through and do the same in the else here - wouldn't that code have the same 'feature' you're trying to take advantage of regarding how the run directory is used? I suppose I assume that the non priv'd environment acts just like the priv'd one except for location. Beyond that - it feels like 2 & 3 need to be combined or reworked to avoid the git bisect issues. John
On 04/15/2014 03:26 PM, John Ferlan wrote:
On 04/10/2014 09:19 AM, Laine Stump wrote:
For some reason these have been stored in /var/lib, although other drivers store their state files in /var/run.
It's much nicer to store them in /var/run because they are automatically cleared out when the system reboots. This can be a useful way of learning whether or not a particular network is active.
Note that this change by itself will cause problems in the case of an upgrade from an older libvirt that uses /var/lib, and also in the case of a downgrade from a newer libvirt using /var/run to an older version using /var/lib. This compatibility problem will be addressed in the next patch. --- src/network/bridge_driver.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index fdddc9a..a027b47 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -446,10 +446,11 @@ networkStateInitialize(bool privileged, * ~/.config/libvirt/... (session/unprivileged) * /etc/libvirt/... && /var/(run|lib)/libvirt/... (system/privileged). * - * NB: The qemu driver puts its domain state in /var/run, and I - * think the network driver should have used /var/run too (instead - * of /var/lib), but it's been this way for a long time, and we - * probably shouldn't change it now. + * NB: The network driver previously stored its status xml in + * /var/lib/libvirt/network instead of + * /var/run/libvirt/network. An upgrade downgrade between libvirt + * versions using different state directories may or may not cause + * problems. */ if (privileged) { if (VIR_STRDUP(driverState->networkConfigDir, @@ -457,7 +458,7 @@ networkStateInitialize(bool privileged, VIR_STRDUP(driverState->networkAutostartDir, SYSCONFDIR "/libvirt/qemu/networks/autostart") < 0 || VIR_STRDUP(driverState->stateDir, - LOCALSTATEDIR "/lib/libvirt/network") < 0 || + LOCALSTATEDIR "/run/libvirt/network") < 0 || VIR_STRDUP(driverState->pidDir, LOCALSTATEDIR "/run/libvirt/network") < 0 || VIR_STRDUP(driverState->dnsmasqStateDir,
This is only being done for the /var/... files and not the non-privileged case, right?
Correct.
I guess part of me wonders why not follow through and do the same in the else here
Because that else clause for non-privileged was added long after the original code for privileged was written, and the directories are already in the correct place from the beginning so they don't need to be fixed.
- wouldn't that code have the same 'feature' you're trying to take advantage of regarding how the run directory is used?
I'm actually not sure exactly what is done with the directories in ~/.config (when/if the $run directory is cleared, etc), but there was some sort of reconfig/standardization of the locations with gnome3 that the qemu driver was made to follow, and the network driver copies those locations faithfully (replacing "qemu" with "network" or "qemu/network" as appropriate).
I suppose I assume that the non priv'd environment acts just like the priv'd one except for location.
non-privileged libvirtd is a bit odd. For starters it isn't run until someone tries to connect to their user's qemu:///session, and then it automatically exits after some timeout. Beyond that, the network driver doesn't actually work (!!) for non-privileged libvirtd - adding the ~/.config directories was only done to prevent it from crashing, with the thinking that if we *did* make it work sometime in the future, those would be the right places to store the files (since it works for qemu...)
Beyond that - it feels like 2 & 3 need to be combined or reworked to avoid the git bisect issues.
Well, yes and no. Both patches independently pass make check and make syntax-check, and I hadn't considered that anyone would do a bisect where the test at each step was to install an older libvirt, then upgrade to the newly built bisect binary. However, my main reason for separating the two was just to make review easier (as well as in case I decided to make any more complicated alternative to 3/5), and there's no point in dooming such a bisect test to failure for no good reason, so I would be fine with merging 2/5 and 3/5.
This eliminates problems with upgrading from older libvirt that stores network status in /var/lib/libvirt/network to newer libvirt that stores it in /var/run/libvirt/network, by also reading any status files from the old location, saving them to the new location, and removing them from the old location. This will not help those trying to downgrade, but in practice this will only be problematic in two cases 1) If there are networks with network-wide bandwidth limits configured and in use by a guest during a downgrade to "old" libvirt. In this case, the class ID's used for that network's tc rules, as well as the currently in-use bandwidth "floor" will be forgotten 2) If someone does this: 1) upgrade, 2) downgrade, 3) modify running state of network (e.g. add a static dhcp host, etc), 4) upgrade. In this case, the modifications to the running network will be lost (but not any persistent changes to the network's config). I have an idea of how to make these cases work properly as well (involving symlinks in /var/lib pointing to /var/run) but want to avoid keeping such an ugly legacy around (forever) unlesss others think it is absolutely necessary to support flawless downgrades across this line *while remaining in service*. --- src/network/bridge_driver.c | 40 ++++++++++++++++++++++++++++++++++++ src/network/bridge_driver_platform.h | 3 ++- 2 files changed, 42 insertions(+), 1 deletion(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index a027b47..98d10bd 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -459,6 +459,8 @@ networkStateInitialize(bool privileged, SYSCONFDIR "/libvirt/qemu/networks/autostart") < 0 || VIR_STRDUP(driverState->stateDir, LOCALSTATEDIR "/run/libvirt/network") < 0 || + VIR_STRDUP(driverState->oldStateDir, + LOCALSTATEDIR "/lib/libvirt/network") < 0 || VIR_STRDUP(driverState->pidDir, LOCALSTATEDIR "/run/libvirt/network") < 0 || VIR_STRDUP(driverState->dnsmasqStateDir, @@ -498,6 +500,44 @@ networkStateInitialize(bool privileged, /* if this fails now, it will be retried later with dnsmasqCapsRefresh() */ driverState->dnsmasqCaps = dnsmasqCapsNewFromBinary(DNSMASQ); + /* Due to a change in location of network state xml beginning in + * libvirt 1.2.4, we must check for state files in two + * locations. Anything found in the old location must be written + * to the new location, then erased from the old location. + */ + if (driverState->oldStateDir && + STRNEQ(driverState->stateDir, driverState->oldStateDir)) { + size_t i; + + /* read all */ + if (virNetworkLoadAllState(&driverState->networks, + driverState->oldStateDir) < 0) + goto error; + + for (i = 0; i < driverState->networks.count; i++) { + virNetworkObjPtr network = driverState->networks.objs[i]; + char *oldFile; + int status; + + /* save in new location */ + virNetworkObjLock(network); + status = virNetworkSaveStatus(driverState->stateDir, network); + virNetworkObjUnlock(network); + if (status < 0) + goto cleanup; + + /* remove from old location */ + oldFile = virNetworkConfigFile(driverState->oldStateDir, + network->def->name); + if (!oldFile) + goto cleanup; + unlink(oldFile); + VIR_FREE(oldFile); + } + /* these will all be re-read from their new location */ + virNetworkObjListFree(&driverState->networks); + } + if (virNetworkLoadAllState(&driverState->networks, driverState->stateDir) < 0) goto error; diff --git a/src/network/bridge_driver_platform.h b/src/network/bridge_driver_platform.h index 6a571da..491068a 100644 --- a/src/network/bridge_driver_platform.h +++ b/src/network/bridge_driver_platform.h @@ -1,7 +1,7 @@ /* * bridge_driver_platform.h: platform specific routines for bridge driver * - * Copyright (C) 2006-2013 Red Hat, Inc. + * Copyright (C) 2006-2014 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -39,6 +39,7 @@ struct _virNetworkDriverState { char *networkConfigDir; char *networkAutostartDir; char *stateDir; + char *oldStateDir; char *pidDir; char *dnsmasqStateDir; char *radvdStateDir; -- 1.9.0
On 04/10/2014 09:19 AM, Laine Stump wrote:
This eliminates problems with upgrading from older libvirt that stores network status in /var/lib/libvirt/network to newer libvirt that stores it in /var/run/libvirt/network, by also reading any status files from the old location, saving them to the new location, and removing them from the old location.
This will not help those trying to downgrade, but in practice this will only be problematic in two cases
1) If there are networks with network-wide bandwidth limits configured and in use by a guest during a downgrade to "old" libvirt. In this case, the class ID's used for that network's tc rules, as well as the currently in-use bandwidth "floor" will be forgotten
2) If someone does this: 1) upgrade, 2) downgrade, 3) modify running state of network (e.g. add a static dhcp host, etc), 4) upgrade. In this case, the modifications to the running network will be lost (but not any persistent changes to the network's config).
I have an idea of how to make these cases work properly as well (involving symlinks in /var/lib pointing to /var/run) but want to avoid keeping such an ugly legacy around (forever) unlesss others think it is absolutely necessary to support flawless downgrades across this line *while remaining in service*.
I would have to believe/think the downgrade issue has happened before with some other change to config or status file locations. Of course, if some state/status is meant to "live" beyond the current execution, then perhaps /run/ isn't the right place to store that. Wouldn't something like that end up in /etc/libvirt... somewhere? That is usage of '--live' vs. '--config' when configuring limits?
--- src/network/bridge_driver.c | 40 ++++++++++++++++++++++++++++++++++++ src/network/bridge_driver_platform.h | 3 ++- 2 files changed, 42 insertions(+), 1 deletion(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index a027b47..98d10bd 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -459,6 +459,8 @@ networkStateInitialize(bool privileged, SYSCONFDIR "/libvirt/qemu/networks/autostart") < 0 || VIR_STRDUP(driverState->stateDir, LOCALSTATEDIR "/run/libvirt/network") < 0 || + VIR_STRDUP(driverState->oldStateDir, + LOCALSTATEDIR "/lib/libvirt/network") < 0 ||
You'll need a corresponding VIR_FREE() in networkStateCleanup()
VIR_STRDUP(driverState->pidDir, LOCALSTATEDIR "/run/libvirt/network") < 0 || VIR_STRDUP(driverState->dnsmasqStateDir, @@ -498,6 +500,44 @@ networkStateInitialize(bool privileged, /* if this fails now, it will be retried later with dnsmasqCapsRefresh() */ driverState->dnsmasqCaps = dnsmasqCapsNewFromBinary(DNSMASQ);
+ /* Due to a change in location of network state xml beginning in + * libvirt 1.2.4, we must check for state files in two + * locations. Anything found in the old location must be written + * to the new location, then erased from the old location. + */
ahh... so this isn't done for the non privileged case... So if privileged and the network state exists in the old state dir... Also could use STRNEQ_NULLABLE() right?
+ if (driverState->oldStateDir && + STRNEQ(driverState->stateDir, driverState->oldStateDir)) { + size_t i; + + /* read all */ + if (virNetworkLoadAllState(&driverState->networks, + driverState->oldStateDir) < 0) + goto error;
So any/all failures to read the old entry/style causes failure... Could this be problematic if something ends up existing in the "new" format that isn't in the "old" format and thus causes exit by the changed parsing code? This is perhaps the opposite problem to flawless downgrades. It also points out the question of how long do we keep this old reading code around before it's removed?
+ + for (i = 0; i < driverState->networks.count; i++) { + virNetworkObjPtr network = driverState->networks.objs[i]; + char *oldFile; + int status; + + /* save in new location */ + virNetworkObjLock(network); + status = virNetworkSaveStatus(driverState->stateDir, network); + virNetworkObjUnlock(network); + if (status < 0) + goto cleanup; + + /* remove from old location */ + oldFile = virNetworkConfigFile(driverState->oldStateDir, + network->def->name); + if (!oldFile) + goto cleanup; + unlink(oldFile);
Alternatively to removing - rename to some suffix which at least gives a chance for someone going through a downgrade process to recover the old file. Of course documenting this would be nice too.
+ VIR_FREE(oldFile); + } + /* these will all be re-read from their new location */ + virNetworkObjListFree(&driverState->networks); + } +
This hunk should be it's own local/static function... ACK pending your thoughts regarding my comments... John
if (virNetworkLoadAllState(&driverState->networks, driverState->stateDir) < 0) goto error; diff --git a/src/network/bridge_driver_platform.h b/src/network/bridge_driver_platform.h index 6a571da..491068a 100644 --- a/src/network/bridge_driver_platform.h +++ b/src/network/bridge_driver_platform.h @@ -1,7 +1,7 @@ /* * bridge_driver_platform.h: platform specific routines for bridge driver * - * Copyright (C) 2006-2013 Red Hat, Inc. + * Copyright (C) 2006-2014 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -39,6 +39,7 @@ struct _virNetworkDriverState { char *networkConfigDir; char *networkAutostartDir; char *stateDir; + char *oldStateDir; char *pidDir; char *dnsmasqStateDir; char *radvdStateDir;
On 04/15/2014 03:27 PM, John Ferlan wrote:
On 04/10/2014 09:19 AM, Laine Stump wrote:
This eliminates problems with upgrading from older libvirt that stores network status in /var/lib/libvirt/network to newer libvirt that stores it in /var/run/libvirt/network, by also reading any status files from the old location, saving them to the new location, and removing them from the old location.
This will not help those trying to downgrade, but in practice this will only be problematic in two cases
1) If there are networks with network-wide bandwidth limits configured and in use by a guest during a downgrade to "old" libvirt. In this case, the class ID's used for that network's tc rules, as well as the currently in-use bandwidth "floor" will be forgotten
2) If someone does this: 1) upgrade, 2) downgrade, 3) modify running state of network (e.g. add a static dhcp host, etc), 4) upgrade. In this case, the modifications to the running network will be lost (but not any persistent changes to the network's config).
I have an idea of how to make these cases work properly as well (involving symlinks in /var/lib pointing to /var/run) but want to avoid keeping such an ugly legacy around (forever) unlesss others think it is absolutely necessary to support flawless downgrades across this line *while remaining in service*. I would have to believe/think the downgrade issue has happened before with some other change to config or status file locations.
Thinking about it, there was one similar situation - when sessionmode libvirtd switched from using gnome2 config directories to gnome3. I don't recall if anything special was done in that case, or if it was just considered not important because ... session mode. AFAIK, the run directory for the qemu, lxc, and network drivers hasn't before been changed, so there's no precendent to follow there.
Of course, if some state/status is meant to "live" beyond the current execution, then perhaps /run/ isn't the right place to store that. Wouldn't something like that end up in /etc/libvirt... somewhere? That is usage of '--live' vs. '--config' when configuring limits?
No, this is exactly what the qemu and lxc drivers do. Their persistent config is stored in /etc/libvirt, and their runtime status is stored in /var/run/libvirt. This is specifically so that when the host is rebooted, stale status is automatically cleared out to avoid confusing libvirtd when it is started the first time after a reboot. When libvirtd is subsequently restarted, its pre-restart state for all domains and networks *must* be maintained to avoid interruption of service, and /var/run is just the right place to do that. (As an aside, in the case of the network driver, much of the state of a network is reconstructed when libvirtd is restarted rather than relying on a correct statedir; this was mostly necessary due to the unreliability of the state data in the past. The only reason I'm going to all this trouble now is that I have a need for a piece of status that can't be reconstructed from other host and domain information but can only come the status file itself - the answer to the basic "is this network active?" question.
--- src/network/bridge_driver.c | 40 ++++++++++++++++++++++++++++++++++++ src/network/bridge_driver_platform.h | 3 ++- 2 files changed, 42 insertions(+), 1 deletion(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index a027b47..98d10bd 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -459,6 +459,8 @@ networkStateInitialize(bool privileged, SYSCONFDIR "/libvirt/qemu/networks/autostart") < 0 || VIR_STRDUP(driverState->stateDir, LOCALSTATEDIR "/run/libvirt/network") < 0 || + VIR_STRDUP(driverState->oldStateDir, + LOCALSTATEDIR "/lib/libvirt/network") < 0 || You'll need a corresponding VIR_FREE() in networkStateCleanup()
Oops!
VIR_STRDUP(driverState->pidDir, LOCALSTATEDIR "/run/libvirt/network") < 0 || VIR_STRDUP(driverState->dnsmasqStateDir, @@ -498,6 +500,44 @@ networkStateInitialize(bool privileged, /* if this fails now, it will be retried later with dnsmasqCapsRefresh() */ driverState->dnsmasqCaps = dnsmasqCapsNewFromBinary(DNSMASQ);
+ /* Due to a change in location of network state xml beginning in + * libvirt 1.2.4, we must check for state files in two + * locations. Anything found in the old location must be written + * to the new location, then erased from the old location. + */
ahh... so this isn't done for the non privileged case...
Right. Not needed because we haven't changed the location of the stateDir for non-privileged (and, as I said previously, the network driver doesn't actually work for non-privileged mode, beyond simply not crashing :-P)
So if privileged and the network state exists in the old state dir... Also could use STRNEQ_NULLABLE() right?
No. STRNEQ_NULLABLE(a, b) will be TRUE in the case that b is NULL, but we only want the body of the conditional executed if oldStateDir is NON-NULL and it doesn't equal stateDir.
+ if (driverState->oldStateDir && + STRNEQ(driverState->stateDir, driverState->oldStateDir)) { + size_t i; + + /* read all */ + if (virNetworkLoadAllState(&driverState->networks, + driverState->oldStateDir) < 0) + goto error; So any/all failures to read the old entry/style causes failure... Could this be problematic if something ends up existing in the "new" format that isn't in the "old" format and thus causes exit by the changed parsing code?
Well, we haven't changed the grammar at all, only the location we're reading from. And we know that, by definition, if we change the grammar in the future, it will be backward compatible with the current grammar, so there will be no failure caused by incompatible grammar during an upgrade. In the case that there is some other problem reading the status files during upgrade, the reaction would be no different from what would happen if there was a problem reading the status files when we *aren't* upgrading - the network driver would fail to load, and the status files would still exist in the oldStateDir, ready to a retry the next time we start. For downgrades, we're not trying to move the state files back to the old location anyway, so I don't see any extra problem beyond the fact that the status directory known by the old libvirt will be empty, so some bandwidth state will be lost, and macvtap/hostdev networks will be marked inactive (but that's already covered in the comments)
This is perhaps the opposite problem to flawless downgrades. It also points out the question of how long do we keep this old reading code around before it's removed?
In a theoretic sense, we should never get rid of it. But practically speaking someday somebody may decide to eliminate it (when libvirt has advanced so many versions that we're sure everyone is running on a "post-epoch" version, or at least that they can't do a live upgrade from a pre-epoch version to the newest version anyway).
+ + for (i = 0; i < driverState->networks.count; i++) { + virNetworkObjPtr network = driverState->networks.objs[i]; + char *oldFile; + int status; + + /* save in new location */ + virNetworkObjLock(network); + status = virNetworkSaveStatus(driverState->stateDir, network); + virNetworkObjUnlock(network); + if (status < 0) + goto cleanup; + + /* remove from old location */ + oldFile = virNetworkConfigFile(driverState->oldStateDir, + network->def->name); + if (!oldFile) + goto cleanup; + unlink(oldFile); Alternatively to removing - rename to some suffix which at least gives a chance for someone going through a downgrade process to recover the old file. Of course documenting this would be nice too.
In that case, the most recent status file will exist in the new stateDir anyway, but if I renamed any file I found in oldStateDir and left it there for potential re-use during a future downgrade, it might be several *months* old and have survived multiple edits to the network + reboots of the host. In the meantime, if we *don't* downgrade, we'll have extra files cluttering up /var/lib/libvirt confusing people essentially forever. Much better to get rid of it as soon as possible.
+ VIR_FREE(oldFile); + } + /* these will all be re-read from their new location */ + virNetworkObjListFree(&driverState->networks); + } + This hunk should be it's own local/static function...
Any particular reason for this? It is only used once in one place, and the function it's in isn't all that long. I suppose I can move it into a static function declared just above this one though.
ACK pending your thoughts regarding my comments...
John
if (virNetworkLoadAllState(&driverState->networks, driverState->stateDir) < 0) goto error; diff --git a/src/network/bridge_driver_platform.h b/src/network/bridge_driver_platform.h index 6a571da..491068a 100644 --- a/src/network/bridge_driver_platform.h +++ b/src/network/bridge_driver_platform.h @@ -1,7 +1,7 @@ /* * bridge_driver_platform.h: platform specific routines for bridge driver * - * Copyright (C) 2006-2013 Red Hat, Inc. + * Copyright (C) 2006-2014 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -39,6 +39,7 @@ struct _virNetworkDriverState { char *networkConfigDir; char *networkAutostartDir; char *stateDir; + char *oldStateDir; char *pidDir; char *dnsmasqStateDir; char *radvdStateDir;
On Tue, Apr 15, 2014 at 05:31:17PM +0300, Laine Stump wrote:
On 04/15/2014 03:27 PM, John Ferlan wrote:
On 04/10/2014 09:19 AM, Laine Stump wrote:
This eliminates problems with upgrading from older libvirt that stores network status in /var/lib/libvirt/network to newer libvirt that stores it in /var/run/libvirt/network, by also reading any status files from the old location, saving them to the new location, and removing them from the old location.
This will not help those trying to downgrade, but in practice this will only be problematic in two cases
1) If there are networks with network-wide bandwidth limits configured and in use by a guest during a downgrade to "old" libvirt. In this case, the class ID's used for that network's tc rules, as well as the currently in-use bandwidth "floor" will be forgotten
2) If someone does this: 1) upgrade, 2) downgrade, 3) modify running state of network (e.g. add a static dhcp host, etc), 4) upgrade. In this case, the modifications to the running network will be lost (but not any persistent changes to the network's config).
I have an idea of how to make these cases work properly as well (involving symlinks in /var/lib pointing to /var/run) but want to avoid keeping such an ugly legacy around (forever) unlesss others think it is absolutely necessary to support flawless downgrades across this line *while remaining in service*. I would have to believe/think the downgrade issue has happened before with some other change to config or status file locations.
Thinking about it, there was one similar situation - when sessionmode libvirtd switched from using gnome2 config directories to gnome3. I don't recall if anything special was done in that case, or if it was just considered not important because ... session mode.
We migrated the locations - see migrateProfile() in daemon/libvirtd.c Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 04/15/2014 10:31 AM, Laine Stump wrote:
On 04/15/2014 03:27 PM, John Ferlan wrote:
On 04/10/2014 09:19 AM, Laine Stump wrote:
This eliminates problems with upgrading from older libvirt that stores network status in /var/lib/libvirt/network to newer libvirt that stores it in /var/run/libvirt/network, by also reading any status files from the old location, saving them to the new location, and removing them from the old location.
This will not help those trying to downgrade, but in practice this will only be problematic in two cases
1) If there are networks with network-wide bandwidth limits configured and in use by a guest during a downgrade to "old" libvirt. In this case, the class ID's used for that network's tc rules, as well as the currently in-use bandwidth "floor" will be forgotten
2) If someone does this: 1) upgrade, 2) downgrade, 3) modify running state of network (e.g. add a static dhcp host, etc), 4) upgrade. In this case, the modifications to the running network will be lost (but not any persistent changes to the network's config).
I have an idea of how to make these cases work properly as well (involving symlinks in /var/lib pointing to /var/run) but want to avoid keeping such an ugly legacy around (forever) unlesss others think it is absolutely necessary to support flawless downgrades across this line *while remaining in service*. I would have to believe/think the downgrade issue has happened before with some other change to config or status file locations.
Thinking about it, there was one similar situation - when sessionmode libvirtd switched from using gnome2 config directories to gnome3. I don't recall if anything special was done in that case, or if it was just considered not important because ... session mode.
AFAIK, the run directory for the qemu, lxc, and network drivers hasn't before been changed, so there's no precendent to follow there.
Of course, if some state/status is meant to "live" beyond the current execution, then perhaps /run/ isn't the right place to store that. Wouldn't something like that end up in /etc/libvirt... somewhere? That is usage of '--live' vs. '--config' when configuring limits?
No, this is exactly what the qemu and lxc drivers do. Their persistent config is stored in /etc/libvirt, and their runtime status is stored in /var/run/libvirt. This is specifically so that when the host is rebooted, stale status is automatically cleared out to avoid confusing libvirtd when it is started the first time after a reboot. When libvirtd is subsequently restarted, its pre-restart state for all domains and networks *must* be maintained to avoid interruption of service, and /var/run is just the right place to do that.
Right - perhaps I understated my thoughts... I was specifically thinking/typing out loud that if someone made a change to their configuration "live only", then downgraded - should they really expect for that to be persistent when the network is restarted after the downgrade? I guess I would think perhaps not. If they really wanted a change to persist, then one would think (or assume - hah!) that they would ensure their changes were persistent before they downgraded.
(As an aside, in the case of the network driver, much of the state of a network is reconstructed when libvirtd is restarted rather than relying on a correct statedir; this was mostly necessary due to the unreliability of the state data in the past. The only reason I'm going to all this trouble now is that I have a need for a piece of status that can't be reconstructed from other host and domain information but can only come the status file itself - the answer to the basic "is this network active?" question.
Right - been down this path before with a previous product and yeah it's ugly/painful. Of course I had a lot more history with that product so I knew which rabbit holes to not jump into for fear of never coming back out :-)... Still navigating my way through the libvirt mazes of twisty little passages...
--- src/network/bridge_driver.c | 40 ++++++++++++++++++++++++++++++++++++ src/network/bridge_driver_platform.h | 3 ++- 2 files changed, 42 insertions(+), 1 deletion(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index a027b47..98d10bd 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -459,6 +459,8 @@ networkStateInitialize(bool privileged, SYSCONFDIR "/libvirt/qemu/networks/autostart") < 0 || VIR_STRDUP(driverState->stateDir, LOCALSTATEDIR "/run/libvirt/network") < 0 || + VIR_STRDUP(driverState->oldStateDir, + LOCALSTATEDIR "/lib/libvirt/network") < 0 || You'll need a corresponding VIR_FREE() in networkStateCleanup()
Oops!
VIR_STRDUP(driverState->pidDir, LOCALSTATEDIR "/run/libvirt/network") < 0 || VIR_STRDUP(driverState->dnsmasqStateDir, @@ -498,6 +500,44 @@ networkStateInitialize(bool privileged, /* if this fails now, it will be retried later with dnsmasqCapsRefresh() */ driverState->dnsmasqCaps = dnsmasqCapsNewFromBinary(DNSMASQ);
+ /* Due to a change in location of network state xml beginning in + * libvirt 1.2.4, we must check for state files in two + * locations. Anything found in the old location must be written + * to the new location, then erased from the old location. + */
ahh... so this isn't done for the non privileged case...
Right. Not needed because we haven't changed the location of the stateDir for non-privileged (and, as I said previously, the network driver doesn't actually work for non-privileged mode, beyond simply not crashing :-P)
So if privileged and the network state exists in the old state dir... Also could use STRNEQ_NULLABLE() right?
No. STRNEQ_NULLABLE(a, b) will be TRUE in the case that b is NULL, but we only want the body of the conditional executed if oldStateDir is NON-NULL and it doesn't equal stateDir.
+ if (driverState->oldStateDir && + STRNEQ(driverState->stateDir, driverState->oldStateDir)) { + size_t i; + + /* read all */ + if (virNetworkLoadAllState(&driverState->networks, + driverState->oldStateDir) < 0) + goto error; So any/all failures to read the old entry/style causes failure... Could this be problematic if something ends up existing in the "new" format that isn't in the "old" format and thus causes exit by the changed parsing code?
Well, we haven't changed the grammar at all, only the location we're reading from. And we know that, by definition, if we change the grammar in the future, it will be backward compatible with the current grammar, so there will be no failure caused by incompatible grammar during an upgrade.
In the case that there is some other problem reading the status files during upgrade, the reaction would be no different from what would happen if there was a problem reading the status files when we *aren't* upgrading - the network driver would fail to load, and the status files would still exist in the oldStateDir, ready to a retry the next time we start.
For downgrades, we're not trying to move the state files back to the old location anyway, so I don't see any extra problem beyond the fact that the status directory known by the old libvirt will be empty, so some bandwidth state will be lost, and macvtap/hostdev networks will be marked inactive (but that's already covered in the comments)
Yep - OK reasonable... again typing/thinking out loud while reading
This is perhaps the opposite problem to flawless downgrades. It also points out the question of how long do we keep this old reading code around before it's removed?
In a theoretic sense, we should never get rid of it. But practically speaking someday somebody may decide to eliminate it (when libvirt has advanced so many versions that we're sure everyone is running on a "post-epoch" version, or at least that they can't do a live upgrade from a pre-epoch version to the newest version anyway).
This is one of those "policy" things - that is - does libvirt have time policy for code to live... Perhaps you just need to set yourself a little reminder that in libvirt 1.2.6 or somewhere thereafter you'll come back and delete the code. It doesn't hurt to have it other than being able to recall history.
+ + for (i = 0; i < driverState->networks.count; i++) { + virNetworkObjPtr network = driverState->networks.objs[i]; + char *oldFile; + int status; + + /* save in new location */ + virNetworkObjLock(network); + status = virNetworkSaveStatus(driverState->stateDir, network); + virNetworkObjUnlock(network); + if (status < 0) + goto cleanup; + + /* remove from old location */ + oldFile = virNetworkConfigFile(driverState->oldStateDir, + network->def->name); + if (!oldFile) + goto cleanup; + unlink(oldFile); Alternatively to removing - rename to some suffix which at least gives a chance for someone going through a downgrade process to recover the old file. Of course documenting this would be nice too.
In that case, the most recent status file will exist in the new stateDir anyway, but if I renamed any file I found in oldStateDir and left it there for potential re-use during a future downgrade, it might be several *months* old and have survived multiple edits to the network + reboots of the host. In the meantime, if we *don't* downgrade, we'll have extra files cluttering up /var/lib/libvirt confusing people essentially forever.
Much better to get rid of it as soon as possible.
right - but if there ever was a need to downgrade - the last known version of something is available. I'm fine with deleting - just providing the obvious alternative since no code other than a downgraded environment would read it. Far more detrimental to keep cruft around. In the end it comes down to which is preferred to support - the hassle of having some shrapnel from a downgrade from someone that lost their changes or the hassle of having stale state/status data in a downgrade.
+ VIR_FREE(oldFile); + } + /* these will all be re-read from their new location */ + virNetworkObjListFree(&driverState->networks); + } + This hunk should be it's own local/static function...
Any particular reason for this? It is only used once in one place, and the function it's in isn't all that long. I suppose I can move it into a static function declared just above this one though.
Mostly readability/flow/isolation and 24 lines of code in it's own separate function that could very easily be eliminated. Just seems other code in the function will make calls and then there's this hunk of code that doesn't... I'm not against leaving it either. I'm OK with your other explanations - I don't see anyone squawking loudly about moving the location and/or saving old state "just in case". Seems just an addition of the VIR_FREE() then the combination of 2&3 to keep the bisection from generating a false positive for someone and we're good to go. John
ACK pending your thoughts regarding my comments...
John
if (virNetworkLoadAllState(&driverState->networks, driverState->stateDir) < 0) goto error; diff --git a/src/network/bridge_driver_platform.h b/src/network/bridge_driver_platform.h index 6a571da..491068a 100644 --- a/src/network/bridge_driver_platform.h +++ b/src/network/bridge_driver_platform.h @@ -1,7 +1,7 @@ /* * bridge_driver_platform.h: platform specific routines for bridge driver * - * Copyright (C) 2006-2013 Red Hat, Inc. + * Copyright (C) 2006-2014 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -39,6 +39,7 @@ struct _virNetworkDriverState { char *networkConfigDir; char *networkAutostartDir; char *stateDir; + char *oldStateDir; char *pidDir; char *dnsmasqStateDir; char *radvdStateDir;
libvirt attempts to determine at startup time which networks are already active, and set their active flags. Previously it has done this by assuming that all networks are inactive, then setting the flag if the network has a bridge device associated with it, and that bridge device exists. This is not useful for macvtap and hostdev based networks, since they do not use a bridge device. Of course the reason that such a check had to be done was that the presence of a status file in the network "stateDir" couldn't be trusted as an indicator of whether or not a network was active. This was due to the network driver mistakenly using /var/lib/libvirt/network to store the status files, rather than /var/run/libvirt/network (similar to what is done by every other libirt driver that stores status xml for its objects). The difference is that /var/run is cleared out when the host reboots, so you cvqan be assured that the state file you are seeing isn't just left over from a previous boot of the host. Now that the network driver has been switched to using /var/run/libvirt/network for status, we can also modify it to assume that any network with an existing status file is by definition active - we do this when reading the status file. To fine tune the results, networkFindActiveConfigs() is changed to networkFindInactiveConfigs(), and only sets active = 0 if the conditions for particular network types are *not* met. The result is that during the first run of libvirtd after the host boots, there are no status files, so no networks are active. Any time it is restarted, and network with a status file will be marked as inactive (unless it uses a bridge device and that device for some reason doesn't exist). --- src/conf/network_conf.c | 1 + src/network/bridge_driver.c | 49 +++++++++++++++++++++++++++++++++------------ 2 files changed, 37 insertions(+), 13 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 56c4a09..69ad929 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -3060,6 +3060,7 @@ virNetworkLoadState(virNetworkObjListPtr nets, net->floor_sum = floor_sum_val; net->taint = taint; + net->active = 1; /* any network with a state file is by definition active */ cleanup: VIR_FREE(configFile); diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 98d10bd..7beab79 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -328,7 +328,7 @@ networkBridgeDummyNicName(const char *brname) } static void -networkFindActiveConfigs(virNetworkDriverStatePtr driver) +networkFindInactiveConfigs(virNetworkDriverStatePtr driver) { size_t i; @@ -336,29 +336,51 @@ networkFindActiveConfigs(virNetworkDriverStatePtr driver) virNetworkObjPtr obj = driver->networks.objs[i]; virNetworkObjLock(obj); - - /* If bridge exists, then mark it active */ - if (obj->def->bridge && - virNetDevExists(obj->def->bridge) == 1) { - obj->active = 1; + switch (obj->def->forward.type) { + case VIR_NETWORK_FORWARD_NONE: + case VIR_NETWORK_FORWARD_NAT: + case VIR_NETWORK_FORWARD_ROUTE: + /* If bridge exists, then mark it active */ + if (!(obj->def->bridge && virNetDevExists(obj->def->bridge) == 1)) { + obj->active = 0; + break; + } /* Try and read dnsmasq/radvd pids if any */ if (obj->def->ips && (obj->def->nips > 0)) { char *radvdpidbase; - ignore_value(virPidFileReadIfAlive(driverState->pidDir, obj->def->name, + ignore_value(virPidFileReadIfAlive(driverState->pidDir, + obj->def->name, &obj->dnsmasqPid, dnsmasqCapsGetBinaryPath(driver->dnsmasqCaps))); - - if (!(radvdpidbase = networkRadvdPidfileBasename(obj->def->name))) - goto cleanup; - ignore_value(virPidFileReadIfAlive(driverState->pidDir, radvdpidbase, + radvdpidbase = networkRadvdPidfileBasename(obj->def->name); + if (!radvdpidbase) + break; + ignore_value(virPidFileReadIfAlive(driverState->pidDir, + radvdpidbase, &obj->radvdPid, RADVD)); VIR_FREE(radvdpidbase); } + break; + + case VIR_NETWORK_FORWARD_BRIDGE: + if (obj->def->bridge) { + if (virNetDevExists(obj->def->bridge) != 1) + obj->active = 0; + break; + } + /* intentionally drop through to common case for all + * macvtap networks (forward='bridge' with no bridge + * device defined is macvtap using its 'bridge' mode) + */ + case VIR_NETWORK_FORWARD_PRIVATE: + case VIR_NETWORK_FORWARD_VEPA: + case VIR_NETWORK_FORWARD_PASSTHROUGH: + case VIR_NETWORK_FORWARD_HOSTDEV: + break; } - cleanup: virNetworkObjUnlock(obj); } @@ -542,12 +564,13 @@ networkStateInitialize(bool privileged, driverState->stateDir) < 0) goto error; + networkFindInactiveConfigs(driverState); + if (virNetworkLoadAllConfigs(&driverState->networks, driverState->networkConfigDir, driverState->networkAutostartDir) < 0) goto error; - networkFindActiveConfigs(driverState); networkReloadFirewallRules(driverState); networkRefreshDaemons(driverState); -- 1.9.0
On 04/10/2014 09:19 AM, Laine Stump wrote:
libvirt attempts to determine at startup time which networks are already active, and set their active flags. Previously it has done this by assuming that all networks are inactive, then setting the flag if the network has a bridge device associated with it, and that bridge device exists. This is not useful for macvtap and hostdev based networks, since they do not use a bridge device.
Of course the reason that such a check had to be done was that the presence of a status file in the network "stateDir" couldn't be trusted as an indicator of whether or not a network was active. This was due to the network driver mistakenly using /var/lib/libvirt/network to store the status files, rather than /var/run/libvirt/network (similar to what is done by every other libirt driver that stores status xml for its objects). The difference is that /var/run is cleared out when the host reboots, so you cvqan be assured that the state file you are seeing isn't just left over from a previous boot of the host.
Now that the network driver has been switched to using /var/run/libvirt/network for status, we can also modify it to assume that any network with an existing status file is by definition active - we do this when reading the status file. To fine tune the results, networkFindActiveConfigs() is changed to networkFindInactiveConfigs(), and only sets active = 0 if the conditions for particular network types are *not* met.
The result is that during the first run of libvirtd after the host boots, there are no status files, so no networks are active. Any time it is restarted, and network with a status file will be marked as inactive (unless it uses a bridge device and that device for some reason doesn't exist). --- src/conf/network_conf.c | 1 + src/network/bridge_driver.c | 49 +++++++++++++++++++++++++++++++++------------ 2 files changed, 37 insertions(+), 13 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 56c4a09..69ad929 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -3060,6 +3060,7 @@ virNetworkLoadState(virNetworkObjListPtr nets, net->floor_sum = floor_sum_val;
net->taint = taint; + net->active = 1; /* any network with a state file is by definition active */
cleanup: VIR_FREE(configFile); diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 98d10bd..7beab79 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -328,7 +328,7 @@ networkBridgeDummyNicName(const char *brname) }
static void -networkFindActiveConfigs(virNetworkDriverStatePtr driver) +networkFindInactiveConfigs(virNetworkDriverStatePtr driver) { size_t i;
@@ -336,29 +336,51 @@ networkFindActiveConfigs(virNetworkDriverStatePtr driver) virNetworkObjPtr obj = driver->networks.objs[i];
virNetworkObjLock(obj); - - /* If bridge exists, then mark it active */ - if (obj->def->bridge && - virNetDevExists(obj->def->bridge) == 1) { - obj->active = 1; + switch (obj->def->forward.type) { + case VIR_NETWORK_FORWARD_NONE: + case VIR_NETWORK_FORWARD_NAT: + case VIR_NETWORK_FORWARD_ROUTE: + /* If bridge exists, then mark it active */ + if (!(obj->def->bridge && virNetDevExists(obj->def->bridge) == 1)) { + obj->active = 0;
Goes against comment... double negatives in if condition are throwing me off. The switch/case break almost got me too (thinking that the break was to break out of the for loop).
+ break; + }
/* Try and read dnsmasq/radvd pids if any */ if (obj->def->ips && (obj->def->nips > 0)) { char *radvdpidbase;
- ignore_value(virPidFileReadIfAlive(driverState->pidDir, obj->def->name, + ignore_value(virPidFileReadIfAlive(driverState->pidDir, + obj->def->name, &obj->dnsmasqPid, dnsmasqCapsGetBinaryPath(driver->dnsmasqCaps))); - - if (!(radvdpidbase = networkRadvdPidfileBasename(obj->def->name))) - goto cleanup; - ignore_value(virPidFileReadIfAlive(driverState->pidDir, radvdpidbase, + radvdpidbase = networkRadvdPidfileBasename(obj->def->name); + if (!radvdpidbase) + break; + ignore_value(virPidFileReadIfAlive(driverState->pidDir, + radvdpidbase, &obj->radvdPid, RADVD)); VIR_FREE(radvdpidbase);
I'm confused as to why the code would be doing this as the active value isn't changed as a result of this. Was it just "convenient" to do it here? In the context of the old code, if we're marking active, then this code runs. In the context of the new code, for only specific types of network forward types that we've ostensibly already deemed active somewhere (virNetworkLoadState()?) we'll then run this code... and I'm assuming that if we get this far obj->active == 1 - whether that's true or not I'm not 100%, but I assume you'd know. Of course, that could be the problem you're trying to resolve in 5... Furthermore, since the check for inactive is before the load of the config now, are you getting what you expected in the right place here? Previously it'd be load all configs, check active. Now it's check inactive, load all configs. If the above is loaded during that process, then the above is all for naught. ACK pending your thoughts on this above hunk. Might need to see a v2... John
} + break; + + case VIR_NETWORK_FORWARD_BRIDGE: + if (obj->def->bridge) { + if (virNetDevExists(obj->def->bridge) != 1) + obj->active = 0; + break; + } + /* intentionally drop through to common case for all + * macvtap networks (forward='bridge' with no bridge + * device defined is macvtap using its 'bridge' mode) + */ + case VIR_NETWORK_FORWARD_PRIVATE: + case VIR_NETWORK_FORWARD_VEPA: + case VIR_NETWORK_FORWARD_PASSTHROUGH: + case VIR_NETWORK_FORWARD_HOSTDEV: + break; }
- cleanup: virNetworkObjUnlock(obj); }
@@ -542,12 +564,13 @@ networkStateInitialize(bool privileged, driverState->stateDir) < 0) goto error;
+ networkFindInactiveConfigs(driverState); + if (virNetworkLoadAllConfigs(&driverState->networks, driverState->networkConfigDir, driverState->networkAutostartDir) < 0) goto error;
- networkFindActiveConfigs(driverState); networkReloadFirewallRules(driverState); networkRefreshDaemons(driverState);
On 04/15/2014 03:31 PM, John Ferlan wrote:
On 04/10/2014 09:19 AM, Laine Stump wrote:
libvirt attempts to determine at startup time which networks are already active, and set their active flags. Previously it has done this by assuming that all networks are inactive, then setting the flag if the network has a bridge device associated with it, and that bridge device exists. This is not useful for macvtap and hostdev based networks, since they do not use a bridge device.
Of course the reason that such a check had to be done was that the presence of a status file in the network "stateDir" couldn't be trusted as an indicator of whether or not a network was active. This was due to the network driver mistakenly using /var/lib/libvirt/network to store the status files, rather than /var/run/libvirt/network (similar to what is done by every other libirt driver that stores status xml for its objects). The difference is that /var/run is cleared out when the host reboots, so you cvqan be assured that the state file you are seeing isn't just left over from a previous boot of the host.
Now that the network driver has been switched to using /var/run/libvirt/network for status, we can also modify it to assume that any network with an existing status file is by definition active - we do this when reading the status file. To fine tune the results, networkFindActiveConfigs() is changed to networkFindInactiveConfigs(), and only sets active = 0 if the conditions for particular network types are *not* met.
The result is that during the first run of libvirtd after the host boots, there are no status files, so no networks are active. Any time it is restarted, and network with a status file will be marked as inactive (unless it uses a bridge device and that device for some reason doesn't exist). --- src/conf/network_conf.c | 1 + src/network/bridge_driver.c | 49 +++++++++++++++++++++++++++++++++------------ 2 files changed, 37 insertions(+), 13 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 56c4a09..69ad929 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -3060,6 +3060,7 @@ virNetworkLoadState(virNetworkObjListPtr nets, net->floor_sum = floor_sum_val;
net->taint = taint; + net->active = 1; /* any network with a state file is by definition active */
cleanup: VIR_FREE(configFile); diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 98d10bd..7beab79 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -328,7 +328,7 @@ networkBridgeDummyNicName(const char *brname) }
static void -networkFindActiveConfigs(virNetworkDriverStatePtr driver) +networkFindInactiveConfigs(virNetworkDriverStatePtr driver) { size_t i;
@@ -336,29 +336,51 @@ networkFindActiveConfigs(virNetworkDriverStatePtr driver) virNetworkObjPtr obj = driver->networks.objs[i];
virNetworkObjLock(obj); - - /* If bridge exists, then mark it active */ - if (obj->def->bridge && - virNetDevExists(obj->def->bridge) == 1) { - obj->active = 1; + switch (obj->def->forward.type) { + case VIR_NETWORK_FORWARD_NONE: + case VIR_NETWORK_FORWARD_NAT: + case VIR_NETWORK_FORWARD_ROUTE: + /* If bridge exists, then mark it active */ + if (!(obj->def->bridge && virNetDevExists(obj->def->bridge) == 1)) { + obj->active = 0; Goes against comment... double negatives in if condition are throwing me off.
Yeah, the original code assumed active == 0, and set active = 1 if the bridge existed. I forgot to change the comments.
The switch/case break almost got me too (thinking that the break was to break out of the for loop).
+ break; + }
/* Try and read dnsmasq/radvd pids if any */ if (obj->def->ips && (obj->def->nips > 0)) { char *radvdpidbase;
- ignore_value(virPidFileReadIfAlive(driverState->pidDir, obj->def->name, + ignore_value(virPidFileReadIfAlive(driverState->pidDir, + obj->def->name, &obj->dnsmasqPid, dnsmasqCapsGetBinaryPath(driver->dnsmasqCaps))); - - if (!(radvdpidbase = networkRadvdPidfileBasename(obj->def->name))) - goto cleanup; - ignore_value(virPidFileReadIfAlive(driverState->pidDir, radvdpidbase, + radvdpidbase = networkRadvdPidfileBasename(obj->def->name); + if (!radvdpidbase) + break; + ignore_value(virPidFileReadIfAlive(driverState->pidDir, + radvdpidbase, &obj->radvdPid, RADVD)); VIR_FREE(radvdpidbase); I'm confused as to why the code would be doing this as the active value isn't changed as a result of this. Was it just "convenient" to do it here? In the context of the old code, if we're marking active, then this code runs. In the context of the new code, for only specific types of network forward types that we've ostensibly already deemed active somewhere (virNetworkLoadState()?)
Correct. Previously the basic assumption was that all networks were inactive when libvirtd restarted and we would only mark them active if they had a bridge and it existed. Since some network types don't use a bridge, this is now inadequate, and there's no other external info we can use to make this determination, so instead we assume that any network which has a state file (that's why the function now runs *before* loading persistent configs - so the list only contains those with state files) is active, and then we change the polarity of the function checking external info to set a network inactive if the check fails.
we'll then run this code...
This is a case of evolution of the code - setting of the active flag and refreshing the pids from the pidfiles have been together in the same function since "a long time ago", as they both happened during driver initialization, and both required cycling through the list of all network objects. Perhaps it would be less offensive if the function name was changed from networkFindInactiveConfigs() to networkUpdateAllState() (or something like that). The idea is that we've just read the state of all networks on disk, and this function will update the results with any external information from the host, e.g. if a bridge has disappeared then the network must be marked down, and the pids (which are not stored in the state file as that would be redundant information) are reread from the pidfiles. Also, I thought I had seen you wondering somewhere about the fact that the check for pidfiles was previously done for all active networks, but now it is only done for networks with forward mode of none, route, or nat. While the previous code ended up behaving properly, this was accidental - none of the other types of networks use dnsmasq or radvd, so they would never have a pidfile.
and I'm assuming that if we get this far obj->active == 1 - whether that's true or not I'm not 100%, but I assume you'd know. Of course, that could be the problem you're trying to resolve in 5...
Furthermore, since the check for inactive is before the load of the config now, are you getting what you expected in the right place here?
Yes. "config" is the persistent config, which is completely self-contained in the xml file in /etc/libvirt/qemu/network, but the pids are part of the network's state.
Previously it'd be load all configs, check active. Now it's check inactive, load all configs. If the above is loaded during that process, then the above is all for naught.
You mean the pids? Nope, those are unrelated to the config.
ACK pending your thoughts on this above hunk. Might need to see a v2...
John
} + break; + + case VIR_NETWORK_FORWARD_BRIDGE: + if (obj->def->bridge) { + if (virNetDevExists(obj->def->bridge) != 1) + obj->active = 0; + break; + } + /* intentionally drop through to common case for all + * macvtap networks (forward='bridge' with no bridge + * device defined is macvtap using its 'bridge' mode) + */ + case VIR_NETWORK_FORWARD_PRIVATE: + case VIR_NETWORK_FORWARD_VEPA: + case VIR_NETWORK_FORWARD_PASSTHROUGH: + case VIR_NETWORK_FORWARD_HOSTDEV: + break; }
- cleanup: virNetworkObjUnlock(obj); }
@@ -542,12 +564,13 @@ networkStateInitialize(bool privileged, driverState->stateDir) < 0) goto error;
+ networkFindInactiveConfigs(driverState); + if (virNetworkLoadAllConfigs(&driverState->networks, driverState->networkConfigDir, driverState->networkAutostartDir) < 0) goto error;
- networkFindActiveConfigs(driverState); networkReloadFirewallRules(driverState); networkRefreshDaemons(driverState);
On 04/16/2014 06:01 AM, Laine Stump wrote:
On 04/15/2014 03:31 PM, John Ferlan wrote:
On 04/10/2014 09:19 AM, Laine Stump wrote:
libvirt attempts to determine at startup time which networks are already active, and set their active flags. Previously it has done this by assuming that all networks are inactive, then setting the flag if the network has a bridge device associated with it, and that bridge device exists. This is not useful for macvtap and hostdev based networks, since they do not use a bridge device.
Of course the reason that such a check had to be done was that the presence of a status file in the network "stateDir" couldn't be trusted as an indicator of whether or not a network was active. This was due to the network driver mistakenly using /var/lib/libvirt/network to store the status files, rather than /var/run/libvirt/network (similar to what is done by every other libirt driver that stores status xml for its objects). The difference is that /var/run is cleared out when the host reboots, so you cvqan be assured that the state file you are seeing isn't just left over from a previous boot of the host.
Now that the network driver has been switched to using /var/run/libvirt/network for status, we can also modify it to assume that any network with an existing status file is by definition active - we do this when reading the status file. To fine tune the results, networkFindActiveConfigs() is changed to networkFindInactiveConfigs(), and only sets active = 0 if the conditions for particular network types are *not* met.
The result is that during the first run of libvirtd after the host boots, there are no status files, so no networks are active. Any time it is restarted, and network with a status file will be marked as inactive (unless it uses a bridge device and that device for some reason doesn't exist). --- src/conf/network_conf.c | 1 + src/network/bridge_driver.c | 49 +++++++++++++++++++++++++++++++++------------ 2 files changed, 37 insertions(+), 13 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 56c4a09..69ad929 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -3060,6 +3060,7 @@ virNetworkLoadState(virNetworkObjListPtr nets, net->floor_sum = floor_sum_val;
net->taint = taint; + net->active = 1; /* any network with a state file is by definition active */
cleanup: VIR_FREE(configFile); diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 98d10bd..7beab79 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -328,7 +328,7 @@ networkBridgeDummyNicName(const char *brname) }
static void -networkFindActiveConfigs(virNetworkDriverStatePtr driver) +networkFindInactiveConfigs(virNetworkDriverStatePtr driver) { size_t i;
@@ -336,29 +336,51 @@ networkFindActiveConfigs(virNetworkDriverStatePtr driver) virNetworkObjPtr obj = driver->networks.objs[i];
virNetworkObjLock(obj); - - /* If bridge exists, then mark it active */ - if (obj->def->bridge && - virNetDevExists(obj->def->bridge) == 1) { - obj->active = 1; + switch (obj->def->forward.type) { + case VIR_NETWORK_FORWARD_NONE: + case VIR_NETWORK_FORWARD_NAT: + case VIR_NETWORK_FORWARD_ROUTE: + /* If bridge exists, then mark it active */ + if (!(obj->def->bridge && virNetDevExists(obj->def->bridge) == 1)) { + obj->active = 0; Goes against comment... double negatives in if condition are throwing me off.
Yeah, the original code assumed active == 0, and set active = 1 if the bridge existed. I forgot to change the comments.
The switch/case break almost got me too (thinking that the break was to break out of the for loop).
+ break; + }
/* Try and read dnsmasq/radvd pids if any */ if (obj->def->ips && (obj->def->nips > 0)) { char *radvdpidbase;
- ignore_value(virPidFileReadIfAlive(driverState->pidDir, obj->def->name, + ignore_value(virPidFileReadIfAlive(driverState->pidDir, + obj->def->name, &obj->dnsmasqPid, dnsmasqCapsGetBinaryPath(driver->dnsmasqCaps))); - - if (!(radvdpidbase = networkRadvdPidfileBasename(obj->def->name))) - goto cleanup; - ignore_value(virPidFileReadIfAlive(driverState->pidDir, radvdpidbase, + radvdpidbase = networkRadvdPidfileBasename(obj->def->name); + if (!radvdpidbase) + break; + ignore_value(virPidFileReadIfAlive(driverState->pidDir, + radvdpidbase, &obj->radvdPid, RADVD)); VIR_FREE(radvdpidbase); I'm confused as to why the code would be doing this as the active value isn't changed as a result of this. Was it just "convenient" to do it here? In the context of the old code, if we're marking active, then this code runs. In the context of the new code, for only specific types of network forward types that we've ostensibly already deemed active somewhere (virNetworkLoadState()?)
Your call on that - although the virNetworkLoadState (Load, Check, Change, Adjust, Update) seems more appropriate since the code is not just adjusting active/inactive value. As an aside, seems as though active could be bool rather than int, but that's a different rabbit hole.
Correct. Previously the basic assumption was that all networks were inactive when libvirtd restarted and we would only mark them active if they had a bridge and it existed. Since some network types don't use a bridge, this is now inadequate, and there's no other external info we can use to make this determination, so instead we assume that any network which has a state file (that's why the function now runs *before* loading persistent configs - so the list only contains those with state files) is active, and then we change the polarity of the function checking external info to set a network inactive if the check fails.
we'll then run this code...
This is a case of evolution of the code - setting of the active flag and refreshing the pids from the pidfiles have been together in the same function since "a long time ago", as they both happened during driver initialization, and both required cycling through the list of all network objects. Perhaps it would be less offensive if the function name was changed from networkFindInactiveConfigs() to networkUpdateAllState() (or something like that). The idea is that we've just read the state of all networks on disk, and this function will update the results with any external information from the host, e.g. if a bridge has disappeared then the network must be marked down, and the pids (which are not stored in the state file as that would be redundant information) are reread from the pidfiles.
Also, I thought I had seen you wondering somewhere about the fact that the check for pidfiles was previously done for all active networks, but now it is only done for networks with forward mode of none, route, or nat. While the previous code ended up behaving properly, this was accidental - none of the other types of networks use dnsmasq or radvd, so they would never have a pidfile.
Ah - ok... Some things are less obvious when you're reading/reviewing code rather than having it firmly ingrained in your brain as a given...
and I'm assuming that if we get this far obj->active == 1 - whether that's true or not I'm not 100%, but I assume you'd know. Of course, that could be the problem you're trying to resolve in 5...
Furthermore, since the check for inactive is before the load of the config now, are you getting what you expected in the right place here?
Yes. "config" is the persistent config, which is completely self-contained in the xml file in /etc/libvirt/qemu/network, but the pids are part of the network's state.
Previously it'd be load all configs, check active. Now it's check inactive, load all configs. If the above is loaded during that process, then the above is all for naught.
You mean the pids? Nope, those are unrelated to the config.
This was more an "ordering comment" especially as it related to the call from networkStateInitialize(). I've seen cases where code like this was changed and other code moved where in the call stack a call such as this was made and then result was unexpected behaviour. At the very least as long as the comments are updated - that's fine. Makes it easier for the next person reading it. John
ACK pending your thoughts on this above hunk. Might need to see a v2...
John
} + break; + + case VIR_NETWORK_FORWARD_BRIDGE: + if (obj->def->bridge) { + if (virNetDevExists(obj->def->bridge) != 1) + obj->active = 0; + break; + } + /* intentionally drop through to common case for all + * macvtap networks (forward='bridge' with no bridge + * device defined is macvtap using its 'bridge' mode) + */ + case VIR_NETWORK_FORWARD_PRIVATE: + case VIR_NETWORK_FORWARD_VEPA: + case VIR_NETWORK_FORWARD_PASSTHROUGH: + case VIR_NETWORK_FORWARD_HOSTDEV: + break; }
- cleanup: virNetworkObjUnlock(obj); }
@@ -542,12 +564,13 @@ networkStateInitialize(bool privileged, driverState->stateDir) < 0) goto error;
+ networkFindInactiveConfigs(driverState); + if (virNetworkLoadAllConfigs(&driverState->networks, driverState->networkConfigDir, driverState->networkAutostartDir) < 0) goto error;
- networkFindActiveConfigs(driverState); networkReloadFirewallRules(driverState); networkRefreshDaemons(driverState);
The check for a network being active during interface attach was being done individually in several places (by both the lxc driver and the qemu driver), but those places were too specific, leading to it *not* being checked when allocating a connection/device from a macvtap or hostdev network. This patch puts a single check in networkAllocateActualDevice(), which is always called before the any network interface is attached to any type of domain. It also removes all the other now-redundant checks from the lxc and qemu drivers. [Note that prior to the previous 4 patches in this series, it would have been very cumbersome to apply this current patch, as macvtap and hostdev networks would be automatically set inactive at each libvirtd restart (unless they were marked as "autostart"). Therefore, those 4 patches should be seen as prerequisites to this patch for any backporting. This comment is a placeholder that I intend to replace with the git commit id's for those 4 patches as soon as I have pushed them.] This fixes: https://bugzilla.redhat.com/show_bug.cgi?id=880483 --- src/lxc/lxc_driver.c | 21 +++------------------ src/lxc/lxc_process.c | 18 ++---------------- src/network/bridge_driver.c | 10 +++++++++- src/qemu/qemu_command.c | 18 ++---------------- src/qemu/qemu_hotplug.c | 11 +---------- 5 files changed, 17 insertions(+), 61 deletions(-) diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 942e139..f84b8c3 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -4155,27 +4155,12 @@ lxcDomainAttachDeviceNetLive(virConnectPtr conn, virNetworkPtr network; char *brname = NULL; bool fail = false; - int active; virErrorPtr errobj; - if (!(network = virNetworkLookupByName(conn, - net->data.network.name))) + if (!(network = virNetworkLookupByName(conn, net->data.network.name))) goto cleanup; - - active = virNetworkIsActive(network); - if (active != 1) { - fail = true; - if (active == 0) - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Network '%s' is not active."), - net->data.network.name); - } - - if (!fail) { - brname = virNetworkGetBridgeName(network); - if (brname == NULL) - fail = true; - } + if (!(brname = virNetworkGetBridgeName(network))) + fail = true; /* Make sure any above failure is preserved */ errobj = virSaveLastError(); diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index 091102b..0aef13a 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -387,27 +387,13 @@ static int virLXCProcessSetupInterfaces(virConnectPtr conn, virNetworkPtr network; char *brname = NULL; bool fail = false; - int active; virErrorPtr errobj; if (!(network = virNetworkLookupByName(conn, def->nets[i]->data.network.name))) goto cleanup; - - active = virNetworkIsActive(network); - if (active != 1) { - fail = true; - if (active == 0) - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Network '%s' is not active."), - def->nets[i]->data.network.name); - } - - if (!fail) { - brname = virNetworkGetBridgeName(network); - if (brname == NULL) - fail = true; - } + if (!(brname = virNetworkGetBridgeName(network))) + fail = true; /* Make sure any above failure is preserved */ errobj = virSaveLastError(); diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 7beab79..132af4f 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -3118,7 +3118,8 @@ static int networkDestroy(virNetworkPtr net) if (!virNetworkObjIsActive(network)) { virReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("network is not active")); + _("network '%s' is not active"), + network->def->name); goto cleanup; } @@ -3462,6 +3463,13 @@ networkAllocateActualDevice(virDomainDefPtr dom, } netdef = network->def; + if (!virNetworkObjIsActive(network)) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("network '%s' is not active"), + netdef->name); + goto error; + } + if (VIR_ALLOC(iface->data.network.actual) < 0) goto error; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 379c094..baf1ad1 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -297,7 +297,6 @@ qemuNetworkIfaceConnect(virDomainDefPtr def, virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK) { - int active; bool fail = false; virErrorPtr errobj; virNetworkPtr network = virNetworkLookupByName(conn, @@ -305,21 +304,8 @@ qemuNetworkIfaceConnect(virDomainDefPtr def, if (!network) return ret; - active = virNetworkIsActive(network); - if (active != 1) { - fail = true; - - if (active == 0) - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Network '%s' is not active."), - net->data.network.name); - } - - if (!fail) { - brname = virNetworkGetBridgeName(network); - if (brname == NULL) - fail = true; - } + if (!(brname = virNetworkGetBridgeName(network))) + fail = true; /* Make sure any above failure is preserved */ errobj = virSaveLastError(); diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 233b183..ccfb358 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1710,7 +1710,6 @@ qemuDomainNetGetBridgeName(virConnectPtr conn, virDomainNetDefPtr net) if (VIR_STRDUP(brname, tmpbr) < 0) goto cleanup; } else if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK) { - int active; virErrorPtr errobj; virNetworkPtr network; @@ -1720,15 +1719,7 @@ qemuDomainNetGetBridgeName(virConnectPtr conn, virDomainNetDefPtr net) net->data.network.name); goto cleanup; } - - active = virNetworkIsActive(network); - if (active == 1) { - brname = virNetworkGetBridgeName(network); - } else if (active == 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Network '%s' is not active."), - net->data.network.name); - } + brname = virNetworkGetBridgeName(network); /* Make sure any above failure is preserved */ errobj = virSaveLastError(); -- 1.9.0
On 04/10/2014 09:19 AM, Laine Stump wrote:
The check for a network being active during interface attach was being done individually in several places (by both the lxc driver and the qemu driver), but those places were too specific, leading to it *not* being checked when allocating a connection/device from a macvtap or hostdev network.
This patch puts a single check in networkAllocateActualDevice(), which is always called before the any network interface is attached to any type of domain. It also removes all the other now-redundant checks from the lxc and qemu drivers.
[Note that prior to the previous 4 patches in this series, it would have been very cumbersome to apply this current patch, as macvtap and hostdev networks would be automatically set inactive at each libvirtd restart (unless they were marked as "autostart"). Therefore, those 4 patches should be seen as prerequisites to this patch for any backporting. This comment is a placeholder that I intend to replace with the git commit id's for those 4 patches as soon as I have pushed them.]
This fixes:
https://bugzilla.redhat.com/show_bug.cgi?id=880483 --- src/lxc/lxc_driver.c | 21 +++------------------ src/lxc/lxc_process.c | 18 ++---------------- src/network/bridge_driver.c | 10 +++++++++- src/qemu/qemu_command.c | 18 ++---------------- src/qemu/qemu_hotplug.c | 11 +---------- 5 files changed, 17 insertions(+), 61 deletions(-)
This appears to be fine with the caveat mentioned/noted. I'm also assuming that by moving the check to higher in the call/check stack that it's not "too generic" not right? Other 'actualType''s would need that object to be active since networkAllocateActualDevice() will fail otherwise. ACK for what's here as long as you're comfortable with the active check for all actualType's now... John
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 942e139..f84b8c3 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -4155,27 +4155,12 @@ lxcDomainAttachDeviceNetLive(virConnectPtr conn, virNetworkPtr network; char *brname = NULL; bool fail = false; - int active; virErrorPtr errobj;
- if (!(network = virNetworkLookupByName(conn, - net->data.network.name))) + if (!(network = virNetworkLookupByName(conn, net->data.network.name))) goto cleanup; - - active = virNetworkIsActive(network); - if (active != 1) { - fail = true; - if (active == 0) - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Network '%s' is not active."), - net->data.network.name); - } - - if (!fail) { - brname = virNetworkGetBridgeName(network); - if (brname == NULL) - fail = true; - } + if (!(brname = virNetworkGetBridgeName(network))) + fail = true;
/* Make sure any above failure is preserved */ errobj = virSaveLastError(); diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index 091102b..0aef13a 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -387,27 +387,13 @@ static int virLXCProcessSetupInterfaces(virConnectPtr conn, virNetworkPtr network; char *brname = NULL; bool fail = false; - int active; virErrorPtr errobj;
if (!(network = virNetworkLookupByName(conn, def->nets[i]->data.network.name))) goto cleanup; - - active = virNetworkIsActive(network); - if (active != 1) { - fail = true; - if (active == 0) - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Network '%s' is not active."), - def->nets[i]->data.network.name); - } - - if (!fail) { - brname = virNetworkGetBridgeName(network); - if (brname == NULL) - fail = true; - } + if (!(brname = virNetworkGetBridgeName(network))) + fail = true;
/* Make sure any above failure is preserved */ errobj = virSaveLastError(); diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 7beab79..132af4f 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -3118,7 +3118,8 @@ static int networkDestroy(virNetworkPtr net)
if (!virNetworkObjIsActive(network)) { virReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("network is not active")); + _("network '%s' is not active"), + network->def->name); goto cleanup; }
@@ -3462,6 +3463,13 @@ networkAllocateActualDevice(virDomainDefPtr dom, } netdef = network->def;
+ if (!virNetworkObjIsActive(network)) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("network '%s' is not active"), + netdef->name); + goto error; + } + if (VIR_ALLOC(iface->data.network.actual) < 0) goto error;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 379c094..baf1ad1 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -297,7 +297,6 @@ qemuNetworkIfaceConnect(virDomainDefPtr def, virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK) { - int active; bool fail = false; virErrorPtr errobj; virNetworkPtr network = virNetworkLookupByName(conn, @@ -305,21 +304,8 @@ qemuNetworkIfaceConnect(virDomainDefPtr def, if (!network) return ret;
- active = virNetworkIsActive(network); - if (active != 1) { - fail = true; - - if (active == 0) - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Network '%s' is not active."), - net->data.network.name); - } - - if (!fail) { - brname = virNetworkGetBridgeName(network); - if (brname == NULL) - fail = true; - } + if (!(brname = virNetworkGetBridgeName(network))) + fail = true;
/* Make sure any above failure is preserved */ errobj = virSaveLastError(); diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 233b183..ccfb358 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1710,7 +1710,6 @@ qemuDomainNetGetBridgeName(virConnectPtr conn, virDomainNetDefPtr net) if (VIR_STRDUP(brname, tmpbr) < 0) goto cleanup; } else if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK) { - int active; virErrorPtr errobj; virNetworkPtr network;
@@ -1720,15 +1719,7 @@ qemuDomainNetGetBridgeName(virConnectPtr conn, virDomainNetDefPtr net) net->data.network.name); goto cleanup; } - - active = virNetworkIsActive(network); - if (active == 1) { - brname = virNetworkGetBridgeName(network); - } else if (active == 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Network '%s' is not active."), - net->data.network.name); - } + brname = virNetworkGetBridgeName(network);
/* Make sure any above failure is preserved */ errobj = virSaveLastError();
On 04/15/2014 03:31 PM, John Ferlan wrote:
On 04/10/2014 09:19 AM, Laine Stump wrote:
The check for a network being active during interface attach was being done individually in several places (by both the lxc driver and the qemu driver), but those places were too specific, leading to it *not* being checked when allocating a connection/device from a macvtap or hostdev network.
This patch puts a single check in networkAllocateActualDevice(), which is always called before the any network interface is attached to any type of domain. It also removes all the other now-redundant checks from the lxc and qemu drivers.
[Note that prior to the previous 4 patches in this series, it would have been very cumbersome to apply this current patch, as macvtap and hostdev networks would be automatically set inactive at each libvirtd restart (unless they were marked as "autostart"). Therefore, those 4 patches should be seen as prerequisites to this patch for any backporting. This comment is a placeholder that I intend to replace with the git commit id's for those 4 patches as soon as I have pushed them.]
This fixes:
https://bugzilla.redhat.com/show_bug.cgi?id=880483 --- src/lxc/lxc_driver.c | 21 +++------------------ src/lxc/lxc_process.c | 18 ++---------------- src/network/bridge_driver.c | 10 +++++++++- src/qemu/qemu_command.c | 18 ++---------------- src/qemu/qemu_hotplug.c | 11 +---------- 5 files changed, 17 insertions(+), 61 deletions(-)
This appears to be fine with the caveat mentioned/noted. I'm also assuming that by moving the check to higher in the call/check stack that it's not "too generic" not right? Other 'actualType''s would need that object to be active since networkAllocateActualDevice() will fail otherwise.
Yes, the behavior change is intentional, and checking for "actualType" will always be okay at any point past this since, as you imply, if networkAllocateActualDevice() had failed, we wouldn't even get to any of the later checks of actualType.
ACK for what's here as long as you're comfortable with the active check for all actualType's now...
John
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 942e139..f84b8c3 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -4155,27 +4155,12 @@ lxcDomainAttachDeviceNetLive(virConnectPtr conn, virNetworkPtr network; char *brname = NULL; bool fail = false; - int active; virErrorPtr errobj;
- if (!(network = virNetworkLookupByName(conn, - net->data.network.name))) + if (!(network = virNetworkLookupByName(conn, net->data.network.name))) goto cleanup; - - active = virNetworkIsActive(network); - if (active != 1) { - fail = true; - if (active == 0) - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Network '%s' is not active."), - net->data.network.name); - } - - if (!fail) { - brname = virNetworkGetBridgeName(network); - if (brname == NULL) - fail = true; - } + if (!(brname = virNetworkGetBridgeName(network))) + fail = true;
/* Make sure any above failure is preserved */ errobj = virSaveLastError(); diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index 091102b..0aef13a 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -387,27 +387,13 @@ static int virLXCProcessSetupInterfaces(virConnectPtr conn, virNetworkPtr network; char *brname = NULL; bool fail = false; - int active; virErrorPtr errobj;
if (!(network = virNetworkLookupByName(conn, def->nets[i]->data.network.name))) goto cleanup; - - active = virNetworkIsActive(network); - if (active != 1) { - fail = true; - if (active == 0) - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Network '%s' is not active."), - def->nets[i]->data.network.name); - } - - if (!fail) { - brname = virNetworkGetBridgeName(network); - if (brname == NULL) - fail = true; - } + if (!(brname = virNetworkGetBridgeName(network))) + fail = true;
/* Make sure any above failure is preserved */ errobj = virSaveLastError(); diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 7beab79..132af4f 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -3118,7 +3118,8 @@ static int networkDestroy(virNetworkPtr net)
if (!virNetworkObjIsActive(network)) { virReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("network is not active")); + _("network '%s' is not active"), + network->def->name); goto cleanup; }
@@ -3462,6 +3463,13 @@ networkAllocateActualDevice(virDomainDefPtr dom, } netdef = network->def;
+ if (!virNetworkObjIsActive(network)) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("network '%s' is not active"), + netdef->name); + goto error; + } + if (VIR_ALLOC(iface->data.network.actual) < 0) goto error;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 379c094..baf1ad1 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -297,7 +297,6 @@ qemuNetworkIfaceConnect(virDomainDefPtr def, virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK) { - int active; bool fail = false; virErrorPtr errobj; virNetworkPtr network = virNetworkLookupByName(conn, @@ -305,21 +304,8 @@ qemuNetworkIfaceConnect(virDomainDefPtr def, if (!network) return ret;
- active = virNetworkIsActive(network); - if (active != 1) { - fail = true; - - if (active == 0) - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Network '%s' is not active."), - net->data.network.name); - } - - if (!fail) { - brname = virNetworkGetBridgeName(network); - if (brname == NULL) - fail = true; - } + if (!(brname = virNetworkGetBridgeName(network))) + fail = true;
/* Make sure any above failure is preserved */ errobj = virSaveLastError(); diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 233b183..ccfb358 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1710,7 +1710,6 @@ qemuDomainNetGetBridgeName(virConnectPtr conn, virDomainNetDefPtr net) if (VIR_STRDUP(brname, tmpbr) < 0) goto cleanup; } else if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK) { - int active; virErrorPtr errobj; virNetworkPtr network;
@@ -1720,15 +1719,7 @@ qemuDomainNetGetBridgeName(virConnectPtr conn, virDomainNetDefPtr net) net->data.network.name); goto cleanup; } - - active = virNetworkIsActive(network); - if (active == 1) { - brname = virNetworkGetBridgeName(network); - } else if (active == 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Network '%s' is not active."), - net->data.network.name); - } + brname = virNetworkGetBridgeName(network);
/* Make sure any above failure is preserved */ errobj = virSaveLastError();
participants (3)
-
Daniel P. Berrange -
John Ferlan -
Laine Stump