[pve-devel] applied: [PATCH manager v2] Jobs: fix scheduling after updating job from a different node

[Fabian Grünbichler]

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.csapak at 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 at lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
>