[pve-devel] [PATCH v4 container/manager 0/3] fix #3413: Add `Disconnect` option for LXC networks

[pve-devel] [PATCH v4 container/manager 0/3] fix #3413: Add `Disconnect` option for LXC networks

[Christoph Heiss]

Add a `Disconnect` option for network interfaces on LXC containers, much
like it already exists for VMs. This has been requested in #3413 [0] and
seems useful, especially considering we already support the same thing
for VMs.

One thing to note is that LXC does not seem to support the notion of
setting an interface down. The `flags` property would suggest that this
possible [1], but AFAICS it does not work. I tried setting the value as
empty and to something else than "up" (since that is really the only
supported option [2][3]), which both had absolutely no effect.

Thus force the host-side link of the container network down and avoid
adding it to the designated bridge if the new option is set, effectively
disconnecting the container network.

The first patch is cleanup only and does not change anything regarding
functionality.

Testing
-------
Testing was done by starting a LXC container (w/ and w/o `link_down`
set), checking if the interface has (or not) LOWERLAYERDOWN set inside
the container (`ip address eth0`) and if packet transit works (or not)
using a simple `ping`. Same thing after toggeling the option on the
interface. Further, the interface(s) should (or should not) be listed
in `brctl show`. Same thing was done for hotplugged interfaces to a
running container.

Also tested with `ifreload -a` (thanks Wolfgang!) thrown in, which did
nothing unexpected: If `link_down` was set, interfaces stayed in
LOWERLAYERDOWN and unplugged from the bridge, and stayed UP and plugged
into the bridge when `link_down` was unset.

[0] https://bugzilla.proxmox.com/show_bug.cgi?id=3413
[1] https://linuxcontainers.org/lxc/manpages/man5/lxc.container.conf.5.html#lbAO
[2] https://github.com/lxc/lxc/blob/08f0e769/src/lxc/confile.c#L453-L467
[3] https://github.com/lxc/lxc/blob/08f0e769/src/lxc/confile.c#L5933-L5952

v1: https://lists.proxmox.com/pipermail/pve-devel/2023-February/055762.html
v2: https://lists.proxmox.com/pipermail/pve-devel/2023-February/055795.html
v3: https://lists.proxmox.com/pipermail/pve-devel/2023-February/055839.html

pve-container:

Christoph Heiss (2):
      net: Pass network config directly to net_tap_plug()
      net: Add `link_down` config to allow setting interfaces as disconnected

 src/PVE/LXC.pm        | 37 +++++++++++++++++++++++--------------
 src/PVE/LXC/Config.pm |  6 ++++++
 src/lxcnetaddbr       |  9 +--------
 3 files changed, 30 insertions(+), 22 deletions(-)

pve-manager:

Christoph Heiss (1):
      lxc: Add `Disconnect` option for network interfaces

 www/manager6/Parser.js      |  3 +++
 www/manager6/lxc/Network.js | 13 +++++++++++++
 2 files changed, 16 insertions(+)

--
2.39.1

[pve-devel] [PATCH v4 container 1/3] net: Pass network config directly to net_tap_plug()

[Christoph Heiss]

No functional changes.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Changes v3 -> v4:
 * New patch

A note: The check in net_tap_plug() whether the MAC address is actually
passed in $opts is removed, as this was always the case anyway.

 src/PVE/LXC.pm  | 20 +++++++++-----------
 src/lxcnetaddbr |  9 +--------
 2 files changed, 10 insertions(+), 19 deletions(-)

diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
index 54ee0d9..54afd97 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -918,15 +918,16 @@ sub vm_stop_cleanup {
     warn $@ if $@; # avoid errors - just warn
 }

-sub net_tap_plug : prototype($$$$$$;$) {
-    my ($iface, $bridge, $tag, $firewall, $trunks, $rate, $opts) = @_;
+sub net_tap_plug : prototype($$) {
+    my ($iface, $net) = @_;
+    my ($bridge, $tag, $firewall, $trunks, $rate, $hwaddr) =
+	$net->@{'bridge', 'tag', 'firewall', 'trunks', 'rate', 'hwaddr'};

     if ($have_sdn) {
 	PVE::Network::SDN::Zones::tap_plug($iface, $bridge, $tag, $firewall, $trunks, $rate);
-	PVE::Network::SDN::Zones::add_bridge_fdb($iface, $opts->{mac}, $bridge, $firewall)
-	    if defined($opts->{mac});
+	PVE::Network::SDN::Zones::add_bridge_fdb($iface, $hwaddr, $bridge, $firewall);
     } else {
-	PVE::Network::tap_plug($iface, $bridge, $tag, $firewall, $trunks, $rate, $opts);
+	PVE::Network::tap_plug($iface, $bridge, $tag, $firewall, $trunks, $rate, { mac => $hwaddr });
     }
 }

@@ -968,8 +969,7 @@ sub update_net {
 		    PVE::LXC::Config->write_config($vmid, $conf);
 		}

-		my ($bridge, $mac, $firewall, $rate) = $newnet->@{'bridge', 'hwaddr', 'firewall', 'rate'};
-		PVE::LXC::net_tap_plug($veth, $bridge, $newnet->{tag}, $firewall, $newnet->{trunks}, $rate, { mac => $mac });
+		PVE::LXC::net_tap_plug($veth, $newnet);

 		# This includes the rate:
 		foreach (qw(bridge tag firewall rate)) {
@@ -1003,10 +1003,8 @@ sub hotplug_net {
     } else {
 	PVE::Network::veth_create($veth, $vethpeer, $newnet->{bridge}, $newnet->{hwaddr});
     }
-    PVE::LXC::net_tap_plug(
-        $veth, $newnet->{bridge}, $newnet->{tag}, $newnet->{firewall}, $newnet->{trunks},
-        $newnet->{rate}, { mac => $newnet->{hwaddr} },
-    );
+
+    PVE::LXC::net_tap_plug($veth, $newnet);

     # attach peer in container
     my $cmd = ['lxc-device', '-n', $vmid, 'add', $vethpeer, "$eth" ];
diff --git a/src/lxcnetaddbr b/src/lxcnetaddbr
index ebd6baa..f2db433 100755
--- a/src/lxcnetaddbr
+++ b/src/lxcnetaddbr
@@ -36,13 +36,7 @@ die "unable to find network definition for interface '$iface'\n"

 my $net = PVE::LXC::Config->parse_lxc_network($netconf);

-my $tag = $net->{tag};
-my $firewall = $net->{firewall};
 my $bridge = $net->{bridge};
-my $trunks = $net->{trunks};
-my $rate = $net->{rate};
-my $hwaddr = $net->{hwaddr};
-
 die "missing bridge configuration" if !$bridge;

 if (-d "/sys/class/net/$iface") {
@@ -54,8 +48,7 @@ if (-d "/sys/class/net/$iface") {

     PVE::Tools::run_command("/sbin/ip link set dev $iface up mtu $bridgemtu");
     PVE::Tools::run_command("/sbin/ip addr add 0.0.0.0/0 dev $iface");
-
-    PVE::LXC::net_tap_plug($iface, $bridge, $tag, $firewall, $trunks, $rate, { mac => $hwaddr });
+    PVE::LXC::net_tap_plug($iface, $net);
 }

 exit 0;
--
2.39.1

[pve-devel] [PATCH v4 container 2/3] net: Add `link_down` config to allow setting interfaces as disconnected

[Christoph Heiss]

If this network option is set, the host-side link will be forced down
and the interface won't be connected to the bridge.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Changes v1 -> v2:
 * Split trailing whitespace fix into separate patch
 * Rename option to kebap-case
 * Proper option comparison using `safe_boolean_ne`
 * Copy option to new network conf like the other options
 * Remove the veth interface from the bridge when disconnected

Changes v2 -> v3:
 * Rename option to snake_case again
 * Move option hotplug-handling before LXC attach again

Changes v3 -> v4:
 * Rebase
 * Shorten and remove some comments as appropriate
 * Update `link_down` schema comment
 * Move `link_down` logic to net_tap_plug()

A note regarding the last change:
The interface is now always set UP if `link_down` is unset. This saves
us from passing the old network configuration to net_tap_plug() and
should not have any effect as setting an interface UP/DOWN is
(hopefully?) idempotent anyway - if it already is UP it does nothing and
if it is currently DOWN we want it UP anyway at that point.

 src/PVE/LXC.pm        | 17 ++++++++++++++---
 src/PVE/LXC/Config.pm |  6 ++++++
 2 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
index 54afd97..c4d53e8 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -920,6 +920,14 @@ sub vm_stop_cleanup {

 sub net_tap_plug : prototype($$) {
     my ($iface, $net) = @_;
+
+    if (defined($net->{link_down})) {
+	PVE::Tools::run_command(['/sbin/ip', 'link', 'set', 'dev', $iface, 'down']);
+	# Don't add disconnected interfaces to the bridge, otherwise e.g. applying any network
+	# change (e.g. `ifreload -a`) could (re-)activate it unintentionally.
+	return;
+    }
+
     my ($bridge, $tag, $firewall, $trunks, $rate, $hwaddr) =
 	$net->@{'bridge', 'tag', 'firewall', 'trunks', 'rate', 'hwaddr'};

@@ -929,6 +937,8 @@ sub net_tap_plug : prototype($$) {
     } else {
 	PVE::Network::tap_plug($iface, $bridge, $tag, $firewall, $trunks, $rate, { mac => $hwaddr });
     }
+
+    PVE::Tools::run_command(['/sbin/ip', 'link', 'set', 'dev', $iface, 'up']);
 }

 sub update_net {
@@ -957,7 +967,8 @@ sub update_net {
 	} else {
 	    if (safe_string_ne($oldnet->{bridge}, $newnet->{bridge}) ||
 		safe_num_ne($oldnet->{tag}, $newnet->{tag}) ||
-		safe_num_ne($oldnet->{firewall}, $newnet->{firewall})
+		safe_num_ne($oldnet->{firewall}, $newnet->{firewall}) ||
+		safe_boolean_ne($oldnet->{link_down}, $newnet->{link_down})
 	    ) {

 		if ($oldnet->{bridge}) {
@@ -972,7 +983,7 @@ sub update_net {
 		PVE::LXC::net_tap_plug($veth, $newnet);

 		# This includes the rate:
-		foreach (qw(bridge tag firewall rate)) {
+		foreach (qw(bridge tag firewall rate link_down)) {
 		    $oldnet->{$_} = $newnet->{$_} if $newnet->{$_};
 		}
 	    } elsif (safe_string_ne($oldnet->{rate}, $newnet->{rate})) {
@@ -1015,7 +1026,7 @@ sub hotplug_net {
     PVE::Tools::run_command($cmd);

     my $done = { type => 'veth' };
-    foreach (qw(bridge tag firewall hwaddr name)) {
+    foreach (qw(bridge tag firewall hwaddr name link_down)) {
 	$done->{$_} = $newnet->{$_} if $newnet->{$_};
     }
     $conf->{$opt} = PVE::LXC::Config->print_lxc_network($done);
diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
index af25a96..bf424f9 100644
--- a/src/PVE/LXC/Config.pm
+++ b/src/PVE/LXC/Config.pm
@@ -814,6 +814,12 @@ our $netconf_desc = {
 	description => "Apply rate limiting to the interface",
 	optional => 1,
     },
+    # TODO: Rename this option and the qemu-server one to `link-down` for PVE 8.0
+    link_down => {
+	type => 'boolean',
+	description => 'Whether this interface should be disconnected (like pulling the plug).',
+	optional => 1,
+    },
 };
 PVE::JSONSchema::register_format('pve-lxc-network', $netconf_desc);

--
2.39.1

[pve-devel] [PATCH v4 manager 3/3] lxc: Add `Disconnect` option for network interfaces

[Christoph Heiss]

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Changes v1 -> v2:
 * Rename option to kebap-case

Changes v2 -> v3:
 * Rename option to snake_case again

Changes v3 -> v4:
 * Rebase

Q: Since this depends on an updated API endpoint from pve-container, the
minimal required 'Depends' version for pve-container probably needs a
bump? How is something like this usually handled?

 www/manager6/Parser.js      |  3 +++
 www/manager6/lxc/Network.js | 13 +++++++++++++
 2 files changed, 16 insertions(+)

diff --git a/www/manager6/Parser.js b/www/manager6/Parser.js
index 9f7b2c84..c3772d3b 100644
--- a/www/manager6/Parser.js
+++ b/www/manager6/Parser.js
@@ -298,6 +298,8 @@ Ext.define('PVE.Parser', {
 		data[match_res[1]] = match_res[2];
 	    } else if ((match_res = p.match(/^firewall=(\d+)$/)) !== null) {
 		data.firewall = PVE.Parser.parseBoolean(match_res[1]);
+	    } else if ((match_res = p.match(/^link_down=(\d+)$/)) !== null) {
+		data.link_down = PVE.Parser.parseBoolean(match_res[1]);
 	    } else if (!p.match(/^type=\S+$/)) {
 		console.warn(`could not parse LXC network string ${p}`);
 	    }
@@ -319,6 +321,7 @@ Ext.define('PVE.Parser', {
 	    name: 1,
 	    rate: 1,
 	    tag: 1,
+	    link_down: 1,
 	};
 	return Object.entries(config)
 	    .filter(([k, v]) => v !== undefined && v !== '' && knownKeys[k])
diff --git a/www/manager6/lxc/Network.js b/www/manager6/lxc/Network.js
index 85033bd8..b2cd9410 100644
--- a/www/manager6/lxc/Network.js
+++ b/www/manager6/lxc/Network.js
@@ -273,6 +273,12 @@ Ext.define('PVE.lxc.NetworkInputPanel', {
 	];

 	me.advancedColumn1 = [
+	    {
+		xtype: 'proxmoxcheckbox',
+		fieldLabel: gettext('Disconnect'),
+		name: 'link_down',
+		value: cdata.link_down,
+	    },
 	    {
 		xtype: 'proxmoxintegerfield',
 		fieldLabel: 'MTU',
@@ -539,6 +545,12 @@ Ext.define('PVE.lxc.NetworkView', {
 		    width: 80,
 		    dataIndex: 'mtu',
 		},
+		{
+		    header: gettext('Disconnected'),
+		    width: 100,
+		    dataIndex: 'link_down',
+		    renderer: Proxmox.Utils.format_boolean,
+		},
 	    ],
 	    listeners: {
 		activate: me.load,
@@ -564,6 +576,7 @@ Ext.define('PVE.lxc.NetworkView', {
 	    'tag',
 	    'firewall',
 	    'mtu',
+	    'link_down',
 	],
     });
 });
--
2.39.1

Re: [pve-devel] [PATCH v4 container/manager 0/3] fix #3413: Add `Disconnect` option for LXC networks

[Friedrich Weber]

As I also missed that feature, I applied the patches to my PVE instance 
with pre-existing containers -- all interfaces stayed up as expected, 
and disconnecting/reconnecting interfaces for running and stopped 
containers via the Web UI worked nicely.

Tested-by: Friedrich Weber <f.weber@proxmox.com>

On 22/02/2023 13:49, Christoph Heiss wrote:
> Add a `Disconnect` option for network interfaces on LXC containers, much
> like it already exists for VMs. This has been requested in #3413 [0] and
> seems useful, especially considering we already support the same thing
> for VMs.
> 
> One thing to note is that LXC does not seem to support the notion of
> setting an interface down. The `flags` property would suggest that this
> possible [1], but AFAICS it does not work. I tried setting the value as
> empty and to something else than "up" (since that is really the only
> supported option [2][3]), which both had absolutely no effect.
> 
> Thus force the host-side link of the container network down and avoid
> adding it to the designated bridge if the new option is set, effectively
> disconnecting the container network.
> 
> The first patch is cleanup only and does not change anything regarding
> functionality.
> 
> Testing
> -------
> Testing was done by starting a LXC container (w/ and w/o `link_down`
> set), checking if the interface has (or not) LOWERLAYERDOWN set inside
> the container (`ip address eth0`) and if packet transit works (or not)
> using a simple `ping`. Same thing after toggeling the option on the
> interface. Further, the interface(s) should (or should not) be listed
> in `brctl show`. Same thing was done for hotplugged interfaces to a
> running container.
> 
> Also tested with `ifreload -a` (thanks Wolfgang!) thrown in, which did
> nothing unexpected: If `link_down` was set, interfaces stayed in
> LOWERLAYERDOWN and unplugged from the bridge, and stayed UP and plugged
> into the bridge when `link_down` was unset.
> 
> [0] https://bugzilla.proxmox.com/show_bug.cgi?id=3413
> [1] https://linuxcontainers.org/lxc/manpages/man5/lxc.container.conf.5.html#lbAO
> [2] https://github.com/lxc/lxc/blob/08f0e769/src/lxc/confile.c#L453-L467
> [3] https://github.com/lxc/lxc/blob/08f0e769/src/lxc/confile.c#L5933-L5952
> 
> v1: https://lists.proxmox.com/pipermail/pve-devel/2023-February/055762.html
> v2: https://lists.proxmox.com/pipermail/pve-devel/2023-February/055795.html
> v3: https://lists.proxmox.com/pipermail/pve-devel/2023-February/055839.html
> 
> pve-container:
> 
> Christoph Heiss (2):
>        net: Pass network config directly to net_tap_plug()
>        net: Add `link_down` config to allow setting interfaces as disconnected
> 
>   src/PVE/LXC.pm        | 37 +++++++++++++++++++++++--------------
>   src/PVE/LXC/Config.pm |  6 ++++++
>   src/lxcnetaddbr       |  9 +--------
>   3 files changed, 30 insertions(+), 22 deletions(-)
> 
> pve-manager:
> 
> Christoph Heiss (1):
>        lxc: Add `Disconnect` option for network interfaces
> 
>   www/manager6/Parser.js      |  3 +++
>   www/manager6/lxc/Network.js | 13 +++++++++++++
>   2 files changed, 16 insertions(+)
> 
> --
> 2.39.1
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 

[pve-devel] applied: [PATCH v4 container/manager 0/3] fix #3413: Add `Disconnect` option for LXC networks

[Thomas Lamprecht]

Am 22/02/2023 um 13:49 schrieb Christoph Heiss:
> Add a `Disconnect` option for network interfaces on LXC containers, much
> like it already exists for VMs. This has been requested in #3413 [0] and
> seems useful, especially considering we already support the same thing
> for VMs.
> 
> One thing to note is that LXC does not seem to support the notion of
> setting an interface down. The `flags` property would suggest that this
> possible [1], but AFAICS it does not work. I tried setting the value as
> empty and to something else than "up" (since that is really the only
> supported option [2][3]), which both had absolutely no effect.
> 
> Thus force the host-side link of the container network down and avoid
> adding it to the designated bridge if the new option is set, effectively
> disconnecting the container network.
> 
> The first patch is cleanup only and does not change anything regarding
> functionality.
> 
> Testing
> -------
> Testing was done by starting a LXC container (w/ and w/o `link_down`
> set), checking if the interface has (or not) LOWERLAYERDOWN set inside
> the container (`ip address eth0`) and if packet transit works (or not)
> using a simple `ping`. Same thing after toggeling the option on the
> interface. Further, the interface(s) should (or should not) be listed
> in `brctl show`. Same thing was done for hotplugged interfaces to a
> running container.
> 
> Also tested with `ifreload -a` (thanks Wolfgang!) thrown in, which did
> nothing unexpected: If `link_down` was set, interfaces stayed in
> LOWERLAYERDOWN and unplugged from the bridge, and stayed UP and plugged
> into the bridge when `link_down` was unset.
> 
> [0] https://bugzilla.proxmox.com/show_bug.cgi?id=3413
> [1] https://linuxcontainers.org/lxc/manpages/man5/lxc.container.conf.5.html#lbAO
> [2] https://github.com/lxc/lxc/blob/08f0e769/src/lxc/confile.c#L453-L467
> [3] https://github.com/lxc/lxc/blob/08f0e769/src/lxc/confile.c#L5933-L5952
> 
> v1: https://lists.proxmox.com/pipermail/pve-devel/2023-February/055762.html
> v2: https://lists.proxmox.com/pipermail/pve-devel/2023-February/055795.html
> v3: https://lists.proxmox.com/pipermail/pve-devel/2023-February/055839.html
> 
> pve-container:
> 
> Christoph Heiss (2):
>       net: Pass network config directly to net_tap_plug()
>       net: Add `link_down` config to allow setting interfaces as disconnected
> 
>  src/PVE/LXC.pm        | 37 +++++++++++++++++++++++--------------
>  src/PVE/LXC/Config.pm |  6 ++++++
>  src/lxcnetaddbr       |  9 +--------
>  3 files changed, 30 insertions(+), 22 deletions(-)
> 


applied above two, with the relevant bits of the cover letter added to the commit
message of the second container patch, thanks!

[pve-devel] applied: [PATCH v4 manager 3/3] lxc: Add `Disconnect` option for network interfaces

[Wolfgang Bumiller]

applied gui patch, thanks