napalm-automation / napalm-nxos

Apache License 2.0
9 stars 21 forks source link

Updates #55

Closed GGabriele closed 7 years ago

GGabriele commented 7 years ago

@dbarrosop as anticipated, I'm migrating NXOS driver from pycsco to pynxos. Here I'm also addressing #54 , #52 , #51 and #30 .

All the config management tests are passing but I'm definitely missing something on the new test framework for getters. The error I get is always likes Failed: No test case for 'test_get_snmp_information' found.

I've lately included these lines on conftest.py.

    def __init__(self):
        full_path = self.find_file('facts.json')
        self.facts = self.read_json_file(full_path)

I did so because the way I'm getting facts is through facts attribute from the pynxos object.

    def get_facts(self):
        pynxos_facts = self.device.facts

Anyway, when I do so the error becomes this:

self = <test.unit.conftest.FakeNXOSDevice object at 0x7f055be6d950>, filename = 'facts.json'

    def find_file(self, filename):
        """Find the necessary file for the given test case."""
        # Find base_dir of submodule
        module_dir = os.path.dirname(sys.modules[self.__module__].__file__)

        full_path = os.path.join(module_dir, 'mocked_data',
>                                self.current_test, self.current_test_case, filename)
E       AttributeError: 'FakeNXOSDevice' object has no attribute 'current_test'

/usr/local/lib/python2.7/dist-packages/napalm_base/test/double.py:27: AttributeError

Any tips to overcome these?

mirceaulinic commented 7 years ago

Oh my! What a lotta changes! :)

mirceaulinic commented 7 years ago

I added 100 labels

mirceaulinic commented 7 years ago

@GGabriele could you please describe briefly the differences between pycsco and pynxos?

GGabriele commented 7 years ago

@mirceaulinic probably the main difference is that pycsco uses NXAPI while pynxos uses RPC, dealing always with JSON output instead of XML, returning always the actual output body. Plus I think @jedelman8 doesn't maintain pycsco that much anymore.

mirceaulinic commented 7 years ago

Thanks! This sounds really good! Any idea if this would require a newer OS version (currently http://napalm.readthedocs.io/en/latest/support/index.html#general-support-matrix says that min is 6.1)?

GGabriele commented 7 years ago

I have no idea honestly, I'll do some search on this.

GGabriele commented 7 years ago

@dbarrosop any tips about this last fail ontest_method_signatures ?

jedelman8 commented 7 years ago

One clarification

probably the main difference is that pycsco uses NXAPI while pynxos uses RPC, dealing always with JSON output instead of XML, returning always the actual output body. Plus I think @jedelman8 doesn't maintain pycsco that much anymore.

Both use NX-API. pycsco supports XML and JSON encoding and pycsco was built using the latest (3rd) encoding type NX-OS supports, i.e. JSON-RPC

Any idea if this would require a newer OS version (currently http://napalm.readthedocs.io/en/latest/support/index.html#general-support-matrix says that min is 6.1)?

Great question...it could newer as JSON-RPC came after XML/JSON. Not sure though.

ktbyers commented 7 years ago

@GGabriele New test framework requires all non-standard NAPALM methods to be private methods. In other words, if it is not a method defined in napalm-base, then it needs to be private.

get_checkpoint_file

Needs to become:

_get_checkpoint_file

Obviously, this will be a backwards incompatible change, but now is a good time to do it (since you are converting over to pynxos).

coveralls commented 7 years ago

Coverage Status

Changes Unknown when pulling acf8a31a2b23446d4acd56a3a90711b47aa01d8d on GGabriele:pynxos into on napalm-automation:develop.

GGabriele commented 7 years ago

@ktbyers thanks!

ktbyers commented 7 years ago

@GGabriele FYI, fix for your Python3 issues is:

from napalm_base.utils import py23_compat

Then where ever you used unicode, use:

py23_compat.text_type

For example:

'os_version': py23_compat.text_type(os_version),

This is because in Python3 a unicode string is just called an 'str' (and there is no built-in called unicode).

GGabriele commented 7 years ago

@ktbyers thanks again!

coveralls commented 7 years ago

Coverage Status

Changes Unknown when pulling 2f3e11fd9d42dcdbab439e14c4a0a8407f6fc498 on GGabriele:pynxos into on napalm-automation:develop.

coveralls commented 7 years ago

Coverage Status

Changes Unknown when pulling 2f3e11fd9d42dcdbab439e14c4a0a8407f6fc498 on GGabriele:pynxos into on napalm-automation:develop.

ktbyers commented 7 years ago

@GGabriele Nice!

dbarrosop commented 7 years ago

This is great! : )

+100

Can we find out if we have you update the documentation? New req. min. version and/or if some of the matrixes/caveats have to be updated? Once a PR with those potential updates are ready we can merge this one and release : )

mirceaulinic commented 7 years ago

I've just noticed another thing on get_interfaces it always says that it's down:

>>> n.get_interfaces()['Ethernet1/3']
{u'is_enabled': True, u'description': u'', u'last_flapped': 1467796042.406583, u'is_up': False, u'mac_address': u'00:2A:6A:F5:AE:2A', u'speed': 10000000000}

Although on the device:

sw01.fco01# sh int Ethernet1/3
Ethernet1/3 is up
 Dedicated Interface

  Belongs to Po22
mirceaulinic commented 7 years ago

get_lldp_neighbors fails because Cisco:

sw01.fco01# sh lldp neighbors | json
ERROR: xml source -- not well-formed (invalid token): line 11, column 17

But detail works:

sw01.fco01# sh lldp neighbors detail | json
{
  "neigh_hdr": "neigh_hdr",
  "TABLE_nbor_detail": {
    "ROW_nbor_detail": [
      {
        "chassis_type": 4,
        "chassis_id": "047d.7ba5.843c",

Where is the facepalm emoji when you need it?

mirceaulinic commented 7 years ago

One more little detail:

{u'is_enabled': False, u'description': u'"Host: reserved"', u'last_flapped': -1.0, u'is_up': False, u'mac_address': u'00:00:00:00:00:00', u'speed': 100000000}

The description is wrapped around double quotes - would be good to remove then.

coveralls commented 7 years ago

Coverage Status

Changes Unknown when pulling 60992ecc885d67942c6fb43f0417576476fb794f on GGabriele:pynxos into on napalm-automation:develop.

GGabriele commented 7 years ago

@mirceaulinic thanks for reviewing this.

>>> device.get_interfaces()['Ethernet2/1']
{u'is_enabled': True, u'description': u'test', u'last_flapped': 1481889789.555331, u'is_up': True, u'mac_address': u'2C:C2:60:4F:FE:B2', u'speed': 1000}

I fixed speed and description formatting. I don't know why, but as you can see here u'is_up is True and last_flapped is not -1.0 (I haven't changed the way it's retrieved). Can you check your sh int Ethernet1/3 | json output?

>>> device.get_facts()
{'uptime': 3281, u'vendor': u'Cisco', u'hostname': 'nxos-spine1', 'fqdn': 'nxos-spine1.test', u'os_version': '7.3(1)D1(1) [build 7.3(1)D1(0.10)]', u'serial_number': 'TM6017D760B', u'model': 'NX-OSv Chassis', u'interface_list': ['mgmt0', 'Ethernet2/1', 'Ethernet2/2', 'Ethernet2/3', 'Ethernet2/4', 'Ethernet2/5', 'Ethernet2/6', 'Ethernet2/7', 'Ethernet2/8', 'Ethernet2/9', 'Ethernet2/10', 'Ethernet2/11', 'Ethernet2/12', 'Ethernet2/13', 'Ethernet2/14', 'Ethernet2/15', 'Ethernet2/16', 'Ethernet2/17', 'Ethernet2/18', 'Ethernet2/19', 'Ethernet2/20', 'Ethernet2/21', 'Ethernet2/22', 'Ethernet2/23', 'Ethernet2/24', 'Ethernet2/25', 'Ethernet2/26', 'Ethernet2/27', 'Ethernet2/28', 'Ethernet2/29', 'Ethernet2/30', 'Ethernet2/31', 'Ethernet2/32', 'Ethernet2/33', 'Ethernet2/34', 'Ethernet2/35', 'Ethernet2/36', 'Ethernet2/37', 'Ethernet2/38', 'Ethernet2/39', 'Ethernet2/40', 'Ethernet2/41', 'Ethernet2/42', 'Ethernet2/43', 'Ethernet2/44', 'Ethernet2/45', 'Ethernet2/46', 'Ethernet2/47', 'Ethernet2/48', 'Ethernet3/1', 'Ethernet3/2', 'Ethernet3/3', 'Ethernet3/4', 'Ethernet3/5', 'Ethernet3/6', 'Ethernet3/7', 'Ethernet3/8', 'Ethernet3/9', 'Ethernet3/10', 'Ethernet3/11', 'Ethernet3/12', 'Ethernet3/13', 'Ethernet3/14', 'Ethernet3/15', 'Ethernet3/16', 'Ethernet3/17', 'Ethernet3/18', 'Ethernet3/19', 'Ethernet3/20', 'Ethernet3/21', 'Ethernet3/22', 'Ethernet3/23', 'Ethernet3/24', 'Ethernet3/25', 'Ethernet3/26', 'Ethernet3/27', 'Ethernet3/28', 'Ethernet3/29', 'Ethernet3/30', 'Ethernet3/31', 'Ethernet3/32', 'Ethernet3/33', 'Ethernet3/34', 'Ethernet3/35', 'Ethernet3/36', 'Ethernet3/37', 'Ethernet3/38', 'Ethernet3/39', 'Ethernet3/40', 'Ethernet3/41', 'Ethernet3/42', 'Ethernet3/43', 'Ethernet3/44', 'Ethernet3/45', 'Ethernet3/46', 'Ethernet3/47', 'Ethernet3/48', 'Ethernet4/1', 'Ethernet4/2', 'Ethernet4/3', 'Ethernet4/4', 'Ethernet4/5', 'Ethernet4/6', 'Ethernet4/7', 'Ethernet4/8', 'Ethernet4/9', 'Ethernet4/10', 'Ethernet4/11', 'Ethernet4/12', 'Ethernet4/13', 'Ethernet4/14', 'Ethernet4/15', 'Ethernet4/16', 'Ethernet4/17', 'Ethernet4/18', 'Ethernet4/19', 'Ethernet4/20', 'Ethernet4/21', 'Ethernet4/22', 'Ethernet4/23', 'Ethernet4/24', 'Ethernet4/25', 'Ethernet4/26', 'Ethernet4/27', 'Ethernet4/28', 'Ethernet4/29', 'Ethernet4/30', 'Ethernet4/31', 'Ethernet4/32', 'Ethernet4/33', 'Ethernet4/34', 'Ethernet4/35', 'Ethernet4/36', 'Ethernet4/37', 'Ethernet4/38', 'Ethernet4/39', 'Ethernet4/40', 'Ethernet4/41', 'Ethernet4/42', 'Ethernet4/43', 'Ethernet4/44', 'Ethernet4/45', 'Ethernet4/46', 'Ethernet4/47', 'Ethernet4/48', 'Vlan1']}

I've fixed the fqdn thing...also, if for any reason interface_list is empty the code will do a double check using get_interfaces()

        if pynxos_facts['interfaces']:
            final_facts['interface_list'] = pynxos_facts['interfaces']
        else:
            final_facts['interface_list'] = self.get_interfaces().keys()

get_lldp_neighbors uses structured data, while get_lldp_neighbors_detail uses raw text (seems that some old devices may not return JSON output...)...I've added a template for get_lldp_neighbors to use raw data as well.

coveralls commented 7 years ago

Coverage Status

Changes Unknown when pulling 957f98f2d4a93ca99ba4005876bfddf4b85ad7ef on GGabriele:pynxos into on napalm-automation:develop.

coveralls commented 7 years ago

Coverage Status

Changes Unknown when pulling 957f98f2d4a93ca99ba4005876bfddf4b85ad7ef on GGabriele:pynxos into on napalm-automation:develop.

GGabriele commented 7 years ago

@dbarrosop I'm trying to find some info about min. version for the doc :)

mirceaulinic commented 7 years ago

I fixed speed and description formatting

Great, thanks!

If for any reason interface_list is empty the code will do a double check using get_interfaces()

Sounds like a reasonable solution for me, for the moment. I think we should also dig into pynxos and see why it returns empty list.

get_lldp_neighbors uses structured data, while get_lldp_neighbors_detail uses raw text (seems that some old devices may not return JSON output...)...I've added a template for get_lldp_neighbors to use raw data as well.

Not quite optimal, but thanks Cisco for this. I tested and... it kida works. Not your fault at all, but I will file a bug later as this may be more specific to my environment and the display is distorted - something pretty close to https://github.com/napalm-automation/napalm-ios/issues/74#issue-192476922, which is very disappointing.

I fixed speed and description formatting. I don't know why, but as you can see here u'is_up is True and last_flapped is not -1.0 (I haven't changed the way it's retrieved). Can you check your sh int Ethernet1/3 | json output?

Sure, here you go:

>>> n.get_interfaces()['Ethernet1/3']
{u'is_enabled': True, u'description': u'', u'last_flapped': 1467816669.670505, u'is_up': False, u'mac_address': u'00:2A:6A:F5:AE:2A', u'speed': 10000}
sw01.fco01# sh int Ethernet1/3 | json
{
  "TABLE_interface": {
    "ROW_interface": {
      "interface": "Ethernet1/3",
      "state": "up",
      "share_state": "Dedicated",
      "eth_bundle": "Po22",
      "eth_hw_desc": "1000/10000 Ethernet",
      "eth_hw_addr": "002a.6af5.ae2a",
      "eth_bia_addr": "002a.6af5.ae2a",
      "eth_mtu": "1500",
      "eth_bw": [
        "10000000",
        "10000000"
      ],
      "eth_dly": 10,
      "eth_reliability": "255",
      "eth_txload": "1",
      "eth_rxload": "1",
      "medium": "broadcast",
      "eth_mode": "trunk",
      "eth_duplex": "full",
      "eth_speed": "10 Gb/s",
      "eth_media": "10G",
      "eth_beacon": "off",
      "eth_in_flowctrl": "off",
      "eth_out_flowctrl": "off",
      "eth_ratemode": "dedicated",
      "eth_swt_monitor": "off",
      "eth_ethertype": "0x8100",
      "eth_link_flapped": "23week(s) 2day(s)",
      "eth_clear_counters": "never",
      "eth_reset_cntr": 1,
      "eth_load_interval1_rx": 30,
      "eth_inrate1_bits": 64,
      "eth_inrate1_pkts": 0,
      "eth_load_interval1_tx": 30,
      "eth_outrate1_bits": 1144,
      "eth_outrate1_pkts": 0,
      "eth_inucast": 0,
      "eth_inmcast": 942951,
      "eth_inbcast": 234858,
      "eth_inpkts": 1177809,
      "eth_inbytes": 228826458,
      "eth_jumbo_inpkts": 0,
      "eth_storm_supp": 0,
      "eth_runts": 0,
      "eth_giants": 0,
      "eth_crc": 0,
      "eth_nobuf": 0,
      "eth_inerr": 0,
      "eth_frame": 0,
      "eth_overrun": 0,
      "eth_underrun": 0,
      "eth_ignored": 0,
      "eth_watchdog": 0,
      "eth_bad_eth": 0,
      "eth_bad_proto": 0,
      "eth_in_ifdown_drops": 0,
      "eth_dribble": 0,
      "eth_indiscard": 0,
      "eth_inpause": 0,
      "eth_outucast": 187,
      "eth_outmcast": 14856380,
      "eth_outbcast": 6650705,
      "eth_outpkts": 21507272,
      "eth_outbytes": 2472479119,
      "eth_jumbo_outpkts": 0,
      "eth_outerr": 0,
      "eth_coll": 0,
      "eth_deferred": 0,
      "eth_latecoll": 0,
      "eth_lostcarrier": 0,
      "eth_nocarrier": 0,
      "eth_babbles": 0,
      "eth_outdiscard": 0,
      "eth_outpause": 0
    }
  }
}

Indeed the last flapped is not always -1, my bad.

GGabriele commented 7 years ago

@mirceaulinic No admin_state key on that json :\

mirceaulinic commented 7 years ago

@GGabriele None of the devices I've checked have it - and there are quite a few. Basically it says that most of the switches from my network have all their interfaces down :)

Any idea version do I need to get the right data? I have 7.1(0)N1(1a) and 7.3(0)N1(1) available which I understand are among the newest (http://www.cisco.com/c/en/us/td/docs/switches/datacenter/nexus5600/sw/release/notes/7x/Nexus5600_Release_Notes_7x.html). Isn't there any other field that could provide this info instead?

GGabriele commented 7 years ago

@mirceaulinic Thanks Cisco :)

I'm testing this with N7K version 7.3(1)D1(1) and N9K version 6.1(2)I3(1) and they both have this key. I think no other key can be useful for this :\ the only thing I can do is use raw text again.

mirceaulinic commented 7 years ago

I think no other key can be useful for this :\ the only thing I can do is use raw text again.

No, please don't do this.

If we leave it like that it returns always false negatives (says the interfaces are down, although they are not). Although not correct either, when the field in cause is not there, we could probably accept to simply return the same info as in is_enabled, which sometimes would return false positives...

coveralls commented 7 years ago

Coverage Status

Changes Unknown when pulling 8de9f3712ad6fabd92d491b0cc24bceecf152fd1 on GGabriele:pynxos into on napalm-automation:develop.

coveralls commented 7 years ago

Coverage Status

Changes Unknown when pulling 8de9f3712ad6fabd92d491b0cc24bceecf152fd1 on GGabriele:pynxos into on napalm-automation:develop.

coveralls commented 7 years ago

Coverage Status

Changes Unknown when pulling 8de9f3712ad6fabd92d491b0cc24bceecf152fd1 on GGabriele:pynxos into on napalm-automation:develop.

GGabriele commented 7 years ago

@mirceaulinic agreed. I've just pushed that fix

mirceaulinic commented 7 years ago

Ooops the commit is broken:

>>> n.commit_config()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/state/home/mircea/napalm-nxos/napalm_nxos/nxos.py", line 303, in commit_config
    self.save_config(self.backup_file)
AttributeError: 'NXOSDriver' object has no attribute 'save_config'
>>>
coveralls commented 7 years ago

Coverage Status

Changes Unknown when pulling f880ab0ac2c9b1cde44641e50749b68c0577aee8 on GGabriele:pynxos into on napalm-automation:develop.

mirceaulinic commented 7 years ago

Sorry @GGabriele by mistake I pushed this commit into your PR: https://github.com/napalm-automation/napalm-nxos/pull/55/commits/f880ab0ac2c9b1cde44641e50749b68c0577aee8 when testing it - hope you don't mind. If a problem, I'll revert it.

mirceaulinic commented 7 years ago

Even with this change, it still doesn't save the config changes.

>>> n.load_merge_candidate(config='ntp peer 172.17.17.1')
>>> n.compare_config()
u'ntp peer 172.17.17.1'
>>> n.commit_config()
>>>
>>>
>>>
>>> n.compare_config()
u''

No error, everything looks good, but the config is not there:

sw01.sfo04# sh run ntp

!Command: show running-config ntp
!Time: Fri Aug  2 12:40:54 2002

version 7.3(0)N1(1)
ntp distribute

sw01.sfo04#
sw01.sfo04# sh start ntp

!Command: show startup-config ntp
!Time: Fri Aug  2 12:41:35 2002
!Startup config saved at: Fri Aug  2 12:31:03 2002

version 7.3(0)N1(1)
ntp distribute

sw01.sfo04#

@GGabriele please make some extensive checks on this before merging. Thanks!

GGabriele commented 7 years ago

@mirceaulinic mmmh interesting, I see this:

>>> device.load_merge_candidate(config='ntp peer 172.17.17.1')
>>> device.commit_config()
>>> 
nxos-spine1# show run ntp

!Command: show running-config ntp
!Time: Mon Dec 19 07:51:30 2016

version 7.3(1)D1(1)
ntp peer 172.17.17.1

nxos-spine1#
mirceaulinic commented 7 years ago

That's very odd...

coveralls commented 7 years ago

Coverage Status

Changes Unknown when pulling ea2427efa1ca828bbd48d8a8b2fcc9f33b949758 on GGabriele:pynxos into on napalm-automation:develop.

coveralls commented 7 years ago

Coverage Status

Changes Unknown when pulling ea2427efa1ca828bbd48d8a8b2fcc9f33b949758 on GGabriele:pynxos into on napalm-automation:develop.

coveralls commented 7 years ago

Coverage Status

Changes Unknown when pulling ea2427efa1ca828bbd48d8a8b2fcc9f33b949758 on GGabriele:pynxos into on napalm-automation:develop.

GGabriele commented 7 years ago

@mirceaulinic I did my tests on 7K and 9K...today I'll try to do the same on 5K too.

GGabriele commented 7 years ago

@mirceaulinic I've just tested with a 3K version 6.0(2)U4(1) and a 5K version 7.2(1)N1(1) and everything looks good

mirceaulinic commented 7 years ago

Thanks for looking @GGabriele!

OK - we can track this later. FTM just please fix this: https://github.com/napalm-automation/napalm-nxos/pull/55#discussion_r92941105 and we can merge!

GGabriele commented 7 years ago

@mirceaulinic I already did it with the last commit :)

mirceaulinic commented 7 years ago

For reference, I submitted this bug: https://github.com/napalm-automation/napalm-nxos/issues/59 regarding the configuration issue from above.