Redesign OpenstackModule class

don't inherit OpenstackModule class from AnsibleModule class to
prevent occasional overriding Ansible methods or vars and failing
module.

Change-Id: Ic34fff0c938eb87cc0d2c5e98fbafed64bf349f6

ci/roles/server/tasks/main.yml

@@ -17,6 +17,12 @@
 - name: Get info about all servers
   openstack.cloud.server_info:
     cloud: "{{ cloud }}"
+  register: info
+
+- name: Check info about servers
+  assert:
+    that:
+      info.openstack_servers|length > 0
 
 - name: Delete server with meta as CSV
   openstack.cloud.server:
@@ -25,6 +31,16 @@
      name: "{{ server_name }}"
      wait: true
 
+- name: Get info about all servers
+  openstack.cloud.server_info:
+    cloud: "{{ cloud }}"
+  register: info
+
+- name: Check info about no servers
+  assert:
+    that:
+      info.openstack_servers|length == 0
+
 - name: Create server with meta as dict
   openstack.cloud.server:
      cloud: "{{ cloud }}"
@@ -46,6 +62,12 @@
   openstack.cloud.server_info:
     cloud: "{{ cloud }}"
     server: "{{ server_name }}"
+  register: info
+
+- name: Check info about server name
+  assert:
+    that:
+      info.openstack_servers[0].name == "{{ server_name }}"
 
 - name: Delete server with meta as dict
   openstack.cloud.server:
@@ -74,6 +96,12 @@
     cloud: "{{ cloud }}"
     server: "{{ server_name }}"
     detailed: true
+  register: info
+
+- name: Check info about server image name
+  assert:
+    that:
+      info.openstack_servers[0].image.name == "{{ image }}"
 
 - name: Delete server (FIP from pool/network)
   openstack.cloud.server:
@@ -99,11 +127,28 @@
 
 - debug: var=server
 
+- name: Get info about servers in all projects
+  openstack.cloud.server_info:
+    cloud: "{{ cloud }}"
+    all_projects: true
+  register: info
+
+- name: Check info about servers in all projects
+  assert:
+    that:
+      info.openstack_servers|length > 0
+
 - name: Get info about one server in all projects
   openstack.cloud.server_info:
     cloud: "{{ cloud }}"
     server: "{{ server_name }}"
     all_projects: true
+  register: info
+
+- name: Check info about one server in all projects
+  assert:
+    that:
+      info.openstack_servers|length > 0
 
 - name: Delete server with volume
   openstack.cloud.server:

plugins/module_utils/openstack.py

@@ -28,6 +28,7 @@
 # USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
 import abc
+import copy
 from distutils.version import StrictVersion
 import importlib
 import os
@@ -35,6 +36,37 @@ import os
 from ansible.module_utils.basic import AnsibleModule
 from ansible.module_utils.six import iteritems
 
+OVERRIDES = {'os_client_config': 'config',
+             'os_endpoint': 'catalog_endpoint',
+             'os_flavor': 'compute_flavor',
+             'os_flavor_info': 'compute_flavor_info',
+             'os_group': 'identity_group',
+             'os_group_info': 'identity_group_info',
+             'os_ironic': 'baremetal_node',
+             'os_ironic_inspect': 'baremetal_inspect',
+             'os_ironic_node': 'baremetal_node_action',
+             'os_keystone_domain': 'identity_domain',
+             'os_keystone_domain_info': 'identity_domain_info',
+             'os_keystone_endpoint': 'endpoint',
+             'os_keystone_identity_provider': 'federation_idp',
+             'os_keystone_identity_provider_info': 'federation_idp_info',
+             'os_keystone_mapping': 'federation_mapping',
+             'os_keystone_mapping_info': 'federation_mapping_info',
+             'os_keystone_role': 'identity_role',
+             'os_keystone_service': 'catalog_service',
+             'os_listener': 'lb_listener',
+             'os_member': 'lb_member',
+             'os_nova_flavor': 'compute_flavor',
+             'os_nova_host_aggregate': 'host_aggregate',
+             'os_pool': 'lb_pool',
+             'os_user': 'identity_user',
+             'os_user_group': 'group_assignment',
+             'os_user_info': 'identity_user_info',
+             'os_user_role': 'role_assignment',
+             'os_zone': 'dns_zone'}
+
+CUSTOM_VAR_PARAMS = ['min_ver', 'max_ver']
+
 
 def openstack_argument_spec():
     # DEPRECATED: This argument spec is only used for the deprecated old
@@ -97,7 +129,12 @@ def openstack_full_argument_spec(**kwargs):
             default='public', choices=['public', 'internal', 'admin'],
             aliases=['endpoint_type']),
     )
-    spec.update(kwargs)
+    # Filter out all our custom parameters before passing to AnsibleModule
+    kwargs_copy = copy.deepcopy(kwargs)
+    for v in kwargs_copy.values():
+        for c in CUSTOM_VAR_PARAMS:
+            v.pop(c, None)
+    spec.update(kwargs_copy)
     return spec
 
 
@@ -109,10 +146,10 @@ def openstack_module_kwargs(**kwargs):
                 ret[key].extend(kwargs[key])
             else:
                 ret[key] = kwargs[key]
-
     return ret
 
 
+# for compatibility with old versions
 def openstack_cloud_from_module(module, min_version='0.12.0'):
     try:
         # Due to the name shadowing we should import other way
@@ -166,25 +203,192 @@ def openstack_cloud_from_module(module, min_version='0.12.0'):
         module.fail_json(msg=str(e))
 
 
-class OpenStackModule(AnsibleModule):
+class OpenStackModule:
+    """Openstack Module is a base class for all Openstack Module classes.
 
+    The class has `run` function that should be overriden in child classes,
+    the provided methods include:
+
+    Methods:
+        params: Dictionary of Ansible module parameters.
+        module_name: Module name (i.e. server_action)
+        sdk_version: Version of used OpenstackSDK.
+        results: Dictionary for return of Ansible module,
+                 must include `changed` keyword.
+        exit, exit_json: Exit module and return data inside, must include
+                         changed` keyword in a data.
+        fail, fail_json: Exit module with failure, has `msg` keyword to
+                         specify a reason of failure.
+        conn: Connection to SDK object.
+        log: Print message to system log.
+        debug: Print debug message to system log, prints if Ansible Debug is
+               enabled or verbosity is more than 2.
+        check_deprecated_names: Function that checks if module was called with
+                                a deprecated name and prints the correct name
+                                with deprecation warning.
+        check_versioned: helper function to check that all arguments are known
+                         in the current SDK version.
+        run: method that executes and shall be overriden in inherited classes.
+
+    Args:
+        deprecated_names: Should specify deprecated modules names for current
+                          module.
+        argument_spec: Used for construction of Openstack common arguments.
+        module_kwargs: Additional arguments for Ansible Module.
+    """
+
+    deprecated_names = ()
     argument_spec = {}
     module_kwargs = {}
 
     def __init__(self):
+        """Initialize Openstack base class.
 
-        super(OpenStackModule, self).__init__(
+        Set up variables, connection to SDK and check if there are
+        deprecated names.
+        """
+        self.ansible = AnsibleModule(
             openstack_full_argument_spec(**self.argument_spec),
             **self.module_kwargs)
+        self.params = self.ansible.params
+        self.module_name = self.ansible._name
+        self.sdk_version = None
+        self.results = {'changed': False}
+        self.exit = self.exit_json = self.ansible.exit_json
+        self.fail = self.fail_json = self.ansible.fail_json
+        self.sdk, self.conn = self.openstack_cloud_from_module()
+        self.check_deprecated_names()
 
-        self.sdk, self.conn = openstack_cloud_from_module(self)
+    def log(self, msg):
+        """Prints log message to system log.
+
+        Arguments:
+            msg {str} -- Log message
+        """
+        self.ansible.log(msg)
+
+    def debug(self, msg):
+        """Prints debug message to system log
+
+        Arguments:
+            msg {str} -- Debug message.
+        """
+        if self.ansible._debug or self.ansible._verbosity > 2:
+            self.ansible.log(
+                " ".join(['[DEBUG]', msg]))
+
+    def check_deprecated_names(self):
+        """Check deprecated module names if `deprecated_names` variable is set.
+        """
+        new_module_name = OVERRIDES.get(self.module_name)
+        if self.module_name in self.deprecated_names and new_module_name:
+            self.ansible.deprecate(
+                "The '%s' module has been renamed to '%s' in openstack "
+                "collection: openstack.cloud.%s" % (
+                    self.module_name, new_module_name, new_module_name),
+                version='2.10')
+
+    def openstack_cloud_from_module(self):
+        """Sets up connection to cloud using provided options. Checks if all
+           provided variables are supported for the used SDK version.
+        """
+        try:
+            # Due to the name shadowing we should import other way
+            sdk = importlib.import_module('openstack')
+            sdk_version_lib = importlib.import_module('openstack.version')
+            self.sdk_version = sdk_version_lib.__version__
+        except ImportError:
+            self.fail_json(msg='openstacksdk is required for this module')
+
+        # Fail if there are set unsupported for this version parameters
+        # New parameters should NOT use 'default' but rely on SDK defaults
+        for param in self.argument_spec:
+            if (self.params[param] is not None
+                and 'min_ver' in self.argument_spec[param]
+                    and StrictVersion(self.sdk_version) < self.argument_spec[param]['min_ver']):
+                self.fail_json(
+                    msg="To use parameter '{param}' with module '{module}', the installed version of "
+                    "the openstacksdk library MUST be >={min_version}.".format(
+                        min_version=self.argument_spec[param]['min_ver'],
+                        param=param,
+                        module=self.module_name))
+            if (self.params[param] is not None
+                and 'max_ver' in self.argument_spec[param]
+                    and StrictVersion(self.sdk_version) > self.argument_spec[param]['max_ver']):
+                self.fail_json(
+                    msg="To use parameter '{param}' with module '{module}', the installed version of "
+                    "the openstacksdk library MUST be <={max_version}.".format(
+                        max_version=self.argument_spec[param]['max_ver'],
+                        param=param,
+                        module=self.module_name))
+
+        cloud_config = self.params.pop('cloud', None)
+        if isinstance(cloud_config, dict):
+            fail_message = (
+                "A cloud config dict was provided to the cloud parameter"
+                " but also a value was provided for {param}. If a cloud"
+                " config dict is provided, {param} should be"
+                " excluded.")
+            for param in (
+                    'auth', 'region_name', 'validate_certs',
+                    'ca_cert', 'client_key', 'api_timeout', 'auth_type'):
+                if self.params[param] is not None:
+                    self.fail_json(msg=fail_message.format(param=param))
+            # For 'interface' parameter, fail if we receive a non-default value
+            if self.params['interface'] != 'public':
+                self.fail_json(msg=fail_message.format(param='interface'))
+        else:
+            cloud_config = dict(
+                cloud=cloud_config,
+                auth_type=self.params['auth_type'],
+                auth=self.params['auth'],
+                region_name=self.params['region_name'],
+                verify=self.params['validate_certs'],
+                cacert=self.params['ca_cert'],
+                key=self.params['client_key'],
+                api_timeout=self.params['api_timeout'],
+                interface=self.params['interface'],
+            )
+        try:
+            return sdk, sdk.connect(**cloud_config)
+        except sdk.exceptions.SDKException as e:
+            # Probably a cloud configuration/login error
+            self.fail_json(msg=str(e))
+
+    # Filter out all arguments that are not from current SDK version
+    def check_versioned(self, **kwargs):
+        """Check that provided arguments are supported by current SDK version
+
+        Returns:
+            versioned_result {dict} dictionary of only arguments that are
+                                    supported by current SDK version. All others
+                                    are dropped.
+        """
+        versioned_result = {}
+        for var_name in kwargs:
+            if ('min_ver' in self.argument_spec[var_name]
+                    and StrictVersion(self.sdk_version) < self.argument_spec[var_name]['min_ver']):
+                continue
+            if ('max_ver' in self.argument_spec[var_name]
+                    and StrictVersion(self.sdk_version) > self.argument_spec[var_name]['max_ver']):
+                continue
+            versioned_result.update({var_name: kwargs[var_name]})
+        return versioned_result
 
     @abc.abstractmethod
     def run(self):
+        """Function for overriding in inhetired classes, it's executed by default.
+        """
         pass
 
     def __call__(self):
+        """Execute `run` function when calling the class.
+        """
         try:
-            self.run()
+            results = self.run()
+            if results and isinstance(results, dict):
+                self.ansible.exit_json(**results)
         except self.sdk.exceptions.OpenStackCloudException as e:
-            self.fail_json(msg=str(e), extra_data=e.extra_data)
+            self.ansible.fail_json(msg=str(e), extra_data=e.extra_data)
+        # if we got to this place, modules didn't exit
+        self.ansible.exit_json(**self.results)

plugins/modules/server.py

@@ -469,11 +469,11 @@ def _network_args(module, cloud):
     nics = module.params['nics']
 
     if not isinstance(nics, list):
-        module.fail_json(msg='The \'nics\' parameter must be a list.')
+        module.fail(msg='The \'nics\' parameter must be a list.')
 
     for num, net in enumerate(_parse_nics(nics)):
         if not isinstance(net, dict):
-            module.fail_json(
+            module.fail(
                 msg='Each entry in the \'nics\' parameter must be a dict.')
 
         if net.get('net-id'):
@@ -481,7 +481,7 @@ def _network_args(module, cloud):
         elif net.get('net-name'):
             by_name = cloud.get_network(net['net-name'])
             if not by_name:
-                module.fail_json(
+                module.fail(
                     msg='Could not find network by net-name: %s' %
                     net['net-name'])
             resolved_net = net.copy()
@@ -493,7 +493,7 @@ def _network_args(module, cloud):
         elif net.get('port-name'):
             by_name = cloud.get_port(net['port-name'])
             if not by_name:
-                module.fail_json(
+                module.fail(
                     msg='Could not find port by port-name: %s' %
                     net['port-name'])
             resolved_net = net.copy()
@@ -614,6 +614,7 @@ def _check_security_groups(module, cloud, server):
 
 
 class ServerModule(OpenStackModule):
+    deprecated_names = ('os_server', 'openstack.cloud.os_server')
 
     argument_spec = dict(
         name=dict(required=True),
@@ -658,6 +659,7 @@ class ServerModule(OpenStackModule):
     )
 
     def run(self):
+
         state = self.params['state']
         image = self.params['image']
         boot_volume = self.params['boot_volume']
@@ -666,12 +668,12 @@ class ServerModule(OpenStackModule):
 
         if state == 'present':
             if not (image or boot_volume):
-                self.fail_json(
+                self.fail(
                     msg="Parameter 'image' or 'boot_volume' is required "
                         "if state == 'present'"
                 )
             if not flavor and not flavor_ram:
-                self.fail_json(
+                self.fail(
                     msg="Parameter 'flavor' or 'flavor_ram' is required "
                         "if state == 'present'"
                 )
@@ -685,7 +687,7 @@ class ServerModule(OpenStackModule):
 
     def _exit_hostvars(self, server, changed=True):
         hostvars = self.conn.get_openstack_vars(server)
-        self.exit_json(
+        self.exit(
             changed=changed, server=server, id=server.id, openstack=hostvars)
 
     def _get_server_state(self):
@@ -693,7 +695,7 @@ class ServerModule(OpenStackModule):
         server = self.conn.get_server(self.params['name'])
         if server and state == 'present':
             if server.status not in ('ACTIVE', 'SHUTOFF', 'PAUSED', 'SUSPENDED'):
-                self.fail_json(
+                self.fail(
                     msg="The instance is available but not Active state: " + server.status)
             (ip_changed, server) = _check_ips(self, self.conn, server)
             (sg_changed, server) = _check_security_groups(self, self.conn, server)
@@ -702,7 +704,7 @@ class ServerModule(OpenStackModule):
         if server and state == 'absent':
             return True
         if state == 'absent':
-            self.exit_json(changed=False, result="not present")
+            self.exit(changed=False, result="not present")
         return True
 
     def _create_server(self):
@@ -715,23 +717,23 @@ class ServerModule(OpenStackModule):
             image_id = self.conn.get_image_id(
                 self.params['image'], self.params['image_exclude'])
             if not image_id:
-                self.fail_json(
+                self.fail(
                     msg="Could not find image %s" % self.params['image'])
 
         if flavor:
             flavor_dict = self.conn.get_flavor(flavor)
             if not flavor_dict:
-                self.fail_json(msg="Could not find flavor %s" % flavor)
+                self.fail(msg="Could not find flavor %s" % flavor)
         else:
             flavor_dict = self.conn.get_flavor_by_ram(flavor_ram, flavor_include)
             if not flavor_dict:
-                self.fail_json(msg="Could not find any matching flavor")
+                self.fail(msg="Could not find any matching flavor")
 
         nics = _network_args(self, self.conn)
 
         self.params['meta'] = _parse_meta(self.params['meta'])
 
-        bootkwargs = dict(
+        bootkwargs = self.check_versioned(
             name=self.params['name'],
             image=image_id,
             flavor=flavor_dict['id'],
@@ -788,8 +790,8 @@ class ServerModule(OpenStackModule):
                 timeout=self.params['timeout'],
                 delete_ips=self.params['delete_fip'])
         except Exception as e:
-            self.fail_json(msg="Error in deleting vm: %s" % e.message)
-        self.exit_json(changed=True, result='deleted')
+            self.fail(msg="Error in deleting vm: %s" % e)
+        self.exit(changed=True, result='deleted')
 
 
 def main():

plugins/modules/server_info.py

@@ -10,33 +10,33 @@ short_description: Retrieve information about one or more compute instances
 author: Monty (@emonty)
 description:
     - Retrieve information about server instances from OpenStack.
-    - This module was called C(openstack.cloud.server_facts) before Ansible 2.9, returning C(ansible_facts).
+    - This module was called C(os_server_facts) before Ansible 2.9, returning C(ansible_facts).
       Note that the M(openstack.cloud.server_info) module no longer returns C(ansible_facts)!
 notes:
     - The result contains a list of servers.
 options:
-   server:
-     description:
-       - restrict results to servers with names or UUID matching
-         this glob expression (e.g., <web*>).
-     type: str
-   detailed:
-     description:
-        - when true, return additional detail about servers at the expense
-          of additional API calls.
-     type: bool
-     default: 'no'
-   filters:
-     description:
-        - restrict results to servers matching a dictionary of
-          filters
-     type: dict
-   all_projects:
-     description:
-       - Whether to list servers from all projects or just the current auth
-         scoped project.
-     type: bool
-     default: 'no'
+  server:
+    description:
+      - restrict results to servers with names or UUID matching
+        this glob expression (e.g., <web*>).
+    type: str
+  detailed:
+    description:
+      - when true, return additional detail about servers at the expense
+        of additional API calls.
+    type: bool
+    default: 'no'
+  filters:
+    description:
+      - restrict results to servers matching a dictionary of
+        filters
+    type: dict
+  all_projects:
+    description:
+      - Whether to list servers from all projects or just the current auth
+        scoped project.
+    type: bool
+    default: 'no'
 requirements:
     - "python >= 3.6"
     - "openstacksdk"
@@ -57,13 +57,13 @@ EXAMPLES = '''
     msg: "{{ result.openstack_servers }}"
 '''
 
-import fnmatch
-
 from ansible_collections.openstack.cloud.plugins.module_utils.openstack import OpenStackModule
 
 
 class ServerInfoModule(OpenStackModule):
 
+    deprecated_names = ('os_server_info', 'openstack.cloud.os_server_info')
+
     argument_spec = dict(
         server=dict(required=False),
         detailed=dict(required=False, type='bool', default=False),
@@ -72,26 +72,16 @@ class ServerInfoModule(OpenStackModule):
     )
 
     def run(self):
-        is_old_facts = self._name == 'openstack.cloud.server_facts'
-        if is_old_facts:
-            self.deprecate("The 'openstack.cloud.server_facts' module has been renamed to 'openstack.cloud.server_info', "
-                           "and the renamed one no longer returns ansible_facts", version='2.13')
-        openstack_servers = self.conn.search_servers(
-            detailed=self.params['detailed'], filters=self.params['filters'],
-            all_projects=self.params['all_projects'])
 
+        kwargs = self.check_versioned(
+            detailed=self.params['detailed'],
+            filters=self.params['filters'],
+            all_projects=self.params['all_projects']
+        )
         if self.params['server']:
-            # filter servers by name
-            pattern = self.params['server']
-            # TODO(mordred) This is handled by sdk now
-            openstack_servers = [server for server in openstack_servers
-                                 if fnmatch.fnmatch(server['name'], pattern)
-                                 or fnmatch.fnmatch(server['id'], pattern)]
-        if is_old_facts:
-            self.exit_json(changed=False, ansible_facts=dict(
-                openstack_servers=openstack_servers))
-        else:
-            self.exit_json(changed=False, openstack_servers=openstack_servers)
+            kwargs['name_or_id'] = self.params['server']
+        openstack_servers = self.conn.search_servers(**kwargs)
+        self.exit(changed=False, openstack_servers=openstack_servers)
 
 
 def main():

tests/sanity/ignore-2.10.txt

@@ -5,5 +5,5 @@ plugins/modules/image_info.py pylint:invalid-tagged-version
 plugins/modules/networks_info.py pylint:invalid-tagged-version
 plugins/modules/port_info.py pylint:invalid-tagged-version
 plugins/modules/project_info.py pylint:invalid-tagged-version
-plugins/modules/server_info.py pylint:invalid-tagged-version
+plugins/module_utils/openstack.py pylint:invalid-tagged-version
 plugins/modules/subnets_info.py pylint:invalid-tagged-version