napalm-automation / napalm-base

Apache License 2.0
32 stars 48 forks source link

Fix `get_route_to` or fix tests #238

Open dbarrosop opened 7 years ago

dbarrosop commented 7 years ago

The get_route_to method is a tricky one as the protocol argument is not thoroughly tested and might lead to differences in terms of support across different drivers. For example, a driver could support static and bgp while another might support only static.

Proposed solutions:

  1. get_route_to only lives in napalm-base and calls for get_route_to_static, get_route_to_bgp, etc... that are individual implementations in the drivers. Would be nice to have an IPv4/IPv6 flag, btw.
  2. We ignore this issue and we just fix the tests so there is no "protocol" sent to get_route_to and control tests/behaviour with mocked data as described here: https://github.com/napalm-automation/napalm-base/pull/233#issuecomment-292147620

Pinging some people that might be interested on this discussion: @napalm-automation/council @manuel-domke @bewing

mirceaulinic commented 7 years ago

I have commented under https://github.com/napalm-automation/napalm-base/pull/233#issuecomment-295618802 -- I don't see (1) as a sane solution. I tend to consider (2) more suitable.

dbarrosop commented 7 years ago

(answering to your comment here instead of on the other thread)

remove an useful functionality

Nobody wants to remove anything, just move it around.

why not just implement ping_vrf, ping_ttl

Easy to answer that and I will just refer to the code:

https://github.com/napalm-automation/napalm-eos/blob/develop/napalm_eos/eos.py#L1054

Protocol is a switch to go to a branch of the code that contains ~100 lines of code. Adding a new protocol implies branching again and executing 100 other different lines of code.

In any case, please, read point 1 again. Nobody mentioned removing anything, jut moving get_route_to to napalm-base and have it call different methods for each individual protocol. Something like:

# This is not working code, just for reference
def get_route_to(self, protocol='', ...):
    if protocol in ["direct", "bgp", "ospf", "static"...]:
        method = getattr(self, "get_route_to_{}".format(protocol))
        return method(...)
    elif protocol == "":
       # code to call all of getters and ignore notimplementederrors
   else:
      # fail as protocol is incorrect

And then each driver just has to implement get_route_to_static, get_route_to_bgp, etc. which serves as implementation and documentation. You can refer to the docs and see that get_route_ospf is not implemented and thus it won't work. Right now it's just ignored as you suggested.

mirceaulinic commented 7 years ago

Thanks for expanding on this @dbarrosop - indeed, I misread the comments above, apologies.

Makes sense to me, however it may take longer to get this done :)

dbarrosop commented 7 years ago

Makes sense to me, however it may take longer to get this done :)

Agreed, but hopefully things will be easier in the future so we may save time in the long run.

madhurabhogate commented 6 years ago

I had an issue if the route installed was through ibgp then it used to error out as -

line 1071, in get_route_to remote_as = int(as_path.split()[-1]) IndexError: list index out of range