Yeela Kaplan has uploaded a new change for review.
Change subject: [WIP]ABRT integration ......................................................................
[WIP]ABRT integration
Change-Id: I1ca5e66c9f029be75483b86414e328d074c7e454 Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=917062 Signed-off-by: Yeela Kaplan ykaplan@redhat.com --- M init/systemd/vdsmd.service.in M init/vdsmd_init_common.sh.in M lib/vdsm/tool/configurators/Makefile.am A lib/vdsm/tool/configurators/abrt.py M vdsm.spec.in M vdsm/vdsm-logrotate 6 files changed, 138 insertions(+), 28 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/87/44287/1
diff --git a/init/systemd/vdsmd.service.in b/init/systemd/vdsmd.service.in index d7d025f..ccacb2e 100644 --- a/init/systemd/vdsmd.service.in +++ b/init/systemd/vdsmd.service.in @@ -2,9 +2,10 @@ Description=Virtual Desktop Server Manager Requires=multipathd.service libvirtd.service time-sync.target \ iscsid.service rpcbind.service supervdsmd.service sanlock.service \ - vdsm-network.service + vdsm-network.service abrtd.service After=multipathd.service libvirtd.service iscsid.service rpcbind.service \ - supervdsmd.service sanlock.service vdsm-network.service + supervdsmd.service sanlock.service vdsm-network.service \ + abrtd.service Conflicts=libvirt-guests.service Wants=mom-vdsm.service
diff --git a/init/vdsmd_init_common.sh.in b/init/vdsmd_init_common.sh.in index 00e4054..df2878c 100644 --- a/init/vdsmd_init_common.sh.in +++ b/init/vdsmd_init_common.sh.in @@ -83,16 +83,6 @@ return 0 }
-task_configure_coredump() { - local conf_file="@CONFDIR@/vdsm.conf" - local getconfitem="@VDSMDIR@/get-conf-item" - - if "${getconfitem}" "${conf_file}" vars core_dump_enable false | - tr A-Z a-z | grep -q true; then - echo "/var/log/core/core.%p.%t.dump" > /proc/sys/kernel/core_pattern - fi -} - task_configure_vdsm_logs() { local vdsm_logs=" @VDSMLOGDIR@/connectivity.log @@ -296,7 +286,6 @@ --pre-start) run_tasks " \ mkdirs \ - configure_coredump \ configure_vdsm_logs \ wait_for_network \ run_init_hooks \ diff --git a/lib/vdsm/tool/configurators/Makefile.am b/lib/vdsm/tool/configurators/Makefile.am index 9fc7e57..f70b4bb 100644 --- a/lib/vdsm/tool/configurators/Makefile.am +++ b/lib/vdsm/tool/configurators/Makefile.am @@ -21,6 +21,7 @@
dist_configurators_PYTHON = \ __init__.py \ + abrt.py \ certificates.py \ libvirt.py \ multipath.py \ diff --git a/lib/vdsm/tool/configurators/abrt.py b/lib/vdsm/tool/configurators/abrt.py new file mode 100644 index 0000000..778f505 --- /dev/null +++ b/lib/vdsm/tool/configurators/abrt.py @@ -0,0 +1,132 @@ +# Copyright 2015 Red Hat, Inc. +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA +# +# Refer to the README and COPYING files for full details of the license +# + +import sys +import os + +from .import \ + MAYBE, \ + NO +from .. import service +from ... import utils +from ... import constants + +from .. import conf_utils + +CONF_VERSION = '4.17.0' + + +services = ("abrtd",) + + +def configure(): + + conf_utils.remove_conf(FILES, CONF_VERSION) + + for conf_file, content in FILES.items(): + content['configure'](content, CONF_VERSION) + + +def isconfigured(): + """ + Check if abrt is already configured for vdsm + """ + ret = MAYBE + + for path in (conf_utils.get_persisted_files(FILES)): + if not conf_utils.open_config(path, CONF_VERSION).hasConf(): + ret = NO + + if ret == MAYBE: + sys.stdout.write("abrt is already configured for vdsm\n") + else: + sys.stdout.write("abrt is not configured for vdsm\n") + return ret + + +FILES = { + 'ABRT_CONF': { + 'path': os.path.join( + constants.SYSCONF_PATH, + 'abrt/abrt.conf' + ), + 'configure': conf_utils.add_section, + 'removeConf': conf_utils.remove_section, + 'persisted': True, + 'fragments': [ + { + 'conditions': {}, + 'content': { + 'DumpLocation': '/var/tmp/abrt' + }, + }, + ] + }, + 'CCPP_CONF': { + 'path': os.path.join( + constants.SYSCONF_PATH, + 'abrt/plugins/CCpp.conf' + ), + 'configure': conf_utils.add_section, + 'removeConf': conf_utils.remove_section, + 'persisted': True, + 'fragments': [ + { + 'conditions': {}, + 'content': { + 'Backtrace': 'yes' + }, + }, + ] + }, + 'PYTHON_CONF': { + 'path': os.path.join( + constants.SYSCONF_PATH, + 'abrt/plugins/python.conf' + ), + 'configure': conf_utils.add_section, + 'removeConf': conf_utils.remove_section, + 'persisted': True, + 'fragments': [ + { + 'conditions': {}, + 'content': { + 'Backtrace': 'yes' + }, + }, + ] + }, + 'PKG_CONF': { + 'path': os.path.join( + constants.SYSCONF_PATH, + 'abrt/abrt-action-save-package-data.conf' + ), + 'configure': conf_utils.add_section, + 'removeConf': conf_utils.remove_section, + 'persisted': True, + 'fragments': [ + { + 'conditions': {}, + 'content': { + 'openGPGCheck': 'no' + }, + }, + ] + } +} diff --git a/vdsm.spec.in b/vdsm.spec.in index 21e1b50..6e524c5 100644 --- a/vdsm.spec.in +++ b/vdsm.spec.in @@ -132,6 +132,7 @@ Requires: nfs-utils Requires: m2crypto Requires: libnl3 +Requires: abrt-desktop Requires: curl Requires: %{name}-xmlrpc = %{version}-%{release} Requires: %{name}-jsonrpc = %{version}-%{release} @@ -709,13 +710,6 @@ /etc/sysctl.conf fi
-# hack until we replace core dump with abrt -if /usr/sbin/selinuxenabled; then - /usr/sbin/semanage fcontext -a -t virt_cache_t '/var/log/core(/.*)?' -fi -/sbin/restorecon -R /var/log/core >/dev/null 2>&1 -# hack until we replace core dump with abrt - # VDSM installs vdsm-modules-load.d.conf file - the following command will # refresh vdsm kernel modules requirements to start on boot /bin/systemctl restart systemd-modules-load.service >/dev/null 2>&1 || : @@ -1097,6 +1091,7 @@ %{python_sitelib}/%{vdsm_name}/tool/conf_utils.py* %{python_sitelib}/%{vdsm_name}/tool/configurator.py* %{python_sitelib}/%{vdsm_name}/tool/configurators/__init__* +%{python_sitelib}/%{vdsm_name}/tool/configurators/abrt.py* %{python_sitelib}/%{vdsm_name}/tool/configurators/certificates.py* %{python_sitelib}/%{vdsm_name}/tool/configurators/libvirt.py* %{python_sitelib}/%{vdsm_name}/tool/configurators/passwd.py* diff --git a/vdsm/vdsm-logrotate b/vdsm/vdsm-logrotate index e2027ac..17d26ab 100755 --- a/vdsm/vdsm-logrotate +++ b/vdsm/vdsm-logrotate @@ -7,12 +7,4 @@ /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=$? - if [ $EXITVALUE != 0 ]; then - /usr/bin/logger -t logrotate "ALERT clean old core files exited abnormally with [$EXITVALUE]" - fi -fi - exit $EXITVALUE
automation@ovirt.org has posted comments on this change.
Change subject: [WIP]ABRT integration ......................................................................
Patch Set 1:
* Update tracker::#917062::OK * Check Bug-Url::OK * Check Public Bug::#917062::OK, public bug * Check Product::#917062::OK, Correct product Red Hat Enterprise Virtualization Manager * Check TR::SKIP, not in a monitored branch (ovirt-3.5 ovirt-3.4 ovirt-3.3 ovirt-3.2) * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
From Yaniv Bronhaim ybronhei@redhat.com:
Yaniv Bronhaim has restored this change.
Change subject: [WIP]ABRT integration ......................................................................
Restored
rechecking that
From Yaniv Bronhaim ybronhei@redhat.com:
Yaniv Bronhaim has posted comments on this change.
Change subject: (wip) Adding abrt dependency and introduce configurator for it ......................................................................
Patch Set 4:
(2 comments)
https://gerrit.ovirt.org/#/c/44287/4/vdsm.spec.in File vdsm.spec.in:
Line 109: Requires: rpm-python Line 110: Requires: nfs-utils Line 111: Requires: m2crypto Line 112: Requires: libnl3 Line 113: Requires: abrt-desktop
We definitely don't want to add abrt-gui and gnome-abrt as VDSM dependency,
Done Line 114: Requires: curl Line 115: Requires: %{name}-xmlrpc = %{version}-%{release} Line 116: Requires: %{name}-jsonrpc = %{version}-%{release} Line 117: Requires: safelease >= 1.0-7
https://gerrit.ovirt.org/#/c/44287/4/vdsm/vdsm-logrotate File vdsm/vdsm-logrotate:
Line 24 Line 25 Line 26 Line 27 Line 28
Shouldn't we rotate also abrt files or abrt does that automatically? If so,
we will need to see while its running if its needed. im not sure yet if it overrides reports
From Yaniv Bronhaim ybronhei@redhat.com:
Yaniv Bronhaim has posted comments on this change.
Change subject: (wip) Adding abrt dependency and introduce configurator for it ......................................................................
Patch Set 5:
(1 comment)
https://gerrit.ovirt.org/#/c/44287/5/lib/vdsm/tool/configurators/abrt.py File lib/vdsm/tool/configurators/abrt.py:
Line 62: 'fragments': [ Line 63: { Line 64: 'conditions': {}, Line 65: 'content': { Line 66: 'DumpLocation': '/var/tmp/abrt'
it's not included, it should be added
yes its part of the action items of the feature to add abrt output to sos plugins. see https://trello.com/c/aRx6a6UN/6-bug-917062-rfe-add-abrt-integration Line 67: }, Line 68: }, Line 69: ] Line 70: },
From Yaniv Bronhaim ybronhei@redhat.com:
Yaniv Bronhaim has posted comments on this change.
Change subject: Adding abrt dependency and introduce configurator for it ......................................................................
Patch Set 13: Verified+1
From Yaniv Bronhaim ybronhei@redhat.com:
Yaniv Bronhaim has posted comments on this change.
Change subject: Adding abrt dependency and introduce configurator for it ......................................................................
Patch Set 15:
(3 comments)
thanks for the review! please continue. replied inline
https://gerrit.ovirt.org/#/c/44287/15/lib/vdsm/tool/configurators/abrt.py File lib/vdsm/tool/configurators/abrt.py:
PS15, Line 27: '4.20.0'
why do we want to have it hard coded here?
this is a way to recognize conf updates. its specific for each configuration module that uses this confutil thing. its written in https://github.com/oVirt/vdsm/blob/master/lib/vdsm/tool/configurators/libvir...
do you think I should copy this comment here as well?
PS15, Line 38: Always true
so why do we need it?
we don't. I'll remove
PS15, Line 47: MAYBE
Is it possible to determine validity of the configuration or we do not care
we don't have something specific to validate and we don't want to say that its not configured at all. with this check we just check that there is vdsm section in the conf file. if not, it means that we never ran configure. validate should say if a configuration attribute is wrong in some wsy - like the ssl conflict in libvirt. with abrt I don't see anything like that for now
From Yaniv Bronhaim ybronhei@redhat.com:
Yaniv Bronhaim has posted comments on this change.
Change subject: Adding abrt dependency and introduce configurator for it ......................................................................
Patch Set 16: Verified+1
From Dan Kenigsberg danken@redhat.com:
Dan Kenigsberg has submitted this change and it was merged.
Change subject: Adding abrt dependency and introduce configurator for it ......................................................................
Adding abrt dependency and introduce configurator for it
- Adding spec requirement for abrt addons to replace current coredump kernel reports (see RFE for more info)
- Adding abrt conf files
- Removing logrotate config for coredump folder - assuming that abrt takes care for not flooding in reports (need to be verified)
- Adding tests
Change-Id: I1ca5e66c9f029be75483b86414e328d074c7e454 Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=917062 Signed-off-by: Yeela Kaplan ykaplan@redhat.com Signed-off-by: Yaniv Bronhaim ybronhei@redhat.com --- M init/systemd/85-vdsmd.preset M init/vdsmd_init_common.sh.in M lib/vdsm/tool/configurators/Makefile.am A lib/vdsm/tool/configurators/abrt.py M static/usr/lib/systemd/system/vdsmd.service.in M tests/Makefile.am A tests/toolTests_CCPP.conf A tests/toolTests_abrt-action-save-package-data.conf A tests/toolTests_abrt.conf A tests/toolTests_vmcore.conf M tests/tool_test.py M vdsm.spec.in M vdsm/vdsm-logrotate 13 files changed, 272 insertions(+), 68 deletions(-)
Approvals: Piotr Kliczewski: Looks good to me, approved Yaniv Bronhaim: Verified Jenkins CI: Passed CI tests Irit Goihman: Looks good to me, but someone else must approve
vdsm-patches@lists.fedorahosted.org
-
automation＠ovirt.org -
Code Review -
ykaplan＠redhat.com