Define generic napalm metaclass to catch exceptions #259

mirceaulinic · 2017-06-05T14:36:09Z

And raise a napalm exception instead.
As discussed under napalm-automation/napalm-junos#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)

And raise a napalm exception instead. As discussed under napalm-automation/napalm-junos#161

dbarrosop · 2017-06-06T08:42:02Z

napalm_base/base.py

 from napalm_base import validate

+log = logging.getLogger(__file__)
+
+ERROR_MAP = {


This should be a class attribute and it should be empty for the base driver. Then each driver can implement it's own ERROR_MAP.

Yes, makes sense. Just that I'm not sure yet what's the best approach... this might be a chicken and egg type thing (napalm-base depends on the driver, which depends on napalm-base). I will investigate.

napalm-base depends on the driver

But it doesn't. It's just a class method that uses a class attribute. Your subclass just happens to override the class attribute. The parent class has no idea about it :)

dbarrosop · 2017-06-06T08:43:05Z

napalm_base/base.py

+                err_class = ERROR_MAP[err_full_name]
+                log.info('Raising {} instead'.format(err_class.__name__))
+                raise err_class(str(error))
+            # Raise everything else, using the original class.


It would be great if the else statement here would do something like this:

https://github.com/napalm-automation/napalm-base/blob/develop/napalm_base/base.py#L87

Potentially raising an UncaughtException or something like that. Something we can distinguish easily.

Sounds good.

dbarrosop

Overall looks great, just proposed a few minor changes.

mirceaulinic · 2017-06-06T10:41:04Z

@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 · 2017-06-06T15:22:26Z

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

ktbyers · 2017-06-06T16:16:09Z

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 · 2017-06-06T16:21:58Z

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 · 2017-06-06T16:29:12Z

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

mirceaulinic · 2017-06-06T16:31:56Z

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 · 2017-06-06T16:33:39Z

Sounds good...

dbarrosop · 2017-06-06T17:06:18Z

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 · 2017-06-06T23:14:26Z

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

dbarrosop · 2017-06-08T07:50:46Z

@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 · 2017-08-22T02:04:05Z

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 · 2017-08-22T09:01:52Z

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.

mirceaulinic added the enhancement label Jun 5, 2017

mirceaulinic added this to the 0.25.0 milestone Jun 5, 2017

mirceaulinic requested review from ktbyers and dbarrosop June 5, 2017 14:36

Define generic napalm metaclass to catch exceptions

160176a

And raise a napalm exception instead. As discussed under napalm-automation/napalm-junos#161

mirceaulinic force-pushed the raise-err-meta branch from ae1e4cf to 160176a Compare June 5, 2017 14:40

mirceaulinic mentioned this pull request Jun 5, 2017

Catch ConnectClosedError from junos-eznc napalm-automation/napalm-junos#161

Closed

dbarrosop reviewed Jun 6, 2017

View reviewed changes

dbarrosop suggested changes Jun 6, 2017

View reviewed changes

mirceaulinic added 4 commits June 6, 2017 09:39

Make sure we're raising the full stack trace

55ad5a8

Include the original exception under the original_exc property

ac7c31a

Raise UncaughtException

86765ff

Use the driver ERROR_MAP

7c10eff

mirceaulinic modified the milestones: 0.25.0, 0.27.0 Oct 7, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Define generic napalm metaclass to catch exceptions #259

Define generic napalm metaclass to catch exceptions #259

mirceaulinic commented Jun 5, 2017 •

edited

Loading

dbarrosop Jun 6, 2017

mirceaulinic Jun 6, 2017

dbarrosop Jun 6, 2017

dbarrosop Jun 6, 2017

dbarrosop Jun 6, 2017

mirceaulinic Jun 6, 2017

dbarrosop left a comment

mirceaulinic commented Jun 6, 2017 •

edited

Loading

dbarrosop commented Jun 6, 2017

ktbyers commented Jun 6, 2017 •

edited

Loading

mirceaulinic commented Jun 6, 2017 •

edited

Loading

ktbyers commented Jun 6, 2017

mirceaulinic commented Jun 6, 2017

ktbyers commented Jun 6, 2017

dbarrosop commented Jun 6, 2017

ktbyers commented Jun 6, 2017 •

edited

Loading

dbarrosop commented Jun 8, 2017

bewing commented Aug 22, 2017

mirceaulinic commented Aug 22, 2017

Define generic napalm metaclass to catch exceptions #259

Are you sure you want to change the base?

Define generic napalm metaclass to catch exceptions #259

Conversation

mirceaulinic commented Jun 5, 2017 • edited Loading

dbarrosop Jun 6, 2017

Choose a reason for hiding this comment

mirceaulinic Jun 6, 2017

Choose a reason for hiding this comment

dbarrosop Jun 6, 2017

Choose a reason for hiding this comment

dbarrosop Jun 6, 2017

Choose a reason for hiding this comment

dbarrosop Jun 6, 2017

Choose a reason for hiding this comment

mirceaulinic Jun 6, 2017

Choose a reason for hiding this comment

dbarrosop left a comment

Choose a reason for hiding this comment

mirceaulinic commented Jun 6, 2017 • edited Loading

dbarrosop commented Jun 6, 2017

ktbyers commented Jun 6, 2017 • edited Loading

mirceaulinic commented Jun 6, 2017 • edited Loading

ktbyers commented Jun 6, 2017

mirceaulinic commented Jun 6, 2017

ktbyers commented Jun 6, 2017

dbarrosop commented Jun 6, 2017

ktbyers commented Jun 6, 2017 • edited Loading

dbarrosop commented Jun 8, 2017

bewing commented Aug 22, 2017

mirceaulinic commented Aug 22, 2017

mirceaulinic commented Jun 5, 2017 •

edited

Loading

mirceaulinic commented Jun 6, 2017 •

edited

Loading

ktbyers commented Jun 6, 2017 •

edited

Loading

mirceaulinic commented Jun 6, 2017 •

edited

Loading

ktbyers commented Jun 6, 2017 •

edited

Loading