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

Dominik Csapak d.csapak at proxmox.com
Tue Mar 5 15:02:39 CET 2019


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






More information about the pve-devel mailing list