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

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

[Dominik Csapak]

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

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

[Fabian Ebner]

Am 15.07.22 um 14:34 schrieb Dominik Csapak:
> 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@proxmox.com>

Looks good to me:
Reviewed-by: Fabian Ebner <f.ebner@proxmox.com>

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