New module: management of the Nuage Networks VSP SDN solution (network/nuage/nuage_vspk) #24895

pdellaert · 2017-05-22T14:57:15Z

SUMMARY

Adding a new module to manage the Nuage Networks VSP SDN solution through its Python SDK.

ISSUE TYPE

New Module Pull Request

COMPONENT NAME

nuage_vspk

ANSIBLE VERSION

ansible 2.3.0.0
  config file =
  configured module search path = Default w/o overrides
  python version = 2.7.13 (default, Dec 17 2016, 23:03:43) [GCC 4.2.1 Compatible Apple LLVM 8.0.0 (clang-800.0.42.1)]

gundalow · 2017-05-24T11:23:56Z

lib/ansible/modules/network/nuage/nuage_vspk.py

+            - Requires a I(api_version) parameter (example v4_0).
+        required: true
+        default: null
+        choices: []


Please delete all occurrences of choices: [], that's the default, so not needed.

This comment applies to a number of places in this module.

gundalow · 2017-05-24T11:24:07Z

lib/ansible/modules/network/nuage/nuage_vspk.py

+        required: true
+        default: null
+        choices: []
+        aliases: []


Please delete all occurrences of aliases: [], that's the default, so not needed.

This comment applies to a number of places in this module.

gundalow · 2017-05-24T11:24:57Z

lib/ansible/modules/network/nuage/nuage_vspk.py

+        default: null
+        choices: []
+        aliases: []
+        version_added: "1.0"


The only version_added should be at the top level, the rest can be removed.

This comment applies to a number of places in this module.

This is the Ansible version that the option was changed.

Please see https://docs.ansible.com/ansible/dev_guide/developing_modules_documenting.html for more information

gundalow · 2017-05-24T11:26:51Z

lib/ansible/modules/network/nuage/nuage_vspk.py

+        except BambouHTTPError as error:
+            self.module.fail_json(msg='Unable to connect to the API URL with given username, password and enterprise: {0}'.format(error))
+
+    def _verify_api(self):


If there are going to be a number of modules that need this logic, it may be good to move this into common code module_utils see https://docs.ansible.com/ansible/dev_guide/developing_modules_in_groups.html for more info.

This is something we are planning once we move to more modules, we'll split the general functionalities (not only this, but building the connection as well) into a module_util. Currently this is not needed yet, i think.

Sure, that sounds sensible.

gundalow · 2017-05-26T10:04:19Z

lib/ansible/modules/network/nuage/nuage_vspk.py

+    auth:
+        description:
+            - Dict with the authentication information required to connect to a Nuage VSP environment.
+            - Requires a I(api_username) parameter (example csproot).


If some options are required together this can be enforced in the argspec,

See the following draft documentation for examples on this:
https://github.com/gundalow/ansible/blob/docs-argspec/docs/docsite/rst/dev_guide/developing_modules_general.rst#main-and-ansiblemodule-argument-spec

Done, thanks for the tip!

One remark, the doc stipulates the use of () brackets, however, that caused issues in some cases, i've used [] everywhere.

Sub options are apparently not available yet in 2.3, as such, i've added them, according to the code in module_utils/basic.py

I believe I may have made a mistake in my documentation regarding () vs [], my apologies for any confusion caused, it's not been reviewed yet. I'll make sure that gets updated.

New modules go into devel which will become the Ansible 2.4 release. What's the comment regarding 2.3 regarding?

gundalow · 2017-05-26T10:09:30Z

lib/ansible/modules/network/nuage/nuage_vspk.py

+            if fetcher is None:
+                self.module.fail_json(msg='No parent specified and root object is not a parent for the type')
+
+        # Verifying state or command is set:


I think most if not all of this can be removed and replaced with argspec code

https://github.com/gundalow/ansible/blob/docs-argspec/docs/docsite/rst/dev_guide/developing_modules_general.rst#main-and-ansiblemodule-argument-spec

gundalow · 2017-05-26T10:10:08Z

lib/ansible/modules/network/nuage/nuage_vspk.py

+            match_filter=dict(default=None, required=False, type='str'),
+            properties=dict(default=None, required=False, type='dict'),
+            children=dict(default=None, required=False, type='list')
+        ),


https://github.com/gundalow/ansible/blob/docs-argspec/docs/docsite/rst/dev_guide/developing_modules_general.rst#main-and-ansiblemodule-argument-spec

ganeshrn · 2017-05-26T11:21:50Z

lib/ansible/modules/network/nuage/nuage_vspk.py

+            self.properties = module.params['properties']
+
+        self.children = None
+        if 'children' in list(module.params.keys()):


Since children, properties and other's are defined as module args, the key will be present in params and initialized to is a default value. Hence the if check here and above is not required.
self.children = module.params['children'] is sufficient

pdellaert · 2017-05-26T15:43:54Z

@gundalow , @ganeshrn, thank you for the review, comments and especially tips!

gundalow · 2017-05-26T18:57:27Z

lib/ansible/modules/network/nuage/nuage_vspk.py

+    auth:
+        description:
+            - Dict with the authentication information required to connect to a Nuage VSP environment.
+            - Requires a I(api_username) parameter (example csproot).


I believe I may have made a mistake in my documentation regarding () vs [], my apologies for any confusion caused, it's not been reviewed yet. I'll make sure that gets updated.

New modules go into devel which will become the Ansible 2.4 release. What's the comment regarding 2.3 regarding?

gundalow · 2017-05-26T19:02:19Z

lib/ansible/modules/network/nuage/nuage_vspk.py

+            auth=dict(
+                required=True,
+                type='dict',
+                no_log=True,


Now you are using subboptionsn you only need no_log on password, & api key.

@gundalow the comment on 2.3 was that i was testing with 2.3. Which makes me realise that i should be testing directly with the devel branch instead.

I'll have a look and also change the no log to just the password and key (potentially certificate as well, it's not that crucial, but they are stored typically together and both point to files)

@gundalow while doing negative tests, i ran into an issue in the current module_utils/basic.py code, namely:

ansible/lib/ansible/module_utils/basic.py

Line 1739 in 757758c

value = self.params[k]

value = self.params[k]

This will fail on when doing the check of the sub options, with the following traceback (cause: it is trying to find the sub option in the main params instead of the sub params):

Traceback (most recent call last): File "/var/folders/7t/j48vp3pd2fzbz30yr8xjxpr00000gq/T/ansible_yoy2Ti/ansible_module_nuage_vspk.py", line 1052, in <module> main() File "/var/folders/7t/j48vp3pd2fzbz30yr8xjxpr00000gq/T/ansible_yoy2Ti/ansible_module_nuage_vspk.py", line 1041, in main supports_check_mode=True File "/var/folders/7t/j48vp3pd2fzbz30yr8xjxpr00000gq/T/ansible_yoy2Ti/ansible_modlib.zip/ansible/module_utils/basic.py", line 759, in __init__ File "/var/folders/7t/j48vp3pd2fzbz30yr8xjxpr00000gq/T/ansible_yoy2Ti/ansible_modlib.zip/ansible/module_utils/basic.py", line 1759, in _check_argument_types File "/var/folders/7t/j48vp3pd2fzbz30yr8xjxpr00000gq/T/ansible_yoy2Ti/ansible_modlib.zip/ansible/module_utils/basic.py", line 1739, in _check_argument_types KeyError: 'api_enterprise'

The code should be:

value = param[k]

(In case you are interested, my negative test cases can be found here: https://github.com/nuagenetworks/vspk-ansible/blob/beccbf3269a3dd5163422814506e9a4b5b244240/roles/nuage-vspk-negative-tests/tasks/main.yml )

@gundalow And i also noticed that if i remove the no_log from the auth section and move it to the sub options, it actually will print those values in debug output:

fatal: [localhost]: FAILED! => { "changed": false, "failed": true, "invocation": { "module_args": { "auth": { "api_enterprise": "csp", "api_password": "csproot", "api_url": "https://localhost:8443", "api_version": "v4_0" }, "command": "wait_for_job", "id": "dumy-job-uuid", "type": "Job" } }, "msg": "missing required arguments: api_username" }

Can you please raise a bug (or PR if you know the fix) for the basic.py issue

suboptions don't (currently) inherit no_log, or type
So you will need to set that explicitly on the sub options as needed

Looks like #2 has been addressed in 78c26d9

pdellaert · 2017-05-27T01:03:08Z

@gundalow I've made the changes as suggested, however, running this will fail because of the bug mentioned here: #25096

For the no_log of sub options, i created issue #25097

Feel free to run the negative tests, you don't need any Nuage component to do so:
https://github.com/nuagenetworks/vspk-ansible/blob/beccbf3269a3dd5163422814506e9a4b5b244240/roles/nuage-vspk-negative-tests/tasks/main.yml

gundalow · 2017-06-06T10:46:32Z

@pdellaert Looks like #25096 has been fixed in devel

gundalow · 2017-06-06T10:50:25Z

All network modules added into 2.4 need to include Unit Tests. So that will need adding before this can be merged.

Have a look at

test/units/modules/network/
test/units/module_utils/ # For when you start adding module_utils code
https://docs.ansible.com/ansible/dev_guide/testing_units.html

How do the tests in https://github.com/nuagenetworks/vspk-ansible/blob/beccbf3269a3dd5163422814506e9a4b5b244240/roles/nuage-vspk-negative-tests/tasks/main.yml work without having a Nuage device? I see them talking to a web endpoint with credentials.

I wonder if we could add them into our CI testing.

pdellaert · 2017-06-11T18:43:54Z

@gundalow For the negative tests, you do not need any system to run, these just verify that all required inputs are met without creating a connection to the Nuage VSP environment.

For any other tests, a setup is required. A potential solution could be to use our https://nuagex.io service. If you create an account, please let me know so i can get it activated as soon as possible.

I'll look into the unit tests, we do differ from the rest of the network modules with unit tests, it seems. As these modules use CLI commands and output from their respective platforms and we talk to our HTTPS REST API to manage the Nuage solution. Unit tests are still very useful, of course, we already have unit tests in the libraries used by the module, i'll add tests for the module specific.

pdellaert · 2017-06-13T16:23:55Z

Seems like something went wrong with my rebase... i'll create a new branch and push that

ansibot · 2017-06-13T16:34:26Z

cc @dharmabumstead
click here for bot help

ansibot · 2017-07-15T01:14:20Z

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

lib/ansible/modules/network/nuage/nuage_vspk.py:376:0: ImportError: No module named importlib

click here for bot help

dagwieers · 2017-07-18T13:32:27Z

-label vmware

This reverts commit eab23af.

alikins changed the title ~~Adding module for management of the Nuage Networks VSP SDN solution~~ New module: Adding module for management of the Nuage Networks VSP SDN solution May 22, 2017

alikins removed the needs_triage Needs a first human triage before being processed. label May 22, 2017

gundalow requested changes May 26, 2017

View reviewed changes

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

ganeshrn reviewed May 26, 2017

View reviewed changes

gundalow reviewed May 26, 2017

View reviewed changes

alikins changed the title ~~New module: Adding module for management of the Nuage Networks VSP SDN solution~~ New module: management of the Nuage Networks VSP SDN solution (network/nuage/nuage_vspk) Jun 5, 2017

ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Jun 5, 2017

ansibot removed the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Jun 6, 2017

ansibot added c:cli/adhoc c:cli/console c:cli/doc c:cli/galaxy and removed module This issue/PR relates to a module. new_module This PR includes a new module. labels Jun 13, 2017

pdellaert added 2 commits July 15, 2017 01:55

Using pypi package for simulator

6cb8525

Cleanup of requirements and adding integration tests

8c9e3f0

ansibot removed merge_commit This PR contains at least one merge commit. Please resolve! needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html labels Jul 15, 2017

Adding aliases for integration tests

0b32fd1

ansibot added the ci_verified Changes made in this PR are causing tests to fail. label Jul 15, 2017

ansibot added module This issue/PR relates to a module. new_module This PR includes a new module. and removed vmware VMware community labels Jul 18, 2017

pdellaert added 2 commits July 22, 2017 21:22

Adding module to import sanity test skip list

eab23af

Revert "Adding module to import sanity test skip list"

e89c5eb

This reverts commit eab23af.

ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Jul 22, 2017

Adding check for importlib and cleanup of requirements

09a5e5b

gundalow removed azure cloud digital_ocean docker inventory Inventory category net_tools Net-tools category openstack virt Virt community (incl. QEMU, KVM, libvirt, ovirt, RHV and Proxmox) labels Jul 24, 2017

gundalow approved these changes Jul 25, 2017

View reviewed changes

gundalow merged commit c005547 into ansible:devel Jul 25, 2017

pdellaert mentioned this pull request Jul 26, 2017

Adding Nuage module to CHANGELOG #27338

Merged

ansible locked and limited conversation to collaborators Apr 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New module: management of the Nuage Networks VSP SDN solution (network/nuage/nuage_vspk) #24895

New module: management of the Nuage Networks VSP SDN solution (network/nuage/nuage_vspk) #24895

pdellaert commented May 22, 2017

gundalow May 24, 2017

gundalow May 24, 2017

gundalow May 24, 2017

gundalow May 24, 2017

pdellaert May 26, 2017

gundalow May 26, 2017

gundalow May 26, 2017

pdellaert May 26, 2017 •

edited

gundalow May 26, 2017

gundalow May 26, 2017

gundalow May 26, 2017

ganeshrn May 26, 2017

pdellaert commented May 26, 2017

gundalow May 26, 2017

gundalow May 26, 2017

pdellaert May 26, 2017

pdellaert May 27, 2017 •

edited

pdellaert May 27, 2017

gundalow Jun 6, 2017

gundalow Jun 6, 2017

pdellaert commented May 27, 2017 •

edited

gundalow commented Jun 6, 2017

gundalow commented Jun 6, 2017

pdellaert commented Jun 11, 2017

pdellaert commented Jun 13, 2017

ansibot commented Jun 13, 2017

ansibot commented Jul 15, 2017

dagwieers commented Jul 18, 2017

New module: management of the Nuage Networks VSP SDN solution (network/nuage/nuage_vspk) #24895

New module: management of the Nuage Networks VSP SDN solution (network/nuage/nuage_vspk) #24895

Conversation

pdellaert commented May 22, 2017

SUMMARY

ISSUE TYPE

COMPONENT NAME

ANSIBLE VERSION

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

pdellaert May 26, 2017 • edited

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

pdellaert commented May 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdellaert May 27, 2017 • edited

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

pdellaert commented May 27, 2017 • edited

gundalow commented Jun 6, 2017

gundalow commented Jun 6, 2017

pdellaert commented Jun 11, 2017

pdellaert commented Jun 13, 2017

ansibot commented Jun 13, 2017

ansibot commented Jul 15, 2017

dagwieers commented Jul 18, 2017

pdellaert May 26, 2017 •

edited

pdellaert May 27, 2017 •

edited

pdellaert commented May 27, 2017 •

edited