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 EFA167BE77 for ; Wed, 3 Nov 2021 10:05:35 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id DE20610E5F for ; Wed, 3 Nov 2021 10:05:05 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 7ED1910E50 for ; Wed, 3 Nov 2021 10:05:04 +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 5426245A10 for ; Wed, 3 Nov 2021 10:05:04 +0100 (CET) To: pve-devel@lists.proxmox.com, Dominik Csapak References: <20211007082727.1385888-1-d.csapak@proxmox.com> <20211007082727.1385888-8-d.csapak@proxmox.com> From: Fabian Ebner Message-ID: <25c9e67f-1b9b-9049-b667-396df8a295be@proxmox.com> Date: Wed, 3 Nov 2021 10:05:02 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.14.0 MIME-Version: 1.0 In-Reply-To: <20211007082727.1385888-8-d.csapak@proxmox.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 1.512 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -2.549 Looks like a legit reply (A) 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. [backup.pm, backupinfo.pm] Subject: Re: [pve-devel] [PATCH manager 6/7] api/backup: handle new vzdump jobs 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: Wed, 03 Nov 2021 09:05:36 -0000 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 > --- > 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 }; > } > >