Re: [pve-devel] [PATCH pve-manager v3 1/3] fix #3903: jobs: add remove vmid from jobs helper

[Fabian Ebner <f.ebner@proxmox.com>]

Am 14.03.22 um 10:26 schrieb Hannes Laimer:
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
>  PVE/Jobs.pm        | 21 ++++++++++++++++++++-
>  PVE/Jobs/Plugin.pm |  7 +++++++
>  PVE/Jobs/VZDump.pm | 13 +++++++++++++
>  3 files changed, 40 insertions(+), 1 deletion(-)
> 
> diff --git a/PVE/Jobs.pm b/PVE/Jobs.pm
> index ba3685ec..5b2c4c7b 100644
> --- a/PVE/Jobs.pm
> +++ b/PVE/Jobs.pm
> @@ -4,7 +4,7 @@ use strict;
>  use warnings;
>  use JSON;
>  
> -use PVE::Cluster qw(cfs_read_file cfs_lock_file);
> +use PVE::Cluster qw(cfs_read_file cfs_lock_file cfs_write_file);
>  use PVE::Jobs::Plugin;
>  use PVE::Jobs::VZDump;
>  use PVE::Tools;
> @@ -274,6 +274,25 @@ sub synchronize_job_states_with_config {
>      die $@ if $@;
>  }
>  
> +sub remove_vmid_from_jobs {
> +    my ($vmid) = @_;
> +
> +    cfs_lock_file('jobs.cfg', undef, sub {
> +	my $jobs_data = cfs_read_file('jobs.cfg');
> +	for my $id (keys %{$jobs_data->{ids}}) {
> +	    my $job = $jobs_data->{ids}->{$id};
> +	    my $type = $job->{type};
> +	    my $plugin = PVE::Jobs::Plugin->lookup($type);
> +	    $jobs_data->{ids}->{$id} = $plugin->remove_vmid_from_job($job, $vmid);
> +	    if (!defined($jobs_data->{ids}->{$id})) {
> +		delete $jobs_data->{ids}->{$id};

If it's not defined, delete shouldn't be necessary anymore.

> +		remove_job($id, $type);
> +	    }
> +	}
> +        cfs_write_file('jobs.cfg', $jobs_data);
> +    });
> +}
> +
>  sub setup_dirs {
>      mkdir $state_dir;
>      mkdir $lock_dir;
> diff --git a/PVE/Jobs/Plugin.pm b/PVE/Jobs/Plugin.pm
> index 6098360b..645c460f 100644
> --- a/PVE/Jobs/Plugin.pm
> +++ b/PVE/Jobs/Plugin.pm
> @@ -80,6 +80,13 @@ sub encode_value {
>      return $plugin->encode_value($type, $key, $value);
>  }
>  
> +sub remove_vmid_from_job {
> +    my ($class, $type, $job, $vmid) = @_;

The signature here is different from the one in the VZDump plugin.

> +
> +    my $plugin = __PACKAGE__->lookup($type);
> +    $plugin->remove_vmid_from_job($job, $vmid);

The default implementation should either do nothing or, IMHO better, die
with something like "remove_vmid_from_job: implement in subclass". With
this implementation, calling remove_vmid_from_job on a plugin which
doesn't override it, will result in an infinite recursion (at least it
would if the signature would match ;)).

> +}
> +
>  sub write_config {
>      my ($class, $filename, $cfg) = @_;
>  
> diff --git a/PVE/Jobs/VZDump.pm b/PVE/Jobs/VZDump.pm
> index 44fe33dc..3aaa66b8 100644
> --- a/PVE/Jobs/VZDump.pm
> +++ b/PVE/Jobs/VZDump.pm
> @@ -64,6 +64,19 @@ sub encode_value {
>      return $value;
>  }
>  
> +sub remove_vmid_from_job {
> +    my ($class, $job, $vmid) = @_;
> +
> +    if (defined($job->{vmid})) {
> +	$job->{vmid} = join(',', grep { $_ ne $vmid } PVE::Tools::split_list($job->{vmid}));
> +	undef $job if $job->{vmid} eq '';
> +    } elsif (defined($job->{exclude})) {
> +	$job->{exclude} = join(',', grep { $_ ne $vmid } PVE::Tools::split_list($job->{exclude}));
> +	delete $job->{exclude} if $job->{exclude} eq '';
> +    }
> +    return $job;
> +}
> +
>  sub run {
>      my ($class, $conf) = @_;
>