The "ctrld start" command is running slow, and using much CPU than
necessary. The problem was made because of several things:
1. ctrld process is waiting for 5 seconds before marking listeners up.
That ends up adding those seconds to the self-check process, even
though the listeners may have been already available.
2. While creating socket control client, "s.Status()" is called to
obtain ctrld service status, so we could terminate early if the
service failed to run. However, that would make a lot of syscall in a
hot loop, eating the CPU constantly while the command is running. On
Windows, that call would become slower after each calls. The same
effect could be seen using Windows services manager GUI, by pressing
start/stop/restart button fast enough, we could see a timeout raised.
3. The socket control server is started lately, after all the listeners
up. That would make the loop for creating socket control client run
longer and use much resources than necessary.
Fixes for these problems are quite obvious:
1. Removing hard code 5 seconds waiting. NotifyStartedFunc is enough to
ensure that listeners are ready for accepting requests.
2. Check "s.Status()" only once before the loop. There has been already
30 seconds timeout, so if anything went wrong, the self-check process
could be terminated, and won't hang forever.
3. Starting socket control server earlier, so newSocketControlClient can
connect to server with fewest attempts, then querying "/started"
endpoint to ensure the listeners have been ready.
With these fixes, "ctrld start" now run much faster on modern machines,
taking ~1-2 seconds (previously ~5-8 seconds) to finish. On dual cores
VM, it takes ~5-8 seconds (previously a few dozen seconds or timeout).
---
While at it, there are two refactoring for making the code easier to
read/maintain:
- PersistentPreRun is now used in root command to init console logging,
so we don't have to initialize them in sub-commands.
- NotifyStartedFunc now use channel for synchronization, instead of a
mutex, making the ugly asymetric calls to lock goes away, making the
code more idiom, and theoretically have better performance.
The systemd-networkd-wait-online is only required if systemd-networkd
is managing any interfaces. Otherwise, it will hang and block ctrld from
starting.
See: https://github.com/systemd/systemd/issues/23304
So with clients which only use SLAAC, ctrld could see client's new ip as
soon as its state changes to REACHABLE.
Moreover, the NDP listener is also changed to listen on all possible
ipv6 link local interfaces. That would allow ctrld to get all NDP events
happening in local network.
SLAAC RFC: https://datatracker.ietf.org/doc/html/rfc4862
This commit implements upgrade command which will:
- Download latest version for current running arch.
- Replacing the binary on disk.
- Self-restart ctrld service.
If the service does not start with new binary, old binary will be
restored and self-restart again.
On BSD, the service is made un-killable since v1.3.4 by using daemon
command "-r" option. However, when reading remote config, the ctrld will
fatally exit if the config is malformed. This causes daemon respawn new
ctrld process immediately, causing the "ctrld start" command hang
forever because of restart loop.
Since "ctrld start" already fetch the resolver config for validating
uid, it should validate the remote config, too. This allows better error
message printed to users, let them know that the config is invalid.
Further, if the remote config was invalid, we should disregard it and
generating the default working one in cd mode.
On BSD platform, using "daemon -r" may fool the status check that ctrld
is still running while it was terminated unexpectedly. This may cause
the check in newSocketControlClient hangs forever.
Using a sane timeout value of 30 seconds, which should be enough for the
ctrld service started in normal condition.
If the server is running old version of ctrld, the deactivation pin
check will return 404 not found, the client should consider this as no
error instead of returning invalid pin code.
This allows v1.3.5 binary `ctrld start` command while the ctrld server
is still running old version. I discover this while testing v1.3.5
binary on a router with old ctrld version running.
The loop is run after the main interface DNS was set, thus the error
would make noise to users. This commit removes the noise, by making
currentStaticDNS returns an additional error, so it's up to the caller
to decive whether to emit the error or not.
Further, the physical interface loop will now only log when the callback
function runs successfully. Emitting the callback error can be done in
the future, until we can figure out how to detect physical interfaces in
Go portably.
The save/restore DNS functionality always perform its job, even though
the DNS is not static, aka set by DHCP. That may lead to confusion to
users. Since DHCP settings was changed to static settings, even though
the namesers set are the same.
To fix this, ctrld should save/restore only there's actual static DNS
set. For DHCP, thing should work as-is like we are doing.
Windows Server which is running Active Directory will have its own DNS
server running. For typical setup, this DNS server will listen on all
interfaces, and receiving queries from others to be able to resolve
computer name in domain.
That would make ctrld default setup never works, since ctrld can listen
on port 53, but requests are never be routed to its listeners.
To integrate ctrld in this case, we need to listen on a local IP
address, then configure this IP as a Forwarder of local DNS server. With
this setup, computer name on domain can still be resolved, and other
queries can still be resolved by ctrld upstream as usual.
After installing as a system service, "ctrld start" does an end-to-end
test for ensuring DNS can be resolved correctly. However, in case the
system is mis-configured (by firewall, other softwares ...) and the test
query could not be sent to ctrld listener, the current error message is
not helpful, causing the confusion from users perspective.
To improve this, selfCheckStatus function now returns the actual status
and error during its process. The caller can now rely on the service
status and the error to produce more useful/friendly message to users.
In some old Windows systems, s.Uninstall does not remove the service
completely at the time s.Install was running, prevent ctrld from being
installed again.
Workaround this by attempting to uninstall ctrld several times, re-check
for service status after each attempt to ensure it was uninstalled.
An interface may have multiple MAC addresses, that leads to the problem
when looking up hostname for its multiple <ip, mac> pairs, because the
"ip" map, which storing "mac => ip" mapping can only store 1 entry. It
ends up returns an empty hostname for a known MAC address.
Fixing this by filling empty hostname based on clients which is already
listed, ensuring all clients with the same MAC address will have the
same hostname information.
Since go1.21, Go standard library have added support for QUIC protocol.
The binary size gains between quic and quic-free version is now minimal.
Removing the quic free build, simplify the code and build process.
Since ctrld now supports MAC rules, the client's mac and ip must always
be sent to ctrld. Otherwise, the mac policy won't work when ctrld is an
upstream of dnsmasq.
When avahi-daemon is avaibale, reading data from its cache help ctrld
populate the mdns data with already known services within local network,
allowing discover client info more quickly.
On some routers, dnsmasq config may change cache-size dynamically after
ctrld starts, causing dnsmasq crashes.
Fixing this by using max-cache-ttl, which have the same effect with
setting cache-size=0 but won't conflict with existing routers config.
Windows may raise WSAEHOSTUNREACH instead WSAENETUNREACH in case of
network not available when resuming from sleep or switching network, so
checkUpstream is never kicked in for this type of error.
To prevent duplicated running of checkUpstream function at the same
time, upstream monitor uses a boolean to report whether the upstream is
checking. If this boolean is true, then other calls after the first one
will be returned immediately.
However, checkUpstream does not set this boolean to false when it
finishes, thus all future calls to checkUpstream won't be run, causing
the upstream is marked as down forever.
Fixing this by ensuring the boolean is reset once checkUpstream done.
While at it, also guarding all upstream monitor operations with a mutex,
ensuring there's no race condition between marking upstream state.
We see number of failed test in Github Action, mostly on MacOS or
Windows due to the fact that goroutines are scheduled to be run
consequently.
This commit improves the test, ensuring at least 2 goroutines were
started before increasing the counting.
Cuong Manh Le