On 5/7/19 4:36 AM, Daniel P. Berrangé wrote:
On Tue, Apr 30, 2019 at 02:34:44PM -0600, Jim Fehlig wrote:
Automated performance tests found that network-centric workloads suffered
a 20 percent decrease when the host libvirt was updated from 5.0.0 to
5.1.0. On the test hosts libvirtd is enabled to start at boot and the
"default" network is defined, but it is not set to autostart.
libvirt 5.1.0 introduced private firewall chains with commit 5f1e6a7d.
The chains are created at libvirtd start, which triggers loading the
conntrack kernel module. Subsequent tracking and processing of
connections resulted in the performance hit. Prior to commit 5f1e6a7d
the conntrack module would not be loaded until starting a network,
when libvirt added rules to the builtin chains.
Restore the behavior of previous libvirt versions by delaying
the creation of private chains until the first network is started.
Signed-off-by: Jim Fehlig <jfehlig@xxxxxxxx>
---
I briefly discussed this issue with Daniel on IRC and just now finding
time to bring it to the list for broader discussion. The adjustment to
the test file feels hacky. The whole approach might by hacky, hence the
RFC.
The test file hackyness is something we had before, so not a big
deal imho.
src/network/bridge_driver_linux.c | 64 +++-------
.../nat-default-linux.args | 116 ++++++++++++++++++
2 files changed, 131 insertions(+), 49 deletions(-)
diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c
index f2827543ca..a3a09caeba 100644
--- a/src/network/bridge_driver_linux.c
+++ b/src/network/bridge_driver_linux.c
@@ -35,44 +35,10 @@ VIR_LOG_INIT("network.bridge_driver_linux");
#define PROC_NET_ROUTE "/proc/net/route"
-static virErrorPtr errInitV4;
-static virErrorPtr errInitV6;
+static bool pvtChainsCreated;
void networkPreReloadFirewallRules(bool startup)
{
- bool created = false;
- int rc;
-
- /* 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.
- *
- * 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
- */
- rc = iptablesSetupPrivateChains(VIR_FIREWALL_LAYER_IPV4);
- if (rc < 0) {
- errInitV4 = virSaveLastError();
- virResetLastError();
- } else {
- virFreeError(errInitV4);
- errInitV4 = NULL;
- }
- if (rc)
- created = true;
-
- rc = iptablesSetupPrivateChains(VIR_FIREWALL_LAYER_IPV6);
- if (rc < 0) {
- errInitV6 = virSaveLastError();
- virResetLastError();
- } else {
- virFreeError(errInitV6);
- errInitV6 = NULL;
- }
- if (rc)
- created = true;
-
/*
* If this is initial startup, and we just created the
* top level private chains we either
@@ -86,8 +52,8 @@ void networkPreReloadFirewallRules(bool startup)
* rules will be present. Thus we can safely just tell it
* to always delete from the builin chain
*/
- if (startup && created)
- iptablesSetDeletePrivate(false);
+ if (startup)
+ iptablesSetDeletePrivate(true);
This is problematic. It means that when upgrading libvirt we will
never delete the legacy rules from the built-in chains.
We needed to create the chains upfront, so that when we recreate
rules for existing running networks, we'll upgrade them to use the
libvirt chains instead of built-in chains.
So I think we'll need to keep the code to create libvirt chains
in this networkPreReloadFirewallRules, but *only* run it if we
have existing virtual networks that are active.
That way when libvirt starts on fresh boot, chains will be crated
only when a network is started. If libvirt is upgraded on running
host, then we'll still do thoings early.
Thanks for the comments. I didn't have time to work on a V2 after travel and
before a vacation. I'll be gone next week but will pick this up the following
week after returning.
Regards,
Jim
}
@@ -701,19 +667,19 @@ int networkAddFirewallRules(virNetworkDefPtr def)
virFirewallPtr fw = NULL;
int ret = -1;
- if (errInitV4 &&
- (virNetworkDefGetIPByIndex(def, AF_INET, 0) ||
- virNetworkDefGetRouteByIndex(def, AF_INET, 0))) {
- virSetError(errInitV4);
- return -1;
- }
+ if (!pvtChainsCreated) {
+ if (iptablesSetupPrivateChains(VIR_FIREWALL_LAYER_IPV4) < 0 &&
+ (virNetworkDefGetIPByIndex(def, AF_INET, 0) ||
+ virNetworkDefGetRouteByIndex(def, AF_INET, 0)))
+ return -1;
- if (errInitV6 &&
- (virNetworkDefGetIPByIndex(def, AF_INET6, 0) ||
- virNetworkDefGetRouteByIndex(def, AF_INET6, 0) ||
- def->ipv6nogw)) {
- virSetError(errInitV6);
- return -1;
+ if (iptablesSetupPrivateChains(VIR_FIREWALL_LAYER_IPV6) < 0 &&
+ (virNetworkDefGetIPByIndex(def, AF_INET6, 0) ||
+ virNetworkDefGetRouteByIndex(def, AF_INET6, 0) ||
+ def->ipv6nogw))
+ return -1;
+
+ pvtChainsCreated = true;
}
if (def->bridgeZone) {
diff --git a/tests/networkxml2firewalldata/nat-default-linux.args b/tests/networkxml2firewalldata/nat-default-linux.args
index c9d523d043..61d620b592 100644
--- a/tests/networkxml2firewalldata/nat-default-linux.args
+++ b/tests/networkxml2firewalldata/nat-default-linux.args
@@ -1,5 +1,121 @@
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
+iptables \
+--table filter \
--insert LIBVIRT_INP \
--in-interface virbr0 \
--protocol tcp \
--
2.21.0
Regards,
Daniel
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
