Define generic napalm metaclass to catch exceptions

[mirceaulinic]

And raise a napalm exception instead. As discussed under https://github.com/napalm-automation/napalm-junos/pull/161

Log trace:

[ERROR   ] Raised jnpr.junos.exception.ConnectClosedError
Traceback (most recent call last):
  File "/home/admin/napalm-base/napalm_base/base.py", line 51, in fun
    return meth(*args, **kwargs)
  File "/home/admin/napalm-junos/napalm_junos/junos.py", line 1069, in get_arp_table
    arp_table_raw.get()
  File "/usr/local/lib/python2.7/dist-packages/jnpr/junos/factory/optable.py", line 64, in get
    self.xml = getattr(self.RPC, self.GET_RPC)(**rpc_args)
  File "/usr/local/lib/python2.7/dist-packages/jnpr/junos/rpcmeta.py", line 336, in _exec_rpc
    return self._junos.execute(rpc, **dec_args)
  File "/usr/local/lib/python2.7/dist-packages/jnpr/junos/decorators.py", line 63, in wrapper
    result = function(*args, **kwargs)
  File "/usr/local/lib/python2.7/dist-packages/jnpr/junos/decorators.py", line 31, in wrapper
    return function(*args, **kwargs)
  File "/usr/local/lib/python2.7/dist-packages/jnpr/junos/device.py", line 695, in execute
    raise EzErrors.ConnectClosedError(self)
ConnectClosedError: ConnectClosedError(ip-172-31-9-153.us-east-2.compute.internal)
[INFO    ] Raising ConnectionClosedException instead
[ERROR   ] Traceback (most recent call last):
  File "/home/admin/salt/salt/utils/napalm.py", line 153, in call
    out = getattr(napalm_device.get('DRIVER'), method)(*args, **kwargs)
  File "/home/admin/napalm-base/napalm_base/base.py", line 51, in fun
    return meth(*args, **kwargs)
  File "/home/admin/napalm-junos/napalm_junos/junos.py", line 1069, in get_arp_table
    arp_table_raw.get()
  File "/usr/local/lib/python2.7/dist-packages/jnpr/junos/factory/optable.py", line 64, in get
    self.xml = getattr(self.RPC, self.GET_RPC)(**rpc_args)
  File "/usr/local/lib/python2.7/dist-packages/jnpr/junos/rpcmeta.py", line 336, in _exec_rpc
    return self._junos.execute(rpc, **dec_args)
  File "/usr/local/lib/python2.7/dist-packages/jnpr/junos/decorators.py", line 63, in wrapper
    result = function(*args, **kwargs)
  File "/usr/local/lib/python2.7/dist-packages/jnpr/junos/decorators.py", line 31, in wrapper
    return function(*args, **kwargs)
  File "/usr/local/lib/python2.7/dist-packages/jnpr/junos/device.py", line 695, in execute
    raise EzErrors.ConnectClosedError(self)
ConnectionClosedException: ConnectClosedError(ip-172-31-9-153.us-east-2.compute.internal)

[mirceaulinic]

@dbarrosop @ktbyers pushed the changes as suggested.

The current approach requires the driver to define a map like:

class JunOSDriver(NetworkDriver):
    """JunOSDriver class - inherits NetworkDriver from napalm_base."""

    ERROR_MAP = {
        'jnpr.junos.exception.ConnectClosedError': napalm_base.exceptions.ConnectionClosedException,
        'jnpr.junos.exception.ConnectAuthError': napalm_base.exceptions.ConnectAuthError,
        'jnpr.junos.exception.ConnectTimeoutError': napalm_base.exceptions.ConnectTimeoutError,
        'jnpr.junos.exception.LockError': napalm_base.exceptions.LockError,
        'jnpr.junos.exception.UnlockError': napalm_base.exceptions.UnlockError,
        'jnpr.junos.exception.CommitError': napalm_base.exceptions.CommitError
    }

We can have either have the mapping between the full path, either 1:1 class, either both (although if we have both the code might become even less obvious). Having the errors identified as str has the advantage that we don't need to import the exception class. What do you think?

[dbarrosop]

I would import the classes directly to avoid issues that are only triggered at runtime because someone misspelled a path or class name.

[ktbyers]

I still don't like using the metaclass. I think a decorator would be a more logical construct to use.

The thing I don't like is that it is hidden in the code. When someone looks at the code in napalm-ios, napalm-eos, et cetera, they will have no clue that this is going on.

Also the pattern we are applying falls pretty logically into the decorator pattern.

Something like

@normalize_exceptions def commit_config(self):

The negative being we are going to explicitly have to decorate functions in each driver, but even with this I think the decorator pattern is a more logical solution than the metaclass.

We could still have normalize_exceptions code in napalm-base and have a map similar to what was proposed earlier.

Having this pattern makes it clear that we are normalizing exceptions and where this normalization applies to.

[mirceaulinic]

I'd fine with an explicit decorator.

FYI: this is exactly what this metaclass does - decorates all the methods (with _raise_napalm_error), without requiring the user to decorate them manually. (I'm not keen on implementing it like that, and I agree it's less obvious)

[ktbyers]

Yes, understood...it is a trade-off of slightly more work versus hidden-behind-the-scenes.

[mirceaulinic]

I'm fine with any of the approaches. So @ktbyers @dbarrosop please let me know which one would be preferable (if you see any advantages/disadvantages). No rush on this, we can leave it aside a few days and think more about it...

[ktbyers]

Sounds good...

[dbarrosop]

I'd rather use the metaclass to avoid "forgetting" about it. Just make sure that the metaclass and the wrapper function are well documented and it should be fine. The traceback should show the wrapper function if something goes bad so not super concerned about the whole stuff failing without being super explicit about it.

[ktbyers]

I won't put up any more objections on this decorator vs metaclass so don't hold it up because of me...

[dbarrosop]

@mirceaulinic when you have the time link a PR using this. Would be great to have them hand in hand to see how it's used :)

[bewing]

Looks like https://pythonhosted.org/six/#six.reraise is needed for full compatibility between py2 and py3 for re-raising exceptions.

Standalone code in https://stackoverflow.com/questions/34463087/raise-exception-in-python-2-x-and-3-x

[mirceaulinic]

Hi @bewing - thanks for suggestion. I recall I had some changes not pushed yet, hope I didn't stash them. Let me revisit this somewhere next week.