Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nxos_l3_interfaces: fix states, add new minor attributes #64853

Merged
merged 10 commits into from Jan 10, 2020

Conversation

@chrisvanheuveln
Copy link
Contributor

chrisvanheuveln commented Nov 14, 2019

Depends-On: #63014

SUMMARY
  • Add new attributes to nxos_l3_interfaces module:

    • dot1q : (encapsulation dot1q ##)
    • redirects: (ip redirects)
    • unreachables: (ip unreachables)
  • Fixes for broken states merged/deleted/replaced/overridden)

    • All of the states had idempotence issues
    • I made some major changes to the way ipv4/ipv6 address changes occur, particularly with replaced.
      • The biggest problem was that the original code does a wholesale no ip address when there's a change to just one address, say changing a tag on a single secondary would remove the all addresses on the interface. This creates an unacceptable level of churn on the device for such a minor change.
      • Another issue that comes with address changes is that the changes may have to occur in sequence; e.g. primaries must be created first but can't be removed while secondaries exist.
      • I added helper methods for ipv4 & ipv6 to make the minimum number of changes while ignoring addresses that are idempotent.
      • replaced was also ignoring interfaces in the want list when they had no attributes defined
      • merged was ignoring tag-only changes

Additional issues were found during testing. I added extensive test cases to the unit test to cover a variety of scenarios. Please refer to the test cases to help understand the code changes that were made. Thanks.

Ref #37375.

ISSUE TYPE
  • Bugfix Pull Request
  • with new attrs
COMPONENT NAME

nxos_l3_interfaces

ADDITIONAL INFORMATION

Tested on N3K,N6K,N7K,N9K,N9Kv (internal testbeds: n9k-108,n6k-77,camden-nx-1,n7k-j,dt-n9k5-1,n3k-173,n3k-105,vergreen-nx-1,greensboro-nx-1,hamilton-nx-1

@ansibot

This comment has been minimized.

@ansibot

This comment was marked as outdated.

Copy link
Contributor

ansibot commented Nov 14, 2019

The test ansible-test sanity --test pylint [explain] failed with 4 errors:

lib/ansible/module_utils/network/nxos/config/l3_interfaces/l3_interfaces.py:237:16: bad-whitespace: No space allowed after bracket         sec_w = [ i for i in want if i.get('secondary')]                 ^
lib/ansible/module_utils/network/nxos/config/l3_interfaces/l3_interfaces.py:238:16: bad-whitespace: No space allowed after bracket         sec_h = [ i for i in have if i.get('secondary')]                 ^
lib/ansible/module_utils/network/nxos/config/l3_interfaces/l3_interfaces.py:239:16: bad-whitespace: No space allowed after bracket         pri_w = [ i for i in want if not i.get('secondary')]                 ^
lib/ansible/module_utils/network/nxos/config/l3_interfaces/l3_interfaces.py:240:16: bad-whitespace: No space allowed after bracket         pri_h = [ i for i in have if not i.get('secondary')]                 ^

The test ansible-test sanity --test pep8 [explain] failed with 22 errors:

lib/ansible/module_utils/network/nxos/config/l3_interfaces/l3_interfaces.py:237:18: E201: whitespace after '['
lib/ansible/module_utils/network/nxos/config/l3_interfaces/l3_interfaces.py:238:18: E201: whitespace after '['
lib/ansible/module_utils/network/nxos/config/l3_interfaces/l3_interfaces.py:239:18: E201: whitespace after '['
lib/ansible/module_utils/network/nxos/config/l3_interfaces/l3_interfaces.py:240:18: E201: whitespace after '['
lib/ansible/module_utils/network/nxos/config/l3_interfaces/l3_interfaces.py:275:46: E225: missing whitespace around operator
test/units/modules/network/nxos/test_nxos_l3_interfaces.py:218:62: E231: missing whitespace after ':'
test/units/modules/network/nxos/test_nxos_l3_interfaces.py:218:69: E261: at least two spaces before inline comment
test/units/modules/network/nxos/test_nxos_l3_interfaces.py:219:62: E231: missing whitespace after ':'
test/units/modules/network/nxos/test_nxos_l3_interfaces.py:219:69: E261: at least two spaces before inline comment
test/units/modules/network/nxos/test_nxos_l3_interfaces.py:221:62: E231: missing whitespace after ':'
test/units/modules/network/nxos/test_nxos_l3_interfaces.py:221:80: E261: at least two spaces before inline comment
test/units/modules/network/nxos/test_nxos_l3_interfaces.py:222:62: E231: missing whitespace after ':'
test/units/modules/network/nxos/test_nxos_l3_interfaces.py:222:80: E261: at least two spaces before inline comment
test/units/modules/network/nxos/test_nxos_l3_interfaces.py:223:62: E231: missing whitespace after ':'
test/units/modules/network/nxos/test_nxos_l3_interfaces.py:224:23: E124: closing bracket does not match visual indentation
test/units/modules/network/nxos/test_nxos_l3_interfaces.py:228:65: E231: missing whitespace after ':'
test/units/modules/network/nxos/test_nxos_l3_interfaces.py:229:23: E124: closing bracket does not match visual indentation
test/units/modules/network/nxos/test_nxos_l3_interfaces.py:321:47: E231: missing whitespace after ','
test/units/modules/network/nxos/test_nxos_l3_interfaces.py:322:62: E261: at least two spaces before inline comment
test/units/modules/network/nxos/test_nxos_l3_interfaces.py:323:12: E131: continuation line unaligned for hanging indent
test/units/modules/network/nxos/test_nxos_l3_interfaces.py:395:74: E261: at least two spaces before inline comment
test/units/modules/network/nxos/test_nxos_l3_interfaces.py:473:17: E124: closing bracket does not match visual indentation

click here for bot help

@chrisvanheuveln chrisvanheuveln changed the title (WIP) nxos_l3_interfaces: fix states, add new minor attributes nxos_l3_interfaces: fix states, add new minor attributes Nov 15, 2019
Copy link
Member

trishnaguha left a comment

The following test is failing


TASK [nxos_l3_interfaces : assert] **************************************************************************************************************************************************************
task path: /home/trishna/workspace/ansible/test/results/.tmp/integration/nxos_l3_interfaces-9eR1kS-ÅÑŚÌβŁÈ/test/integration/targets/nxos_l3_interfaces/tests/cli/merged.yaml:56
<localhost> attempting to start connection
<localhost> using connection plugin network_cli
Found ansible-connection at path /usr/bin/ansible-connection
<localhost> found existing local domain socket, using it!
<localhost> Response received, triggered 'persistent_buffer_read_timeout' timer of 0.1 seconds
<localhost> Response received, triggered 'persistent_buffer_read_timeout' timer of 0.1 seconds
<localhost> Response received, triggered 'persistent_buffer_read_timeout' timer of 0.1 seconds
<localhost> updating play_context for connection
<localhost> 
<localhost> local domain socket path is /home/trishna/.ansible/pc/e6b47aa962
fatal: [localhost]: FAILED! => changed=false 
  assertion: result.after|length == (ansible_facts.network_resources.l3_interfaces|length - 1)
  evaluated_to: false
  msg: Assertion failed

Tested on N9K-9000v (Version 7.0(3)I7(3))

@ansibot ansibot removed the needs_triage label Nov 19, 2019
@chrisvanheuveln

This comment has been minimized.

Copy link
Contributor Author

chrisvanheuveln commented Nov 19, 2019

The following test is failing

I'm guessing that you don't use mgmt0 in your testbeds, which would throw off the count. I'll find a different way to test this or just remove the count from the assert.

@NilashishC

This comment has been minimized.

Copy link
Contributor

NilashishC commented Dec 5, 2019

recheck

1 similar comment
@NilashishC

This comment has been minimized.

Copy link
Contributor

NilashishC commented Dec 5, 2019

recheck

Copy link
Contributor

pabelanger left a comment

I'd like to see us change prepare_nxos_tests here.

when: item.interface is regex('port-channel|loopback|\.[0-9]+$')
ignore_errors: yes
no_log: True

This comment has been minimized.

Copy link
@pabelanger

pabelanger Dec 5, 2019

Contributor

So, for this. I'd like to see each test clean up after isn't self, rather then prepare. The reason for this, doing it here, isn't really efficient. Today, we likely spend on about 1.5 hours just doing prepare_nxos_tests things. In the case of zuul testing I am working on, we don't actually run this code but create the variables in the inventory file.

This comment has been minimized.

Copy link
@chrisvanheuveln

chrisvanheuveln Dec 5, 2019

Author Contributor

@pabelanger I'll remove this. It was a convenient solution for us since we share our testbeds with more than just ansible testing and they're often left in a stale state.

This comment has been minimized.

Copy link
@pabelanger

pabelanger Dec 5, 2019

Contributor

thanks! in our case, our nxos instances are ephemeral, so between different PRs, we are safe from it. I believe, we can many work to update your test-bed to do this before running ansible-test, and update tests to try and clean up leaked data.

This comment has been minimized.

Copy link
@chrisvanheuveln

chrisvanheuveln Dec 5, 2019

Author Contributor

It's been removed. I'll add this cleanup to our local CI infra, that should cover the majority of cases. Agreed, we should also keep an eye out for dirty test teardowns.

This comment has been minimized.

Copy link
@pabelanger

pabelanger Dec 5, 2019

Contributor

Hmm, do you mind removing the whitespace here too? Trying to get zuul to work, and this is a merge conflict for us

@ansibot ansibot removed the core_review label Dec 5, 2019
@NilashishC

This comment has been minimized.

Copy link
Contributor

NilashishC commented Dec 30, 2019

recheck

@ansible-zuul

This comment was marked as outdated.

Copy link

ansible-zuul bot commented Dec 30, 2019

This change depends on a change that failed to merge.

@NilashishC

This comment has been minimized.

Copy link
Contributor

NilashishC commented Dec 30, 2019

recheck

@ansible-zuul

This comment has been minimized.

Copy link

ansible-zuul bot commented Dec 30, 2019

Build succeeded (third-party-check pipeline).

@ansibot ansibot added needs_revision and removed core_review labels Dec 30, 2019
@chrisvanheuveln chrisvanheuveln force-pushed the chrisvanheuveln:nxos_l3_interfaces-add-attrs branch from 41fdca8 to f647769 Jan 6, 2020
@ansibot ansibot added core_review and removed needs_revision labels Jan 6, 2020
@chrisvanheuveln

This comment has been minimized.

Copy link
Contributor Author

chrisvanheuveln commented Jan 6, 2020

It seems that the module is not removing the secondary IP address before negating the primary one and since we do not have an entry for Must delete secondary address before deleting primary address in our terminal error regex, the task run is not failing.

@NilashishC I fixed the issue with the ip address removal, which was fairly straightforward.

But while testing I noticed that ip redirects has a behavior that we failed to address. The issue is that when ip address ... secondary is added to an interface, the device will automatically disable redirects (no ip redirects); when all secondaries are removed from an interface redirects is re-enabled automatically (ip redirects).

Unfortunately, the redirect auto-enable behavior is inconsistent on legacy platforms such as N3K or N7K - I found that the device would often fail to re-enable redirects, and I was not able to find a consistent trigger for the failure. My solution was to explicitly re-enable redirects for those legacy platforms.

I also found that it was important to process the redirect commands after ipv4-related changes since adding/removing secondaries affects the redirects state, so I added a simple helper method to move redirect commands to the end of the cmds list.

I added another test to the UT for the redirect issue. I also updated the integration test to address the ip address scenario you discovered.

@ansible-zuul

This comment has been minimized.

Copy link

ansible-zuul bot commented Jan 7, 2020

Build succeeded (third-party-check pipeline).

@ansibot ansibot added needs_revision and removed core_review labels Jan 7, 2020
@NilashishC

This comment has been minimized.

Copy link
Contributor

NilashishC commented Jan 10, 2020

@chrisvanheuveln Thanks a lot for this PR!

@NilashishC

This comment has been minimized.

Copy link
Contributor

NilashishC commented Jan 10, 2020

bot_status

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Jan 10, 2020

Components

lib/ansible/module_utils/network/nxos/argspec/l3_interfaces/l3_interfaces.py
support: network
maintainers: chrisvanheuveln mikewiebe rahushen trishnaguha

lib/ansible/module_utils/network/nxos/config/l3_interfaces/l3_interfaces.py
support: network
maintainers: chrisvanheuveln mikewiebe rahushen trishnaguha

lib/ansible/module_utils/network/nxos/facts/l3_interfaces/l3_interfaces.py
support: network
maintainers: chrisvanheuveln mikewiebe rahushen trishnaguha

lib/ansible/modules/network/nxos/nxos_l3_interfaces.py
support: network
maintainers: chrisvanheuveln mikewiebe rahushen trishnaguha

lib/ansible/plugins/terminal/nxos.py
support: network
maintainers: NilashishC Qalthos danielmellado ganeshrn justjais trishnaguha

test/integration/targets/nxos_l3_interfaces/tasks/main.yaml
support: network
maintainers: chrisvanheuveln mikewiebe rahushen trishnaguha

test/integration/targets/nxos_l3_interfaces/tests/cli/deleted.yaml
support: network
maintainers: chrisvanheuveln mikewiebe rahushen trishnaguha

test/integration/targets/nxos_l3_interfaces/tests/cli/merged.yaml
support: network
maintainers: chrisvanheuveln mikewiebe rahushen trishnaguha

test/integration/targets/nxos_l3_interfaces/tests/cli/overridden.yaml
support: network
maintainers: chrisvanheuveln mikewiebe rahushen trishnaguha

test/integration/targets/nxos_l3_interfaces/tests/cli/replaced.yaml
support: network
maintainers: chrisvanheuveln mikewiebe rahushen trishnaguha

test/units/modules/network/nxos/test_nxos_l3_interfaces.py
support: core
maintainers: NilashishC Qalthos chrisvanheuveln danielmellado ganeshrn justjais mikewiebe rahushen trishnaguha

Metadata

waiting_on: chrisvanheuveln
changes_requested_by: pabelanger
needs_info: False
needs_revision: True
needs_rebase: False
merge_commits: []
too many files or commits: False
mergeable_state: clean
shippable_status: success
maintainer_shipits (module maintainers): False
community_shipits (namespace maintainers): False
ansible_shipits (core team members): False
shipit_actors (maintainer or core team member): None
shipit_actors_other:
automerge: automerge shipit test failed

click here for bot help

@NilashishC NilashishC merged commit 3ebc96e into ansible:devel Jan 10, 2020
2 checks passed
2 checks passed
Shippable Run 154975 status is SUCCESS.
Details
ansible/third-party-check third-party-check status: success
Details
@chrisvanheuveln

This comment has been minimized.

Copy link
Contributor Author

chrisvanheuveln commented Jan 10, 2020

@NilashishC Thanks for merging this. I know it was complicated. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.