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

[Daniel P. Berrangé]

On Fri, May 10, 2019 at 04:02:07PM -0600, Jim Fehlig wrote:
> 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 at suse.com>
> > > ---
> > > 
> > > 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.

Don't worry about it, I'll probably get time to do an updated patch
this week.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|