add canonical_interface #493

itdependsnetworks · 2017-11-06T04:06:35Z

Added canonical_interface and updated some linter complaints when I first had everything in string_parsers.py.

Didn't want to go to crazy until I had consensus on naming and implementation.

dbarrosop · 2017-11-06T07:47:22Z

napalm/base/helpers.py

+    Function to retun interface canonical name
+    This puposely does not use regex, or first X characters, to ensure
+    there is no chance for false positives. As an example, Po = PortChannel, and
+    PO = POS. With either character or regex, that would produce a false positive.


regexes are case sensitive so po.* and Po.* are not going to cause any false positives. regexes has the advantage the code becomes simpler and more reliable as it should work if cisco in their next version decide to come up with yet another combination of short letters.

I am usually regex heavy, but having been bit by this issue nearly a decade ago, I am cautious about it. As an example, there is Virtual-Access and Virtual-Template, the idea of getting this right the first time, without being explicit does not seem likely.

This should be helpful only, and like I said, knowing the pain it has caused me before, I try and do as much as possible to to avoid false positive's.

dbarrosop · 2017-11-06T07:47:57Z

I have a question regarding this PR:

Does cisco only hire sociopaths that hate people and deliberately make things horrible so they are harder to consume?

dbarrosop · 2017-11-06T07:54:26Z

napalm/base/interface_map.py

+        ],
+        "VLAN_short": "Vl"
+      },
+      "os_map": {


Is there any reason to break this down by platform? Why not just have a single mapping for all?

dbarrosop · 2017-11-06T08:08:09Z

I am a bit confused by the implementation. I might be missing something but I am not sure why:

we have to split this up by interface.
we have such a complicated dictionary.

I think this could be extremely simplified by doing something like:

def canonical_interface(interface, device_os, short=False):
    mapping = {
        'Po': 'Port-channel',
        'PO': 'POS',
        'Fa': 'FastEthernet',
        'Gi': 'GigabitEthernet',
        'Te': 'TenGigabitEthernet',
        ...
    }
    if short:
        interface_type = re.match('(\w+)\d.*', interface_name).groups(0)
        mapping = {v: k for k, v in map.items()}
    else:
        interface_type = interface_name[0:2]

    return mapping[interface_type]

Unless I am missing something, of course.

mirceaulinic · 2017-11-06T09:46:40Z

Under napalm-automation/hackathon-2017#5 I had the feeling we have agreed this would be a method defined under each driver. Of course I wouldn't have a problem having this helper, but this way we'd solve a couple of things:

Avoids having such a complicated dictionary. (David's comment above) Instead, each driver keeps its own mapping.
You don't need to pass explicitly the OS -- it's more uniform to call driver. canonical_interface than canonical_interface(..., 'ios'), as that's already there, which will be useful for various things.
Solve platform / os / minor release version / other stupid differences locally, inside the driver. We'll probably need sooner or later to check if you're running a certain hardware/software platform combination to determine the right name, and inside the driver instance you already have these details.

dbarrosop · 2017-11-06T10:20:31Z

If there is a need for separation between OSes I agree with @mirceaulinic in that the mappings should live in each specific driver but I don't really see why we would need this separation so we could have the mapping in the base driver and override if necessary.
Regarding the API, I think we agreed on two things:
a. Specific method for this. I.e. device.get_canonical_interface_name('et1')
b. A driver-wide optional arg that defaults to False and that tells the driver to return the canonical name instead of whatever the vendor provided.

itdependsnetworks · 2017-11-06T12:08:32Z

Separate per is fine, I was simply trying to de-duplicate where applicable, (e.g. Ethernet on eos, ios, xr, nxos, etc...). I see this as the opposite of inheritance, instead as subscribing to an interface type.
I missed A, but will implement. I actually started on B with optional args, it didn't work on first try, and called it a night, but I will implement.

itdependsnetworks · 2017-11-06T12:14:57Z

In regards to:

def canonical_interface(interface, device_os, short=False):
    mapping = {
        'Po': 'Port-channel',
        'PO': 'POS',

I think I am missing something, how does that account for PortChannel? The only way I see is to enumerate each potential, which seems worse than I have.

dbarrosop · 2017-11-06T13:33:00Z

ffs why won't people stop buying cisco crap...

Anyway, you can add extra k,v pairs to the reverse mapping. But what's the use case for the short naming anyway? Are you expecting people to do something like:

# so much inception
my_short_normalized_name = device.get_normalized_name(device.get_normalized_name(iface), short=True)

itdependsnetworks · 2017-11-06T13:54:31Z

So thought it if I wanted to use neighbors data to generate a diagram, I would want to use short name in diagram.

So some kind of toggle to force short name usage.

dbarrosop · 2017-11-06T15:29:20Z

Ok, makes sense.

itdependsnetworks · 2017-11-06T16:07:55Z

@dbarrosop Closed as in not gonna happen, hold off until reunification done, or need to update implementation??

dbarrosop · 2017-11-06T16:48:35Z

no, sorry, I didn't close it. It was a side effect of deleting the branch. Reopining against develop

itdependsnetworks · 2017-11-14T14:45:45Z

My todo:

create a method device.get_canonical_interface_name('et1')
create a setting that is applied globally

Outstanding questions:

Do I move the mappings to the specific driver? my initial response Separate per is fine, I was simply trying to de-duplicate where applicable, (e.g. Ethernet on eos, ios, xr, nxos, etc...). I see this as the opposite of inheritance, instead as subscribing to an interface type.
Are you ok with my non-regex approach?

Other than that, is everything good to go here?

dbarrosop · 2017-11-16T13:11:13Z

I think the method should provide a predefined dictionary that doesn't care about vendors ("po" is going to a port-channel regardless of the platform or is simply not have any meaning).

Regarding non-regex approach, sure, if you can make it simpler than this #493 (comment) be my guest.

itdependsnetworks · 2017-11-16T17:18:39Z

from napalm import get_network_driver

ios_device='10.1.100.1'
ios_user='ntc'
ios_password='ntc123'
driver = get_network_driver('ios')
optional_args = {"canonical_int": True}
device = driver(ios_device, ios_user, ios_password, optional_args=optional_args)
device.open()
device.get_mac_address_table()

Works as expected:
without:
{u'vlan': 1, u'last_move': -1.0, u'active': True, u'mac': u'2C:6B:F5:53:9D:8F', u'static': False, u'interface': u'Fa1/0/2', u'moves': -1}
with:
{u'vlan': 1, u'last_move': -1.0, u'active': True, u'mac': u'2C:6B:F5:53:9D:8F', u'static': False, u'interface': u'FastEthernet1/0/2', u'moves': -1}}

coveralls · 2017-11-16T17:25:05Z

Coverage decreased (-0.3%) to 77.598% when pulling b2d2bfa on itdependsnetworks:canonical_interface into a04c348 on napalm-automation:develop.

coveralls · 2017-11-16T17:27:14Z

Coverage decreased (-0.3%) to 77.598% when pulling b2d2bfa on itdependsnetworks:canonical_interface into a04c348 on napalm-automation:develop.

dbarrosop · 2017-11-17T13:00:59Z

napalm/ios/ios.py

@@ -433,6 +435,13 @@ def commit_config(self):
        # Save config to startup (both replace and merge)
        output += self.device.send_command_expect("write mem")

+    def _canonical_int(self, interface):


I think this one should be in the base driver.

My thought process was

Within the os, the only variable is interface

Shorten from napalm.base.helpers.canonical_interface(interface, "cisco_ios", True) -> self._canonical_int(interface)

dbarrosop · 2017-11-17T13:04:36Z

napalm/base/helpers.py

+    if not change:
+        return interface
+
+    if device_os == "cisco_nxos_ssh":


I'd get rid of the device_os tbh. I don't see much value in doing this check. What we could do instead is replace the device_os with a fail_on_miss bool that does something like:

try: return reverse_mapping[long_int] + str(interface_number) except KeyError: if fail_on_miss: raise return long_int+str(interface_number)

I am super cautious in making sure I know I am not messing up something here. I'm fine either way, and in reality this only get's turned on if implemented in said os driver, so maybe not that big of an issue.

I would like to keep the transposing of cisco_nxos_ssh -> cisco_nxos in case there needs to be a base_interfaces = base_interfaces.update(interface_map.get(device_os)) on the nxos.

dbarrosop · 2017-11-17T13:07:50Z

napalm/base/helpers.py

@@ -255,3 +256,47 @@ def as_number(as_number_val):
        return (int(big) << 16) + int(little)
    else:
        return int(as_number_str)
+
+
+def canonical_interface(interface, device_os, change, short=False):


I think this should be two different methods; one for each direction. @mirceaulinic @ktbyers thoughts?

I'm ok either way. To clarify to M & K, one way is short to long (et -> Ethernet), the other is long to short (Ethernet -> eth ).

dbarrosop · 2017-11-17T13:09:07Z

napalm/base/interface_map.py

+from __future__ import unicode_literals
+
+
+def _interface_map():


Why don't you just expose the variables directly so you can do something like:

from napalm.base import interface_map ... short_name = interface_map.long_to_short[iface_name] long_name = interface_map.short_to_long[short_name]

dbarrosop · 2017-11-17T13:15:00Z

napalm/base/helpers.py

+    int_map = _interface_map()
+    base_interfaces = int_map['base_interfaces']
+    reverse_mapping = int_map['reverse_mapping']
+    if int_map.get(device_os):


This should come from the driver itself.

class IOSDriver(NetworkDriver): # probably needs a better name but this is a class attribute long_to_short = {"my_unique_iface_name_that_collides_with_something_else": "ohmy"} ... def canonical_interface(interface, device, change, short=False): ... base_interfaces = base_interfaces.update(device.long_to_short) ...

Missed this before.

coveralls · 2017-11-17T13:56:16Z

Coverage decreased (-0.3%) to 77.712% when pulling bec16e1 on itdependsnetworks:canonical_interface into 4b2fcbf on napalm-automation:develop.

coveralls · 2017-11-17T16:24:41Z

Coverage decreased (-0.2%) to 77.726% when pulling 9d7487c on itdependsnetworks:canonical_interface into 4b2fcbf on napalm-automation:develop.

ktbyers · 2017-11-17T20:48:10Z

Per conversations with @itdependsnetworks I think Ken is going to move this to the parent class:

def _canonical_int(self, interface):

I will review after that is completed.

coveralls · 2017-11-17T21:00:16Z

Coverage decreased (-0.2%) to 77.776% when pulling a5fac96 on itdependsnetworks:canonical_interface into 4b2fcbf on napalm-automation:develop.

ktbyers

See feedback.

ktbyers · 2017-11-17T21:21:47Z

napalm/ios/ios.py

@@ -128,6 +128,8 @@ def __init__(self, hostname, username, password, timeout=60, optional_args=None)

        self.profile = ["ios"]

+        self.set_canonical_interface = optional_args.get('canonical_int', False)


How about self.use_canonical_int? I don't like set_canonical_interface as it makes me think we are doing a setter on an attribute.

ktbyers · 2017-11-17T21:24:10Z

napalm/base/helpers.py

@@ -255,3 +256,41 @@ def as_number(as_number_val):
        return (int(big) << 16) + int(little)
    else:
        return int(as_number_str)
+
+
+def canonical_interface(interface, change, short=False, update_long_to_short=None):


Why not make this be?

def canonical_interface(interface, change=True, short=False, update_long_to_short=None):

Do we even need change argument since we would always call this with True?

Also you need to document the arguments here? Particularly short and update_long_to_short?

So update_long_to_short is a dictionary of additional names someone could feed in going from abbreviated names to full names? I don't think that name makes sense.

np with it being change=False, but the idea was to not change it so drastically at first. @mirceaulinic (and I) had concerns about changing outcome so abruptly, perhaps next major version update, after we know it is good?

Documentation will be easier once everything has been agreed upon.

ktbyers · 2017-11-17T21:36:05Z

napalm/base/helpers.py

@@ -19,6 +19,7 @@
 import napalm.base.exceptions
 from napalm.base.utils.jinja_filters import CustomJinjaFilters
 from napalm.base.utils import py23_compat
+from napalm.base.interface_map import interface_map


We shouldn't use the name interface_map since there is already an existing interface_map in the ios.py code.

ktbyers · 2017-11-17T21:37:06Z

napalm/base/base.py

@@ -1583,3 +1583,11 @@ def compliance_report(self, validation_file=None, validation_source=None):
        """
        return validate.compliance_report(self, validation_file=validation_file,
                                          validation_source=validation_source)
+
+    def canonical_int(self, interface):


I think this probably should be a private method, for end-user calls people can just directly call what is in helpers.

ktbyers · 2017-11-17T21:38:13Z

napalm/base/interface_map.py

+from __future__ import unicode_literals
+
+
+interface_map = {


Shouldn't be named interface_map due to existing name conflict.

Also file name should not be interface_map.py.

ktbyers · 2017-11-17T21:40:33Z

napalm/base/interface_map.py

+
+
+interface_map = {
+  "base_interfaces": {


Why a nested dictionary as opposed to two separate dictionaries? I think they should be two separate dictionaries.

ktbyers · 2017-11-17T21:47:27Z

napalm/base/helpers.py

+    if base_interfaces.get(interface_type):
+        long_int = base_interfaces.get(interface_type)
+        # return short form if set
+        if short is True:


Do we want to overload this method with returning the abbreviated name or have a second method? I think it is more logical to have these as two separate methods.

Method one returns the canonical name.

Method two returns the abbreviated name (from the canonical name).

Was waiting on consensus on this

coveralls · 2017-11-17T22:12:14Z

Coverage decreased (-0.2%) to 77.805% when pulling 24c5b39 on itdependsnetworks:canonical_interface into 4b2fcbf on napalm-automation:develop.

itdependsnetworks · 2017-11-17T22:58:54Z

@mirceaulinic and @dbarrosop I think I have a +1 from Kirk. Let me know if anything should change, going to start implementing everywhere tomorrow my AM.

I will probably work on a proposal PR for writing output files tonight.

ktbyers

I reviewed this extensively with @itdependsnetworks.

coveralls · 2017-11-18T00:49:14Z

Coverage decreased (-0.2%) to 77.787% when pulling 09a4772 on itdependsnetworks:canonical_interface into 4b2fcbf on napalm-automation:develop.

add canonical_interface

9e54e2c

dbarrosop reviewed Nov 6, 2017

View reviewed changes

dbarrosop closed this Nov 6, 2017

dbarrosop changed the base branch from reunification_core to develop November 6, 2017 16:48

dbarrosop reopened this Nov 6, 2017

mirceaulinic added this to the DISCUSSION milestone Nov 8, 2017

mirceaulinic added base enhancement labels Nov 8, 2017

itdependsnetworks added 3 commits November 16, 2017 11:47

implment suggested changes

9f5a8b6

fix test reverse mapping

892643b

update pep

4bb8673

Merge branch 'develop' into canonical_interface

b2d2bfa

dbarrosop reviewed Nov 17, 2017

View reviewed changes

itdependsnetworks added 3 commits November 17, 2017 08:31

update mapping

a86dc53

Merge branch 'develop' into canonical_interface

f19a7c4

pylama

bec16e1

os diff sent to driver

9d7487c

move to base class

a5fac96

ktbyers reviewed Nov 17, 2017

View reviewed changes

updates as per kirk recommendations

24c5b39

itdependsnetworks added 3 commits November 17, 2017 17:29

split methods, and update names

6ff2c96

move change check to base not helper

3b7be7d

bug fixes

1bcd70d

ktbyers approved these changes Nov 17, 2017

View reviewed changes

update pylama

09a4772

ktbyers merged commit bc4e6d8 into napalm-automation:develop Nov 18, 2017

		from __future__ import unicode_literals


		def _interface_map():

		@@ -128,6 +128,8 @@ def __init__(self, hostname, username, password, timeout=60, optional_args=None)

		self.profile = ["ios"]

		self.set_canonical_interface = optional_args.get('canonical_int', False)

add canonical_interface #493

add canonical_interface #493

Conversation

itdependsnetworks commented Nov 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbarrosop commented Nov 6, 2017

Choose a reason for hiding this comment

dbarrosop commented Nov 6, 2017 • edited Loading

mirceaulinic commented Nov 6, 2017

dbarrosop commented Nov 6, 2017 • edited Loading

itdependsnetworks commented Nov 6, 2017

itdependsnetworks commented Nov 6, 2017 • edited Loading

dbarrosop commented Nov 6, 2017 • edited Loading

itdependsnetworks commented Nov 6, 2017

dbarrosop commented Nov 6, 2017

itdependsnetworks commented Nov 6, 2017

dbarrosop commented Nov 6, 2017

itdependsnetworks commented Nov 14, 2017

dbarrosop commented Nov 16, 2017

itdependsnetworks commented Nov 16, 2017

coveralls commented Nov 16, 2017 • edited Loading

coveralls commented Nov 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 17, 2017 • edited Loading

coveralls commented Nov 17, 2017 • edited Loading

ktbyers commented Nov 17, 2017

coveralls commented Nov 17, 2017 • edited Loading

ktbyers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 17, 2017 • edited Loading

itdependsnetworks commented Nov 17, 2017

ktbyers left a comment

Choose a reason for hiding this comment

coveralls commented Nov 18, 2017 • edited Loading

dbarrosop commented Nov 6, 2017 •

edited

Loading

dbarrosop commented Nov 6, 2017 •

edited

Loading

itdependsnetworks commented Nov 6, 2017 •

edited

Loading

dbarrosop commented Nov 6, 2017 •

edited

Loading

coveralls commented Nov 16, 2017 •

edited

Loading

coveralls commented Nov 16, 2017 •

edited

Loading

coveralls commented Nov 17, 2017 •

edited

Loading

coveralls commented Nov 17, 2017 •

edited

Loading

coveralls commented Nov 17, 2017 •

edited

Loading

coveralls commented Nov 17, 2017 •

edited

Loading

coveralls commented Nov 18, 2017 •

edited

Loading