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

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

[Oguz Bektas]

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
+
     $self->protected_call(sub {
 	$self->{plugin}->unified_cgroupv2_support();
     });
-- 
2.30.2

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

[Oguz Bektas]

any news here?

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
> +
>      $self->protected_call(sub {
>  	$self->{plugin}->unified_cgroupv2_support();
>      });
> -- 
> 2.30.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 

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

[Wolfgang Bumiller]

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`)

> +
>      $self->protected_call(sub {
>  	$self->{plugin}->unified_cgroupv2_support();
>      });
> -- 
> 2.30.2

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

[Thomas Lamprecht]

On 20.07.21 13:29, Oguz Bektas wrote:
> 
> any news here?
> 

feels to much like a hack to me, sprinkling that side effects of setting the
plugin to base for the whole worker in some "random" helper is far to subtle
and dangerous...

If, it should have been set only locally, not writing to the $self state, but
I've some idea to avoid some of the (less evil) unmanaged hacks we already have..

> 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
>> +
>>      $self->protected_call(sub {
>>  	$self->{plugin}->unified_cgroupv2_support();
>>      });
>> -- 
>> 2.30.2

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

[Thomas Lamprecht]

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.

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

[Wolfgang Bumiller]

On Tue, Jul 20, 2021 at 01:40:59PM +0200, 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`)

Thinking about this some more, I think the initial strategy in v1 was
almost right.

Simply return `1` for unmanaged containers.

This is only used to produce warnings anyway, and we can simply assume
that unmanaged containers shouldn't break because we run a more modern
system, and if they do, it's not our responsibility anyway...

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

[Oguz Bektas]

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

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

[Thomas Lamprecht]

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...

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

[Wolfgang Bumiller]

On Tue, Jul 20, 2021 at 01:59:45PM +0200, Thomas Lamprecht wrote:
> 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...

sounds good to me 👍