Re: [pve-devel] [PATCH manager 6/7] api/backup: handle new vzdump jobs

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

Am 07.10.21 um 10:27 schrieb Dominik Csapak:
> in addition to listing the vzdump.cron jobs, also list from the
> jobs.cfg file.
> 
> updates/creations go into the new jobs.cfg only now
> and on update, starttime+dow get converted to a schedule
> this transformation is straight forward, since 'dow'
> is already in a compatible format (e.g. 'mon,tue') and we simply
> append the starttime (if any)
> 
> id on creation is optional for now (for api compat), but will
> be autogenerated (uuid). on update, we simply take the id from before
> (the ids of the other entries in vzdump.cron will change but they would
> anyway)
> 
> as long as we have the vzdump.cron file, we must lock both
> vzdump.cron and jobs.cfg, since we often update both
> 
> we also change the backupinfo api call to read the jobs.cfg too
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>   PVE/API2/Backup.pm             | 243 +++++++++++++++++++++++++++------
>   PVE/API2/Cluster/BackupInfo.pm |  10 ++
>   2 files changed, 208 insertions(+), 45 deletions(-)
> 
> diff --git a/PVE/API2/Backup.pm b/PVE/API2/Backup.pm
> index 9dc3b48e..56ed4be8 100644
> --- a/PVE/API2/Backup.pm
> +++ b/PVE/API2/Backup.pm
> @@ -3,6 +3,7 @@ package PVE::API2::Backup;
>   use strict;
>   use warnings;
>   use Digest::SHA;
> +use UUID;
>   
>   use PVE::SafeSyslog;
>   use PVE::Tools qw(extract_param);
> @@ -14,6 +15,7 @@ use PVE::Storage;
>   use PVE::Exception qw(raise_param_exc);
>   use PVE::VZDump;
>   use PVE::VZDump::Common;
> +use PVE::Jobs; # for VZDump Jobs
>   
>   use base qw(PVE::RESTHandler);
>   
> @@ -45,6 +47,19 @@ my $assert_param_permission = sub {
>       }
>   };
>   
> +my $convert_to_schedule = sub {
> +    my ($job) = @_;
> +
> +    my $starttime = $job->{starttime};
> +    my $dow = $job->{dow};
> +
> +    if (!$dow || $dow eq ALL_DAYS) {
> +	return "$starttime";
> +    }
> +
> +    return "$dow $starttime";
> +};
> +
>   __PACKAGE__->register_method({
>       name => 'index',
>       path => '',
> @@ -74,8 +89,20 @@ __PACKAGE__->register_method({
>   	my $user = $rpcenv->get_user();
>   
>   	my $data = cfs_read_file('vzdump.cron');
> +	my $jobs_data = cfs_read_file('jobs.cfg');
> +	my $order = $jobs_data->{order};
> +	my $jobs = $jobs_data->{ids};
>   
>   	my $res = $data->{jobs} || [];
> +	foreach my $job (@$res) {
> +	    $job->{schedule} = $convert_to_schedule->($job);
> +	}
> +
> +	foreach my $jobid (sort { $order->{$a} <=> $order->{$b} } keys %$jobs) {
> +	    my $job = $jobs->{$jobid};
> +	    next if $job->{type} ne 'vzdump';
> +	    push @$res, $job;
> +	}
>   
>   	return $res;
>       }});
> @@ -93,11 +120,24 @@ __PACKAGE__->register_method({
>       parameters => {
>       	additionalProperties => 0,
>   	properties => PVE::VZDump::Common::json_config_properties({
> +	    id => {
> +		type => 'string',
> +		description => "Job ID (will be autogenerated).",
> +		format => 'pve-configid',
> +		optional => 1, # FIXME: make required on 8.0
> +	    },
> +	    schedule => {
> +		description => "Backup schedule. The format is a subset of `systemd` calendar events.",
> +		type => 'string', format => 'pve-calendar-event',
> +		maxLength => 128,
> +		optional => 1,
> +	    },
>   	    starttime => {
>   		type => 'string',
>   		description => "Job Start time.",
>   		pattern => '\d{1,2}:\d{1,2}',
>   		typetext => 'HH:MM',
> +		optional => 1,
>   	    },
>   	    dow => {
>   		type => 'string', format => 'pve-day-of-week-list',
> @@ -127,19 +167,40 @@ __PACKAGE__->register_method({
>   	    $rpcenv->check($user, "/pool/$pool", ['VM.Backup']);
>   	}
>   
> +	if (defined($param->{schedule})) {
> +	    if (defined($param->{starttime})) {
> +		raise_param_exc({ starttime => "'starttime' and 'schedule' cannot both be set" });
> +	    }

Should schedule also clash with dow? Or use requires => 'starttime' like 
for the update call?

> +	} elsif (!defined($param->{starttime})) {
> +	    raise_param_exc({ schedule => "neither 'starttime' nor 'schedule' were set" });
> +	} else {
> +	    $param->{schedule} = $convert_to_schedule->($param);
> +	}
> +
> +	delete $param->{starttime};
> +	delete $param->{dow};
> 

The schedule conversion/checks could be factored out to avoid 
duplication for the update call.

> -	my $create_job = sub {
> -	    my $data = cfs_read_file('vzdump.cron');
> +	$param->{enabled} = 1 if !defined($param->{enabled});
> +
> +	# autogenerate id for api compatibility FIXME remove with 8.0
> +	my $id = extract_param($param, 'id') // uuid();
> +
> +	cfs_lock_file('jobs.cfg', undef, sub {
> +	    my $data = cfs_read_file('jobs.cfg');
> +
> +	    die "Job '$id' already exists\n"
> +		if $data->{ids}->{$id};
>   
> -	    $param->{dow} = ALL_DAYS if !defined($param->{dow});
> -	    $param->{enabled} = 1 if !defined($param->{enabled});
>   	    PVE::VZDump::verify_vzdump_parameters($param, 1);
> +	    my $plugin = PVE::Jobs::Plugin->lookup('vzdump');
> +	    my $opts = $plugin->check_config($id, $param, 1, 1);
>   
> -	    push @{$data->{jobs}}, $param;
> +	    $data->{ids}->{$id} = $opts;
>   
> -	    cfs_write_file('vzdump.cron', $data);
> -	};
> -	cfs_lock_file('vzdump.cron', undef, $create_job);
> +	    PVE::Jobs::create_job($id, 'vzdump');
> +
> +	    cfs_write_file('jobs.cfg', $data);
> +	});
>   	die "$@" if ($@);
>   
>   	return undef;
> @@ -173,9 +234,16 @@ __PACKAGE__->register_method({
>   	my $jobs = $data->{jobs} || [];
>   
>   	foreach my $job (@$jobs) {
> -	    return $job if $job->{id} eq $param->{id};
> +	    if ($job->{id} eq $param->{id}) {
> +		$job->{schedule} = $convert_to_schedule->($job);
> +		return $job;
> +	    }
>   	}
>   
> +	my $jobs_data = cfs_read_file('jobs.cfg');
> +	my $job = $jobs_data->{ids}->{$param->{id}};
> +	return $job if $job && $job->{type} eq 'vzdump';
> +
>   	raise_param_exc({ id => "No such job '$param->{id}'" });
>   
>       }});
> @@ -202,6 +270,8 @@ __PACKAGE__->register_method({
>   	my $rpcenv = PVE::RPCEnvironment::get();
>   	my $user = $rpcenv->get_user();
>   
> +	my $id = $param->{id};
> +
>   	my $delete_job = sub {
>   	    my $data = cfs_read_file('vzdump.cron');
>   
> @@ -210,18 +280,32 @@ __PACKAGE__->register_method({
>   
>   	    my $found;
>   	    foreach my $job (@$jobs) {
> -		if ($job->{id} eq $param->{id}) {
> +		if ($job->{id} eq $id) {
>   		    $found = 1;
>   		} else {
>   		    push @$newjobs, $job;
>   		}
>   	    }
>   
> -	    raise_param_exc({ id => "No such job '$param->{id}'" }) if !$found;
> +	    if (!$found) {
> +		cfs_lock_file('jobs.cfg', undef, sub {
> +		    my $jobs_data = cfs_read_file('jobs.cfg');
>   
> -	    $data->{jobs} = $newjobs;
> +		    if (!defined($jobs_data->{ids}->{$id})) {
> +			raise_param_exc({ id => "No such job '$id'" });
> +		    }
> +		    delete $jobs_data->{ids}->{$id};
> +
> +		    PVE::Jobs::remove_job($id, 'vzudmp');
> +
> +		    cfs_write_file('jobs.cfg', $jobs_data);
> +		});
> +		die "$@" if $@;
> +	    } else {
> +		$data->{jobs} = $newjobs;
>   
> -	    cfs_write_file('vzdump.cron', $data);
> +		cfs_write_file('vzdump.cron', $data);
> +	    }
>   	};
>   	cfs_lock_file('vzdump.cron', undef, $delete_job);
>   	die "$@" if ($@);
> @@ -243,15 +327,23 @@ __PACKAGE__->register_method({
>       	additionalProperties => 0,
>   	properties => PVE::VZDump::Common::json_config_properties({
>   	    id => $vzdump_job_id_prop,
> +	    schedule => {
> +		description => "Backup schedule. The format is a subset of `systemd` calendar events.",
> +		type => 'string', format => 'pve-calendar-event',
> +		maxLength => 128,
> +		optional => 1,
> +	    },
>   	    starttime => {
>   		type => 'string',
>   		description => "Job Start time.",
>   		pattern => '\d{1,2}:\d{1,2}',
>   		typetext => 'HH:MM',
> +		optional => 1,
>   	    },
>   	    dow => {
>   		type => 'string', format => 'pve-day-of-week-list',
>   		optional => 1,
> +		requires => 'starttime',
>   		description => "Day of week selection.",
>   	    },
>   	    delete => {
> @@ -281,57 +373,111 @@ __PACKAGE__->register_method({
>   	    $rpcenv->check($user, "/pool/$pool", ['VM.Backup']);
>   	}
>   
> +	if (defined($param->{schedule})) {
> +	    if (defined($param->{starttime})) {
> +		raise_param_exc({ starttime => "'starttime' and 'schedule' cannot both be set" });
> +	    }
> +	} elsif (!defined($param->{starttime})) {
> +	    raise_param_exc({ schedule => "neither 'starttime' nor 'schedule' were set" });
> +	} else {
> +	    $param->{schedule} = $convert_to_schedule->($param);
> +	}
> +
> +	delete $param->{starttime};
> +	delete $param->{dow};
> +
> +	my $id = extract_param($param, 'id');
> +	my $delete = extract_param($param, 'delete');
> +	if ($delete) {
> +	    $delete = [PVE::Tools::split_list($delete)];
> +	}
> +
>   	my $update_job = sub {
>   	    my $data = cfs_read_file('vzdump.cron');
> +	    my $jobs_data = cfs_read_file('jobs.cfg');
>   
>   	    my $jobs = $data->{jobs} || [];
>   
>   	    die "no options specified\n" if !scalar(keys %$param);
>   
>   	    PVE::VZDump::verify_vzdump_parameters($param);
> +	    my $plugin = PVE::Jobs::Plugin->lookup('vzdump');
> +	    my $opts = $plugin->check_config($id, $param, 0, 1);
> +
> +	    # try to find it in old vzdump.cron and convert it to a job
> +	    my $idx = 0;
> +	    my $found = 0;
> +	    foreach my $j (@$jobs) {
> +		if ($j->{id} eq $id) {
> +		    $found = 1;
> +		    last;
> +		}
> +		$idx++;
> +	    }

Haven't tested it, but something like:
     my ($idx) = grep { $jobs->[$_]->{id} eq $id } (0 .. scalar(@$jobs) 
- 1);
would be a bit shorter, with using defined($idx) instead of $found. Just 
an idea though.

>   
> -	    my @delete = PVE::Tools::split_list(extract_param($param, 'delete'));
> +	    my $job;
> +	    if ($found) {
> +		$job = splice @$jobs, $idx, 1;
> +		$job->{schedule} = $convert_to_schedule->($job);
> +		delete $job->{starttime};
> +		delete $job->{dow};
> +		delete $job->{id};
> +		$job->{type} = 'vzdump';
> +		$jobs_data->{ids}->{$id} = $job;
> +	    } else {
> +		$job = $jobs_data->{ids}->{$id};
> +		die "no such vzdump job\n" if !$job || $job->{type} ne 'vzdump';
> +	    }
>   
> -	    foreach my $job (@$jobs) {
> -		if ($job->{id} eq $param->{id}) {
> +	    foreach my $k (@$delete) {
> +		if (!PVE::VZDump::option_exists($k)) {
> +		    raise_param_exc({ delete => "unknown option '$k'" });
> +		}
>   
> -		    foreach my $k (@delete) {
> -			if (!PVE::VZDump::option_exists($k)) {
> -			    raise_param_exc({ delete => "unknown option '$k'" });
> -			}
> +		delete $job->{$k};
> +	    }
>   
> -			delete $job->{$k};
> -		    }
> +	    my $schedule_updated = 0;
> +	    if ($param->{schedule} ne $job->{schedule}) {
> +		$schedule_updated = 1;
> +	    }
>   
> -		    foreach my $k (keys %$param) {
> -			$job->{$k} = $param->{$k};
> -		    }
> +	    foreach my $k (keys %$param) {
> +		$job->{$k} = $param->{$k};
> +	    }
>   
> -		    $job->{all} = 1 if (defined($job->{exclude}) && !defined($job->{pool}));
> -
> -		    if (defined($param->{vmid})) {
> -			delete $job->{all};
> -			delete $job->{exclude};
> -			delete $job->{pool};
> -		    } elsif ($param->{all}) {
> -			delete $job->{vmid};
> -			delete $job->{pool};
> -		    } elsif ($job->{pool}) {
> -			delete $job->{vmid};
> -			delete $job->{all};
> -			delete $job->{exclude};
> -		    }
> +	    $job->{all} = 1 if (defined($job->{exclude}) && !defined($job->{pool}));
> +
> +	    if (defined($param->{vmid})) {
> +		delete $job->{all};
> +		delete $job->{exclude};
> +		delete $job->{pool};
> +	    } elsif ($param->{all}) {
> +		delete $job->{vmid};
> +		delete $job->{pool};
> +	    } elsif ($job->{pool}) {
> +		delete $job->{vmid};
> +		delete $job->{all};
> +		delete $job->{exclude};
> +	    }
>   
> -		    PVE::VZDump::verify_vzdump_parameters($job, 1);
> +	    PVE::VZDump::verify_vzdump_parameters($job, 1);
>   
> -		    cfs_write_file('vzdump.cron', $data);
> +	    if ($schedule_updated) {
> +		PVE::Jobs::updated_job_schedule($id, 'vzdump');
> +	    }
>   
> -		    return undef;
> -		}
> +	    if ($found) {
> +		cfs_write_file('vzdump.cron', $data);
>   	    }
> -	    raise_param_exc({ id => "No such job '$param->{id}'" });
> +	    cfs_write_file('jobs.cfg', $jobs_data); # FIXME LOCKING

Left-over reminder?

> +
> +	    return undef;

Nit: return is favorable over return undef (matters in list context).

>   	};
> -	cfs_lock_file('vzdump.cron', undef, $update_job);
> +	cfs_lock_file('vzdump.cron', undef, sub {
> +	    cfs_lock_file('jobs.cfg', undef, $update_job);
> +	    die "$@" if ($@);
> +	});
>   	die "$@" if ($@);
>       }});
>   
> @@ -422,6 +568,13 @@ __PACKAGE__->register_method({
>   	       last;
>   	    }
>   	}
> +	if (!$job) {
> +	    my $jobs_data = cfs_read_file('jobs.cfg');
> +	    my $j = $jobs_data->{ids}->{$param->{id}};
> +	    if ($j && $j->{type} eq 'vzdump') {
> +		$job = $j;
> +	    }
> +	}
>   	raise_param_exc({ id => "No such job '$param->{id}'" }) if !$job;
>   
>   	my $vmlist = PVE::Cluster::get_vmlist();
> diff --git a/PVE/API2/Cluster/BackupInfo.pm b/PVE/API2/Cluster/BackupInfo.pm
> index 236e171d..101e45a7 100644
> --- a/PVE/API2/Cluster/BackupInfo.pm
> +++ b/PVE/API2/Cluster/BackupInfo.pm
> @@ -28,6 +28,16 @@ sub get_included_vmids {
>   	push @all_vmids, ( map { @{$_} } values %{$job_included_guests} );
>       }
>   
> +    my $vzjobs = cfs_read_file('jobs.cfg');
> +
> +    for my $id (keys %{$vzjobs->{ids}}) {
> +	my $job = $vzjobs->{ids}->{$id};

Nit: loop could iterate over values rather than keys.

> +	next if $job->{type} ne 'vzdump';
> +
> +	my $job_included_guests = PVE::VZDump::get_included_guests($job);
> +	push @all_vmids, ( map { @{$_} } values %{$job_included_guests} );
> +    }
> +
>       return { map { $_ => 1 } @all_vmids };
>   }
>   
>