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

Jan Zerebecki posted 1 patch 4 years, 9 months ago
Test syntax-check passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20190708182823.26735-1-jan.suse@zerebecki.de
src/remote/libvirtd.qemu.logrotate.in | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
[libvirt] [PATCHv2] Do not keep empty log files for deleted domains
Posted by Jan Zerebecki 4 years, 9 months ago
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@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
 }
-- 
2.20.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2] Do not keep empty log files for deleted domains
Posted by Jim Fehlig 4 years, 9 months ago
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@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". 
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
+        endscript
          weekly
          missingok
          rotate 4

Regards,
Jim

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2] Do not keep empty log files for deleted domains
Posted by Jan Zerebecki 4 years, 9 months ago

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

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

-- 
Best regards,
Jan Zerebecki

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2] Do not keep empty log files for deleted domains
Posted by Daniel P. Berrangé 4 years, 9 months ago
On Tue, Jul 09, 2019 at 03:51:41AM +0200, 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@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. 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. 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.

If virtlogd is active, we do *not* want logrotate doing anything at all.
Trying to get sensible interaction between two separate log rotation
apps is adding too much complexity.


Further, any purging of log files needs to take in to account what
guests actually exist in libvirt. The proposed change has no such
checks. It must only purge log files for guests which are neither
running, nor have any persistent config on disk, and where the log
file is older than "N days" for some configurable "N".

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2] Do not keep empty log files for deleted domains
Posted by Jan Zerebecki 4 years, 9 months ago
On 09/07/2019 10.35, Daniel P. Berrangé wrote:
> If virtlogd is active, we do *not* want logrotate doing anything at all.

Are you saying that to fix the bug at hand, I should be required to
first implement 2 features in virtlogd and replace the current logrotate
config (none of which caused the bug)?

> Trying to get sensible interaction between two separate log rotation
> apps is adding too much complexity.

It is not added. It is already there. Multiple distros I checked ship
this as the default config. Sorry, if that wasn't your intention.

> Further, any purging of log files needs to take in to account what
> guests actually exist in libvirt. The proposed change has no such
> checks. It must only purge log files for guests which are neither
> running, nor have any persistent config on disk, and where the log
> file is older than "N days" for some configurable "N".

Not if time based rotation is already desired, then there is no need to
know if the guest still exists. If its not desired, then this is as
complicated as you say, but the existing logrotate config already
doesn't fulfill that.

-- 
Best regards,
Jan Zerebecki

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2] Do not keep empty log files for deleted domains
Posted by Daniel P. Berrangé 4 years, 9 months ago
On Tue, Jul 09, 2019 at 12:55:18PM +0200, Jan Zerebecki wrote:
> 
> On 09/07/2019 10.35, Daniel P. Berrangé wrote:
> > If virtlogd is active, we do *not* want logrotate doing anything at all.
> 
> Are you saying that to fix the bug at hand, I should be required to
> first implement 2 features in virtlogd and replace the current logrotate
> config (none of which caused the bug)?

I'm simply saying that trying to use logrotate at the same time
as virtlogd is not a desirable way to solve the problem. There
are a variety of other options that could be explored

> > Trying to get sensible interaction between two separate log rotation
> > apps is adding too much complexity.
> 
> It is not added. It is already there. Multiple distros I checked ship
> this as the default config. Sorry, if that wasn't your intention.

Shipping the configs is not a problem as long as make the logrotate
config a no-op when virtlogd is active, which was the intention and
is fixed in:

  https://www.redhat.com/archives/libvir-list/2019-July/msg00313.html

> > Further, any purging of log files needs to take in to account what
> > guests actually exist in libvirt. The proposed change has no such
> > checks. It must only purge log files for guests which are neither
> > running, nor have any persistent config on disk, and where the log
> > file is older than "N days" for some configurable "N".
> 
> Not if time based rotation is already desired, then there is no need to
> know if the guest still exists. If its not desired, then this is as
> complicated as you say, but the existing logrotate config already
> doesn't fulfill that.

It will no longer be time based with the above fix.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2] Do not keep empty log files for deleted domains
Posted by Jim Fehlig 4 years, 9 months ago
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@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list