[libvirt] [PATCHv2] Do not keep empty log files for deleted domains

[Jim Fehlig]

On 7/8/19 7:51 PM, Jan Zerebecki wrote:
> 
> 
> On 08/07/2019 23.28, Jim Fehlig wrote:
>> On 7/8/19 12:28 PM, Jan Zerebecki wrote:
>>> With logrotates copytruncate when e.g. domain1 doesn't exist anymore
>>> /var/log/libvirt/qemu/domain1.log will still exist after rotation even
>>> though it will never be written to. When new domain names keep getting
>>> used this leads to a lot of empty logfiles. This may lead to slowdown or
>>> lack of free disk space / inodes.
>>>
>>> Fix this by replacing copytruncate with the apropriate postrotate
>>> command to reopen log files. Thus after the apropriate time log files
>>> for deleted domains will be gone. This also has the advantage that the
>>> chance for loss of a few lines during copytruncate is gone.
>>>
>>> This only fixes the issue for qemu domains, others still have the same
>>> problem unfixed.
>>>
>>> Signed-off-by: Jan Zerebecki <jan.suse at zerebecki.de>
>>> ---
>>> v2: drop changes to other logrotate confis
>>>
>>>    src/remote/libvirtd.qemu.logrotate.in | 4 +++-
>>>    1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/remote/libvirtd.qemu.logrotate.in b/src/remote/libvirtd.qemu.logrotate.in
>>> index cdb399ef23..95407cec1a 100644
>>> --- a/src/remote/libvirtd.qemu.logrotate.in
>>> +++ b/src/remote/libvirtd.qemu.logrotate.in
>>> @@ -4,5 +4,7 @@
>>>            rotate 4
>>>            compress
>>>            delaycompress
>>> -        copytruncate
>>> +        postrotate
>>> +                /usr/bin/killall -USR1 virtlogd
>>> +        endscript
>>>    }
>>
>> In V1 Daniel said "logrotate should not be in effect when virtlogd is running".
> As suggested the logrotate config does those things which virtlogd
> doesn't, so they fit well together.
> 
> On 03/07/2019 10.42, Daniel P. Berrangé wrote:
>> We should
>> ensure that logrotate rollover size is *larger* than the rollover size
>> configured for virtlogd.
> 
> This is the case.
> 
> That someone has virtlogd running does not necessarily mean they don't
> want logrotate to run.

If that's the case, you don't want my patch :-). It assumes logrotate service is 
not wanted if virtlogd is running.

> virtlogd does not support rotating out logs for
> domains that don't exist anymore, which is the problem this patch
> intends to fix with logrotate.

Oh, I didn't realize that was the intended behavior.

> Any suggestion for an alternate
> implementation should at least consider how to fix the problem
> mentioned. It also does not support rotating by time which some might
> want to use to minimize their data retention.

Shouldn't some of these log retention policies be deferred to the admin? They 
are free to adjust their logrotate configurations.

> 
>> Perhaps we can accomplish this with firstaction/endscript? E.g. something like
>>
>> diff --git a/src/remote/libvirtd.qemu.logrotate.in
>> b/src/remote/libvirtd.qemu.logrotate.in
>> index cdb399ef23..e14adb10e8 100644
>> --- a/src/remote/libvirtd.qemu.logrotate.in
>> +++ b/src/remote/libvirtd.qemu.logrotate.in
>> @@ -1,4 +1,10 @@
>>    @localstatedir@/log/libvirt/qemu/*.log {
>> +        firstaction
>> +               pgrep virtlogd > /dev/null 2>&1
>> +               if [ $? -ne 0 ]; then
>> +                       exit 1
>> +               fi
> 
> In addition to the above arguments, with this implementation it might
> miss the case where virtlogd is configured to never rotate.

I'd think the admin doesn't want rotation if they've configured virtlogd to 
never rotate.

Regards,
Jim