search

napalm-automation / napalm-junos

Apache License 2.0
22 stars 42 forks source link

Adds optional_arg for enabling keepalives. #147

Closed zachmoody closed 7 years ago

zachmoody commented 7 years ago
Had a quick discussion with @mirceaulinic about adding an optional_arg that's passed to Paramiko's `set_keepalive()`. Here's a pretty quick pass at some working code. If I need some tests or refactoring let me know. Thanks!
coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.06%) to 83.194% when pulling 848e2d5d716e4c94c4a652727527562f7edca12d on zachmoody:keepalive into 7421f9523ce454c475d70f8ea00b5a0808675625 on napalm-automation:develop.

dbarrosop commented 7 years ago

Could you open a PR on the main napalm repo and link it here to document this optional_arg, please?

Thanks!

mirceaulinic commented 7 years ago

I'm very surprised that ncclient does not set a keepalive by default. We should probably replicate this for all devices having SSH-based connection.

Would you agree having a default value for the keepalive, as for timeout, say 30 seconds or so?

dbarrosop commented 7 years ago

Sounds sane to me.

zachmoody commented 7 years ago

I had a look at OpenSSH's stance on keepalives and they're disabled by default as well. Maybe parity with that was what Paramiko, ncclient, etc. were going for. Made a couple more commits to set a default value. Will have a PR raised on napalm-base to get this in all applicable drivers shortly.

mirceaulinic commented 7 years ago

Thanks @zachmoody

zachmoody commented 7 years ago

PR for doc change is https://github.com/napalm-automation/napalm/pull/372

mirceaulinic commented 7 years ago

This will be delayed a bit due to other bugfixes to be addresses in the next release. I hope to have the time for them this week or early next week.