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

[Christoph Heiss <c.heiss@proxmox.com>]

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