Repository navigation
tunnel: improve startup resilience and diagnostics - #2438
Conversation
PR Summary by QodoHarden VTun startup and share tunnel script helpers
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
Code Review by Qodo
1. All images ship a tunnel-only helper
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
|
Why is the rule violation still reported if the corresponding issue was dismissed? |
openipc-ai
left a comment
openipc-ai
left a comment
There was a problem hiding this comment.
The design here is right and it works. I ran it on hardware rather than reading it, and nothing I found blocks it on correctness — the only thing standing in the way is that CI has never run (see the bottom).
Verified on a lab T31 camera
Copied the three files from bef20949 to /tmp on the board (the . /usr/sbin/common line rewritten to point at the temporary copy, so nothing persistent was touched) and ran tunnel directly.
Normal path, server 127.0.0.1:
$ sh /tmp/tt/tunnel 127.0.0.1
tunnel
script returned rc=0 immediately
$ ps w | grep -E 'vtund|tunnel'
19846 root 0:00 sh /tmp/tt/tunnel 127.0.0.1
19868 root 0:00 vtund[c]: 380146A2E638 connecting to 127.0.0.1
$ ifconfig tunnel
tunnel Link encap:Ethernet HWaddr 38:01:46:A2:E6:38
UP BROADCAST MTU:1500 Metric:1
The interface is up with the right MAC before vtund starts, so the EIO workaround does what it claims. The generated config is correct, including the quoted pid-file paths.
Unresolvable host, server no-such-vtun-host.invalid — one line, then silence for 45 s, no config written, vtund never started:
Sep 18 11:27:42 t31-sc2332 daemon.warn tunnel[19905]: waiting for a default route and for no-such-vtun-host.invalid to resolve
That is the fix working. Cleaned up afterwards; the board is back as it was.
All the no-hardware gates pass on the branch: test_shell_parse.sh, test_strip_shell_comments.sh, test_sysupgrade.sh, test_check_mac.sh, test_load_hisilicon.sh, test_excludes_report.sh, ci-matrix.py --self-test, lint-workflow-shell.py --self-test.
Four things that look like defects and are not
Recording these so nobody has to re-raise them:
modprobe tun || breakon boards that build tun in. 31 defconfigs haveCONFIG_TUN=y— all the sigmastar infinity6/6b0/6c/6e boards, xm510/xm530/xm550, rv1103/rv1106, v851s, hi3518ev300_ultimate — so there is notun.ko. busybox modprobe readsmodules.builtinas well asmodules.dep, Buildroot installs it, andkernel/drivers/net/tun.kois listed in it on ssc325 and ssc378de. Confirmed on two cameras:modprobe unix(built in) returns 0,modprobe definitely_no_such_module_xyzreturns 1.ifconfig … hw ether … up || breakon the second and later passes, when the interface is already UP.tun_net_initsetsIFF_LIVE_ADDR_CHANGE, soeth_prepare_mac_addr_changeskips itsnetif_running→EBUSYbranch. Checked in the 4.9.37/4.9.84/5.10.61 trees and confirmed on hardware (MAC changed on an up tap, rc=0).- The single quotes in
-p '$udhcpc_pid'andkill_pid_file -9 '$udhcpc_pid' udhcpc. vtun'sprogramstatement with no leadingPATHtoken leavescmd->progNULL, sorun_cmd()inlib.cexecs/bin/sh -c <args>rather thansplit_args+execv. The shell strips the quotes and searches$PATH, and/usr/sbinis on the bootPATH(checked/proc/<pid>/environof a boot-started daemon). So the wrapper resolves and the paths are right. - Dropping the
^\[branch. Correct: vtun 3.0.2 has noAF_INET6,sockaddr_in6orgetaddrinfoanywhere in its sources.
Findings
1. The flood most users will actually see is inside vtund, and this PR cannot reach it. With persist yes, vtund does not exit when the server is unreachable — client() loops internally on a hard-coded sleep(5) (there is no knob for it; the per-host timeout 10; is the connect timeout) and logs two daemon.info lines per attempt:
Sep 18 11:27:07 t31-sc2332 daemon.info vtund[19868]: Connecting to 127.0.0.1
Sep 18 11:27:07 t31-sc2332 daemon.info vtund[19868]: Connect to 127.0.0.1 failed. Connection refused(146)
24 lines/min, so the 64 KB ring S01syslogd -C64 sets up is overwritten in about half an hour, and the 10 s pacing never runs because vtund never returns to the shell. This PR fixes the config-error flood, which was the real root cause of #2319 (empty identity → bare { on line 5), and that is the right fix. It is just worth saying so on #2319 and #2406, so "vtun floods syslog" is not closed wholesale.
2. The syslog tag names a pid that has already exited. prog="${0##*/}[$$]" is evaluated before done &, and $$ is the parent shell's pid, not the background subshell's. In the run above the log line says tunnel[19905] while ps shows the loop as 19906. run_cmd_internal recomputes the same thing and has the same problem. Either drop [$$] or compute the tag inside the subshell.
3. A persistent setup failure still fills the ring, just ~50× slower. run_cmd_nonfatal logs unconditionally on every failure — measured 2 syslog lines per failed ifconfig, so 12 lines/min at the 10 s cadence. Given the PR's purpose it would be worth logging the first failure and then backing off (10 → 30 → 300) or staying quiet until the state changes, the way ready() already does so nicely.
4. ready()'s one warning never repeats, and does not say which precondition failed. A camera that waits forever logs a single line at boot and is then silent. It is also possible to wait forever on a route the awk does not parse — ip r printing default dev ppp0 scope link makes $5 equal scope, the MAC read fails, and ready never returns 0. Naming the failing precondition, and re-logging occasionally, would make that diagnosable.
5. general/overlay/usr/sbin/common is a sourced library living executable in a $PATH directory under a very generic name. Every camera gets a command called common that does nothing when run. /usr/share/openipc/common.sh at mode 0644 would say what it is. It also carries no comments at all — and since shipped scripts are comment-stripped at build time, a header explaining what may source it and what kill_pid_file's 251/252/253 mean costs nothing on the camera.
6. On where kill_pid_file belongs. Your "it is a generic utility, not VTun-specific" answer is reasonable, and there is a third option that fits this tree better than either side of that thread: general/overlay/usr/sbin/extutils is already the multi-call host for shared overlay helpers — check_mac, cli, get_mac, set_mac, netip_hash, sysinfo and ipctool are all symlinks into it. Adding kill_pid_file as an applet there gives you the generic utility you want without a new standalone wrapper, and sidesteps the "package-specific file in the unconditional overlay" objection.
To your question about why the violation is still reported: the dismissal struck the inline comment (it renders with strikethrough), but the summary comment posted at 09:05 is a separate, static comment that the bot does not rewrite afterwards. The two disagree by design.
7. Two behaviour changes are hidden under a "Refactor" title. In wireguard, ( … ) >>/tmp/wireguard.conf became { … } >, i.e. append became truncate. That is a correct fix — a manual re-run used to append a second copy, though at boot /tmp is empty so it never bit in the field — but nothing in the title or description says so. run_cmd's non-fatal path also now returns the real exit status instead of a bare return, which the new tunnel depends on. Both are improvements; they should just be visible. Related: the PR title becomes the squash subject, and this tree's style is area: lowercase imperative summary, so something like tunnel: wait for the network before writing a vtund config would fit better than "Refactor wireguard and tunnel scripts".
8. Small things.
- All four touched files now end without a trailing newline;
general/overlay/usr/sbin/wireguardhad one before this PR. - Stray tab after
shiftinkill_pid_file. tunctl's success output now reaches the boot console — it prints the interface name, andrun_cmd_nonfatalechoes captured output on success when no variable is given. The old script had>/dev/null 2>&1.- The
# OpenIPC.org | v.20230212 / by Igor Zalatovheader was dropped fromtunnel, while the siblingtapipin the same package keeps its equivalent. The# Busybox applets:line was useful documentation too — the script now additionally needsgrep,logger,nslookup,ifconfig,killandrm. - The comment on the
ifconfigline is a trailing comment, sostrip-shell-comments.awkleaves it in the shipped file. A full-line comment above the command would be stripped at build and cost the camera nothing.
Before this can merge
Every workflow on bef20949 is sitting in action_required — this is a fork PR whose runs a maintainer has never approved, so there is no CI on it at all yet. general/overlay/usr/sbin/common widens the matrix to all 99 boards, which is right for a file that enters every image (about +684 bytes compressed). I am approving the runs now; it needs to go green before merge.
One more piece of housekeeping: #2406 is a draft of mine covering the same ground in a single file. If this lands, that one should be closed in its favour.
usa-
changed the title
|
Regarding finding 4: The The changes to Regarding findings 5 and 6: Would you be OK with moving both the shared functions currently in
Regarding finding 8: Very little of the original Is the I will also clean up the minor formatting issues mentioned in this finding, such as trailing newlines and whitespace. Regarding the new commit: Could you please review the current version of the PR? |
ecb9dd3 to
3c4871c
Compare
|
Rebased onto master and CI approved. #2440 landed as 947a366, so For the record, since it was the reason this PR went red rather than anything you wrote: both The
Three small things left, none of which need to hold this up:
Still open from the earlier review and worth a maintainer's opinion rather than yours: where The full 99-board matrix is running now — the overlay file reaches every image, so |
|
Ran The backoff works, and the pid fix works. Unresolvable server, warnings at increasing intervals, and note the tag: 10 s, 20 s, 30 s, 41 s — climbing as intended. And Normal path, literal Interface up with the right MAC before To be clear about what this is and is not: a T31 exercising the scripts, not any of the SoCs in your test matrix, and not a |
|
So at this point, are we just waiting for the maintainer's opinion on where Personally, I’m leaning towards moving both of them into |
openipc-ai
left a comment
openipc-ai
left a comment
There was a problem hiding this comment.
Green across all 99 boards, including gk7205v300_lite and hi3516ev300_lite, which needed #2440 first.
Everything actionable from the review is addressed across ecb9dd35 and 66ba0112: the pid the syslog tag names, backoff on both the readiness wait and the restart loop, ready saying which precondition failed, the restart-delay reset gated on vtund actually exiting cleanly, the informational line moved back to daemon.info, and the comment placed where strip-shell-comments.awk will take it out of the shipped file. Verified the revised script on a lab T31 — backoff climbing 10/20/30/41 s, and the tag matching the ps pid.
Two things stay open and are mine to carry, not yours: where /usr/sbin/common should live, and whether kill_pid_file belongs in extutils. Both are placement questions about files this PR introduces correctly; neither is worth another round here.
Worth restating for anyone reading #2319 later: this fixes the flood caused by an invalid generated config, which was the actual root cause — the empty identity writing a bare { on line 5. It does not touch the other flood, vtund's own persist yes reconnect, which loops on a hard-coded sleep(5) in client.c and logs two daemon.info lines per attempt whenever the server is unreachable. That one is inside vtund and out of scope here.
Thanks for the quick turnaround on the review points.
2 participants
Problem
This change follows up on the problem described in #2406.
The current
/usr/sbin/tunnelscript startsvtundimmediately and restarts it in a tight loop. If the network is not ready yet, there may be no default-route interface from which to obtain the camera identity and MAC address. The resulting VTun configuration is invalid andvtundexits with errors such as:The immediate restart loop can then repeatedly produce the same errors and quickly fill the 64 KB syslog ring, making other messages from the affected boot difficult or impossible to recover.
There are also several shell functions shared by
wireguardandtunnel, but they were previously implemented only inside/usr/sbin/wireguard.During testing I also observed an intermittent VTun startup failure:
This is consistent with the race condition described in Red Hat Bugzilla #1462458, comment 26. VTun can attempt to write to the TUN/TAP device while the newly created interface is not yet administratively up, causing the write to
/dev/net/tunto returnEIO.In the OpenIPC configuration, the interface is normally brought up by the
upcommand from the VTun configuration. The failure is intermittent; after VTun is restarted, it usually does not occur again.The new
tunnelscript works around this by configuring and bringing the tunnel interface up before startingvtund.Hardware tested on
The changed scripts were tested for two days on remote cameras running:
The testing covered normal startup conditions with the network and DNS already available.
I have not yet tested the failure/recovery cases where the network route or DNS resolution is unavailable when
tunnelstarts.Evidence
Before:
After:
The negative startup/recovery cases were not directly reproduced during this test period.
Scope
general/package/all-patches/linux/(those go to [OpenIPC/linux](https://github.com/OpenIPC/linux))general/overlay/or in a sharedload_<vendor>script hardcodes a value specific to my boardLD_PRELOAD, and no binaries that cannot be rebuilt from sourceNo package source or version is changed by this PR.
Changes
Moved shell functions shared by
wireguardandtunnelinto/usr/sbin/common.Added
/usr/sbin/kill_pid_fileas a small executable wrapper so thekill_pid_filehelper can also be invoked from commands launched by the VTun configuration.Reworked
/usr/sbin/tunnelto:udhcpcinstance using the commonkill_pid_filehelper;vtund;EIOstartup race described in Red Hat Bugzilla #1462458, comment 26;udhcpcwhen the VTun session goes down.Changed
/usr/sbin/wireguardto overwrite/tmp/wireguard.confinstead of appending to it, preventing duplicate configuration when the script is run manually more than once.Changed the non-fatal path of the common command runner to return the actual command exit status, allowing callers such as
tunnelto react to command failures.