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 30E4093A57 for ; Tue, 21 Feb 2023 18:25:42 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 0E2F05C24 for ; Tue, 21 Feb 2023 18:25:42 +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 ; Tue, 21 Feb 2023 18:25:41 +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 055F747F24 for ; Tue, 21 Feb 2023 18:25:41 +0100 (CET) Message-ID: <026094ca-8c02-234b-d3a7-4cffe2e5964a@proxmox.com> Date: Tue, 21 Feb 2023 18:25:39 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:110.0) Gecko/20100101 Thunderbird/110.0 Content-Language: en-GB, de-AT To: Proxmox VE development discussion , Christoph Heiss References: <20230221080550.43336-1-c.heiss@proxmox.com> <20230221080550.43336-4-c.heiss@proxmox.com> From: Thomas Lamprecht In-Reply-To: <20230221080550.43336-4-c.heiss@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.004 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 NICE_REPLY_A -0.095 Looks like a legit reply (A) SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [lxc.pm, config.pm] 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: Tue, 21 Feb 2023 17:25:42 -0000 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 > > 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});