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_hsrp_interfaces: new module #61498

Open
wants to merge 4 commits into
base: devel
from

Conversation

@chrisvanheuveln
Copy link
Contributor

commented Aug 28, 2019

SUMMARY

This is a new module to support hsrp attributes on interfaces. This PR is based on the code used for #61407 so it's very similar.

This module uses Resource Module Builder. This initial commit includes the following attributes:

  • bfd
    • this functionality is not present on some platforms (N35)
    • The enable value equates to cli: hsrp bfd
    • The disable value equates to cli: no hsrp bfd. This is the default value. It does not nvgen, i.e. is not displayed by show commands.

These new attributes are boolean states but we are using enable/disable states for consistency with bfd options in other modules.

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

nxos nxos_hsrp_interfaces

ADDITIONAL INFORMATION
  • Tested on integration / regression platforms: N3K
@ansibot

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2019

The test ansible-test sanity --test future-import-boilerplate [explain] failed with 1 error:

lib/ansible/module_utils/network/nxos/argspec/hsrp_interfaces/hsrp_interfaces.py:0:0: missing: from __future__ import (absolute_import, division, print_function)

The test ansible-test sanity --test metaclass-boilerplate [explain] failed with 1 error:

lib/ansible/module_utils/network/nxos/argspec/hsrp_interfaces/hsrp_interfaces.py:0:0: missing: __metaclass__ = type

click here for bot help

@trishnaguha
Copy link
Member

left a comment

Please resolve conflict and shippable failures.

@ansibot ansibot added needs_rebase and removed needs_triage labels Aug 29, 2019

def set_none_vals_to_defaults(self, want):
# Set dict None values to default states
if 'bfd' in want and want['bfd'] is None:
want['bfd'] = 'disable'

This comment has been minimized.

Copy link
@trishnaguha

trishnaguha Aug 29, 2019

Member

Why is bfd set todisable explicitly if the default state is disable.

This comment has been minimized.

Copy link
@chrisvanheuveln

chrisvanheuveln Aug 29, 2019

Author Contributor

It's a workaround due to a problem I saw with merged, as a result of setting 'default': 'disable' in the argspec.

Consider:

Device:

    interface Ethernet1/1
      hsrp bfd
    interface Ethernet1/2
      hsrp bfd

Playbook:

      nxos_hsrp_interfaces:
        config:
        - name: Ethernet1/1
          # NO HSRP ATTRS SPECIFIED

Result when default in argspec: the want dict sets Eth1/1's bfd to disabled, and with merged it turns off hsrp bfd on the device. But the playbook doesn't specify the attribute so the device state should not be changed.

But by not setting the default in argspec it causes an issue with overridden, in which case the bfd state is set to None for interfaces not shown in the playbook, which results in an idempotency issue. Therefore I needed to be able to set the state set to default (disabled) somehow to get overridden to work correctly. That's what the explicit disable is doing.

This comment has been minimized.

Copy link
@trishnaguha

trishnaguha Aug 29, 2019

Member

@chrisvanheuveln Since the default state of bfd itself is disabled, the device will set the state to disabled in running-config on removal of config for overridden. Also since remove_empties in facts will remove any null valued parameter from the dictionary so in this case bfd parameter won't be present. It's the coding logic that needs to modified. Please refer to nxos_vlans module code.

This comment has been minimized.

Copy link
@chrisvanheuveln

chrisvanheuveln Sep 3, 2019

Author Contributor

The nxos_vlans module is not a valid comparison because it doesn't have any defaults specified in argspec. However, I looked at nxos_interfaces which does ('enable' has a default to True) and in fact I see the same failure condition for that attribute when I try the same kind of test case (using nxos_interfaces). I'll send you a unit test diff through email that you can try which will demonstrate the failure.

Also, I think I may have confused you so let me clarify. There are two issues but only one is present at a time:

Issue 1. When I set 'default':'disable' in argspec it breaks the merged case. Overridden does work in this case.

Issue 2. When I do NOT set 'default':'disable' in argspec everything works except for overridden, which is why I added the workaround method to handle that.

@ansibot ansibot removed the ci_verified label Aug 29, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Aug 29, 2019

@chrisvanheuveln this PR contains the following merge commits:

Please rebase your branch to remove these commits.

click here for bot help

@chrisvanheuveln chrisvanheuveln force-pushed the chrisvanheuveln:nxos_hsrp_interfaces branch from 6d70d9f to a2cd1fc Sep 3, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2019

The test ansible-test sanity --test validate-modules [explain] failed with 1 error:

lib/ansible/modules/network/nxos/nxos_hsrp_interfaces.py:0:0: module-incorrect-version-added: version_added should be '2.10'. Currently 2.9

click here for bot help

@ansibot ansibot added the ci_verified label Sep 3, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2019

@chrisvanheuveln this PR contains the following merge commits:

Please rebase your branch to remove these commits.

click here for bot help

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2019

@chrisvanheuveln this PR contains the following merge commits:

Please rebase your branch to remove these commits.

click here for bot help

@ansibot ansibot added aws cloud labels Sep 4, 2019

@chrisvanheuveln chrisvanheuveln force-pushed the chrisvanheuveln:nxos_hsrp_interfaces branch from 134160b to 6827548 Sep 4, 2019

@trishnaguha trishnaguha self-assigned this Sep 11, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2019

The test ansible-test sanity --test validate-modules [explain] failed with 1 error:

lib/ansible/modules/network/nxos/nxos_hsrp_interfaces.py:0:0: module-incorrect-version-added: version_added should be '2.10'. Currently 2.9

click here for bot help

@ansibot ansibot added the ci_verified label Sep 11, 2019

@ansibot ansibot added the stale_ci label Sep 19, 2019

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