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

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

[Lorenz Stechauner]

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';
+    };
+
+    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

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

[Fabian Grünbichler]

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

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

[Lorenz Stechauner]

ping

On 29.06.21 15:53, 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';
> +    };
> +
> +    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";