napalm-automation / napalm-base

Apache License 2.0
32 stars 48 forks source link

session_keepalive method base #249

Closed mirceaulinic closed 7 years ago

mirceaulinic commented 7 years ago

This PR introduces a new method as discussed under https://github.com/napalm-automation/napalm-ios/pull/152. Apparently, the only way to maintain the connection alive with the remote device is sending a special character at regular intervals. This method only sends the special sequence, does not handle any timing etc. Under the napalm-ios driver, the implementation would be as simple as:

def anti_idle(self):
    self.write_channel(chr(0))
dbarrosop commented 7 years ago

Mmm, this is interesting but how would this work on stateless protocols like REST or RESTCONF? Just a NOOP or actually do some lightweight operation to verify device is reachable? We should decide and document that case.

mirceaulinic commented 7 years ago

These drivers are generally HTTP based so we don't need to maintain the session (each command issues a separate HTTP request). In that case, I think we can either leave the body empty (i.e. pass or just return None), either just not implement the method at all.

ktbyers commented 7 years ago

I think it would be more logical to call this session_keepalive or napalm_keepalive (i.e. periodic calling of this will hopefully keep the napalm session alive).

In the case of the SSH transport, I also think we should explicitly define null = chr(0) just to make it a bit clearer. Also the write_channel is a Netmiko method so it would probably be:

    self.device.write_channel(null)
mirceaulinic commented 7 years ago

session_keepalive sounds good. If there aren't any other comments, I think I am going to merge this tomorrow (after rename) and release.

ktbyers commented 7 years ago

That works for me.

dbarrosop commented 7 years ago

I am wondering about this method. I think we need the functionality but not sure about this method. Couldn't we use is_alive to send the null character and return true if it made it through and fail if it didn't? It would serve as keepalive and boolean. Also, it would be consistent with REST based protocols where we test the connection to ensure we can still connect to the device.

mirceaulinic commented 7 years ago

I opened a connection with a device and let it a couple of minutes:

>>> j.open()
>>> j.get_arp_table()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/state/home/mircea/napalm-ios/napalm_ios/ios.py", line 1371, in get_arp_table
    output = self._send_command(command)
  File "/state/home/mircea/napalm-ios/napalm_ios/ios.py", line 141, in _send_command
    output = self.device.send_command(command)
  File "/usr/local/lib/python2.7/dist-packages/netmiko/base_connection.py", line 721, in send_command
    prompt = self.find_prompt(delay_factor=delay_factor)
  File "/usr/local/lib/python2.7/dist-packages/netmiko/base_connection.py", line 612, in find_prompt
    self.write_channel("\n")
  File "/usr/local/lib/python2.7/dist-packages/netmiko/base_connection.py", line 164, in write_channel
    self.remote_conn.sendall(write_bytes(out_data))
  File "/usr/local/lib/python2.7/dist-packages/paramiko/channel.py", line 813, in sendall
    sent = self.send(s)
  File "/usr/local/lib/python2.7/dist-packages/paramiko/channel.py", line 767, in send
    return self._send(s, m)
  File "/usr/local/lib/python2.7/dist-packages/paramiko/channel.py", line 1134, in _send
    raise socket.error('Socket is closed')
socket.error: Socket is closed
>>> j.device.write_channel(chr(0))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/local/lib/python2.7/dist-packages/netmiko/base_connection.py", line 164, in write_channel
    self.remote_conn.sendall(write_bytes(out_data))
  File "/usr/local/lib/python2.7/dist-packages/paramiko/channel.py", line 813, in sendall
    sent = self.send(s)
  File "/usr/local/lib/python2.7/dist-packages/paramiko/channel.py", line 767, in send
    return self._send(s, m)
  File "/usr/local/lib/python2.7/dist-packages/paramiko/channel.py", line 1134, in _send
    raise socket.error('Socket is closed')
socket.error: Socket is closed

So the send null fails itself when the device kills the connection for a reason or another...

mirceaulinic commented 7 years ago

As per error above and internal discussion, we'll introduce the send_alive bits under is_alive. If sending the idle sequence fails for one reason or another, we can safely tell that the connection becomes unusable and is_alive returns False. Let's see how it goes like that. However, I'm leaving this open so we can revisit that later.

dbarrosop commented 7 years ago

Nice! Should we close this one then?

mirceaulinic commented 7 years ago

Yes, let's close this for the moment.