[pve-devel] [PATCH pve-manager 0/2] add pause bulk action

[pve-devel] [PATCH pve-manager 0/2] add pause bulk action

[Hannes Laimer]

Add "Pause" to available bulk actions, only VMs can be paused(suspended).

Hannes Laimer (2):
  api2: add suspendall endpoint
  ui: add "Bulk Pause" action to node

 PVE/API2/Nodes.pm               | 113 ++++++++++++++++++++++++++++++++
 www/manager6/Utils.js           |   1 +
 www/manager6/form/VMSelector.js |  10 +++
 www/manager6/node/CmdMenu.js    |  15 +++++
 www/manager6/node/Config.js     |  13 ++++
 5 files changed, 152 insertions(+)

-- 
2.20.1

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

[Hannes Laimer]

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 PVE/API2/Nodes.pm | 113 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 113 insertions(+)

diff --git a/PVE/API2/Nodes.pm b/PVE/API2/Nodes.pm
index 8172231e..0c11fe35 100644
--- a/PVE/API2/Nodes.pm
+++ b/PVE/API2/Nodes.pm
@@ -1943,6 +1943,119 @@ __PACKAGE__->register_method ({
 	return $rpcenv->fork_worker('stopall', undef, $authuser, $code);
     }});
 
+my $create_suspend_worker = sub {
+    my ($nodename, $type, $vmid, $down_timeout) = @_;
+
+    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});
+    } 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' ]],
+    },
+    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,
+	    },
+	},
+    },
+    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}");
+		};
+
+		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}); };
+		    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);
+		    }
+		}
+		while (scalar(keys %$workers)) {
+		    foreach my $p (keys %$workers) {
+			if (!PVE::ProcFSTools::check_process_running($p)) {
+			    &$finish_worker($p);
+			}
+		    }
+		    sleep(1);
+		}
+	    }
+
+	    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) = @_;
 
-- 
2.20.1

[pve-devel] [PATCH pve-manager 2/2] ui: add "Bulk Pause" action to node

[Hannes Laimer]

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 www/manager6/Utils.js           |  1 +
 www/manager6/form/VMSelector.js | 10 ++++++++++
 www/manager6/node/CmdMenu.js    | 15 +++++++++++++++
 www/manager6/node/Config.js     | 13 +++++++++++++
 4 files changed, 39 insertions(+)

diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
index 5278152d..941eb44a 100644
--- a/www/manager6/Utils.js
+++ b/www/manager6/Utils.js
@@ -1706,6 +1706,7 @@ Ext.define('PVE.Utils', {
 	    spiceshell: ['', gettext('Shell') + ' (Spice)'],
 	    startall: ['', gettext('Start all VMs and Containers')],
 	    stopall: ['', gettext('Stop all VMs and Containers')],
+	    suspendall: ['', gettext('Pause all VMs')],
 	    unknownimgdel: ['', gettext('Destroy image from unknown guest')],
 	    vncproxy: ['VM/CT', gettext('Console')],
 	    vncshell: ['', gettext('Shell')],
diff --git a/www/manager6/form/VMSelector.js b/www/manager6/form/VMSelector.js
index 6a51a73d..0c176a9b 100644
--- a/www/manager6/form/VMSelector.js
+++ b/www/manager6/form/VMSelector.js
@@ -180,6 +180,7 @@ Ext.define('PVE.form.VMSelector', {
 	// only show the relevant guests by default
 	if (me.action) {
 	    var statusfilter = '';
+	    var typefilter = '';
 	    switch (me.action) {
 		case 'startall':
 		    statusfilter = 'stopped';
@@ -187,6 +188,10 @@ Ext.define('PVE.form.VMSelector', {
 		case 'stopall':
 		    statusfilter = 'running';
 		    break;
+		case 'suspendall':
+		    statusfilter = 'running';
+		    typefilter = 'qemu';
+		    break;
 	    }
 	    if (statusfilter !== '') {
 		me.store.filters.add({
@@ -197,6 +202,11 @@ Ext.define('PVE.form.VMSelector', {
 		    operator: 'in',
 		    property: 'status',
 		    value: [statusfilter],
+		}, {
+		    id: 'x-gridfilter-type',
+		    operator: 'in',
+		    property: 'type',
+		    value: [typefilter],
 		});
 	    }
 	}
diff --git a/www/manager6/node/CmdMenu.js b/www/manager6/node/CmdMenu.js
index b650bfa0..38001343 100644
--- a/www/manager6/node/CmdMenu.js
+++ b/www/manager6/node/CmdMenu.js
@@ -60,6 +60,21 @@ Ext.define('PVE.node.CmdMenu', {
 		win.show();
 	    },
 	},
+	{
+	    text: gettext('Bulk Pause'),
+	    itemId: 'bulksuspend',
+	    iconCls: 'fa fa-fw fa-pause',
+	    handler: function() {
+		var me = this.up('menu');
+		var win = Ext.create('PVE.window.BulkAction', {
+		    nodename: me.nodename,
+		    title: gettext('Bulk Pause'),
+		    btnText: gettext('Pause'),
+		    action: 'suspendall',
+		});
+		win.show();
+	    },
+	},
 	{
 	    text: gettext('Bulk Migrate'),
 	    itemId: 'bulkmigrate',
diff --git a/www/manager6/node/Config.js b/www/manager6/node/Config.js
index ef3ac32c..55e09c58 100644
--- a/www/manager6/node/Config.js
+++ b/www/manager6/node/Config.js
@@ -63,6 +63,19 @@ Ext.define('PVE.node.Config', {
 			    win.show();
 			},
 		    },
+		    {
+			text: gettext('Bulk Pause'),
+			iconCls: 'fa fa-fw fa-pause',
+			handler: function() {
+			    var win = Ext.create('PVE.window.BulkAction', {
+				nodename: nodename,
+				title: gettext('Bulk Pause'),
+				btnText: gettext('Pause'),
+				action: 'suspendall',
+			    });
+			    win.show();
+			},
+		    },
 		    {
 			text: gettext('Bulk Migrate'),
 			iconCls: 'fa fa-fw fa-send-o',
-- 
2.20.1

Re: [pve-devel] [PATCH pve-manager 0/2] add pause bulk action

[Thomas Lamprecht]

On 05.02.21 11:35, Hannes Laimer wrote:
> Add "Pause" to available bulk actions, only VMs can be paused(suspended).
> 

as mentioned off-list, this is not pause (or at least should not be), but
hibernation or also called suspension.

There exists also a pause in QEMU, which is just pausing running the guests,
without any state saving. Here we do not want to do that, this is seldom useful,
what we actually want to do is suspend to disk, so that the whole VM state
is saved to.

Please adapt the commit message and also the web interface, as mentioned
already off-list before you sent this!

> Hannes Laimer (2):
>   api2: add suspendall endpoint
>   ui: add "Bulk Pause" action to node
> 
>  PVE/API2/Nodes.pm               | 113 ++++++++++++++++++++++++++++++++
>  www/manager6/Utils.js           |   1 +
>  www/manager6/form/VMSelector.js |  10 +++
>  www/manager6/node/CmdMenu.js    |  15 +++++
>  www/manager6/node/Config.js     |  13 ++++
>  5 files changed, 152 insertions(+)
> 

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

[Thomas Lamprecht]

On 05.02.21 11:35, Hannes Laimer wrote:

Missing reference to bug #804 which this patch addresses.
https://pve.proxmox.com/wiki/Developer_Documentation#Commits_and_Commit_Messages

Mentioning that this is mostly a 1:1 copy of the stop-, start-, all
methods would also be good; improves confidence in code and explains
why some parts seem so familiar ;-)


Looks OK code wise, but lots of code deduplication potential with the
other "start/stop/.. all" endpoints and IMO some missing thoughts on
the used API privileges.

> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
>  PVE/API2/Nodes.pm | 113 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 113 insertions(+)
> 
> diff --git a/PVE/API2/Nodes.pm b/PVE/API2/Nodes.pm
> index 8172231e..0c11fe35 100644
> --- a/PVE/API2/Nodes.pm
> +++ b/PVE/API2/Nodes.pm
> @@ -1943,6 +1943,119 @@ __PACKAGE__->register_method ({
>  	return $rpcenv->fork_worker('stopall', undef, $authuser, $code);
>      }});
>  
> +my $create_suspend_worker = sub {
> +    my ($nodename, $type, $vmid, $down_timeout) = @_;
> +
> +    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});

You could avoid the $upid and rather just return here

> +    } else {
> +	die "suspension is only supported on VMs, not on '$type'\n";


As we won't get other suspendable types in the foreseeable future we could
make this an early check.

> +    }
> +
> +    return $upid;
> +};
> +
> +__PACKAGE__->register_method ({
> +    name => 'suspendall',
> +    path => 'suspendall',
> +    method => 'POST',
> +    protected => 1,
> +    permissions => {
> +	check => ['perm', '/', [ 'VM.PowerMgmt' ]],

In contrast to the pretty much state less start and stop, supension results in
saving states to disks, so not sure if just having VM.PowerMgmt is enough?

Or at least I see no reasoning telling so in the (rather non existent) commit
message..

> +    },
> +    proxyto => 'node',
> +    description => "Suspend all VMs.",

Suspend all or a specific set of 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,
> +	    },
> +	},
> +    },
> +    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});

Use new call syntax for code references in new code:

$get_start_stop_list->(...)

> +
> +	    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};

IIRC, above lines are now duplicated two or three times, refactoring it
out into a separate method would avoid code duplication and reduce the
length of this scope.

> +
> +	    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}");
> +		};
> +
> +		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}); };

above could be shorter, i.e., write as:

my $upid = eval { $create_suspend_worker->($nodename, $d->{type}, $vmid, $d->{down}) };

> +		    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);
> +		    }
> +		}
> +		while (scalar(keys %$workers)) {
> +		    foreach my $p (keys %$workers) {
> +			if (!PVE::ProcFSTools::check_process_running($p)) {
> +			    &$finish_worker($p);
> +			}
> +		    }


we have that worker loop twice time per "stop/start/.. all" endpoint,
would be nice to be factored out in a sub which takes the hash reference
to $workers and the code from finish_worker inlined, as that is also
duplicated a few times.

> +		    sleep(1);
> +		}
> +	    }
> +
> +	    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) = @_;
>  
>