[pve-devel] [PATCH guest-common] Send email when a replication job fails.

[Wolfgang Bumiller]

This should be tagged [PATCH manager].

Some minor cleanup requests inline:

On Fri, Oct 13, 2017 at 10:08:09AM +0200, Wolfgang Link wrote:
> A email notification will be send for each job when the job fails.
> This message will only send when an error occurs and the fail count is on 1.
> ---
>  PVE/API2/Replication.pm  | 18 ++++++++++++++++--
>  PVE/CLI/pvesr.pm         | 11 ++++++++++-
>  bin/init.d/pvesr.service |  2 +-
>  3 files changed, 27 insertions(+), 4 deletions(-)
> 
> diff --git a/PVE/API2/Replication.pm b/PVE/API2/Replication.pm
> index f396615d..8e98c17c 100644
> --- a/PVE/API2/Replication.pm
> +++ b/PVE/API2/Replication.pm
> @@ -72,7 +72,7 @@ sub run_single_job {
>  
>  # passing $now and $verbose is useful for regression testing
>  sub run_jobs {
> -    my ($now, $logfunc, $verbose) = @_;
> +    my ($now, $logfunc, $verbose, $mail) = @_;
>  
>      my $iteration = $now // time();
>  
> @@ -83,7 +83,21 @@ sub run_jobs {

The indentation in this function is wrong and your changes now all ended
up being off by 1 space. Could you please prepend an indentation cleanup
patch to this one and add the changes as a 2nd patch on top of that with
the indentation corrected?

>  
>         while (my $jobcfg = PVE::ReplicationState::get_next_job($iteration, $start_time)) {
>             my $guest_class = $lookup_guest_class->($jobcfg->{vmtype});
> -           PVE::Replication::run_replication($guest_class, $jobcfg, $iteration, $start_time, $logfunc, 1, $verbose);
> +
> +	   eval {
> +	       PVE::Replication::run_replication($guest_class, $jobcfg, $iteration, $start_time, $logfunc, $verbose);
> +	   };
> +	   if (my $err = $@) {
> +	       warn "$jobcfg->{id}: got unexpected replication job error - $err";
> +	       my $state = PVE::ReplicationState::read_state();
> +	       my $jobstate = PVE::ReplicationState::extract_job_state($state, $jobcfg);
> +	       eval {
> +		   PVE::Tools::sendmail('root', "Replication Job: $jobcfg->{id} failed", $err)
> +		       if $jobstate->{fail_count}  == 1 && $mail;

There's an extra space before the '=='.

> +	       };
> +	       warn "Can't send email: - $@" if $@;

Please remove the leftover '-' in front of the $@.

> +	   };
> +
>             $start_time = $now // time();
>         }
>      };
> diff --git a/PVE/CLI/pvesr.pm b/PVE/CLI/pvesr.pm
> index 7da94404..70aa3642 100644
> --- a/PVE/CLI/pvesr.pm
> +++ b/PVE/CLI/pvesr.pm
> @@ -221,12 +221,21 @@ __PACKAGE__->register_method ({
>  		default => 0,
>  		optional => 1,
>  	    },
> +	    mail => {
> +		description => "With email notification in case of a failure",

s/With/Send an/
and add a period at the end ;-)
The documentation looks nicer with proper sentences.

> +		type => 'boolean',
> +		default => 0,
> +		optional => 1,
> +	    },
>  	},
>      },
>      returns => { type => 'null' },
>      code => sub {
>  	my ($param) = @_;
>  
> +	die "Mail and id are not possible in conjunction!\n"

I find "... are mutually exclusive" to be a nicer pattern for this kind
of error message.

> +	    if $param->{id} && $param->{mail};
> +
>  	my $logfunc;
>  
>  	if ($param->{verbose}) {
> @@ -242,7 +251,7 @@ __PACKAGE__->register_method ({
>  
>  	} else {
>  
> -	    PVE::API2::Replication::run_jobs(undef, $logfunc);
> +	    PVE::API2::Replication::run_jobs(undef, $logfunc, 0, $param->{mail});
>  	}
>  
>  	return undef;
> diff --git a/bin/init.d/pvesr.service b/bin/init.d/pvesr.service
> index 5706d426..e0c082af 100644
> --- a/bin/init.d/pvesr.service
> +++ b/bin/init.d/pvesr.service
> @@ -4,4 +4,4 @@ ConditionPathExists=/usr/bin/pvesr
>  
>  [Service]
>  Type=oneshot
> -ExecStart=/usr/bin/pvesr run
> +ExecStart=/usr/bin/pvesr run --mail 1
> -- 
> 2.11.0