* [pve-devel] [PATCH V2 pve-network 1/1] bridge-disable-mac-learning : use $opts for tap_plug
2022-08-24 16:26 [pve-devel] [PATCH qemu-server/container/network 0/3] implement bridge disable learning Alexandre Derumier
@ 2022-08-24 16:26 ` Alexandre Derumier
2022-11-13 9:46 ` [pve-devel] applied: " Thomas Lamprecht
2022-08-24 16:26 ` [pve-devel] [PATCH V3 pve-container 1/1] net : add support for bridge disable mac learning Alexandre Derumier
` (4 subsequent siblings)
5 siblings, 1 reply; 15+ messages in thread
From: Alexandre Derumier @ 2022-08-24 16:26 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 15+ messages in thread* [pve-devel] [PATCH V3 pve-container 1/1] net : add support for bridge disable mac learning
2022-08-24 16:26 [pve-devel] [PATCH qemu-server/container/network 0/3] implement bridge disable learning Alexandre Derumier
2022-08-24 16:26 ` [pve-devel] [PATCH V2 pve-network 1/1] bridge-disable-mac-learning : use $opts for tap_plug Alexandre Derumier
@ 2022-08-24 16:26 ` Alexandre Derumier
2022-08-24 16:26 ` [pve-devel] [PATCH V3 qemu-server 1/3] tap_plug: add support for bridge disable learning Alexandre Derumier
` (3 subsequent siblings)
5 siblings, 0 replies; 15+ messages in thread
From: Alexandre Derumier @ 2022-08-24 16:26 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 15+ messages in thread* [pve-devel] [PATCH V3 qemu-server 1/3] tap_plug: add support for bridge disable learning
2022-08-24 16:26 [pve-devel] [PATCH qemu-server/container/network 0/3] implement bridge disable learning Alexandre Derumier
2022-08-24 16:26 ` [pve-devel] [PATCH V2 pve-network 1/1] bridge-disable-mac-learning : use $opts for tap_plug Alexandre Derumier
2022-08-24 16:26 ` [pve-devel] [PATCH V3 pve-container 1/1] net : add support for bridge disable mac learning Alexandre Derumier
@ 2022-08-24 16:26 ` Alexandre Derumier
2022-11-09 14:19 ` Mira Limbeck
2022-11-12 16:24 ` Thomas Lamprecht
2022-08-24 16:26 ` [pve-devel] [PATCH V3 qemu-server 2/3] vm_start/vm_resume : add_nets_bridge_fdb Alexandre Derumier
` (2 subsequent siblings)
5 siblings, 2 replies; 15+ messages in thread
From: Alexandre Derumier @ 2022-08-24 16:26 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [pve-devel] [PATCH V3 qemu-server 1/3] tap_plug: add support for bridge disable learning
2022-08-24 16:26 ` [pve-devel] [PATCH V3 qemu-server 1/3] tap_plug: add support for bridge disable learning Alexandre Derumier
@ 2022-11-09 14:19 ` Mira Limbeck
2022-11-11 8:36 ` DERUMIER, Alexandre
2022-11-12 16:24 ` Thomas Lamprecht
1 sibling, 1 reply; 15+ messages in thread
From: Mira Limbeck @ 2022-11-09 14:19 UTC (permalink / raw)
To: pve-devel
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?
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [pve-devel] [PATCH V3 qemu-server 1/3] tap_plug: add support for bridge disable learning
2022-11-09 14:19 ` Mira Limbeck
@ 2022-11-11 8:36 ` DERUMIER, Alexandre
2022-11-11 9:05 ` Mira Limbeck
0 siblings, 1 reply; 15+ messages in thread
From: DERUMIER, Alexandre @ 2022-11-11 8:36 UTC (permalink / raw)
To: pve-devel
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)
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [pve-devel] [PATCH V3 qemu-server 1/3] tap_plug: add support for bridge disable learning
2022-11-11 8:36 ` DERUMIER, Alexandre
@ 2022-11-11 9:05 ` Mira Limbeck
0 siblings, 0 replies; 15+ messages in thread
From: Mira Limbeck @ 2022-11-11 9:05 UTC (permalink / raw)
To: pve-devel
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.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [pve-devel] [PATCH V3 qemu-server 1/3] tap_plug: add support for bridge disable learning
2022-08-24 16:26 ` [pve-devel] [PATCH V3 qemu-server 1/3] tap_plug: add support for bridge disable learning Alexandre Derumier
2022-11-09 14:19 ` Mira Limbeck
@ 2022-11-12 16:24 ` Thomas Lamprecht
1 sibling, 0 replies; 15+ messages in thread
From: Thomas Lamprecht @ 2022-11-12 16:24 UTC (permalink / raw)
To: Proxmox VE development discussion, Alexandre Derumier
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.
^ permalink raw reply [flat|nested] 15+ messages in thread
* [pve-devel] [PATCH V3 qemu-server 2/3] vm_start/vm_resume : add_nets_bridge_fdb
2022-08-24 16:26 [pve-devel] [PATCH qemu-server/container/network 0/3] implement bridge disable learning Alexandre Derumier
` (2 preceding siblings ...)
2022-08-24 16:26 ` [pve-devel] [PATCH V3 qemu-server 1/3] tap_plug: add support for bridge disable learning Alexandre Derumier
@ 2022-08-24 16:26 ` Alexandre Derumier
2022-11-12 16:32 ` Thomas Lamprecht
2022-08-24 16:26 ` [pve-devel] [PATCH V3 qemu-server 3/3] migration : add del_nets_bridge_fdb Alexandre Derumier
2022-11-13 15:10 ` [pve-devel] [PATCH qemu-server/container/network 0/3] implement bridge disable learning Thomas Lamprecht
5 siblings, 1 reply; 15+ messages in thread
From: Alexandre Derumier @ 2022-08-24 16:26 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [pve-devel] [PATCH V3 qemu-server 2/3] vm_start/vm_resume : add_nets_bridge_fdb
2022-08-24 16:26 ` [pve-devel] [PATCH V3 qemu-server 2/3] vm_start/vm_resume : add_nets_bridge_fdb Alexandre Derumier
@ 2022-11-12 16:32 ` Thomas Lamprecht
0 siblings, 0 replies; 15+ messages in thread
From: Thomas Lamprecht @ 2022-11-12 16:32 UTC (permalink / raw)
To: Proxmox VE development discussion, Alexandre Derumier
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;
^ permalink raw reply [flat|nested] 15+ messages in thread
* [pve-devel] [PATCH V3 qemu-server 3/3] migration : add del_nets_bridge_fdb
2022-08-24 16:26 [pve-devel] [PATCH qemu-server/container/network 0/3] implement bridge disable learning Alexandre Derumier
` (3 preceding siblings ...)
2022-08-24 16:26 ` [pve-devel] [PATCH V3 qemu-server 2/3] vm_start/vm_resume : add_nets_bridge_fdb Alexandre Derumier
@ 2022-08-24 16:26 ` Alexandre Derumier
2022-11-07 12:41 ` Mira Limbeck
2022-11-13 15:10 ` [pve-devel] [PATCH qemu-server/container/network 0/3] implement bridge disable learning Thomas Lamprecht
5 siblings, 1 reply; 15+ messages in thread
From: Alexandre Derumier @ 2022-08-24 16:26 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [pve-devel] [PATCH V3 qemu-server 3/3] migration : add del_nets_bridge_fdb
2022-08-24 16:26 ` [pve-devel] [PATCH V3 qemu-server 3/3] migration : add del_nets_bridge_fdb Alexandre Derumier
@ 2022-11-07 12:41 ` Mira Limbeck
2022-11-10 8:12 ` DERUMIER, Alexandre
0 siblings, 1 reply; 15+ messages in thread
From: Mira Limbeck @ 2022-11-07 12:41 UTC (permalink / raw)
To: pve-devel
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?
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [pve-devel] [PATCH V3 qemu-server 3/3] migration : add del_nets_bridge_fdb
2022-11-07 12:41 ` Mira Limbeck
@ 2022-11-10 8:12 ` DERUMIER, Alexandre
0 siblings, 0 replies; 15+ messages in thread
From: DERUMIER, Alexandre @ 2022-11-10 8:12 UTC (permalink / raw)
To: pve-devel
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
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [pve-devel] [PATCH qemu-server/container/network 0/3] implement bridge disable learning
2022-08-24 16:26 [pve-devel] [PATCH qemu-server/container/network 0/3] implement bridge disable learning Alexandre Derumier
` (4 preceding siblings ...)
2022-08-24 16:26 ` [pve-devel] [PATCH V3 qemu-server 3/3] migration : add del_nets_bridge_fdb Alexandre Derumier
@ 2022-11-13 15:10 ` Thomas Lamprecht
5 siblings, 0 replies; 15+ messages in thread
From: Thomas Lamprecht @ 2022-11-13 15:10 UTC (permalink / raw)
To: Proxmox VE development discussion, Alexandre Derumier
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).
^ permalink raw reply [flat|nested] 15+ messages in thread