systemvm: fix subprocess deadlock in router health check scripts - #13737
systemvm: fix subprocess deadlock in router health check scripts#13737richevanscybermyte wants to merge 1 commit into
Conversation
The health check scripts run shell commands with Popen(stdout=PIPE) and call wait() before reading the pipe. When the command output exceeds the OS pipe capacity (64 KiB on Linux), the child blocks writing to the full pipe and the parent blocks in wait() forever. This is the deadlock warned about in the Python subprocess documentation. On routers with large iptables rule sets this makes iptables_check.py hang on every run of the advanced health check. A new stuck process chain accumulates each advanced-check interval (10 minutes by default) until the router exhausts memory and swap, keepalived's track script starves, and the router enters FAULT and reboots. On a 512 MB router this produced a reboot roughly every 9 hours, and redundant pairs failed together because their timers are synchronized at creation. Drain the pipe with communicate() and check returncode instead of calling wait() with an unread pipe. The same latent pattern is also fixed in memory_usage_check.py, cpu_usage_check.py and gateways_check.py. Fixes: apache#13735
|
@blueorangutan package |
|
@weizhouapache a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #13737 +/- ##
============================================
- Coverage 19.65% 19.64% -0.01%
+ Complexity 19792 19787 -5
============================================
Files 6368 6368
Lines 574881 574881
Branches 70351 70351
============================================
- Hits 112970 112958 -12
- Misses 449639 449650 +11
- Partials 12272 12273 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18699 |
|
@blueorangutan test |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-16645)
|
There was a problem hiding this comment.
Pull request overview
This PR updates multiple SystemVM (virtual router) health check scripts to avoid a known subprocess deadlock pattern (wait() with an unread stdout=PIPE) by draining output via communicate() and checking returncode, addressing the process/memory leak scenario described in issue #13735.
Changes:
- Replace
pout.wait()+ later pipe reads withstdout, _ = pout.communicate()andpout.returncodechecks in health check scripts that spawn shell commands. - Apply the same deadlock-avoidance pattern consistently across iptables, CPU, memory, and gateway reachability checks.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| systemvm/debian/root/health_checks/iptables_check.py | Drains subprocess stdout via communicate() to prevent pipe-buffer deadlocks in the iptables rules check. |
| systemvm/debian/root/health_checks/memory_usage_check.py | Switches to communicate() before interpreting command output to avoid deadlocks. |
| systemvm/debian/root/health_checks/cpu_usage_check.py | Switches to communicate() before interpreting command output to avoid deadlocks. |
| systemvm/debian/root/health_checks/gateways_check.py | Drains ping command output via communicate() before checking exit status to avoid deadlocks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fetchIpTableEntriesCmd = "iptables-save | grep " + destIp | ||
| pout = Popen(fetchIpTableEntriesCmd, shell=True, stdout=PIPE) | ||
| if pout.wait() != 0: | ||
| stdout, _ = pout.communicate() | ||
| if pout.returncode != 0: | ||
| failedCheck = True |
Description
This PR fixes #13735.
The router health check scripts run shell commands with
Popen(cmd, shell=True, stdout=PIPE)and then callpout.wait()before reading the pipe. When the command output exceeds the OS pipe capacity (64 KiB on Linux), the child process blocks writing to the full pipe and the parent blocks inwait()forever. This is the deadlock warned about in the Pythonsubprocessdocumentation.In
iptables_check.pythe command isiptables-save | grep <destIp>. On routers with large port-forwarding rule sets the output exceeds the pipe buffer and the check hangs on every run. Because the advanced health check fires everyrouter.health.checks.advanced.interval(10 minutes by default), a new stuck process chain (monitorServices.py advanced->sh->iptables_check.py) accumulates every interval, about 13 MB RSS each. On a 512 MB router, RAM and swap are exhausted after roughly 9 hours, keepalived's heartbeat track script can no longer fork within its timeout, the router enters FAULT, demotes, wipes its keepalived config and reboots. Redundant pairs fail near-simultaneously because both routers are created together, so the exhaustion clocks are synchronized. See the issue for full evidence, including a control group of routers with small rule sets running the same check cleanly for 152 days.The fix drains the pipe with
communicate()and checksreturncode, instead of callingwait()with an unread pipe. The same latent pattern is fixed inmemory_usage_check.py,cpu_usage_check.pyandgateways_check.py; their outputs are normally tiny so they do not hang today, but they share the same hazard.Types of changes
Bug Severity
How Has This Been Tested?
Reproduced on CloudStack 4.22.0.0/4.22.1.0 (KVM, redundant VPC, systemvm template 4.22.0) with a VPC whose
iptables-saveoutput is 77,029 bytes. Before the fix, every advanced health check run left a permanently stuckiptables_check.pychain (verified viaps, both processes indo_wait) and router memory climbed about 87 MB/h until the router self-rebooted at about 9 hours of uptime.With the fixed script logic (equivalent change validated on the affected router), the check completes in under a second against the same 77 KB rule set, no processes accumulate, and the router has run past the previous failure window with flat memory usage. Routers with small rule sets (1-9 KB) behave identically before and after, since their output never filled the pipe.