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_interface: add bfd,bfd_echo,hsrp_bfd options #58340

Open
wants to merge 4 commits into
base: devel
from

Conversation

@chrisvanheuveln
Copy link
Contributor

commented Jun 25, 2019

SUMMARY

Add new options to the nxos_interface module:

  • bfd
  • bfd_echo
  • hsrp_bfd

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

Add'l changes:

  • Removed redundant normalize_interface() and show run interface calls in map_config_to_obj method.
  • I noticed some issues with determining admin_state on some platforms:
    • N6K platforms don't have an admin_state key in the show interface | json output.
    • A previous workaround was to use the state_rsn_desc key, but that is not always reliable (it may be set to "SFP not inserted" instead of showing actual admin state)
    • My workaround adds the all keyword to show run interface so that the output includes the shutdown config state, to be used as a fallback state check when that key is not available.
ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

nxos_interface

ADDITIONAL INFORMATION

Tested on integration / regression platforms: N3K,N6K,N7K,N9K,N3K-F

chrisvanheuveln added some commits Jun 25, 2019

nxos_interface: add bfd,bfd_echo,hsrp_bfd options
Add new options to the `nxos_interface` module:
- bfd
- bfd_echo
- hsrp_bfd

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

Add'l changes:
- Removed redundant `show run interface` calls in `map_config_to_obj` method.
- I noticed some issues with determining `admin_state` on some platforms:
  - `N6K` platforms don't have an `admin_state` key in the `show interface | json` output.
  - A previous workaround was to use the `state_rsn_desc` key, but that is not always reliable (it may be set to `"SFP not inserted"` instead of showing actual admin state)
  - My workaround adds the `all` keyword to `show run interface` so that the output includes the `shutdown` config state, to be used as a fallback state check when that key is not available.

Testing: Integration / regression platforms: `N3K,N6K,N7K,N9K,N3K-F`
@mikewiebe

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2019

shipit

@trishnaguha trishnaguha self-assigned this Jun 26, 2019

- "Dependency: feature bfd, feature hsrp"
version_added: 2.9
type: str
choices: ['enable', 'disable']

This comment has been minimized.

Copy link
@trishnaguha

trishnaguha Jul 4, 2019

Member

Instead of having three different parameters, can we add these parameteres as options for bfd?
bfd_options: {bfd: '', echo: '', hsrp: ''}

This comment has been minimized.

Copy link
@chrisvanheuveln

chrisvanheuveln Jul 5, 2019

Author Contributor

I like your idea but it may not be a good fit in this case. My concern is that there are other hsrp and bfd options to consider.

With hsrp interface options, the bfd state is just one of several possible hsrp options and it seems like it would be better to associate the hsrp bfd state with hsrp options:

n9k-109(config-if)# hsrp ?
  <0-4095>     Group number (0-255 for HSRPv1)
  bfd          BFD protocol
  delay        HSRP initialisation delay
  mac-refresh  Interface mac-refresh time
  use-bia      HSRP uses interface's burned in address
  version      HSRP version

For bfd interface options, several of them mimic the bfd global settings (to allow per-interface tweaking) so I would expect them to use the same convention as in bfd_global. A case can be made for bfd_options: {bfd: '', echo: ''} but those two options would be exceptions to the standalone option format used with the other bfd options. (which may be fine, I just wanted you to be aware before we change anything)

global:

n9k-109(config)# bfd ?
  echo-interface    Configure interface used for bfd echo frames
  echo-rx-interval  Configure BFD session echo rx interval
  interval          Configure BFD session interval parameters
  ipv4              Ipv4 sessions
  ipv6              Ipv6 sessions
  slow-timer        Configure slow mode timer for sessions
  startup-timer     Configure Delayed Start Up timer for sessions

interface:

n9k-109(config)# interface ethernet 1/1
n9k-109(config-if)# bfd ?
  <CR>
  authentication    Configure BFD authentication parameters
  echo              Configure Echo function for all address families
  echo-rx-interval  Configure BFD session echo rx interval
  interval          Configure BFD session interval parameters
  ipv4              Ipv4 sessions
  ipv6              Ipv6 sessions
  neighbor          BFD neighbor configuration commands (simulate client)
  optimize          Optimize

@trishnaguha trishnaguha requested a review from cidrblock Jul 9, 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.