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 <tgole...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczew...@gmail.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
