Re: [pve-devel] [PATCH container v3] fix #3516: fix unmanaged containers

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

On Tue, Jul 20, 2021 at 01:49:45PM +0200, Thomas Lamprecht wrote:
> On 20.07.21 13:40, Wolfgang Bumiller wrote:
> > On Wed, Jul 14, 2021 at 11:51:51AM +0200, Oguz Bektas wrote:
> >> unmanaged containers should run the unified cgroupv2 code from our base
> >> plugin so that they can start correctly instead of erroring out
> >>
> >> Tested-by: Stoiko Ivanov <s.ivanov@proxmox.com>
> >> Reviewed-by: Stoiko Ivanov <s.ivanov@proxmox.com>
> >> Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
> >> ---
> >> v2-> v3:
> >> * added comment from stoiko's reply
> >>
> >>
> >>  src/PVE/LXC/Setup.pm | 4 ++++
> >>  1 file changed, 4 insertions(+)
> >>
> >> diff --git a/src/PVE/LXC/Setup.pm b/src/PVE/LXC/Setup.pm
> >> index 9abdc85..4408dcc 100644
> >> --- a/src/PVE/LXC/Setup.pm
> >> +++ b/src/PVE/LXC/Setup.pm
> >> @@ -424,6 +424,10 @@ sub get_ct_os_release {
> >>  sub unified_cgroupv2_support {
> >>      my ($self) = @_;
> >>  
> >> +    # code in base plugin is a generic check and should work
> >> +    # for most distributions
> >> +    $self->{plugin} //= 'PVE::LXC::Setup::Base'; # unmanaged
> > 
> > This has the side effect that all later checks for unmanaged containers
> > via `$self->{plugin}` are broken.
> > Please either move this *into* the `protected_call` below (and add a
> > comment that the assignment is temporary due to how `protected_call`
> > works), or cleanup this change afterwards (but that would need to be
> > `die`-safe (iow. would need an eval around the `protected_call`)
> > 
> 
> for that it could have just used a local variable, e.g.:
> 
> my $plugin = $self->{plugin} // 'PVE::LXC::Setup::Base'; # fallback to base for unmanaged
> 
> $self->protected_call(sub { $plugin->unified_cgroupv2_support() });
> 
> but I'd rather avoid adding more of those "unmanaged hacks" in general.

i think this is acceptable approach, if you don't mind i'd send another
version with that.

or we can simply skip it like in the v1 but with a true return value to
omit systemd error message