Issue-406: Added changes for command timeouts (NDA-34)

[jeetugangwar11]

Related Issue(s)

Fixes #406

Proposed Changes

1. Remove timeout from nd_argument_spec() in nd_argument_specs.py.
2. Remove the persistent_command_timeout assignment from nd.py
3. Remove timeout and ND_TIMEOUT from the shared documentation fragment.

Now we can use timeout by providing below ways:

1. By setting in playbook facts:

• name: set facts
  ansible.builtin.set_fact:
  ansible_command_timeout: 100

2. By playbook vars:
   vars:
   ansible_command_timeout: 300
3. By adding below props in inventory:
   ansible_command_timeout=100
4. By exporting variable:
   export ANSIBLE_PERSISTENT_COMMAND_TIMEOUT=300

NOTE:

Variable precedence order - module reads from:

1. Playbook facts
2. Playbook vars
3. Inventory
4. Env varialble ANSIBLE_PERSISTENT_COMMAND_TIMEOUT

Test Notes

I have added test case for the same.
I tested above precedence order in local by adding logs.

Cisco Nexus Dashboard Version

4.3.1

Related ND API Resource Category

NA

Checklist

• Latest commit is rebased from develop with merge conflicts resolved
• New or updates to documentation has been made accordingly
• Assigned the proper reviewers

[mtarking]

Thanks for tackling this — the core fix is exactly right and minimal: dropping the argspec timeout default that unconditionally clobbered persistent_command_timeout, and stopping the httpapi plugin from writing that option at all. test_nd_argument_specs.py ("timeout" not in spec) and the doc-fragment removal are good regression guards, and verify.timeout / persistent_connect_timeout are correctly left untouched.

Requesting changes on a few items before merge (details inline):

1. set_fact: ansible_command_timeout may not reconfigure an already-open persistent socket — it's a connection var resolved at connection setup. The old timeout: applied per-request dynamically; this doesn't. Please verify the migrated targets still get the extended window, or move to play-level vars: / inventory [nd:vars] (as the interface and maintenance-mode targets do). Affects nd_backup, nd_backup_restore, nd_service, nd_service_instance, nd_rest/*, nd_vpc_pair/*.
2. Scope regression — set_fact is play-lifetime and per-host; prefer block-scoped vars: to match the original module_defaults scoping. Also confirm the nd_vpc_pair change from nd_vpc_pair_module_timeout | default(600) to a hardcoded 90 is intended.
3. Verify no residual timeout / ND_TIMEOUT readers — #406 named nd.py/nd_v2.py; please grep to confirm nothing else (e.g. nd_v2.py, sender_nd.set_params(), action plugins) still reads the removed param.

Non-blocking:
4. The vrf_attachment_manager._delete_wait_timeout removal drops a user override of an internal wait ceiling — confirm the adaptive formula covers the large-delete cases it existed for.
5. test_httpapi.py has a tautological assertion and is missing a license/copyright header (sanity may flag it).

Once 1–3 are addressed this looks ready. Nice, well-scoped change overall.

[mtarking]

Blocking: ansible_command_timeout is a connection variable read by ansible-connection when the persistent socket is initialized. Setting it via set_fact after an ND task has already opened the persistent connection may not reconfigure the already-running socket, so the effective persistent_command_timeout can stay at the default 30s.

The old timeout: 90 worked because the httpapi plugin called connection.set_option("persistent_command_timeout", ...) on every request (dynamic). That path is now gone, so timing here depends entirely on native connection-var resolution.

Please confirm empirically that a >30s operation in this target still gets the extended window. If not, move this to play-level vars: / inventory [nd:vars] (as the maintenance-mode and interface targets already do), which is resolved at connection setup and is reliable. Same concern applies to the other set_fact: ansible_command_timeout targets: nd_backup_restore, nd_service, nd_service_instance, nd_rest/*, and nd_vpc_pair/*.

[mtarking]

Scope regression: set_fact persists for the remainder of the play and is per-host, whereas the replaced module_defaults:/inline timeout: was scoped to this specific task file. The block-scoped vars: conversions elsewhere in this PR (e.g. nd_interface_port_channel_*, nd_maintenance_mode) preserve the original scoping and are the better pattern.

Recommend converting these set_fact cases to block/task-scoped vars: for consistency and to avoid leaking a long timeout into unrelated later tasks. Also note this previously honored nd_vpc_pair_module_timeout | default(600) — the new hardcoded 90 drops that override knob and lowers the ceiling from 600→90; confirm that's intended.

[mtarking]

This assertion is tautological: connection.get_option is wired to options.get via side_effect, so it returns 1000 regardless of the plugin's behavior — it doesn't prove anything. The meaningful check is the next line (that set_option was never called with persistent_command_timeout). Suggest dropping this line or replacing it with something that verifies the plugin left the option untouched.

Also: this new file has no GPL/copyright header, unlike the other test files — ansible-test sanity will likely flag a missing license/copyright header. Please add one at the top.

[mtarking]

This removes a real user override, not just a connection timeout: previously an explicit module timeout raised the internal delete-wait ceiling. That's internal query/retry behavior — arguably the same category as verify.timeout, which #406 said to leave unchanged. Removing the argspec key forces this branch to change (it'd always be None), so the edit is justified, but please confirm the adaptive formula min(900, 30 + extra_chunks * delete_wait_delay) actually covers the large-VRF-delete cases this override existed for. The new unit test asserts the formula but not the dropped-override intent.

[mtarking]

cc @AKDRG

[AKDRG]

The removal of explicit_timeout is acceptable. However, the adaptive formula needs more tuning and seems insufficient when scale increases. I have opened #492 for tracking and fixing it.

[mtarking]

Removing the argspec key is the right fix. One verification request: issue #406 explicitly named plugins/module_utils/nd.py and nd_v2.py. This PR changes nd_argument_specs.py (the current shared source) but I don't see those files touched. Please grep the tree for timeout and ND_TIMEOUT to confirm nothing else still reads the removed param — e.g. nd_v2.py, action plugins, or sender_nd.set_params() — so it can't silently resolve to None/absent at runtime.

[allenrobel]

Code review

One docstring-consistency finding, inline below.

🤖 Generated with Claude Code

[allenrobel]

This bullet ("The key set matches the historical spec from nd.py exactly") is now stale: with timeout removed from the asserted key set (and the bullet two lines below updated to say the option is absent), the key set intentionally no longer matches the historical nd.py spec exactly. Suggest rewording, e.g. "The key set matches the historical spec from nd.py, minus the removed timeout option".