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

Update get probe code to make it work it IOS XE 16.12.x #1285

Conversation

javcasalc
Copy link

This patch makes ios.py get_probes_config() getter able to work with new IOS XE versions:

csr1000v#show version | i , Version 
Cisco IOS XE Software, Version 16.12.03
Cisco IOS Software [Gibraltar], Virtual XE Software (X86_64_LINUX_IOSD-UNIVERSALK9-M), Version 16.12.3, RELEASE SOFTWARE (fc5)
csr1000v#sh run | section ip sla     
ip sla 1
 icmp-echo 10.0.0.2 source-interface GigabitEthernet11
 tag napalm_test
 frequency 15
 history buckets-kept 60
ip sla schedule 1 life forever start-time now
csr1000v#

This code is pretty inflexible as it is based on python regex multiline, so the block (ip sla config) must match all lines and options. It should be improved.

@javcasalc javcasalc force-pushed the feature/cisco_ios_get_probes_results branch 3 times, most recently from aff2481 to bf88aee Compare August 31, 2020 11:57
@coveralls
Copy link

coveralls commented Aug 31, 2020

Coverage Status

Coverage remained the same at 0.0% when pulling b570363 on javcasalc:feature/cisco_ios_get_probes_results into 4b17b93 on napalm-automation:develop.

@javcasalc
Copy link
Author

#1281

Copy link
Member

@mirceaulinic mirceaulinic left a comment

Choose a reason for hiding this comment

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

Thanks for this PR @javcasalc. The regex looks good, but in make it fruitful, we'll need a new test case with the output you're seeing on your platform (to show why the current code is not working, and prove that the new regex would solve it).

probe_args = {
"icmp-echo": r"^(?P<target>\S+)\s+source-(?:ip|interface)\s+(?P<source>\S+)$"
}
probe_type_map = {"icmp-echo": "icmp-ping"}
command = "show run | include ip sla [0-9]"
command = "show run | section ip sla [0-9]"
Copy link
Member

Choose a reason for hiding this comment

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

I don't have a lot of experience on IOS platforms, but my understanding from past PRs / conversations is that older versions may not support | section (while | include should always be there). Could you clarify what's the advantage or why you chose to switch @javcasalc?

Copy link
Author

Choose a reason for hiding this comment

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

According to Cisco doc, the right flag is section: https://www.cisco.com/c/en/us/td/docs/ios/fundamentals/command/reference/cf_book/cf_s1.html

Examples

The following examples compare the filtering characteristics of the show running-config | include command with the show running-config | section command. The first example gathers just the lines from the configuration file with "interface" in them.

Router# show running-config | include interface


interface Ethernet0/0 
interface Ethernet1/0 
interface Serial2/0 
interface Serial3/0


The next example uses the show command section command to gather the lines in the configuration file with "interface" in them as well as any lines associated with those entries. In this example, interface configuration information is captured.

Router# show running-config | section include interface


interface Ethernet0/0 
 shutdown 
 no cdp enable

interface Ethernet1/0 
 shutdown 
 no cdp enable 
interface Serial2/0 
 shutdown 
 no cdp enable 
interface Serial3/0 
 shutdown 
 no cdp enable

I did some tests in GNS3 with some old cisco routers (cisco 3640 with IOS 12.4; cisco 7200 with IOS 15.2) and I do confirm that section is the right choice:
imagen

Regarding the test case, I've included an extra "ip sla 3" entry, formatted with the new ios format, in order to test the correct parsing of both styles (old and new styles). Is this ok?

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough... any thoughts on this @ktbyers ?

@javcasalc javcasalc force-pushed the feature/cisco_ios_get_probes_results branch from a3ca270 to d1979cd Compare September 1, 2020 17:50
@mirceaulinic mirceaulinic merged commit 965966d into napalm-automation:develop Sep 3, 2020
@mirceaulinic mirceaulinic added this to the 3.2.0 milestone Sep 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants