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

Fix Issue#587 #588

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

scottwdavis
Copy link

Description

Updated REGEX for the parser for "show ip bgp neighbors received-routes" to make the CIDR notation after the prefix optional, as when it's not optional, some routes don't get parsed if they don't have the /CIDR notation.

Motivation and Context

As reported in Issue#587, when running "show ip bgp neighbors received-routes" on a device, if run it manually, in one example i saw 91 prefixes. When using pyATS parser for this command, i only saw 85 prefixes. 6 of the prefixes received were just the subnet address, with no /CIDR notation, and these 6 routes did not get parsed. With this REGEX update, the parser will capture these routes as well.

Checklist:

  • I have updated the changelog.
  • I have updated the documentation (If applicable).
  • I have added tests to cover my changes (If applicable).
  • All new and existing tests passed.
  • All new code passed compilation.

Updated REGEX in p3_2 inside ShowBgpNeighborsReceivedRoutesSuperParser() (iosxe)
To resolve the issue described in Issue#587
@scottwdavis scottwdavis requested a review from a team as a code owner December 7, 2021 17:14
@scottwdavis scottwdavis requested review from LukasMcClelland and a user December 7, 2021 17:14
Copy link
Contributor

@LukasMcClelland LukasMcClelland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thank you for contributing a fix for the issue you encountered.

Do you have some device output that you can add as a test? All tests are currently passing, so I know your fix hasn't broken anything. It would be more about ensuring that all future changes to this parser will still work with the output you've been seeing issues with.

Docs for writing tests can be found here
https://pubhub.devnetcloud.com/media/pyats-development-guide/docs/writeparser/writeparser.html#folder-based-testing-asa-ios-and-iosxe

@scottwdavis
Copy link
Author

I ran the folder tests for:
python folder_parsing_job.py -o iosxe -c ShowBgpNeighborsReceivedRoutes
I included a 4th test with actual output from the Production router i was testing on, with the expected output, and the test passed. I can't include that actual output here for security reasons (let me know if you need to see it, and we'll come up with a way for me to show you the golden_output4 files that doesn't involve me uploading them to the internet), but the tests all passed, with the modified REGEX expression correctly grabbing the 6 routes that were missed prior to the regex update:

2021-12-13T10:25:19: %GENIE-INFO: +------------------------------------------------------------------------------+
2021-12-13T10:25:19: %GENIE-INFO: | Unittest results |
2021-12-13T10:25:19: %GENIE-INFO: +------------------------------------------------------------------------------+
2021-12-13T10:25:19: %GENIE-INFO: SECTIONS/TESTCASES RESULT
2021-12-13T10:25:19: %GENIE-INFO: --------------------------------------------------------------------------------
2021-12-13T10:25:19: %GENIE-INFO: .
2021-12-13T10:25:19: %GENIE-INFO: |-- SuperFileBasedTesting PASSED
2021-12-13T10:25:19: %GENIE-INFO: | -- setup PASSED 2021-12-13T10:25:19: %GENIE-INFO: -- iosxe PASSED
2021-12-13T10:25:19: %GENIE-INFO: |-- setup PASSED
2021-12-13T10:25:19: %GENIE-INFO: |-- ShowBgpNeighborsReceivedRoutes PASSED
2021-12-13T10:25:19: %GENIE-INFO: | |-- Step 1: iosxe -> ShowBgpNeighborsReceivedRoutes PASSED
2021-12-13T10:25:19: %GENIE-INFO: | |-- Step 1.1: Test Golden -> iosxe -> ShowBgpNeighborsRecei... PASSED
2021-12-13T10:25:19: %GENIE-INFO: | |-- Step 1.1.1: Gold -> iosxe -> ShowBgpNeighborsReceivedRo... PASSED
2021-12-13T10:25:19: %GENIE-INFO: | |-- Step 1.1.2: Gold -> iosxe -> ShowBgpNeighborsReceivedRo... PASSED
2021-12-13T10:25:19: %GENIE-INFO: | |-- Step 1.1.3: Gold -> iosxe -> ShowBgpNeighborsReceivedRo... PASSED
2021-12-13T10:25:19: %GENIE-INFO: | |-- Step 1.1.4: Gold -> iosxe -> ShowBgpNeighborsReceivedRo... PASSED
2021-12-13T10:25:19: %GENIE-INFO: | |-- Step 1.2: Test Empty -> iosxe -> ShowBgpNeighborsReceiv... PASSED
2021-12-13T10:25:19: %GENIE-INFO: | -- Step 1.2.1: Empty -> iosxe -> ShowBgpNeighborsReceivedR... PASSED 2021-12-13T10:25:19: %GENIE-INFO: -- cleanup PASSED
2021-12-13T10:25:19: %GENIE-INFO: +------------------------------------------------------------------------------+
2021-12-13T10:25:19: %GENIE-INFO: | Summary |
2021-12-13T10:25:19: %GENIE-INFO: +------------------------------------------------------------------------------+
2021-12-13T10:25:19: %GENIE-INFO: Number of ABORTED 0
2021-12-13T10:25:19: %GENIE-INFO: Number of BLOCKED 0
2021-12-13T10:25:19: %GENIE-INFO: Number of ERRORED 0
2021-12-13T10:25:19: %GENIE-INFO: Number of FAILED 0
2021-12-13T10:25:19: %GENIE-INFO: Number of PASSED 2
2021-12-13T10:25:19: %GENIE-INFO: Number of PASSX 0
2021-12-13T10:25:19: %GENIE-INFO: Number of SKIPPED 0
2021-12-13T10:25:19: %GENIE-INFO: Total Number 2
2021-12-13T10:25:19: %GENIE-INFO: Success Rate 100.0%

@sjpatel21 sjpatel21 requested review from ademz and domachad May 25, 2022 18:19
@domachad
Copy link
Contributor

Hi @scottwdavis, we need at least one unit test that supports this change so we can cover all known scenarios. Can you please add a unit test to your pull request?

@GerriorL
Copy link
Contributor

Hi @scottwdavis as previously said we need to see a test can can cover the changes made to the regex to ensure any future changes don't break it. Since the device's actual output is sensitive information feel free to obfuscate the information however you see fit, so long as it is representative in some way of the actual output that required the change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants