gnoi-shutdown: use per-thread DB connections to avoid crossed DPU reads

[gpunathilell]

What I did

gnoi-shutdown-daemon spawns one worker thread per DPU on shutdown, but all threads shared a single CONFIG_DB/STATE_DB redis connection created once in main(). A redis DBConnector is a single, non-thread-safe socket, so when multiple DPUs shut down in parallel their hget/Table reads interleave on the wire and replies get matched to the wrong request — producing crossed IP/port values between DPUs.

This gave gnoi_client a bad target like 169.254.200.1:169.254.200.2 (dpu0's IP as host, dpu1's IP in the port slot), causing:

ERR gnoi-shutdown-daemon: DPU1: Reboot command failed (rc=2, target=169.254.200.1:169.254.200.2):
panic: rpc error: ... dial tcp: lookup tcp/169.254.200.2: unknown port

Fix

Give each per-DPU worker thread its own CONFIG_DB/STATE_DB connection so reads can never cross:

• handle_and_cleanup() opens fresh db_connect("CONFIG_DB") / db_connect("STATE_DB") per thread and passes them into _handle_transition().
• _handle_transition() / _wait_for_gnoi_halt_in_progress() take optional per-thread connections, falling back to the shared ones for direct/unit-test callers (backward compatible).

Testing

• py_compile clean.
• Smoke-tested both paths: per-thread connections are the ones actually used (correct 169.254.200.2:50052), and fallback-to-shared preserves existing unit-test behavior.

sonic-net/sonic-host-services#408

Summary by CodeRabbit

• Bug Fixes
  • Improved reliability of gNOI HALT handling during concurrent DPU shutdown operations.
  • Prevented database connection conflicts between background shutdown tasks.
  • Improved status monitoring while waiting for shutdown completion.

[coderabbitai]

📝 Walkthrough

Walkthrough

The gNOI shutdown daemon now creates per-thread CONFIG_DB and STATE_DB connections for DPU transitions and passes them through HALT handling, configuration lookup, progress waiting, and reboot-status polling.

Changes

gNOI HALT thread safety

Layer / File(s)	Summary
Handler connection selection scripts/gnoi_shutdown_daemon.py	_handle_transition and _wait_for_gnoi_halt_in_progress accept optional database connections, using them for state polling and DPU configuration lookup while preserving shared-connection fallbacks.
Per-thread connection wiring scripts/gnoi_shutdown_daemon.py	Each transition thread creates dedicated CONFIG_DB and STATE_DB connections and passes them to _handle_transition.

Estimated code review effort: 3 (Moderate) | ~20 minutes

🚥 Pre-merge checks | ✅ 5 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Dco Signoff	⚠️ Warning	HEAD commit f68f548 has no Signed-off-by line; committer is Gagan Ellath gpunathilell@nvidia.com.	Add a Signed-off-by: Gagan Ellath gpunathilell@nvidia.com line to each commit message in the PR.

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: per-thread DB connections to prevent crossed DPU reads in gnoi-shutdown.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[sw-r2d2-bot]

✅ SONiC CI #4569 finished with status: SUCCESS

[dgsudharsan]

PR Review

Summary

This PR fixes a real concurrency bug in gnoi-shutdown-daemon. When multiple DPUs shut down in parallel, worker threads shared a single Redis DBConnector socket. Concurrent hget/Table.get calls interleaved on the wire, causing replies to be matched to the wrong requests — producing crossed IP/port values like 169.254.200.1:169.254.200.2.

The fix gives each per-DPU worker thread its own CONFIG_DB and STATE_DB connection and threads them through _handle_transition() and _wait_for_gnoi_halt_in_progress().

---

Root Cause Analysis — Correct

The diagnosis is sound. swsscommon DBConnector is a single non-thread-safe Redis socket. The failure mode (DPU0's IP with DPU1's value in the port slot) is consistent with crossed read replies on a shared connection.

The threaded shutdown path in handle_and_cleanup() previously called _handle_transition() using self._config_db and self._db created once in main().

---

Code Review

What looks good:

1. Targeted fix — Only the DB reads inside the threaded path are changed. The main pubsub loop still uses the shared config_db safely (single-threaded).
2. Backward-compatible API — Optional config_db/state_db parameters with fallback to self._config_db/self._db preserve existing unit-test call sites.
3. All threaded DB access is covered:
   • _wait_for_gnoi_halt_in_progress() → state_db
   • get_dpu_ip() / get_dpu_gnmi_port() → config_db
   • _clear_halt_flag() uses chassis platform API, not Redis — correctly left alone
4. Clear inline comment in handle_and_cleanup() explaining why per-thread connections are needed.
5. Low risk — Small, focused diff with a clear causal link to Redmine #5136310.

Minor suggestions (non-blocking):

1. Add a unit test for the per-thread path — Existing tests call _handle_transition() directly (fallback path) or mock threading.Thread in test_main_loop_flow, so the new db_connect calls in handle_and_cleanup are never exercised. A small test that invokes handle_and_cleanup directly and asserts db_connect("CONFIG_DB") / db_connect("STATE_DB") are passed into _handle_transition would lock in the fix and prevent regressions.

2. Consider logging the gNOI target — The crossed-read bug produced target=169.254.200.1:169.254.200.2. Logging dpu_ip:port in _send_reboot_command before the RPC would make future misconfiguration easier to spot. Optional, not required for this fix.

3. Connection lifecycle — Per-thread connections are not explicitly closed. For short-lived shutdown threads this is fine and matches common SONiC daemon patterns. No action needed unless you see connection buildup under repeated shutdown cycles.

---

Testing Assessment

Area	Status
py_compile	Author verified
Existing unit tests	Should pass (fallback path unchanged; Thread mocked in main-loop test)
Concurrent DPU shutdown	Author smoke-tested manually
Automated concurrent-path test	Missing — recommend adding
SONiC CI	Passed

---

Process / Compliance

• DCO sign-off missing — Commit f68f548 has no Signed-off-by line. Other commits in this repo include it. Please add before merge:

  Signed-off-by: Gagan Ellath <gpunathilell@nvidia.com>
  

---

Verdict

Approve with minor nits.

The root cause is correctly identified, the fix is the right approach for parallel DPU shutdown, and the change is minimal and low-risk. I'd merge after:

1. Adding the DCO Signed-off-by line
2. (Recommended) Adding a unit test that verifies per-thread DB connections are created and passed through in handle_and_cleanup

The crossed-read failure (169.254.200.1:169.254.200.2 → lookup tcp/169.254.200.2: unknown port) should be resolved by this change. Cherry-pick to 202605_RC as noted in the PR description is appropriate given where the bug was seen.