search

napalm-automation / napalm-base

Apache License 2.0
32 stars 48 forks source link

More exceptions #248

Closed mirceaulinic closed 7 years ago

mirceaulinic commented 7 years ago

Define new exception ConnectionClosedException as per https://github.com/napalm-automation/napalm-base/pull/247

Add the exceptions proposed in https://github.com/napalm-automation/napalm-base/issues/218

mirceaulinic commented 7 years ago

@dbarrosop I've pushed some changes: https://github.com/napalm-automation/napalm-base/pull/248/commits/b3b5c9be7982169f43968fa5af9291e8af1c2807, feel free to suggest more improvements if anything still doesn't sound good.

ktbyers commented 7 years ago

In SSH you can also have the SSH channel closed by remote end (currently raises an EOFError in Netmiko). In this case the TCP connection is still open, but you are unable to communicate.

I think this should probably fall into this ConnectionClosedException as you would need to treat it the same way in practice (i.e. re-open a new connection).

mirceaulinic commented 7 years ago

Yes, that was my understanding too @ktbyers. Would you suggest including this explanation in the exception docsting?

ktbyers commented 7 years ago

I think we might want to include that in the docstring (i.e. 'or SSH channel closing').

dbarrosop commented 7 years ago

We don't really need to be super explicit. Most of the time people will just want to know the connection was dropped due to inactivity (regardless of who or how) and just try to reconnect. My point is that a single ConnectionDroppedException and then different messages for logging purposes should probably be enough. I can't imagine someone treating a timeout because a firewall dropped a flow being treated different than ssh closing the channel.