[pve-devel] [RFC PATCH common] RESTEnvironment: better SIGCHLD handling in AnyEvent event loop

[pve-devel] [RFC PATCH common] RESTEnvironment: better SIGCHLD handling in AnyEvent event loop

[Dominik Csapak]

when we're in an API server that uses AnyEvent, we must postpone
the worker_reaper, since it calls 'active_workers' which might already
be called and then we're inside the lock twice (flocks are per process
for us, see PVE::Tools::lock_file)

This resulted in an error like this:
close (rename) atomic file '/var/log/pve/tasks/active' failed: No such file or directory

We use the fact that only 'pub' and 'priv' RESTEnvironment types are an
api server with anyevent. For other types we call it like before.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
Not super happy about the coupling between the RESTEnvironment and AnyEvent.
We could try to just save the worker_reaper in 'self' and let the users
of the env decide when to call it, but that would be more involved.

OTOH, we already do some anyevent specific things in PVE::Daemon
(without depending on the AnyEvent package though)...

Also i did not find a way to dynamically find out if we're in an
AnyEvent loop...

 debian/control             |  1 +
 src/PVE/RESTEnvironment.pm | 13 ++++++++++++-
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/debian/control b/debian/control
index 232a0e4..1c75985 100644
--- a/debian/control
+++ b/debian/control
@@ -3,6 +3,7 @@ Section: perl
 Priority: optional
 Maintainer: Proxmox Support Team <support@proxmox.com>
 Build-Depends: debhelper (>= 12~),
+               libanyevent-perl,
                libclone-perl,
                libdevel-cycle-perl,
                libfilesys-df-perl,
diff --git a/src/PVE/RESTEnvironment.pm b/src/PVE/RESTEnvironment.pm
index bf89c12..c258b1e 100644
--- a/src/PVE/RESTEnvironment.pm
+++ b/src/PVE/RESTEnvironment.pm
@@ -14,6 +14,7 @@ use IO::File;
 use IO::Handle;
 use IO::Select;
 use POSIX qw(:sys_wait_h EINTR);
+use AnyEvent;
 
 use PVE::Exception qw(raise raise_perm_exc);
 use PVE::INotify;
@@ -111,7 +112,17 @@ sub init {
     die "unknown environment type"
 	if !$type || $type !~ m/^(cli|pub|priv|ha)$/;
 
-    $SIG{CHLD} = $worker_reaper;
+    my $has_anyevent = $type eq 'pub' || $type eq 'priv';
+
+    $SIG{CHLD} = sub {
+	# when we're in an api server, we have to postpone the call to worker_reaper, otherwise it
+	# might interfere with running api calls
+	if ($has_anyevent) {
+	    AnyEvent::postpone { $worker_reaper->() };
+	} else {
+	    $worker_reaper->();
+	}
+    };
 
     # environment types
     # cli  ... command started fron command line
-- 
2.30.2

Re: [pve-devel] [RFC PATCH common] RESTEnvironment: better SIGCHLD handling in AnyEvent event loop

[Wolfgang Bumiller]

On Mon, Feb 20, 2023 at 11:08:28AM +0100, Dominik Csapak wrote:
> when we're in an API server that uses AnyEvent, we must postpone
> the worker_reaper, since it calls 'active_workers' which might already
> be called and then we're inside the lock twice (flocks are per process
> for us, see PVE::Tools::lock_file)
> 
> This resulted in an error like this:
> close (rename) atomic file '/var/log/pve/tasks/active' failed: No such file or directory
> 
> We use the fact that only 'pub' and 'priv' RESTEnvironment types are an
> api server with anyevent. For other types we call it like before.
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> Not super happy about the coupling between the RESTEnvironment and AnyEvent.
> We could try to just save the worker_reaper in 'self' and let the users
> of the env decide when to call it, but that would be more involved.
> 
> OTOH, we already do some anyevent specific things in PVE::Daemon
> (without depending on the AnyEvent package though)...
> 
> Also i did not find a way to dynamically find out if we're in an
> AnyEvent loop...

The current SIGCHLD handling is a no-go either way, and I don't think an
AnyEvent dependency is an issue. I mean, we may not be using it on the
CLI, but I wouldn't be surprised if we needed to at some point either...

I'm actually surprised it's not even used in the 'ha' environment types?

> 
>  debian/control             |  1 +
>  src/PVE/RESTEnvironment.pm | 13 ++++++++++++-
>  2 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/debian/control b/debian/control
> index 232a0e4..1c75985 100644
> --- a/debian/control
> +++ b/debian/control
> @@ -3,6 +3,7 @@ Section: perl
>  Priority: optional
>  Maintainer: Proxmox Support Team <support@proxmox.com>
>  Build-Depends: debhelper (>= 12~),
> +               libanyevent-perl,
>                 libclone-perl,
>                 libdevel-cycle-perl,
>                 libfilesys-df-perl,
> diff --git a/src/PVE/RESTEnvironment.pm b/src/PVE/RESTEnvironment.pm
> index bf89c12..c258b1e 100644
> --- a/src/PVE/RESTEnvironment.pm
> +++ b/src/PVE/RESTEnvironment.pm
> @@ -14,6 +14,7 @@ use IO::File;
>  use IO::Handle;
>  use IO::Select;
>  use POSIX qw(:sys_wait_h EINTR);
> +use AnyEvent;
>  
>  use PVE::Exception qw(raise raise_perm_exc);
>  use PVE::INotify;
> @@ -111,7 +112,17 @@ sub init {
>      die "unknown environment type"
>  	if !$type || $type !~ m/^(cli|pub|priv|ha)$/;
>  
> -    $SIG{CHLD} = $worker_reaper;
> +    my $has_anyevent = $type eq 'pub' || $type eq 'priv';
> +
> +    $SIG{CHLD} = sub {
> +	# when we're in an api server, we have to postpone the call to worker_reaper, otherwise it
> +	# might interfere with running api calls
> +	if ($has_anyevent) {
> +	    AnyEvent::postpone { $worker_reaper->() };
> +	} else {
> +	    $worker_reaper->();
> +	}
> +    };
>  
>      # environment types
>      # cli  ... command started fron command line
> -- 
> 2.30.2

[pve-devel] applied: [RFC PATCH common] RESTEnvironment: better SIGCHLD handling in AnyEvent event loop

[Thomas Lamprecht]

On 20/02/2023 11:08, Dominik Csapak wrote:
> when we're in an API server that uses AnyEvent, we must postpone
> the worker_reaper, since it calls 'active_workers' which might already
> be called and then we're inside the lock twice (flocks are per process
> for us, see PVE::Tools::lock_file)
> 
> This resulted in an error like this:
> close (rename) atomic file '/var/log/pve/tasks/active' failed: No such file or directory
> 
> We use the fact that only 'pub' and 'priv' RESTEnvironment types are an
> api server with anyevent. For other types we call it like before.
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> Not super happy about the coupling between the RESTEnvironment and AnyEvent.
> We could try to just save the worker_reaper in 'self' and let the users
> of the env decide when to call it, but that would be more involved.
> 
> OTOH, we already do some anyevent specific things in PVE::Daemon
> (without depending on the AnyEvent package though)...
> 
> Also i did not find a way to dynamically find out if we're in an
> AnyEvent loop...
> 
>  debian/control             |  1 +
>  src/PVE/RESTEnvironment.pm | 13 ++++++++++++-
>  2 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/debian/control b/debian/control
> index 232a0e4..1c75985 100644
> --- a/debian/control
> +++ b/debian/control
> @@ -3,6 +3,7 @@ Section: perl
>  Priority: optional
>  Maintainer: Proxmox Support Team <support@proxmox.com>
>  Build-Depends: debhelper (>= 12~),
> +               libanyevent-perl,

if we manage to guard calling into anyevent correctly it would not really be a
hard-dependency; OTOH, in practice not much (nothing?) would change so not really
that hard feelings either - especially as you mention the use in PVE::Daemon ..

>                 libclone-perl,
>                 libdevel-cycle-perl,
>                 libfilesys-df-perl,
> diff --git a/src/PVE/RESTEnvironment.pm b/src/PVE/RESTEnvironment.pm
> index bf89c12..c258b1e 100644
> --- a/src/PVE/RESTEnvironment.pm
> +++ b/src/PVE/RESTEnvironment.pm
> @@ -14,6 +14,7 @@ use IO::File;
>  use IO::Handle;
>  use IO::Select;
>  use POSIX qw(:sys_wait_h EINTR);
> +use AnyEvent;
>  
>  use PVE::Exception qw(raise raise_perm_exc);
>  use PVE::INotify;
> @@ -111,7 +112,17 @@ sub init {
>      die "unknown environment type"
>  	if !$type || $type !~ m/^(cli|pub|priv|ha)$/;
>  
> -    $SIG{CHLD} = $worker_reaper;
> +    my $has_anyevent = $type eq 'pub' || $type eq 'priv';
> +

meh, I'd like some more direct check, FWIW, we could check for `$INC{'AnyEvent.pm'}` if
that's any use here? I hoped $AnyEvent::MODEL could be used, but from a very quick test
it was uninitialized, maybe I held it wrong..

> +    $SIG{CHLD} = sub {
> +	# when we're in an api server, we have to postpone the call to worker_reaper, otherwise it
> +	# might interfere with running api calls
> +	if ($has_anyevent) {
> +	    AnyEvent::postpone { $worker_reaper->() };
> +	} else {
> +	    $worker_reaper->();
> +	}
> +    };
>  
>      # environment types
>      # cli  ... command started fron command line

Anyhow: applied for now, thanks! Finding a better way to detect if this is required would
be still nice though. if anybody got ideas please lets hear them ;-)