search

napalm-automation / napalm-base

Apache License 2.0
32 stars 48 forks source link

Enhancement: Add vrf and neighbor_address arguments for get_bgp_neighbors and get_bgp_neighbors_detail #105

Open mirceaulinic opened 8 years ago

mirceaulinic commented 8 years ago

Without changing the current behaviour, the argument would increase the performances of the get_bgp_neighbors method. If not passed or empty/null, will return everything. Of course we can retrieve everything and filter, but this is not optimal when there's a very high number of neighbors and you need the operational data for a specific peer only:

PoC:

Without neighbor_address:

>>> start = time.time(); w = e.get_bgp_neighbors(); print time.time() - start
8.1069419384

With neighbor_address:

>>> start = time.time(); w = e.get_bgp_neighbors('X.X.X.X'); print time.time() - start
0.508651018143
dbarrosop commented 8 years ago

I guess that should will have to specify both the VRF and the IP you want to filter on. Other than that tiny detail, sounds good to me.

mirceaulinic commented 8 years ago

Updated the title. @dbarrosop can you please spread this issue on junos, iosxr and eos drivers? I still have some problems with the napalm-tools although I set the env token...

mirceaulinic commented 8 years ago

And actually I think we should add the vrf arg also for get_bgp_neighbors_detail

dbarrosop commented 8 years ago

still have some problems with the napalm-tools although I set the env token

Let's take a look tomorrow and try to fix it tomorrow. It should work : (

And actually I think we should add the vrf arg also for get_bgp_neighbors_detail

That would break the API and I'd rather not. Hopefully we will have a method like that based on YANG models soon.

mirceaulinic commented 8 years ago

That would break the API and I'd rather not.

What would be the difference - I think it would make more sense to have both consistent:

get_bgp_neighbors(vrf=None, neighbor_address=None)

and 

get_bgp_neighbors_detail(vrf=None, neighbor_address=None)

doesn't it?

dbarrosop commented 8 years ago

Well, what do we do today. Return from global? Return all of the neighbors regarding their VRF? I'd actually replace vrf=None with vrf="global".

mirceaulinic commented 8 years ago

Well, what do we do today. Return from global?

No, from all VRFs.

I'd actually replace vrf=None with vrf="global".

If we do this, by default will retrieve data only from the default VRF, which means will change the current behaviour, hence will break the API.

dbarrosop commented 8 years ago

Mmmm, ok, let's do it then.