From 2cf7706e3d90cc23fe9861d2f4bceea023934db5 Mon Sep 17 00:00:00 2001 From: Dominik Csapak Date: Fri, 15 Jul 2022 14:34:35 +0200 Subject: [PATCH] Jobs: fix scheduling after updating job from a different node 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. to also update the state file on the other nodes, we introduce a new 'detect_changed_runtime_props' function that checks and saves relevant properties from the config to the statefile each round of the scheduler if they changed. this way, we can detect changes in those and update the last runtime too. 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. 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. Fixes: 0c8d7468 ("fix #4053: don't run vzdump jobs when they change from disabled->enabled") Signed-off-by: Dominik Csapak Reviewed-by: Fiona Ebner --- PVE/API2/Backup.pm | 22 ++++--------------- PVE/Jobs.pm | 53 +++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 54 insertions(+), 21 deletions(-) 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({ $data->{ids}->{$id} = $opts; - PVE::Jobs::create_job($id, 'vzdump'); + PVE::Jobs::create_job($id, 'vzdump', $opts); 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'; } - my $old_enabled = $job->{enabled} // 1; - my $deletable = { comment => 1, 'repeat-missed' => 1, @@ -469,21 +467,10 @@ __PACKAGE__->register_method({ delete $job->{$k}; } - my $need_run_time_update = 0; - if (defined($param->{schedule}) && $param->{schedule} ne $job->{schedule}) { - $need_run_time_update = 1; - } - foreach my $k (keys %$param) { $job->{$k} = $param->{$k}; } - my $new_enabled = $job->{enabled} // 1; - - if ($new_enabled && !$old_enabled) { - $need_run_time_update = 1; - } - $job->{all} = 1 if (defined($job->{exclude}) && !defined($job->{pool})); if (defined($param->{vmid})) { @@ -501,14 +488,13 @@ __PACKAGE__->register_method({ PVE::VZDump::verify_vzdump_parameters($job, 1); - 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 = { time => 0, }; +my $saved_config_props = [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) = @_; + + lock_job_state($jobid, $type, sub { + my $old_state = read_job_state($jobid, $type) // $default_state; + + my $updated = 0; + for my $prop (@$saved_config_props) { + my $old_prop = $old_state->{config}->{$prop} // ''; + my $new_prop = $cfg->{$prop} // ''; + next if "$old_prop" eq "$new_prop"; + + if (defined($cfg->{$prop})) { + $old_state->{config}->{$prop} = $cfg->{$prop}; + } else { + delete $old_state->{config}->{$prop}; + } + + $updated = 1; + } + + return if !$updated; + $old_state->{updated} = time(); + + my $path = $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) = @_; @@ -91,6 +125,7 @@ sub update_job_stopped { state => 'stopped', msg => $get_job_task_status->($state) // 'internal error', upid => $state->{upid}, + config => $state->{config}, }; if ($state->{updated}) { # save updated time stamp @@ -105,7 +140,7 @@ sub update_job_stopped { # must be called when the job is first created sub create_job { - my ($jobid, $type) = @_; + my ($jobid, $type, $cfg) = @_; lock_job_state($jobid, $type, sub { my $state = read_job_state($jobid, $type) // $default_state; @@ -115,6 +150,11 @@ sub create_job { } $state->{time} = time(); + for my $prop (@$saved_config_props) { + if (defined($cfg->{$prop})) { + $state->{config}->{$prop} = $cfg->{$prop}; + } + } my $path = $get_state_file->($jobid, $type); PVE::Tools::file_set_contents($path, encode_json($state)); @@ -144,6 +184,7 @@ sub starting_job { my $new_state = { state => 'starting', time => time(), + config => $state->{config}, }; my $path = $get_state_file->($jobid, $type); @@ -173,6 +214,7 @@ sub started_job { upid => $upid, }; } + $new_state->{config} = $state->{config}; my $path = $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 = $data->{ids}->{$id}; my $type = $job->{type}; - my $jobstate = read_job_state($id, $type); - create_job($id, $type) if !defined($jobstate); + + my $path = $get_state_file->($id, $type); + if (-e $path) { + detect_changed_runtime_props($id, $type, $job); + } else { + create_job($id, $type, $job); + } } PVE::Tools::dir_glob_foreach($state_dir, '(.*?)-(.*).json', sub {