search

napalm-automation / napalm-ios

Apache License 2.0
31 stars 40 forks source link

Add SSH keepalive #149

Closed mirceaulinic closed 7 years ago

mirceaulinic commented 7 years ago

Under https://github.com/napalm-automation/napalm-junos/pull/147 an user has added the keepalive optional argument. We have then thought that it would be good to leverage this option for other SSH-based drivers. Opening this PR to add the new keepalive optional arg.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.06%) to 67.484% when pulling 1d78cc4da687bb4facb6fb1581aee807e20b134c on ssh-keepalivd into dcd0ade538eab448c116664339d107bf16c578ec on develop.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.06%) to 67.484% when pulling 1d78cc4da687bb4facb6fb1581aee807e20b134c on ssh-keepalivd into dcd0ade538eab448c116664339d107bf16c578ec on develop.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.06%) to 67.484% when pulling 1d78cc4da687bb4facb6fb1581aee807e20b134c on ssh-keepalivd into dcd0ade538eab448c116664339d107bf16c578ec on develop.

mirceaulinic commented 7 years ago

@ktbyers is this something we may want under this driver, or do you think we should move it more rather under netmiko though? I thought that we can add this optional arg also under panos & others that use SSH and implicitly netmiko - let me know if that would make sense.

ktbyers commented 7 years ago

@mirceaulinic Yeah, it is probably more logical under Netmiko, let me look at it a bit more.

mirceaulinic commented 7 years ago

@ktbyers I submitted a pull request to Nemiko for this: https://github.com/ktbyers/netmiko/pull/476. If that's sane, I'll adjust this PR so napalm-ios will use the new argument.

ktbyers commented 7 years ago

@mirceaulinic That PR has been included in Netmiko.

mirceaulinic commented 7 years ago

Thanks for reminder @ktbyers - I will adapt the code after Netmiko is released, otherwise I think the tests will fail.

ktbyers commented 7 years ago

Waiting on Netmiko release.

ktbyers commented 7 years ago

@mirceaulinic Netmiko has been released with support for Paramiko Keepalive.

Do we still need this PR any more? I assume we will just close this and keep the is_alive PR?

mirceaulinic commented 7 years ago

Yes, we still need this PR, I will update it shortly. The keepalive is independent of the NULL byte: it's used to survive firewall purges even when is_alive is not called. For example, under napalm-junos we have keepalive, but we won't be sending any equivalent of the NULL byte (otherwise NETCONF will close the connection),

mirceaulinic commented 7 years ago

@ktbyers I have updated this PR and we'll be using Netmiko's keepalive arg directly - sorry for late reply here.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+2.8%) to 70.221% when pulling 15856d4bfd5f435dd4410d298b4b12db57c5438d on ssh-keepalivd into dcd0ade538eab448c116664339d107bf16c578ec on develop.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+2.8%) to 70.221% when pulling 15856d4bfd5f435dd4410d298b4b12db57c5438d on ssh-keepalivd into dcd0ade538eab448c116664339d107bf16c578ec on develop.