Closed benmaddison closed 7 years ago
I will see if I can re-write this to not use TextFSM. But implement the feature requested and integrate the unit tests.
I don't think it makes sense to add TextFSM especially given the probably long term direction of YANG for the getters.
I'd be surprised if we ever saw a usable yang implementation in ios classic/xe land to be honest. We are going to be scraping screens for a while!
Re-implementing using re
only shouldn't be that hard, and I'm happy to assist.
My strong preference, however, would be to pull this change and then rework in a new PR, since the performance and lack of multi-AF support makes the current version unusable in all but the most trivial networks.
YANG is just a modeling language. So NAPALM-Yang will parse configuration/output and convert to YANG abstractions. Or go from YANG models to vendor configs.
The network device doesn't have to be capable of supporting YANG.
So yes, screen-scraping, but to/from YANG models.
Anything I can assist with to get this merged? I need to publish a release of a package that has napalm
as a dependency in the next few days, but without this fix the performance becomes unacceptably poor for IOS targets.
Yes, sorry about delay on this. Let me see if I can review this on Thursday or Friday.
Thanks @ktbyers. Let me know if you come across anything you need me to tidy up.
Some of the items I mention or mostly from a perspective of keeping things consistent versus one being better than the other (for example, netaddr vs ipaddress, or using conditionals and exceptions instead of assert statements).
all comments addressed except the switch from ipaddress
to netaddr
. will try and get to that later today. let me know if there is anything else you pick up the meantime?
I think that's everything ;-)
@benmaddison I am going to merge this change into a separate branch as there are some things that I want to change.
Given the size of the change it is to hard to manage just via feedback on this channel.
I will post the link to the branch after I create it.
Here is the branch:
https://github.com/napalm-automation/napalm-ios/tree/get_bgp_neighbors_pr
@benmaddison On the BGP summary processing isn't there an issue with the fill-down fields?
It looks like you are propagating the 'uptime', 'remote_as', and 'remote_addr' as fill-down from the BGP summary?
@ktbyers those fields are guaranteed to be present if a line in the output is matched against a pattern that has record: True
, so they'll never fall through from an earlier match.
It won't change the behaviour if we add those to no_fill_fields
, so we can do that it you think it makes the intent clearer?
On the asdot syntax question, i'm not inclined to spend time supporting something that was deprecated almost 10 years ago (rfc5396), and hasn't been the default in XE since 2.4! Is there need for it that doesn't have an easy workaround that you're aware of?
@ktbyers I have no issues with your other clean-ups
@benmaddison On the asdot syntax...it is a configuration option, so we are pretty much guaranteed to see AS numbers in this format.
It is a simple fix so I am going to put it in.
You don't have to do it though...I can add that in.
@benmaddison Okay, I will probably update the non-fill fields to include others.
Basically, I think this will make the risk lower to propagate values obtained earlier from a different peer (in case the regular expression patterns change, or some other change happens).
@benmaddison Why did you choose?
# merge other values in a sane manner
existing['is_up'] = existing['is_up'] or is_up
existing['uptime'] = max(existing['uptime'], uptime)
I am inclined to make the is_up be a logical and (i.e. if either address families are down, then flag it as down).
Similarly, I am inclined to make the uptime the min of the uptime (i.e. I want to know one of them bounced recently).
Thoughts?
The revised implementation provides:
textfsm
is used for output parsing. I'm willing to help re-write usingre
if the consensus is against the use oftextfsm
.