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.
>
> 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 <d.csa...@proxmox.com>
> ---
> 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
>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
