[WIP] Show IP of all VRF on nxos

[aruhier]

If a VRF is not specified in the nxos command, it will only show the default vrf IP. As the vrf cannot be specified in the method, now it doesn't restrict the result to one vrf.

The SSH parser needed to be fixed to allow multiple VRF

[coveralls]

Coverage remained the same at ?% when pulling 318a436 on Anthony25:nxos_vrf_all into 5610fdc on napalm-automation:develop.

[aruhier]

I will update your tests ASAP

[ktbyers]

Yes, I think that probably won't work. I think the fix is probably to accept VRF as an argument to the getter and then return only that VRF (and have it default to global/default.

Adding @bewing into this discussion also.

This is a more general problem than this particular getter so we should probably discuss the more general issue (i.e. how do we want to approach this problem in general across several getters).

[aruhier]

I can work on JunOS, iosxr, ios and nxos to implement an optional vrf parameter, if it can help, as I have some devices to test it. I don't have any Arista device, on the other hand.

[ktbyers]

Let's see if @bewing has an opinion, then we figure out how to go about it and get it done.

[ktbyers]

Okay, I think we should just start to move forward on this.

I think we should add a vrf=None argument to the spec for all the drivers. And then have the default behavior in this case just be the current behavior.

We will then need to start adding VRF support to each of the five core platforms (and have it fail if someone passes in a VRF and VRF support has yet to be added).

We will also need to consider what to do / how to handle the 'all' VRFs case.

[ktbyers]

@Anthony25 Does that sound reasonable?

[FloLaco]

We need to think about all modifications that we need to do in order to support an argument.
For example, how to use ˋcompliance_report` or ˋnapalm_validateˋ.
I’ve done some work gor handle multiple VRF on my own repo (branch PRODC4 https://github.com/FloLaco/napalm/tree/PRODC4?files=1)

[aruhier]

@Anthony25 Does that sound reasonable?

Sure, I thought about that before doing this PR, so we ended up on the same idea :)

[ktbyers]

@FloLaco Can you link to something specific where you added VRF support to an existing getter. I looked quickly in a compare of your repo and didn't see anything that jumped out at me.

[FloLaco]

This (in PRODC4 branch, not the develop): https://github.com/FloLaco/napalm/blob/e84913974b1933b2b0c6de111ae98f54f8592b45/napalm/nxos/nxos.py#L591

https://github.com/FloLaco/napalm/blob/e84913974b1933b2b0c6de111ae98f54f8592b45/napalm/nxos/nxos.py#L831

I’ve also added some new getters but it’s out of this PR.

[bewing]

I took a look at the EOS implementation of get_interfaces_ip(), and it actually returns all interfaces in all VRFs already, so implementing a VRF flag for the method might be a breaking change.

The other alternative would be to update the output of the function to include the VRF of each interface?

I do note that the fix for get_arp_table #512 was to default to the main VRF, and we specifically commented that users should leverage get_network_instances() to loop through the tables.

[FloLaco]

On my own multi vrf implementation in NXOS I’ve done like EOS, meaning that I’ve added the ˋvrf all`

[ktbyers]

I somewhat think we should add the vrf='foo' argument and try to implement that (including support for vrf='all'). After we get that implemented we can decide if we want to change the default argument to be vrf='all'.

I like this as it provides us an incremental way of getting it done platform by platform which makes it less likely to get stuck and never completed (which frequently happens if we have changes that require all core drivers to be updated).

I am open to disagreement though :-)

[FloLaco]

Right now, if we check some methods already implemented, the vrf all already exist in some cases (get_bgp_neighbors and get_bgp_neighbors_detail), so we have to change it a bit to fit the version with an argument. So why not making changes with the vrf all in the first step and then iterate it with an argument ?

But first of all, because I don’t know about junos, are they supporting the vrf all natively ?

[aruhier]

For what I've seen, on JunOS by default they show the result for all vrf.

[FloLaco]

Thanks.
So IMHO, first iteration is easier if we support all vrf by default. The second one would be to support a vrf provided by user. What do you think about it ?

[aruhier]

Yep, seems good to me. That's what I had in mind with this PR, actually, because some platforms already support all vrf by default.

[ktbyers]

Current behavior:

base Ambiguous as to what behavior should be
eos ALL VRFs
ios ALL VRFS
nxos/nxos_ssh Default VRF
iosxr A bit strange looking at the code--IPv4 looks like probably all; IPv6 looks like Global VRF only (this would need looked at more).
junos Probably all

[ktbyers]

@FloLaco I didn't understand this statement you made?

Right now, if we check some methods already implemented, the vrf 
all already exist in some cases (get_bgp_neighbors and get_bgp_neighbors_detail)
so we have to change it a bit to fit the version with an argument.

[ktbyers]

Here are three options:

1. get_interfaces_ip() always returns all VRFs. Is unaware of VRFs. Would have to fix nxos/nxos_ssh and maybe iosxr IPv6.
2. Have a VRF argument with default probably being all since most of the platforms already return all. Return the same data structure, however, i.e. no reference to VRF in the returned data structure.
3. Change the returned data structure and indicate the VRF that interface IPs are associated with. VRF would probably be the key.

What are people's preferences?

The only one I don't like is 3 (mainly because it is a lot more work and based on past experience I somewhat doubt it would get completed).

[FloLaco]

I was just saying that some methods already return all vrf. So if we need to change something in the project should be the remaining methods that does not support vrf right now and make it as the same as the 2 examples I gives (for example).

[bewing]

I'm in favor of option 2. We should probably define some constants for VRF_ALL and VRF_DEFAULT, however?

[ktbyers]

@bewing Can you expand on your constant discussion that we were having via Slack. Just to make sure we are on the same page here.