fix(motd): repair legacy 00_lxc-details.sh header and dead color placeholders - #53
Conversation
Try this branchThe engine and the scripts resolve independently, so a production script can COMMUNITY_SCRIPTS_CORE_URL=https://raw.githubusercontent.com/BadFlo/core/fix-legacy-motd-header-after-migrate \
bash -c "$(curl -fsSL https://raw.githubusercontent.com/community-scripts/ProxmoxVED/main/ct/debian.sh)"Swap Run a script from a fork as wellcurl -fsSL https://raw.githubusercontent.com/BadFlo/core/fix-legacy-motd-header-after-migrate/tools/run.sh |
bash -s -- https://raw.githubusercontent.com/YOU/ProxmoxVED/your-branch ct/debian.sh \
https://raw.githubusercontent.com/BadFlo/core/fix-legacy-motd-header-after-migrateNote that Useful flags while testing
|
|
What is broken? Can you add examples? I deploy ~25-50 LXC per Day and i dont see any broken MOTD? (Screenshots with Example please) |
|
Good question — worth being precise about when this actually shows up, because it won't appear on any freshly-deployed LXC, which is why it's easy to miss deploying 25-50/day. Why fresh deploys never see it: current Concrete repro, verbatim from one of my own containers (a Before the fix, the legacy header block is still present: and Because the legacy header logic greps this file's own text for whatever follows instead of The color half of this PR has the same "invisible until you look" property: I reproduced this on 15 already-existing LXCs across two Proxmox VE nodes in my own homelab - different apps and install ages (plex, adguard x2, jeedom, qbittorrent, zoraxy, cloudflared x2, paperless-ngx, openobserve, rustdesk-server, pbs, apt-cacher-ng, docker, uptime-kuma) - applied this fix, and confirmed both the header and the color lines render correctly afterward, checked via a real pty-backed login session ( |
|
Clean up the comments first please, the review is hard, if 25+ Lines of 50 are comments. Tell your AI to minimize or minimize by yourself. Kill "useless" comments first |
Containers built before motd_ssh started baking ${APPLICATION} into
/etc/profile.d/00_lxc-details.sh statically instead re-derived the app
name on every login by grepping /usr/bin/update's own text for
whatever followed "/ct/", assuming a literal URL.
migrate_update_entrypoint() (already run earlier in the same update,
see menu.func) rewrites that URL to be built from $UPDATE_SCRIPT_NAME
instead of a literal script name. Once that happens, the literal text
following "/ct/" in the file is the placeholder itself, so the old
regex faithfully extracts it and the header prints literally as
"${UPDATE_SCRIPT_NAME} LXC Container" forever after, since this file
is only ever patched (OS/Hostname/IP), never regenerated.
update_motd_ip() now detects that legacy block (signature: it still
contains APPL_NAME) and collapses it into a static line using the name
migrate_update_entrypoint already resolved into /usr/bin/update's own
export line. The replacement reuses the same raw \033[1m/\033[m bytes
the original line already had rather than ${YW}/${CL}: those vars are
unset even in a real login shell on affected containers (see the
follow-up commit), so a variable-based replacement would have quietly
rendered as plain text.
Reproduced and verified end-to-end on 17 live Proxmox VE containers
across two cluster nodes: syntax-checked, diffed against a real
pty-rendered login session before and after. Incus's own
update_motd_ip() (incus/tools.func) never wrote this file at all, so
it isn't affected and isn't touched here.
update_motd_ip() refreshes the OS/Hostname/IP lines in
/etc/profile.d/00_lxc-details.sh by writing values wrapped in
${GN}/${CL}, expecting those to resolve to color codes at login.
They never do: both are only ever defined transiently inside the
installer/updater's own process (see the color() helpers in
core/core.func, pve/vm-core.func, incus/core.func), never exported
into the container's persistent environment for a later interactive
shell to read. Confirmed empty even in a real pty-backed login shell
on an affected container. The result is a line that always renders
in plain text.
Worse, the "is this already up to date?" checks above only compare
the *value* text, not whether the color wrapper actually resolves. So
once a line shows the correct value, it looks current forever and the
dead placeholder never gets a chance to self-heal, even across
further updates.
Force a rewrite to raw \033[1;92m/\033[m bytes -- the same convention
already used by the OS/Hostname/IP label styling two lines up in this
same file, and by every field in containers whose motd_ssh predates
this ${GN}/${CL} convention entirely -- whenever a line still carries
the dead placeholder, independent of whether the value itself needs
updating.
Verified on a live container carrying the legacy placeholder on all
three fields: OS/Hostname/IP now render in bright green matching
sibling containers that never had this bug.
57ca904 to
ad3d1a2
Compare
|
Trimmed - comments cut from ~40% of added lines to ~22%, kept only the one-liners needed to explain the non-obvious parts (why raw bytes instead of $YW/$CL, why the staleness check needs a force-rewrite). Full rationale stays in the commit messages/PR description instead of the diff. Force-pushed. |
Two issues from review: - The header-collapse sed used an unguarded range end (/APPL_NAME/,/LXC Container/). Entry only checked APPL_NAME was present, not that LXC Container also was. On a variant header that never says "LXC Container", the range never closes: it runs to EOF, d deletes everything from APPL_NAME onward, and r never fires since its own address never matches either - silently truncating the file past that point, OS/Hostname/IP lines included. Now require both markers before attempting the range at all. - Renamed broken_os/broken_hostname/broken_ip to dead_color_os/dead_color_hostname/dead_color_ip. Nothing on these lines is broken - they render fine, just without color - and this codebase already reserves "broken" for actual failures. Verified the guard against both cases: a header missing "LXC Container" now leaves the file byte-for-byte untouched (previously truncated it), and a normal legacy header still matches and collapses as before.
|
Both addressed in 17a16a1:
Left the ^ capitalization as-is per your note that it's probably fine - happy to add word-boundary handling too if you'd rather. |


Summary
Two related bugs in
update_motd_ip()(ui/validate.func), both in containers built beforemotd_sshstarted baking${APPLICATION}in statically:/usr/bin/update's own text for whatever followed/ct/, assuming a literal URL. Oncemigrate_update_entrypoint()rewrites that URL to be built from$UPDATE_SCRIPT_NAMEinstead of a literal name, the literal text following/ct/in the file becomes the placeholder itself, so the header renders as${UPDATE_SCRIPT_NAME} LXC Containerverbatim, forever (this file is only ever patched, never regenerated).${GN}/${CL}, but those are only ever defined transiently inside the installer/updater's own process (thecolor()helpers in core.func/vm-core.func/incus/core.func) - never exported into the container's persistent environment. Confirmed empty even in a real pty-backed login shell. Worse, the "already up to date?" check only compares the value text, so once a line shows the right value it looks current forever and the dead placeholder never gets a chance to self-heal.Fix
APPL_NAME) and collapse it into a static line using the namemigrate_update_entrypointalready resolved, reusing the same raw\033[1m/\033[mbytes the original line had (not a variable, since those don't reliably resolve).${GN}placeholder on the OS/Hostname/IP lines and force a rewrite to raw\033[1;92m/\033[mbytes, independent of whether the value itself needs updating.Two commits, one per bug, each with the full root-cause writeup in the commit message.
Test plan
bash -non the patched fileshellcheck: zero new warnings vs. the pre-patch baseline (diffed both, only line-number shifts)shfmt -d: no formatting diff introduced by this changescript -qec 'bash -li -c ...'), before/after byte-for-byte diffsupdate_motd_ip()(incus/tools.func) never wrote this file at all, so it isn't affected and isn't touched here