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

[pve-devel] [PATCH v3 container/manager 0/4] 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 two patches are cleanup-patches only and do 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

pve-container:

Christoph Heiss (3):
      lxc: Fix some trailing whitespace
      lxc: Avoid open-coding normal vs SDN-specific tap_plug()
      lxc: Add `link_down` config to allow setting interfaces as disconnected

 src/PVE/LXC.pm        | 41 ++++++++++++++++++++++++++++++++++-------
 src/PVE/LXC/Config.pm |  6 ++++++
 src/lxcnetaddbr       |  7 +++++--
 3 files changed, 45 insertions(+), 9 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 v3 container 1/4] lxc: Fix some trailing whitespace

[Christoph Heiss]

No functional changes.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Changes v1 -> v2:
 * New patch, split out from patch 2

Changes v2 -> v3:
 * Fix botched whitespace patch

 src/PVE/LXC.pm | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
index 7cf1dcf..cbbb82d 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -669,7 +669,7 @@ sub update_lxc_config {
 
     # some init scripts expect a linux terminal (turnkey).
     $raw .= "lxc.environment = TERM=linux\n";
-    
+
     my $utsname = $conf->{hostname} || "CT$vmid";
     $raw .= "lxc.uts.name = $utsname\n";
 
@@ -1704,14 +1704,14 @@ sub __mountpoint_mount {
     my $type = $mountpoint->{type};
     my $quota = !$snapname && !$mountpoint->{ro} && $mountpoint->{quota};
     my $mounted_dev;
-    
+
     return if !$volid || !$mount;
 
     $mount =~ s!/+!/!g;
 
     my $mount_path;
     my ($mpfd, $parentfd, $last_dir);
-    
+
     if (defined($rootdir)) {
 	($rootdir, $mount_path, $mpfd, $parentfd, $last_dir) =
 	    __mount_prepare_rootdir($rootdir, $mount, $rootuid, $rootgid);
@@ -1720,7 +1720,7 @@ sub __mountpoint_mount {
     if (defined($stage_mount)) {
 	$mount_path = $rootdir;
     }
-    
+
     my ($storage, $volname) = PVE::Storage::parse_volume_id($volid, 1);
 
     die "unknown snapshot path for '$volid'" if !$storage && defined($snapname);
@@ -1829,7 +1829,7 @@ sub __mountpoint_mount {
 	warn "cannot enable quota control for bind mounts\n" if $quota;
 	return wantarray ? ($volid, 0, undef) : $volid;
     }
-    
+
     die "unsupported storage";
 }
 
-- 
2.39.1

[pve-devel] [PATCH v3 container 2/4] lxc: Avoid open-coding normal vs SDN-specific tap_plug()

[Christoph Heiss]

This pattern is used in multiple places, thus just extract it into a sub
on its own.

No functional changes.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Might not be the best place for net_tap_plug(), putting this logic
inside PVE::Network would probably make more sense. But that would
entail a (bigger) refactoring, since it then also must be done for all
other tap_*() and veth_*() subroutines (and maybe some other things?)
for consistency..
In any case, that definitely would be too much for this series. I can do
that, but I'd do it as a follow-up series.

Changes v1 -> v2:
 * New patch

Changes v2 -> v3:
 * Add prototype to net_tap_plug()

 src/PVE/LXC.pm  | 28 ++++++++++++++++------------
 src/lxcnetaddbr | 15 ++-------------
 2 files changed, 18 insertions(+), 25 deletions(-)

diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
index cbbb82d..d419124 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -918,6 +918,18 @@ sub vm_stop_cleanup {
     warn $@ if $@; # avoid errors - just warn
 }

+sub net_tap_plug : prototype($$$$$$;$) {
+    my ($iface, $bridge, $tag, $firewall, $trunks, $rate, $opts) = @_;
+
+    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});
+    } else {
+	PVE::Network::tap_plug($iface, $bridge, $tag, $firewall, $trunks, $rate, $opts);
+    }
+}
+
 sub update_net {
     my ($vmid, $conf, $opt, $newnet, $netid, $rootdir) = @_;

@@ -957,14 +969,7 @@ sub update_net {
 		}

 		my ($bridge, $mac, $firewall, $rate) = $newnet->@{'bridge', 'hwaddr', 'firewall', 'rate'};
-		if ($have_sdn) {
-		    PVE::Network::SDN::Zones::tap_plug(
-		        $veth, $bridge, $newnet->{tag}, $firewall, $newnet->{trunks}, $rate);
-		    PVE::Network::SDN::Zones::add_bridge_fdb($veth, $mac, $bridge, $firewall);
-		} else {
-		    PVE::Network::tap_plug(
-		        $veth, $bridge, $newnet->{tag}, $firewall, $newnet->{trunks}, $rate, { mac => $mac });
-		}
+		PVE::LXC::net_tap_plug($veth, $bridge, $newnet->{tag}, $firewall, $newnet->{trunks}, $rate, { mac => $mac });

 		# This includes the rate:
 		foreach (qw(bridge tag firewall rate)) {
@@ -995,13 +1000,12 @@ 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 {
 	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::add_bridge_fdb($veth, $newnet->{hwaddr}, $newnet->{firewall}); # early returns if brport has learning on
     }
+    PVE::LXC::net_tap_plug(
+	$veth, $newnet->{bridge}, $newnet->{tag}, $newnet->{firewall}, $newnet->{trunks},
+	$newnet->{rate}, { mac => $newnet->{hwaddr} });

     # attach peer in container
     my $cmd = ['lxc-device', '-n', $vmid, 'add', $vethpeer, "$eth" ];
diff --git a/src/lxcnetaddbr b/src/lxcnetaddbr
index 83052e1..ebd6baa 100755
--- a/src/lxcnetaddbr
+++ b/src/lxcnetaddbr
@@ -7,15 +7,8 @@ exit 0 if $ENV{LXC_NAME} && $ENV{LXC_NAME} !~ /^\d+$/;

 use PVE::LXC;
 use PVE::Tools qw(run_command);
-use PVE::Network;
 use PVE::ProcFSTools;

-my $have_sdn;
-eval {
-    require PVE::Network::SDN::Zones;
-    $have_sdn = 1;
-};
-
 die "got unexpected argument count\n" if scalar(@ARGV) != 5;

 my ($vmid, $arg2, $arg3, $type, $iface) = @ARGV;
@@ -48,6 +41,7 @@ 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;

@@ -61,12 +55,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");

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

 exit 0;
--
2.39.1

[pve-devel] [PATCH v3 container 3/4] lxc: 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
 * Moved option hotplug-handling before LXC attach again

 src/PVE/LXC.pm        | 41 ++++++++++++++++++++++++++++++++++-------
 src/PVE/LXC/Config.pm |  6 ++++++
 src/lxcnetaddbr       |  7 +++++--
 3 files changed, 45 insertions(+), 9 deletions(-)

diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
index d419124..2c10108 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -956,7 +956,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}) {
@@ -969,10 +970,28 @@ sub update_net {
 		}

 		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 });
+
+		if (defined($newnet->{link_down})) {
+		    # The interface must not be connected to the designated
+		    # bridge if the link was requested to be disconnected.
+		    # Otherwise it could get re-enabled by something like
+		    # `ifreload`.
+		    #
+		    # Thus only force the host-side link down here and skip
+		    # adding it to the bridge.
+		    PVE::Tools::run_command(['/sbin/ip', 'link', 'set', 'dev', $veth, 'down']);
+		} else {
+		    # Connect the interface to the bridge
+		    PVE::LXC::net_tap_plug(
+			$veth, $bridge, $newnet->{tag}, $firewall, $newnet->{trunks}, $rate, { mac => $mac });
+
+		    # Force the host-side link up if it was previously down.
+		    PVE::Tools::run_command(['/sbin/ip', 'link', 'set', 'dev', $veth, 'up'])
+			if defined($oldnet->{link_down});
+		}

 		# 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})) {
@@ -1003,9 +1022,17 @@ 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} });
+
+    if (defined($newnet->{link_down})) {
+	# In case the network device should be disconnected, force the host-link down ..
+	PVE::Tools::run_command(['/sbin/ip', 'link', 'set', 'dev', $veth, 'down']);
+    } else {
+	# .. otherwise, connect it normally to the bridge.
+	# The interface is already up from creation.
+	PVE::LXC::net_tap_plug(
+	    $veth, $newnet->{bridge}, $newnet->{tag}, $newnet->{firewall}, $newnet->{trunks},
+	    $newnet->{rate}, { mac => $newnet->{hwaddr} });
+    }

     # attach peer in container
     my $cmd = ['lxc-device', '-n', $vmid, 'add', $vethpeer, "$eth" ];
@@ -1016,7 +1043,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..746df7b 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 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);

diff --git a/src/lxcnetaddbr b/src/lxcnetaddbr
index ebd6baa..0940206 100755
--- a/src/lxcnetaddbr
+++ b/src/lxcnetaddbr
@@ -52,10 +52,13 @@ if (-d "/sys/class/net/$iface") {
     #avoid insecure dependency;
     ($bridgemtu) = $bridgemtu =~ /(\d+)/;

-    PVE::Tools::run_command("/sbin/ip link set dev $iface up mtu $bridgemtu");
+    my $linkstate = defined($net->{link_down}) ? 'down' : 'up';
+    PVE::Tools::run_command("/sbin/ip link set dev $iface $linkstate 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 });
+    # Only plug the interface into the bridge if it is not set as disconnected by the user.
+    PVE::LXC::net_tap_plug($iface, $bridge, $tag, $firewall, $trunks, $rate, { mac => $hwaddr })
+	if !defined($net->{link_down});
 }

 exit 0;
--
2.39.1

[pve-devel] [PATCH v3 manager 4/4] 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

 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

[pve-devel] applied: [PATCH v3 container 1/4] lxc: Fix some trailing whitespace

[Thomas Lamprecht]

Am 21/02/2023 um 09:05 schrieb Christoph Heiss:
> No functional changes.
> 
> Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
> ---
> Changes v1 -> v2:
>  * New patch, split out from patch 2
> 
> Changes v2 -> v3:
>  * Fix botched whitespace patch
> 
>  src/PVE/LXC.pm | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
>

applied, thanks!

[pve-devel] applied: [PATCH v3 container 2/4] lxc: Avoid open-coding normal vs SDN-specific tap_plug()

[Thomas Lamprecht]

Am 21/02/2023 um 09:05 schrieb Christoph Heiss:
> This pattern is used in multiple places, thus just extract it into a sub
> on its own.
> 
> No functional changes.
> 
> Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
> ---
> Might not be the best place for net_tap_plug(), putting this logic
> inside PVE::Network would probably make more sense. But that would
> entail a (bigger) refactoring, since it then also must be done for all
> other tap_*() and veth_*() subroutines (and maybe some other things?)
> for consistency..
> In any case, that definitely would be too much for this series. I can do
> that, but I'd do it as a follow-up series.
> 
> Changes v1 -> v2:
>  * New patch
> 
> Changes v2 -> v3:
>  * Add prototype to net_tap_plug()
> 
>  src/PVE/LXC.pm  | 28 ++++++++++++++++------------
>  src/lxcnetaddbr | 15 ++-------------
>  2 files changed, 18 insertions(+), 25 deletions(-)
> 

applied, thanks! But I got some feedback/question inline affecting patch 3/4

> diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
> index cbbb82d..d419124 100644
> --- a/src/PVE/LXC.pm
> +++ b/src/PVE/LXC.pm
> @@ -918,6 +918,18 @@ sub vm_stop_cleanup {
>      warn $@ if $@; # avoid errors - just warn
>  }
> 
> +sub net_tap_plug : prototype($$$$$$;$) {
> +    my ($iface, $bridge, $tag, $firewall, $trunks, $rate, $opts) = @_;

IMO having more than ~5 parameter is most of the time a code smell, and sure while we
ain't in rust where we can ensure some sane API and existence of struct/trait members
or methods it's not really that better to expand everything, as scalar on it's own is
way to broad anyway to guarantee anything relevant on calling.

So, maybe we could change this to take

sub net_tap_plug : prototype($$;$) {
    my ($iface, $net, $old_net) = @_;

as then we might even pull in the whole link_down logic separation in here and avoid
duplicating that then again (after just cleaning something similar like that up here).

What do you think?

Re: [pve-devel] [PATCH v3 container 3/4] lxc: Add `link_down` config to allow setting interfaces as disconnected

[Thomas Lamprecht]

Am 21/02/2023 um 09:05 schrieb 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
>  * Moved option hotplug-handling before LXC attach again

while this would work I'd like to avoid duplicating the link_down check logic
see my reply to patch 2/4

> 
>  src/PVE/LXC.pm        | 41 ++++++++++++++++++++++++++++++++++-------
>  src/PVE/LXC/Config.pm |  6 ++++++
>  src/lxcnetaddbr       |  7 +++++--
>  3 files changed, 45 insertions(+), 9 deletions(-)
> 
> diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
> index d419124..2c10108 100644
> --- a/src/PVE/LXC.pm
> +++ b/src/PVE/LXC.pm
> @@ -969,10 +970,28 @@ sub update_net {
>  		}
> 

from here until & including the if/else block could probably move to the net_tap_plug
helper, as then we could save repeating most of it in the hotplug_net call site.

>  		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 });
> +
> +		if (defined($newnet->{link_down})) {
> +		    # The interface must not be connected to the designated
> +		    # bridge if the link was requested to be disconnected.
> +		    # Otherwise it could get re-enabled by something like
> +		    # `ifreload`.
> +		    #
> +		    # Thus only force the host-side link down here and skip
> +		    # adding it to the bridge.

(new) comments should expand to 100cc and while highlighting that it could get
auto "UP'd" unintentionally otherwise is def. warranted here, I'd prefer a bit
more concise comments as above feels a bit redundant and crowds the function

A one, or maybe two liner should be enough to convey the basic hint, something
like:

# don't add a disabled iface to the bridge, otherwise e.g. appyling any network change
# (-> ifreload -a) could (re-)activate it unintentionally

> +		    PVE::Tools::run_command(['/sbin/ip', 'link', 'set', 'dev', $veth, 'down']);
> +		} else {
> +		    # Connect the interface to the bridge

IMO above comments is not adding that much "why" info

> +		    PVE::LXC::net_tap_plug(
> +			$veth, $bridge, $newnet->{tag}, $firewall, $newnet->{trunks}, $rate, { mac => $mac });
> +
> +		    # Force the host-side link up if it was previously down.
> +		    PVE::Tools::run_command(['/sbin/ip', 'link', 'set', 'dev', $veth, 'up'])
> +			if defined($oldnet->{link_down});
> +		}

> diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
> index af25a96..746df7b 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 to link-down for PVE 8.0

maybe highlight that VMs need to change too here, e.g.:

# TODO: rename this *and* the qemu-server one to [...]


> diff --git a/src/lxcnetaddbr b/src/lxcnetaddbr
> index ebd6baa..0940206 100755
> --- a/src/lxcnetaddbr
> +++ b/src/lxcnetaddbr

> -    PVE::LXC::net_tap_plug($iface, $bridge, $tag, $firewall, $trunks, $rate, { mac => $hwaddr });
> +    # Only plug the interface into the bridge if it is not set as disconnected by the user.

no hard feelings here but above also reads like the code tells us anyway, so could live
without it, but if you think it really helps I'm fine with that comment too.

> +    PVE::LXC::net_tap_plug($iface, $bridge, $tag, $firewall, $trunks, $rate, { mac => $hwaddr })
> +	if !defined($net->{link_down});

Re: [pve-devel] applied: [PATCH v3 container 2/4] lxc: Avoid open-coding normal vs SDN-specific tap_plug()

[Christoph Heiss]

Thanks!

On Tue, Feb 21, 2023 at 06:07:20PM +0100, Thomas Lamprecht wrote:
> Am 21/02/2023 um 09:05 schrieb Christoph Heiss:
> > [..]
> >
>
> applied, thanks! But I got some feedback/question inline affecting patch 3/4
>
> > diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
> > index cbbb82d..d419124 100644
> > --- a/src/PVE/LXC.pm
> > +++ b/src/PVE/LXC.pm
> > @@ -918,6 +918,18 @@ sub vm_stop_cleanup {
> >      warn $@ if $@; # avoid errors - just warn
> >  }
> >
> > +sub net_tap_plug : prototype($$$$$$;$) {
> > +    my ($iface, $bridge, $tag, $firewall, $trunks, $rate, $opts) = @_;
>
> IMO having more than ~5 parameter is most of the time a code smell, and sure while we
> ain't in rust where we can ensure some sane API and existence of struct/trait members
> or methods it's not really that better to expand everything, as scalar on it's own is
> way to broad anyway to guarantee anything relevant on calling.
I agree, it indeed is not all that sane. Plus it really clutters up all
the callsites.

>
> So, maybe we could change this to take
>
> sub net_tap_plug : prototype($$;$) {
>     my ($iface, $net, $old_net) = @_;
>
> as then we might even pull in the whole link_down logic separation in here and avoid
> duplicating that then again (after just cleaning something similar like that up here).
>
> What do you think?
Sounds like a very reasonable idea, especially being able to
de-duplicate the whole link_down logic later on. I didn't even notice
that I was basically introducing the same pattern again that I cleaned
up ..

I'll rework this a send a re-spin of patch 3/4 soon.

Re: [pve-devel] [PATCH v3 container 3/4] lxc: Add `link_down` config to allow setting interfaces as disconnected

[Christoph Heiss]

On Tue, Feb 21, 2023 at 06:25:39PM +0100, Thomas Lamprecht wrote:
> Am 21/02/2023 um 09:05 schrieb 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
> >  * Moved option hotplug-handling before LXC attach again
>
> while this would work I'd like to avoid duplicating the link_down check logic
> see my reply to patch 2/4
Ack, will do as said/discussed in the replies to patch 2.

>
> >
> >  src/PVE/LXC.pm        | 41 ++++++++++++++++++++++++++++++++++-------
> >  src/PVE/LXC/Config.pm |  6 ++++++
> >  src/lxcnetaddbr       |  7 +++++--
> >  3 files changed, 45 insertions(+), 9 deletions(-)
> >
> > diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
> > index d419124..2c10108 100644
> > --- a/src/PVE/LXC.pm
> > +++ b/src/PVE/LXC.pm
> > @@ -969,10 +970,28 @@ sub update_net {
> >  		}
> >
>
> from here until & including the if/else block could probably move to the net_tap_plug
> helper, as then we could save repeating most of it in the hotplug_net call site.
Ack.

>
> >  		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 });
> > +
> > +		if (defined($newnet->{link_down})) {
> > +		    # The interface must not be connected to the designated
> > +		    # bridge if the link was requested to be disconnected.
> > +		    # Otherwise it could get re-enabled by something like
> > +		    # `ifreload`.
> > +		    #
> > +		    # Thus only force the host-side link down here and skip
> > +		    # adding it to the bridge.
>
> (new) comments should expand to 100cc
Good to know! I wasn't sure about that while writing that, will change
that.

> and while highlighting that it could get
> auto "UP'd" unintentionally otherwise is def. warranted here, I'd prefer a bit
> more concise comments as above feels a bit redundant and crowds the function
>
> A one, or maybe two liner should be enough to convey the basic hint, something
> like:
>
> # don't add a disabled iface to the bridge, otherwise e.g. appyling any network change
> # (-> ifreload -a) could (re-)activate it unintentionally
Ack, I'll generally try to be more concise in the future with my
comments.

>
> > +		    PVE::Tools::run_command(['/sbin/ip', 'link', 'set', 'dev', $veth, 'down']);
> > +		} else {
> > +		    # Connect the interface to the bridge
>
> IMO above comments is not adding that much "why" info
I'll remove it, I agree that it does not add much value.

>
> > +		    PVE::LXC::net_tap_plug(
> > +			$veth, $bridge, $newnet->{tag}, $firewall, $newnet->{trunks}, $rate, { mac => $mac });
> > +
> > +		    # Force the host-side link up if it was previously down.
> > +		    PVE::Tools::run_command(['/sbin/ip', 'link', 'set', 'dev', $veth, 'up'])
> > +			if defined($oldnet->{link_down});
> > +		}
>
> > diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
> > index af25a96..746df7b 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 to link-down for PVE 8.0
>
> maybe highlight that VMs need to change too here, e.g.:
>
> # TODO: rename this *and* the qemu-server one to [...]
Ack.

>
>
> > diff --git a/src/lxcnetaddbr b/src/lxcnetaddbr
> > index ebd6baa..0940206 100755
> > --- a/src/lxcnetaddbr
> > +++ b/src/lxcnetaddbr
>
> > -    PVE::LXC::net_tap_plug($iface, $bridge, $tag, $firewall, $trunks, $rate, { mac => $hwaddr });
> > +    # Only plug the interface into the bridge if it is not set as disconnected by the user.
>
> no hard feelings here but above also reads like the code tells us anyway, so could live
> without it, but if you think it really helps I'm fine with that comment too.
Re-reading that, it certainly does seem a bit redundant, I'll probably
remove that with the next spin. Sometimes I unfortunately get a bit
(too) inclined to comment stuff.

>
> > +    PVE::LXC::net_tap_plug($iface, $bridge, $tag, $firewall, $trunks, $rate, { mac => $hwaddr })
> > +	if !defined($net->{link_down});
>