[pve-devel] [PATCH qemu-server 1/2] Fix #2062: New timeout switch for guest cmd

[Dominik Csapak]

looks good, one comment inline

On 04/03/2019 15:24, Rhonda D'Vine wrote:
> The guest cmd commands set different timeouts.  Some of those might take
> longer, so for debugging purposes it would be useful to allow overriding
> the internal timeout setting.
> 
> Pulling the timeout definition into the function registration allows it
> to appear in the API documentation.
> 
> Signed-off-by: Rhonda D'Vine <rhonda at proxmox.com>
> ---
>   PVE/API2/Qemu/Agent.pm | 26 +++++++++++++++++++++++---
>   PVE/QMPClient.pm       |  9 ---------
>   2 files changed, 23 insertions(+), 12 deletions(-)
> 
> diff --git a/PVE/API2/Qemu/Agent.pm b/PVE/API2/Qemu/Agent.pm
> index 839146c..4e3dc01 100644
> --- a/PVE/API2/Qemu/Agent.pm
> +++ b/PVE/API2/Qemu/Agent.pm
> @@ -18,6 +18,8 @@ my $MAX_READ_SIZE = 16 * 1024 * 1024; # 16 MiB
>   # list of commands
>   # will generate one api endpoint per command
>   # needs a 'method' property and optionally a 'perms' property (default VM.Monitor)
> +# 'timeout' defines an optional custom timeout in seconds for the command,
> +# the default is 3
>   my $guest_agent_commands = {
>       'ping' => {
>   	method => 'POST',
> @@ -33,9 +35,15 @@ my $guest_agent_commands = {
>       },
>       'fsfreeze-freeze' => {
>   	method => 'POST',
> +	# freeze syncs all guest FS, if we kill it it stays in an unfreezable
> +	# locked state with high probability, so use an generous timeout
> +	timeout => 60*60, # 1 hour
>       },
>       'fsfreeze-thaw' => {
>   	method => 'POST',
> +	# thaw has no possible long blocking actions, either it returns
> +	# instantly or never (dead locked)
> +	timeout => 10,
>       },
>       'fstrim' => {
>   	method => 'POST',
> @@ -66,6 +74,7 @@ my $guest_agent_commands = {
>       },
>       'shutdown' => {
>   	method => 'POST',
> +	timeout => 10*60, # 10 mins
>       },
>       # added since qemu 2.9
>       'get-host-name' => {
> @@ -130,7 +139,7 @@ __PACKAGE__->register_method({
>       }});
>   
>   sub register_command {
> -    my ($class, $command, $method, $perm) = @_;
> +    my ($class, $command, $method, $perm, $timeout) = @_;
>   
>       die "no method given\n" if !$method;
>       die "no command given\n" if !defined($command);
> @@ -144,6 +153,9 @@ sub register_command {
>   	$permission = { check => [ 'perm', '/vms/{vmid}', [ $perm ]]};
>       }
>   
> +    # see PVE/QMPClient.pm sub cmd for default value
> +    $timeout = 3 if !defined($timeout);
> +
>       my $parameters = {
>   	additionalProperties => 0,
>   	properties => {
> @@ -155,6 +167,13 @@ sub register_command {
>   		description => "The QGA command.",
>   		enum => [ sort keys %$guest_agent_commands ],
>   	    },
> +	    timeout => {
> +		description => "Timeout in seconds. Default depends on command.",
> +		type => 'integer',
> +		default => $timeout,
> +		minimum => 1,
> +		optional => 1,
> +	    }
>   	},
>       };
>   
> @@ -190,7 +209,8 @@ sub register_command {
>   	    agent_available($vmid, $conf);
>   
>   	    my $cmd = $param->{command} // $command;
> -	    my $res = PVE::QemuServer::vm_mon_cmd($vmid, "guest-$cmd");
> +	    my $res = PVE::QemuServer::vm_mon_cmd($vmid, "guest-$cmd",
> +		defined($param->{timeout}) ? (timeout => $param->{timeout}) : () );
>   
>   	    return { result => $res };
>   	}});
> @@ -201,7 +221,7 @@ __PACKAGE__->register_command('', 'POST');
>   
>   for my $cmd (sort keys %$guest_agent_commands) {
>       my $props = $guest_agent_commands->{$cmd};
> -    __PACKAGE__->register_command($cmd, $props->{method}, $props->{perms});
> +    __PACKAGE__->register_command($cmd, $props->{method}, $props->{perms}, $props->{timeout});
>   }
>   
>   # commands with parameters are complicated and we want to register them manually
> diff --git a/PVE/QMPClient.pm b/PVE/QMPClient.pm
> index 6be4a41..ac2c728 100755
> --- a/PVE/QMPClient.pm
> +++ b/PVE/QMPClient.pm
> @@ -108,14 +108,6 @@ sub cmd {
>   	    $timeout = 60*60; # 1 hour
>   	} elsif ($cmd->{execute} =~ m/^(eject|change)/) {
>   	    $timeout = 60; # note: cdrom mount command is slow
> -	} elsif ($cmd->{execute} eq 'guest-fsfreeze-freeze') {
> -	    # freeze syncs all guest FS, if we kill it it stays in an unfreezable
> -	    # locked state with high probability, so use an generous timeout
> -	    $timeout = 60*60; # 1 hour
> -	} elsif ($cmd->{execute} eq 'guest-fsfreeze-thaw') {
> -	    # thaw has no possible long blocking actions, either it returns
> -	    # instantly or never (dead locked)
> -	    $timeout = 10;
>   	} elsif ($cmd->{execute} eq 'savevm-start' ||
>   		 $cmd->{execute} eq 'savevm-end' ||
>   		 $cmd->{execute} eq 'query-backup' ||
> @@ -125,7 +117,6 @@ sub cmd {
>   		 $cmd->{execute} eq 'backup-cancel' ||
>   		 $cmd->{execute} eq 'query-savevm' ||
>   		 $cmd->{execute} eq 'delete-drive-snapshot' ||
> -		 $cmd->{execute} eq 'guest-shutdown' ||
>   		 $cmd->{execute} eq 'blockdev-snapshot-internal-sync' ||
>   		 $cmd->{execute} eq 'blockdev-snapshot-delete-internal-sync' ||
>   		 $cmd->{execute} eq 'snapshot-drive'  ) {
> 

if we want to remove those timeouts from here, we have to make sure we 
set them everywhere

we call some of the guest commands internally, e.g. fsfreeze in
PVE/VZDump/QemuServer.pm:414
PVE/QemuConfig.pm:175

etc.

it would be great of course if we would be able to get the defaults
from one place instead of hardcoding them on several levels...