Re: [pve-devel] [PATCH manager v2 1/3] fix #4026: add 'repeat-missed' option for jobs

[Fabian Ebner <f.ebner@proxmox.com>]

Am 02.06.22 um 09:42 schrieb Dominik Csapak:
> diff --git a/PVE/API2/Backup.pm b/PVE/API2/Backup.pm
> index 5d36789a..ced48977 100644
> --- a/PVE/API2/Backup.pm
> +++ b/PVE/API2/Backup.pm
> @@ -180,6 +180,13 @@ __PACKAGE__->register_method({
>  		description => "Enable or disable the job.",
>  		default => '1',
>  	    },
> +	    'repeat-missed' => {
> +		optional => 1,
> +		type => 'boolean',
> +		description => "If true, the job will be run as soon as possible if it was missed while".
> +		" the scheduler was not running.",

Style nit: Line too long and continuation should be indented once more.

> +		default => 0,
> +	    },
>  	    comment => {
>  		optional => 1,
>  		type => 'string',
> @@ -381,6 +388,13 @@ __PACKAGE__->register_method({
>  		description => "Enable or disable the job.",
>  		default => '1',
>  	    },
> +	    'repeat-missed' => {
> +		optional => 1,
> +		type => 'boolean',
> +		description => "If true, the job will be run as soon as possible if it was missed while".
> +		" the scheduler was not running.",

Same here.

> +		default => 0,
> +	    },
>  	    comment => {
>  		optional => 1,
>  		type => 'string',
> diff --git a/PVE/Jobs.pm b/PVE/Jobs.pm
> index da648630..ac44df00 100644
> --- a/PVE/Jobs.pm
> +++ b/PVE/Jobs.pm
> @@ -209,6 +209,7 @@ sub get_last_runtime {
>  }
>  
>  sub run_jobs {
> +    my ($first_run) = @_;

Style nit: could use a blank line

>      synchronize_job_states_with_config();
>  
>      my $jobs_cfg = cfs_read_file('jobs.cfg');
> @@ -228,6 +229,10 @@ sub run_jobs {
>  	    next;
>  	}
>  
> +	# update last_run_time on the first run when 'repeat-missed' is 0, so that a missed job will not

What is last_run_time ;)?
Style nit: line too long.

> +	# start immediately after boot
> +	updated_job_schedule($id, $type) if $first_run && !$cfg->{'repeat-missed'};

Can we rename the function? updated_job_schedule() sounds like something
that's only called when the job schedule was updated (and the next patch
introduces yet another code path calling it).

> +
>  	next if defined($cfg->{enabled}) && !$cfg->{enabled}; # only schedule actually enabled jobs
>  
>  	my $last_run = get_last_runtime($id, $type);
> diff --git a/PVE/Jobs/Plugin.pm b/PVE/Jobs/Plugin.pm
> index 6098360b..b8cbef1e 100644
> --- a/PVE/Jobs/Plugin.pm
> +++ b/PVE/Jobs/Plugin.pm
> @@ -39,6 +39,13 @@ my $defaultData = {
>  	    description => "Description for the Job.",
>  	    maxLength => 512,
>  	},
> +	'repeat-missed' => {
> +	    optional => 1,
> +	    type => 'boolean',
> +	    description => "If true, the job will be run as soon as possible if it was missed while".

Style nit: also one char too long.

> +		" the scheduler was not running.",
> +	    default => 0,

Do we plan to change the behavior for replication jobs in the future
too? Otherwise, the documented default here will not apply to those.

> +	},
>      },
>  };
>