Catch ConnectClosedError from junos-eznc

[mirceaulinic]

And raise ConnectionClosedException from napalm-base. We can easily raise what we need using a metaclass that catches the exceptions and raises the desired exception as required. This models has the advantage that it's easier to raise other kind of exceptions. We will soon need to raise other new exceptions from napalm-base, e.g.: LockException, UnlockException etc.

[coveralls]

Coverage decreased (-1.2%) to 82.334% when pulling 733ed957a39023e5d5825766e428e0c57b4f7b8f on conn-closed-err into 12663f9d4c464f4cdc9f69e6edf0c7c6cd0fa55d on develop.

[coveralls]

Coverage decreased (-1.2%) to 82.334% when pulling 733ed957a39023e5d5825766e428e0c57b4f7b8f on conn-closed-err into 12663f9d4c464f4cdc9f69e6edf0c7c6cd0fa55d on develop.

[dbarrosop]

i wonder if we should generalize this code. For example, the metaclass could be part of the base driver and use a dictionary to map different exceptions. So the wrapper and everything you put in here would be instead in the base driver (with some changes to use the map but same idea) and then on the drivers have something like:

exception_map = {
    ConnectClosedError: ConnectionClosedException,
    SomeOtherErrorFromUpperClass: NapalmException,
}

This would allow us to easily achieve the same everywhere with very little code.

[mirceaulinic]

Yes, I like the principle, but I find two issues here:

• Under napalm-base we don't have the ConnectClosedError class, as junos-eznc is not a dependency of napalm-base. We could map using the class name instead, i.e.:

exception_map = {
    'ConnectClosedError': ConnectionClosedException,
    'SomeOtherErrorFromUpperClass': NapalmException,
}

Which introduces my second objection:

• Under the driver we will be then restricted to import the exception with the name exactly as in the exception_map (to make sure I make myself understood, we won't be able to do for example from jnpr.junos.exception import ConnectClosedError as CCErr etc.)

I don't think this would cause a massive problem, but let's make sure we agree on this. If not, I can define here a private method and, although there will be a bit more redundant code, it would be more obvious. I'm fine with both approaches. @dbarrosop @ktbyers thoughts?

[mirceaulinic]

Submitted https://github.com/napalm-automation/napalm-base/pull/259 for the discussion above.

[ktbyers]

I don't really like this solution (as it has a pretty high-degree of complexity to it). I don't have a better one, however.

Can you explain a bit better why it matters whether the exception comes out as ConnectClosedError from Junos or ConnectionClosedException from napalm-base.

I am worried that in trying to unify errors especially if we do this broadly that we are going to add quite a bit of complexity. Additionally, I am a bit concerned that this might mask errors and make the tracking down of what really happened harder (would have to see what stack trace here looks like).

[mirceaulinic]

I don't really like this solution (as it has a pretty high-degree of complexity to it). I don't have a better one, however.

Indeed, it's a bit complex. But, as I said, I am happy to revert and switch to a more natural alternative, but more redundant and would require the developer write the same for each and every new method. Also, I'm reading @dbarrosop's comment for another perspective: this is good enough to solve more generic cases (thinking about the future napalm-ng where we won't have bespoken getters).

Can you explain a bit better why it matters whether the exception comes out as ConnectClosedError from Junos or ConnectionClosedException from napalm-base.

We, as humans, see the difference between ConnectClosedError and ConnectionClosedException, but the machine sees two different classes: jnpr.junos.exception.ConnectClosedError and napalm_base.exceptions. ConnectionClosedException -- they are tottally different. So we can either unify either try to catch each and every sub-driver etc. thus too many exceptions classes. That would be pointless.

I am worried that in trying to unify errors especially if we do this broadly that we are going to add quite a bit of complexity. Additionally, I am a bit concerned that this might mask errors and make the tracking down of what really happened harder (would have to see what stack trace here looks like). Merge state

I totally disagree:

1. the point of NAPALM is to unify also at the level of getters, configuration management, but also at the level of errors.
2. if (1) is not achieved, users will complain. And they have the right to. You can see for example https://github.com/napalm-automation/napalm-junos/pull/53 -- we need to raise napalm-specific exceptions, not the exception of library X.

TL;DR: For me is very clear that we need to raise unified exceptions, but I'm very open to change the approach.

[ktbyers]

I probably disagree on the need of NAPALM needing to raise unified exceptions (at least to the degree specified in this PR). I think this is the point we need to discuss.

There is also the question of how far to take this unified exceptions...i.e. generally unified NAPALM exceptions versus going how to far to catch all possible exceptions from lower level libraries.

I think this issue will have pretty broad implications to the NAPALM code so we should probably discuss this prior to implementing this PR.

[ktbyers]

Note, I am not against unified exceptions as such...I am worried that the cost of unifying exceptions outweighs the benefits.

In particular, that complexity in debugging problems goes up significantly (as we mask the source of exceptions). Also that our code complexity goes up significantly to do this.

[mirceaulinic]

In particular, that complexity in debugging problems goes up significantly (as we mask the source of exceptions). Also that our code complexity goes up significantly to do this.

If that's the only argument, I consider it invalid: it does not mask anything -- see a sample traceback below:

Traceback (most recent call last):
  File "/home/admin/napalm-base/napalm_base/base.py", line 50, 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)

Could you please explain what is it masking?

I think this issue will have pretty broad implications to the NAPALM code so we should probably discuss this prior to implementing this PR.

This PR introduces and uses the Exception we have already agreed on https://github.com/napalm-automation/napalm-base/pull/247 and introduced in https://github.com/napalm-automation/napalm-base/pull/248. It is also the idea you exposed under https://github.com/napalm-automation/napalm-ios/pull/152#issuecomment-304472526 (linked directly to your own comment):

I would consider the logical NAPALM behavior to throw an exception.

I hope we don't start repudiating our own statements, depending on the context...

I am worried that the cost of unifying exceptions outweighs the benefits.

I agree, some Exceptions may not be possible to implement. But I strongly believe we should target as many as possible. And ConnectionClosedException specifically is something already debated, agreed and released.

[ktbyers]

Agreed...it is not masking anything so I have no objections to that.

It wasn't apparent to me what the stack trace was going to look like, but that is totally fine.

I am okay with this...

On @dbarrosop proposal:

i wonder if we should generalize this code. For example, the metaclass could be 
part of the base driver and use a dictionary to map different exceptions. So the 
wrapper and everything you put in here would be instead in the base driver 
(with some changes to use the map but same idea) and then on the 
drivers have something like:

exception_map = {
    ConnectClosedError: ConnectionClosedException,
    SomeOtherErrorFromUpperClass: NapalmException,
}

I agree I think the metaclass should be moved to napalm-base.

I think we should have an exception_map in napalm-base and then one in the driver (i.e. do update on the dictionary so that any exceptions that were common or driver specific could be mapped).

This would also allow us to avoid the issue of having to import any library (just for exceptions) since they would be in napalm-driver and not in base.

we won't be able to do for example from jnpr.junos.exception import 
ConnectClosedError as CCErr etc.)

I don't think this is important...i.e. I think requiring the use of the canonical name is fine.

[dbarrosop]

So, two things:

1. All napalm exception should have an original_exc attribute that contains the original exception that was caught and "unified". That way the user still can access it programmatically if needed. It should be consistent across all exceptions.
2. Yes, the idea was that the "exception_maps" would live under each driver. The one on the base class should be empty so there is no need for napalm-base to import anything extra.

[mirceaulinic]

Alright, let's continue this discussion under napalm-base. I'm welcoming your suggestions under https://github.com/napalm-automation/napalm-base/pull/259.