Initial commits for integration of HPE OneView resources with Ansible #26026

fgbulsoni · 2017-06-22T20:29:29Z

SUMMARY

HPE OneView is an infrastructure automation engine built with software intelligence. It streamlines provisioning and lifecycle management across compute, storage and fabric and enables IT staff to control resources programmatically through a unified API.

It is heavily focused on the DevOps workflow and has many integrations already, including an ansible module not integrated into core.

This PR aims to add the OneView base class to be used for all OneView resources, together with the module for managing the HPE OneView FC Network resource: FcNetworkModule and its unit tests.

This resource has the present and absent states which will manage all of its aspects, including creating and updating the resources (using present) and removal of resources (using absent).

ISSUE TYPE

New Module Pull Request

COMPONENT NAME

FcNetworkModule

ANSIBLE VERSION

ansible 2.4.0 (devel de4a8b83df) last updated 2017/06/22 18:57:00 (GMT +000)
  config file = None
  configured module search path = ['/home/vagrant/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /home/vagrant/temp/ansible/lib/ansible
  executable location = /home/vagrant/temp/ansible/bin/ansible
  python version = 3.5.3 (default, Jun 22 2017, 17:14:20) [GCC 4.8.4]

ADDITIONAL INFORMATION

Example of usage:

    - name: Create a Fibre Channel Network
      oneview_fc_network:
        config: "{{ config }}"
        state: present
        validate_etag: false
        data:
          name: 'Test FC Network'
          fabricType: 'FabricAttach'
          linkStabilityTime: '30'
          autoLoginRedistribution: True
      delegate_to: localhost
      register: fc_network_1

    - name: Update the Fibre Channel Network changing the attribute autoLoginRedistribution to True
      oneview_fc_network:
        config: "{{ config }}"
        state: present
        data:
          name: 'Test FC Network'
          autoLoginRedistribution: True
          fabricType: 'FabricAttach'
          linkStabilityTime: '30'
      delegate_to: localhost

    - name: Delete the Fibre Channel Network
      oneview_fc_network:
        config: "{{ config }}"
        state: absent
        data: "{{ fc_network_1.ansible_facts.fc_network }}"
      delegate_to: localhost
      register: deleted

mattclay · 2017-06-22T20:53:02Z

@fgbulsoni Have you considered mocking hpOneView instead of importing it? That way we're only testing the module, rather than its dependencies.

Here's another PR which is doing that: #25450

ansibot · 2017-06-22T21:09:39Z

The test ansible-test compile --python 2.6 failed with the following error:

lib/ansible/module_utils/oneview.py:524:61: SyntaxError: existing_connection_map = {x[SPKeys.ID]: x.copy() for x in resource[SPKeys.CONNECTIONS]}

The test ansible-test sanity --test shebang failed with the following error:

Command "test/sanity/code-smell/shebang.sh" returned exit status 1.
>>> Standard Output
./lib/ansible/module_utils/oneview.py:#!/usr/bin/python
./lib/ansible/utils/module_docs_fragments/oneview.py:#!/usr/bin/python
./test/units/modules/cloud/hpe/oneview_module_loader.py:#!/usr/bin/python
./test/units/modules/cloud/hpe/hpe_test_utils.py:#!/usr/bin/python
./test/units/modules/cloud/hpe/test_oneview_fc_network_facts.py:#!/usr/bin/python
./test/units/modules/cloud/hpe/test_oneview_fc_network.py:#!/usr/bin/python
One or more file(s) listed above have an unexpected shebang.
See test/sanity/code-smell/shebang.sh for the list of acceptable values.

click here for bot help

fgbulsoni · 2017-06-23T12:50:51Z

Have you considered mocking hpOneView instead of importing it?

Hey @mattclay , thanks for the feedback! I'll look into the PR mentioned and see if I can do the same.

ansibot · 2017-06-26T17:54:58Z

The test ansible-test sanity --test shebang failed with the following error:

Command "test/sanity/code-smell/shebang.sh" returned exit status 1.
>>> Standard Output
./lib/ansible/module_utils/oneview.py:#!/usr/bin/python
./lib/ansible/utils/module_docs_fragments/oneview.py:#!/usr/bin/python
./test/units/modules/cloud/hpe/oneview_module_loader.py:#!/usr/bin/python
./test/units/modules/cloud/hpe/hpe_test_utils.py:#!/usr/bin/python
./test/units/modules/cloud/hpe/test_oneview_fc_network_facts.py:#!/usr/bin/python
./test/units/modules/cloud/hpe/test_oneview_fc_network.py:#!/usr/bin/python
One or more file(s) listed above have an unexpected shebang.
See test/sanity/code-smell/shebang.sh for the list of acceptable values.

click here for bot help

ansibot · 2017-06-26T18:12:47Z

cc @robinro
click here for bot help

robinro · 2017-06-26T19:04:28Z

I don't see how this relates to me.

fgbulsoni · 2017-06-26T19:41:25Z

I don't see how this relates to me.

Hey @robinro , yeah, not sure what made the bot cc you. Thank you for replying though!

@mattclay , I've added mocks to the hpOneView SDK and now it seems we're hitting an error related to the collections method OrderedDict which we call inside the oneview module_utils file.

Our module does make use of the OrderedDict functionality, but I'm not sure about copying the entire method from collections and mocking it for it to work on Python 2.6, as it will be quite big and I don't feel this will add to the tests. I've also seen that OrderedDict is used by some Junos modules but it is not mocked for their tests, so I'm not entirely sure how to deal with this.

Would you have any suggestion on how to deal with this? Perhaps a way to skip the 2.6 tests since the module will not be compatible with that version?

ansibot · 2017-06-28T14:57:35Z

click here for bot help

fgbulsoni · 2017-06-28T15:04:09Z

I've fixed the unit tests issues regarding OrderedDict and the unit tests for 2.6 should be passing now.

It seems that for some reason I can't pinpoint, the other tests are failing on this command pip install --disable-pip-version-check -c test/runner/requirements/constraints.txt -r test/runner/requirements/sanity.txt coverage junit-xml.

From the logs and running it locally, it appears to fail when trying to install yamllint with this output:

Collecting yamllint
/usr/local/lib/python2.6/site-packages/pip/_vendor/requests/packages/urllib3/util/ssl_.py:318: SNIMissingWarning: An HTTPS request has been made, but the SNI (Subject Name Indication) extension to TLS is not available on this platform. This may cause the server to present an incorrect TLS certificate, which can cause validation failures. You can upgrade to a newer version of Python to solve this. For more information, see https://urllib3.readthedocs.io/en/latest/security.html#snimissingwarning.
  SNIMissingWarning
/usr/local/lib/python2.6/site-packages/pip/_vendor/requests/packages/urllib3/util/ssl_.py:122: InsecurePlatformWarning: A true SSLContext object is not available. This prevents urllib3 from configuring SSL appropriately and may cause certain SSL connections to fail. You can upgrade to a newer version of Python to solve this. For more information, see https://urllib3.readthedocs.io/en/latest/security.html#insecureplatformwarning.
  InsecurePlatformWarning
  Using cached yamllint-1.8.0-py2.py3-none-any.whl
Collecting pathspec (from yamllint)
  Using cached pathspec-0.5.2.tar.gz
    Complete output from command python setup.py egg_info:
    Traceback (most recent call last):
      File "<string>", line 1, in <module>
      File "/tmp/pip-build-omI9Qg/pathspec/setup.py", line 6, in <module>
        from pathspec import __author__, __email__, __license__, __project__, __version__
      File "pathspec/__init__.py", line 33, in <module>
        from .pathspec import PathSpec
      File "pathspec/pathspec.py", line 9, in <module>
        from . import util
      File "pathspec/util.py", line 167
        return {normalize_file(path, separators=separators): path for path in files}
                                                                    ^
    SyntaxError: invalid syntax

I've not added yamllint nor am I sure why is it failing, this looks to me like code that was already in the repo. Any help/ideas here?

mattclay · 2017-06-28T15:09:38Z

@fgbulsoni I'm working on a fix now.

mattclay · 2017-06-28T15:51:19Z

@fgbulsoni The issue has been resolved. I've restarted CI for this PR.

ansibot · 2017-06-28T17:08:10Z

The test ansible-test sanity --test pep8 failed with the following errors:

contrib/inventory/zabbix.py:27:71: W291 trailing whitespace
contrib/inventory/zabbix.py:107:1: E101 indentation contains mixed spaces and tabs
contrib/inventory/zabbix.py:107:1: W191 indentation contains tabs
contrib/inventory/zabbix.py:108:1: E101 indentation contains mixed spaces and tabs
contrib/inventory/zabbix.py:108:26: E201 whitespace after '{'
contrib/inventory/zabbix.py:108:48: E202 whitespace before '}'
contrib/inventory/zabbix.py:114:26: E221 multiple spaces before operator
contrib/inventory/zabbix.py:115:27: E221 multiple spaces before operator
contrib/inventory/zabbix.py:118:18: E221 multiple spaces before operator

click here for bot help

mattclay · 2017-06-28T17:13:33Z

These failures are also unrelated. They've been fixed and I'll restart CI again.

fgbulsoni · 2017-06-28T18:17:25Z

These failures are also unrelated. They've been fixed and I'll restart CI again.

Many thanks for that @mattclay !

Is there anything else that needs to be addressed in this PR or may I wait for it to be merged?

Best regards,
Felipe.

fgbulsoni · 2017-08-02T16:35:18Z

@abadger I believe I've made all of the requested changes.
Would you perhaps be able to review the PR again?

abadger · 2017-08-02T16:42:42Z

lib/ansible/module_utils/oneview.py

+    MSG_ALREADY_PRESENT = 'Resource is already present.'
+    MSG_ALREADY_ABSENT = 'Resource is already absent.'
+    HPE_ONEVIEW_SDK_REQUIRED = 'HPE OneView Python SDK is required for this module.'
+    FUTURE_PACKAGE_REQUIRED = 'The Future Python package is required for this module.'


We don't need this message anymore as the future package is no longer used.

Ops. True, removing.

abadger · 2017-08-02T17:13:16Z

lib/ansible/module_utils/oneview.py

+
+from ansible.module_utils import six
+from ansible.module_utils.basic import AnsibleModule
+from ansible.module_utils._text import to_native


style wise, the ansible imports go below the hpOneView imports.

abadger · 2017-08-02T17:14:15Z

lib/ansible/module_utils/oneview.py

+import abc
+import collections
+import json
+import logging


We don't need the logging import anymore

abadger · 2017-08-02T17:16:26Z

lib/ansible/module_utils/oneview.py

+
+
+class OneViewModuleBase(object):
+    __metaclass__ = abc.ABCMeta


Oh sorry, I should have mentioned, how to specify metaclasses changed between python2 and python3. So there's a helper in six so that you can do this acroos both versions.

from ansible.module_utils.six import with_metaclass [...] class OneViewModuleBase(with_metaclass(ABCMeta, object)):

Is it fine to do it as:

@six.add_metaclass(abc.ABCMeta) class OneViewModuleBase(object): MSG_CREATED = 'Resource created successfully.' MSG_UPDATED = 'Resource updated successfully.'

?
Or would that not work? I've seen it used similarly at the crypto module.

abadger · 2017-08-02T17:19:34Z

lib/ansible/modules/remote_management/hpe/oneview_fc_network.py

+        else:
+            return self.resource_absent(resource)
+
+    def __present(self, resource):


There's still a few of these that are using double-underscore to mark private. We generally do that with single underscore instead.

- def __present(self, resource): + def _present(self, resource):

Ops. Missed that, was a new method that I added and ended up not verifying the underscores. Fixing.

abadger · 2017-08-02T17:23:02Z

lib/ansible/module_utils/oneview.py

+            # If both values are null, empty or False it will be considered equal.
+            elif not resource1[key] and not resource2[key]:
+                continue
+            elif isinstance(resource1[key], dict):


Use collections.Mapping for dict

…meta class, removed extra underscores on methods and swapped isinstance validations of dict for collections.Mapping

abadger · 2017-08-02T17:54:10Z

lib/ansible/module_utils/oneview.py

+import json
+import os
+import traceback
+from copy import deepcopy


No longer need deepcopy

abadger · 2017-08-02T18:00:55Z

lib/ansible/module_utils/oneview.py

+
+        :return: dict: It must return a dictionary with the attributes for the module result,
+            such as ansible_facts, msg and changed.
+"""


indentation of the end of the string should match with the beginning (I see this in a few other places as well)

abadger · 2017-08-02T18:02:31Z

lib/ansible/module_utils/oneview.py

+            ansible_facts={fact_name: resource}
+        )
+
+    MSG_DIFF_AT_KEY = 'Difference found at key \'{0}\'. '


Should this be at the top with the other class attributes?

abadger · 2017-08-02T18:03:04Z

lib/ansible/module_utils/oneview.py

+                List with all the scope URIs to be added to the resource.
+        Returns:
+            A dictionary with the expected arguments for the AnsibleModule.exit_json
+    """


sphinx docstring formatting was missed for this one

abadger · 2017-08-02T18:08:51Z

lib/ansible/module_utils/oneview.py

+                merged_items[item_key] = items_map[item_key].copy()
+                merged_items[item_key].update(item)
+            else:
+                merged_items[item_key] = item.copy()


I see that we're still doing copies of the item here twice, once when we enter it into items map at the beginning of hte function and once when we enter it into merged_items here. Is that needed?

…methods from merge_lists and moved MSG that was in the middle of the code to top of the class

abadger · 2017-08-02T20:14:14Z

lib/ansible/module_utils/oneview.py

+
+        return str(value)
+
+    def merge_list_by_key(self, original_list, updated_list, key, ignore_when_null=[]):


Looks like merge_list_by_key(), _standardize_value(), and _str_sorted() should be moved to be toplevel functions? (They don't make use of self).

abadger · 2017-08-02T20:17:41Z

lib/ansible/utils/module_docs_fragments/oneview.py

+# 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, see <http://www.gnu.org/licenses/>.


If you want to, you can change this license header to the short, one line version as shown here: https://github.com/ansible/ansible/blob/devel/docs/docsite/rst/dev_guide/developing_modules_documenting.rst#copyright

I've been trying to move modules to that since modules are copied over the network to the remote machine and so making them smaller can speed that up.

Ok, so, in that link I see this short licence:

#!/usr/bin/python # Copyright (c) 2017 [New Contributor(s)] # Copyright (c) 2015 [Original Contributor(s)] # GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt)

I'm assuming the oneview_fc_network.py will then have:

#!/usr/bin/python # Copyright (c) 2016-2017 Hewlett Packard Enterprise Development LP # GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt)

Correct? Also, since that licence is GNU and module_utils must be BSD, is there a similar model for BSD?

…cense header in oneview_fc_network.py to short version

abadger · 2017-08-02T21:04:14Z

Your gplv3 question looks correct. There is none for bsd at this time so you can leave that one alone.

fgbulsoni · 2017-08-02T21:05:06Z

Your gplv3 question looks correct. There is none for bsd at this time so you can leave that one alone.

Awesome, thanks. Changes made then, waiting on CI to complete to see if I messed up anywhere.

abadger · 2017-08-03T02:55:27Z

Merged to devel for 2.4.0. Thanks for the persistence and hard work!

fgbulsoni · 2017-08-24T17:57:24Z

For history purposes, related issue: #28354

s-hertel removed the needs_triage Needs a first human triage before being processed. label Jun 23, 2017

fgbulsoni force-pushed the hpe-oneview/fc-network branch from c464e0b to c0236ca Compare June 26, 2017 16:47

fgbulsoni force-pushed the hpe-oneview/fc-network branch from abd45fc to fb9701b Compare June 26, 2017 18:20

fgbulsoni force-pushed the hpe-oneview/fc-network branch 2 times, most recently from bc7a3ef to 969f94d Compare June 28, 2017 14:13

ansibot added community_review In order to be merged, this PR must follow the community review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jun 28, 2017

fgbulsoni force-pushed the hpe-oneview/fc-network branch from 41cd56f to a6cdde8 Compare August 2, 2017 15:07

abadger reviewed Aug 2, 2017

View reviewed changes

Removed future required message since future is no longer used

a5e98bd

abadger reviewed Aug 2, 2017

View reviewed changes

fixed imports order, removed logging import, changed style of add abc…

80a76c9

…meta class, removed extra underscores on methods and swapped isinstance validations of dict for collections.Mapping

abadger reviewed Aug 2, 2017

View reviewed changes

Fixed docstrings and docstrings identation, removed a couple of copy …

c8d3fdc

…methods from merge_lists and moved MSG that was in the middle of the code to top of the class

ansibot added community_review In order to be merged, this PR must follow the community review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Aug 2, 2017

Changed condition of not <reference> to is None according to review.

e36e64b

abadger reviewed Aug 2, 2017

View reviewed changes

Moving toplevel functions out of OneViewBaseClass and changing the li…

09142f4

…cense header in oneview_fc_network.py to short version

abadger merged commit b060d0c into ansible:devel Aug 3, 2017

fgbulsoni mentioned this pull request Nov 28, 2017

DatacenterModule for HPE OneView #32798

Closed

ansible locked and limited conversation to collaborators Apr 26, 2019


		return str(value)

		def merge_list_by_key(self, original_list, updated_list, key, ignore_when_null=[]):

Initial commits for integration of HPE OneView resources with Ansible #26026

Initial commits for integration of HPE OneView resources with Ansible #26026

Conversation

fgbulsoni commented Jun 22, 2017 • edited

SUMMARY

ISSUE TYPE

COMPONENT NAME

ANSIBLE VERSION

ADDITIONAL INFORMATION

mattclay commented Jun 22, 2017

ansibot commented Jun 22, 2017

fgbulsoni commented Jun 23, 2017

ansibot commented Jun 26, 2017

ansibot commented Jun 26, 2017

robinro commented Jun 26, 2017 • edited by mattclay

fgbulsoni commented Jun 26, 2017 • edited

ansibot commented Jun 28, 2017

fgbulsoni commented Jun 28, 2017 • edited

mattclay commented Jun 28, 2017

mattclay commented Jun 28, 2017

ansibot commented Jun 28, 2017

mattclay commented Jun 28, 2017

fgbulsoni commented Jun 28, 2017

fgbulsoni commented Aug 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abadger commented Aug 2, 2017 via email

fgbulsoni commented Aug 2, 2017

abadger commented Aug 3, 2017

fgbulsoni commented Aug 24, 2017

fgbulsoni commented Jun 22, 2017 •

edited

robinro commented Jun 26, 2017 •

edited by mattclay

fgbulsoni commented Jun 26, 2017 •

edited

fgbulsoni commented Jun 28, 2017 •

edited