Re: [pve-devel] [PATCH container 1/2] fix #2044: add openwrt support

[Oguz Bektas <o.bektas@proxmox.com>]

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.