Re: [pve-devel] [PATCH pve-manager 1/2] api: add suspendall endpoint

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

Am 18/10/2023 um 12:34 schrieb Hannes Laimer:
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
> 
> code is mostly taken from the already existing stopal endpoint, since
> all checks are basically the same for both suspend and stop.
> 
>  PVE/API2/Nodes.pm | 118 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 118 insertions(+)
> 
> diff --git a/PVE/API2/Nodes.pm b/PVE/API2/Nodes.pm
> index 0843c3a3..bb77474f 100644
> --- a/PVE/API2/Nodes.pm
> +++ b/PVE/API2/Nodes.pm
> @@ -286,6 +286,7 @@ __PACKAGE__->register_method ({
>  	    { name => 'spiceshell' },
>  	    { name => 'startall' },
>  	    { name => 'status' },
> +	    { name => 'suspendall' },

this list is alphanumerical sorted, please insert the new entry that way
too.

>  	    { name => 'stopall' },
>  	    { name => 'storage' },
>  	    { name => 'subscription' },
> @@ -2019,6 +2020,123 @@ __PACKAGE__->register_method ({
>  	return $rpcenv->fork_worker('stopall', undef, $authuser, $code);
>      }});
>  
> +my $create_suspend_worker = sub {
> +    my ($nodename, $vmid) = @_;
> +    return if !PVE::QemuServer::check_running($vmid, 1);

what if one passes VMIDs for Containers? should be either checked early and
errored out, or explictily returned early here to avoid strange effects.

> +    print STDERR "Suspending VM $vmid\n";
> +    return PVE::API2::Qemu->vm_suspend(
> +	{ node => $nodename, vmid => $vmid, todisk => 1 }
> +    );
> +};
> +
> +__PACKAGE__->register_method ({
> +    name => 'suspendall',
> +    path => 'suspendall',
> +    method => 'POST',
> +    protected => 1,
> +    permissions => {
> +	description => "The 'VM.PowerMgmt' permission is required on '/' or on '/vms/<ID>' for "
> +	    ."each ID passed via the 'vms' parameter.",

not complete, as we pass todisk to the actual suspend API call, there the access
for 'VM.Config.Disk' and 'Datastore.AllocateSpace' on the resolved storage is also
tested.

I had the following diff as follow-up, but due to the container VMID not being handled
it might make more sense to send a further revision.

diff --git a/PVE/API2/Nodes.pm b/PVE/API2/Nodes.pm
index f2f08dd7..995f0086 100644
--- a/PVE/API2/Nodes.pm
+++ b/PVE/API2/Nodes.pm
@@ -2027,8 +2027,9 @@ __PACKAGE__->register_method ({
     method => 'POST',
     protected => 1,
     permissions => {
-       description => "The 'VM.PowerMgmt' permission is required on '/' or on '/vms/<ID>' for "
-           ."each ID passed via the 'vms' parameter.",
+       description => "The 'VM.PowerMgmt' permission is required on '/' or on '/vms/<ID>' for each"
+           ." ID passed via the 'vms' parameter. Additionally, you need 'VM.Config.Disk' on  the"
+           ." '/vms/{vmid}' path and 'Datastore.AllocateSpace' for the configured state-storage(s)",
        user => 'all',
     },
     proxyto => 'node',
@@ -2053,12 +2054,13 @@ __PACKAGE__->register_method ({
        my $rpcenv = PVE::RPCEnvironment::get();
        my $authuser = $rpcenv->get_user();
 
-       if (!$rpcenv->check($authuser, "/", [ 'VM.PowerMgmt' ], 1)) {
+       # we cannot really check access to the state-storage here, that's happening per worker.
+       if (!$rpcenv->check($authuser, "/", [ 'VM.PowerMgmt', 'VM.Config.Disk' ], 1)) {
            my @vms = PVE::Tools::split_list($param->{vms});
            if (scalar(@vms) > 0) {
-               $rpcenv->check($authuser, "/vms/$_", [ 'VM.PowerMgmt' ]) for @vms;
+               $rpcenv->check($authuser, "/vms/$_", [ 'VM.PowerMgmt', 'VM.Config.Disk' ]) for @vms;
            } else {
-               raise_perm_exc("/, VM.PowerMgmt");
+               raise_perm_exc("/, VM.PowerMgmt && VM.Config.Disk");
            }
        }