[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 at 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});