[pve-devel] [PATCH manager] Fix #352: Limit the length of backup logs for mails

[Thomas Lamprecht]

Am 5/17/19 um 12:16 PM schrieb Dominik Csapak:
> first of all, great that you include tests :) +1

for me also +1 for that one! 

> 
> second, i am not really happy with the approach, since it
> is quite a bit of code, for not that much gain
> 
> On 5/14/19 12:20 PM, Dominic Jäger wrote:
>> When creating a backup the log for a single job can be to big to be
>> transferred by mail. To ensure that mails get delivered nonetheless
>> the amount of lines and their length are limited.
>>
>> Signed-off-by: Dominic Jäger <d.jaeger at proxmox.com>
>> ---
>> The alternative way would have been to build the whole mail, check the
>> size, then go into the string back again to identify parts of which the
>> size explodes.
>> Limiting while building up the string seemed more straightforward and
>> efficient to me. The loss of information is none, as everything is
>> in the task history anyway.
> 
> one approach i would favor is to generate the mail, check the size,
> and only send the initial overview table with a notice that the
> mail was too long and where to find the log
> 
> with your approach, the mail sending is much more complicated,
> for very little gain (a user must probably look for the full
> task log anyway if he wants to get the information) and
> does not even prevent the problem if someone has many vms that
> get backed up (or am i missing that bit?)
> 
> in any case, one comment about the code itself is inline

We control the output format so a simple:

next if $line =~ /^status: \d+/;

would omit all status line, on long backups I really do not see their
use (and if one wants to look more close, the task log _and_ the backup
log (on target storage) includes them. Also the total time + average time
is much more intresteting anyway, for an overview.

Additionally a simple check like you proposed (for really big backup
tasks), where we switch to sending a simple overview table (as HTML
already does) and be good, that should be easier to do and maintain.

> 
>>
>>   PVE/VZDump.pm     | 106 +++++++++++++++++++++++++++++++++++++++++++++++-------
>>   test/Makefile     |   1 +
>>   test/mail_test.pl |  70 ++++++++++++++++++++++++++++++++++++
>>   3 files changed, 164 insertions(+), 13 deletions(-)
>>   create mode 100755 test/mail_test.pl
>>
>> diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
>> index 6508c833..62097ad7 100644
>> --- a/PVE/VZDump.pm
>> +++ b/PVE/VZDump.pm
>> @@ -341,6 +341,96 @@ sub read_vzdump_defaults {
>>       return $res;
>>   }
>>   +use constant MAX_LINE_LENGTH => 200;
> 
> why hardcoded here?
> 
> the MAX_LOG_LINES and MAX_LINE_LENGTH are global constants,
> but the $LINES_AT_END (below) is defined in the function
> 
> i would rather have them as parameter, so that
> someone can tune this (if necessary)

it's reused in the test, that's why, I'd guess - but making it dynamic
can work for the tests too..