From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 7F05B6BC75 for ; Fri, 19 Feb 2021 17:42:01 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 6A7731B642 for ; Fri, 19 Feb 2021 17:41:31 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [212.186.127.180]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 922981B633 for ; Fri, 19 Feb 2021 17:41:29 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 56CB04497C for ; Fri, 19 Feb 2021 17:41:29 +0100 (CET) Message-ID: <28fbc813-d29f-1bfb-7e9c-622e45b54ac5@proxmox.com> Date: Fri, 19 Feb 2021 17:41:27 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:86.0) Gecko/20100101 Thunderbird/86.0 Content-Language: en-US To: Proxmox VE development discussion , Hannes Laimer References: <20210209103124.1949709-1-h.laimer@proxmox.com> <20210209103124.1949709-2-h.laimer@proxmox.com> From: Thomas Lamprecht In-Reply-To: <20210209103124.1949709-2-h.laimer@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.058 Adjusted score from AWL reputation of From: address KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -0.001 Looks like a legit reply (A) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [proxmox.com, nodes.pm] Subject: Re: [pve-devel] [PATCH v2 pve-manager 1/2] api2: add suspendall endpoint X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 19 Feb 2021 16:42:01 -0000 On 09.02.21 11:31, Hannes Laimer wrote: > Handels pause and hibernation, the reason for not splitting it was to mirror > the behaviour of the already existing suspend endpoint for single VMs. > > Signed-off-by: Hannes Laimer > --- > > Endpoint code is mostly taken from already existing ednpoints, namely > stopall and startall. > > PVE/API2/Nodes.pm | 119 ++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 119 insertions(+) > > diff --git a/PVE/API2/Nodes.pm b/PVE/API2/Nodes.pm > index 8172231e..3e6e9fa2 100644 > --- a/PVE/API2/Nodes.pm > +++ b/PVE/API2/Nodes.pm > @@ -1943,6 +1943,125 @@ __PACKAGE__->register_method ({ > return $rpcenv->fork_worker('stopall', undef, $authuser, $code); > }}); > > +my $create_suspend_worker = sub { > + my ($nodename, $type, $vmid, $down_timeout, $todisk) = @_; > + > + my $upid; > + if ($type eq 'qemu') { > + return if !PVE::QemuServer::check_running($vmid, 1); > + my $timeout = defined($down_timeout) ? int($down_timeout) : 60*3; > + print STDERR "Suspending VM $vmid (timeout = $timeout seconds)\n"; > + $upid = PVE::API2::Qemu->vm_suspend({node => $nodename, vmid => $vmid, todisk => $todisk}); > + } else { > + die "suspension is only supported on VMs, not on '$type'\n"; > + } > + > + return $upid; > +}; > + > +__PACKAGE__->register_method ({ > + name => 'suspendall', > + path => 'suspendall', > + method => 'POST', > + protected => 1, > + permissions => { > + check => ['perm', '/', [ 'VM.PowerMgmt' ]], permissions are still unchanged? From: https://pve.proxmox.com/pve-docs/api-viewer/index.html#/nodes/{node}/qemu/{vmid}/status/suspend "You need 'VM.PowerMgmt' on /vms/{vmid}, and if you have set 'todisk', you need also 'VM.Config.Disk' on /vms/{vmid} and 'Datastore.AllocateSpace' on the storage for the vmstate." But you call PVE::API2::Qemu->vm_suspend directly, so all schema based checks, i.e., those not done there in code directly, get circumvented. Did you checked that this is OK?? > + }, > + proxyto => 'node', > + description => "Suspend all VMs.", > + parameters => { > + additionalProperties => 0, > + properties => { > + node => get_standard_option('pve-node'), > + vms => { > + description => "Only consider Guests with these IDs.", > + type => 'string', format => 'pve-vmid-list', > + optional => 1, > + }, > + todisk => { > + type => 'boolean', > + default => 0, I'd even enable this by defaults, prime use case. Could be defaulted to true via gui to, as alternative. > + optional => 1, > + description => 'If set, suspends the VM to disk. Will be resumed on next VM start.', > + }, a state storage maybe also useful, some may even want a mapping per VM? could be overkill though as people can already configure state storage in the VM options. > + }, > + }, > + returns => { > + type => 'string', > + }, > + code => sub { > + my ($param) = @_; > + > + my $rpcenv = PVE::RPCEnvironment::get(); > + my $authuser = $rpcenv->get_user(); > + > + my $nodename = $param->{node}; > + $nodename = PVE::INotify::nodename() if $nodename eq 'localhost'; > + > + my $code = sub { > + > + $rpcenv->{type} = 'priv'; # to start tasks in background > + > + my $stopList = &$get_start_stop_list($nodename, undef, $param->{vms}); > + > + my $cpuinfo = PVE::ProcFSTools::read_cpuinfo(); > + my $datacenterconfig = cfs_read_file('datacenter.cfg'); > + # if not set by user spawn max cpu count number of workers > + my $maxWorkers = $datacenterconfig->{max_workers} || $cpuinfo->{cpus}; > + > + foreach my $order (sort {$b <=> $a} keys %$stopList) { > + my $vmlist = $stopList->{$order}; > + my $workers = {}; > + > + my $finish_worker = sub { > + my $pid = shift; > + my $d = $workers->{$pid}; > + return if !$d; > + delete $workers->{$pid}; > + > + syslog('info', "end task $d->{upid}"); > + }; still not factored out > + > + foreach my $vmid (sort {$b <=> $a} keys %$vmlist) { > + my $d = $vmlist->{$vmid}; > + my $upid; > + eval { $upid = &$create_suspend_worker($nodename, $d->{type}, $vmid, $d->{down}, $param->{todisk}); }; > + warn $@ if $@; > + next if !$upid; > + > + my $res = PVE::Tools::upid_decode($upid, 1); > + next if !$res; > + > + my $pid = $res->{pid}; > + > + $workers->{$pid} = { type => $d->{type}, upid => $upid, vmid => $vmid }; > + while (scalar(keys %$workers) >= $maxWorkers) { > + foreach my $p (keys %$workers) { > + if (!PVE::ProcFSTools::check_process_running($p)) { > + &$finish_worker($p); > + } > + } > + sleep(1); still not factored out > + } > + } > + while (scalar(keys %$workers)) { > + foreach my $p (keys %$workers) { > + if (!PVE::ProcFSTools::check_process_running($p)) { > + &$finish_worker($p); > + } > + } > + sleep(1); > + } still not factored out... did you even read my last review?! > + } > + > + syslog('info', "all VMs suspended"); > + > + return; > + }; > + > + return $rpcenv->fork_worker('suspendall', undef, $authuser, $code); > + }}); > + > my $create_migrate_worker = sub { > my ($nodename, $type, $vmid, $target, $with_local_disks) = @_; > >