IOS driver get_network_instances fix

[chonty]

Corrects the IOS driver's get_network_instances output.

Problems addressed:

1. on my test devices (IOS and IOS-XE), show vrf detail returns abbreviated interface names. This uses the canonical_interface_name function to expand them.
2. the function adds all interfaces to the default instance resulting in an interface appearing in default + vrf xyz in Napalm's output. This change removes them from default.
3. base/canonical_map.py maps V* to VLAN instead of Vlan which breaks when removing interfaces from default (above + example error below). IOS SVIs are named Vlan123 and this caters for that. Not sure if it's better to update the mapping or to pass addl_name_map into canonical_interface_name.

example of error covered in point 3:

  File "/usr/local/lib/python3.6/dist-packages/napalm/ios/ios.py", line 3333, in get_network_instances
    del instances['default']['interfaces']['interface'][item]
KeyError: 'VLAN666'

Vl666 expands to VLAN666 and VLAN666 != Vlan666:

R5#sh ip int bri
Interface              IP-Address      OK? Method Status                Protocol
Vlan666                66.66.66.66     YES manual up                    up

R5#sh vrf detail
VRF test (VRF Id = 3); default RD <not set>; default VPNID <not set>
  New CLI format, supports multiple address-families
  Flags: 0x1808
  Interfaces:
    Lo500                    Vl666                    Vl900                         

[mirceaulinic]

I think this looks good, but please add a test case for this change. Alas, the following checks are failing: black and pylama. Could you take a look? Thanks!

[coveralls]

Coverage remained the same at 0.0% when pulling cb09cbf on chonty:ios-get_network_instances into ce790e3 on napalm-automation:develop.

[chonty]

Sure. I'll familiarise myself and have a look.

[mirceaulinic]

FYI: we are planning to release NAPALM 3.0.0 soon. If you'd like your patch to be included in this version, please address the comments above. Thanks!

[mirceaulinic]

@chonty were you going to revisit this? Thanks!

[chonty]

closing it was the boot up the arse I needed. Tests added and an unhandled error when no VRFs present was fixed. Can you re-open and review please?

[mirceaulinic]

Sure thing, @chonty. I'll reopen, then you can push your changes. Cheers!

[chonty]

Changes pushed. Does this mean the tests were successful?

[chonty]

My repo is based on 2.something and I didn't notice that a54ceba was already fixed in #938 and with an error case I didn't cover.

[mirceaulinic]

Looks good to me, and the tests are passing. I only have one suggestion, then this should be good to go.

[mirceaulinic]

I'd ask you to refrain from changing the existing test-cases, but just add new ones (as you already did). In other words, please keep the no_vrf case and revert the changes under normal. Of course, if there's anything incorrect with the existing data, please clarify here.

[chonty]

Sure. I just re-read the PR description and the test data I threw together was irrelevant anyway. New test cases incoming.

[chonty]

Relevant test cases added and I dirty reverted the originals by copying them and pushing them again with 4dff67c. Hopefully that's not a problem.

[chonty]

made a dog's breakfast out of that. The normal test case is now failing in a way I don't understand:

AssertionError: Expected result varies on some keys 

{
    "default": {
        "interfaces": {
            "interface": {
                "GigabitEthernet0": {
                    "result": null,
                    "expected": {}
                }
            }
        }
    },
    "Mgmt-intf": {
        "interfaces": {
            "interface": {
                "Gi0": {
                    "result": null,
                    "expected": {}
                },
                "GigabitEthernet0": {
                    "result": {},
                    "expected": null
                }
            }
        }
    }
}

and the data is actually invalid. GigabitEthernet0 appears in both default and Mgmt-intf instances which this PR corrects.

[mirceaulinic]

No problem at all, I don't mind what method you use as long as we get to there ;)
Looks like the tests are failing on the normal test case now, I think you can just copy the content from https://github.com/napalm-automation/napalm/tree/develop/test/ios/mocked_data/test_get_network_instances/normal and should be fine.

[chonty]

The tests were failing with https://github.com/napalm-automation/napalm/tree/develop/test/ios/mocked_data/test_get_network_instances/normal for some reason and the data was invalid anyway because Gi0 in Mgmt-intf unwraps to GigabitEthernet0 which also appears in the default instance. Replaced the test case with my new one.

[mirceaulinic]

Okay, makes sense