[PATCH common/network 0/3] Performance improvements for applying VLAN / QinQ zones

[PATCH common/network 0/3] Performance improvements for applying VLAN / QinQ zones

[Stefan Hanreich]

## Problem

When creating a VLAN / QinQ zone on a non VLAN-aware bridge, the SDN config
generation enters a code path that calls ip_link_details from PVE::IPRoute2
repeatedly. This causes a severe slowdown when applying configurations with a
large amount of VNets. What exacerbated this problem was that, aside from
calling iproute2 twice, every vnet recalculated the bridge ports for its bridge
by iterating over all existing interfaces on the host which leads to a quadratic
increase in runtime once the SDN configuration had been applied once.

## Solution

This patch series introduces a new helper to pve-common that pre-computes the
bridge port mappings used in the SDN config generation. The SDN stack then calls
that function once at the beginning of the 'Apply' step, then re-uses the result
for each vnet. This avoids calling iproute2 repeatedly and the nested loop
inside the generate_sdn_config function in the VLAN / QinQ plugins.

An initial approach only cached the ip_link_details output and reused that, but
this approach did not avoid the nested loops, so I opted for pre-calculating the
required information instead and passing only that to the zone plugins. The
first patch is a remnant of the initial approach, which was still included
because it is useful for the other callsites.

## Benchmarks

This slashes the runtime for generating the SDN configuration considerably on a
test setup with 1000 VLAN VNets present on the host / config:

Without these patches applied:

  root@pve-vlan-test:~# time perl -e 'use PVE::Network::SDN; PVE::Network::SDN::generate_raw_etc_network_config(PVE::Network::SDN::compile_running_cfg(1))'
  real    5m59.853s
  user    5m49.009s
  sys     0m34.887s

With these patches applied:

  root@pve-vlan-test:~# time perl -e 'use PVE::Network::SDN; PVE::Network::SDN::generate_raw_etc_network_config(PVE::Network::SDN::compile_running_cfg(1))'
  real    0m0.868s
  user    0m0.818s
  sys     0m0.109s

## Future Work

This patch series does not address any bottlenecks in ifupdown2, which is now
the limiting factor when applying large configurations. Applying a SDN
configuration via ifreload with 1000 VLANs takes 22 seconds. I'll be exploring
potential improvements there with Christoph, but currently we do not have high
hopes of improving the runtime there considerably.

Currently the SDN configuration is applied sequentially on each node, I'll be
looking into creating a patch series that applies the network configuration in
parallel.


pve-common:

Stefan Hanreich (2):
  iproute2: restructure get_physical_bridge_ports
  iproute2: add helper for getting bridge port mappings

 src/PVE/IPRoute2.pm | 30 +++++++++++++++++++++++++++---
 1 file changed, 27 insertions(+), 3 deletions(-)


pve-network:

Stefan Hanreich (1):
  fix #7738: zones: use pre-computed bridge port list

 src/PVE/Network/SDN/Zones.pm            |  3 +++
 src/PVE/Network/SDN/Zones/Plugin.pm     |  7 +------
 src/PVE/Network/SDN/Zones/QinQPlugin.pm |  5 +++--
 src/PVE/Network/SDN/Zones/VlanPlugin.pm |  5 +++--
 src/test/run_test_zones.pl              | 13 ++++++++++---
 5 files changed, 20 insertions(+), 13 deletions(-)


Summary over all repositories:
  6 files changed, 47 insertions(+), 16 deletions(-)

-- 
Generated by murpp 0.12.0

[PATCH pve-common 1/3] iproute2: restructure get_physical_bridge_ports

[Stefan Hanreich]

By checking first if the interface is a member of the bridge, instead
of determining its type, a lot of unnecessary checks are not performed
since checking for membership first is a lot more discriminatory. When
applying a SDN configuration with 100 VLAN VNets on a non-VLAN-aware
bridge, this saves a significant amount of statements executed:

without this optimization:
  Executed 438813 statements in 110ms

with this optimization:
  Executed 3693 statements in 10.2ms

Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
---

Notes:
    This is a remnant from an earlier approach where only the
    ip_link_details output was used, but the bridge ports were still
    recalculated. Nevertheless I decided to still include this patch
    instead of dropping it, since other call sites also profit from it.

 src/PVE/IPRoute2.pm | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/PVE/IPRoute2.pm b/src/PVE/IPRoute2.pm
index 53a17b0..ae837dc 100644
--- a/src/PVE/IPRoute2.pm
+++ b/src/PVE/IPRoute2.pm
@@ -83,11 +83,11 @@ sub get_physical_bridge_ports($bridge, $ip_links = undef) {
     }
 
     return grep {
-        (ip_link_is_physical($ip_links->{$_})
+        defined($ip_links->{$_}->{master})
+            && $ip_links->{$_}->{master} eq $bridge
+            && (ip_link_is_physical($ip_links->{$_})
                 || ip_link_is_bond($ip_links->{$_})
                 || ip_link_is_vlan($ip_links->{$_}))
-            && defined($ip_links->{$_}->{master})
-            && $ip_links->{$_}->{master} eq $bridge
     } keys $ip_links->%*;
 }
 
-- 
2.47.3

[PATCH pve-common 2/3] iproute2: add helper for getting bridge port mappings

[Stefan Hanreich]

Add a new helper for getting all bridges and their enslaved ports.
This is particularly useful for the SDN VLAN and QinQ zones, which
require a list of all bridge ports for generating the configuration
when they utilize a non-VLAN-aware bridge. With this function, SDN can
pre-compute the list and then use it to lookup the bridge ports,
instead of recalculating the list for every vnet.

Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
---
 src/PVE/IPRoute2.pm | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/src/PVE/IPRoute2.pm b/src/PVE/IPRoute2.pm
index ae837dc..cef0cbc 100644
--- a/src/PVE/IPRoute2.pm
+++ b/src/PVE/IPRoute2.pm
@@ -127,4 +127,28 @@ sub get_vlan_information() {
     return \%vlan_information;
 }
 
+sub get_bridge_port_mapping($ip_links = undef) {
+    $ip_links = ip_link_details() if !defined($ip_links);
+
+    my $mapping = {};
+
+    for my $iface_name (keys $ip_links->%*) {
+        my $ip_link = $ip_links->{$iface_name};
+        next if !ip_link_is_bridge_member($ip_link);
+
+        if (!defined($ip_link->{master})) {
+            # should never happen, but handle the case nonetheless
+            warn "found bridge_member without master";
+            next;
+        }
+
+        push($mapping->{$ip_link->{master}}->@*, $iface_name)
+            if ip_link_is_physical($ip_link)
+                || ip_link_is_vlan($ip_link)
+                || ip_link_is_bond($ip_link);
+    }
+
+    return $mapping;
+}
+
 1;
-- 
2.47.3

[PATCH pve-network 3/3] fix #7738: zones: use pre-computed bridge port list

[Stefan Hanreich]

The VLAN and QinQ zone utilized ip_link_details to obtain information
about the physical ports contained on a bridge. Instead of executing
iproute2 and computing the bridge ports for every VNet, precalculate
the bridge port list using the new helper in pve-common and then
re-use the ouput obtained at the start of the run.

Remove the get_bridge_ifaces function, since there are no longer any
call sites for this function.

Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
---
 src/PVE/Network/SDN/Zones.pm            |  3 +++
 src/PVE/Network/SDN/Zones/Plugin.pm     |  7 +------
 src/PVE/Network/SDN/Zones/QinQPlugin.pm |  5 +++--
 src/PVE/Network/SDN/Zones/VlanPlugin.pm |  5 +++--
 src/test/run_test_zones.pl              | 13 ++++++++++---
 5 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/src/PVE/Network/SDN/Zones.pm b/src/PVE/Network/SDN/Zones.pm
index 4c1468cf..f1fb32d9 100644
--- a/src/PVE/Network/SDN/Zones.pm
+++ b/src/PVE/Network/SDN/Zones.pm
@@ -7,6 +7,7 @@ use JSON;
 
 use PVE::Tools qw(extract_param dir_glob_regex run_command);
 use PVE::Cluster qw(cfs_read_file cfs_write_file cfs_lock_file);
+use PVE::IPRoute2;
 use PVE::Network;
 
 use PVE::Network::SDN::Vnets;
@@ -117,6 +118,7 @@ sub generate_etc_network_config {
     return if !$vnet_cfg && !$zone_cfg;
 
     my $interfaces_config = PVE::INotify::read_file('interfaces');
+    my $bridge_ports = PVE::IPRoute2::get_bridge_port_mapping();
 
     #generate configuration
     my $config = {};
@@ -157,6 +159,7 @@ sub generate_etc_network_config {
                 $subnet_cfg,
                 $interfaces_config,
                 $config,
+                $bridge_ports,
             );
         };
         if (my $err = $@) {
diff --git a/src/PVE/Network/SDN/Zones/Plugin.pm b/src/PVE/Network/SDN/Zones/Plugin.pm
index 74a3384c..184fca4e 100644
--- a/src/PVE/Network/SDN/Zones/Plugin.pm
+++ b/src/PVE/Network/SDN/Zones/Plugin.pm
@@ -5,7 +5,6 @@ use warnings;
 
 use PVE::Tools qw(run_command);
 use Net::IP qw(ip_get_version);
-use PVE::IPRoute2;
 use PVE::JSONSchema;
 use PVE::Cluster;
 use PVE::Network;
@@ -112,6 +111,7 @@ sub generate_sdn_config {
         $subnet_cfg,
         $interfaces_config,
         $config,
+        $bridge_ports,
     ) = @_;
 
     die "please implement inside plugin";
@@ -385,11 +385,6 @@ sub is_vlanaware {
     return PVE::Tools::file_read_firstline("/sys/class/net/$bridge/bridge/vlan_filtering");
 }
 
-sub get_bridge_ifaces {
-    my ($bridge) = @_;
-    return PVE::IPRoute2::get_physical_bridge_ports($bridge);
-}
-
 sub datacenter_config {
     return PVE::Cluster::cfs_read_file('datacenter.cfg');
 }
diff --git a/src/PVE/Network/SDN/Zones/QinQPlugin.pm b/src/PVE/Network/SDN/Zones/QinQPlugin.pm
index a75940c6..642420e9 100644
--- a/src/PVE/Network/SDN/Zones/QinQPlugin.pm
+++ b/src/PVE/Network/SDN/Zones/QinQPlugin.pm
@@ -64,6 +64,7 @@ sub generate_sdn_config {
         $subnet_cfg,
         $interfaces_config,
         $config,
+        $bridge_ports,
     ) = @_;
 
     my ($bridge, $mtu, $stag) = $plugin_config->@{ 'bridge', 'mtu', 'tag' };
@@ -128,9 +129,9 @@ sub generate_sdn_config {
     } else {
         # eth--->eth.x(svlan)----->vlanwarebridge-(tag)----->vnet---->vnet
 
-        my @bridge_ifaces = PVE::Network::SDN::Zones::Plugin::get_bridge_ifaces($bridge);
+        my $bridge_ifaces = $bridge_ports->{$bridge};
 
-        for my $bridge_iface (@bridge_ifaces) {
+        for my $bridge_iface ($bridge_ifaces->@*) {
             # use named vlan interface to avoid too long names
             my $svlan_iface = "sv_$zoneid";
 
diff --git a/src/PVE/Network/SDN/Zones/VlanPlugin.pm b/src/PVE/Network/SDN/Zones/VlanPlugin.pm
index 9102b34f..271b3fab 100644
--- a/src/PVE/Network/SDN/Zones/VlanPlugin.pm
+++ b/src/PVE/Network/SDN/Zones/VlanPlugin.pm
@@ -63,6 +63,7 @@ sub generate_sdn_config {
         $subnet_cfg,
         $interfaces_config,
         $config,
+        $bridge_ports,
     ) = @_;
 
     my $bridge = $plugin_config->{bridge};
@@ -112,10 +113,10 @@ sub generate_sdn_config {
 
         my $bridgevlan = $bridge . "v" . $tag;
 
-        my @bridge_ifaces = PVE::Network::SDN::Zones::Plugin::get_bridge_ifaces($bridge);
+        my $bridge_ifaces = $bridge_ports->{$bridge};
 
         my $bridge_ports = "";
-        foreach my $bridge_iface (@bridge_ifaces) {
+        foreach my $bridge_iface ($bridge_ifaces->@*) {
             $bridge_ports .= " $bridge_iface.$tag";
         }
 
diff --git a/src/test/run_test_zones.pl b/src/test/run_test_zones.pl
index dd458b77..f8ef7375 100755
--- a/src/test/run_test_zones.pl
+++ b/src/test/run_test_zones.pl
@@ -55,6 +55,16 @@ foreach my $test (@tests) {
         },
     );
 
+    my $pve_common_iproute;
+    $pve_common_iproute = Test::MockModule->new('PVE::IPRoute2');
+    $pve_common_iproute->mock(
+        get_bridge_port_mapping => sub {
+            return {
+                vmbr0 => ["eth0"],
+            },;
+        },
+    );
+
     my $pve_common_network;
     $pve_common_network = Test::MockModule->new('PVE::Network');
     $pve_common_network->mock(
@@ -94,9 +104,6 @@ foreach my $test (@tests) {
         is_vlanaware => sub {
             return $interfaces_config->{ifaces}->{vmbr0}->{'bridge_vlan_aware'};
         },
-        get_bridge_ifaces => sub {
-            return ('eth0');
-        },
         find_bridge => sub {
             return;
         },
-- 
2.47.3