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_vpc: pkl_vrf fixes for #57069 #57370

Open
wants to merge 7 commits into
base: devel
from

Conversation

Projects
None yet
4 participants
@chrisvanheuveln
Copy link
Contributor

commented Jun 4, 2019

SUMMARY

Fixes #57069

Symptom
When playbooks specify pkl_vrf: default, the result is that the cli does not set the vrf state.

Analysis

  • First issue: default is a cli reserved word (for peer-keepalive) when used with the vrf keyword. It refers to the default rib.

    • This is confusing in several ways because peer-keepalive's default vrf is the management vrf.
  • Second issue: When changing only one optional value (e.g. pkl_vrf) while other optional values are idempotent (e.g. pkl_src), the result is that the idempotent values are ignored; unfortunately the device cli replaces the entire command, in which case the idempotent values are removed.

    • e.g. playbook specifies this:
    pkl_dest: 10.1.1.1
    pkl_src: 10.2.2.2
    pkl_vrf: my_vrf
    
    peer-keepalive dest 10.1.1.1 src 10.2.2.2             # original
    
    peer-keepalive dest 10.1.1.1 src 10.2.2.2 vrf my_vrf  # intended result
    
    peer-keepalive dest 10.1.1.1 vrf my_vrf               # actual result
    
  • Third issue: the pkl getter was relying on positional data. This broke when the udp keyword nvgen'd where vrf used to appear (shifting all keywords to the right).

  • Added new unit tests to validate vrf behavior

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

nxos_vpc

ADDITIONAL INFORMATION
  • Tested on regression platforms: N3K,N6k,N7K,N9K,N3K-F
@mikewiebe

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2019

shipit

@@ -58,9 +58,12 @@
pkl_dest:
description:
- Destination (remote) IP address used for peer keepalive link
- pkl_dest is required whenever pkl options are used.

This comment has been minimized.

Copy link
@trishnaguha

trishnaguha Jun 6, 2019

Member

Is pkl_dest required for both pkl_vrf and pkl_src?

This comment has been minimized.

Copy link
@chrisvanheuveln

chrisvanheuveln Jun 6, 2019

Author Contributor

Yes, pkl_dest is required for all peer-keepalive options:

n9k-109(config-vpc-domain)# peer-keepalive ?
  destination  Specify destination ip address of peer switch

n9k-109(config-vpc-domain)# peer-keepalive destination ?
  A.B.C.D  IPv4 address (A.B.C.D) of destination

n9k-109(config-vpc-domain)# peer-keepalive destination 192.168.1.1 ?
  <CR>
  hold-timeout  Hold timeout to ignore stale peer alive messages
  interval      Enter interval in milleseconds
  precedence    Precedence
  source        Source interface for hello
  tos           Type of Service
  tos-byte      Type of Service Byte
  udp-port      Enter UDP port number used for hello
  vrf           Vrf to be used for hello messages

There is also a dependency check for it here:
https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/network/nxos/nxos_vpc.py#L369

@trishnaguha trishnaguha self-assigned this Jun 6, 2019

@ansibot ansibot added core_review and removed shipit labels Jun 6, 2019

@ansibot ansibot added the stale_ci label Jun 14, 2019

chrisvanheuveln added some commits Jun 4, 2019

nxos_vpc: pkl_vrf fixes for #57069
Fixes #57069

- Symptom: When playbooks specify `pkl_vrf: default`, the result is that the cli does not set the `vrf` state.

- Analysis:
 - First issue: 'default' is a reserved word when used with the `peer-keepalive` `vrf` keyword. It refers to the default rib.
   - This is confusing in several ways because `peer-keepalive`'s *default* vrf is the `management` vrf.

 - Second issue: When changing only one optional value (`pkl_vrf`) while other optional values are idempotent (`pkl_src`), the result is that the idempotent values are ignored; unfortunately the device cli *replaces* the entire command, in which case the idempotent values are removed.
   - e.g. playbook specifies this:
     ```
     { pkl_dest: 10.1.1.1, pkl_src: 10.2.2.2, pkl_vrf: my_vrf }
     ```

     ```
     peer-keepalive dest 10.1.1.1 src 10.2.2.2             # original

     peer-keepalive dest 10.1.1.1 src 10.2.2.2 vrf my_vrf  # intended result

     peer-keepalive dest 10.1.1.1 vrf my_vrf               # actual result
     ```

 - Third issue: the `pkl` getter was relying on positional data. This broke when the `udp` keyword nvgen'd where `vrf` used to appear (shifting all keywords to the right).

- Tested on regression platforms: `N3K,N6k,N7K,N9K,N3K-F,N9K-F`

@chrisvanheuveln chrisvanheuveln force-pushed the chrisvanheuveln:nxos_vpc-fix-57069 branch from 73c6db8 to d88a9a6 Jun 18, 2019

@mikewiebe

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

shipit

@ansibot ansibot added shipit and removed core_review stale_ci labels Jun 18, 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.