From: Christoph Heiss <c.heiss@proxmox.com>
To: Thomas Lamprecht <t.lamprecht@proxmox.com>
Cc: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Subject: Re: [pve-devel] [PATCH v3 container 3/4] lxc: Add `link_down` config to allow setting interfaces as disconnected
Date: Wed, 22 Feb 2023 10:51:47 +0100 [thread overview]
Message-ID: <20230222095147.74ju4fph5ubpotgw@maui.proxmox.com> (raw)
In-Reply-To: <026094ca-8c02-234b-d3a7-4cffe2e5964a@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});
>
next prev parent reply other threads:[~2023-02-22 9:52 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-02-21 8:05 [pve-devel] [PATCH v3 container/manager 0/4] fix #3413: Add `Disconnect` option for LXC networks Christoph Heiss
2023-02-21 8:05 ` [pve-devel] [PATCH v3 container 1/4] lxc: Fix some trailing whitespace Christoph Heiss
2023-02-21 16:59 ` [pve-devel] applied: " Thomas Lamprecht
2023-02-21 8:05 ` [pve-devel] [PATCH v3 container 2/4] lxc: Avoid open-coding normal vs SDN-specific tap_plug() Christoph Heiss
2023-02-21 17:07 ` [pve-devel] applied: " Thomas Lamprecht
2023-02-22 9:34 ` Christoph Heiss
2023-02-21 8:05 ` [pve-devel] [PATCH v3 container 3/4] lxc: Add `link_down` config to allow setting interfaces as disconnected Christoph Heiss
2023-02-21 17:25 ` Thomas Lamprecht
2023-02-22 9:51 ` Christoph Heiss [this message]
2023-02-21 8:05 ` [pve-devel] [PATCH v3 manager 4/4] lxc: Add `Disconnect` option for network interfaces Christoph Heiss
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230222095147.74ju4fph5ubpotgw@maui.proxmox.com \
--to=c.heiss@proxmox.com \
--cc=pve-devel@lists.proxmox.com \
--cc=t.lamprecht@proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox