Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Connected property #664

Merged
merged 9 commits into from
Feb 16, 2017
Merged

Connected property #664

merged 9 commits into from
Feb 16, 2017

Conversation

spidercensus
Copy link
Contributor

@spidercensus spidercensus commented Feb 9, 2017

This partly addresses issue #663

@spidercensus spidercensus reopened this Feb 9, 2017
@coveralls
Copy link

coveralls commented Feb 9, 2017

Coverage Status

Coverage decreased (-0.1%) to 98.943% when pulling d4dc413 on spidercensus:connected-property into 5981152 on Juniper:master.

@spidercensus
Copy link
Contributor Author

This converts the Device.connected attribute into a property that depends on the new _connected member value. There is a getter and a setter defined.

There is also now a Session Listener attached to the session object in the netconf transport, which responds to errors in the Transport in the background by setting the Device.connected value to False and raising a TransportError exception.

I also added a step in the Device.cli method to properly catch the ConnectClosedError, which before was just being blackholed into the except Exception as ex: and reported back as "invalid command".

@coveralls
Copy link

coveralls commented Feb 9, 2017

Coverage Status

Coverage decreased (-0.1%) to 98.943% when pulling d4dc413 on spidercensus:connected-property into 5981152 on Juniper:master.

@@ -940,6 +966,14 @@ def __init__(self, *vargs, **kvargs):
# -----------------------------------------------------------------------
# Basic device methods
# -----------------------------------------------------------------------
@property
def connected(self):
return self._connected
Copy link
Contributor

@vnitinv vnitinv Feb 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where is _connected initialized? I guess this is coming from ncclient. Not sure if this will be
self._cnnected or self._conn._connected

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vnitinv line 959 in __init__() initializes self._connected to False because it's calling the connected setter.

Copy link
Contributor

@vnitinv vnitinv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really what I wanted to have. Thanks a lot Jason.

Copy link
Contributor

@stacywsmith stacywsmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@spidercensus It generally looks good. Just one small issue with errback(). Once you're able to make that change I will merge.

:type ex: :exc:`Exception`
"""
self._device.connected = False
raise NcErrors.TransportError
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe we should raise an exception here. It's not actually doing anything anyway because ncclient catches the exception and just attempts to log a debug message:
https://github.com/ncclient/ncclient/blob/master/ncclient/transport/session.py#L77-L80

A TransportError exception will already be raised the next time there's an attempt to send on this connection:
https://github.com/ncclient/ncclient/blob/master/ncclient/transport/session.py#L155

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you're right. I'll remove that.

def __init__(self, device):
self._device = device
def callback(self, root, raw):
"""Called when a new XML document is received. The *root* argument allows the callback to determine whether it wants to further process the document.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please run through pep8 and clean up any reported problems like this long line.

@vnitinv
Copy link
Contributor

vnitinv commented Feb 13, 2017

@stacywsmith Give me 1-2 days to test this pull request. Then we can merge this.

Copy link
Contributor Author

@spidercensus spidercensus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

made requested changes and ran thru pep8

@coveralls
Copy link

coveralls commented Feb 13, 2017

Coverage Status

Coverage decreased (-0.1%) to 98.943% when pulling b376bd9 on spidercensus:connected-property into 5981152 on Juniper:master.

@coveralls
Copy link

coveralls commented Feb 13, 2017

Coverage Status

Coverage decreased (-0.08%) to 98.969% when pulling 1c5fe41 on spidercensus:connected-property into 5981152 on Juniper:master.

Copy link
Contributor

@stacywsmith stacywsmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vnitinv I approve of this pull request, but I don't want to merge without your agreement. Yes, this pull request only addresses the case where the Junos device closes the NETCONF over SSH connection due to an inactivity timeout. However, we can address the issue where the connection is interrupted due to an intermediate network problem in either #668 or a new pull request.

@vnitinv vnitinv merged commit 87d170e into Juniper:master Feb 16, 2017
@spidercensus spidercensus mentioned this pull request Mar 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants