Change in vdsm[master]: vdsm: Rely on system for logrotation

[Tomas Golembiovsky]

Tomas Golembiovsky has uploaded a new change for review.

Change subject: vdsm: Rely on system for logrotation
......................................................................

vdsm: Rely on system for logrotation

When logrotate is installed it is normaly run periodicaly by cron. There
is no real reason to invoke logrotate manualy.

Also if we rely on system to run logrotate we can remove the dependency
on it from RPMs. It is enough to install the configuration file into the
the directory with logrotate configuration.

The patch also renames script vdsm-logrotate to vdsm-remove-logs because
it doesn't run logrotate anymore. It only removes old log files.

Change-Id: Ica888244bd7c65121f55983e5716a6eae5662879
Signed-off-by: Tomáš Golembiovský <tgolembi(a)redhat.com&gt;
---
M static/Makefile.am
R static/etc/logrotate.d/vdsm
M vdsm.spec.in
M vdsm/Makefile.am
R vdsm/vdsm-remove-logs
5 files changed, 12 insertions(+), 21 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/82/63682/1

diff --git a/static/Makefile.am b/static/Makefile.am
index 040d6ec..22c8736 100644
--- a/static/Makefile.am
+++ b/static/Makefile.am
@@ -37,10 +37,10 @@
 	./etc/vdsm/mom.d/05-iotune.policy \
 	$(NULL)
 
-vdsmconfrotatedir = $(vdsmconfdir)/logrotate
+vdsmconfrotatedir = $(sysconfdir)/logrotate.d
 
 vdsmconfrotate_DATA = \
-	./etc/vdsm/logrotate/vdsm \
+	./etc/logrotate.d/vdsm \
 	$(NULL)
 
 sudoersdir = $(sysconfdir)/sudoers.d
diff --git a/static/etc/vdsm/logrotate/vdsm b/static/etc/logrotate.d/vdsm
similarity index 100%
rename from static/etc/vdsm/logrotate/vdsm
rename to static/etc/logrotate.d/vdsm
diff --git a/vdsm.spec.in b/vdsm.spec.in
index 26c36d3..dafeb9c 100644
--- a/vdsm.spec.in
+++ b/vdsm.spec.in
@@ -131,7 +131,6 @@
 Requires: ethtool
 Requires: which
 Requires: sudo >= 1.7.3
-Requires: logrotate
 Requires: xz
 Requires: ntp
 Requires: iproute >= 3.10.0
@@ -948,7 +947,7 @@
 %config(noreplace) %{_sysconfdir}/%{vdsm_name}/svdsm.logger.conf
 %config(noreplace) %{_sysconfdir}/%{vdsm_name}/mom.conf
 %config(noreplace) %{_sysconfdir}/%{vdsm_name}/mom.d/*.policy
-%config(noreplace) %{_sysconfdir}/%{vdsm_name}/logrotate/vdsm
+%config(noreplace) %{_sysconfdir}/logrotate.d/vdsm
 %config(noreplace) %{_sysconfdir}/rwtab.d/vdsm
 %config(noreplace) %{_sysconfdir}/sysctl.d/vdsm.conf
 %config(noreplace) %{_sysconfdir}/modules-load.d/vdsm.conf
@@ -956,7 +955,7 @@
 %{_sysconfdir}/dhcp/dhclient.d/sourceRoute.sh
 %{_sysconfdir}/modprobe.d/vdsm-bonding-modprobe.conf
 %{_sysconfdir}/sudoers.d/50_vdsm
-%{_sysconfdir}/cron.hourly/vdsm-logrotate
+%{_sysconfdir}/cron.hourly/vdsm-remove-logs
 %{_sysconfdir}/libvirt/hooks/qemu
 %{_libexecdir}/%{vdsm_name}/curl-img-wrap
 %{_libexecdir}/%{vdsm_name}/fc-scan
diff --git a/vdsm/Makefile.am b/vdsm/Makefile.am
index 8b535b6..0b5e807 100644
--- a/vdsm/Makefile.am
+++ b/vdsm/Makefile.am
@@ -76,7 +76,7 @@
 	vdsm-gencerts.sh.in \
 	vdsm-libvirt-access.pkla \
 	vdsm-libvirt-access.rules \
-	vdsm-logrotate \
+	vdsm-remove-logs \
 	vdsm-store-net-config.in \
 	vdsmd.8.in \
 	$(NULL)
@@ -94,7 +94,7 @@
 		install-data-bonding-defaults \
 		install-data-dhclient-hooks \
 		install-data-libvirtpass \
-		install-data-logrotate \
+		install-data-logs \
 		install-data-vdsmconf
 	$(MKDIR_P) $(DESTDIR)$(vdsmrepo)
 	$(MKDIR_P) $(DESTDIR)$(vdsmrepo)/hsm-tasks
@@ -118,7 +118,7 @@
 		uninstall-data-bonding-defaults \
 		uninstall-data-dhclient-hooks \
 		uninstall-data-libvirtpass \
-		uninstall-data-logrotate \
+		uninstall-data-logs \
 		uninstall-data-vdsmconf
 
 install-data-libvirtpass:
@@ -139,14 +139,13 @@
 	$(RM) $(DESTDIR)$(vdsmdir)/bonding-defaults.json
 	$(RM) $(DESTDIR)$(vdsmdir)/bonding-name2numeric.json
 
-install-data-logrotate:
+install-data-logs:
 	$(MKDIR_P) $(DESTDIR)$(sysconfdir)/cron.hourly
-	$(INSTALL_SCRIPT) $(srcdir)/vdsm-logrotate \
-		$(DESTDIR)$(sysconfdir)/cron.hourly/vdsm-logrotate
-	$(MKDIR_P) $(DESTDIR)$(sysconfdir)/cron.d
+	$(INSTALL_SCRIPT) $(srcdir)/vdsm-remove-logs \
+		$(DESTDIR)$(sysconfdir)/cron.hourly/vdsm-remove-logs
 
-uninstall-data-logrotate:
-	$(RM) $(DESTDIR)$(sysconfdir)/cron.hourly/vdsm-logrotate
+uninstall-data-logs:
+	$(RM) $(DESTDIR)$(sysconfdir)/cron.hourly/vdsm-remove-logs
 
 install-data-vdsmconf:
 	$(MKDIR_P) $(DESTDIR)$(vdsmconfdir)/vdsm.conf.d
diff --git a/vdsm/vdsm-logrotate b/vdsm/vdsm-remove-logs
similarity index 77%
rename from vdsm/vdsm-logrotate
rename to vdsm/vdsm-remove-logs
index 11050b9..7760b6b 100755
--- a/vdsm/vdsm-logrotate
+++ b/vdsm/vdsm-remove-logs
@@ -1,12 +1,5 @@
 #!/bin/sh
 
-/usr/sbin/logrotate /etc/vdsm/logrotate/vdsm
-
-EXITVALUE=$?
-if [ $EXITVALUE != 0 ]; then
-    /usr/bin/logger -t logrotate "ALERT exited abnormally with [$EXITVALUE]"
-fi
-
 if [ -d /var/log/core ] ; then
     /usr/bin/find /var/log/core -type f -name '*xz' -mtime +7 -exec /bin/rm -f
'{}' \;
     EXITVALUE=$?


-- 
To view, visit https://gerrit.ovirt.org/63682
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ica888244bd7c65121f55983e5716a6eae5662879
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Golembiovsky <tgolembi(a)redhat.com&gt;

gerrit-hooks has posted comments on this change.

Change subject: vdsm: Rely on system for logrotation
......................................................................


Patch Set 1:

* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and
is a valid url.
* Check merged to previous::IGNORE,         Not in stable branch (['ovirt-3.6',
'ovirt-4.0'])

-- 
To view, visit https://gerrit.ovirt.org/63682
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ica888244bd7c65121f55983e5716a6eae5662879
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Golembiovsky <tgolembi(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Francesco Romani has posted comments on this change.

Change subject: vdsm: Rely on system for logrotation
......................................................................


Patch Set 1: Code-Review+1

initial review

-- 
To view, visit https://gerrit.ovirt.org/63682
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ica888244bd7c65121f55983e5716a6eae5662879
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Golembiovsky <tgolembi(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: vdsm: Rely on system for logrotation
......................................................................


Patch Set 1:

(4 comments)

https://gerrit.ovirt.org/#/c/63682/1//COMMIT_MSG
Commit Message:

Line 10: is no real reason to invoke logrotate manualy.
Line 11: 
Line 12: Also if we rely on system to run logrotate we can remove the dependency
Line 13: on it from RPMs. It is enough to install the configuration file into the
Line 14: the directory with logrotate configuration.
What if logrotate is not installed on the system?
Line 15: 
Line 16: The patch also renames script vdsm-logrotate to vdsm-remove-logs because
Line 17: it doesn't run logrotate anymore. It only removes old log files.
Line 18: 


https://gerrit.ovirt.org/#/c/63682/1/static/Makefile.am
File static/Makefile.am:

Line 36: 	./etc/vdsm/mom.d/04-cputune.policy \
Line 37: 	./etc/vdsm/mom.d/05-iotune.policy \
Line 38: 	$(NULL)
Line 39: 
Line 40: vdsmconfrotatedir = $(sysconfdir)/logrotate.d
This looks more like syslogrotatedir - nothing in this path is related to vdsm, so better
update the name.
Line 41: 
Line 42: vdsmconfrotate_DATA = \
Line 43: 	./etc/logrotate.d/vdsm \
Line 44: 	$(NULL)


https://gerrit.ovirt.org/#/c/63682/1/vdsm.spec.in
File vdsm.spec.in:

Line 954: %{_tmpfilesdir}/%{vdsm_name}.conf
Line 955: %{_sysconfdir}/dhcp/dhclient.d/sourceRoute.sh
Line 956: %{_sysconfdir}/modprobe.d/vdsm-bonding-modprobe.conf
Line 957: %{_sysconfdir}/sudoers.d/50_vdsm
Line 958: %{_sysconfdir}/cron.hourly/vdsm-remove-logs
Why do we need to remove logs if logrotate is handling the logs?
Line 959: %{_sysconfdir}/libvirt/hooks/qemu
Line 960: %{_libexecdir}/%{vdsm_name}/curl-img-wrap
Line 961: %{_libexecdir}/%{vdsm_name}/fc-scan
Line 962: %{_libexecdir}/%{vdsm_name}/persist-vdsm-hooks


https://gerrit.ovirt.org/#/c/63682/1/vdsm/vdsm-remove-logs
File vdsm/vdsm-remove-logs:

Line 20:     EXITVALUE=$?
Line 21:     if [ $EXITVALUE != 0 ]; then
Line 22:         /usr/bin/logger -t logrotate "ALERT clean of old import log files
exited abnormally with [$EXITVALUE]"
Line 23:     fi
Line 24: fi
Can we use logrotate for this?
Line 25: 


-- 
To view, visit https://gerrit.ovirt.org/63682
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ica888244bd7c65121f55983e5716a6eae5662879
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Golembiovsky <tgolembi(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

[Piotr Kliczewski]

Piotr Kliczewski has posted comments on this change.

Change subject: vdsm: Rely on system for logrotation
......................................................................


Patch Set 1:

(2 comments)

https://gerrit.ovirt.org/#/c/63682/1//COMMIT_MSG
Commit Message:

Line 5: CommitDate: 2016-09-11 13:33:20 +0200
Line 6: 
Line 7: vdsm: Rely on system for logrotation
Line 8: 
Line 9: When logrotate is installed it is normaly run periodicaly by cron. There
I am missing value of this change here. Why do we want to do it?
Line 10: is no real reason to invoke logrotate manualy.
Line 11: 
Line 12: Also if we rely on system to run logrotate we can remove the dependency
Line 13: on it from RPMs. It is enough to install the configuration file into the


Line 10: is no real reason to invoke logrotate manualy.
Line 11: 
Line 12: Also if we rely on system to run logrotate we can remove the dependency
Line 13: on it from RPMs. It is enough to install the configuration file into the
Line 14: the directory with logrotate configuration.
>  What if logrotate is not installed on the system? 
Based on spec
change in line 134 it was installed with vdsm.
Line 15: 
Line 16: The patch also renames script vdsm-logrotate to vdsm-remove-logs because
Line 17: it doesn't run logrotate anymore. It only removes old log files.
Line 18: 


-- 
To view, visit https://gerrit.ovirt.org/63682
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ica888244bd7c65121f55983e5716a6eae5662879
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Golembiovsky <tgolembi(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

[Tomas Golembiovsky]

Tomas Golembiovsky has posted comments on this change.

Change subject: vdsm: Rely on system for logrotation
......................................................................


Patch Set 1:

(4 comments)

https://gerrit.ovirt.org/#/c/63682/1//COMMIT_MSG
Commit Message:

Line 5: CommitDate: 2016-09-11 13:33:20 +0200
Line 6: 
Line 7: vdsm: Rely on system for logrotation
Line 8: 
Line 9: When logrotate is installed it is normaly run periodicaly by cron. There
>  I am missing value of this change here. Why do we want to do it?
The value is simplification at least on the two fronts: 

1) We are duplicating code that is already present in the system. If logrotate is already
installed it knows how to make sure it is run properly.

2) We are enforcing dependency on logrotate. The dependency is not necessary. See next
comment.
Line 10: is no real reason to invoke logrotate manualy.
Line 11: 
Line 12: Also if we rely on system to run logrotate we can remove the dependency
Line 13: on it from RPMs. It is enough to install the configuration file into the


Line 10: is no real reason to invoke logrotate manualy.
Line 11: 
Line 12: Also if we rely on system to run logrotate we can remove the dependency
Line 13: on it from RPMs. It is enough to install the configuration file into the
Line 14: the directory with logrotate configuration.
>  Based on spec change in line 134 it was installed with vdsm.
It's not our problem. Whether logrotate is installed or not should be
administrator's decision and not ours. We are present with a .d directory where to put
our configuration. If and how the configuraiton is used shouldn't be our concern
(arguably in d/s, but definitely not in u/s).

NB: IIRC logrotate is installed as part of base system on RHEL and CentOS. So if the
package is not installed there is probably a reason for that.
Line 15: 
Line 16: The patch also renames script vdsm-logrotate to vdsm-remove-logs because
Line 17: it doesn't run logrotate anymore. It only removes old log files.
Line 18: 


https://gerrit.ovirt.org/#/c/63682/1/static/Makefile.am
File static/Makefile.am:

Line 36: 	./etc/vdsm/mom.d/04-cputune.policy \
Line 37: 	./etc/vdsm/mom.d/05-iotune.policy \
Line 38: 	$(NULL)
Line 39: 
Line 40: vdsmconfrotatedir = $(sysconfdir)/logrotate.d
>  This looks more like syslogrotatedir - nothing in this path is
> related to v 
You mean to define new variable like:

    syslogrotatedir=${sysconfdir}/logrotate.d

and use the variable in Makefile? (sysconfdir comes from configure)
Line 41: 
Line 42: vdsmconfrotate_DATA = \
Line 43: 	./etc/logrotate.d/vdsm \
Line 44: 	$(NULL)


https://gerrit.ovirt.org/#/c/63682/1/vdsm/vdsm-remove-logs
File vdsm/vdsm-remove-logs:

Line 20:     EXITVALUE=$?
Line 21:     if [ $EXITVALUE != 0 ]; then
Line 22:         /usr/bin/logger -t logrotate "ALERT clean of old import log files
exited abnormally with [$EXITVALUE]"
Line 23:     fi
Line 24: fi
>  Can we use logrotate for this? 
That would be nice. But
unfortunately I have no idea how to do that.
Line 25: 


-- 
To view, visit https://gerrit.ovirt.org/63682
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ica888244bd7c65121f55983e5716a6eae5662879
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Golembiovsky <tgolembi(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: Tomas Golembiovsky <tgolembi(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: vdsm: Rely on system for logrotation
......................................................................


Patch Set 1:

(4 comments)

https://gerrit.ovirt.org/#/c/63682/1//COMMIT_MSG
Commit Message:

Line 10: is no real reason to invoke logrotate manualy.
Line 11: 
Line 12: Also if we rely on system to run logrotate we can remove the dependency
Line 13: on it from RPMs. It is enough to install the configuration file into the
Line 14: the directory with logrotate configuration.
>  It's not our problem. Whether logrotate is installed or not
> should be admin 
This approach is problematic.

Previously we rotated the log, now you want to let the user decide. We don't have any
user or customer bug complaining about rotated logs, and we know that vdsm logs are huge.

I don't think we can do this, this can be harmful on machines with small harddisks
(like node).

Using logrotate is little nicer, filling /var/log is not. Please require logrotate in this
patch.

If a user want to disable vdsm log rotation she can disable the vdsm logrotate
configuration or edit it.
Line 15: 
Line 16: The patch also renames script vdsm-logrotate to vdsm-remove-logs because
Line 17: it doesn't run logrotate anymore. It only removes old log files.
Line 18: 


https://gerrit.ovirt.org/#/c/63682/1/static/Makefile.am
File static/Makefile.am:

Line 36: 	./etc/vdsm/mom.d/04-cputune.policy \
Line 37: 	./etc/vdsm/mom.d/05-iotune.policy \
Line 38: 	$(NULL)
Line 39: 
Line 40: vdsmconfrotatedir = $(sysconfdir)/logrotate.d
>  You mean to define new variable like: 
Yes
Line 41: 
Line 42: vdsmconfrotate_DATA = \
Line 43: 	./etc/logrotate.d/vdsm \
Line 44: 	$(NULL)


https://gerrit.ovirt.org/#/c/63682/1/vdsm/vdsm-remove-logs
File vdsm/vdsm-remove-logs:

Line 10: if [ -d /var/log/core ] ; then
Line 11:     /usr/bin/find /var/log/core -type f -name '*xz' -mtime +7 -exec
/bin/rm -f '{}' \;
Line 12:     EXITVALUE=$?
Line 13:     if [ $EXITVALUE != 0 ]; then
Line 14:         /usr/bin/logger -t logrotate "ALERT clean old core files exited
abnormally with [$EXITVALUE]"
If we depend on logrotate running from cron, why do we need to run it here?
Line 15:     fi
Line 16: fi
Line 17: 
Line 18: if [ -d /var/log/vdsm/import ] ; then


Line 18: if [ -d /var/log/vdsm/import ] ; then
Line 19:     /usr/bin/find /var/log/vdsm/import -type f -mtime +30 -exec /bin/rm -f
'{}' \;
Line 20:     EXITVALUE=$?
Line 21:     if [ $EXITVALUE != 0 ]; then
Line 22:         /usr/bin/logger -t logrotate "ALERT clean of old import log files
exited abnormally with [$EXITVALUE]"
Same
Line 23:     fi
Line 24: fi
Line 25: 


-- 
To view, visit https://gerrit.ovirt.org/63682
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ica888244bd7c65121f55983e5716a6eae5662879
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Golembiovsky <tgolembi(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Fabian Deutsch <fabiand(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: Tomas Golembiovsky <tgolembi(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

[Tomas Golembiovsky]

Tomas Golembiovsky has posted comments on this change.

Change subject: vdsm: Rely on system for logrotation
......................................................................


Patch Set 1:

(4 comments)

https://gerrit.ovirt.org/#/c/63682/1//COMMIT_MSG
Commit Message:

Line 10: is no real reason to invoke logrotate manualy.
Line 11: 
Line 12: Also if we rely on system to run logrotate we can remove the dependency
Line 13: on it from RPMs. It is enough to install the configuration file into the
Line 14: the directory with logrotate configuration.
>  This approach is problematic. 
Ok, let's take one step at a
time then. I will restore the denpendency on logrotate.
Line 15: 
Line 16: The patch also renames script vdsm-logrotate to vdsm-remove-logs because
Line 17: it doesn't run logrotate anymore. It only removes old log files.
Line 18: 


https://gerrit.ovirt.org/#/c/63682/1/static/Makefile.am
File static/Makefile.am:

Line 36: 	./etc/vdsm/mom.d/04-cputune.policy \
Line 37: 	./etc/vdsm/mom.d/05-iotune.policy \
Line 38: 	$(NULL)
Line 39: 
Line 40: vdsmconfrotatedir = $(sysconfdir)/logrotate.d
>  Yes 
Ah, now I understand what you're aiming at. Will do.
Line 41: 
Line 42: vdsmconfrotate_DATA = \
Line 43: 	./etc/logrotate.d/vdsm \
Line 44: 	$(NULL)


https://gerrit.ovirt.org/#/c/63682/1/vdsm/vdsm-remove-logs
File vdsm/vdsm-remove-logs:

Line 10: if [ -d /var/log/core ] ; then
Line 11:     /usr/bin/find /var/log/core -type f -name '*xz' -mtime +7 -exec
/bin/rm -f '{}' \;
Line 12:     EXITVALUE=$?
Line 13:     if [ $EXITVALUE != 0 ]; then
Line 14:         /usr/bin/logger -t logrotate "ALERT clean old core files exited
abnormally with [$EXITVALUE]"
>  If we depend on logrotate running from cron, why do we need to run it
> here? 
We don't. This only logs a message to syslog. Maybe we should change the
tag (-t) from 'logrotate' to 'vdsm'. To avoid cofusion and make it clear
where the log message comes from.
Line 15:     fi
Line 16: fi
Line 17: 
Line 18: if [ -d /var/log/vdsm/import ] ; then


Line 18: if [ -d /var/log/vdsm/import ] ; then
Line 19:     /usr/bin/find /var/log/vdsm/import -type f -mtime +30 -exec /bin/rm -f
'{}' \;
Line 20:     EXITVALUE=$?
Line 21:     if [ $EXITVALUE != 0 ]; then
Line 22:         /usr/bin/logger -t logrotate "ALERT clean of old import log files
exited abnormally with [$EXITVALUE]"
>  Same 
Same as above.
Line 23:     fi
Line 24: fi
Line 25: 


-- 
To view, visit https://gerrit.ovirt.org/63682
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ica888244bd7c65121f55983e5716a6eae5662879
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Golembiovsky <tgolembi(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Fabian Deutsch <fabiand(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: Tomas Golembiovsky <tgolembi(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

gerrit-hooks has posted comments on this change.

Change subject: vdsm: Rely on system for logrotation
......................................................................


Patch Set 2:

* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and
is a valid url.
* Check merged to previous::IGNORE,         Not in stable branch (['ovirt-3.6',
'ovirt-4.0'])

-- 
To view, visit https://gerrit.ovirt.org/63682
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ica888244bd7c65121f55983e5716a6eae5662879
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Golembiovsky <tgolembi(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Fabian Deutsch <fabiand(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: Tomas Golembiovsky <tgolembi(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

gerrit-hooks has posted comments on this change.

Change subject: vdsm: Rely on system for logrotation
......................................................................


Patch Set 3:

* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and
is a valid url.
* Check merged to previous::IGNORE,         Not in stable branch (['ovirt-3.6',
'ovirt-4.0'])

-- 
To view, visit https://gerrit.ovirt.org/63682
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ica888244bd7c65121f55983e5716a6eae5662879
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Golembiovsky <tgolembi(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Fabian Deutsch <fabiand(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: Tomas Golembiovsky <tgolembi(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: vdsm: Rely on system for logrotation
......................................................................


Patch Set 3: Code-Review+1

(1 comment)

https://gerrit.ovirt.org/#/c/63682/3/vdsm/vdsm-remove-logs
File vdsm/vdsm-remove-logs:

Line 15:         /usr/bin/logger -t vdsm "ALERT clean of old import log files exited
abnormally with [$EXITVALUE]"
Line 16:     fi
Line 17: fi
Line 18: 
Line 19: exit $EXITVALUE
For another day, it would be nice to fix this bug. If the first operation failed and the
second succeeded, we lie and return 0.


-- 
To view, visit https://gerrit.ovirt.org/63682
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ica888244bd7c65121f55983e5716a6eae5662879
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Golembiovsky <tgolembi(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Fabian Deutsch <fabiand(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: Tomas Golembiovsky <tgolembi(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: vdsm: Rely on system for logrotation
......................................................................


Patch Set 3:

I am not 100% that this still applies, but please discuss
https://bugzilla.redhat.com/show_bug.cgi?id=1113264 and
https://gerrit.ovirt.org/#/c/29624/ in your commit message, so we are sure that we do not
reintroduce something that we used to have.

-- 
To view, visit https://gerrit.ovirt.org/63682
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ica888244bd7c65121f55983e5716a6eae5662879
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Golembiovsky <tgolembi(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Fabian Deutsch <fabiand(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: Tomas Golembiovsky <tgolembi(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

[Tomas Golembiovsky]

Tomas Golembiovsky has posted comments on this change.

Change subject: vdsm: Rely on system for logrotation
......................................................................


Patch Set 3: Code-Review-1

Thanks Dan. Somehow I have missed this change when browsing through related history.

If we really can generate more than 15 MB of logs per day and we want to run logrotate
hourly (as opposed to daily by the system) then it makes sense to stick with what we do
now. In this case we can abandon this change.

-- 
To view, visit https://gerrit.ovirt.org/63682
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ica888244bd7c65121f55983e5716a6eae5662879
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Golembiovsky <tgolembi(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Fabian Deutsch <fabiand(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: Tomas Golembiovsky <tgolembi(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: vdsm: Rely on system for logrotation
......................................................................


Patch Set 3:

Please send another patch documenting why we are not using logrotate in the standard way,
with url to the relevant bugs.

-- 
To view, visit https://gerrit.ovirt.org/63682
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ica888244bd7c65121f55983e5716a6eae5662879
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Golembiovsky <tgolembi(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Fabian Deutsch <fabiand(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: Tomas Golembiovsky <tgolembi(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Change in vdsm[master]: vdsm: document why we prefer running logrotate manually

gerrit-hooks has posted comments on this change.

Change subject: vdsm: document why we prefer running logrotate manually
......................................................................


Patch Set 4:

* Update Tracker::#1113264::IGNORE, not relevant for Red Hat classification
* Check Bug-Url::IGNORE, not relevant for branch: master
* Check Public Bug::#1113264::OK, public bug
* Check Product::IGNORE, not relevant for branch: master
* Check TM::IGNORE, not relevant for branch: master
* Check merged to previous::IGNORE,         Not in stable branch (['ovirt-3.6',
'ovirt-4.0'])

-- 
To view, visit https://gerrit.ovirt.org/63682
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ica888244bd7c65121f55983e5716a6eae5662879
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Golembiovsky <tgolembi(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Fabian Deutsch <fabiand(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: Tomas Golembiovsky <tgolembi(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Change in vdsm[master]: vdsm: document why we prefer running logrotate manually

[Tomas Golembiovsky]

Tomas Golembiovsky has posted comments on this change.

Change subject: vdsm: document why we prefer running logrotate manually
......................................................................


Patch Set 4:

CI failure unrelated:

    stderr: fatal: Unable to look up gerrit.ovirt.org (port 9418) (Name or service not
known)

-- 
To view, visit https://gerrit.ovirt.org/63682
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ica888244bd7c65121f55983e5716a6eae5662879
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Golembiovsky <tgolembi(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Fabian Deutsch <fabiand(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: Tomas Golembiovsky <tgolembi(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No