Skip to content

Fix EOSDevice.close() leaking the Netmiko SSH session opened for file copy - #424

Open
jvanderaa wants to merge 2 commits into
networktocode:developfrom
jvanderaa:fix/eos-close-netmiko-session
Open

Fix EOSDevice.close() leaking the Netmiko SSH session opened for file copy#424
jvanderaa wants to merge 2 commits into
networktocode:developfrom
jvanderaa:fix/eos-close-netmiko-session

Conversation

@jvanderaa

Copy link
Copy Markdown
Contributor

New Pull Request

Have you:

  • Updated the README if necessary? — no user-facing docs change; this restores documented close() behavior.
  • Updated any configuration settings? — none needed.
  • Written a unit test? — four, covering the leak, the never-opened path, idempotency, and reopen-after-close.

Closes #423

Change Notes

EOSDevice.close() was pass. It now disconnects the Netmiko SSH session when _connected is set, matching IOSDevice.close().

The _connected guard matters: native_ssh is only assigned inside open(), so an unguarded disconnect would raise AttributeError on a device that has only ever spoken eAPI. With the guard, a pure-eAPI caller that never copies a file sees byte-for-byte the old no-op behavior.

Also removes EOSSSHDevice.close(), which became an exact duplicate of the parent — native_ssh on that class is a property returning native, so both bodies make the same call. Its docstring also claimed EOSDevice.close is a no-op, which is no longer true. Happy to restore the override if reviewers prefer it kept explicit.

Tests added to tests/unit/test_devices/test_eos_device.py (there were previously none for close() — it was only ever mocked out):

Test Pins
test_close_disconnects_netmiko_session the leak itself
test_close_is_noop_when_never_opened no AttributeError on an eAPI-only device
test_close_is_idempotent second call does not re-disconnect
test_close_does_not_strand_the_device open() rebuilds a torn-down session

The first two are the ones that failed before the fix; the last two guard properties that already held.

Justification

EOSDevice is a hybrid driver. show and config go over eAPI, but file_copy, check_file_exists, get_remote_checksum and remote_file_copy each call open(), which builds a real Netmiko session. Nothing released it, so a caller holding many device objects accumulated one open socket per device.

eAPI itself is stateless HTTP and needs no teardown, so the no-op was reasonable before file copy arrived over SSH — but it silently stopped being correct once it did.

Risk

Low. Nothing inside pyntc calls EOSDevice.close(); the only in-library callers of close() are in the ios, nxos, iosxr, aireos and jnpr drivers, each on their own class. So the behavior change is scoped to external callers and explicit teardown, which is exactly the leak being fixed.

Every SSH-backed method calls self.open() first, and open() rebuilds the session when _connected is False, so a closed device object stays reusable rather than being stranded. That is what test_close_does_not_strand_the_device locks in.

Found while building the arista_eos_ssh driver (#419); filed separately to keep that PR scoped.

close() was a no-op, but open() creates a real SSH session for the file-transfer paths (file_copy, check_file_exists, get_remote_checksum, remote_file_copy). A caller holding many device objects accumulated one open socket per device.

Disconnect when _connected is set, matching IOSDevice.close(). The guard also keeps close() safe on a device that only ever spoke eAPI, since native_ssh is assigned inside open().

Removes EOSSSHDevice.close(), now an exact duplicate of the parent: native_ssh on that class is a property returning native, so both bodies make the same call.
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.

EOSDevice.close() does not release the SSH session opened for file copy

1 participant