Re: [pve-devel] [PATCH guest-common v2 1/6] guest helpers: add helper to overrule active tasks of a specific type

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

Am 30/01/2024 um 18:10 schrieb Friedrich Weber:

Maybe start of with "Add a helper to abort all tasks from a specific
(type, user, vmid) tuple. It will be used ...

> This helper is used to abort any active qmshutdown/vzshutdown tasks
> before attempting to stop a VM/CT (if requested).
>
> Signed-off-by: Friedrich Weber <f.weber@proxmox.com>
> ---
> 
> Notes:
>     no changes v1 -> v2
> 
>  src/PVE/GuestHelpers.pm | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/src/PVE/GuestHelpers.pm b/src/PVE/GuestHelpers.pm
> index 961a7b8..bd94ed2 100644
> --- a/src/PVE/GuestHelpers.pm
> +++ b/src/PVE/GuestHelpers.pm
> @@ -416,4 +416,22 @@ sub check_vnet_access {
>  	if !($tag || $trunks);
>  }
>  
> +sub overrule_tasks {

hmm, overruling is the thing you want to do now, but one might
be use this for other reasons, so maybe naming it about what it
does would be a bit better compared to what is used for (now).

From top of my head that could be "abort_guest_tasks", but maybe
you got a better idea.

> +    my ($type, $user, $vmid) = @_;
> +
> +    my $active_tasks = PVE::INotify::read_file('active');
> +    my $res = [];
> +    for my $task (@$active_tasks) {
> +       if (!$task->{saved}
> +           && $task->{type} eq $type
> +           && $task->{user} eq $user

This also means that e.g. root@pam cannot overrule a task started by
apprentice@pam, which might be something admins what to do (they like
overruling users after all ;-P). Or some automation triggering the
shutdown (using a token with a separate user) might be also a good
examples of things I'd like to be able to overrule. 

Maybe make this optional and only enforce it for users that do not
have some more powerful priv?

Or does it even make sense to check this at all?
As long as the user has the rights to execute a stop they probably
should also be able to force it at any time, even for other users?

> +           && $task->{id} eq $vmid
> +       ) {

meh, the pre-existing $killit param is way too subtle for my taste...
Some %param that takes a `kill => "reason"` property for this would be
much more telling.

But changing that is a bit out of scope, a comment would be great for
now though.

> +           PVE::RPCEnvironment->check_worker($task->{upid}, 1);
> +           push @$res, $task->{upid};

renaming $res to $killed_tasks would also help in reading this code out
of further context.