Re: [pve-devel] [PATCH container] fix #3478: abort container creation on arch detection timeout

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On June 29, 2021 3:53 pm, Lorenz Stechauner wrote:
> increased the timeout for detect_arch from 5 to 10 seconds.
> 
> until now, on any error detect_architecture would fall back to amd64.
> to avoid falling back due to an timeout error this function now dies
> on timeout errors.
> 
> additionally minor changes to the error messages have been made.
> 
> Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
> ---
>  src/PVE/LXC/Create.pm | 27 ++++++++++++++++++++++++---
>  1 file changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/src/PVE/LXC/Create.pm b/src/PVE/LXC/Create.pm
> index 82d7ad9..0260578 100644
> --- a/src/PVE/LXC/Create.pm
> +++ b/src/PVE/LXC/Create.pm
> @@ -52,10 +52,31 @@ sub detect_architecture {
>  	return $arch;
>      };
>  
> -    my $arch = eval { PVE::Tools::run_fork_with_timeout(5, $detect_arch) };
> -    if (my $err = $@) {
> +    my $arch;
> +    my $status = 'error';
> +    eval {
> +	$arch = PVE::Tools::run_fork_with_timeout(10, $detect_arch);
> +	if (!defined($arch)) {
> +	    if ($@) {
> +		$status = 'timeout';
> +		die $@;
> +	    } else {
> +		$status = 'error';
> +		die "unknown error\n";
> +	    }
> +	}
> +	$status = 'success';
> +    };

I'd structure this this differently (the unknown error should not be 
possible to reach, and this makes it more complicated than necessary 
IMHO):

my $arch = eval { run... };

now if $@ is set, $detect_arch died and we have an error (and can do the 
fallback, like before).

if $@ is not set, but $arch is undef, we know we've hit the timeout (as 
run_fork_with_timeout does not die when it runs into a timeout, but 
returns no result and prints a 'got timeout' warning).

finally, if $@ is not set, but $arch is defined, architecture detection 
worked.

you can of course keep $@ as $err and re-order the conditions to have

if (no $arch and no $err) {
  die # timeout
} elsif ($err) {
  # fallback
} else {
  # ok
}

> +
> +    my $err = $@;
> +    if ($status eq 'timeout') {
> +	# on timeout
> +	die "Architecture detection failed: $err";  # $err ends with \n
> +    } elsif ($status eq 'error') {
> +	# any other error
>  	$arch = 'amd64';
> -	print "Architecture detection failed: $err\nFalling back to amd64.\n" .
> +	print "Architecture detection failed: $err" .  # $err ends with \n
> +	      "Falling back to $arch.\n" .
>  	      "Use `pct set VMID --arch ARCH` to change.\n";
>      } else {
>  	print "Detected container architecture: $arch\n";
> -- 
> 2.30.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
>