Skip to content

Serialize concurrent NAPALM calls on a shared device connection (#55332)#69819

Open
ggiesen wants to merge 1 commit into
saltstack:3006.xfrom
ggiesen:fix-55332-napalm-cli-race
Open

Serialize concurrent NAPALM calls on a shared device connection (#55332)#69819
ggiesen wants to merge 1 commit into
saltstack:3006.xfrom
ggiesen:fix-55332-napalm-cli-race

Conversation

@ggiesen

@ggiesen ggiesen commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes the race in #55332 where two jobs running against the same always-alive NAPALM proxy can interleave on the device connection.

A NAPALM proxy runs with multiprocessing disabled by default (its drivers are SSH-based), so concurrent jobs on one proxy are threads in a single process. In always-alive mode proxy_napalm_wrap hands every thread the same cached device object, and salt.utils.napalm.call() invokes the driver method on the one shared command channel. When two calls land at once -- e.g. a net.cli while a grains refresh or a state run is in flight -- they interleave on that channel: output gets mixed, and on drivers that share a raw CLI session without their own locking (the reporter used the netmiko-based onyx driver) the session can be corrupted.

The fix gives each device a reentrant lock, created in get_device(), and holds it in call() for the duration of the call:

  • Reentrant (RLock) is required because call() re-enters itself (close -> open -> re-exec) on a ConnectionClosedException reconnect; a plain Lock would self-deadlock.
  • The lock is per-device, not global, so a deltaproxy hosting many sub-proxies in one process does not needlessly serialize calls across unrelated devices.
  • Devices built without a LOCK (hand-constructed in tests, or inherited via inherit_napalm_device) run unserialized, so existing behaviour and callers are unchanged.

The trade-off is intentional head-of-line blocking: a slow or hung command now blocks other jobs (and the keepalive) on that one device until it returns, instead of corrupting the channel.

Fixes

Fixes #55332

Tests

New tests/pytests/unit/utils/test_napalm.py (deterministic, no hardware):

  • test_call_serialises_concurrent_access -- two threads on a shared device; asserts the driver interactions do not interleave (fails against current code, where they do).
  • test_call_acquires_device_lock -- asserts call() enters/exits the device lock around the driver call (fails against current code).
  • test_call_uses_reentrant_lock_on_reconnect -- injects a ConnectionClosedException so call() recurses while holding the lock; guards against a future Lock-for-RLock regression (would deadlock).
  • test_call_without_lock_runs_unserialised -- backwards-compat guard for devices with no LOCK.

Also validated against a live Junos device (junos-eznc/NETCONF): two concurrent cli calls with this change are serialized on the channel (non-overlapping) and both return correct output; against the current code the two calls overlap on the channel. (On Junos the overlap did not corrupt output because ncclient serializes NETCONF at the transport layer -- the visible corruption is specific to drivers that share a raw CLI channel, like the reporter's onyx -- but the concurrent overlap is real either way, and this change serializes it for all drivers.)

Note

This touches the same functions (get_device/call) as #69796 and also creates tests/pytests/unit/utils/test_napalm.py, so a small merge-order conflict with that PR is expected (both add test functions to the same new file; they combine cleanly).

…stack#55332)

An always-alive NAPALM proxy runs with multiprocessing disabled, so jobs
executing at the same time are threads that share one cached device object and
its single command channel (get_device returns the same device by reference).
Two concurrent calls -- e.g. net.cli, or a grains refresh landing during a
state run -- can then interleave on that channel, mixing each other's output
and, on drivers that share a raw CLI session without their own locking,
corrupting the connection.

Give each device a reentrant lock, created in get_device(), and hold it in
salt.utils.napalm.call() for the duration of the call. A reentrant lock is
required because call() re-enters itself (close/open/re-exec) on a reconnect;
a plain Lock would deadlock. Devices built without a LOCK (hand-constructed in
tests, or inherited via inherit_napalm_device) run unserialized, unchanged.

The lock is per-device, not global, so a deltaproxy hosting many sub-proxies
in one process does not needlessly serialize calls across unrelated devices.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test:full Run the full test suite

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants