Skip to content

Commit 3e897e9

Browse files
committed
feat: group Docker/Podman container logs by category instead of one file per container
Real usability gap found via live testing on an actual ovos-docker install: bridging each of ~26 containers to its own file/log-source meant 26 Sources: checkboxes and 26 different tag colors (all falling back to the same default color, since none matched the known skills/audio/voice/etc names) - unwieldy compared to a normal install's small, familiar set. Fixed with categorize_container_name(): maps real ovos-docker container names (confirmed against a live install) to the SAME category names file-based installs already use - every ovos_skill_* container (and ovos_core, whose own log content is exactly what already lands in skills.log on a normal install) maps to "skills", ovos_audio to "audio", ovos_listener to "voice", ovos_messagebus to "bus", ovos_phal/ovos_phal_admin to "phal", anything with "gui" in the name to "gui", and anything unrecognized (ovos_cli, ovos_plugin_ggwave, confirmed via the same live install) to a new "other" category. start_container_log_bridges() now groups containers by this category and has every container in a group append to the SAME shared file (skills.log, audio.log, etc) rather than its own separate one - multiple containers' `docker logs -f` subprocesses writing to one file concurrently is safe without locking (POSIX guarantees atomic writes under PIPE_BUF with O_APPEND, true for any normal single log line). This means discover_log_sources() no longer needs a custom `names=` override for the bridge case at all - the files it creates already match the default KNOWN_LOG_NAMES (added "other" to that list), so coloring, tag formatting, and the Sources: checkboxes all work completely unchanged, identical in shape to a normal install. Verified end-to-end against real, locally-run podman containers (two separate "skill" containers deliberately given different names - confirmed both landed in the same skills.log, correctly interleaved, tagged [skills] and colored green together, while a third "audio" container stayed separate and yellow). 222 tests passing (9 new - categorize_container_name coverage per category, plus a test confirming the grouping/shared-file behavior specifically); verified sdist->wheel build.
1 parent 436e67e commit 3e897e9

6 files changed

Lines changed: 158 additions & 33 deletions

File tree

README.md

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -94,13 +94,17 @@ documentation directly, not guessed at:
9494
install that follows that guide as written, there are no log files
9595
on the host filesystem at all, only container stdout. When this tool
9696
finds no log files but detects a Docker/Podman install, it
97-
automatically bridges each container's `docker logs -f` (or
98-
`podman logs -f`) into a temp file and tails that instead - the
99-
Sources: checkboxes then show the actual container names
100-
(`ovos_core`, `ovos_audio`, etc) rather than the usual `skills`/
101-
`audio`/etc, since there's no fully-confirmed container-name ->
102-
service-name mapping to translate through. Bridge processes are
103-
cleaned up on quit. If bridging isn't possible for some reason (no
97+
automatically bridges each container's `docker logs -f` (or `podman
98+
logs -f`) into the same small set of log files a normal install
99+
already produces - `skills.log`, `audio.log`, `voice.log`, etc,
100+
grouped by container name pattern (every `ovos_skill_*` container
101+
lands in `skills.log` together, `ovos_audio` in `audio.log`, and so
102+
on; anything unrecognized goes to `other.log`) - not one file/
103+
checkbox per container. Confirmed against a real ovos-docker install
104+
with 26 running containers: this keeps the Sources: checkboxes down
105+
to a handful of familiar categories with the usual colors, instead
106+
of two dozen individually-named ones. Bridge processes are cleaned
107+
up on quit. If bridging isn't possible for some reason (no
104108
`docker`/`podman` binary available), it says so explicitly instead
105109
and points at `docker logs <container>` / `docker compose logs -f`
106110
directly.

ovos_tui_client/app.py

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -578,7 +578,16 @@ def on_mount(self) -> None:
578578
self.log_bridge_handles = start_container_log_bridges(containers, bridge_dir)
579579
if self.log_bridge_handles:
580580
self.log_dir = bridge_dir
581-
self.log_sources = discover_log_sources(bridge_dir, names=containers)
581+
# No `names=` override here - start_container_log_
582+
# bridges() groups containers into the SAME
583+
# skills.log/audio.log/etc filenames a normal
584+
# install already uses (see
585+
# categorize_container_name()), not one file per
586+
# container, so the default KNOWN_LOG_NAMES
587+
# already finds them - same coloring, same
588+
# Sources: checkboxes, no special-casing needed
589+
# here at all.
590+
self.log_sources = discover_log_sources(bridge_dir)
582591
bridged = True
583592
self._write_status(
584593
f"No log files, but bridged {len(containers)} Docker/Podman "

ovos_tui_client/logs.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@
3232
# since which one exists depends on install age.
3333
KNOWN_LOG_NAMES = [
3434
"bus", "skills", "audio", "media", "voice", "gui", "enclosure", "phal",
35+
"other", # Docker/Podman bridge catch-all - see services.py's categorize_container_name()
3536
]
3637

3738

ovos_tui_client/services.py

Lines changed: 65 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -157,14 +157,70 @@ def _find_container_binary():
157157
return None
158158

159159

160+
def categorize_container_name(name):
161+
"""Maps a Docker/Podman container name to the SAME category names
162+
file-based installs already use (see logs.py's KNOWN_LOG_NAMES) -
163+
"skills", "audio", "voice", "bus", "phal", "gui" - or "other" if it
164+
doesn't recognizably fit one of those.
165+
166+
This exists so start_container_log_bridges() (below) can make
167+
Docker/Podman container logs land in the exact same small,
168+
familiar set of log files a normal systemd/venv install already
169+
produces - "skills.log", "audio.log", etc - rather than one file
170+
per container (which on a real ovos-docker install is easily 15-25
171+
separate files/checkboxes for what's conceptually still just a
172+
handful of categories). Confirmed directly against a real,
173+
running ovos-docker install's actual container names (ovos_core,
174+
ovos_audio, ovos_listener, ovos_messagebus, ovos_phal,
175+
ovos_phal_admin, ovos_skill_*, plus a few - ovos_cli,
176+
ovos_plugin_ggwave - that don't map to anything and land in
177+
"other").
178+
179+
Pattern-matched, not an exhaustive lookup table - "ovos_skill_"
180+
(any suffix) always maps to "skills", matching how every
181+
individual skill already shares ONE skills.log file on a normal
182+
install, not one file per skill. "ovos_core" also maps to
183+
"skills" specifically because its own log content (intent-service/
184+
pipeline handling) is exactly what already lands in skills.log on
185+
a normal install - confirmed by comparing real log line prefixes
186+
between a systemd install and this container's own output."""
187+
n = name.lower()
188+
if n.startswith("ovos_skill") or n == "ovos_core":
189+
return "skills"
190+
if n == "ovos_audio":
191+
return "audio"
192+
if n == "ovos_listener":
193+
return "voice"
194+
if n == "ovos_messagebus":
195+
return "bus"
196+
if n.startswith("ovos_phal"):
197+
return "phal"
198+
if "gui" in n:
199+
return "gui"
200+
return "other"
201+
202+
160203
def start_container_log_bridges(container_names, target_dir):
161-
"""For each container name, starts 'docker logs -f <name>' (or
162-
podman, whichever detect_container_runtime() would have used) with
163-
its combined stdout+stderr redirected into target_dir/<name>.log -
164-
then that directory can be handed to discover_log_sources() and
165-
treated exactly like any other log directory, reusing 100% of the
166-
existing file-tailing/filtering machinery. No new "log source"
167-
abstraction needed inside the app itself.
204+
"""Bridges Docker/Podman container stdout into the SAME small set
205+
of log files a normal file-based install already produces
206+
(skills.log, audio.log, voice.log, bus.log, phal.log, plus
207+
other.log for anything uncategorized - see
208+
categorize_container_name() above) - not one file per container.
209+
Then that directory can be handed to discover_log_sources() (its
210+
DEFAULT KNOWN_LOG_NAMES, no override needed) and treated exactly
211+
like any other log directory, reusing 100% of the existing
212+
file-tailing/coloring/filtering machinery, including the Sources:
213+
checkboxes actually being a small, meaningful set instead of one
214+
checkbox per container.
215+
216+
Multiple containers sharing a category (most commonly "skills" -
217+
every ovos_skill_* container) each get their OWN `docker logs -f`
218+
subprocess, but all of them append to the SAME shared file -
219+
concurrent appends from separate processes are safe here without
220+
explicit locking, since POSIX guarantees a single write() to a
221+
file opened with O_APPEND is atomic as long as it's smaller than
222+
PIPE_BUF (4096 bytes on Linux) - true for any normal single log
223+
line.
168224
169225
This exists because a confirmed real gap (see the discussion that
170226
led here): on a Docker/Podman install following ovos-docker's own
@@ -174,15 +230,6 @@ def start_container_log_bridges(container_names, target_dir):
174230
log file, rather than teaching the app a second, parallel way to
175231
receive log lines.
176232
177-
Log source NAMES intentionally come from the container names
178-
themselves (e.g. "ovos_core", "ovos_audio"), not a hardcoded
179-
container->service mapping - the exact mapping isn't fully
180-
confirmed for every core service (see issue #24's own notes on
181-
this), and guessing wrong would silently mislabel things. Using
182-
the container's own name sidesteps that entirely: whatever it's
183-
actually called is what shows up, no mapping table to get wrong or
184-
keep in sync as ovos-docker's own naming evolves.
185-
186233
Returns a list of subprocess.Popen handles - the caller owns their
187234
lifecycle and MUST terminate them (e.g. on app quit); they are not
188235
cleaned up automatically here. Returns [] immediately (no
@@ -193,7 +240,8 @@ def start_container_log_bridges(container_names, target_dir):
193240
target_dir.mkdir(parents=True, exist_ok=True)
194241
handles = []
195242
for name in container_names:
196-
log_path = target_dir / f"{name}.log"
243+
category = categorize_container_name(name)
244+
log_path = target_dir / f"{category}.log"
197245
log_file = open(log_path, "a")
198246
try:
199247
proc = subprocess.Popen(

tests/test_app.py

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -118,10 +118,11 @@ async def test_no_log_sources_on_a_docker_install_explains_stdout_logging(tmp_pa
118118
@pytest.mark.asyncio
119119
async def test_no_log_sources_bridges_docker_containers_when_possible(tmp_path):
120120
"""The success path: start_container_log_bridges() actually
121-
returns process handles (mocked here - real subprocess behavior is
122-
covered separately in test_services.py) - the app should pick up
123-
the bridged sources and NOT show the "no logs found" fallback
124-
message at all."""
121+
returns process handles (mocked here - real subprocess behavior,
122+
including the container-name -> category grouping, is covered
123+
separately in test_services.py) - the app should pick up the
124+
bridged sources and NOT show the "no logs found" fallback message
125+
at all."""
125126
empty_dir = tmp_path / "empty"
126127
empty_dir.mkdir()
127128
app = OVOSTUIApp(log_dir_override=str(empty_dir))
@@ -135,16 +136,20 @@ def fake_bridge(container_names, target_dir):
135136
# test can predict ahead of time - so writing it here, once
136137
# on_mount() passes in the real path, is the reliable way to
137138
# populate it before the (unmocked) discover_log_sources() call
138-
# right after this one runs)
139+
# right after this one runs). "ovos_core" categorizes to
140+
# "skills" (see categorize_container_name()), so the file is
141+
# named skills.log, not ovos_core.log - matching how the real
142+
# bridge now groups containers into the same small set of
143+
# filenames a normal install already uses.
139144
target_dir.mkdir(parents=True, exist_ok=True)
140-
(target_dir / "ovos_core.log").write_text("")
145+
(target_dir / "skills.log").write_text("")
141146
return [fake_proc]
142147

143148
with patch("ovos_tui_client.app.detect_container_runtime", return_value=["ovos_core"]), \
144149
patch("ovos_tui_client.app.start_container_log_bridges", side_effect=fake_bridge):
145150
async with app.run_test() as pilot:
146151
assert len(app.log_sources) == 1
147-
assert app.log_sources[0].name == "ovos_core"
152+
assert app.log_sources[0].name == "skills"
148153
assert app.log_bridge_handles == [fake_proc]
149154
conv = app.query_one("#conversation", RichLog)
150155
conv_text = "\n".join(str(line) for line in conv.lines)

tests/test_services.py

Lines changed: 59 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
real service management is exercised here."""
33
from unittest.mock import MagicMock, patch
44

5-
from ovos_tui_client.services import discover_services, discover_services_with_state, restart_service, stop_service, start_service, detect_container_runtime, start_container_log_bridges, stop_container_log_bridges
5+
from ovos_tui_client.services import discover_services, discover_services_with_state, restart_service, stop_service, start_service, detect_container_runtime, start_container_log_bridges, stop_container_log_bridges, categorize_container_name
66

77

88
def _fake_completed(stdout="", stderr="", returncode=0):
@@ -182,6 +182,46 @@ def fake_run(cmd, **kwargs):
182182
# podman container during development - these tests cover the
183183
# subprocess-management logic itself, mocked)
184184

185+
# --- categorize_container_name(): maps real ovos-docker container
186+
# names (confirmed against a real, running install) to the same
187+
# category names file-based installs already use ---
188+
189+
def test_categorize_container_name_skills():
190+
assert categorize_container_name("ovos_skill_wikihow") == "skills"
191+
assert categorize_container_name("ovos_skill_date_time") == "skills"
192+
assert categorize_container_name("ovos_core") == "skills"
193+
194+
195+
def test_categorize_container_name_audio():
196+
assert categorize_container_name("ovos_audio") == "audio"
197+
198+
199+
def test_categorize_container_name_voice():
200+
assert categorize_container_name("ovos_listener") == "voice"
201+
202+
203+
def test_categorize_container_name_bus():
204+
assert categorize_container_name("ovos_messagebus") == "bus"
205+
206+
207+
def test_categorize_container_name_phal():
208+
assert categorize_container_name("ovos_phal") == "phal"
209+
assert categorize_container_name("ovos_phal_admin") == "phal"
210+
211+
212+
def test_categorize_container_name_gui():
213+
assert categorize_container_name("ovos_gui_websocket") == "gui"
214+
215+
216+
def test_categorize_container_name_falls_back_to_other():
217+
"""Real container names seen on a live install that don't map to
218+
a known category - ovos_cli (a debug/interactive tool, not a
219+
logging service) and ovos_plugin_ggwave (an audio-data-over-sound
220+
plugin, not one of the core categories)."""
221+
assert categorize_container_name("ovos_cli") == "other"
222+
assert categorize_container_name("ovos_plugin_ggwave") == "other"
223+
224+
185225
def test_start_container_log_bridges_spawns_one_process_per_container(tmp_path):
186226
with patch("subprocess.run", return_value=_fake_completed(returncode=0)):
187227
with patch("subprocess.Popen") as mock_popen:
@@ -190,6 +230,24 @@ def test_start_container_log_bridges_spawns_one_process_per_container(tmp_path):
190230
assert len(handles) == 2
191231

192232

233+
def test_start_container_log_bridges_groups_same_category_containers_into_one_file(tmp_path):
234+
"""The actual point of this design: on a real ovos-docker install
235+
there can be 15-25 individual skill containers - all of them must
236+
append to the SAME skills.log, not create 15-25 separate files/
237+
checkboxes for what's conceptually one category."""
238+
with patch("subprocess.run", return_value=_fake_completed(returncode=0)):
239+
with patch("subprocess.Popen") as mock_popen:
240+
start_container_log_bridges(
241+
["ovos_skill_wikihow", "ovos_skill_weather", "ovos_skill_wolfie"], tmp_path
242+
)
243+
# 3 separate `docker logs -f` processes (one per container - each
244+
# needs its own subprocess, docker can't merge streams itself)...
245+
assert mock_popen.call_count == 3
246+
# ...but all writing to the same single file
247+
stdout_targets = {call.kwargs["stdout"].name for call in mock_popen.call_args_list}
248+
assert stdout_targets == {str(tmp_path / "skills.log")}
249+
250+
193251
def test_start_container_log_bridges_uses_docker_logs_dash_f_with_container_name(tmp_path):
194252
with patch("subprocess.run", return_value=_fake_completed(returncode=0)):
195253
with patch("subprocess.Popen") as mock_popen:

0 commit comments

Comments
 (0)