Refactor tunnel startup logic similar to WireGuard - #2467
Conversation
PR Summary by QodoUnify VTun startup checks with shared retry backoff
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
Code Review by Qodo
1. Tunnel recovery remains unverified
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
|
Could you please take a look and help verify the negative case, as you have done with my previous PRs? |
openipc-ai
left a comment
openipc-ai
left a comment
There was a problem hiding this comment.
I ran the negative cases on a lab ssc377d (SigmaStar infinity6c, busybox 1.36.1), driving this branch's tunnel and master's side by side with the same shims so the two are comparable.
The paths you asked about all work.
DNS never resolves — tunnel nonexistent-vtun-test.invalid:
07:36:13 daemon.warn tunnel[3320]: Unable to resolve the VTun server hostname
07:36:23 +10
07:36:43 +20
07:37:13 +30
07:37:53 +40
07:38:43 +50
07:39:43 +60
DNS temporarily unavailable, then restored — /etc/resolv.conf pointed at an unroutable 192.0.2.1, server openipc.org, resolver restored at 07:42:01:
07:40:22 daemon.warn tunnel[3417]: Unable to resolve the VTun server hostname
07:40:35 +10
07:40:58 +20
07:41:31 +30
07:42:11 daemon.info tunnel[3417]: Starting the tunnel to openipc.org
It recovers on the next rung after DNS returns, and goes on to start vtund normally. -timeout=3 -retry=1 costs about 3 s per failed lookup against one unroutable nameserver, as intended.
Setup command fails — tunctl -t tunnel forced to exit 1:
07:51:06 daemon.err tunnel[619]: Command `tunctl -t tunnel` finished with code 1.
07:51:16 +10
07:51:36 +20
07:52:06 +30
07:52:46 +40
Same ladder as master on that path.
And your reading of persist yes holds. Real vtund pointed at an unreachable server never came back to the shell — it retried internally for as long as I let it run:
07:44:33 vtund[3650]: Connecting to openipc.org
07:44:33 vtund[3650]: Connect to openipc.org failed. Connection refused(111)
07:44:38 vtund[3650]: Connecting to openipc.org
...
07:45:09 vtund[3650]: Connect to openipc.org failed. Connection refused(111)
One thing needs changing before this goes in, and one thing is worth knowing — both inline.
CI has never run on this branch: all five workflows are sitting at action_required, waiting for a maintainer to approve the fork run. I will approve them once you push, so the matrix runs once on the final head rather than twice.
The repository checks pass on this branch as it stands — test_shell_parse.sh, test_strip_shell_comments.sh and ci-matrix.py --self-test — and the branch is current with master.
|
Could you please re-review the latest version of the tunnel changes, especially the short-lived |
openipc-ai
left a comment
openipc-ai
left a comment
There was a problem hiding this comment.
Fix verified on the lab ssc377d — the flat 10 s retry is gone and the ladder is back. Details inline.
device_name is restored and the point about vtund's own chatter is understood, so nothing is outstanding from my side. Approving; the two remarks inline are take-it-or-leave-it.
One tidy-up: the description still describes the old behaviour in two places — "The result of the vtund command is no longer used to control the retry loop" and "Once vtund has been started, start() therefore completes successfully". Both are now the opposite of what the code does.
I have approved the five workflow runs on 0e5e2fdb, so the matrix is running on the head you will merge. Let it come back green first.
|
I also moved the I made the same change for WireGuard for consistency. |
openipc-ai
left a comment
openipc-ai
left a comment
There was a problem hiding this comment.
Re-approving on 43c1cf8. The switch to /proc/uptime and the raised threshold are verified on the lab ssc377d, the matrix is green at 100/100, and both Qodo threads are resolved.
2 participants
Problem
The
/usr/sbin/tunnelstartup logic is split between a separate readiness check and the main retry loop, with two independent increasing retry delays.This change refactors the startup flow to use a single
start()function, similar to the recent WireGuard startup changes — PR #2465.The previous implementation used one increasing delay while waiting for network/DNS readiness and another after failures of setup commands before
vtundwas started. The latter was intended to reduce repeated failures from flooding the syslog. During the review of PR #2438, it was noted that persistent setup failures could still fill the syslog ring and that retries should be backed off rather than repeated at a fixed short interval. The relevant review discussion is [here](#2438 (review)) (finding 3).Since the readiness and setup checks are now handled by the same
start()function, they can share a single increasing retry delay. This removes the two separate backoff mechanisms while retaining the retry pacing needed for persistent failures.The initial DNS lookup also uses
-timeout=3 -retry=1to bound the time spent on each failed resolution attempt.The common logging helpers from
/usr/sbin/commonare used instead of local copies.The startup variables were renamed for consistency and clarity, for example
vtund_server→server,vtund_port→port,identity_cfg→config_path, andudhcpc_pid→udhcpc_pid_path. The identity-related variables were renamed as well.The result of the
vtundcommand is not used directly to determine whether startup was successful. Withpersist yes,vtundis expected to remain running and handle reconnection internally rather than return to the shell when the server is temporarily unavailable, as noted during the review of PR #2438. [Review discussion](#2438 (comment))To handle fatal startup failures regardless of
vtund's exit status,start()measures how long thevtundsession remains alive using the monotonic system uptime from/proc/uptime. A session that ends before the 10-second startup threshold is treated as a failed attempt and enters the increasing retry backoff.Once
vtundhas been started successfully,start()returns success and the outer loop resumes its normal 10-second interval. The outer loop is retained for now as a precaution; whether it is needed at all will be investigated separately and is outside the scope of this PR.The
Starting the tunnel...log message is emitted immediately before thevtundinvocation, so it reflects an actual tunnel start attempt. The corresponding WireGuard startup log message was moved in the same way for consistency.Hardware tested on
ssc378de
The positive startup scenario was tested successfully with the VTun server address immediately resolvable at camera startup.
The negative paths were also verified on a lab ssc377d using shims:
tunctl -tfails — retries use the increasing backoff.vtundexits immediately with either status 1 or status 0 — retries use the increasing backoff.The exit-status-0 case is particularly relevant because checking the
vtundduration catches critical failures that are not reflected in its exit status.Evidence
Before:
After:
The new output shows the
tunnelstartup message immediately before thevtundlaunch, followed by a successful VTun connection, andifconfigconfirms that thetunnelinterface is up with its DHCP address.Scope
general/package/all-patches/linux/(those go to 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 source