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 <d.csapak@proxmox.com>
Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
This commit is contained in:
Dominik Csapak 2022-07-15 14:34:35 +02:00 committed by Fabian Grünbichler
parent 9018237e24
commit 2cf7706e3d
2 changed files with 54 additions and 21 deletions

View File

@ -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 {

View File

@ -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 {