From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id E897B93BDF for ; Wed, 22 Feb 2023 10:52:20 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id C7845D421 for ; Wed, 22 Feb 2023 10:51:50 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Wed, 22 Feb 2023 10:51:49 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id B3E3C480DF for ; Wed, 22 Feb 2023 10:51:49 +0100 (CET) Date: Wed, 22 Feb 2023 10:51:47 +0100 From: Christoph Heiss To: Thomas Lamprecht Cc: Proxmox VE development discussion Message-ID: <20230222095147.74ju4fph5ubpotgw@maui.proxmox.com> References: <20230221080550.43336-1-c.heiss@proxmox.com> <20230221080550.43336-4-c.heiss@proxmox.com> <026094ca-8c02-234b-d3a7-4cffe2e5964a@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <026094ca-8c02-234b-d3a7-4cffe2e5964a@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL -0.102 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pve-devel] [PATCH v3 container 3/4] lxc: Add `link_down` config to allow setting interfaces as disconnected X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 22 Feb 2023 09:52:21 -0000 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 > > --- > > 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}); >