diff --git a/README.md b/README.md index 42bde21..bb97190 100644 --- a/README.md +++ b/README.md @@ -46,6 +46,7 @@ uv run cv4pve-tui | `Enter` then `n` | in info: edit network (bridge, VLAN, firewall, IPv4/IPv6; VM IP via cloud-init at next boot); `u` there = undo the last network change of this guest (this session only) | | `Enter` then `o` | in info: edit autostart, startup order (order, up and down delay) and tags (one confirmed `set config` call; empty fields remove the setting) | | `Enter` then `p` / `R` / `D` | in info: take a snapshot (name, description, VM: include RAM) / roll back to the snapshot under the cursor (confirms; starts the guest again if it is running) / delete the snapshot under the cursor (confirms) | +| `Enter` then `f` | in info: fstrim in a running VM with the guest agent enabled (confirms; containers: use `pct fstrim` on the node) | | `t` | shell in a running LXC container (`ssh -t root@ pct enter `) | | `l` | tasks: recent cluster tasks (start, duration, node, type, VMID, user, status; `!` marks failures); `Enter` shows the log of the task under the cursor, `g` reloads. In info: `l` = only this guest's tasks | | `U` | package updates: pending updates per online node (node, package, current, new, origin, priority), read-only; `g` reloads. The list is as of the node's last `apt update`. The frame's bottom border shows the count | diff --git a/docs/plans/guest-tui-mvp.md b/docs/plans/guest-tui-mvp.md index a06bd5a..f97efc3 100644 --- a/docs/plans/guest-tui-mvp.md +++ b/docs/plans/guest-tui-mvp.md @@ -697,3 +697,41 @@ CLI facts (read-only probes, PVE 9.2): or None, `tags` string or None); invalid -> error notify, dialog stays, no call; no change -> info notify; else `ConfirmScreen` titled "Boot and tags" listing old -> new per key; yes -> busy guard, `cli.set_net(guest, changes)` via start_job, info reloads. + +--- + +# fstrim in a VM via the guest agent (issue #19) + +## Decision + +Key `f` on the info screen, VMs only. Not run live (the issue asks for approval first); +tested with mocks. + +CLI facts: + +- `do vm agent ping -o json` -> `[{"key": "result", "value": "{}"}]` when the + agent answers (verified read-only on VM 108); non-zero exit when it does not. +- `do vm agent fstrim -o json` -> the same key/value shape; QEMU guest + agent's result is `{"paths": [{"path": "/", "trimmed": , "minimum": 0}, ...]}` + (a path may carry `"error"` instead of `trimmed`). Not verified live. +- VM config `agent` = `enabled=1` (or `1`) when the agent is enabled. + +### `cli.py` + +- `agent_enabled(config) -> bool` - `agent` is `1` or starts with `1` / has `enabled=1`. +- `agent_ping(guest)` -> `do vm agent ping -o json`; CliError if the agent + does not answer. +- `fstrim(guest) -> list[dict]` -> `do vm agent fstrim -o json`; returns + the `paths` list parsed from the `result` value (`[]` if missing or unparseable). + +### `app.py` (UI contract) + +- `f` on the info screen: + - container -> info notify that containers have no guest agent (`pct fstrim` on the + node instead); nothing runs; + - VM not running, or `agent_enabled` false -> warning notify saying why; nothing runs; + - otherwise `ConfirmScreen` titled "fstrim" naming the VM and saying it frees unused + blocks on thin-provisioned storage and adds I/O load while it runs; + - yes -> busy guard, worker: `cli.agent_ping` then `cli.fstrim`; done notify with the + trimmed total in GiB/MiB and the number of paths (errors per path mentioned); + CliError -> error notify (ping failure says the agent did not answer). diff --git a/src/cv4pve_tui/app.py b/src/cv4pve_tui/app.py index ca0cbf3..984d314 100644 --- a/src/cv4pve_tui/app.py +++ b/src/cv4pve_tui/app.py @@ -718,6 +718,7 @@ class InfoScreen(ModalScreen): ("R", "rollback", "Roll back"), ("D", "delete_snapshot", "Delete snapshot"), ("l", "tasks", "Tasks"), + ("f", "fstrim", "fstrim"), ] DEFAULT_CSS = """ InfoScreen DataTable { height: auto; max-height: 12; } @@ -1064,6 +1065,64 @@ class InfoScreen(ModalScreen): ), ) + def action_fstrim(self) -> None: + who = f"{self.guest['name']} ({self.guest['vmid']})" + if not self.config: # running/agent are only known once the screen has loaded + self.app.notify("Info is still loading; try again in a moment") + return + if cli.kind(self.guest) == "ct": + self.app.notify( + "Containers have no guest agent; run pct fstrim on the node instead" + ) + return + if not self.running: + self.app.notify( + f"{who} is not running; fstrim needs a running VM", severity="warning" + ) + return + if not cli.agent_enabled(self.config): + self.app.notify( + f"The guest agent is not enabled for {who}", severity="warning" + ) + return + + def job() -> None: + try: + cli.agent_ping(self.guest) + except cli.CliError as exc: + raise cli.CliError( + f"the guest agent did not answer: {_message(exc)}" + ) from exc + paths = cli.fstrim(self.guest) + if not paths: + self.app.call_from_thread( + self.app.notify, + f"fstrim finished in {who}; the agent reported no results", + ) + return + total = sum(p.get("trimmed") or 0 for p in paths) + size = ( + f"{total / 2**30:.1f} GiB" + if total >= 2**30 + else f"{total / 2**20:.1f} MiB" + ) + errs = "".join( + f"; {p.get('path', '?')}: {p['error']}" for p in paths if p.get("error") + ) + self.app.call_from_thread( + self.app.notify, + f"Trimmed {size} in {len(paths)} path{'s' if len(paths) != 1 else ''}{errs}", + ) + + self.app.push_screen( + ConfirmScreen( + f"Run fstrim in {who}? It frees unused blocks on thin-provisioned " + "storage and adds I/O load while it runs.", + "fstrim", + ), + lambda ok: self.app.start_job(self.guest, "fstrim", job) if ok else None, + ) + def _when(epoch: int | None) -> str: return datetime.fromtimestamp(epoch).strftime("%Y-%m-%d %H:%M") if epoch else "-" @@ -1163,6 +1222,11 @@ class GuestApp(App): #filter { display: none; } """ + def notify(self, message: str, **kwargs) -> None: + # messages carry guest names, paths and CLI/agent errors: never parse them as markup + kwargs.setdefault("markup", False) + super().notify(message, **kwargs) + def compose(self) -> ComposeResult: yield Header() with Vertical(id="guests_section") as section: diff --git a/src/cv4pve_tui/cli.py b/src/cv4pve_tui/cli.py index 36987db..79e30c6 100644 --- a/src/cv4pve_tui/cli.py +++ b/src/cv4pve_tui/cli.py @@ -28,7 +28,7 @@ class CliError(Exception): return str(self) -def run(*args: str) -> str: +def run(*args: str, timeout: int = 330) -> str: try: proc = subprocess.run( ["cv4pve-cli", *args], @@ -36,7 +36,7 @@ def run(*args: str) -> str: capture_output=True, text=True, stdin=subprocess.DEVNULL, - timeout=330, + timeout=timeout, ) except (OSError, subprocess.TimeoutExpired) as exc: raise CliError(str(exc)) from exc @@ -236,6 +236,58 @@ def backup_now(guest: dict, storage: str) -> None: ) # fmt: skip +def agent_enabled(config: dict) -> bool: + """VM option `agent`: `1`, `1,=...` or `enabled=1,...`.""" + parts = str(config.get("agent") or "").split(",") + return parts[0] == "1" or "enabled=1" in parts + + +def agent_ping(guest: dict) -> None: + """CliError if the guest agent does not answer.""" + run("do", "vm", "agent", "ping", guest["node"], str(guest["vmid"]), "-o", "json") + + +def fstrim(guest: dict) -> list[dict]: + """Trim the guest's filesystems; the per-path results, `[]` if they cannot be read. + + Well-formed entries come back as the agent sent them. Odd ones are repaired, not + fatal - the trim has already happened when the output is parsed: non-dict entries + are dropped, a non-int `trimmed` becomes 0 and a missing `path` becomes "?". + """ + args = ( + "do", + "vm", + "agent", + "fstrim", + guest["node"], + str(guest["vmid"]), + "-o", + "json", + ) + out = run( + *args, timeout=1800 + ) # a large disk can take longer than the default 330 s + try: + items = json.loads(out) + value = {x["key"]: x["value"] for x in items}.get("result") or "{}" + result = value if isinstance(value, dict) else json.loads(value) + paths = result.get("paths") if isinstance(result, dict) else None + except (ValueError, TypeError, KeyError, AttributeError): + return [] + found = [] + for p in paths if isinstance(paths, list) else []: + if not isinstance(p, dict): + continue + p = dict(p) + if "trimmed" in p and ( + not isinstance(p["trimmed"], int) or isinstance(p["trimmed"], bool) + ): + p["trimmed"] = 0 + p.setdefault("path", "?") + found.append(p) + return found + + def parse_net(value: str) -> dict: """`k=v,k=v` to an ordered dict; a VM's leading `=` is just the first pair.""" return dict(item.partition("=")[::2] for item in value.split(",") if item) diff --git a/tests/test_app.py b/tests/test_app.py index 82caad3..522ece8 100644 --- a/tests/test_app.py +++ b/tests/test_app.py @@ -4,6 +4,7 @@ All cli functions are monkeypatched: cv4pve-cli is never executed. """ import contextlib +import re import subprocess import threading from datetime import datetime @@ -89,6 +90,15 @@ class Fake: self.nodes_error, self.updates_errors = None, {} # node -> exception self.nodes_calls, self.updates_calls = 0, [] self.updates_hook = None + # issue #19 (fstrim via the guest agent) + self.events = [] # "ping"/"fstrim" in call order + self.ping_error, self.fstrim_error, self.fstrim_hook, self.ping_hook = ( + None, + None, + None, + None, + ) + self.fstrim_paths = [{"path": "/", "trimmed": 2**30, "minimum": 0}] def list_guests(self): self.list_calls += 1 @@ -259,6 +269,21 @@ class Fake: raise self.updates_errors[node] return rows + def agent_ping(self, guest): + self.events.append(("ping", guest["vmid"])) + if self.ping_hook: + self.ping_hook() + if self.ping_error: + raise self.ping_error + + def fstrim(self, guest): + self.events.append(("fstrim", guest["vmid"])) + if self.fstrim_hook: + self.fstrim_hook() + if self.fstrim_error: + raise self.fstrim_error + return [dict(p) for p in self.fstrim_paths] + def task_log(self, upid): self.log_calls.append(upid) if self.log_error: @@ -324,6 +349,8 @@ def fake(monkeypatch): "task_log", "nodes", "package_updates", + "agent_ping", + "fstrim", ): monkeypatch.setattr(cli, name, getattr(f, name), raising=False) for name in ("list_guests", "guest_config", "disks", "action", "set_resources", @@ -4195,3 +4222,242 @@ async def test_boot_set_net_failure_notifies_error_and_releases_busy(fake): assert any("unable to apply onboot" in m for m in errors(app)) assert app.busy == set() and app.is_running assert len(fake.set_calls) == 1 + + +# --- issue #19: fstrim via the guest agent (spec: "fstrim in a VM via the guest agent") ----- + + +def agent_vm(fake, config="enabled=1", status="running"): + fake.guests = [dict(VM, status=status)] + fake.live_status = {108: status} + if config is not None: + fake.net_configs[108] = {"agent": config} + + +def notes(app): + return " | ".join(m for m, _ in app.seen).lower() + + +async def press_f(app, pilot): + await pilot.press("f") + await settle(app, pilot) + + +@pytest.mark.parametrize("live", [None, "running"]) +async def test_fstrim_on_container_notifies_pct_fstrim_and_runs_nothing(fake, live): + # spec: container -> info notify (pct fstrim on the node instead); nothing runs + if live: + fake.live_status = {100: live} + app = make_app() + async with app.run_test(size=SIZE) as pilot: + await open_info(app, pilot, down=1) + await press_f(app, pilot) + assert any("pct fstrim" in m and sev == "information" for m, sev in app.seen) + assert fake.events == [] + assert isinstance(app.screen, appmod.InfoScreen) + + +async def test_fstrim_on_stopped_vm_warns_and_runs_nothing(fake): + # spec: VM not running -> warning notify saying why; nothing runs + agent_vm(fake, status="stopped") + app = make_app() + async with app.run_test(size=SIZE) as pilot: + await open_info(app, pilot) + await press_f(app, pilot) + assert any("running" in m.lower() and sev == "warning" for m, sev in app.seen) + assert fake.events == [] + assert isinstance(app.screen, appmod.InfoScreen) + + +@pytest.mark.parametrize("config", [None, "enabled=0", "0"]) +async def test_fstrim_on_vm_without_agent_warns_and_runs_nothing(fake, config): + # spec: agent_enabled false -> warning notify saying why; nothing runs + agent_vm(fake, config=config) + app = make_app() + async with app.run_test(size=SIZE) as pilot: + await open_info(app, pilot) + await press_f(app, pilot) + assert any("agent" in m.lower() and sev == "warning" for m, sev in app.seen) + assert fake.events == [] + assert isinstance(app.screen, appmod.InfoScreen) + + +async def test_fstrim_asks_confirm_titled_fstrim_naming_vm_before_any_call(fake): + # spec: ConfirmScreen titled "fstrim" naming the VM; nothing runs before yes + agent_vm(fake) + app = make_app() + async with app.run_test(size=SIZE) as pilot: + await open_info(app, pilot) + await press_f(app, pilot) + assert isinstance(app.screen, ConfirmScreen) + assert app.screen.title_text == "fstrim" + assert "docker" in text_of(app.screen) + assert fake.events == [] + + +async def test_fstrim_yes_pings_then_trims_once_busy_then_done_with_size_and_count( + fake, +): + # spec: yes -> busy guard, agent_ping then fstrim; done notify with GiB total and path count + agent_vm(fake) + fake.fstrim_paths = [ + {"path": "/", "trimmed": 2**30, "minimum": 0}, + {"path": "/boot", "error": "not supported"}, + ] + busy_seen = [] + fake.fstrim_hook = lambda: busy_seen.append(set(app.busy)) + app = make_app() + async with app.run_test(size=SIZE) as pilot: + await open_info(app, pilot) + await press_f(app, pilot) + await press_button(app, pilot, "#yes") + assert fake.events == [("ping", 108), ("fstrim", 108)] + assert busy_seen and busy_seen[0], "guest must be busy while fstrim runs" + assert app.busy == set() + done = [m for m, sev in app.seen if "1.0 GiB" in m] + assert done, app.seen + assert any(re.search(r"\b2\b", m) for m in done), "path count missing" + assert "not supported" in notes(app), "per-path error must be mentioned" + assert errors(app) == [] + + +async def test_fstrim_small_total_is_shown_in_mib(fake): + # spec: trimmed total in GiB/MiB + agent_vm(fake) + fake.fstrim_paths = [{"path": "/", "trimmed": 5 * 2**20, "minimum": 0}] + app = make_app() + async with app.run_test(size=SIZE) as pilot: + await open_info(app, pilot) + await press_f(app, pilot) + await press_button(app, pilot, "#yes") + assert "mib" in notes(app) and "gib" not in notes(app) + assert errors(app) == [] + + +@pytest.mark.parametrize("how", ["no", "escape"]) +async def test_fstrim_declined_makes_no_call(fake, how): + # spec: ConfirmScreen; declining runs nothing + agent_vm(fake) + app = make_app() + async with app.run_test(size=SIZE) as pilot: + await open_info(app, pilot) + await press_f(app, pilot) + assert isinstance(app.screen, ConfirmScreen) + if how == "no": + await press_button(app, pilot, "#no") + else: + await pilot.press("escape") + await settle(app, pilot) + assert fake.events == [] + assert not isinstance(app.screen, ConfirmScreen) + assert app.busy == set() + + +async def test_fstrim_ping_failure_errors_agent_did_not_answer_and_skips_fstrim(fake): + # spec: CliError -> error notify (ping failure says the agent did not answer) + agent_vm(fake) + fake.ping_error = cli_error("Error: VM 108 qmp command failed - timeout") + app = make_app() + async with app.run_test(size=SIZE) as pilot: + await open_info(app, pilot) + await press_f(app, pilot) + await press_button(app, pilot, "#yes") + assert fake.events == [("ping", 108)] + assert any("agent" in m.lower() and "answer" in m.lower() for m in errors(app)) + assert app.busy == set() and app.is_running + + +async def test_fstrim_failure_notifies_error_and_releases_busy(fake): + # spec: CliError -> error notify + agent_vm(fake) + fake.fstrim_error = cli_error("Error: fstrim command failed") + app = make_app() + async with app.run_test(size=SIZE) as pilot: + await open_info(app, pilot) + await press_f(app, pilot) + await press_button(app, pilot, "#yes") + assert fake.events == [("ping", 108), ("fstrim", 108)] + assert any("fstrim command failed" in m for m in errors(app)) + assert app.busy == set() and app.is_running + assert not any("GiB" in m or "MiB" in m for m, _ in app.seen) + + +async def test_fstrim_while_guest_busy_warns_and_makes_no_second_call(fake): + # spec: busy guard + agent_vm(fake) + started, release = threading.Event(), threading.Event() + fake.ping_hook = lambda: (started.set(), release.wait(10)) + app = make_app() + async with app.run_test(size=SIZE) as pilot: + await open_info(app, pilot) + try: + await press_f(app, pilot) + app.screen.query_one("#yes", Button).press() + for _ in range(200): + if started.is_set(): + break + await pilot.pause(0.01) + assert started.is_set() + await pilot.press("f") + await pilot.pause() + if isinstance(app.screen, ConfirmScreen): # refusal may come at "yes" + app.screen.query_one("#yes", Button).press() + await pilot.pause() + assert any(sev == "warning" for _, sev in app.seen) + assert fake.events.count(("ping", 108)) == 1 + finally: + release.set() + await settle(app, pilot) + assert fake.events.count(("ping", 108)) == 1 + assert fake.events.count(("fstrim", 108)) == 1 + + +# --- #19 review findings ---------------------------------------------------------------- + + +async def test_notifications_never_parse_markup(fake): + # guest names, paths and agent errors are untrusted text; notify must show them literally + app = make_app() + async with app.run_test(size=SIZE) as pilot: + await settle(app, pilot) + app.notify("path [/b] failed") + await pilot.pause() + assert app.is_running + assert all(n.markup is False for n in app._notifications) + + +async def test_fstrim_without_results_says_so_and_singular_path(fake): + agent_vm(fake) + app = make_app() + async with app.run_test(size=SIZE) as pilot: + fake.fstrim_paths = [] + await open_info(app, pilot) + await press_f(app, pilot) + await press_button(app, pilot, "#yes") + assert any("no result" in m.lower() for m, _ in app.seen) + fake.fstrim_paths = [{"path": "/", "trimmed": 2**20, "error": None}] + await press_f(app, pilot) + await press_button(app, pilot, "#yes") + assert any("in 1 path" in m and "paths" not in m for m, _ in app.seen) + + +async def test_fstrim_before_the_info_screen_loaded_says_still_loading( + fake, monkeypatch +): + agent_vm(fake) + release = threading.Event() + real = cli.guest_config + monkeypatch.setattr(cli, "guest_config", lambda g: (release.wait(10), real(g))[1]) + app = make_app() + async with app.run_test(size=SIZE) as pilot: + await pilot.pause() + await pilot.press("enter") + await pilot.pause() + try: + await pilot.press("f") + await pilot.pause() + assert any("loading" in m.lower() for m, _ in app.seen) + assert fake.events == [] + finally: + release.set() + await settle(app, pilot) diff --git a/tests/test_cli.py b/tests/test_cli.py index 2824d08..756e90e 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -1504,3 +1504,120 @@ def test_tags_value_is_idempotent_property(text): else: assert cli.tags_value(once) == once assert len(set(once.split(";"))) == len(once.split(";")) # no duplicates + + +# --- issue #19: guest-agent fstrim (spec: "fstrim in a VM via the guest agent") ---------- + + +@pytest.mark.parametrize( + "agent,expected", + [ + ("enabled=1", True), + ("1", True), + ("1,fstrim_cloned_disks=1", True), + ("enabled=0", False), + ("0", False), + (None, False), + ], +) +def test_agent_enabled(agent, expected): + config = {} if agent is None else {"agent": agent} + assert cli.agent_enabled(config) is expected + + +def test_agent_ping_exact_argv(fake): + fake.result = (json.dumps([{"key": "result", "value": "{}"}]), "", 0) + cli.agent_ping(VMID108) + assert fake.argv == [ + "cv4pve-cli", "do", "vm", "agent", "ping", "mainframe", "108", "-o", "json", + ] # fmt: skip + + +def test_agent_ping_nonzero_exit_raises_clierror(monkeypatch): + FakeRun(monkeypatch, "Error: QEMU guest agent is not running", returncode=5) + with pytest.raises(cli.CliError): + cli.agent_ping(VMID108) + + +FSTRIM_PATHS = [ + {"path": "/", "trimmed": 1073741824, "minimum": 0}, + {"path": "/boot", "error": "not supported"}, +] + + +def fstrim_stdout(value): + return json.dumps([{"key": "result", "value": value}]) + + +def test_fstrim_exact_argv_and_parses_paths(fake): + fake.result = (fstrim_stdout(json.dumps({"paths": FSTRIM_PATHS})), "", 0) + assert cli.fstrim(VMID108) == FSTRIM_PATHS + assert fake.argv == [ + "cv4pve-cli", "do", "vm", "agent", "fstrim", "mainframe", "108", "-o", "json", + ] # fmt: skip + + +@pytest.mark.parametrize( + "stdout", + [ + "[]", + json.dumps([{"key": "other", "value": "{}"}]), + fstrim_stdout("not json at all"), + fstrim_stdout("{}"), + fstrim_stdout(""), + ], +) +def test_fstrim_missing_or_garbage_result_is_empty_list(fake, stdout): + fake.result = (stdout, "", 0) + assert cli.fstrim(VMID108) == [] + + +def test_fstrim_nonzero_exit_raises_clierror(monkeypatch): + FakeRun(monkeypatch, "Error: fstrim failed", returncode=5) + with pytest.raises(cli.CliError): + cli.fstrim(VMID108) + + +# --- #19 review findings ---------------------------------------------------------------- + +VM19 = {"vmid": 108, "node": "mainframe", "type": "qemu"} + + +@pytest.mark.parametrize( + "stdout", + [ + "garbage", + "", + json.dumps({"key": "result"}), # object instead of list + json.dumps([{"value": "{}"}]), # item without "key" + json.dumps([{"key": "result", "value": {"paths": "x"}}]), # already a dict + ], +) +def test_fstrim_unreadable_output_is_empty_not_an_error(fake, stdout): + fake.result = (stdout, "", 0) + assert cli.fstrim(VM19) == [] + + +def test_fstrim_normalises_odd_path_entries(fake): + paths = [ + {"path": "/", "trimmed": "12"}, + {"error": "x"}, + "junk", + {"path": "/var", "trimmed": 5}, + ] + fake.result = ( + json.dumps([{"key": "result", "value": json.dumps({"paths": paths})}]), + "", + 0, + ) + assert cli.fstrim(VM19) == [ + {"path": "/", "trimmed": 0}, # non-int trimmed -> 0 + {"error": "x", "path": "?"}, # missing path -> "?"; "junk" dropped + {"path": "/var", "trimmed": 5}, # well-formed: unchanged + ] + + +def test_fstrim_gets_a_longer_timeout_than_other_calls(fake): + fake.result = (json.dumps([{"key": "result", "value": "{}"}]), "", 0) + cli.fstrim(VM19) + assert fake.calls[0][1]["timeout"] >= 1800 # a big disk can take longer than 330 s