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 1CDF17C5A5 for ; Fri, 15 Jul 2022 14:35:08 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 0B7E328C40 for ; Fri, 15 Jul 2022 14:34:38 +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, 15 Jul 2022 14:34:36 +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 BD17A42A28 for ; Fri, 15 Jul 2022 14:34:36 +0200 (CEST) From: Dominik Csapak To: pve-devel@lists.proxmox.com Date: Fri, 15 Jul 2022 14:34:35 +0200 Message-Id: <20220715123435.3720554-1-d.csapak@proxmox.com> X-Mailer: git-send-email 2.30.2 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.098 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] [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, 15 Jul 2022 12:35:08 -0000 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: 530b0a71 ("fix #4053: don't run vzdump jobs when they change from disabled->enabled") 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 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 { -- 2.30.2