[pve-devel] [PATCH qemu-server v2 3/5] api: add reboot api call

Thomas Lamprecht t.lamprecht at proxmox.com
Thu Sep 5 12:03:19 CEST 2019


On 23.08.19 10:55, Dominik Csapak wrote:
> this creates a reboot trigger file (inspired by pve-container)
> and relies on the 'qm cleanup' call by the qmeventd to detect
> and restart the vm afterwards
> 
> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
> ---
> changes from rfc:
> * use PVE::QemuServer:vm_stop instead of the api call, to prevent
>   the ha manager to change the target state
> * create the trigger only during the lock in vm_stop, to be sure we are
>   actually shutting down
> 
>  PVE/API2/Qemu.pm  | 63 +++++++++++++++++++++++++++++++++++++++++++++++
>  PVE/QemuServer.pm |  4 ++-
>  2 files changed, 66 insertions(+), 1 deletion(-)
> 
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index b30931d..5094721 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -2339,6 +2339,69 @@ __PACKAGE__->register_method({
>  	}
>      }});
>  
> +__PACKAGE__->register_method({
> +    name => 'vm_reboot',
> +    path => '{vmid}/status/reboot',
> +    method => 'POST',
> +    protected => 1,
> +    proxyto => 'node',
> +    description => "Reboot the VM by shutting it down, and starting it again.",
> +    permissions => {
> +	check => ['perm', '/vms/{vmid}', [ 'VM.PowerMgmt' ]],
> +    },
> +    parameters => {
> +	additionalProperties => 0,
> +	properties => {
> +	    node => get_standard_option('pve-node'),
> +	    vmid => get_standard_option('pve-vmid',
> +					{ completion => \&PVE::QemuServer::complete_vmid_running }),
> +	    timeout => {
> +		description => "Wait maximal timeout seconds for the shutdown.",
> +		type => 'integer',
> +		minimum => 0,
> +		optional => 1,
> +	    },
> +	},
> +    },
> +    returns => {
> +	type => 'string',
> +    },
> +    code => sub {
> +	my ($param) = @_;
> +
> +	my $rpcenv = PVE::RPCEnvironment::get();
> +	my $authuser = $rpcenv->get_user();
> +
> +	my $node = extract_param($param, 'node');
> +	my $vmid = extract_param($param, 'vmid');
> +
> +	my $storecfg = PVE::Storage::config();
> +
> +	my $shutdown = 1;
> +	my $reboot = 1;

variable declaration vs use. locality is not too good here, IMO.

$storecfg could be moved below; above the vm_stop call, the two
variables which try to improve readability could be also moved there.

> +
> +	my $qmpstatus = eval {
> +	    PVE::QemuServer::vm_qmp_command($vmid, { execute => "query-status" }, 0);
> +	};
> +	my $err = $@ if $@;
> +
> +	if (!$err && $qmpstatus->{status} eq "paused") {
> +	    die "VM is paused - cannot shutdown\n";
> +	}
> +
> +	my $realcmd = sub {
> +	    my $upid = shift;
> +
> +	    syslog('info', "shutdown VM $vmid: $upid\n");

s/shutdown/reboot/

> +
> +	    PVE::QemuServer::vm_stop($storecfg, $vmid, undef, 0, $param->{timeout},
> +		$shutdown, undef, undef, undef, $reboot);
> +	    return;
> +	};
> +
> +	return $rpcenv->fork_worker('qmshutdown', $vmid, $authuser, $realcmd);
> +    }});
> +
>  __PACKAGE__->register_method({
>      name => 'vm_suspend',
>      path => '{vmid}/status/suspend',
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index cc0c95e..ab4ee3e 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -5739,7 +5739,7 @@ sub vm_stop_cleanup {
>  # We need that when migration VMs to other nodes (files already moved)
>  # Note: we set $keepActive in vzdump stop mode - volumes need to stay active
>  sub vm_stop {
> -    my ($storecfg, $vmid, $skiplock, $nocheck, $timeout, $shutdown, $force, $keepActive, $migratedfrom) = @_;
> +    my ($storecfg, $vmid, $skiplock, $nocheck, $timeout, $shutdown, $force, $keepActive, $migratedfrom, $reboot) = @_;

I'd like to rather avoid adding such a parameter to this mess of
a method signature..

Also, your call to this above has 4 undefs and a 0 which could be undef,
so 5 fixed params..

Maybe move the actual anonymous sub here doing the shutdown in locked
context to a "_do_vm_stop method" (with comment that it's required to call
it in locked context) and add your own "vm_reboot" method with a reduced
method-signature, which just calls "_do_vm_stop" and the "create_reboot_trigger"
(or "create_reboot_request", if you agree with the review of 1/5) methods.

would be IMO much nicer. 

>  
>      $force = 1 if !defined($force) && !$shutdown;
>  
> @@ -5756,6 +5756,8 @@ sub vm_stop {
>  	my $pid = check_running($vmid, $nocheck);
>  	return if !$pid;
>  
> +	create_reboot_trigger($vmid) if $reboot;

maybe a comment to the create_reboot... call that it's handled by the qm eventd
called post stop hooks would be great, else people reading this will question where
or how it happens.

> +
>  	my $conf;
>  	if (!$nocheck) {
>  	    $conf = PVE::QemuConfig->load_config($vmid);
> 





More information about the pve-devel mailing list