[pve-devel] [PATCH 0/2] Work around systemd warning about KillMode 'none'

[pve-devel] [PATCH 0/2] Work around systemd warning about KillMode 'none'

[Stefan Reiter]

systemd deprecated KillMode 'none', but we still need it, since we manage our VM
scopes/slices manually via another service, 'pve-guests.service'. To this
extent, replicate the previous behaviour without using the deprecated mode.

Example log message:
/run/systemd/transient/101.scope:3: Unit configured to use KillMode=none. [...]

I still get the same warning about a 'ceph-volume@.service' in my logs, after
upgrading to pacific, though I'm not sure if that's something we need to/can
fix, or something left over from the upgrade - any ceph expertise welcome :)


qemu-server: Stefan Reiter (1):
  use KillMode 'process' for systemd scope

 PVE/QemuServer.pm | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

common: Stefan Reiter (1):
  systemd: allow SendSIGKILL and TimeoutStopUSec dbus properties

 src/PVE/Systemd.pm | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

-- 
2.30.2

[pve-devel] [PATCH qemu-server 1/2] use KillMode 'process' for systemd scope

[Stefan Reiter]

KillMode 'none' is deprecated, and systemd loudly complains about that
in the journal. To avoid the warning, but keep the behaviour the same,
use KillMode 'process'.

This mode does two things differently, which we have to stop it from
doing:
* it sends SIGTERM right when the scope is cancelled (e.g. on shutdown)
 -> but only to the "root" process, which in our case is the worker
 instance forking QEMU, so it is already dead by the time this happens
* it sends SIGKILL to *all* children after a timeout
 -> can be avoided by setting either SendSIGKILL to false, or
 TimeoutStopUSec to infinity - for safety, we do both

In my testing, this replicated the previous behaviour exactly, but
without using the deprecated 'none' mode.

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---

Depends on updated pve-common from patch 2.

 PVE/QemuServer.pm | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 07dd14a..d5b7ead 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -5286,7 +5286,9 @@ sub vm_start_nolock {
 
     my %properties = (
 	Slice => 'qemu.slice',
-	KillMode => 'none'
+	KillMode => 'process',
+	SendSIGKILL => 0,
+	TimeoutStopUSec => ULONG_MAX, # infinity
     );
 
     if (PVE::CGroup::cgroup_mode() == 2) {
-- 
2.30.2

Re: [pve-devel] [PATCH qemu-server 1/2] use KillMode 'process' for systemd scope

[Thomas Lamprecht]

On 21.06.21 18:35, Stefan Reiter wrote:
> KillMode 'none' is deprecated, and systemd loudly complains about that
> in the journal. To avoid the warning, but keep the behaviour the same,
> use KillMode 'process'.
> 
> This mode does two things differently, which we have to stop it from
> doing:
> * it sends SIGTERM right when the scope is cancelled (e.g. on shutdown)
>  -> but only to the "root" process, which in our case is the worker
>  instance forking QEMU, so it is already dead by the time this happens
> * it sends SIGKILL to *all* children after a timeout
>  -> can be avoided by setting either SendSIGKILL to false, or
>  TimeoutStopUSec to infinity - for safety, we do both
> 
> In my testing, this replicated the previous behaviour exactly, but
> without using the deprecated 'none' mode.
> 
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
> 
> Depends on updated pve-common from patch 2.
> 
>  PVE/QemuServer.pm | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 07dd14a..d5b7ead 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -5286,7 +5286,9 @@ sub vm_start_nolock {
>  
>      my %properties = (
>  	Slice => 'qemu.slice',
> -	KillMode => 'none'
> +	KillMode => 'process',
> +	SendSIGKILL => 0,
> +	TimeoutStopUSec => ULONG_MAX, # infinity


I wasn't sure if  ULONG_MAX is used literally, making 71 minutes on 32 bit and ~584k years
on 64bit, or if it is translated internally to 'infinity', I mean with us only supporting
64-bit a duration of 584k year, while not infinity, would be more than enough, but still,
always good to check those things IMO:

From `src/basic/time-util.h`

typedef uint64_t usec_t;
 ...
#define USEC_INFINITY ((usec_t) -1)

So, yes, literally means infinity.

Re: [pve-devel] [PATCH qemu-server 1/2] use KillMode 'process' for systemd scope

[Stefan Reiter]

On 22/06/2021 08:02, Thomas Lamprecht wrote:
> On 21.06.21 18:35, Stefan Reiter wrote:
>> KillMode 'none' is deprecated, and systemd loudly complains about that
>> in the journal. To avoid the warning, but keep the behaviour the same,
>> use KillMode 'process'.
>>
>> This mode does two things differently, which we have to stop it from
>> doing:
>> * it sends SIGTERM right when the scope is cancelled (e.g. on shutdown)
>>   -> but only to the "root" process, which in our case is the worker
>>   instance forking QEMU, so it is already dead by the time this happens
>> * it sends SIGKILL to *all* children after a timeout
>>   -> can be avoided by setting either SendSIGKILL to false, or
>>   TimeoutStopUSec to infinity - for safety, we do both
>>
>> In my testing, this replicated the previous behaviour exactly, but
>> without using the deprecated 'none' mode.
>>
>> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
>> ---
>>
>> Depends on updated pve-common from patch 2.
>>
>>   PVE/QemuServer.pm | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
>> index 07dd14a..d5b7ead 100644
>> --- a/PVE/QemuServer.pm
>> +++ b/PVE/QemuServer.pm
>> @@ -5286,7 +5286,9 @@ sub vm_start_nolock {
>>   
>>       my %properties = (
>>   	Slice => 'qemu.slice',
>> -	KillMode => 'none'
>> +	KillMode => 'process',
>> +	SendSIGKILL => 0,
>> +	TimeoutStopUSec => ULONG_MAX, # infinity
> 
> 
> I wasn't sure if  ULONG_MAX is used literally, making 71 minutes on 32 bit and ~584k years
> on 64bit, or if it is translated internally to 'infinity', I mean with us only supporting
> 64-bit a duration of 584k year, while not infinity, would be more than enough, but still,
> always good to check those things IMO:
> 
>  From `src/basic/time-util.h`
> 
> typedef uint64_t usec_t;
>   ...
> #define USEC_INFINITY ((usec_t) -1)
> 
> So, yes, literally means infinity.
> 

yeah sorry, should have made the comment more obvious - that's where I 
got the value from too

[pve-devel] applied: [PATCH qemu-server 1/2] use KillMode 'process' for systemd scope

[Thomas Lamprecht]

On 21.06.21 18:35, Stefan Reiter wrote:
> KillMode 'none' is deprecated, and systemd loudly complains about that
> in the journal. To avoid the warning, but keep the behaviour the same,
> use KillMode 'process'.
> 
> This mode does two things differently, which we have to stop it from
> doing:
> * it sends SIGTERM right when the scope is cancelled (e.g. on shutdown)
>  -> but only to the "root" process, which in our case is the worker
>  instance forking QEMU, so it is already dead by the time this happens
> * it sends SIGKILL to *all* children after a timeout
>  -> can be avoided by setting either SendSIGKILL to false, or
>  TimeoutStopUSec to infinity - for safety, we do both
> 
> In my testing, this replicated the previous behaviour exactly, but
> without using the deprecated 'none' mode.
> 
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
> 
> Depends on updated pve-common from patch 2.
> 
>  PVE/QemuServer.pm | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
>

applied, thanks!

[pve-devel] [PATCH common 2/2] systemd: allow SendSIGKILL and TimeoutStopUSec dbus properties

[Stefan Reiter]

Used in qemu-server for avoiding KillMode 'none'. SendSIGKILL is a
boolean, so we need to use dbus_boolean to serialize it.

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---
 src/PVE/Systemd.pm | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/PVE/Systemd.pm b/src/PVE/Systemd.pm
index e9712e0..2517d31 100644
--- a/src/PVE/Systemd.pm
+++ b/src/PVE/Systemd.pm
@@ -3,7 +3,7 @@ package PVE::Systemd;
 use strict;
 use warnings;
 
-use Net::DBus qw(dbus_uint32 dbus_uint64);
+use Net::DBus qw(dbus_uint32 dbus_uint64 dbus_boolean);
 use Net::DBus::Callback;
 use Net::DBus::Reactor;
 
@@ -107,7 +107,9 @@ sub enter_systemd_scope {
     foreach my $key (keys %extra) {
 	if ($key eq 'Slice' || $key eq 'KillMode') {
 	    push @{$properties}, [$key, $extra{$key}];
-	} elsif ($key eq 'CPUShares' || $key eq 'CPUWeight') {
+	} elsif ($key eq 'SendSIGKILL') {
+	    push @{$properties}, [$key, dbus_boolean($extra{$key})];
+	} elsif ($key eq 'CPUShares' || $key eq 'CPUWeight' || $key eq 'TimeoutStopUSec') {
 	    push @{$properties}, [$key, dbus_uint64($extra{$key})];
 	} elsif ($key eq 'CPUQuota') {
 	    push @{$properties}, ['CPUQuotaPerSecUSec',
-- 
2.30.2

[pve-devel] applied: [PATCH common 2/2] systemd: allow SendSIGKILL and TimeoutStopUSec dbus properties

[Thomas Lamprecht]

On 21.06.21 18:35, Stefan Reiter wrote:
> Used in qemu-server for avoiding KillMode 'none'. SendSIGKILL is a
> boolean, so we need to use dbus_boolean to serialize it.
> 
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
>  src/PVE/Systemd.pm | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
>

applied, thanks!