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

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

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