minor service_mgr facts fixes (#74894)

* minor service_mgr facts fixes

  handle case in which ps command fails or returns empty
  updated tests since it now does keep trying to detect after ps fails

3 files changed, 14 insertions, 11 deletions

diff --git a/changelogs/fragments/service_mgr_facts_fix.yml b/changelogs/fragments/service_mgr_facts_fix.yml
new file mode 100644
index 0000000000..c64e251635
--- /dev/null
+++ b/changelogs/fragments/service_mgr_facts_fix.yml
@@ -0,0 +1,2 @@
+bugfixes:
+- facts, service_mgr, handle issues if ps command fails or returns empty.
diff --git a/lib/ansible/module_utils/facts/system/service_mgr.py b/lib/ansible/module_utils/facts/system/service_mgr.py
index f0b9e0154c..1ca3cce59f 100644
--- a/lib/ansible/module_utils/facts/system/service_mgr.py
+++ b/lib/ansible/module_utils/facts/system/service_mgr.py
@@ -85,13 +85,11 @@ class ServiceMgrFactCollector(BaseFactCollector):
         # try various forms of querying pid 1
         proc_1 = get_file_content('/proc/1/comm')
         if proc_1 is None:
-            # FIXME: return code isnt checked
-            # FIXME: if stdout is empty string, odd things
-            # FIXME: other code seems to think we could get proc_1 == None past this point
             rc, proc_1, err = module.run_command("ps -p 1 -o comm|tail -n 1", use_unsafe_shell=True)
-            # If the output of the command starts with what looks like a PID, then the 'ps' command
-            # probably didn't work the way we wanted, probably because it's busybox
-            if re.match(r' *[0-9]+ ', proc_1):
+
+            # if command fails, or stdout is empty string or the output of the command starts with what looks like a PID,
+            # then the 'ps' command probably didn't work the way we wanted, probably because it's busybox
+            if rc != 0 or not proc_1.strip() or re.match(r' *[0-9]+ ', proc_1):
                 proc_1 = None
 
         # The ps command above may return "COMMAND" if the user cannot read /proc, e.g. with grsecurity
@@ -101,7 +99,6 @@ class ServiceMgrFactCollector(BaseFactCollector):
         if proc_1 is None and os.path.islink('/sbin/init'):
             proc_1 = os.readlink('/sbin/init')
 
-        # FIXME: empty string proc_1 staus empty string
         if proc_1 is not None:
             proc_1 = os.path.basename(proc_1)
             proc_1 = to_native(proc_1)
@@ -114,10 +111,8 @@ class ServiceMgrFactCollector(BaseFactCollector):
         # if not init/None it should be an identifiable or custom init, so we are done!
         if proc_1 is not None:
             # Lookup proc_1 value in map and use proc_1 value itself if no match
-            # FIXME: empty string still falls through
             service_mgr_name = proc_1_map.get(proc_1, proc_1)
 
-        # FIXME: replace with a system->service_mgr_name map?
         # start with the easy ones
         elif collected_facts.get('ansible_distribution', None) == 'MacOSX':
             # FIXME: find way to query executable, version matching is not ideal
diff --git a/test/units/module_utils/facts/test_collectors.py b/test/units/module_utils/facts/test_collectors.py
index ae25636bad..5492582bf5 100644
--- a/test/units/module_utils/facts/test_collectors.py
+++ b/test/units/module_utils/facts/test_collectors.py
@@ -366,7 +366,10 @@ class TestServiceMgrFacts(BaseFactsTest):
     # TODO: dedupe some of this test code
 
     @patch('ansible.module_utils.facts.system.service_mgr.get_file_content', return_value=None)
-    def test_no_proc1(self, mock_gfc):
+    @patch('ansible.module_utils.facts.system.service_mgr.ServiceMgrFactCollector.is_systemd_managed', return_value=False)
+    @patch('ansible.module_utils.facts.system.service_mgr.ServiceMgrFactCollector.is_systemd_managed_offline', return_value=False)
+    @patch('ansible.module_utils.facts.system.service_mgr.os.path.exists', return_value=False)
+    def test_service_mgr_runit(self, mock_gfc, mock_ism, mock_ismo, mock_ope):
         # no /proc/1/comm, ps returns non-0
         # should fallback to 'service'
         module = self._mock_module()
@@ -388,7 +391,10 @@ class TestServiceMgrFacts(BaseFactsTest):
         self.assertEqual(facts_dict['service_mgr'], 'sys11')
 
     @patch('ansible.module_utils.facts.system.service_mgr.get_file_content', return_value=None)
-    def test_clowncar(self, mock_gfc):
+    @patch('ansible.module_utils.facts.system.service_mgr.ServiceMgrFactCollector.is_systemd_managed', return_value=False)
+    @patch('ansible.module_utils.facts.system.service_mgr.ServiceMgrFactCollector.is_systemd_managed_offline', return_value=False)
+    @patch('ansible.module_utils.facts.system.service_mgr.os.path.exists', return_value=False)
+    def test_service_mgr_runit(self, mock_gfc, mock_ism, mock_ismo, mock_ope):
         # no /proc/1/comm, ps fails, distro and system are clowncar
         # should end up return 'sys11'
         module = self._mock_module()