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

Jan Zerebecki posted 1 patch 4 years, 10 months ago
Test syntax-check passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20190702182409.16576-1-jan.suse@zerebecki.de
There is a newer version of this series
src/remote/libvirtd.libxl.logrotate.in | 4 +++-
src/remote/libvirtd.logrotate.in       | 4 +++-
src/remote/libvirtd.lxc.logrotate.in   | 4 +++-
src/remote/libvirtd.qemu.logrotate.in  | 4 +++-
4 files changed, 12 insertions(+), 4 deletions(-)
[libvirt] [PATCH] Do not keep empty log files for deleted domains
Posted by Jan Zerebecki 4 years, 10 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.

Signed-off-by: Jan Zerebecki <jan.suse@zerebecki.de>
---
 src/remote/libvirtd.libxl.logrotate.in | 4 +++-
 src/remote/libvirtd.logrotate.in       | 4 +++-
 src/remote/libvirtd.lxc.logrotate.in   | 4 +++-
 src/remote/libvirtd.qemu.logrotate.in  | 4 +++-
 4 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/src/remote/libvirtd.libxl.logrotate.in b/src/remote/libvirtd.libxl.logrotate.in
index cb7f07d846..d4f25f8cab 100644
--- a/src/remote/libvirtd.libxl.logrotate.in
+++ b/src/remote/libvirtd.libxl.logrotate.in
@@ -4,5 +4,7 @@
         rotate 4
         compress
         delaycompress
-        copytruncate
+        postrotate
+                /usr/bin/killall -USR1 virtlogd
+        endscript
 }
diff --git a/src/remote/libvirtd.logrotate.in b/src/remote/libvirtd.logrotate.in
index 4e02510c8b..73dab7579f 100644
--- a/src/remote/libvirtd.logrotate.in
+++ b/src/remote/libvirtd.logrotate.in
@@ -4,6 +4,8 @@
         rotate 4
         compress
         delaycompress
-        copytruncate
         minsize 100k
+        postrotate
+                /usr/bin/killall -USR1 virtlogd
+        endscript
 }
diff --git a/src/remote/libvirtd.lxc.logrotate.in b/src/remote/libvirtd.lxc.logrotate.in
index 2bb9dfba12..eed47bb55f 100644
--- a/src/remote/libvirtd.lxc.logrotate.in
+++ b/src/remote/libvirtd.lxc.logrotate.in
@@ -4,5 +4,7 @@
         rotate 4
         compress
         delaycompress
-        copytruncate
+        postrotate
+                /usr/bin/killall -USR1 virtlogd
+        endscript
 }
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] [PATCH] Do not keep empty log files for deleted domains
Posted by Peter Krempa 4 years, 10 months ago
On Tue, Jul 02, 2019 at 20:24:09 +0200, 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.
> 
> Signed-off-by: Jan Zerebecki <jan.suse@zerebecki.de>
> ---
>  src/remote/libvirtd.libxl.logrotate.in | 4 +++-
>  src/remote/libvirtd.logrotate.in       | 4 +++-
>  src/remote/libvirtd.lxc.logrotate.in   | 4 +++-
>  src/remote/libvirtd.qemu.logrotate.in  | 4 +++-
>  4 files changed, 12 insertions(+), 4 deletions(-)

I'm not sure that this is the right thing to do. virtlogd has some
internal log rotation mechanisms and also the SIGUSR1 action is reserved
for re-exec updates and not for dealing with modified files

Additionally the internals are supposed to keep all the FD's open during
re-exec so I'm not even sure that this will help.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Do not keep empty log files for deleted domains
Posted by Jan Zerebecki 4 years, 10 months ago
On 03/07/2019 08.03, Peter Krempa wrote:
> I'm not sure that this is the right thing to do. virtlogd has some
> internal log rotation mechanisms

logrotate is already in use here and this patch doesn't change what is
rotated nor how often it is rotated.

> and also the SIGUSR1 action is reserved
> for re-exec updates and not for dealing with modified files

The mechanism (re-exec) and implementation details for both uses do not
conflict.

> Additionally the internals are supposed to keep all the FD's open during
> re-exec so I'm not even sure that this will help.

This test shows that it helps:
# ls -la /var/log/libvirt/qemu/foo.log
-rw------- 1 root root 0 Jun 30 00:00 /var/log/libvirt/qemu/foo.log
# mv /var/log/libvirt/qemu/foo.log /var/log/libvirt/qemu/foo.log.0
# /usr/bin/killall -USR1 virtlogd
# ls -la /var/log/libvirt/qemu/foo.log
-rw------- 1 root root 0 Jul  3 08:06 /var/log/libvirt/qemu/foo.log

-- 
Best regards,
Jan Zerebecki

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Do not keep empty log files for deleted domains
Posted by Peter Krempa 4 years, 10 months ago
On Wed, Jul 03, 2019 at 08:32:21 +0200, Jan Zerebecki wrote:
> On 03/07/2019 08.03, Peter Krempa wrote:
> > I'm not sure that this is the right thing to do. virtlogd has some
> > internal log rotation mechanisms
> 
> logrotate is already in use here and this patch doesn't change what is
> rotated nor how often it is rotated.
> 
> > and also the SIGUSR1 action is reserved
> > for re-exec updates and not for dealing with modified files
> 
> The mechanism (re-exec) and implementation details for both uses do not
> conflict.

The problem is that the logrotate configs predate use of virtlogd and I
don't think they were adapted to be used together. In fact if I remember
correctly the idea of virtlogd was to avoid using logrotate altogether.

Additionally virtlogd is currently used only with the qemu VM log files.
Everything else logs itself, so your changes to libvirtd/lxc and others
seem to be unnecessary.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Do not keep empty log files for deleted domains
Posted by Jan Zerebecki 4 years, 10 months ago

On 03/07/2019 08.55, Peter Krempa wrote:
> On Wed, Jul 03, 2019 at 08:32:21 +0200, Jan Zerebecki wrote:
>> On 03/07/2019 08.03, Peter Krempa wrote:
>>> I'm not sure that this is the right thing to do. virtlogd has some
>>> internal log rotation mechanisms
>>
>> logrotate is already in use here and this patch doesn't change what is
>> rotated nor how often it is rotated.
>>
>>> and also the SIGUSR1 action is reserved
>>> for re-exec updates and not for dealing with modified files
>>
>> The mechanism (re-exec) and implementation details for both uses do not
>> conflict.
> 
> The problem is that the logrotate configs predate use of virtlogd and I
> don't think they were adapted to be used together. In fact if I remember
> correctly the idea of virtlogd was to avoid using logrotate altogether.

virtlogd rotates by size and logrotate in this config by time, so they
work fine together. Yes, you could implement rotating out logfiles from
deleted domains in virtlogd, but I don't see a reason to reimplement it.

> Additionally virtlogd is currently used only with the qemu VM log files.
> Everything else logs itself, so your changes to libvirtd/lxc and others
> seem to be unnecessary.

I don't have some of these handy to test, how do you get them to reopen
logs?

-- 
Best regards,
Jan Zerebecki

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Do not keep empty log files for deleted domains
Posted by Daniel P. Berrangé 4 years, 10 months ago
On Wed, Jul 03, 2019 at 08:55:15AM +0200, Peter Krempa wrote:
> On Wed, Jul 03, 2019 at 08:32:21 +0200, Jan Zerebecki wrote:
> > On 03/07/2019 08.03, Peter Krempa wrote:
> > > I'm not sure that this is the right thing to do. virtlogd has some
> > > internal log rotation mechanisms
> > 
> > logrotate is already in use here and this patch doesn't change what is
> > rotated nor how often it is rotated.
> > 
> > > and also the SIGUSR1 action is reserved
> > > for re-exec updates and not for dealing with modified files
> > 
> > The mechanism (re-exec) and implementation details for both uses do not
> > conflict.
> 
> The problem is that the logrotate configs predate use of virtlogd and I
> don't think they were adapted to be used together. In fact if I remember
> correctly the idea of virtlogd was to avoid using logrotate altogether.

Yes, logrotate should not be in effect when virtlogd is running. We should
ensure that logrotate rollover size is *larger* than the rollover size
configured for virtlogd. This would ensure virtlogd rolls over first, and
thus when logrotate runs it should see nothing todo.


> Additionally virtlogd is currently used only with the qemu VM log files.
> Everything else logs itself, so your changes to libvirtd/lxc and others
> seem to be unnecessary.

Yep, they're simply wrong.

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] [PATCH] Do not keep empty log files for deleted domains
Posted by Jim Fehlig 4 years, 10 months ago
On 7/3/19 2:42 AM, Daniel P. Berrangé  wrote:
> On Wed, Jul 03, 2019 at 08:55:15AM +0200, Peter Krempa wrote:
>> On Wed, Jul 03, 2019 at 08:32:21 +0200, Jan Zerebecki wrote:
>>> On 03/07/2019 08.03, Peter Krempa wrote:
>>>> I'm not sure that this is the right thing to do. virtlogd has some
>>>> internal log rotation mechanisms
>>>
>>> logrotate is already in use here and this patch doesn't change what is
>>> rotated nor how often it is rotated.
>>>
>>>> and also the SIGUSR1 action is reserved
>>>> for re-exec updates and not for dealing with modified files
>>>
>>> The mechanism (re-exec) and implementation details for both uses do not
>>> conflict.
>>
>> The problem is that the logrotate configs predate use of virtlogd and I
>> don't think they were adapted to be used together. In fact if I remember
>> correctly the idea of virtlogd was to avoid using logrotate altogether.
> 
> Yes, logrotate should not be in effect when virtlogd is running. We should
> ensure that logrotate rollover size is *larger* than the rollover size
> configured for virtlogd. This would ensure virtlogd rolls over first, and
> thus when logrotate runs it should see nothing todo.

Hmm, it sounds like the proper fix is to simply remove
src/remote/libvirtd.qemu.logrotate.in?

Regards,
Jim

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Do not keep empty log files for deleted domains
Posted by Daniel P. Berrangé 4 years, 10 months ago
On Wed, Jul 03, 2019 at 03:26:36PM +0000, Jim Fehlig wrote:
> On 7/3/19 2:42 AM, Daniel P. Berrangé  wrote:
> > On Wed, Jul 03, 2019 at 08:55:15AM +0200, Peter Krempa wrote:
> >> On Wed, Jul 03, 2019 at 08:32:21 +0200, Jan Zerebecki wrote:
> >>> On 03/07/2019 08.03, Peter Krempa wrote:
> >>>> I'm not sure that this is the right thing to do. virtlogd has some
> >>>> internal log rotation mechanisms
> >>>
> >>> logrotate is already in use here and this patch doesn't change what is
> >>> rotated nor how often it is rotated.
> >>>
> >>>> and also the SIGUSR1 action is reserved
> >>>> for re-exec updates and not for dealing with modified files
> >>>
> >>> The mechanism (re-exec) and implementation details for both uses do not
> >>> conflict.
> >>
> >> The problem is that the logrotate configs predate use of virtlogd and I
> >> don't think they were adapted to be used together. In fact if I remember
> >> correctly the idea of virtlogd was to avoid using logrotate altogether.
> > 
> > Yes, logrotate should not be in effect when virtlogd is running. We should
> > ensure that logrotate rollover size is *larger* than the rollover size
> > configured for virtlogd. This would ensure virtlogd rolls over first, and
> > thus when logrotate runs it should see nothing todo.
> 
> Hmm, it sounds like the proper fix is to simply remove
> src/remote/libvirtd.qemu.logrotate.in?

We didn't want to remove it because whether or not to use virtlogd is
a config file tunable. Hence we want to make sure that its default
settings end up being a no-op if virtlogd has already done its own
rollover

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