[pve-devel] [PATCH qemu-server/container/network 0/3] implement bridge disable learning

[pve-devel] [PATCH qemu-server/container/network 0/3] implement bridge disable learning

[Alexandre Derumier]

Hi,

This is a resent of missing patches to implement bridge disable learning

in 7.2 release note
https://pve.proxmox.com/wiki/Roadmap#Proxmox_VE_7.2

It has been announced that bridge-disable-mac-learning could be disable in /etc/network/interfaces,

but the qemu-server/lxc plugging patches has not been yet commit.


changelog:

- rebase on last git.

qemu-server:

Alexandre Derumier (3):
  tap_plug: add support for bridge disable learning
  vm_start/vm_resume : add_nets_bridge_fdb
  migration : add del_nets_bridge_fdb

 PVE/QemuMigrate.pm                    |  1 +
 PVE/QemuServer.pm                     | 53 +++++++++++++++++++++++++--
 test/MigrationTest/QemuMigrateMock.pm |  3 ++
 vm-network-scripts/pve-bridge         |  6 ++-
 4 files changed, 59 insertions(+), 4 deletions(-)

pve-container:

Alexandre Derumier (1):
  net : add support for bridge disable mac learning

 src/PVE/LXC.pm  | 16 ++++++++++++++--
 src/lxcnetaddbr |  7 ++++++-
 2 files changed, 20 insertions(+), 3 deletions(-)

pve-network:

Alexandre Derumier (1):
  bridge-disable-mac-learning : use $opts for tap_plug

 PVE/Network/SDN/Zones.pm        | 5 +++--
 PVE/Network/SDN/Zones/Plugin.pm | 4 +++-
 2 files changed, 6 insertions(+), 3 deletions(-)

-- 
2.30.2

[pve-devel] [PATCH V2 pve-network 1/1] bridge-disable-mac-learning : use $opts for tap_plug

[Alexandre Derumier]

Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
---
 PVE/Network/SDN/Zones.pm        | 5 +++--
 PVE/Network/SDN/Zones/Plugin.pm | 4 +++-
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/PVE/Network/SDN/Zones.pm b/PVE/Network/SDN/Zones.pm
index 492defd..f8e40b1 100644
--- a/PVE/Network/SDN/Zones.pm
+++ b/PVE/Network/SDN/Zones.pm
@@ -309,8 +309,9 @@ sub tap_plug {
     my $vnet = PVE::Network::SDN::Vnets::get_vnet($bridge, 1);
     if (!$vnet) { # fallback for classic bridge
 	my $interfaces_config = PVE::INotify::read_file('interfaces');
-	my $disablelearning = 1 if $interfaces_config->{ifaces}->{$bridge} && $interfaces_config->{ifaces}->{$bridge}->{'bridge-disable-mac-learning'};
-	PVE::Network::tap_plug($iface, $bridge, $tag, $firewall, $trunks, $rate, $disablelearning);
+	my $opts = {};
+	$opts->{learning} = 0 if $interfaces_config->{ifaces}->{$bridge} && $interfaces_config->{ifaces}->{$bridge}->{'bridge-disable-mac-learning'};
+	PVE::Network::tap_plug($iface, $bridge, $tag, $firewall, $trunks, $rate, $opts);
 	return;
     }
 
diff --git a/PVE/Network/SDN/Zones/Plugin.pm b/PVE/Network/SDN/Zones/Plugin.pm
index 1f5b5c2..2c707b3 100644
--- a/PVE/Network/SDN/Zones/Plugin.pm
+++ b/PVE/Network/SDN/Zones/Plugin.pm
@@ -227,7 +227,9 @@ sub tap_plug {
     my $vlan_aware = PVE::Tools::file_read_firstline("/sys/class/net/$vnetid/bridge/vlan_filtering");
     die "vm vlans are not allowed on vnet $vnetid" if !$vlan_aware && ($tag || $trunks);
 
-    PVE::Network::tap_plug($iface, $vnetid, $tag, $firewall, $trunks, $rate, $plugin_config->{'bridge-disable-mac-learning'});
+    my $opts = {};
+    $opts->{learning} = 0 if $plugin_config->{'bridge-disable-mac-learning'};
+    PVE::Network::tap_plug($iface, $vnetid, $tag, $firewall, $trunks, $rate, $opts);
 }
 
 #helper
-- 
2.30.2

[pve-devel] [PATCH V3 pve-container 1/1] net : add support for bridge disable mac learning

[Alexandre Derumier]

Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
---
 src/PVE/LXC.pm  | 16 ++++++++++++++--
 src/lxcnetaddbr |  7 ++++++-
 2 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
index fe63087..f6501bb 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -949,8 +949,14 @@ sub update_net {
 
 		if ($have_sdn) {
 		    PVE::Network::SDN::Zones::tap_plug($veth, $newnet->{bridge}, $newnet->{tag}, $newnet->{firewall}, $newnet->{trunks}, $newnet->{rate});
+		    PVE::Network::SDN::Zones::add_bridge_fdb($veth, $newnet->{hwaddr}, $newnet->{bridge}, $newnet->{firewall});
 		} else {
-		    PVE::Network::tap_plug($veth, $newnet->{bridge}, $newnet->{tag}, $newnet->{firewall}, $newnet->{trunks}, $newnet->{rate});
+		    my $interfaces_config = PVE::INotify::read_file('interfaces');
+		    my $bridge = $newnet->{bridge};
+		    my $opts = {};
+		    $opts->{learning} = 0 if $interfaces_config->{ifaces}->{$bridge} && $interfaces_config->{ifaces}->{$bridge}->{'bridge-disable-mac-learning'};
+		    PVE::Network::tap_plug($veth, $newnet->{bridge}, $newnet->{tag}, $newnet->{firewall}, $newnet->{trunks}, $newnet->{rate}, $opts);
+		    PVE::Network::add_bridge_fdb($veth, $newnet->{hwaddr}, $newnet->{firewall}) if defined($opts->{learning}) && !$opts->{learning};
 		}
 
 		# This includes the rate:
@@ -983,9 +989,15 @@ sub hotplug_net {
     if ($have_sdn) {
 	PVE::Network::SDN::Zones::veth_create($veth, $vethpeer, $newnet->{bridge}, $newnet->{hwaddr});
 	PVE::Network::SDN::Zones::tap_plug($veth, $newnet->{bridge}, $newnet->{tag}, $newnet->{firewall}, $newnet->{trunks}, $newnet->{rate});
+	PVE::Network::SDN::Zones::add_bridge_fdb($veth, $newnet->{hwaddr}, $newnet->{bridge}, $newnet->{firewall});
     } else {
+	my $interfaces_config = PVE::INotify::read_file('interfaces');
+	my $bridge = $newnet->{bridge};
+	my $opts = {};
+	$opts->{learning} = 0 if $interfaces_config->{ifaces}->{$bridge} && $interfaces_config->{ifaces}->{$bridge}->{'bridge-disable-mac-learning'};
 	PVE::Network::veth_create($veth, $vethpeer, $newnet->{bridge}, $newnet->{hwaddr});
-	PVE::Network::tap_plug($veth, $newnet->{bridge}, $newnet->{tag}, $newnet->{firewall}, $newnet->{trunks}, $newnet->{rate});
+	PVE::Network::tap_plug($veth, $newnet->{bridge}, $newnet->{tag}, $newnet->{firewall}, $newnet->{trunks}, $newnet->{rate}, $opts);
+	PVE::Network::add_bridge_fdb($veth, $newnet->{hwaddr}, $newnet->{firewall}) if defined($opts->{learning}) && !$opts->{learning};
     }
 
     # attach peer in container
diff --git a/src/lxcnetaddbr b/src/lxcnetaddbr
index 8bd9d22..1d72ac2 100755
--- a/src/lxcnetaddbr
+++ b/src/lxcnetaddbr
@@ -63,8 +63,13 @@ if (-d "/sys/class/net/$iface") {
 
     if ($have_sdn) {
 	PVE::Network::SDN::Zones::tap_plug($iface, $net->{bridge}, $tag, $firewall, $trunks, $rate);
+	PVE::Network::SDN::Zones::add_bridge_fdb($iface, $net->{hwaddr}, $net->{bridge}, $net->{firewall});
     } else {
-	PVE::Network::tap_plug($iface, $bridge, $tag, $firewall, $trunks, $rate);
+	my $interfaces_config = PVE::INotify::read_file('interfaces');
+	my $opts = {};
+	$opts->{learning} = 0 if $interfaces_config->{ifaces}->{$bridge} && $interfaces_config->{ifaces}->{$bridge}->{'bridge-disable-mac-learning'};
+	PVE::Network::tap_plug($iface, $bridge, $tag, $firewall, $trunks, $rate, $opts);
+	PVE::Network::add_bridge_fdb($iface, $net->{hwaddr}, $net->{firewall}) if defined($opts->{learning}) && !$opts->{learning};
     }
 }
 
-- 
2.30.2

[pve-devel] [PATCH V3 qemu-server 1/3] tap_plug: add support for bridge disable learning

[Alexandre Derumier]

This disabling mac learning && unicast flood for the tap interface

for vmstart, we don't add mac directly to fdb.
We set it latter if it's a migration or a fresh start.

for nic hotplug, we directly add mac to fdb

Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
---
 PVE/QemuServer.pm             | 8 +++++++-
 vm-network-scripts/pve-bridge | 6 +++++-
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index c706653..0114d06 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -5137,8 +5137,14 @@ sub vmconfig_update_net {
 
 		if ($have_sdn) {
 		    PVE::Network::SDN::Zones::tap_plug($iface, $newnet->{bridge}, $newnet->{tag}, $newnet->{firewall}, $newnet->{trunks}, $newnet->{rate});
+		    PVE::Network::SDN::Zones::add_bridge_fdb($iface, $newnet->{macaddr}, $newnet->{bridge}, $newnet->{firewall});
 		} else {
-		    PVE::Network::tap_plug($iface, $newnet->{bridge}, $newnet->{tag}, $newnet->{firewall}, $newnet->{trunks}, $newnet->{rate});
+		    my $interfaces_config = PVE::INotify::read_file('interfaces');
+		    my $bridge = $newnet->{bridge};
+		    my $opts = {};
+		    $opts->{learning} = 0 if $interfaces_config->{ifaces}->{$bridge} && $interfaces_config->{ifaces}->{$bridge}->{'bridge-disable-mac-learning'};
+		    PVE::Network::tap_plug($iface, $newnet->{bridge}, $newnet->{tag}, $newnet->{firewall}, $newnet->{trunks}, $newnet->{rate}, $opts);
+		    PVE::Network::add_bridge_fdb($iface, $newnet->{macaddr}, $newnet->{firewall}) if defined($opts->{learning}) && !$opts->{learning};
 		}
 	    } elsif (safe_num_ne($oldnet->{rate}, $newnet->{rate})) {
 		# Rate can be applied on its own but any change above needs to
diff --git a/vm-network-scripts/pve-bridge b/vm-network-scripts/pve-bridge
index d37ce33..38cf2f6 100755
--- a/vm-network-scripts/pve-bridge
+++ b/vm-network-scripts/pve-bridge
@@ -47,8 +47,12 @@ if ($have_sdn) {
     PVE::Network::SDN::Zones::tap_create($iface, $net->{bridge});
     PVE::Network::SDN::Zones::tap_plug($iface, $net->{bridge}, $net->{tag}, $net->{firewall}, $net->{trunks}, $net->{rate});
 } else {
+    my $interfaces_config = PVE::INotify::read_file('interfaces');
+    my $bridge = $net->{bridge};
+    my $opts = {};
+    $opts->{learning} = 0 if $interfaces_config->{ifaces}->{$bridge} && $interfaces_config->{ifaces}->{$bridge}->{'bridge-disable-mac-learning'};
     PVE::Network::tap_create($iface, $net->{bridge});
-    PVE::Network::tap_plug($iface, $net->{bridge}, $net->{tag}, $net->{firewall}, $net->{trunks}, $net->{rate});
+    PVE::Network::tap_plug($iface, $net->{bridge}, $net->{tag}, $net->{firewall}, $net->{trunks}, $net->{rate}, $opts);
 }
 
 exit 0;
-- 
2.30.2

[pve-devel] [PATCH V3 qemu-server 2/3] vm_start/vm_resume : add_nets_bridge_fdb

[Alexandre Derumier]

on vm start (no live migration), we can simply add mac address in fdb.
In case of a live migration, we add the mac address just before the resume.

Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
---
 PVE/QemuServer.pm | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 0114d06..6d71006 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -5797,6 +5797,7 @@ sub vm_start_nolock {
 	    my $nicconf = parse_net($conf->{$opt});
 	    qemu_set_link_status($vmid, $opt, 0) if $nicconf->{link_down};
 	}
+	add_nets_bridge_fdb($conf, $vmid);
     }
 
     mon_cmd($vmid, 'qom-set',
@@ -6155,6 +6156,7 @@ sub vm_resume {
 	my $res = mon_cmd($vmid, 'query-status');
 	my $resume_cmd = 'cont';
 	my $reset = 0;
+	my $conf = PVE::QemuConfig->load_config($vmid);
 
 	if ($res->{status}) {
 	    return if $res->{status} eq 'running'; # job done, go home
@@ -6164,8 +6166,6 @@ sub vm_resume {
 
 	if (!$nocheck) {
 
-	    my $conf = PVE::QemuConfig->load_config($vmid);
-
 	    PVE::QemuConfig->check_lock($conf)
 		if !($skiplock || PVE::QemuConfig->has_lock($conf, 'backup'));
 	}
@@ -6175,6 +6175,9 @@ sub vm_resume {
 	    # request before the backup finishes for example
 	    mon_cmd($vmid, "system_reset");
 	}
+
+	add_nets_bridge_fdb($conf, $vmid) if $resume_cmd eq 'cont';
+
 	mon_cmd($vmid, $resume_cmd);
     });
 }
@@ -8237,4 +8240,22 @@ sub check_volume_storage_type {
     return 1;
 }
 
+sub add_nets_bridge_fdb {
+    my ($conf, $vmid) = @_;
+
+    foreach my $opt (keys %$conf) {
+	if ($opt =~  m/^net(\d+)$/) {
+	    my $net = parse_net($conf->{$opt});
+	    next if !$net;
+	    next if !$net->{macaddr};
+
+	    my $iface = "tap${vmid}i$1";
+	    if ($have_sdn) {
+		PVE::Network::SDN::Zones::add_bridge_fdb($iface, $net->{macaddr}, $net->{bridge}, $net->{firewall});
+	    } else {
+		PVE::Network::add_bridge_fdb($iface, $net->{macaddr}, $net->{firewall});
+	    }
+	}
+    }
+}
 1;
-- 
2.30.2

[pve-devel] [PATCH V3 qemu-server 3/3] migration : add del_nets_bridge_fdb

[Alexandre Derumier]

at the end of a live migration, we need to remove old mac entries
on source host (vm is not yet stopped), before resume vm on target host

Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
---
 PVE/QemuMigrate.pm                    |  1 +
 PVE/QemuServer.pm                     | 20 ++++++++++++++++++++
 test/MigrationTest/QemuMigrateMock.pm |  3 +++
 3 files changed, 24 insertions(+)

diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
index d52dc8d..b72a3fe 100644
--- a/PVE/QemuMigrate.pm
+++ b/PVE/QemuMigrate.pm
@@ -1157,6 +1157,7 @@ sub phase3_cleanup {
 
     # transfer replication state before move config
     $self->transfer_replication_state() if $self->{is_replicated};
+    PVE::QemuServer::del_nets_bridge_fdb($conf, $vmid);
     PVE::QemuConfig->move_config_to_node($vmid, $self->{node});
     $self->switch_replication_job_target() if $self->{is_replicated};
 
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 6d71006..47a4882 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -8258,4 +8258,24 @@ sub add_nets_bridge_fdb {
 	}
     }
 }
+
+sub del_nets_bridge_fdb {
+    my ($conf, $vmid) = @_;
+
+    foreach my $opt (keys %$conf) {
+	if ($opt =~  m/^net(\d+)$/) {
+	    my $net = parse_net($conf->{$opt});
+	    next if !$net;
+	    next if !$net->{macaddr};
+
+	    my $iface = "tap${vmid}i$1";
+	    if ($have_sdn) {
+		PVE::Network::SDN::Zones::del_bridge_fdb($iface, $net->{macaddr}, $net->{bridge}, $net->{firewall});
+	    } else {
+		PVE::Network::del_bridge_fdb($iface, $net->{macaddr}, $net->{firewall});
+	    }
+	}
+    }
+}
+
 1;
diff --git a/test/MigrationTest/QemuMigrateMock.pm b/test/MigrationTest/QemuMigrateMock.pm
index f2c0281..f00b974 100644
--- a/test/MigrationTest/QemuMigrateMock.pm
+++ b/test/MigrationTest/QemuMigrateMock.pm
@@ -158,6 +158,9 @@ $MigrationTest::Shared::qemu_server_module->mock(
 	$vm_stop_executed = 1;
 	delete $expected_calls->{'vm_stop'};
     },
+    del_nets_bridge_fdb => sub {
+	return;
+    },
 );
 
 my $qemu_server_cpuconfig_module = Test::MockModule->new("PVE::QemuServer::CPUConfig");
-- 
2.30.2

Re: [pve-devel] [PATCH V3 qemu-server 3/3] migration : add del_nets_bridge_fdb

[Mira Limbeck]

On 8/24/22 18:26, Alexandre Derumier wrote:
> at the end of a live migration, we need to remove old mac entries
> on source host (vm is not yet stopped), before resume vm on target host
>
> Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
> ---
>   PVE/QemuMigrate.pm                    |  1 +
>   PVE/QemuServer.pm                     | 20 ++++++++++++++++++++
>   test/MigrationTest/QemuMigrateMock.pm |  3 +++
>   3 files changed, 24 insertions(+)
>
> diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
> index d52dc8d..b72a3fe 100644
> --- a/PVE/QemuMigrate.pm
> +++ b/PVE/QemuMigrate.pm
> @@ -1157,6 +1157,7 @@ sub phase3_cleanup {
>   
>       # transfer replication state before move config
>       $self->transfer_replication_state() if $self->{is_replicated};
> +    PVE::QemuServer::del_nets_bridge_fdb($conf, $vmid);

I'm currently going through this patch series, and I'm wondering. 
Wouldn't it make more sense to delete the bridge fdb entries as part of 
the VM shutdown instead?

This way those would be cleaned up whenever the VM is stopped (and the 
tap device gets destroyed). No special handling for migrations needed, 
unless I'm missing something?

Re: [pve-devel] [PATCH V3 qemu-server 1/3] tap_plug: add support for bridge disable learning

[Mira Limbeck]

On 8/24/22 18:26, Alexandre Derumier wrote:
> This disabling mac learning && unicast flood for the tap interface
>
> for vmstart, we don't add mac directly to fdb.
> We set it latter if it's a migration or a fresh start.
>
> for nic hotplug, we directly add mac to fdb
>
> Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
> ---
>   PVE/QemuServer.pm             | 8 +++++++-
>   vm-network-scripts/pve-bridge | 6 +++++-
>   2 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index c706653..0114d06 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -5137,8 +5137,14 @@ sub vmconfig_update_net {
>   
>   		if ($have_sdn) {
>   		    PVE::Network::SDN::Zones::tap_plug($iface, $newnet->{bridge}, $newnet->{tag}, $newnet->{firewall}, $newnet->{trunks}, $newnet->{rate});
> +		    PVE::Network::SDN::Zones::add_bridge_fdb($iface, $newnet->{macaddr}, $newnet->{bridge}, $newnet->{firewall});
>   		} else {
> -		    PVE::Network::tap_plug($iface, $newnet->{bridge}, $newnet->{tag}, $newnet->{firewall}, $newnet->{trunks}, $newnet->{rate});
> +		    my $interfaces_config = PVE::INotify::read_file('interfaces');
> +		    my $bridge = $newnet->{bridge};
> +		    my $opts = {};
> +		    $opts->{learning} = 0 if $interfaces_config->{ifaces}->{$bridge} && $interfaces_config->{ifaces}->{$bridge}->{'bridge-disable-mac-learning'};
> +		    PVE::Network::tap_plug($iface, $newnet->{bridge}, $newnet->{tag}, $newnet->{firewall}, $newnet->{trunks}, $newnet->{rate}, $opts);
> +		    PVE::Network::add_bridge_fdb($iface, $newnet->{macaddr}, $newnet->{firewall}) if defined($opts->{learning}) && !$opts->{learning};
>   		}
>   	    } elsif (safe_num_ne($oldnet->{rate}, $newnet->{rate})) {
>   		# Rate can be applied on its own but any change above needs to
> diff --git a/vm-network-scripts/pve-bridge b/vm-network-scripts/pve-bridge
> index d37ce33..38cf2f6 100755
> --- a/vm-network-scripts/pve-bridge
> +++ b/vm-network-scripts/pve-bridge
> @@ -47,8 +47,12 @@ if ($have_sdn) {
>       PVE::Network::SDN::Zones::tap_create($iface, $net->{bridge});
>       PVE::Network::SDN::Zones::tap_plug($iface, $net->{bridge}, $net->{tag}, $net->{firewall}, $net->{trunks}, $net->{rate});
>   } else {
> +    my $interfaces_config = PVE::INotify::read_file('interfaces');
> +    my $bridge = $net->{bridge};
> +    my $opts = {};
> +    $opts->{learning} = 0 if $interfaces_config->{ifaces}->{$bridge} && $interfaces_config->{ifaces}->{$bridge}->{'bridge-disable-mac-learning'};
>       PVE::Network::tap_create($iface, $net->{bridge});
> -    PVE::Network::tap_plug($iface, $net->{bridge}, $net->{tag}, $net->{firewall}, $net->{trunks}, $net->{rate});
> +    PVE::Network::tap_plug($iface, $net->{bridge}, $net->{tag}, $net->{firewall}, $net->{trunks}, $net->{rate}, $opts);
>   }

Why not add the bridge in the pve-bridge script as well? This way there 
would be no need for patch 2 for qemu-server since we always add the MAC 
address to the FDB whenever the tap device is plugged.

If we then remove it in the pve-bridgedown script, there should be no 
need for patch 3, right?

Re: [pve-devel] [PATCH V3 qemu-server 3/3] migration : add del_nets_bridge_fdb

[DERUMIER, Alexandre]

Le lundi 07 novembre 2022 à 13:41 +0100, Mira Limbeck a écrit :
> 
> I'm currently going through this patch series
Thanks for looking at it ! 

>  and I'm wondering. 
> Wouldn't it make more sense to delete the bridge fdb entries as part
> of 
> the VM shutdown instead?
> 
> This way those would be cleaned up whenever the VM is stopped (and
> the 
> tap device gets destroyed). No special handling for migrations
> needed, 
> unless I'm missing something?
> 

Well, the fdb entry is already auto-deleted when the tap in unplug from
the bridge. (so when qemu process is stopped too)

We want the fdb entries only present in the bridge on active vm.
(some advanced controlplane like evpn, also blocking the write of
duplicated mac on multiple nodes)

as we resume the targetvm before stopping the sourcevm, we need to
remove fdb entries on source bridge manually.




The workflow is:
----------------

1) original vm is started, fbd entries are written in bridge

2) start migration, destination vm is started paused without writting
fdb entries   (I think I don't implement this currently, I need to
verify)

3) At the end of the migration, when sourcevm is stopped, before the
resume of target vm:

  - fdb entries are removed on source vm bridge

  - fdb entries are added in destination vm bridge

  - targetvm is resumed

  - sourcevm is stopped





I'll review and retest the full patch series tomorrow
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://antiphishing.cetsi.fr/proxy/v3?i=Zk92VEFKaGQ4Ums4cnZEUWMTpfHaXFQGRw1_CnOoOH0&r=bHA1dGV3NWJQVUloaWNFUZPm0fiiBviaiy_RDav2GQ1U4uy6lsDDv3uBszpvvWYQN5FqKqFD6WPYupfAUP1c9g&f=SlhDbE9uS2laS2JaZFpNWvmsxai1zlJP9llgnl5HIv-4jAji8Dh2BQawzxID5bzr6Uv-3EQd-eluQbsPfcUOTg&u=https%3A//lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel&k=XRKU
> 

Re: [pve-devel] [PATCH V3 qemu-server 1/3] tap_plug: add support for bridge disable learning

[DERUMIER, Alexandre]

Le mercredi 09 novembre 2022 à 15:19 +0100, Mira Limbeck a écrit :
> 
> 
> Why not add the bridge in the pve-bridge script as well? This way
> there 
> would be no need for patch 2 for qemu-server since we always add the
> MAC 
> address to the FDB whenever the tap device is plugged.
> 
> If we then remove it in the pve-bridgedown script, there should be no
> need for patch 3, right?
> 
> 

ok, I review the whole patch serie, the current behaviour is good.
(Seem the workflow I have send in my previous comment on patch3)


We can't add fdb entry inconditionally in pve-bridge,as on live
migration, we don't want to add fdb entry until the migration is
finished.

(If we do that, the others vms on the target host, will not be able to
communicate with source vms during the migration)

Re: [pve-devel] [PATCH V3 qemu-server 1/3] tap_plug: add support for bridge disable learning

[Mira Limbeck]

On 11/11/22 09:36, DERUMIER, Alexandre wrote:
> Le mercredi 09 novembre 2022 à 15:19 +0100, Mira Limbeck a écrit :
>>
>> Why not add the bridge in the pve-bridge script as well? This way
>> there
>> would be no need for patch 2 for qemu-server since we always add the
>> MAC
>> address to the FDB whenever the tap device is plugged.
>>
>> If we then remove it in the pve-bridgedown script, there should be no
>> need for patch 3, right?
>>
>>
> ok, I review the whole patch serie, the current behaviour is good.
> (Seem the workflow I have send in my previous comment on patch3)
>
>
> We can't add fdb entry inconditionally in pve-bridge,as on live
> migration, we don't want to add fdb entry until the migration is
> finished.
>
> (If we do that, the others vms on the target host, will not be able to
> communicate with source vms during the migration)
>
Wouldn't it still send the traffic to the source VM while the entry is 
still alive?

But since you're a lot more knowledgeable about networks, I trust your 
judgement in this case.


In this case consider the 3 qemu-server patches:

Reviewed-by: Mira Limbeck <m.limbeck@proxmox.com>

Tested-by: Mira Limbeck <m.limbeck@proxmox.com>


The pve-network patch has only been tested without SDN for now, I'll try 
to set something up to test it with SDN as well.

The pve-container patch is on my todo list as well.

Re: [pve-devel] [PATCH V3 qemu-server 1/3] tap_plug: add support for bridge disable learning

[Thomas Lamprecht]

Am 24/08/2022 um 18:26 schrieb Alexandre Derumier:
> This disabling mac learning && unicast flood for the tap interface
> 
> for vmstart, we don't add mac directly to fdb.
> We set it latter if it's a migration or a fresh start.
> 
> for nic hotplug, we directly add mac to fdb
> 
> Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
> ---
>  PVE/QemuServer.pm             | 8 +++++++-
>  vm-network-scripts/pve-bridge | 6 +++++-
>  2 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index c706653..0114d06 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -5137,8 +5137,14 @@ sub vmconfig_update_net {
>  
>  		if ($have_sdn) {
>  		    PVE::Network::SDN::Zones::tap_plug($iface, $newnet->{bridge}, $newnet->{tag}, $newnet->{firewall}, $newnet->{trunks}, $newnet->{rate});
> +		    PVE::Network::SDN::Zones::add_bridge_fdb($iface, $newnet->{macaddr}, $newnet->{bridge}, $newnet->{firewall});
>  		} else {
> -		    PVE::Network::tap_plug($iface, $newnet->{bridge}, $newnet->{tag}, $newnet->{firewall}, $newnet->{trunks}, $newnet->{rate});
> +		    my $interfaces_config = PVE::INotify::read_file('interfaces');
> +		    my $bridge = $newnet->{bridge};
> +		    my $opts = {};
> +		    $opts->{learning} = 0 if $interfaces_config->{ifaces}->{$bridge} && $interfaces_config->{ifaces}->{$bridge}->{'bridge-disable-mac-learning'};
> +		    PVE::Network::tap_plug($iface, $newnet->{bridge}, $newnet->{tag}, $newnet->{firewall}, $newnet->{trunks}, $newnet->{rate}, $opts);
> +		    PVE::Network::add_bridge_fdb($iface, $newnet->{macaddr}, $newnet->{firewall}) if defined($opts->{learning}) && !$opts->{learning};
>  		}
>  	    } elsif (safe_num_ne($oldnet->{rate}, $newnet->{rate})) {
>  		# Rate can be applied on its own but any change above needs to
> diff --git a/vm-network-scripts/pve-bridge b/vm-network-scripts/pve-bridge
> index d37ce33..38cf2f6 100755
> --- a/vm-network-scripts/pve-bridge
> +++ b/vm-network-scripts/pve-bridge
> @@ -47,8 +47,12 @@ if ($have_sdn) {
>      PVE::Network::SDN::Zones::tap_create($iface, $net->{bridge});
>      PVE::Network::SDN::Zones::tap_plug($iface, $net->{bridge}, $net->{tag}, $net->{firewall}, $net->{trunks}, $net->{rate});
>  } else {
> +    my $interfaces_config = PVE::INotify::read_file('interfaces');
> +    my $bridge = $net->{bridge};
> +    my $opts = {};
> +    $opts->{learning} = 0 if $interfaces_config->{ifaces}->{$bridge} && $interfaces_config->{ifaces}->{$bridge}->{'bridge-disable-mac-learning'};
>      PVE::Network::tap_create($iface, $net->{bridge});
> -    PVE::Network::tap_plug($iface, $net->{bridge}, $net->{tag}, $net->{firewall}, $net->{trunks}, $net->{rate});
> +    PVE::Network::tap_plug($iface, $net->{bridge}, $net->{tag}, $net->{firewall}, $net->{trunks}, $net->{rate}, $opts);
>  }
>  
>  exit 0;

what about moving this into pve-common instead? IOW. something like:

diff --git a/src/PVE/Network.pm b/src/PVE/Network.pm
index c468e40..cc2403c 100644
--- a/src/PVE/Network.pm
+++ b/src/PVE/Network.pm
@@ -460,7 +460,12 @@ sub tap_plug {
 
     $opts = {} if !defined($opts);
 
-    my $no_learning = defined($opts->{learning}) && !$opts->{learning}; # default to learning on
+    if (!defined($opts->{learning})) { # auto-detect
+       my $interfaces_config = PVE::INotify::read_file('interfaces');
+       my $bridge = $interfaces_config->{ifaces}->{$bridge};
+       $opts->{learning} = !($bridge && $bridge->{'bridge-disable-mac-learning'}); # default learning to on
+    }
+    my $no_learning = !$opts->{learning};
 
     # cleanup old port config from any openvswitch bridge
     eval {

That way we'd not need to touch all call sites and avoid forgetting it on new ones.

Re: [pve-devel] [PATCH V3 qemu-server 2/3] vm_start/vm_resume : add_nets_bridge_fdb

[Thomas Lamprecht]

Am 24/08/2022 um 18:26 schrieb Alexandre Derumier:
> on vm start (no live migration), we can simply add mac address in fdb.
> In case of a live migration, we add the mac address just before the resume.
> 
> Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
> ---
>  PVE/QemuServer.pm | 25 +++++++++++++++++++++++--
>  1 file changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 0114d06..6d71006 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -5797,6 +5797,7 @@ sub vm_start_nolock {
>  	    my $nicconf = parse_net($conf->{$opt});
>  	    qemu_set_link_status($vmid, $opt, 0) if $nicconf->{link_down};
>  	}
> +	add_nets_bridge_fdb($conf, $vmid);
>      }
>  
>      mon_cmd($vmid, 'qom-set',
> @@ -6155,6 +6156,7 @@ sub vm_resume {
>  	my $res = mon_cmd($vmid, 'query-status');
>  	my $resume_cmd = 'cont';
>  	my $reset = 0;
> +	my $conf = PVE::QemuConfig->load_config($vmid);
>  
>  	if ($res->{status}) {
>  	    return if $res->{status} eq 'running'; # job done, go home
> @@ -6164,8 +6166,6 @@ sub vm_resume {
>  
>  	if (!$nocheck) {
>  
> -	    my $conf = PVE::QemuConfig->load_config($vmid);
> -
>  	    PVE::QemuConfig->check_lock($conf)
>  		if !($skiplock || PVE::QemuConfig->has_lock($conf, 'backup'));
>  	}
> @@ -6175,6 +6175,9 @@ sub vm_resume {
>  	    # request before the backup finishes for example
>  	    mon_cmd($vmid, "system_reset");
>  	}
> +
> +	add_nets_bridge_fdb($conf, $vmid) if $resume_cmd eq 'cont';
> +
>  	mon_cmd($vmid, $resume_cmd);
>      });
>  }
> @@ -8237,4 +8240,22 @@ sub check_volume_storage_type {
>      return 1;
>  }
>  
> +sub add_nets_bridge_fdb {
> +    my ($conf, $vmid) = @_;
> +
> +    foreach my $opt (keys %$conf) {
> +	if ($opt =~  m/^net(\d+)$/) {
> +	    my $net = parse_net($conf->{$opt});
> +	    next if !$net;
> +	    next if !$net->{macaddr};

note that parse_net *always* checks for a $net->{macaddr} itself and auto_generates
one if not present:
https://git.proxmox.com/?p=qemu-server.git;a=blob;f=PVE/QemuServer.pm;h=2376bf46439e300ec5e0191f3bd9552510e91467;hb=HEAD#l1943

So this will never call next and register potentially a random mac address that then
isn't used by the VM.

I think it could be better to change this method to not get the full config but the
already parsed network entry, as then we can better ensure that the MAC address is
actually the one we'll use.

We'd then need to keep track of such auto-generated ones so that we got that available
when we actually need to add the entry, or read that out again from the QEMU command
line opts (or maybe its available via QMP?)

> +
> +	    my $iface = "tap${vmid}i$1";
> +	    if ($have_sdn) {
> +		PVE::Network::SDN::Zones::add_bridge_fdb($iface, $net->{macaddr}, $net->{bridge}, $net->{firewall});
> +	    } else {
> +		PVE::Network::add_bridge_fdb($iface, $net->{macaddr}, $net->{firewall});
> +	    }
> +	}
> +    }
> +}
>  1;

[pve-devel] applied: [PATCH V2 pve-network 1/1] bridge-disable-mac-learning : use $opts for tap_plug

[Thomas Lamprecht]

Am 24/08/2022 um 18:26 schrieb Alexandre Derumier:
> Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
> ---
>  PVE/Network/SDN/Zones.pm        | 5 +++--
>  PVE/Network/SDN/Zones/Plugin.pm | 4 +++-
>  2 files changed, 6 insertions(+), 3 deletions(-)
> 
>

applied this one for now to unbreak calling tap_plug which now expects an hash, thanks!

Re: [pve-devel] [PATCH qemu-server/container/network 0/3] implement bridge disable learning

[Thomas Lamprecht]

fyi, I now went the "auto-detect" in pve-common route and recorded a Breaks in
d/control for older qemu-server/pve-container that do not yet manually add the
MAC to the fdb themselves.

Would be great if you could do a quick sanity check; if it looks ok I'd also bump
pve-network (but common also has a compat change for accepting the old learning
option too, so not much hurry from that regard).