Uh oh!
There was an error while loading. Please reload this page.
Stability fixes - #1574
Open
troglobit wants to merge 10 commits into
Open
Conversation
There was a problem hiding this comment.
Pull request overview
Stabilizes statd and confd under datastore contention and partial-apply failure modes by reducing event-loop blocking, chunking/isolating operational snapshots, and making interface generation more convergent, plus related build/runtime cleanup.
Changes:
statd: debounce mDNS operational datastore applies with retry/backoff; move periodic operational snapshots into a low-priority forked helper process.confd: make interface teardown/create more convergent by tolerating missing links and removing leftovers beforelink add.- Build/ops: skip enabling unavailable optional services, prune dangling Finit symlinks, and bump
mdns-aliasto v1.3.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/statd/statd.c | Adds getopt-based -h/-V/-v handling, adjusts logging defaults, removes extra sysrepo session, wires journal to main ev loop. |
| src/statd/Makefile.am | Injects STATD_VERSION from $(PACKAGE_VERSION) for -V output. |
| src/statd/journal.h | Refactors journal API/types for forked snapshot model and conditional stub builds. |
| src/statd/journal.c | Replaces threaded snapshotting with forked statd-journal child and per-module dump chunking. |
| src/statd/avahi.h | Extends mDNS context with debounce/retry apply timer state. |
| src/statd/avahi.c | Debounces and retries sr_apply_changes() to avoid blocking the event loop under contention. |
| src/confd/src/main.c | Aligns verbosity handling so info maps to LOG_INFO and debug enables debug mode. |
| src/confd/src/interfaces.c | Makes interface deletion tolerant and ensures leftover links are removed before creation. |
| src/confd/src/core.c | Avoids creating dangling Finit enable symlinks for optional services not present in the image. |
| package/statd/statd.conf | Updates service command line to match new statd option handling. |
| package/mdns-alias/mdns-alias.mk | Bumps mdns-alias version to 1.3. |
| package/mdns-alias/mdns-alias.hash | Updates tarball hash for mdns-alias 1.3. |
| doc/ChangeLog.md | Documents the stability fixes and related upgrades. |
| board/common/post-build.sh | Drops optional daemon confs when not built and prunes dangling enabled symlinks. |
Suppressed comments (1)
src/statd/journal.c:256
- w->rstatus is the raw wait(2) status word, so logging it with %d is misleading (e.g., exit code 1 often prints as 256). Log the decoded exit code / signal instead.
if (!WIFEXITED(w->rstatus) || WEXITSTATUS(w->rstatus))
ERROR("Journal snapshot failed, status %d", w->rstatus);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Fixes crash on SIGHUP while disconnected from Avahi, e.g., when the hostname change that triggered the reload also restarted avahi-daemon: mdns-alias[8943]: Failed creating new entry group: Bad state finit[1]: Service mdns-alias keeps crashing, not restarting. Entry group failures, CNAME collisions, and publish errors are now treated as transient and retried with a full reconnect. Also quieter logs by default, routine lifecycle events demoted to INFO. Release notes: https://github.com/troglobit/mdns-alias/releases/tag/v1.3 Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
Reading operational data can be slow, or time out entirely, while mDNS neighbors are being discovered: statd[3658]: mdns: sr_apply_changes: Timeout expired statd[3658]: Error, getting operational data: User callback failed statd is single threaded; the avahi handlers apply datastore changes per resolver event, from the same event loop that serves all operational get callbacks. Every apply blocks the loop, and resolver events arrive in bursts, e.g., after an mDNS daemon restart. Batch all edits with a debounce timer and apply once the burst has settled. On datastore contention, back off and retry later instead of blocking the loop. Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
A configuration change that is interrupted, e.g., by a frontend timeout, can leave interfaces behind in the kernel: the datastore transaction is rolled back but the dagger generation's side effects are not. The next generation then fails creating the interface: dagger[2599]: Aborting: /run/net/131/action/init/br0/50-init.ip failed with exitcode 1 confd[2599]: Failed to apply interface configuration with 'RTNETLINK answers: File exists' in the script log. A failed generation is abandoned, but its executed actions remain, so every subsequent interface change fails, in either the create or the delete direction, until reboot. Delete any leftover before creating a netlink interface, and tolerate an already-missing interface when deleting one. Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
Every configuration change that restarts the mDNS daemon logs a NOTICE level reconnect message. Routine noise, demote to INFO; the WARN/NOTE pair for an unresponsive daemon is kept as is. Also, minor code refactor included, dropping code useless block. Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
statd logs everything, including INFO level messages, to syslog by default, unlike other services which default to NOTICE. Port option handling from confd: -h, -V, and -v <level>, backed by setlogmask(). Drop the stale flags from statd.conf, they were never parsed and are rejected now that statd has strict option handling. Unlike confd, -v info maps to LOG_INFO rather than LOG_NOTICE, and -v debug also enables the DEBUG() macro without requiring the DEBUG environment variable. Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
Configuring the system, or querying status, can be slow or time out
entirely while the periodic (5 min) journal snapshot is running. On
slow systems with a big configuration a snapshot can take minutes.
The snapshot ran in a statd thread, reading ALL operational data in a
single sr_get_data("/*") call: every subsystem's callbacks are invoked
back-to-back while datastore locks are held for the entire read, with
statd's main loop busy serving them. Other datastore users queue up
behind it.
Fork the snapshot instead, renamed statd-journal using prctl(), running
at nice 10 with its own sysrepo connection. The dump is chunked, one
module per sr_get_data() call with a short breather in between, so
interactive users interleave with the snapshot instead of waiting for
all of it. The snapshot timer is one-shot, re-armed only when the
previous snapshot has finished: snapshots can never overlap, and the
interval is the rest between them rather than a fixed cadence.
Each module read waits at most ten seconds: reading operational data
holds the module's read lock, also while waiting for providers, so on
a busy system the journal skips that module for the round instead of
holding up configuration changes:
Failed sending edit-config RPC: Locking a rwlock failed (sr_shmmod_lock:
Connection timed out), read lock held by running process 28334 (CID 85),
running process 3627 (CID 3). Retrying ...
Skipped modules are counted in the snapshot completion log message.
Also drops the last thread from statd, and the second sysrepo session,
which was only used by the journal.
Signed-off-by: Joachim Wiberg <troglobit@gmail.com>A failing yanger invocation logged only: statd[3539]: Error, running yanger statd[3539]: Error adding interface yanger data leaving no trace of which model or interface failed, or how. Include the model, the interface for per-interface queries, and yanger's exit code. Also plug a small memory leak: the extracted interface name was never freed after the query. Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
Minimal images log the following on every boot and initctl reload, e.g., when the regression test framework reconfigures the system: finit[1]: Skipping /etc/finit.d/enabled/netbrowse.conf, dangling symlink: No such file or directory finit[1]: service_register():/etc/finit.d/enabled/ttyd.conf: skipping ttyd: No such file or directory finit[1]: Skipping /etc/finit.d/enabled/webui.conf, dangling symlink: No such file or directory The web services are enabled in the default configuration, so confd's finit_enable() creates enabled/ symlinks also on images where the service was never installed. svc_enable() already guards its nginx symlinks with the corresponding check. The ttyd case differs: its conf ships unconditionally in the common rootfs skeleton while the daemon itself is an optional package. Skip enable, with a log message at INFO, when the service conf is not available in the image. At build time, drop the ttyd confs when ttyd is not selected, and prune any dangling enabled/*.conf symlinks. Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
-v info mapped to LOG_NOTICE, making actual INFO level messages unreachable from the command line. -v debug opened the log mask but not the DEBUG() macro, which is gated on the debug variable, so debug messages still required the DEBUG environment variable to be set. Same behavior as statd. Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
| if [ "$BR2_PACKAGE_WEBUI" = "y" ]; then | ||
| mkuserguide | ||
| fi | ||
Contributor
There was a problem hiding this comment.
Isn't a better solution to move the installation of the ttyd.conf to the ttyd package instead?
| } | ||
| static int link_gen_del(struct lyd_node *dif, FILE *ip) | ||
| /* |
Contributor
There was a problem hiding this comment.
This we have talked multiple times about, i am fine with this solution, but i remember @wkz had some good examples why this was not good.
Contributor
There was a problem hiding this comment.
Not exactly sure what example I was thinking about.
My main concerns about this is (1) that we mask the sources of errors (i.e., if there is a problem in removing an interface, we won't see it until we try to configure the next instance of it - or worse: never); and (2) that we lull the user into thinking we support things (rolling back a partially applied config) that we do not (i.e., we might still be leaving addresses, routes, containers, etc. around from the previous config). Unless we have that sorted out through some other means?
| return SR_ERR_OK; | ||
| } | ||
| /* |
Contributor
There was a problem hiding this comment.
We need to discuss this next week. What should happen when something fails, should we handle it or should it never fail?
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Follow-up to the
statdtimeout revert (#1573), fixing the actual root causes behind the regression failures on main, plus related cleanups found along the way.The meltdown had four layers:
statdblocking its event loop (the reverted trigger), the journal snapshot monopolizing the datastore for minutes, mDNS browse storms feeding more contention, and interrupted commits leaving dagger generations half-applied in the kernel, permanently breaking all subsequent interface configuration until reboot.statd: mDNS neighbor updates are debounced into one apply per burst and retried with backoff on datastore contention, the event loop never blocks on a busy datastorestatd: journal snapshots run in a forked, low-priority statd-journal process with its own sysrepo connection, reading one module per call so other datastore users interleave. The timer is re-armed only after completion, so snapshots never overlap. Drops the last thread, and the second sysrepo session, fromstatdconfd: interface setup/teardown is now convergent, leftovers from a partially applied generation are removed before link add, and deleting an already-missing interface no longer aborts the generationconfd: enabling an optional service not present in the image no longer creates a dangling Finit symlink; build also drops leftover confs (ttyd) and prunes dangling symlinks, silencing minimal buildsstatd: proper option handling (-h,-V,-v <level>) backed bysetlogmask(), defaulting toNOTICElike other services; matching-v info/debugfixes inconfd;yangererrors now name the module/interface and exit codeChecklist
Tick relevant boxes, this PR is-a or has-a: