Re: [libvirt] [PATCH RFC] network: Delay creating private chains until starting network

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(a)suse.com&gt; > > --- > > > > 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.