[libvirt] [PATCH 3/3] network: delay global firewall setup if no networks are running
Jim Fehlig
jfehlig at suse.com
Wed May 22 21:59:00 UTC 2019
On 5/22/19 6:29 AM, Daniel P. Berrangé wrote:
> Creating firewall rules for the virtual networks causes the kernel to
> load the conntrack module. This imposes a significant performance
> penalty on Linux network traffic. Thus we want to only take that hit if
> we actually have virtual networks running.
>
> We need to create global firewall rules during startup in order to
> "upgrade" rules for any running networks created by older libvirt.
> If no running networks are present though, we can safely delay setup
> until the time we actually start a network.
>
> Signed-off-by: Daniel P. Berrangé <berrange at redhat.com>
> ---
> src/network/bridge_driver.c | 2 +-
> src/network/bridge_driver_linux.c | 56 ++++++++++++++++++++++---
> src/network/bridge_driver_nop.c | 3 +-
> src/network/bridge_driver_platform.h | 2 +-
> tests/networkxml2firewalldata/base.args | 34 +++++++++++++++
syntax-check is complaining about the line wrapping of this file.
> tests/networkxml2firewalltest.c | 36 +++++++++++++---
> 6 files changed, 119 insertions(+), 14 deletions(-)
> create mode 100644 tests/networkxml2firewalldata/base.args
>
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 0e9bb78c32..c0c026e242 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -2122,7 +2122,7 @@ networkReloadFirewallRules(virNetworkDriverStatePtr driver, bool startup)
> * but until we untangle the virt driver that's not viable */
> if (!driver->privileged)
> return;
> - networkPreReloadFirewallRules(startup);
> + networkPreReloadFirewallRules(driver, startup);
> virNetworkObjListForEach(driver->networks,
> networkReloadFirewallRulesHelper,
> NULL);
> diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c
> index 2b29363f3c..fdd5d7066e 100644
> --- a/src/network/bridge_driver_linux.c
> +++ b/src/network/bridge_driver_linux.c
> @@ -84,16 +84,57 @@ static void networkSetupPrivateChains(void)
> }
> }
>
> -void networkPreReloadFirewallRules(bool startup)
> +
> +static int
> +networkHasRunningNetworksHelper(virNetworkObjPtr obj,
> + void *opaque)
> {
> - /* We create global rules upfront as we don't want
> - * the perf hit of conditionally figuring out whether
> - * to create them each time a network is started.
> + bool *running = opaque;
> +
> + virObjectLock(obj);
> + if (virNetworkObjIsActive(obj))
> + *running = true;
> + virObjectUnlock(obj);
> +
> + return 0;
> +}
> +
> +
> +static bool
> +networkHasRunningNetworks(virNetworkDriverStatePtr driver)
> +{
> + bool running = false;
> + virNetworkObjListForEach(driver->networks,
> + networkHasRunningNetworksHelper,
> + &running);
> + return running;
> +}
> +
> +
> +void networkPreReloadFirewallRules(virNetworkDriverStatePtr driver, bool startup)
> +{
> + /*
> + * If there are any running networks, we need to
> + * create the global rules upfront. This allows us
> + * convert rules created by old libvirt into the new
> + * format.
> + *
> + * If there are not any running networks, then we
> + * must not create rules, because the rules will
> + * cause the contrack kernel module to be loaded.
s/contrack/conntrack/
> + * This imposes a significant performance hit on
> + * the networking stack. Thus we will only create
> + * rules if a network is later startup.
> *
> * Any errors here are saved to be reported at time
> * of starting the network though as that makes them
> * more likely to be seen by a human
> */
> + if (!networkHasRunningNetworks(driver)) {
> + VIR_DEBUG("Delayed global rule setup as no networks are running");
> + return;
> + }
> +
> ignore_value(virOnce(&createdOnce, networkSetupPrivateChains));
>
> /*
> @@ -109,8 +150,10 @@ void networkPreReloadFirewallRules(bool startup)
> * rules will be present. Thus we can safely just tell it
> * to always delete from the builin chain
> */
> - if (startup && createdChains)
> + if (startup && createdChains) {
> + VIR_DEBUG("Requesting cleanup of legacy firewall rules");
Perhaps intended for the previous patch?
I tested all the restart scenarios I could think of, including updating from
"older" libvirt without private chains (with and without running networks).
Looks good!
Reviewed-by: Jim Fehlig <jfehlig at suse.com>
Regards,
Jim
> iptablesSetDeletePrivate(false);
> + }
> }
>
>
> @@ -724,6 +767,9 @@ int networkAddFirewallRules(virNetworkDefPtr def)
> virFirewallPtr fw = NULL;
> int ret = -1;
>
> + if (virOnce(&createdOnce, networkSetupPrivateChains) < 0)
> + return -1;
> +
> if (errInitV4 &&
> (virNetworkDefGetIPByIndex(def, AF_INET, 0) ||
> virNetworkDefGetRouteByIndex(def, AF_INET, 0))) {
> diff --git a/src/network/bridge_driver_nop.c b/src/network/bridge_driver_nop.c
> index ea9db338cb..ec7b1ed8b7 100644
> --- a/src/network/bridge_driver_nop.c
> +++ b/src/network/bridge_driver_nop.c
> @@ -19,7 +19,8 @@
>
> #include <config.h>
>
> -void networkPreReloadFirewallRules(bool startup ATTRIBUTE_UNUSED)
> +void networkPreReloadFirewallRules(virNetworkDriverStatePtr driver ATTRIBUTE_UNUSED,
> + bool startup ATTRIBUTE_UNUSED)
> {
> }
>
> diff --git a/src/network/bridge_driver_platform.h b/src/network/bridge_driver_platform.h
> index 95fd64bdc7..5f534fc132 100644
> --- a/src/network/bridge_driver_platform.h
> +++ b/src/network/bridge_driver_platform.h
> @@ -58,7 +58,7 @@ struct _virNetworkDriverState {
> typedef struct _virNetworkDriverState virNetworkDriverState;
> typedef virNetworkDriverState *virNetworkDriverStatePtr;
>
> -void networkPreReloadFirewallRules(bool startup);
> +void networkPreReloadFirewallRules(virNetworkDriverStatePtr driver, bool startup);
> void networkPostReloadFirewallRules(bool startup);
>
> int networkCheckRouteCollision(virNetworkDefPtr def);
> diff --git a/tests/networkxml2firewalldata/base.args b/tests/networkxml2firewalldata/base.args
> new file mode 100644
> index 0000000000..f76ce87b50
> --- /dev/null
> +++ b/tests/networkxml2firewalldata/base.args
> @@ -0,0 +1,34 @@
> +iptables --table filter --list-rules
> +iptables --table nat --list-rules
> +iptables --table mangle --list-rules
> +iptables --table filter --new-chain LIBVIRT_INP
> +iptables --table filter --insert INPUT --jump LIBVIRT_INP
> +iptables --table filter --new-chain LIBVIRT_OUT
> +iptables --table filter --insert OUTPUT --jump LIBVIRT_OUT
> +iptables --table filter --new-chain LIBVIRT_FWO
> +iptables --table filter --insert FORWARD --jump LIBVIRT_FWO
> +iptables --table filter --new-chain LIBVIRT_FWI
> +iptables --table filter --insert FORWARD --jump LIBVIRT_FWI
> +iptables --table filter --new-chain LIBVIRT_FWX
> +iptables --table filter --insert FORWARD --jump LIBVIRT_FWX
> +iptables --table nat --new-chain LIBVIRT_PRT
> +iptables --table nat --insert POSTROUTING --jump LIBVIRT_PRT
> +iptables --table mangle --new-chain LIBVIRT_PRT
> +iptables --table mangle --insert POSTROUTING --jump LIBVIRT_PRT
> +ip6tables --table filter --list-rules
> +ip6tables --table nat --list-rules
> +ip6tables --table mangle --list-rules
> +ip6tables --table filter --new-chain LIBVIRT_INP
> +ip6tables --table filter --insert INPUT --jump LIBVIRT_INP
> +ip6tables --table filter --new-chain LIBVIRT_OUT
> +ip6tables --table filter --insert OUTPUT --jump LIBVIRT_OUT
> +ip6tables --table filter --new-chain LIBVIRT_FWO
> +ip6tables --table filter --insert FORWARD --jump LIBVIRT_FWO
> +ip6tables --table filter --new-chain LIBVIRT_FWI
> +ip6tables --table filter --insert FORWARD --jump LIBVIRT_FWI
> +ip6tables --table filter --new-chain LIBVIRT_FWX
> +ip6tables --table filter --insert FORWARD --jump LIBVIRT_FWX
> +ip6tables --table nat --new-chain LIBVIRT_PRT
> +ip6tables --table nat --insert POSTROUTING --jump LIBVIRT_PRT
> +ip6tables --table mangle --new-chain LIBVIRT_PRT
> +ip6tables --table mangle --insert POSTROUTING --jump LIBVIRT_PRT
> diff --git a/tests/networkxml2firewalltest.c b/tests/networkxml2firewalltest.c
> index 575b68379a..c25282ebb1 100644
> --- a/tests/networkxml2firewalltest.c
> +++ b/tests/networkxml2firewalltest.c
> @@ -22,6 +22,7 @@
> #include <config.h>
>
> #include "testutils.h"
> +#include "viralloc.h"
>
> #if defined (__linux__)
>
> @@ -57,13 +58,15 @@ testCommandDryRun(const char *const*args ATTRIBUTE_UNUSED,
> }
>
> static int testCompareXMLToArgvFiles(const char *xml,
> - const char *cmdline)
> + const char *cmdline,
> + const char *baseargs)
> {
> char *expectargv = NULL;
> char *actualargv = NULL;
> virBuffer buf = VIR_BUFFER_INITIALIZER;
> virNetworkDefPtr def = NULL;
> int ret = -1;
> + char *actual;
>
> virCommandSetDryRun(&buf, testCommandDryRun, NULL);
>
> @@ -76,11 +79,18 @@ static int testCompareXMLToArgvFiles(const char *xml,
> if (virBufferError(&buf))
> goto cleanup;
>
> - actualargv = virBufferContentAndReset(&buf);
> + actual = actualargv = virBufferContentAndReset(&buf);
> virTestClearCommandPath(actualargv);
> virCommandSetDryRun(NULL, NULL, NULL);
>
> - if (virTestCompareToFile(actualargv, cmdline) < 0)
> + /* The first network to be created populates the
> + * libvirt global chains. We must skip args for
> + * that if present
> + */
> + if (STRPREFIX(actual, baseargs))
> + actual += strlen(baseargs);
> +
> + if (virTestCompareToFile(actual, cmdline) < 0)
> goto cleanup;
>
> ret = 0;
> @@ -95,6 +105,7 @@ static int testCompareXMLToArgvFiles(const char *xml,
>
> struct testInfo {
> const char *name;
> + const char *baseargs;
> };
>
>
> @@ -112,7 +123,7 @@ testCompareXMLToIPTablesHelper(const void *data)
> abs_srcdir, info->name, RULESTYPE) < 0)
> goto cleanup;
>
> - result = testCompareXMLToArgvFiles(xml, args);
> + result = testCompareXMLToArgvFiles(xml, args, info->baseargs);
>
> cleanup:
> VIR_FREE(xml);
> @@ -133,11 +144,13 @@ static int
> mymain(void)
> {
> int ret = 0;
> + VIR_AUTOFREE(char *)basefile = NULL;
> + VIR_AUTOFREE(char *)baseargs = NULL;
>
> # define DO_TEST(name) \
> do { \
> - static struct testInfo info = { \
> - name, \
> + struct testInfo info = { \
> + name, baseargs, \
> }; \
> if (virTestRun("Network XML-2-iptables " name, \
> testCompareXMLToIPTablesHelper, &info) < 0) \
> @@ -156,6 +169,17 @@ mymain(void)
> goto cleanup;
> }
>
> + if (virAsprintf(&basefile, "%s/networkxml2firewalldata/base.args",
> + abs_srcdir) < 0) {
> + ret = -1;
> + goto cleanup;
> + }
> +
> + if (virTestLoadFile(basefile, &baseargs) < 0) {
> + ret = -1;
> + goto cleanup;
> + }
> +
> DO_TEST("nat-default");
> DO_TEST("nat-tftp");
> DO_TEST("nat-many-ips");
>
More information about the libvir-list
mailing list