Skip to content

fix(hot-reload): write the config in place and mount its directory - #134

Open
rvalitov wants to merge 3 commits into
SamNet-dev:mainfrom
rvalitov:fix/hot-reload-bind-mount-inode
Open

fix(hot-reload): write the config in place and mount its directory#134
rvalitov wants to merge 3 commits into
SamNet-dev:mainfrom
rvalitov:fix/hot-reload-bind-mount-inode

Conversation

@rvalitov

Copy link
Copy Markdown

Fixes #133

Summary

Every command that changes users or limits reported success while the running
engine kept the old config — secret add printed "Config reloaded (hot-reload,
no restart)" and the engine never saw the change, so only mtproxymax restart
applied it. It failed in both directions: new users could not connect, and
removed or rotated secrets kept working, which is exactly the operation you
reach for after a leak.

Root cause: the container bind-mounted config.toml as a single file, which
pins one inode, and generate_telemt_config rewrote that file with cp. On the
affected install cp replaced the inode instead of writing through it, so the
mount was left pointing at a deleted file and the engine could not be reached by
any amount of signalling or polling.

Root cause

Measured on the affected install (ext2/ext3, so not an overlay copy-up):

Write operation Inode
cp onto the bind-mounted config.toml replaced → container detached
cp onto an ordinary file preserved
append (>>) preserved
> redirect preserved
dd conv=notrunc preserved

mount then reports the mount source as .../config.toml//deleted — the kernel
saying the mounted inode no longer has a name at that path. Once that happens no
SIGHUP, inotify event or content poll can deliver the new bytes, because they are
not visible inside the container at all. Only recreating the container re-mounts
the current file, which
On the mechanism: busybox cp (libbb/copy_file.c) opens the destination with
O_WRONLY|O_CREAT|O_TRUNC in its normal path, and falls back to unlinking and
recreating when that open fails. There is no mount-point special case in that
file, so the mount point must be causing the initial open to fail rather than
being special-cased — but the outcome either way is that the destination is
replaced rather than written through. That the inode changes only on the first
generation (1125 → 1125 afterwards) is consistent with this: once replaced,
the path is no longer a mount point and later cp calls behave normally.

Conclusion for this PR: do not depend on cp writing in place, and do not
bind-mount a live config as a single file.
Both are now removed.

The engine side is not at fault. telemt hot-reloads [access.users] and the
per-user limit fields (src/config/hot_reload/fields.rs) and has three reload
triggers — inotify, a 3s compare_contents(true) poll watcher, and a SIGHUP
handler (src/config/hot_reload/watcher.rs); ENTRYPOINT ["telemt"] makes
telemt PID 1, so docker kill -s SIGHUP lands. Confirmed once the mount was
repaired: a live secret add with no restart was picked up immediately.

Changes

  1. Write the config in placecat "$tmp" > "$dest" instead of
    cp "$tmp" "$dest" (generate_telemt_config, 1222/1486). A shell redirect
    can only truncate, so it cannot replace the inode. Guards against clobbering
    a good config with an empty generation, and sets 644 explicitly since >
    creates with the umask when the file is absent.
  2. Mount the config directory, not the file-v "${CONFIG_DIR}:/etc/telemt:ro"
    with the engine pointed at /etc/telemt/config.toml, at all nine docker run
    sites (primary 9253/9269/9289/9295, instances 9399/9409 and 13560/13569/13573).
  3. Stop routing instance configs through config.toml
    generate_telemt_config takes an optional destination, and the three
    instance paths write straight to config-<port>.toml. The previous
    mv "${CONFIG_DIR}/config.toml" "$inst_config" was a rename(2): it gave
    the instance config a new inode and unlinked config.toml, detaching a
    running primary on instance-enabled installs.
  4. Verify and report honestly_engine_config_in_sync (9452) compares the
    file against the container's own view via /proc/<pid>/root, falling back to
    docker exec ... cat, and reload_proxy_config (9474) restarts to apply the
    change when the engine cannot see it. The "Config reloaded" line is only
    printed when the reload signal was actually delivered; failures now warn
    instead of being swallowed by 2>/dev/null || true. A stopped container is
    never mistaken for an out-of-sync one (_instance_container_running, 9468).

Why both the write and the mount

The in-place write alone fixes cp. The directory mount is what covers
everything else that replaces that file — editors, sed -i, restore from
backup, config-management tools that rename. Measured on the affected install,
with a sed -i that forces a new inode:

old single-file mount directory mount
host inode changed changed
container inode pinned → detached follows it
mountinfo config.toml//deleted clean
engine applies the change no yes

With the file as the mount point, any of those tools detached the engine exactly
as cp did. With the directory mounted, replacement is harmless.

Validation

Tested on the affected install (Alpine/OpenRC LXC, telemt 3.5.6, default ports):

Test Result
Container starts on the directory mount; engine reads /etc/telemt/config.toml pass
secret add applies live; inode identical on both sides; StartedAt unchanged pass
3 consecutive adds — no restart, zero stale-mount warnings pass
secret remove clears the user from the config-driven metric pass
sed -i replaces the inode; container follows it; mountinfo clean pass
Upgrade from the old single-file mount: warns, restarts exactly once, then hot-reloads pass

Not covered: an end-to-end connection from a real Telegram client with a
newly-added secret, and a rejected connection after removal. The engine's config
and its config-driven metric both update correctly; the client half is still
untested.

Note for anyone re-testing: use telemt_user_unique_ips_current to check
whether the engine knows a user. The telemt_user_octets_* and
telemt_user_connections_* families are rendered from a runtime traffic
registry, not from the config, so a configured user with no traffic emits no
series and a removed user keeps its own — absence there proves nothing.

Tests

tests/test_hot_reload_inode.sh — 19 assertions covering the mount spec, the
primary config inode and content across a reload with an instance enabled,
instance config isolation, honest reporting, the restart fallback and stopped
containers. 10 of the 19 fail against the previous implementation.

The existing suites pass; tests/test_client_mss.sh has one pre-existing
failure unrelated to this change (reproduces on main).

… the engine

The engine container was given a single-file bind mount
(-v "$CONFIG_DIR/config.toml:/etc/telemt.toml:ro"). Such a mount pins one
inode, so once that path is replaced the container keeps reading the unlinked
one -- `mount` reports the source as ".../config.toml//deleted". No SIGHUP,
inotify event or content poll can then deliver the new bytes, so secret
add/remove/rotate/toggle and limit changes silently did nothing until the
container was recreated. That also let a removed or rotated secret keep
working, which is what made the bug dangerous.

Mount the directory instead and point the engine at the file inside it, at
all nine docker run sites (primary and secondary instances). A directory
mount tracks directory entries rather than pinning one inode, so replacing
the file is picked up immediately.

Instance configs are now written straight to their own file through a new
optional destination argument to generate_telemt_config, instead of being
generated into config.toml and then moved into place. The old mv both
unlinked config.toml (permanently detaching a running primary) and briefly
left it carrying an instance's port and metrics port, which the engine can
now actually observe.

reload_proxy_config additionally verifies that the running engine can see the
bytes it wrote and falls back to a restart when it cannot, so this class of
failure self-heals rather than passing unnoticed. A stopped container is
never mistaken for an out-of-sync one, and the change is only announced as a
hot reload when the reload signal was actually delivered.
…invariants

Covers the reload path end to end: the container mounts the config directory
rather than a single file, a reload with an instance enabled leaves the
primary config.toml inode and content untouched, instance configs are written
to their own file, a failed reload signal never claims a hot reload, a
detached config triggers a restart, and stopped containers are never treated
as out-of-sync.

Ten of the sixteen assertions fail against the previous implementation.
A field A/B on the affected box settles the trigger. With config.toml as the
source of the container's bind mount, so that the file is itself a mount
point:

  cp onto the bind-mounted config.toml   -> inode replaced  -> detached
  cp onto an ordinary file               -> inode preserved
  append, `>` redirect, dd conv=notrunc  -> inode preserved

So `cp` takes an unlink-and-recreate path when the destination is a mount
point instead of truncating in place. That inode swap is what produced
".../config.toml//deleted" in mountinfo and left the engine reading the
unlinked original, making every reload a silent no-op until the container was
recreated -- and making secret removal and rotation appear to do nothing.

Write through a shell redirect, which can only truncate. Guard against a
failed generation clobbering a good config with an empty file, and set the
mode explicitly because `>` creates with the umask when the destination does
not exist yet.

This complements the directory mount in the previous commit rather than
replacing it: the in-place write removes the trigger we proved, while the
directory mount keeps the engine working when anything else replaces the file
(editors, sed -i, restore from backup, config management) -- the same A/B
implies those detach it just as cp did.

Why busybox cp unlinks a mount point is still unexplained and is left to the
maintainer; a strace settles it. The fix does not depend on the answer.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Hot-reload never reaches the engine: cp onto the single-file config.toml bind mount replaces its inode

1 participant