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 63E0B8AAB7 for ; Fri, 5 Aug 2022 13:10:10 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 523CD2EC82 for ; Fri, 5 Aug 2022 13:09:40 +0200 (CEST) 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 for ; Fri, 5 Aug 2022 13:09:39 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id D163242EDA for ; Fri, 5 Aug 2022 13:09:32 +0200 (CEST) Date: Fri, 05 Aug 2022 13:09:25 +0200 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Proxmox VE development discussion References: <20220715123435.3720554-1-d.csapak@proxmox.com> In-Reply-To: <20220715123435.3720554-1-d.csapak@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.15.0 (https://github.com/astroidmail/astroid) Message-Id: <1659697710.ye0pfehrbi.astroid@nora.none> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL 0.163 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 SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - Subject: [pve-devel] applied: [PATCH manager v2] Jobs: fix scheduling after updating job from a different node 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: Fri, 05 Aug 2022 11:10:10 -0000 with R-B, thanks! On July 15, 2022 2:34 pm, Dominik Csapak wrote: > since the jobs are configured clusterwide in pmxcfs, a user can use any > node to update the config of them. for some configs (schedule/enabled) > we need to update the last runtime in the state file, but this > is sadly only node-local. >=20 > to also update the state file on the other nodes, we introduce > a new 'detect_changed_runtime_props' function that checks and saves relev= ant > properties from the config to the statefile each round of the scheduler i= f they > changed. >=20 > this way, we can detect changes in those and update the last runtime too. >=20 > the only situation where we don't detect a config change is when the > user changes back to the previous configuration in between iterations. > This can be ignored though, since it would not be scheduled then > anyway. >=20 > in 'synchronize_job_states_with_config' we switch from reading the > jobstate unconditionally to check the existance of the statefile > (which is the only condition that can return undef anyway) > so that we don't read the file multiple times each round. >=20 > Fixes: 530b0a71 ("fix #4053: don't run vzdump jobs when they change from > disabled->enabled") >=20 > Signed-off-by: Dominik Csapak > --- > changes from v1: > * renamed update_job_props to detect_changed_runtime_props > * moved props into a sub-prop 'config', this way it's only a single line > to preserve them > * preserve them also in _started/_starting > * improved commit message > * also give config to create_job when we're creating a new one via api > * call 'detect_changed_runtime_props' unconditionally when updating > a job instead of keeping track if we need to update the last runtime >=20 > PVE/API2/Backup.pm | 22 ++++--------------- > PVE/Jobs.pm | 53 +++++++++++++++++++++++++++++++++++++++++++--- > 2 files changed, 54 insertions(+), 21 deletions(-) >=20 > diff --git a/PVE/API2/Backup.pm b/PVE/API2/Backup.pm > index 0041d4fb..233a6ebf 100644 > --- a/PVE/API2/Backup.pm > +++ b/PVE/API2/Backup.pm > @@ -228,7 +228,7 @@ __PACKAGE__->register_method({ > =20 > $data->{ids}->{$id} =3D $opts; > =20 > - PVE::Jobs::create_job($id, 'vzdump'); > + PVE::Jobs::create_job($id, 'vzdump', $opts); > =20 > cfs_write_file('jobs.cfg', $data); > }); > @@ -454,8 +454,6 @@ __PACKAGE__->register_method({ > die "no such vzdump job\n" if !$job || $job->{type} ne 'vzdump'; > } > =20 > - my $old_enabled =3D $job->{enabled} // 1; > - > my $deletable =3D { > comment =3D> 1, > 'repeat-missed' =3D> 1, > @@ -469,21 +467,10 @@ __PACKAGE__->register_method({ > delete $job->{$k}; > } > =20 > - my $need_run_time_update =3D 0; > - if (defined($param->{schedule}) && $param->{schedule} ne $job->{sch= edule}) { > - $need_run_time_update =3D 1; > - } > - > foreach my $k (keys %$param) { > $job->{$k} =3D $param->{$k}; > } > =20 > - my $new_enabled =3D $job->{enabled} // 1; > - > - if ($new_enabled && !$old_enabled) { > - $need_run_time_update =3D 1; > - } > - > $job->{all} =3D 1 if (defined($job->{exclude}) && !defined($job->{p= ool})); > =20 > if (defined($param->{vmid})) { > @@ -501,14 +488,13 @@ __PACKAGE__->register_method({ > =20 > PVE::VZDump::verify_vzdump_parameters($job, 1); > =20 > - if ($need_run_time_update) { > - PVE::Jobs::update_last_runtime($id, 'vzdump'); > - } > - > if (defined($idx)) { > cfs_write_file('vzdump.cron', $data); > } > cfs_write_file('jobs.cfg', $jobs_data); > + > + PVE::Jobs::detect_changed_runtime_props($id, 'vzdump', $job); > + > return; > }; > cfs_lock_file('vzdump.cron', undef, sub { > diff --git a/PVE/Jobs.pm b/PVE/Jobs.pm > index 1091bc22..ebaacfc2 100644 > --- a/PVE/Jobs.pm > +++ b/PVE/Jobs.pm > @@ -25,6 +25,40 @@ my $default_state =3D { > time =3D> 0, > }; > =20 > +my $saved_config_props =3D [qw(enabled schedule)]; > + > +# saves some properties of the jobcfg into the jobstate so we can track > +# them on different nodes (where the update was not done) > +# and update the last runtime when they change > +sub detect_changed_runtime_props { > + my ($jobid, $type, $cfg) =3D @_; > + > + lock_job_state($jobid, $type, sub { > + my $old_state =3D read_job_state($jobid, $type) // $default_state; > + > + my $updated =3D 0; > + for my $prop (@$saved_config_props) { > + my $old_prop =3D $old_state->{config}->{$prop} // ''; > + my $new_prop =3D $cfg->{$prop} // ''; > + next if "$old_prop" eq "$new_prop"; > + > + if (defined($cfg->{$prop})) { > + $old_state->{config}->{$prop} =3D $cfg->{$prop}; > + } else { > + delete $old_state->{config}->{$prop}; > + } > + > + $updated =3D 1; > + } > + > + return if !$updated; > + $old_state->{updated} =3D time(); > + > + my $path =3D $get_state_file->($jobid, $type); > + PVE::Tools::file_set_contents($path, encode_json($old_state)); > + }); > +} > + > # lockless, since we use file_get_contents, which is atomic > sub read_job_state { > my ($jobid, $type) =3D @_; > @@ -91,6 +125,7 @@ sub update_job_stopped { > state =3D> 'stopped', > msg =3D> $get_job_task_status->($state) // 'internal error', > upid =3D> $state->{upid}, > + config =3D> $state->{config}, > }; > =20 > if ($state->{updated}) { # save updated time stamp > @@ -105,7 +140,7 @@ sub update_job_stopped { > =20 > # must be called when the job is first created > sub create_job { > - my ($jobid, $type) =3D @_; > + my ($jobid, $type, $cfg) =3D @_; > =20 > lock_job_state($jobid, $type, sub { > my $state =3D read_job_state($jobid, $type) // $default_state; > @@ -115,6 +150,11 @@ sub create_job { > } > =20 > $state->{time} =3D time(); > + for my $prop (@$saved_config_props) { > + if (defined($cfg->{$prop})) { > + $state->{config}->{$prop} =3D $cfg->{$prop}; > + } > + } > =20 > my $path =3D $get_state_file->($jobid, $type); > PVE::Tools::file_set_contents($path, encode_json($state)); > @@ -144,6 +184,7 @@ sub starting_job { > my $new_state =3D { > state =3D> 'starting', > time =3D> time(), > + config =3D> $state->{config}, > }; > =20 > my $path =3D $get_state_file->($jobid, $type); > @@ -173,6 +214,7 @@ sub started_job { > upid =3D> $upid, > }; > } > + $new_state->{config} =3D $state->{config}; > =20 > my $path =3D $get_state_file->($jobid, $type); > PVE::Tools::file_set_contents($path, encode_json($new_state)); > @@ -265,8 +307,13 @@ sub synchronize_job_states_with_config { > for my $id (keys $data->{ids}->%*) { > my $job =3D $data->{ids}->{$id}; > my $type =3D $job->{type}; > - my $jobstate =3D read_job_state($id, $type); > - create_job($id, $type) if !defined($jobstate); > + > + my $path =3D $get_state_file->($id, $type); > + if (-e $path) { > + detect_changed_runtime_props($id, $type, $job); > + } else { > + create_job($id, $type, $job); > + } > } > =20 > PVE::Tools::dir_glob_foreach($state_dir, '(.*?)-(.*).json', sub { > --=20 > 2.30.2 >=20 >=20 >=20 > _______________________________________________ > pve-devel mailing list > pve-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel >=20 >=20 >=20