[code sync] Merge code from sonic-net/sonic-utilities:202605 to 202607 - #440
Merged
Conversation
mssonicbld
commented
Aug 25, 2026
mssonicbld
commented
Collaborator
…ure (#4797) #### What I did Made `fetch_routes()` terminate the entire `vtysh` process group when the streaming JSON parse fails, instead of logging a warning and calling `proc.wait()` on a child it never drained. `fetch_routes` runs `sudo vtysh -c "show ip route json"`. On a SONiC host `/usr/bin/vtysh` is a wrapper script that runs `docker exec -i bgp vtysh ...`, so the process actually holding the exec session into the bgp container sits two levels below the child `Popen` returns: ``` sudo <- Popen's direct child └─ /bin/bash /usr/bin/vtysh <- wrapper └─ docker exec -i bgp vtysh -c "show ip route json" ┊ containerd-shim └─ vtysh (in container) <- blocked in write() ``` When the parse raised, the read was abandoned while `vtysh` still had megabytes queued. On a route table larger than the 64KB pipe buffer the in-container `vtysh` stays blocked in `write()`, so `proc.wait()` never returns and the exec session is never released. From that point the bgp container can no longer be stopped, and every later `docker exec` into it fails in `setns`: ``` OCI runtime exec failed: exec failed: unable to start container process: error starting setns process: fork/exec /proc/self/fd/6: no such file or directory ``` `docker ps` still reports the container as `Up`, so this presents as a healthy container that nothing can talk to. The exposure was introduced when `fetch_routes` moved from a single full read to a streaming parse. Until then it was `subprocess.check_output(cmd, text=True)` followed by `json.loads(output)`: stdout is drained to EOF before anything is parsed, so a parse error is raised after the child has already exited, and there is no state in which the reader stops while the writer still has data queued. Streaming with `ijson` made that state reachable — any exception leaving the `ijson.kvitems` loop ends the read for good, while `vtysh` may still have megabytes to write into a 64KB pipe, so it blocks deterministically rather than as a race. #### How I did it - `subprocess.Popen(..., start_new_session=True)`, so the child leads its own process group and the wrapper plus the `docker exec` client inherit it. This also isolates the concurrent IPv4 and IPv6 fetches from each other and from route_check's own process group. - New `terminate_vtysh_process_group()` helper that does `os.killpg(proc.pid, SIGKILL)` and reaps with a bounded `wait()`. Signalling is best effort: a cleanup failure must never mask the error that got us there. It is reported at `WARNING` rather than `DEBUG` because a failed `killpg` is the only early signal that this cleanup did not take effect, and the default report level hides `DEBUG`. - On any parse failure the helper runs before `proc.wait()`, which is what releases the exec session. Error handling is otherwise untouched: parse failures, a missing command and unexpected errors are still logged and swallowed, and `fetch_routes` still returns whatever it managed to parse. This PR changes cleanup only. One thing worth calling out for review: the helper deliberately passes `proc.pid` rather than `os.getpgid(proc.pid)`. It is only correct because `start_new_session=True` makes pgid == pid; deriving the group would target route_check's *own* group if the session was ever not created. There is a unit test guarding this. Killing only the direct child — `proc.kill()` — does not fix this. `SIGKILL` cannot be propagated by `sudo`, so the wrapper and the `docker exec` client are orphaned and keep the exec session open. Measured on a switch with a 6.7MB route table, six concurrent abandoned parses, with the container-recreate workaround removed so the container ID is reused on restart: | route_check behaviour | sudo | wrapper | docker exec client | in-container vtysh | container stop | result | |---|---|---|---|---|---|---| | no kill (before this change) | alive | alive | alive | alive | 15s + 15s timeouts | every later `docker exec` hangs | | `proc.kill()` | dead | **alive** | **alive** | **alive** | 15s + 15s timeouts | every later `docker exec` hangs | | `os.killpg(SIGKILL)` (this change) | dead | dead | dead | **dead** | **clean, no timeouts** | healthy | Note that the in-container `vtysh` is reaped indirectly: killing the host-side `docker exec` client drops the attach, the daemon closes the exec stdout stream, and the in-container process gets `EPIPE`. The container-side process is not in the signalled group. #### How to verify it Unit tests: ``` pytest tests/route_check_test.py -k TestFetchRoutes -v ``` `test_terminates_whole_process_group_on_parse_error` is the regression guard. It builds a three level process chain whose *grandchild* floods the pipe — the stand-in for the `docker exec` client — feeds `fetch_routes` truncated JSON, and asserts the call still returns `([], [])` while the grandchild is gone and the group no longer exists. Against the previous code it does not fail fast, it deadlocks: the call is made on a worker thread so the hang is reported as a failure after 60s instead of stalling the run. On a switch, with the container-recreate workaround removed from `bgp.sh` so the container ID is reused: 1. Wait for BGP to converge to a route table well over 64KB (a few MB makes it deterministic). 2. Drive six concurrent `fetch_routes()` calls with `ijson.kvitems` patched to consume a chunk, wait long enough for the pipe to fill, then raise. 3. Before the kill, confirm the chain really formed: six `docker exec -i bgp vtysh` clients on the host and six in-container `vtysh` processes with `wchan == pipe_write`. 4. After the kill, all of those counts must be zero. 5. `systemctl restart bgp`, then confirm the journal has no `Container failed to exit within ...` lines and that a burst of parallel `docker exec bgp true` all succeed. Result of that run: preconditions met (6/6 and 6/6 before the kill), everything reaped afterwards, container stop clean with zero timeout messages even though the container ID was reused, and 240/240 parallel `docker exec` returned 0. A normal `route_check.py -m DEBUG` run against the same 6.7MB table completed with exit 0 and empty missing/failing lists. Signed-off-by: Sonic Build Admin <sonicbld@microsoft.com>
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
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
1 participant
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.