Update nxos_install_os module #40102

mikewiebe · 2018-05-14T18:57:19Z

SUMMARY

This update includes the following:

Change to the check_ansible_timer method inside the module to make sure both ANSIBLE_PERSISTENT_COMMAND_TIMEOUT and ANSIBLE_PERSISTENT_CONNECTION_TIMEOUT are set properly when using this module.
- I understand that there are multiple ways to set these timers, but we need a way to make sure they are set for connection local, network_cli and nxapi.
- Please advise if there is a way that we can check that these timers are set using all methods available (env vars, config file)
Add integration test playbooks that can be used to upgrade Nexus devices.
- By default they will not run since a meta: end_play has been added in the main install_os.yaml playbook.

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

nxos_install_os

ANSIBLE VERSION

ansible 2.6.0 (rel260/nxos_install_os 1b5f7dc422) last updated 2018/05/14 14:50:05 (GMT -400)
  config file = /etc/ansible/ansible.cfg
  configured module search path = [u'/Users/mwiebe/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /Users/mwiebe/Projects/nxos_ansible/fix_ansible/lib/ansible
  executable location = /Users/mwiebe/Projects/nxos_ansible/fix_ansible/bin/ansible
  python version = 2.7.13 (default, Apr  4 2017, 08:47:57) [GCC 4.2.1 Compatible Apple LLVM 8.1.0 (clang-802.0.38)]

mikewiebe · 2018-05-14T19:03:23Z

test/integration/targets/nxos_install_os/tasks/upgrade/clear_persistent_sockets.yaml

@@ -0,0 +1,15 @@
+---


@trishnaguha I added this because as you know this is what we have to do for tasks to execute successfully following an upgrade. It's commented out in the actual call flow (see install_os.yaml below) but I decided to include it for users that might make use of it.

ansibot · 2018-05-14T19:06:55Z

cc @GGabriele @jedelman8 @privateip @rahushen @rcarrillocruz
click here for bot help

trishnaguha · 2018-05-15T09:22:52Z

lib/ansible/modules/network/nxos/nxos_install_os.py

-    data = module.params.get('provider')
+    import os
+    command_timer = os.environ.get('ANSIBLE_PERSISTENT_COMMAND_TIMEOUT')
+    connect_timer = os.environ.get('ANSIBLE_PERSISTENT_CONNECT_TIMEOUT')


@mikewiebe this will only work if command_timeout/connect_timeout is set as ENV var, but won't work if someone sets the timeout in ansible.cfg within persistent_connection section.
You can do the following instead.

+ +from ansible import constants as C from ansible.module_utils.network.nxos.nxos import load_config, run_commands from ansible.module_utils.network.nxos.nxos import nxos_argument_spec, check_args from ansible.module_utils.basic import AnsibleModule @@ -131,6 +133,8 @@ from ansible.module_utils.basic import AnsibleModule def check_ansible_timer(module): '''Check Ansible Timer Values''' + command_timer = C.PERSISTENT_COMMAND_TIMEOUT + connect_timer = C.PERSISTENT_CONNECT_TIMEOUT

This takes care of both the scenarios.

Excellent. Thanks for the suggestion.

Qalthos · 2018-05-15T12:34:37Z

test/integration/targets/nxos_install_os/tasks/network_cli.yaml

+  set_fact: test_items="{{ test_cases.files | map(attribute='path') | list }}"
+
+- name: run test cases (connection=network_cli)
+  include: "{{ test_case_to_run }} ansible_connection=network_cli connection={}"


There's your problem, replace connection={} with connection={{ cli }} (or, again, anything really, as long as it's a dictionary) and #40027 will go away

(see #40027 (comment) for more info)

Thanks. Will update.

ansibot · 2018-05-15T15:16:44Z

The test ansible-test sanity --test import --python 2.6 [explain] failed with 1 error:

lib/ansible/modules/network/nxos/nxos_install_os.py:127:0: ImportError: cannot import name constants

The test ansible-test sanity --test import --python 2.7 [explain] failed with 1 error:

lib/ansible/modules/network/nxos/nxos_install_os.py:127:0: ImportError: cannot import name constants

The test ansible-test sanity --test import --python 3.5 [explain] failed with 1 error:

lib/ansible/modules/network/nxos/nxos_install_os.py:127:0: ImportError: cannot import name 'constants'

The test ansible-test sanity --test import --python 3.6 [explain] failed with 1 error:

lib/ansible/modules/network/nxos/nxos_install_os.py:127:0: ImportError: cannot import name 'constants'

The test ansible-test sanity --test import --python 3.7 [explain] failed with 1 error:

lib/ansible/modules/network/nxos/nxos_install_os.py:127:0: ImportError: cannot import name 'constants' from 'ansible' (/root/ansible/test/sanity/import/lib/ansible/__init__.py)

click here for bot help

mikewiebe · 2018-05-15T16:06:14Z

@trishnaguha Looks like ansibot does not like the from ansible import constants as C line. Please advise.

mattclay · 2018-05-15T17:09:59Z

lib/ansible/modules/network/nxos/nxos_install_os.py

@@ -124,25 +124,24 @@

 import re
 from time import sleep
+from ansible import constants as C


Imports from ansible are not available to modules. Imports must be from ansible.module_utils.

mattclay · 2018-05-15T17:11:55Z

lib/ansible/modules/network/nxos/nxos_install_os.py

-    msg = msg + 'provider: "{{ connection | combine({\'timeout\': 500}) }}"'
-    data = module.params.get('provider')
+    command_timer = C.PERSISTENT_COMMAND_TIMEOUT
+    connect_timer = C.PERSISTENT_CONNECT_TIMEOUT


These will need to be module parameters if they need to be configurable. Constants are not available to modules.

@mattclay I am simply trying to make sure that users are setting the ANSIBLE_ PERSISTENT_COMMAND_TIMEOUT and ANSIBLE_ PERSISTENT_CONNECT_TIMEOUT high enough to be used with this module. This module controls Nexus upgrades so these need to be set in a way to avoid timeout issues. Can I change these timers inside an Ansible module directly? I thought that they were checked/used before we get to the module code.

Modules do not have access to get or set configuration values. An action plugin should be able to check the configuration before invoking the module, but I don't think that is something we're doing with any other modules currently. You may want to bring this up in tomorrow's Network Working Group meeting on IRC to see what thoughts the network team has on this.

trishnaguha · 2018-05-16T13:46:08Z

@mikewiebe Apologies for suggesting importing constants.
After looking in to the module code, what I understood is that the module fails if the timeout value is low as higher value of timeout is required for the particular module - this is what the function does. Since we do not have the scope of using constants in module code, as of workaround what I can think is that you can add this info in Note section of the module doc.

trishnaguha · 2018-05-16T14:48:25Z

@mikewiebe As discussed with @Qalthos, on addition to adding notes in module doc, you can check timer in nxos action plugin. I hope this will help you to achieve what you are trying to do in the module.
Something similar to this:

$ git diff
diff --git a/lib/ansible/plugins/action/nxos.py b/lib/ansible/plugins/action/nxos.py
index 4b3663f8b3..3d85bdb965 100644
--- a/lib/ansible/plugins/action/nxos.py
+++ b/lib/ansible/plugins/action/nxos.py
@@ -46,6 +46,10 @@ class ActionModule(_ActionModule):
         if self._task.args.get('provider', {}).get('transport') == 'nxapi' and self._task.action == 'nxos_nxapi':
             return {'failed': True, 'msg': "Transport type 'nxapi' is not valid for '%s' module." % (self._task.action)}
 
+        if self._task.action == 'nxos_install_os':
+            if C.PERSISTENT_COMMAND_TIMEOUT < 500 or C.PERSISTENT_CONNECT_TIMEOUT < 500:
+                return {'failed': True, 'msg': "timers need to be set to 500 seconds or higher when using nxos_install_os module"}
+
         if self._play_context.connection == 'network_cli':
             provider = self._task.args.get('provider', {})
             if any(provider.values()):

mikewiebe · 2018-05-25T23:49:19Z

@trishnaguha I am done making the updates here. There are still issues with upgrading using connection type network_cli, local and httpapi but still more work to do there.

I added a layer of tests so that we can still run tests using provider. Not sure if there is a more efficient way to do that, but this gets the job done.

trishnaguha · 2018-05-29T14:16:07Z

lib/ansible/modules/network/nxos/nxos_install_os.py

-      The module will exit with a failure message if the timer is
-      not set to 500 seconds or higher.
+    - This module requires both the ANSIBLE_PERSISTENT_CONNECT and
+      ANSIBLE_PERSISTENT_COMMAND timers to be set to 600 seconds or higher.


Can you please change the env vars to ANSIBLE_PERSISTENT_COMMAND_TIMEOUT and ANSIBLE_PERSISTENT_CONNECT_TIMEOUT instead?

trishnaguha · 2018-05-30T04:14:36Z

cherry-picked to 2.6

* Add nxos_install_os integration tests * Update call to check timers * Update check_ansible_timer method * Modify network_cli integration tests * Add timer check for nxos_install_os * Add comments for clear_persistent_sockets * Update connection info for tests * More updates * Restructure files for provider and non-provider testing * Update env var name and add check for ISSU switchover (cherry picked from commit 9f02630)

* httpapi fix nxos (#40806) * httpapi fix nxos Signed-off-by: Trishna Guha <trishnaguha17@gmail.com> * nxos_hsrp fix Signed-off-by: Trishna Guha <trishnaguha17@gmail.com> (cherry picked from commit a7421e8) * fix nxos_vrf and migrate get_interface_type to module_utils (#40825) Signed-off-by: Trishna Guha <trishnaguha17@gmail.com> (cherry picked from commit b4baa2d) * nxos_vlan fix (#40822) * nxos_vlan fix Signed-off-by: Trishna Guha <trishnaguha17@gmail.com> * uncomment mode test as nxapi now has get_capabilities Signed-off-by: Trishna Guha <trishnaguha17@gmail.com> (cherry picked from commit 17b6ecf) * Update nxos_install_os module (#40102) * Add nxos_install_os integration tests * Update call to check timers * Update check_ansible_timer method * Modify network_cli integration tests * Add timer check for nxos_install_os * Add comments for clear_persistent_sockets * Update connection info for tests * More updates * Restructure files for provider and non-provider testing * Update env var name and add check for ISSU switchover (cherry picked from commit 9f02630)

* Add nxos_install_os integration tests * Update call to check timers * Update check_ansible_timer method * Modify network_cli integration tests * Add timer check for nxos_install_os * Add comments for clear_persistent_sockets * Update connection info for tests * More updates * Restructure files for provider and non-provider testing * Update env var name and add check for ISSU switchover

mikewiebe mentioned this pull request May 14, 2018

Cannot combine with network provider setting for connection network_cli #40027

Closed

mikewiebe commented May 14, 2018

View reviewed changes

trishnaguha self-assigned this May 15, 2018

trishnaguha removed the needs_triage Needs a first human triage before being processed. label May 15, 2018

trishnaguha reviewed May 15, 2018

View reviewed changes

Qalthos reviewed May 15, 2018

View reviewed changes

ansibot added ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels May 15, 2018

trishnaguha added this to In Review in zzz NOT USED: Networking Bugs May 15, 2018

mattclay reviewed May 15, 2018

View reviewed changes

mikewiebe mentioned this pull request May 16, 2018

Network Working Group Meeting Agenda 2 ansible/community#247

Closed

mikewiebe changed the title ~~Update nxos_install_os module~~ [WIP] Update nxos_install_os module May 18, 2018

ansibot added WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. affects_2.6 This issue/PR affects Ansible v2.6 labels May 18, 2018

mikewiebe force-pushed the rel260/nxos_install_os branch from d78974c to 69ed6c9 Compare May 22, 2018 16:47

ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label May 25, 2018

mikewiebe changed the title ~~[WIP] Update nxos_install_os module~~ Update nxos_install_os module May 25, 2018

ansibot removed the WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. label May 25, 2018

mikewiebe added 9 commits May 29, 2018 09:47

Add nxos_install_os integration tests

e95b49d

Update call to check timers

95ed638

Update check_ansible_timer method

61b8160

Modify network_cli integration tests

7b7e2ea

Add timer check for nxos_install_os

3e075eb

Add comments for clear_persistent_sockets

29be869

Update connection info for tests

7ab96a6

More updates

83805b5

Restructure files for provider and non-provider testing

da22aa2

mikewiebe force-pushed the rel260/nxos_install_os branch from 1dff6ce to da22aa2 Compare May 29, 2018 13:47

trishnaguha reviewed May 29, 2018

View reviewed changes

Update env var name and add check for ISSU switchover

6de6c3d

trishnaguha approved these changes May 29, 2018

View reviewed changes

trishnaguha merged commit 9f02630 into ansible:devel May 29, 2018

trishnaguha moved this from In Review to Done in zzz NOT USED: Networking Bugs May 30, 2018

mikewiebe deleted the rel260/nxos_install_os branch November 26, 2018 18:05

dagwieers added the cisco Cisco technologies label Feb 23, 2019

ansible locked and limited conversation to collaborators May 29, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update nxos_install_os module #40102

Update nxos_install_os module #40102

mikewiebe commented May 14, 2018

mikewiebe May 14, 2018

ansibot commented May 14, 2018

trishnaguha May 15, 2018

mikewiebe May 15, 2018

Qalthos May 15, 2018 •

edited

mikewiebe May 15, 2018

ansibot commented May 15, 2018

mikewiebe commented May 15, 2018

mattclay May 15, 2018

mattclay May 15, 2018

mikewiebe May 15, 2018

mattclay May 15, 2018

trishnaguha commented May 16, 2018 •

edited

trishnaguha commented May 16, 2018 •

edited

mikewiebe commented May 25, 2018

trishnaguha May 29, 2018

trishnaguha commented May 30, 2018

Update nxos_install_os module #40102

Update nxos_install_os module #40102

Conversation

mikewiebe commented May 14, 2018

SUMMARY

ISSUE TYPE

COMPONENT NAME

ANSIBLE VERSION

Choose a reason for hiding this comment

ansibot commented May 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Qalthos May 15, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ansibot commented May 15, 2018

mikewiebe commented May 15, 2018

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

trishnaguha commented May 16, 2018 • edited

trishnaguha commented May 16, 2018 • edited

mikewiebe commented May 25, 2018

Choose a reason for hiding this comment

trishnaguha commented May 30, 2018

Qalthos May 15, 2018 •

edited

trishnaguha commented May 16, 2018 •

edited

trishnaguha commented May 16, 2018 •

edited