Serialize concurrent NAPALM calls on a shared device connection (#55332)#69819
Open
ggiesen wants to merge 1 commit into
Open
Serialize concurrent NAPALM calls on a shared device connection (#55332)#69819ggiesen wants to merge 1 commit into
ggiesen wants to merge 1 commit into
Conversation
…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.
twangboy
approved these changes
Jul 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
multiprocessingdisabled by default (its drivers are SSH-based), so concurrent jobs on one proxy are threads in a single process. In always-alive modeproxy_napalm_wraphands every thread the same cached device object, andsalt.utils.napalm.call()invokes the driver method on the one shared command channel. When two calls land at once -- e.g. anet.cliwhile 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-basedonyxdriver) the session can be corrupted.The fix gives each device a reentrant lock, created in
get_device(), and holds it incall()for the duration of the call:RLock) is required becausecall()re-enters itself (close->open-> re-exec) on aConnectionClosedExceptionreconnect; a plainLockwould self-deadlock.LOCK(hand-constructed in tests, or inherited viainherit_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-- assertscall()enters/exits the device lock around the driver call (fails against current code).test_call_uses_reentrant_lock_on_reconnect-- injects aConnectionClosedExceptionsocall()recurses while holding the lock; guards against a futureLock-for-RLockregression (would deadlock).test_call_without_lock_runs_unserialised-- backwards-compat guard for devices with noLOCK.Also validated against a live Junos device (junos-eznc/NETCONF): two concurrent
clicalls 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'sonyx-- 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 createstests/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).