From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <t.lamprecht@proxmox.com>
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 <pve-devel@lists.proxmox.com>; 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 <pve-devel@lists.proxmox.com>; 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 <pve-devel@lists.proxmox.com>; 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 <pve-devel@lists.proxmox.com>; 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 <pve-devel@lists.proxmox.com>,
Christoph Heiss <c.heiss@proxmox.com>
References: <20230221080550.43336-1-c.heiss@proxmox.com>
<20230221080550.43336-4-c.heiss@proxmox.com>
From: Thomas Lamprecht <t.lamprecht@proxmox.com>
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 <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=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 <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
>
> 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});