Closed ktbyers closed 7 years ago
@mirceaulinic or @dbarrosop or @ebeahan ...one of you should also review this (as I made quite a few modifications to it).
I have reviewed the original work pretty extensively (so main requirement is to review my changes).
The CI issue looks like a Travis-CI issue with Python3.5 (Python3.4 works...so I think we should be good there).
@ktbyers on your question RE: merging the is_up
and uptime
values across different AFIs using OR and MAX, respectively:
I did it this way because I interpret the resulting fields in the .get_bgp_neighbors()
return dict
as referring to the state of the session itself, not of the individual AFIs negotiated within the session.
It is possible, for example, to have ipv4 in Established
and ipv6 in NoNeg
, in which case returning is_up = False
for the session seems misleading.
I think that the real fix for this should be to provide the ability for the method to return per-AFI state: but that obviously needs to change library wide, not just in napalm-ios
.
Yes, agreed that separate reflection of is_up and uptime per AFI would be better. This will change in napalm-yang so I don't think it makes sense to update here.
@mirceaulinic @dbarrosop What are your thoughts on the issue of is_up / uptime across both AFI of IPv4 and IPv6. I still think a logical-or and minimum uptime is the more conservative approach. I.E. if you configured them both then if both of them are not up, reflect is_up as down (and use the minimum uptime).
is_up
and uptime
are related to the BGP session which is independent from the AFI. NoNeg
is actually a "negotiation" capability mismatch (not really a session issue) so there are a few things we can do:
get_bgp_neighbors
returns state, not configuration.
b. Alternatively, we could also discuss adding a capabilities
field where we list which capabilities were negotiated successfully.@dbarrosop @benmaddison Okay, I converted is_up back to a logical or and I changed uptime back to max.
Realistically, the up_time and is_up should be essentially the same for both AFIs (except possibly for transition events...i.e. we query IPv4, and then the session goes down, then we query IPv6).
I also changed 'is_up' to be based on uptime and not to be based on state/prefixes field (as NoNeg state would previously flag BGP session as down even though it was up).
Here is an example:
Neighbor V AS MsgRcvd MsgSent TblVer InQ OutQ Up/Down State/PfxRcd
xxxx:xxx:61F::11 4 701 251053 262007 0 0 0 00:04:51 (NoNeg)
xx.xx.228.5 4 701 5404546 267918 105 0 0 00:22:44 6
I also converted the no-value for accepted/received/sent prefixes to be -1 (since zero can be a valid value when the session is up).
Note, I am going to move this and track it as a separate issue. Basically, I think the current code is a significant improvement over what we previously had and I think we should move forward with it. Then we can separately address what do with respect to an address family not being negotiated.
Now, we probably want to signal if an AF is negotiated or not. We have two
solutions IMHO:
a. Easiest would be to not return it at all. After all, get_bgp_neighbors returns state,
not configuration.
I agree. Approved :)
cc: @benmaddison