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) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 3EC7D7D6F1 for ; Tue, 9 Nov 2021 11:58:16 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 14D03A625 for ; Tue, 9 Nov 2021 11:57:46 +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 id 22520A61A for ; Tue, 9 Nov 2021 11:57:45 +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 E7F3F4273D for ; Tue, 9 Nov 2021 11:57:44 +0100 (CET) Date: Tue, 9 Nov 2021 11:57:42 +0100 From: Oguz Bektas To: Thomas Lamprecht Cc: Proxmox VE development discussion Message-ID: Mail-Followup-To: Oguz Bektas , Thomas Lamprecht , Proxmox VE development discussion References: <20211108134400.1884660-1-o.bektas@proxmox.com> <20211108134400.1884660-2-o.bektas@proxmox.com> <1546d7cd-9a15-6444-075d-9fe1268d8b63@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1546d7cd-9a15-6444-075d-9fe1268d8b63@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.815 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 URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [gentoo.pm, openwrt.pm, ubuntu.pm, fedora.pm, setup.pm, unmanaged.pm, suse.pm] Subject: Re: [pve-devel] [PATCH container 1/2] fix #2044: add openwrt support 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, 09 Nov 2021 10:58:16 -0000 hi, thanks for the comments :) On Mon, Nov 08, 2021 at 04:11:06PM +0100, Thomas Lamprecht wrote: > no sign-off and zero commit message stating thoughts about design choices > (like just adding the last named configured vNIC which as single interface > to the CT network config?!) what openWRT is based on (e.g., that it basically > just uses busybox) and a, at least basic, pve-docs patch would be also good > to have.. > > On 08.11.21 14:43, Oguz Bektas wrote: > > --- > > src/PVE/LXC/Setup.pm | 4 ++ > > src/PVE/LXC/Setup/Makefile | 1 + > > src/PVE/LXC/Setup/OpenWrt.pm | 82 ++++++++++++++++++++++++++++++++++++ > > file/module name should be cased OpenWRT hmm, they call it 'OpenWrt' everywhere on their website though, also on wikipedia etc. it's spelled in this way ;) > > > 3 files changed, 87 insertions(+) > > create mode 100644 src/PVE/LXC/Setup/OpenWrt.pm > > > > diff --git a/src/PVE/LXC/Setup.pm b/src/PVE/LXC/Setup.pm > > index 5cc56af..e89886f 100644 > > --- a/src/PVE/LXC/Setup.pm > > +++ b/src/PVE/LXC/Setup.pm > > @@ -17,6 +17,7 @@ use PVE::LXC::Setup::Fedora; > > use PVE::LXC::Setup::Gentoo; > > use PVE::LXC::Setup::SUSE; > > use PVE::LXC::Setup::Ubuntu; > > +use PVE::LXC::Setup::OpenWrt; > > use PVE::LXC::Setup::Unmanaged; > > > > my $plugins = { > > @@ -28,6 +29,7 @@ my $plugins = { > > fedora => 'PVE::LXC::Setup::Fedora', > > gentoo => 'PVE::LXC::Setup::Gentoo', > > opensuse => 'PVE::LXC::Setup::SUSE', > > + openwrt => 'PVE::LXC::Setup::OpenWrt', > > ubuntu => 'PVE::LXC::Setup::Ubuntu', > > unmanaged => 'PVE::LXC::Setup::Unmanaged', > > }; > > @@ -75,6 +77,8 @@ my $autodetect_type = sub { > > return "alpine"; > > } elsif (-f "$rootdir/etc/gentoo-release") { > > return "gentoo"; > > + } elsif (-f "$rootdir/etc/openwrt_release") { > > + return "openwrt"; > > } elsif (-f "$rootdir/etc/os-release") { > > die "unable to detect OS distribution\n"; > > } else { > > diff --git a/src/PVE/LXC/Setup/Makefile b/src/PVE/LXC/Setup/Makefile > > index 04ee2e4..22702ca 100644 > > --- a/src/PVE/LXC/Setup/Makefile > > +++ b/src/PVE/LXC/Setup/Makefile > > @@ -9,6 +9,7 @@ SOURCES=\ > > Fedora.pm \ > > Gentoo.pm \ > > SUSE.pm \ > > + OpenWrt.pm \ > > Ubuntu.pm \ > > Unmanaged.pm \ > > > > diff --git a/src/PVE/LXC/Setup/OpenWrt.pm b/src/PVE/LXC/Setup/OpenWrt.pm > > new file mode 100644 > > index 0000000..89ea740 > > --- /dev/null > > +++ b/src/PVE/LXC/Setup/OpenWrt.pm > > @@ -0,0 +1,82 @@ > > +package PVE::LXC::Setup::OpenWrt; > > + > > +use strict; > > +use warnings; > > + > > +use PVE::LXC; > > +use PVE::Network; > > +use File::Path; > > + > > +use PVE::LXC::Setup::Base; > > +use base qw(PVE::LXC::Setup::Base); > > + > > +my $known_versions = { > > + '21.02.1' => 1, > > so a simple upgrade from 21.02.1 -> 21.02.2 may already break CT start? > > Busybox based CTs tend to be quite forgiving and not that likely to change, > so I'd not fatally die on an unknown (newer) version, maybe just checking > for > 21.02 would be enough of a safety net. yes that makes sense, i was mainly concerned with the current version working but should be fine to allow future newer versions as well. > > > +}; > > + > > +sub new { > > + my ($class, $conf, $rootdir) = @_; > > + > > + my $version; > > + my $release = PVE::Tools::file_get_contents("$rootdir/etc/openwrt_release"); > > + if ($release =~ m/^DISTRIB_RELEASE=\'(\d+\.\d+\.\d+)\'$/mi) { > > + $version = $1; > > + } > > + die "unsupported OpenWrt version '$version'\n" > > + if !$known_versions->{$version}; > > + > > + my $self = { conf => $conf, rootdir => $rootdir, version => $version }; > > + > > + $conf->{ostype} = "openwrt"; > > + > > + return bless $self, $class; > > +} > > + > > +sub template_fixup { > > + my ($self, $conf) = @_; > > +} > > + > > +sub setup_init { > > + my ($self, $conf) = @_; > > +} > > + > > +sub setup_network { > > + my ($self, $conf) = @_; > > + > > + my $d; > > + foreach my $k (keys %$conf) { > > + next if $k !~ m/^net(\d+)$/; > > + $d = PVE::LXC::Config->parse_lxc_network($conf->{$k}); > > + next if !$d->{name}; > > what is this and why does it selects it a random (perl's `keys` is actively shuffling > stuff around) different netX config to use on each new start?! > > I'd figure that such nondeterministic behavior would be rather confusing and undesired > for an user.. ah yes. i guess it would make sense to sort the keys before. i wanted to add the interfaces in a loop but then got lost, i'll fix that in v2. > > > + } > > + > > + my $proto = ($d->{ip} eq 'dhcp') ? 'dhcp' : 'static'; > > + my $ip = ""; > > + if ($proto eq 'static') { > > + $ip = $d->{ip}; > > + } > > + > > + my $data = <<"DATA"; > > +config interface 'loopback' > > + option proto 'static' > > + option ipaddr '127.0.0.1' > > + option netmask '255.0.0.0' > > + option device 'lo' > > + > > +config interface 'wan' > > + option proto '$proto' > > + option device '$d->{name}' > > + option ipaddr '$ip' > > + option netmask '255.255.255.0' > > so no IPv6 and not multiple interfaces and no dhcp (for testing)? I'd imagine that some > user may not be too happy with that, especially as they can configure such things without > any complaint from the backend, at least some good reasoning in a commit message and short > hints in code comments would be good _if_ we go for that overly simple config printer, I > mean we surely do not need to support all of openWRT, nor can't we, but at least a minimum > basic set of common used stuff would be nice, as would be multiple interfaces. dhcp works (rather hackily but it does :D), see the line with 'proto' above. we just leave $ip empty in case of dhcp, otherwise it takes the 'static' param in there. ipv6 can be configured in the same way by adding 'wan6' interface and doing the same.