fix(ansible): serialize module resolution

[auspham]

Description of PR

Summary:

Serialize pytest-ansible module discovery to prevent concurrent SONiC neighbor initialization from temporarily reporting existing custom modules such as sonic_basic_facts as missing.

Fixes # (issue)

ADO: 38183684

Type of change

• Bug fix
• Testbed and Framework(new/improvement)
• New Test case
  • Skipped for non-supported platforms
• Test case improvement

Back port request

• 202311
• 202405
• 202411
• 202505
• 202511
• 202512
• 202605

Tracking issue/work item for backport/cherry-pick request: 38183684
Failure type: regression

Approach

What is the motivation for this PR?

PR KVM runs intermittently fail while constructing SONiC neighbor hosts in parallel. AnsibleHostBase.__getattr__ asks pytest-ansible whether sonic_basic_facts exists, but pytest-ansible resolves modules through mutable plugin-loader caches shared by all threads in the current Python process. A concurrent cache update can return a false unresolved result, causing an AttributeError before the test body runs. The issue is active on master and 202605, and most affected plans pass on retry after the loader and facts caches are warm.

How did you do it?

Added a process-local re-entrant lock around the two Ansible module-resolution operations in AnsibleHostBase: checking whether the dynamic module exists and resolving the executable module wrapper. Registered the lock with os.register_at_fork so a forked child reinitializes its copy instead of inheriting a locked guard from a vanished parent thread. Remote Ansible execution remains outside the lock, preserving parallel multi-host operations.

How did you verify/test it?

Reviewed a representative Elastictest failure where one of four SONiC neighbors reported sonic_basic_facts missing while the other three resolved it, followed by a successful module retry. The failing nbrhosts path uses SafeThreadPoolExecutor, which is backed by multiprocessing.pool.ThreadPool; xdist is not the concurrency boundary for the four neighbor constructors. Validated fork safety by holding the lock in a parent thread, forking a child, and confirming the child could acquire the reinitialized lock.

Any platform specific information?

The failure was observed on KVM VS t0-64-32 runs using SONiC neighbors. The fix is in the shared Ansible host wrapper and is not ASIC-specific.

Supported testbed topology if it's a new test case?

Not a new test case.

Documentation

Not applicable.

[azure-pipelines]

Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[lolyu]

📊 Overview

Files Changed: 1 (tests/common/devices/base.py) · +14

Adds a process-wide re-entrant lock (_ansible_module_resolution_lock) around the two pytest-ansible module-resolution operations in AnsibleHostBase — has_module() in __getattr__ and getattr(self.host, module_name) in _run (extracted into _get_ansible_module). Fixes intermittent AttributeError: sonic_basic_facts during parallel SONiC-neighbor construction, where a concurrent update of Ansible's process-global plugin-loader caches returns a false "unresolved" result.

✅ Strengths

• Correct root-cause fix — the bug is a race on Ansible's global mutable loader caches; serializing only the resolution step is the right, minimal scope.
• Remote execution stays outside the lock, so parallel multi-host runs are preserved. Good — the lock covers cache lookup only, not the RPC.
• RLock (re-entrant) is the right choice; matches the existing _signal_patch_lock pattern in the same module.

📝 Review Findings

📝 Minor

• The new comment above the lock is a fragment ("pytest-ansible resolves modules through Ansible's process-global, mutable plugin-loader caches."). Consider adding the consequence ("...so concurrent resolution can return a false negative; serialize it") so the reason for the lock is self-documenting.

💡 Suggestions

• Since the resolve→execute window is now split across two separately-locked sections, there's a tiny theoretical gap where a module resolves as present in __getattr__ but the later _get_ansible_module still races. In practice the loader cache only grows warm (never evicts mid-run) so this is fine, but worth a mental note.

🧪 Testing

Verification is by inspection of a representative Elastictest failure rather than a reproduced run. Given the intermittent, cache-warmup nature that's reasonable, though a note that the affected t0-64-32 KVM plan passed N consecutive runs post-fix would strengthen confidence.

Status

✅ Approved — sound, correctly-scoped concurrency fix that preserves parallelism. Only nits.

[lolyu]

⚠️ Follow-up on my earlier approval — retracting to raise a correctness concern

On a second pass I don't think this fix is sound, and I want to flag it before it merges (and gets backported to 202605).

🚨 Major — a threading.RLock can't serialize multi-process concurrency

_ansible_module_resolution_lock = threading.RLock() only provides mutual exclusion between threads within a single Python process. If the parallel SONiC-neighbor construction that triggers the sonic_basic_facts false-negative happens across separate processes (xdist workers / multiprocessing / forked parallel_run), each process gets its own independent copy of this lock object and its own copy of the module-level globals — so the with blocks serialize nothing across the actual concurrency boundary. The race would still fire.

This creates an internal contradiction with the stated root cause:

• The comment/PR say the bug is a race on Ansible's process-global mutable plugin-loader caches.
• But if the neighbors are built in different processes, those "global" caches are also per-process — there's no shared mutable state between them to corrupt, so a cross-process race on that cache shouldn't exist in the first place.
• Conversely, if the concurrency is actually threads inside one process, then a threading.RLock is the right tool — but then "process-global" is a misleading description and the fix is fine.

Both can't be true. So one of these needs nailing down before merge:

1. Confirm the concurrency model. Are the four SONiC neighbors constructed by threads in one interpreter, or by separate processes (pytest-xdist -n, parallel_run with multiprocessing, forks)? A quick os.getpid() log at the AttributeError site on the representative Elastictest failure would settle it definitively.
2. If it's multi-process, a threading.RLock is the wrong primitive. Options: multiprocessing.Lock/Manager().RLock() (must be created pre-fork and inherited), a filesystem lock (filelock), or — likely cleanest here — warm the loader/module caches once before the fork/parallel fan-out so no worker has to resolve sonic_basic_facts from a cold, mutating cache.
3. If it's genuinely single-process multi-thread, then the fix is correct — please just update the comment to say "threads racing on the shared in-process loader cache" rather than "process-global," so the next reader isn't misled, and note in the description that xdist isn't in play for the affected plan.

📝 Also still open (from my prior review)

• The resolve→execute window is split across two separately-locked sections (__getattr__ vs _get_ansible_module), so even in the threaded case a module can resolve present in __getattr__ and still be re-resolved under a fresh lock acquire in _run. Benign only if the cache never evicts mid-run — worth stating that assumption explicitly.

Status

🚨 Changes requested (supersedes my earlier approval) — need confirmation of the threaded-vs-multiprocess model; if any of the parallelism is process-based, threading.RLock won't fix the reported race and a cross-process mechanism (or pre-fork cache warmup) is required.

[auspham]

Thanks for the follow-up. Replying point by point:

A threading.RLock can't serialize multi-process concurrency.

Agreed as a general statement, but the failing concurrency is not process-based. The nbrhosts fixture submits each initial_neighbor call to SafeThreadPoolExecutor in tests/conftest.py:1212-1230. SafeThreadPoolExecutor is backed by multiprocessing.pool.ThreadPool in tests/common/helpers/multi_thread_utils.py:1-34, whose workers are threads in the same Python interpreter. The representative failure traceback also enters initial_neighbor through multi_thread_utils.py:45.

If the parallel SONiC-neighbor construction happens across separate processes, each process gets its own independent copy of this lock object.

That is true, but it does not describe this path. The four neighbor constructors run as threads and share both the module-level _ansible_module_resolution_lock and Ansible's module-level module_loader. If an outer xdist worker is present, that worker has its own interpreter, loader, and lock; its neighbor threads share that corresponding pair. A cross-process lock would only serialize independent loader instances.

The comment/PR say the bug is a race on Ansible's process-global mutable plugin-loader caches.

Here, "process-global" was intended to mean global within one process and therefore shared by its threads, not shared across processes. I updated the code comment and PR description to use "shared in-process" wording so that scope is explicit.

Confirm the concurrency model. Are the four SONiC neighbors constructed by threads in one interpreter, or by separate processes?

They are constructed by threads in one interpreter. The source path is:

nbrhosts -> SafeThreadPoolExecutor.submit(initial_neighbor, ...) -> multiprocessing.pool.ThreadPool.

At runtime, this executor gives each worker a distinct thread ID but the same PID and shared module-level objects.

If it's multi-process, a threading.RLock is the wrong primitive.

Agreed, but that branch does not apply to the reported failure. The lock and the mutable loader it protects have the same in-process scope.

The resolve-to-execute window is split across two separately locked sections.

The first lookup only decides whether Python should expose the dynamic attribute. The second lookup obtains the executable module wrapper. Both calls that reach pytest-ansible's module_loader.find_plugin_with_context() are serialized by the same RLock. No resolver state from the first call is consumed by the second, so an intervening serialized lookup cannot recreate concurrent cache mutation. The lock intentionally ends before remote module execution so parallel host operations remain parallel.

The implementation is unchanged apart from clarifying the lock scope in the comment and PR description. Please take another look.

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[lolyu]

The concurrency analysis in your thread reply is convincing — SafeThreadPoolExecutor → multiprocessing.pool.ThreadPool means the four neighbor constructors are threads in one interpreter sharing the module-level loader, so an RLock is the right primitive for that boundary. The comment clarification helps.

My main concern is scope rather than direction (inline):

• The lock guards two independent point lookups (has_module in __getattr__, getattr in _run) but not the resolve→execute window as one critical section. A module that resolves at attribute-access can still hit the false-unresolved path at execution time if another thread mutated the shared loader cache in between. This narrows the window rather than closing it — same statistical character as "passes on retry."
• It also serializes every AnsibleHostBase.__getattr__ for the whole session, adding steady-state contention long after the cache is warm, where the race no longer exists.

Both point to the same alternative: prime/warm the plugin-loader cache once under lock before parallel neighbor construction (e.g. in nbrhosts), which eliminates the race at the source and drops the per-access lock. If you'd rather land this narrower fix now for the backport and follow up, that's reasonable — just want the resolve/execute atomicity gap on record.

Non-blocking from me.

[lolyu]

Following up after a closer look at how these device classes are used. AnsibleHostBase is the base for every device class (SonicHost, PTFHost, VMHost, Localhost, all fanout/neighbor host types), and those classes are driven not just by the thread-based SafeThreadPoolExecutor path you analyzed but also by the fork-based parallel_run in tests/common/helpers/parallel.py, which spawns SonicProcess (multiprocessing.Process) workers.

That surfaces a real blocker (inline on the lock definition): a module-level RLock on the hot __getattr__ path, not registered with os.register_at_fork, risks a fork-while-locked deadlock — if any parent thread holds the lock when parallel_run forks, the child inherits it locked with no owner, and the child's first __getattr__ hangs forever. This is the exact failure class parallel.py already goes to great lengths to prevent (all the os.register_at_fork / atfork logging fixups). On top of that, the lock doesn't even serialize the fork path (each child gets its own copy), so the bug class isn't fully closed there.

Requesting changes: at minimum register the lock with os.register_at_fork(after_in_child=..._at_fork_reinit). Better, prime the plugin-loader cache once before parallelism (thread or fork) so no lock is held across a fork at all — that closes the race at the source and avoids the deadlock exposure entirely.

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[mssonicbld]

This PR has backport request label(s) for branch(es): 202605, but is missing required test information. Please make sure you tick the tested branch(es) in the Tested branch section and provide test evidence (e.g., 202605: <test result>) in the Test result section as well in your PR description.

_{---Powered by SONiC BuildBot}

[lolyu]

LGTM, please fix the PR test.

[auspham]

/azpw retry

[mssonicbld]

Retrying failed(or canceled) jobs...

[mssonicbld]

Retrying failed(or canceled) stages in build 1170417:

✅Stage Test:

• Job impacted-area-kvmtest-t1-lag-vpp by Elastictest: retried.

[mssonicbld]

This PR has backport request label(s) for branch(es): 202511,msft-202606,msft-202607,msft-202608, but is missing required test information. Please make sure you tick the tested branch(es) in the Tested branch section and provide test evidence (e.g., 202511: <test result>) in the Test result section as well in your PR description.

_{---Powered by SONiC BuildBot}

[mssonicbld]

Cherry-pick PR to msft-202608: Azure/sonic-mgmt.msft#1355