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@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.
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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
