pids - refactor module to make version-based behavior consistent (#3315)

* Initial commit

* Adding changelog fragment

* Further refactoring

* Fixing bad copy/paste and adding task for psutil >= 5.7.0 install

* Inverting psutil installation order to reduce duplication

* Optimizing regex compilation

changelogs/fragments/3315-pids-refactor.yml

@@ -0,0 +1,4 @@
+---
+minor_changes:
+  - pids - refactor to add support for older ``psutil`` versions to the ``pattern`` option
+    (https://github.com/ansible-collections/community.general/pull/3315).

plugins/modules/system/pids.py

@@ -54,9 +54,12 @@ pids:
   sample: [100,200]
 '''
 
+import abc
 import re
+from distutils.version import LooseVersion
 from os.path import basename
 
+from ansible.module_utils import six
 from ansible.module_utils.basic import AnsibleModule, missing_required_lib
 from ansible.module_utils.common.text.converters import to_native
 
@@ -68,6 +71,100 @@ except ImportError:
     HAS_PSUTIL = False
 
 
+class PSAdapterError(Exception):
+    pass
+
+
+@six.add_metaclass(abc.ABCMeta)
+class PSAdapter(object):
+    NAME_ATTRS = ('name', 'cmdline')
+    PATTERN_ATTRS = ('name', 'exe', 'cmdline')
+
+    def __init__(self, psutil):
+        self._psutil = psutil
+
+    @staticmethod
+    def from_package(psutil):
+        version = LooseVersion(psutil.__version__)
+        if version < LooseVersion('2.0.0'):
+            return PSAdapter100(psutil)
+        elif version < LooseVersion('5.3.0'):
+            return PSAdapter200(psutil)
+        else:
+            return PSAdapter530(psutil)
+
+    def get_pids_by_name(self, name):
+        return [p.pid for p in self._process_iter(*self.NAME_ATTRS) if self._has_name(p, name)]
+
+    def _process_iter(self, *attrs):
+        return self._psutil.process_iter()
+
+    def _has_name(self, proc, name):
+        attributes = self._get_proc_attributes(proc, *self.NAME_ATTRS)
+        return (compare_lower(attributes['name'], name) or
+                attributes['cmdline'] and compare_lower(attributes['cmdline'][0], name))
+
+    def _get_proc_attributes(self, proc, *attributes):
+        return dict((attribute, self._get_attribute_from_proc(proc, attribute)) for attribute in attributes)
+
+    @staticmethod
+    @abc.abstractmethod
+    def _get_attribute_from_proc(proc, attribute):
+        pass
+
+    def get_pids_by_pattern(self, pattern, ignore_case):
+        flags = 0
+        if ignore_case:
+            flags |= re.I
+
+        try:
+            regex = re.compile(pattern, flags)
+        except re.error as e:
+            raise PSAdapterError("'%s' is not a valid regular expression: %s" % (pattern, to_native(e)))
+
+        return [p.pid for p in self._process_iter(*self.PATTERN_ATTRS) if self._matches_regex(p, regex)]
+
+    def _matches_regex(self, proc, regex):
+        # See https://psutil.readthedocs.io/en/latest/#find-process-by-name for more information
+        attributes = self._get_proc_attributes(proc, *self.PATTERN_ATTRS)
+        matches_name = regex.search(to_native(attributes['name']))
+        matches_exe = attributes['exe'] and regex.search(basename(to_native(attributes['exe'])))
+        matches_cmd = attributes['cmdline'] and regex.search(to_native(' '.join(attributes['cmdline'])))
+
+        return any([matches_name, matches_exe, matches_cmd])
+
+
+class PSAdapter100(PSAdapter):
+    def __init__(self, psutil):
+        super(PSAdapter100, self).__init__(psutil)
+
+    @staticmethod
+    def _get_attribute_from_proc(proc, attribute):
+        return getattr(proc, attribute)
+
+
+class PSAdapter200(PSAdapter):
+    def __init__(self, psutil):
+        super(PSAdapter200, self).__init__(psutil)
+
+    @staticmethod
+    def _get_attribute_from_proc(proc, attribute):
+        method = getattr(proc, attribute)
+        return method()
+
+
+class PSAdapter530(PSAdapter):
+    def __init__(self, psutil):
+        super(PSAdapter530, self).__init__(psutil)
+
+    def _process_iter(self, *attrs):
+        return self._psutil.process_iter(attrs=attrs)
+
+    @staticmethod
+    def _get_attribute_from_proc(proc, attribute):
+        return proc.info[attribute]
+
+
 def compare_lower(a, b):
     if a is None or b is None:
         # this could just be "return False" but would lead to surprising behavior if both a and b are None
@@ -76,38 +173,36 @@ def compare_lower(a, b):
     return a.lower() == b.lower()
 
 
-def get_pid(name):
+class Pids(object):
-    pids = []
+    def __init__(self, module):
+        if not HAS_PSUTIL:
+            module.fail_json(msg=missing_required_lib('psutil'))
 
-    try:
+        self._ps = PSAdapter.from_package(psutil)
-        for proc in psutil.process_iter(attrs=['name', 'cmdline']):
-            if compare_lower(proc.info['name'], name) or \
-                    proc.info['cmdline'] and compare_lower(proc.info['cmdline'][0], name):
-                pids.append(proc.pid)
-    except TypeError:  # EL6, EL7: process_iter() takes no arguments (1 given)
-        for proc in psutil.process_iter():
-            try:  # EL7
-                proc_name, proc_cmdline = proc.name(), proc.cmdline()
-            except TypeError:  # EL6: 'str' object is not callable
-                proc_name, proc_cmdline = proc.name, proc.cmdline
-            if compare_lower(proc_name, name) or \
-                    proc_cmdline and compare_lower(proc_cmdline[0], name):
-                pids.append(proc.pid)
-    return pids
 
+        self._module = module
+        self._name = module.params['name']
+        self._pattern = module.params['pattern']
+        self._ignore_case = module.params['ignore_case']
 
-def get_matching_command_pids(pattern, ignore_case):
+        self._pids = []
-    flags = 0
-    if ignore_case:
-        flags |= re.I
 
-    regex = re.compile(pattern, flags)
+    def execute(self):
-    # See https://psutil.readthedocs.io/en/latest/#find-process-by-name for more information
+        if self._name:
-    return [p.pid for p in psutil.process_iter(["name", "exe", "cmdline"])
+            self._pids = self._ps.get_pids_by_name(self._name)
-            if regex.search(to_native(p.info["name"]))
+        else:
-            or (p.info["exe"] and regex.search(basename(to_native(p.info["exe"]))))
+            try:
-            or (p.info["cmdline"] and regex.search(to_native(' '.join(p.cmdline()))))
+                self._pids = self._ps.get_pids_by_pattern(self._pattern, self._ignore_case)
-            ]
+            except PSAdapterError as e:
+                self._module.fail_json(msg=to_native(e))
+
+        return self._module.exit_json(**self.result)
+
+    @property
+    def result(self):
+        return {
+            'pids': self._pids,
+        }
 
 
 def main():
@@ -126,22 +221,7 @@ def main():
         supports_check_mode=True,
     )
 
-    if not HAS_PSUTIL:
+    Pids(module).execute()
-        module.fail_json(msg=missing_required_lib('psutil'))
-
-    name = module.params["name"]
-    pattern = module.params["pattern"]
-    ignore_case = module.params["ignore_case"]
-
-    if name:
-        response = dict(pids=get_pid(name))
-    else:
-        try:
-            response = dict(pids=get_matching_command_pids(pattern, ignore_case))
-        except re.error as e:
-            module.fail_json(msg="'%s' is not a valid regular expression: %s" % (pattern, to_native(e)))
-
-    module.exit_json(**response)
 
 
 if __name__ == '__main__':

tests/integration/targets/pids/tasks/main.yml

@@ -6,10 +6,16 @@
 # Test code for the pids module
 # Copyright: (c) 2019, Saranya Sridharan
 # GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt)
-- name: "Installing the psutil module"
+- name: Attempt installation of latest 'psutil' version
+  pip:
+    name: psutil
+  ignore_errors: true
+  register: psutil_latest_install
+
+- name: Install greatest 'psutil' version which will work with all pip versions
   pip:
     name: psutil < 5.7.0
-    # Version 5.7.0 breaks on older pip versions. See https://github.com/ansible/ansible/pull/70667
+  when: psutil_latest_install is failed
 
 - name: "Checking the empty result"
   pids: