[pve-devel] [PATCH qemu-server v3 4/4] api: add reboot api call

Thomas Lamprecht t.lamprecht at proxmox.com
Tue Sep 10 15:17:33 CEST 2019


On 06.09.19 14:24, 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
> 
> includes some refactoring of the 'vm_stop' call, so that
> we can have a clean 'vm_reboot' sub without the many parameters
> of 'vm_stop'
> 
> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
> ---
> better view with '-w' as it is mostly code shift
> 
> changes from v2:
> * refactored vm_stop so that the code can be reused in vm_reboot

looks better, but I'd done the refactoring in a separate no-semantic-change
patch first.

> * better function signature
> * add vm running check in api, such that it cannot be called on
>   stopped vms (which would not start the vm)
> 
>  PVE/API2/Qemu.pm  |  60 +++++++++++++++++
>  PVE/CLI/qm.pm     |   2 +
>  PVE/QemuServer.pm | 162 ++++++++++++++++++++++++++--------------------
>  3 files changed, 153 insertions(+), 71 deletions(-)
> 
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 9db8967..fb63cab 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -1910,6 +1910,7 @@ __PACKAGE__->register_method({
>  	    { subdir => 'reset' },
>  	    { subdir => 'shutdown' },
>  	    { subdir => 'suspend' },
> +	    { subdir => 'reboot' },
>  	    ];
>  
>  	return $res;
> @@ -2333,6 +2334,65 @@ __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.",

we could add a hint that thus pending changes get applied? Or do you think
that this is clear and expected?

> +    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 $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";
> +	}
> +
> +	die "VM $vmid not running\n" if !PVE::QemuServer::check_running($vmid);
> +
> +	my $realcmd = sub {
> +	    my $upid = shift;
> +
> +	    syslog('info', "requesting reboot of VM $vmid: $upid\n");
> +	    my $storecfg = PVE::Storage::config();

hmm, I'd maybe move the storecfg read into vm_reboot, removing it from the
method parameter signature? As nothing else is done with it here anyway?

> +	    PVE::QemuServer::vm_reboot($storecfg, $vmid, $param->{timeout});
> +	    return;
> +	};
> +
> +	return $rpcenv->fork_worker('qmreboot', $vmid, $authuser, $realcmd);
> +    }});
> +
>  __PACKAGE__->register_method({
>      name => 'vm_suspend',
>      path => '{vmid}/status/suspend',
> diff --git a/PVE/CLI/qm.pm b/PVE/CLI/qm.pm
> index dea3deb..3ec8a8c 100755
> --- a/PVE/CLI/qm.pm
> +++ b/PVE/CLI/qm.pm
> @@ -1000,6 +1000,8 @@ our $cmddef = {
>  
>      shutdown => [ "PVE::API2::Qemu", 'vm_shutdown', ['vmid'], { node => $nodename }, $upid_exit ],
>  
> +    reboot => [ "PVE::API2::Qemu", 'vm_reboot', ['vmid'], { node => $nodename }, $upid_exit ],
> +
>      suspend => [ "PVE::API2::Qemu", 'vm_suspend', ['vmid'], { node => $nodename }, $upid_exit ],
>  
>      resume => [ "PVE::API2::Qemu", 'vm_resume', ['vmid'], { node => $nodename }, $upid_exit ],
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index ba83b10..a1d4969 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -5773,85 +5773,42 @@ sub vm_stop_cleanup {
>      warn $@ if $@; # avoid errors - just warn
>  }
>  
> -# Note: use $nockeck to skip tests if VM configuration file exists.
> -# 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) = @_;
> +# call only in locked context
> +sub _do_vm_stop {
> +    my ($storecfg, $vmid, $skiplock, $nocheck, $timeout, $shutdown, $force, $keepActive) = @_;
>  
> -    $force = 1 if !defined($force) && !$shutdown;
> +    my $pid = check_running($vmid, $nocheck);
> +    return if !$pid;
>  
> -    if ($migratedfrom){
> -	my $pid = check_running($vmid, $nocheck, $migratedfrom);
> -	kill 15, $pid if $pid;
> -	my $conf = PVE::QemuConfig->load_config($vmid, $migratedfrom);
> -	vm_stop_cleanup($storecfg, $vmid, $conf, $keepActive, 0);
> -	return;
> -    }
> -
> -    PVE::QemuConfig->lock_config($vmid, sub {
> -
> -	my $pid = check_running($vmid, $nocheck);
> -	return if !$pid;
> -
> -	my $conf;
> -	if (!$nocheck) {
> -	    $conf = PVE::QemuConfig->load_config($vmid);
> -	    PVE::QemuConfig->check_lock($conf) if !$skiplock;
> -	    if (!defined($timeout) && $shutdown && $conf->{startup}) {
> -		my $opts = PVE::JSONSchema::pve_parse_startup_order($conf->{startup});
> -		$timeout = $opts->{down} if $opts->{down};
> -	    }
> -	    PVE::GuestHelpers::exec_hookscript($conf, $vmid, 'pre-stop');
> +    my $conf;
> +    if (!$nocheck) {
> +	$conf = PVE::QemuConfig->load_config($vmid);
> +	PVE::QemuConfig->check_lock($conf) if !$skiplock;
> +	if (!defined($timeout) && $shutdown && $conf->{startup}) {
> +	    my $opts = PVE::JSONSchema::pve_parse_startup_order($conf->{startup});
> +	    $timeout = $opts->{down} if $opts->{down};
>  	}
> +	PVE::GuestHelpers::exec_hookscript($conf, $vmid, 'pre-stop');
> +    }
>  
> -	eval {
> -	    if ($shutdown) {
> -		if (defined($conf) && parse_guest_agent($conf)->{enabled}) {
> -		    vm_qmp_command($vmid, {
> +    eval {
> +	if ($shutdown) {
> +	    if (defined($conf) && parse_guest_agent($conf)->{enabled}) {
> +		vm_qmp_command($vmid, {
>  			execute => "guest-shutdown",
>  			arguments => { timeout => $timeout }
>  		    }, $nocheck);
> -		} else {
> -		    vm_qmp_command($vmid, { execute => "system_powerdown" }, $nocheck);
> -		}
> -	    } else {
> -		vm_qmp_command($vmid, { execute => "quit" }, $nocheck);
> -	    }
> -	};
> -	my $err = $@;
> -
> -	if (!$err) {
> -	    $timeout = 60 if !defined($timeout);
> -
> -	    my $count = 0;
> -	    while (($count < $timeout) && check_running($vmid, $nocheck)) {
> -		$count++;
> -		sleep 1;
> -	    }
> -
> -	    if ($count >= $timeout) {
> -		if ($force) {
> -		    warn "VM still running - terminating now with SIGTERM\n";
> -		    kill 15, $pid;
> -		} else {
> -		    die "VM quit/powerdown failed - got timeout\n";
> -		}
>  	    } else {
> -		vm_stop_cleanup($storecfg, $vmid, $conf, $keepActive, 1) if $conf;
> -		return;
> +		vm_qmp_command($vmid, { execute => "system_powerdown" }, $nocheck);
>  	    }
>  	} else {
> -	    if ($force) {
> -		warn "VM quit/powerdown failed - terminating now with SIGTERM\n";
> -		kill 15, $pid;
> -	    } else {
> -		die "VM quit/powerdown failed\n";
> -	    }
> +	    vm_qmp_command($vmid, { execute => "quit" }, $nocheck);
>  	}
> +    };
> +    my $err = $@;
>  
> -	# wait again
> -	$timeout = 10;
> +    if (!$err) {
> +	$timeout = 60 if !defined($timeout);
>  
>  	my $count = 0;
>  	while (($count < $timeout) && check_running($vmid, $nocheck)) {
> @@ -5860,12 +5817,75 @@ sub vm_stop {
>  	}
>  
>  	if ($count >= $timeout) {
> -	    warn "VM still running - terminating now with SIGKILL\n";
> -	    kill 9, $pid;
> -	    sleep 1;
> +	    if ($force) {
> +		warn "VM still running - terminating now with SIGTERM\n";
> +		kill 15, $pid;
> +	    } else {
> +		die "VM quit/powerdown failed - got timeout\n";
> +	    }
> +	} else {
> +	    vm_stop_cleanup($storecfg, $vmid, $conf, $keepActive, 1) if $conf;
> +	    return;
>  	}
> +    } else {
> +	if ($force) {
> +	    warn "VM quit/powerdown failed - terminating now with SIGTERM\n";
> +	    kill 15, $pid;
> +	} else {
> +	    die "VM quit/powerdown failed\n";
> +	}
> +    }
> +
> +    # wait again
> +    $timeout = 10;
> +
> +    my $count = 0;
> +    while (($count < $timeout) && check_running($vmid, $nocheck)) {
> +	$count++;
> +	sleep 1;
> +    }
> +
> +    if ($count >= $timeout) {
> +	warn "VM still running - terminating now with SIGKILL\n";
> +	kill 9, $pid;
> +	sleep 1;
> +    }
> +
> +    vm_stop_cleanup($storecfg, $vmid, $conf, $keepActive, 1) if $conf;
> +}
> +
> +# Note: use $nockeck to skip tests if VM configuration file exists.

s/nockeck/nocheck

> +# 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) = @_;
> +
> +    $force = 1 if !defined($force) && !$shutdown;
> +
> +    if ($migratedfrom){
> +	my $pid = check_running($vmid, $nocheck, $migratedfrom);
> +	kill 15, $pid if $pid;
> +	my $conf = PVE::QemuConfig->load_config($vmid, $migratedfrom);
> +	vm_stop_cleanup($storecfg, $vmid, $conf, $keepActive, 0);
> +	return;
> +    }
> +
> +    PVE::QemuConfig->lock_config($vmid, sub {
> +	_do_vm_stop($storecfg, $vmid, $skiplock, $nocheck, $timeout, $shutdown, $force, $keepActive);
> +   });
> +}
> +
> +sub vm_reboot {
> +    my ($storecfg, $vmid, $timeout) = @_;
> +
> +    PVE::QemuConfig->lock_config($vmid, sub {
> +	# do not request reboot if it is not running, since
> +	# it can only start again if it qmeventd detects that it stopped

double negation, maybe:

# only reboot if running, as a qmeventd stop event starts it again

> +	return if !check_running($vmid);
> +
> +	create_reboot_request($vmid);
>  
> -	vm_stop_cleanup($storecfg, $vmid, $conf, $keepActive, 1) if $conf;
> +	_do_vm_stop($storecfg, $vmid, undef, undef, $timeout, 1);
>     });
>  }
>  
> 





More information about the pve-devel mailing list