Skip to content
This repository has been archived by the owner on Sep 17, 2019. It is now read-only.

Fix get_route_to or fix tests #238

Open
dbarrosop opened this issue Apr 16, 2017 · 5 comments
Open

Fix get_route_to or fix tests #238

dbarrosop opened this issue Apr 16, 2017 · 5 comments
Labels
Milestone

Comments

@dbarrosop
Copy link
Member

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: Test case for static routes (v4+v6) #233 (comment)

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

@mirceaulinic
Copy link
Member

mirceaulinic commented Apr 20, 2017

I have commented under #233 (comment) -- I don't see (1) as a sane solution. I tend to consider (2) more suitable.

@mirceaulinic mirceaulinic added this to the DISCUSSION milestone Apr 20, 2017
@dbarrosop
Copy link
Member Author

dbarrosop commented Apr 21, 2017

(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
Copy link
Member

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
Copy link
Member Author

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
Copy link

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

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

No branches or pull requests

3 participants