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

[pve-devel] [PATCH v2 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

pve-container:

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

 src/PVE/LXC.pm        | 41 ++++++++++++++++++++++++++++++++++-------
 src/PVE/LXC/Config.pm |  5 +++++
 src/lxcnetaddbr       |  7 +++++--
 3 files changed, 44 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 v2 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

 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 ce6d5a5..345e343 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -668,7 +668,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";

@@ -1703,14 +1703,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);
@@ -1719,7 +1719,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);
@@ -1828,7 +1828,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 v2 container 2/4] lxc: Avoid open-coding normal vs SDN-specific tap_plug()

[Christoph Heiss]

This pattern is used in multiple places, thus extract it into a
subroutine 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

 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 345e343..0de5ba3 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -917,6 +917,18 @@ sub vm_stop_cleanup {
     warn $@ if $@; # avoid errors - just warn
 }

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

@@ -956,14 +968,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)) {
@@ -994,13 +999,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 v2 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

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

diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
index 0de5ba3..1b93f48 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -955,7 +955,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}) {
@@ -968,10 +969,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})) {
@@ -1002,9 +1021,6 @@ 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} });

     # attach peer in container
     my $cmd = ['lxc-device', '-n', $vmid, 'add', $vethpeer, "$eth" ];
@@ -1014,8 +1030,19 @@ sub hotplug_net {
     $cmd = ['lxc-attach', '-n', $vmid, '-s', 'NETWORK', '--', '/sbin/ip', 'link', 'set', $eth ,'up'  ];
     PVE::Tools::run_command($cmd);

+    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} });
+    }
+
     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..26a2fac 100644
--- a/src/PVE/LXC/Config.pm
+++ b/src/PVE/LXC/Config.pm
@@ -814,6 +814,11 @@ our $netconf_desc = {
 	description => "Apply rate limiting to the interface",
 	optional => 1,
     },
+    '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..c5d724b 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 v2 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

 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..120a4168 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..46bf062c 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 v2 container 1/4] lxc: Fix some trailing whitespace

[Wolfgang Bumiller]

Does not apply, somewhere along the line you're stripping the trailing
newlines of these patches, how did you generate them?

On Wed, Feb 15, 2023 at 03:02:42PM +0100, Christoph Heiss wrote:
> No functional changes.
> 
> Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
> ---
> Changes v1 -> v2:
>  * New patch, split out from patch 2
> 
>  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 ce6d5a5..345e343 100644
> --- a/src/PVE/LXC.pm
> +++ b/src/PVE/LXC.pm
> @@ -668,7 +668,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";
> 
> @@ -1703,14 +1703,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);
> @@ -1719,7 +1719,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);
> @@ -1828,7 +1828,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

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

[Wolfgang Bumiller]

On Wed, Feb 15, 2023 at 03:02:43PM +0100, Christoph Heiss wrote:
> This pattern is used in multiple places, thus extract it into a
> subroutine 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
> 
>  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 345e343..0de5ba3 100644
> --- a/src/PVE/LXC.pm
> +++ b/src/PVE/LXC.pm
> @@ -917,6 +917,18 @@ sub vm_stop_cleanup {
>      warn $@ if $@; # avoid errors - just warn
>  }
> 
> +sub net_tap_plug {
> +    if ($have_sdn) {
> +	my ($iface, $bridge, $tag, $firewall, $trunks, $rate, $opts) = @_;

^ please don't do this conditional on `$have_sdn`. This just makes it
harder to figure out how to use the function later (ie. quicker to write
this one time now at the cost of reduced maintainability in the future).

I'd much rather additionally have a prototype on the sub which may even
declare the $opts parameter explicitly as being optional like so:

    sub net_tap_plug : prototype($$$$$$;$) {
        my ($iface, $bridge, $tag, $firewall, $trunks, $rate, $opts) = @_;
        ...

Apart from that, the refactoring makes sense.

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

[Wolfgang Bumiller]

On Wed, Feb 15, 2023 at 03:02:44PM +0100, Christoph Heiss wrote:
> 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

Sorry I missed that when Thomas replied to the v1.
This is a case where the property already exists in the VM api, so
better reuse the snake_case version here. @Thomas?

>  * 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
> 
>  src/PVE/LXC.pm        | 41 ++++++++++++++++++++++++++++++++++-------
>  src/PVE/LXC/Config.pm |  5 +++++
>  src/lxcnetaddbr       |  7 +++++--
>  3 files changed, 44 insertions(+), 9 deletions(-)
> 
> diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
> index 0de5ba3..1b93f48 100644
> --- a/src/PVE/LXC.pm
> +++ b/src/PVE/LXC.pm
> @@ -955,7 +955,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}) {
> @@ -968,10 +969,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})) {
> @@ -1002,9 +1021,6 @@ 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} });
> 
>      # attach peer in container
>      my $cmd = ['lxc-device', '-n', $vmid, 'add', $vethpeer, "$eth" ];
> @@ -1014,8 +1030,19 @@ sub hotplug_net {
>      $cmd = ['lxc-attach', '-n', $vmid, '-s', 'NETWORK', '--', '/sbin/ip', 'link', 'set', $eth ,'up'  ];
>      PVE::Tools::run_command($cmd);
> 
> +    if (defined($newnet->{'link-down'})) {

Any particular reason for moving this down here now?
I'm sure *someone* will run into issues with badly and wrongly
configured containers with this ;-)
It's just nicer for containers to immediately see a readily connected
interface, so maybe keep it up there where the original tap_plug call
was.

> +	# 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} });
> +    }
> +
>      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..26a2fac 100644
> --- a/src/PVE/LXC/Config.pm
> +++ b/src/PVE/LXC/Config.pm
> @@ -814,6 +814,11 @@ our $netconf_desc = {
>  	description => "Apply rate limiting to the interface",
>  	optional => 1,
>      },
> +    '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..c5d724b 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

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

[Thomas Lamprecht]

Am 17/02/2023 um 15:51 schrieb Wolfgang Bumiller:
>>  * Rename option to kebap-case
> Sorry I missed that when Thomas replied to the v1.
> This is a case where the property already exists in the VM api, so
> better reuse the snake_case version here. @Thomas?
> 

I assumed that, that's why I asked if keeping it that way would help to reuse some
code infra; not sure about how useful it is to keep it for API consumers to keep it
the same as in a quite different API end point, but I see where you come from and don't
insist for going with snake-case, so if you think using the same style here has benefits.
we can ignore the guidance that new params should always use kebab-case.

but as said off-list, a big s/_/-/ (and same for our camelCase usages -.-) for PVE 8.0
seems long overdue; then we can also add some style asserts in the api schema verifier
to avoid reintroducing those things.

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

[Christoph Heiss]

On Fri, Feb 17, 2023 at 03:22:39PM +0100, Wolfgang Bumiller wrote:
> Does not apply, somewhere along the line you're stripping the trailing
> newlines of these patches, how did you generate them?
Normally with `git format-patch`, but I think I know where they got
"lost". I will take better care before sending the next version and try
to apply the series myself before sending!

Sorry for that.

>
> On Wed, Feb 15, 2023 at 03:02:42PM +0100, Christoph Heiss wrote:
> > No functional changes.
> >
> > Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
> > ---
> > Changes v1 -> v2:
> >  * New patch, split out from patch 2
> >
> >  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 ce6d5a5..345e343 100644
> > --- a/src/PVE/LXC.pm
> > +++ b/src/PVE/LXC.pm
> > @@ -668,7 +668,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";
> >
> > @@ -1703,14 +1703,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);
> > @@ -1719,7 +1719,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);
> > @@ -1828,7 +1828,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

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

[Christoph Heiss]

Thanks for the review!

On Fri, Feb 17, 2023 at 03:34:24PM +0100, Wolfgang Bumiller wrote:
> On Wed, Feb 15, 2023 at 03:02:43PM +0100, Christoph Heiss wrote:
> > This pattern is used in multiple places, thus extract it into a
> > subroutine 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
> >
> >  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 345e343..0de5ba3 100644
> > --- a/src/PVE/LXC.pm
> > +++ b/src/PVE/LXC.pm
> > @@ -917,6 +917,18 @@ sub vm_stop_cleanup {
> >      warn $@ if $@; # avoid errors - just warn
> >  }
> >
> > +sub net_tap_plug {
> > +    if ($have_sdn) {
> > +	my ($iface, $bridge, $tag, $firewall, $trunks, $rate, $opts) = @_;
>
> ^ please don't do this conditional on `$have_sdn`. This just makes it
> harder to figure out how to use the function later (ie. quicker to write
> this one time now at the cost of reduced maintainability in the future).
>
> I'd much rather additionally have a prototype on the sub which may even
> declare the $opts parameter explicitly as being optional like so:
>
>     sub net_tap_plug : prototype($$$$$$;$) {
>         my ($iface, $bridge, $tag, $firewall, $trunks, $rate, $opts) = @_;
>         ...
Ack, will change that for v2. Wasn't sure about prototypes in Perl yet,
good time for me to read up on them a bit. Thanks again!

>
> Apart from that, the refactoring makes sense.

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

[Christoph Heiss]

On Fri, Feb 17, 2023 at 05:38:57PM +0100, Thomas Lamprecht wrote:
> Am 17/02/2023 um 15:51 schrieb Wolfgang Bumiller:
> >>  * Rename option to kebap-case
> > Sorry I missed that when Thomas replied to the v1.
> > This is a case where the property already exists in the VM api, so
> > better reuse the snake_case version here. @Thomas?
> >
>
> I assumed that, that's why I asked if keeping it that way would help to reuse some
> code infra; not sure about how useful it is to keep it for API consumers to keep it
> the same as in a quite different API end point,
There really is no code-reuse between those two endpoints, so it made
sense to me too to rename it.

> but I see where you come from and don't
> insist for going with snake-case, so if you think using the same style here has benefits.
> we can ignore the guidance that new params should always use kebab-case.
.. but I will rename it back to snake_case for v3, since that seems to
be consensus here?

> but as said off-list, a big s/_/-/ (and same for our camelCase usages -.-) for PVE 8.0
> seems long overdue; then we can also add some style asserts in the api schema verifier
> to avoid reintroducing those things.

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

[Wolfgang Bumiller]

On Mon, Feb 20, 2023 at 12:51:55PM +0100, Christoph Heiss wrote:
> On Fri, Feb 17, 2023 at 05:38:57PM +0100, Thomas Lamprecht wrote:
> > Am 17/02/2023 um 15:51 schrieb Wolfgang Bumiller:
> > >>  * Rename option to kebap-case
> > > Sorry I missed that when Thomas replied to the v1.
> > > This is a case where the property already exists in the VM api, so
> > > better reuse the snake_case version here. @Thomas?
> > >
> >
> > I assumed that, that's why I asked if keeping it that way would help to reuse some
> > code infra; not sure about how useful it is to keep it for API consumers to keep it
> > the same as in a quite different API end point,
> There really is no code-reuse between those two endpoints, so it made
> sense to me too to rename it.

It's more about possible code reuse in front-ends rather than in the
backend though, where a set of common options with common code to
fill/read/... them can definitely be a thing ;-)

> 
> > but I see where you come from and don't
> > insist for going with snake-case, so if you think using the same style here has benefits.
> > we can ignore the guidance that new params should always use kebab-case.
> .. but I will rename it back to snake_case for v3, since that seems to
> be consensus here?

Given Thomas' plan to rename stuff in the future, I'd say yes, for now.

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

[Thomas Lamprecht]

Am 20/02/2023 um 13:33 schrieb Wolfgang Bumiller:
>>> I assumed that, that's why I asked if keeping it that way would help to reuse some
>>> code infra; not sure about how useful it is to keep it for API consumers to keep it
>>> the same as in a quite different API end point,
>> There really is no code-reuse between those two endpoints, so it made
>> sense to me too to rename it.
> It's more about possible code reuse in front-ends rather than in the
> backend though, where a set of common options with common code to
> fill/read/... them can definitely be a thing 😉

fwiw, it won't matter for any of our front ends either, the CT and VM network
API is way to different to share full (edit) panels.

But, for automation tooling that talks with the API it might be quite relevant.

> 
>>> but I see where you come from and don't
>>> insist for going with snake-case, so if you think using the same style here has benefits.
>>> we can ignore the guidance that new params should always use kebab-case.
>> .. but I will rename it back to snake_case for v3, since that seems to
>> be consensus here?
> Given Thomas' plan to rename stuff in the future, I'd say yes, for now.

fine by me, snake_case it is for now, for this specific option.