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

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

On 20.07.21 13:51, Oguz Bektas wrote:
> 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.

no. As said, I do not want any more hacks even if they're at least correct
like the proposal.

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

or we can avoid further hacks, separate the plugin ABI in an actual "interface only"
module which base and a new unmanaged plugin depends on, so that we can actually drop
that special case and with it all the `return if  !$self->{plugin}; # unmanaged` stanzas
completely...