[Plugin] Add default journal collection for Plugin services

Adds automatic collection of journals for any service defined in a
plugin's `services` tuple, if that service exists on the system.

Updates several plugins to define a `services` tuple in place of
manually calling `add_journal()` (and as a bonus, removing redundant
`add_service_status()` calls that are already performed based on the
tuple).

Additionally, add an appropriate tag to each `add_journal` call for
easier collection lookup in the manifest.

Resolves: #2579

Signed-off-by: Jake Hunsaker <jhunsake@redhat.com>

17 files changed, 64 insertions, 29 deletions

diff --git a/sos/report/plugins/__init__.py b/sos/report/plugins/__init__.py
index d713da99..ebecde2c 100644
--- a/sos/report/plugins/__init__.py
+++ b/sos/report/plugins/__init__.py
@@ -2397,7 +2397,7 @@ class Plugin(object):
     def add_journal(self, units=None, boot=None, since=None, until=None,
                     lines=None, allfields=False, output=None,
                     timeout=None, identifier=None, catalog=None,
-                    sizelimit=None, pred=None, tags=[], priority=10):
+                    sizelimit=None, pred=None, tags=None, priority=10):
         """Collect journald logs from one of more units.
 
         :param units:   Which journald units to collect
@@ -2454,9 +2454,15 @@ class Plugin(object):
         if isinstance(units, str):
             units = [units]
 
+        if isinstance(tags, str):
+            tags = [tags]
+        elif not tags:
+            tags = []
+
         if units:
             for unit in units:
                 journal_cmd += unit_opt % unit
+                tags.append("journal_%s" % unit)
 
         if identifier:
             journal_cmd += identifier_opt % identifier
@@ -2692,6 +2698,7 @@ class Plugin(object):
         for service in self.services:
             if self.is_service(service):
                 self.add_service_status(service)
+                self.add_journal(service)
 
     def setup(self):
         """Collect the list of files declared by the plugin. This method
diff --git a/sos/report/plugins/ceph.py b/sos/report/plugins/ceph.py
index fa1fdbe1..39b47a69 100644
--- a/sos/report/plugins/ceph.py
+++ b/sos/report/plugins/ceph.py
@@ -125,9 +125,6 @@ class Ceph(Plugin, RedHatPlugin, UbuntuPlugin):
             "ceph %s --format json-pretty" % s for s in ceph_cmds
         ], subdir="json_output", tags="insights_ceph_health_detail")
 
-        for service in self.services:
-            self.add_journal(units=service)
-
         self.add_forbidden_path([
             "/etc/ceph/*keyring*",
             "/var/lib/ceph/*keyring*",
diff --git a/sos/report/plugins/clear_containers.py b/sos/report/plugins/clear_containers.py
index 8fa44c92..e337a760 100644
--- a/sos/report/plugins/clear_containers.py
+++ b/sos/report/plugins/clear_containers.py
@@ -20,6 +20,7 @@ class ClearContainers(Plugin, IndependentPlugin):
 
     runtime = 'cc-runtime'
     packages = (runtime,)
+    services = ('cc-proxy',)
 
     def attach_cc_config_files(self):
 
@@ -75,7 +76,6 @@ class ClearContainers(Plugin, IndependentPlugin):
         self.attach_cc_config_files()
 
         self.attach_cc_log_files()
-        self.add_journal(units="cc-proxy")
         self.add_journal(identifier="cc-shim")
 
 # vim: set et ts=4 sw=4 :
diff --git a/sos/report/plugins/cockpit.py b/sos/report/plugins/cockpit.py
index ea450eff..f6b68a70 100644
--- a/sos/report/plugins/cockpit.py
+++ b/sos/report/plugins/cockpit.py
@@ -17,6 +17,7 @@ class Cockpit(Plugin, IndependentPlugin):
 
     plugin_name = 'cockpit'
     packages = ('cockpit-ws', 'cockpit-system')
+    services = ('cockpit',)
 
     def setup(self):
         self.add_copy_spec([
@@ -26,6 +27,4 @@ class Cockpit(Plugin, IndependentPlugin):
 
         self.add_cmd_output('remotectl certificate')
 
-        self.add_journal(units='cockpit')
-
 # vim: set et ts=4 sw=4 :
diff --git a/sos/report/plugins/crio.py b/sos/report/plugins/crio.py
index 38ac0d35..c256404b 100644
--- a/sos/report/plugins/crio.py
+++ b/sos/report/plugins/crio.py
@@ -17,6 +17,7 @@ class CRIO(Plugin, RedHatPlugin, UbuntuPlugin):
     plugin_name = 'crio'
     profiles = ('container',)
     packages = ('cri-o', 'cri-tools')
+    services = ('crio',)
 
     option_list = [
         ("all", "enable capture for all containers, even containers "
@@ -43,7 +44,6 @@ class CRIO(Plugin, RedHatPlugin, UbuntuPlugin):
             'ALL_PROXY'
         ])
 
-        self.add_journal(units="crio")
         self.add_cmd_output([
             "ls -alhR /etc/cni",
             "crio config"
diff --git a/sos/report/plugins/cups.py b/sos/report/plugins/cups.py
index ab7b6b70..c606836d 100644
--- a/sos/report/plugins/cups.py
+++ b/sos/report/plugins/cups.py
@@ -15,7 +15,7 @@ class Cups(Plugin, IndependentPlugin):
 
     plugin_name = 'cups'
     profiles = ('hardware',)
-
+    services = ('cups', 'cups-browsed')
     packages = ('cups',)
 
     def setup(self):
@@ -39,7 +39,4 @@ class Cups(Plugin, IndependentPlugin):
             "lpstat -d"
         ])
 
-        self.add_journal(units="cups")
-        self.add_journal(units="cups-browsed")
-
 # vim: set et ts=4 sw=4 :
diff --git a/sos/report/plugins/etcd.py b/sos/report/plugins/etcd.py
index 2f25cd9c..bfc81fef 100644
--- a/sos/report/plugins/etcd.py
+++ b/sos/report/plugins/etcd.py
@@ -21,6 +21,7 @@ class etcd(Plugin, RedHatPlugin):
     packages = ('etcd',)
     profiles = ('container', 'system', 'services', 'cluster')
     files = ('/etc/origin/node/pods/etcd.yaml',)
+    services = ('etcd',)
 
     def setup(self):
         if path.exists('/etc/origin/node/pods/etcd.yaml'):
@@ -59,7 +60,6 @@ class etcd(Plugin, RedHatPlugin):
             self.add_cmd_output(['curl -s %s%s' % (etcd_url, u) for u in urls])
 
         self.add_cmd_output("ls -lR /var/lib/etcd/")
-        self.add_journal(units="etcd")
 
     def get_etcd_url(self):
         try:
diff --git a/sos/report/plugins/fwupd.py b/sos/report/plugins/fwupd.py
index 2f019192..f5c1b588 100644
--- a/sos/report/plugins/fwupd.py
+++ b/sos/report/plugins/fwupd.py
@@ -15,7 +15,7 @@ class Fwupd(Plugin, IndependentPlugin):
 
     plugin_name = 'fwupd'
     profiles = ('system', )
-
+    services = ('fwupd',)
     packages = ('fwupd',)
 
     def setup(self):
@@ -31,8 +31,6 @@ class Fwupd(Plugin, IndependentPlugin):
 
         self.add_copy_spec("/etc/fwupd")
 
-        self.add_journal(units="fwupd")
-
     def postproc(self):
         self.do_path_regex_sub(
             "/etc/fwupd/remotes.d/*",
diff --git a/sos/report/plugins/gdm.py b/sos/report/plugins/gdm.py
index 7fe3705c..09a8930a 100644
--- a/sos/report/plugins/gdm.py
+++ b/sos/report/plugins/gdm.py
@@ -16,13 +16,12 @@ class Gdm(Plugin, IndependentPlugin):
     plugin_name = 'gdm'
     profiles = ('desktop',)
     packages = ('gdm', 'gdm3',)
+    services = ('gdm',)
 
     def setup(self):
         self.add_copy_spec([
             "/etc/gdm/*",
             "/etc/gdm3/*"
         ])
-        self.add_journal(units="gdm")
-        self.add_service_status("gdm")
 
 # vim: set et ts=4 sw=4 :
diff --git a/sos/report/plugins/lightdm.py b/sos/report/plugins/lightdm.py
index a2816cdd..ac4d8ca8 100644
--- a/sos/report/plugins/lightdm.py
+++ b/sos/report/plugins/lightdm.py
@@ -16,11 +16,10 @@ class LightDm(Plugin, IndependentPlugin):
     short_desc = 'Light Display Manager'
     packages = ('lightdm', )
     profiles = ('desktop', )
+    services = ('lightdm', )
     plugin_name = 'lightdm'
 
     def setup(self):
-        self.add_service_status("lightdm")
-        self.add_journal(units="lightdm")
         self.add_copy_spec([
             "/etc/lightdm/lightdm.conf",
             "/etc/lightdm/users.conf"
diff --git a/sos/report/plugins/maas.py b/sos/report/plugins/maas.py
index d992c1ab..f4e3662e 100644
--- a/sos/report/plugins/maas.py
+++ b/sos/report/plugins/maas.py
@@ -99,9 +99,6 @@ class Maas(Plugin, UbuntuPlugin):
                 "apt-cache policy python-django-*",
             ])
 
-        for service in self.services:
-            self.add_journal(units=service)
-
         if self.is_installed("maas-region-controller"):
             self.add_cmd_output([
                 "maas-region dumpdata",
diff --git a/sos/report/plugins/peripety.py b/sos/report/plugins/peripety.py
index 80d56e81..501256e1 100644
--- a/sos/report/plugins/peripety.py
+++ b/sos/report/plugins/peripety.py
@@ -38,6 +38,5 @@ class Peripety(Plugin, RedHatPlugin):
                 "prpt info %s" % disk,
                 "prpt query --blk %s" % disk
             ])
-        self.add_journal('peripetyd')
 
 # vim: set et ts=4 sw=4 :
diff --git a/sos/report/plugins/rasdaemon.py b/sos/report/plugins/rasdaemon.py
index afba02df..aab87e16 100644
--- a/sos/report/plugins/rasdaemon.py
+++ b/sos/report/plugins/rasdaemon.py
@@ -30,6 +30,5 @@ class Rasdaemon(Plugin, RedHatPlugin):
             '--summary'
         ]
         self.add_cmd_output(["ras-mc-ctl %s" % sub for sub in subcmds])
-        self.add_journal('rasdaemon')
 
 # vim: set et ts=4 sw=4 :
diff --git a/sos/report/plugins/snap.py b/sos/report/plugins/snap.py
index a3623c57..1ad219cb 100644
--- a/sos/report/plugins/snap.py
+++ b/sos/report/plugins/snap.py
@@ -17,6 +17,7 @@ class Snap(Plugin, IndependentPlugin):
     plugin_name = 'snap'
     profiles = ('system', 'sysmgmt', 'packagemanager')
     packages = ('snapd',)
+    services = ('snapd',)
 
     def setup(self):
         self.add_cmd_output("snap list --all", root_symlink="installed-snaps")
@@ -25,7 +26,5 @@ class Snap(Plugin, IndependentPlugin):
             "snap changes"
         ])
         self.add_cmd_output("snap debug connectivity", timeout=10)
-        self.add_service_status("snapd")
-        self.add_journal(units="snapd")
 
 # vim: set et ts=4 sw=4 :
diff --git a/sos/report/plugins/stratis.py b/sos/report/plugins/stratis.py
index 3e565f95..6897baea 100644
--- a/sos/report/plugins/stratis.py
+++ b/sos/report/plugins/stratis.py
@@ -32,6 +32,5 @@ class Stratis(Plugin, RedHatPlugin):
         ]
 
         self.add_cmd_output(["stratis %s" % subcmd for subcmd in subcmds])
-        self.add_journal(units='stratisd')
 
 # vim: set et ts=4 sw=4 :
diff --git a/tests/report_tests/plugin_tests/defaults.py b/tests/report_tests/plugin_tests/defaults.py
new file mode 100644
index 00000000..4cdaabd4
--- /dev/null
+++ b/tests/report_tests/plugin_tests/defaults.py
@@ -0,0 +1,42 @@
+# This file is part of the sos project: https://github.com/sosreport/sos
+#
+# This copyrighted material is made available to anyone wishing to use,
+# modify, copy, or redistribute it subject to the terms and conditions of
+# version 2 of the GNU General Public License.
+#
+# See the LICENSE file in the source distribution for further information.
+
+from sos_tests import StageTwoReportTest
+
+
+class DefaultCollectionsTest(StageTwoReportTest):
+    """Ensure that the default collections are firing for Plugins based on
+    their enablement triggers, which gives us more atomicity in Plugin design
+
+    :avocado: tags=stagetwo
+    """
+
+    packages = {'rhel': 'cups',
+                'Ubuntu': 'cups'}
+
+    sos_cmd = '-o cups'
+
+    def test_service_status_collected(self):
+        self.assertFileCollected('sos_commands/cups/systemctl_status_cups')
+        _m = self.get_plugin_manifest('cups')
+        ent = None
+        for cmd in _m['commands']:
+            if cmd['exec'] == 'systemctl status cups':
+                ent = cmd
+        assert ent, "No manifest entry for systemctl status cups"
+
+    def test_journal_collected(self):
+        self.assertFileCollected('sos_commands/cups/journalctl_--no-pager_--unit_cups')
+        _m = self.get_plugin_manifest('cups')
+        ent = None
+        for cmd in _m['commands']:
+            if cmd['exec'] == 'journalctl --no-pager  --unit cups':
+                ent = cmd
+        assert ent, "No manifest entry for journalctl cups"
+
+        assert 'journal_cups' in ent['tags'], "Journal tags not correct: %s" % ent['tags']
diff --git a/tests/sos_tests.py b/tests/sos_tests.py
index 5df94794..8c8e6d05 100644
--- a/tests/sos_tests.py
+++ b/tests/sos_tests.py
@@ -651,6 +651,10 @@ class StageTwoReportTest(BaseSoSReportTest):
         self.installer = software_manager
         self.sm = self.installer.SoftwareManager()
 
+        for dist in self.packages:
+            if isinstance(self.packages[dist], str):
+                self.packages[dist] = [self.packages[dist]]
+
         keys = self.packages.keys()
         # allow for single declaration of packages for the RH family
         # for our purposes centos == rhel here
@@ -716,7 +720,7 @@ class StageTwoReportTest(BaseSoSReportTest):
                 return
             installed = self.installer.install_distro_packages(self.packages)
             if not installed:
-                raise("Unable to install requested packages %"
+                raise("Unable to install requested packages %s"
                       % ', '.join(pkg for pkg in self.packages[self.local_distro]))
             # save installed package list to our tmpdir to be removed later
             self._write_file_to_tmpdir('mocked_packages', json.dumps(self.packages[self.local_distro]))