From 90768f17e147185b7bb7cea419d52cfd0480bf8d Mon Sep 17 00:00:00 2001 From: "Claude (agent)" Date: Mon, 21 Sep 2026 14:11:09 -0400 Subject: [PATCH] feat(flr): windows paths, recovery-site gate, stable partition reads (#4) --- CONTEXT.md | 1 + README.md | 2 +- docs/recover-ladder.md | 24 +++++- skills/zerto-rewind/SKILL.md | 5 +- src/zerto_rewind_mcp/client.py | 10 ++- src/zerto_rewind_mcp/recover.py | 100 +++++++++++++++++----- src/zerto_rewind_mcp/server.py | 56 ++++++++++++ tests/test_recover.py | 147 ++++++++++++++++++++++++++------ 8 files changed, 295 insertions(+), 50 deletions(-) diff --git a/CONTEXT.md b/CONTEXT.md index 6580ee9..d3b8871 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -22,6 +22,7 @@ _Avoid_: recover (unqualified), fail back, restore the VPG **File-level recovery (FLR)**: Mount a VM from a journal checkpoint and pull files. The VM stays up. 10.9 FLR Operator RBAC is broken; Administrator is the documented workaround. +Runs at the VPG's **recovery** site, so this server supports it only for locally replicated VPGs (protected site == recovery site). Paths are partition-rooted; on Windows the drive letter is the partition. _Avoid_: file restore (unqualified), instant restore (local-journal VMs only, not v1) **find_protection**: diff --git a/README.md b/README.md index b00366b..5f8fd58 100644 --- a/README.md +++ b/README.md @@ -8,7 +8,7 @@ If the loop works, these tools are the delta to add to official ZVM MCP (`ZVM.MC 1. `zerto_find_protection` — VM name, hostname, or vmIdentifier to exactly one VM and every VPG. Zero or two-plus VMs: stop. 2. `zerto_create_tagged_checkpoint` / `zerto_guard_before_mutate` — same tag on every protecting VPG, wait until listed. The name records which agent and what it is doing: `ai: | | vm= | change= | `. -3. `zerto_recover_file` — FLR after a human sets `confirmed=true`. Reports its own unmount; `zerto_list_flr_sessions` / `zerto_end_flr_session` find and reap a mount orphaned by a crashed recovery. +3. `zerto_recover_file` — FLR after a human sets `confirmed=true`. Linux and Windows guest paths. Locally replicated VPGs only: FLR runs at the VPG's recovery site. Reports its own unmount; `zerto_list_flr_sessions` / `zerto_end_flr_session` find and reap a mount orphaned by a crashed recovery. 4. Mutating catalog — opt-in list of MCP tools that must be guarded. Unlisted tools pass through. Users add entries. Official ZVM MCP already has inventory and failover test. It does not insert tagged checkpoints or run FLR. diff --git a/docs/recover-ladder.md b/docs/recover-ladder.md index da0e444..24ad153 100644 --- a/docs/recover-ladder.md +++ b/docs/recover-ladder.md @@ -11,7 +11,29 @@ known path (config, dropped file, one directory). FLR mounts a checkpoint and copies files out. The protected VM stays up. Official API: `POST /v1/flrs` then browse/download. This MCP writes the file -to `recovery_dir` on the MCP host. Putting it back on the guest is a second +to `recovery_dir` on the MCP host. + +**FLR runs at the VPG's recovery site**, because that is where the mount is +created. A VPG replicating to a cloud ZCA must be recovered through that ZCA's +API, not the protected ZVM's. A production server would hold credentials for +every ZVM/ZCA in the estate and route the call; this one does not, so +`zerto_recover_file` is gated to **locally replicated VPGs** (protected site == +recovery site) and refuses anything else while naming the site that owns the +operation. + +Paths are rooted at partitions, and Linux and Windows are not symmetrical: + +| | guest path | FLR path | +|---|---|---| +| Linux | `/home/j/app.yaml` | `Volume2-Ext4%2fhome%2fj%2fapp.yaml` | +| Windows | `C:\Users\j\app.conf` | `C%3a%2fUsers%2fj%2fapp.conf` | + +On Windows the drive letter **is** the partition name, so nothing is +prepended. Browse form-encodes: `%2f` separator, `%3a` drive colon, and a +space as `+` (`Program+Files`). A session reports mounted before volume +enumeration settles, so the partition list must be polled until it stops +changing -- an early read can show a restorable NTFS disk as +`Volume4-Unknown`. Putting it back on the guest is a second step (scp/ssh). That copy-back is not Zerto; it is ordinary file transfer. Do not use FLR when: diff --git a/skills/zerto-rewind/SKILL.md b/skills/zerto-rewind/SKILL.md index c257389..eec10a5 100644 --- a/skills/zerto-rewind/SKILL.md +++ b/skills/zerto-rewind/SKILL.md @@ -37,7 +37,10 @@ A VM can be in up to three VPGs (local backup + remote DR is common). Tag all of Human must confirm. Pass `confirmed=true` only after they say yes. -- Bad config / dropped file: `zerto_recover_file` from **that tag**. +- Bad config / dropped file: `zerto_recover_file` from **that tag**. Pass the guest path + (`/home/j/app.yaml` or `C:\Users\j\app.conf`); the server maps it into the FLR + namespace. **Locally replicated VPGs only** -- FLR happens at the recovery site, so a + cloud-replicated VPG must be recovered from that ZCA. The tool refuses and names the site. - Inspect a whole VM: `zerto_offsite_clone` or `zerto_start_failover_test`. - Never Failover Live. Never Move. Those are DR, not rewind. diff --git a/src/zerto_rewind_mcp/client.py b/src/zerto_rewind_mcp/client.py index 7ff388f..9d68d5e 100644 --- a/src/zerto_rewind_mcp/client.py +++ b/src/zerto_rewind_mcp/client.py @@ -151,7 +151,7 @@ class ZertoClient: async def get_vpg(self, vpg_identifier: str) -> dict[str, Any]: data = await self.json("GET", f"/v1/vpgs/{vpg_identifier}") if not isinstance(data, dict): - raise ZertoError("GET /v1/vpgs/{id} did not return an object") + raise ZertoError(f"GET /v1/vpgs/{vpg_identifier} did not return an object") return data async def list_checkpoints(self, vpg_identifier: str) -> list[dict[str, Any]]: @@ -231,6 +231,14 @@ class ZertoClient: status_code=last.status_code if last else None, ) + async def get_localsite(self) -> dict[str, Any]: + data = await self.json("GET", "/v1/localsite") + return data if isinstance(data, dict) else {} + + async def get_peersites(self) -> list[dict[str, Any]]: + data = await self.json("GET", "/v1/peersites") + return [r for r in data if isinstance(r, dict)] if isinstance(data, list) else [] + async def list_flrs(self) -> Any: """GET /v1/flrs. Every FLR session the ZVM currently knows about.""" return await self.json("GET", "/v1/flrs") diff --git a/src/zerto_rewind_mcp/recover.py b/src/zerto_rewind_mcp/recover.py index 7235690..bb8f39b 100644 --- a/src/zerto_rewind_mcp/recover.py +++ b/src/zerto_rewind_mcp/recover.py @@ -9,6 +9,9 @@ from typing import Any from zerto_rewind_mcp.client import ZertoClient, ZertoError from zerto_rewind_mcp.util import pick +# Seconds between partition-enumeration polls. Module level so tests can shrink it. +PARTITION_POLL_INTERVAL_S = 5.0 + READY_STATUSES = { "ready", "mounted", @@ -98,8 +101,15 @@ async def wait_flr_ready( def _decode_flr_path(value: Any) -> str: - """Browse returns paths percent-encoded (%2f). Download wants them decoded.""" - return urllib.parse.unquote(str(value or "")).replace("\\", "/") + """Decode a browse path for display and comparison. + + Browse form-encodes: '%2f' is the separator, '%3a' the drive colon, and a + SPACE comes back as '+' ("Program+Files"). unquote alone leaves the '+', + so a basename compare against "Program Files" would never match. + Download accepts the raw and the decoded form, so we send the raw one and + only decode for matching and display. + """ + return urllib.parse.unquote_plus(str(value or "")).replace("\\", "/") def path_items(payload: Any) -> list[dict[str, Any]]: @@ -112,46 +122,94 @@ def path_items(payload: Any) -> list[dict[str, Any]]: async def browsable_partitions(client: ZertoClient, session_id: str) -> list[str]: - """FLR is rooted at partitions (Volume2-Ext4), not the guest's /. + """FLR is rooted at partitions, not the guest's /. - Volume1-Unknown and friends report IsBrowsable false and cannot be restored. + Linux: Volume2-Ext4. Windows: the drive letter is the partition name and + comes back percent-encoded, e.g. 'C%3a'. Partitions reporting IsBrowsable + false (FAT32, MicrosoftReservedPartition, Unknown) cannot be restored from. """ rows = path_items(await client.browse_flr(session_id, path="")) return [str(r.get("Path")) for r in rows if r.get("IsBrowsable")] -async def resolve_flr_path(client: ZertoClient, session_id: str, guest_path: str) -> str: - """Map a guest absolute path to the FLR namespace path the download API accepts. +async def wait_partitions_stable( + client: ZertoClient, + session_id: str, + *, + timeout_s: float = 120.0, + interval_s: float | None = None, +) -> list[str]: + """Wait until the partition list stops changing, then return it. - /home/justin/app-config.yaml -> Volume2-Ext4/home/justin/app-config.yaml + A session reports mounted before the ZVM has finished identifying volumes. + Browsing in that window returns a partial and MIS-LABELLED list: the same + Windows VM enumerated as 'Volume4-Unknown' with no C: drive, and moments + later as a browsable 'C%3a' holding the whole filesystem. Acting on the + early list makes a restorable disk look permanently unrestorable. + """ + interval_s = PARTITION_POLL_INTERVAL_S if interval_s is None else interval_s + deadline = asyncio.get_event_loop().time() + timeout_s + previous: list[str] | None = None + while asyncio.get_event_loop().time() < deadline: + current = await browsable_partitions(client, session_id) + if current and previous is not None and current == previous: + return current + previous = current + await asyncio.sleep(interval_s) + if previous: + return previous + raise ZertoError( + "FLR mounted but no browsable partition appeared. Unsupported partition " + "type (FAT32, MicrosoftReservedPartition, LVM, unknown) cannot be restored." + ) + + +async def resolve_flr_path(client: ZertoClient, session_id: str, guest_path: str) -> str: + """Map a guest absolute path to the path the FLR download API accepts. + + Linux /home/justin/app-config.yaml -> Volume2-Ext4%2fhome%2fjustin%2f... + Windows C:\\Users\\x\\f.txt -> C%3a%2fUsers%2fx%2ff.txt + + The two are not symmetrical. On Linux the partition is a separate root that + must be prepended. On Windows the drive letter IS the partition, so the + guest path already carries it and prepending again yields 'C:/C:/Users'. + + Returns the raw path exactly as browse reported it; download accepts that + verbatim, which avoids re-encoding the '+' and '%3a' back by hand. """ rel = guest_path.replace("\\", "/").strip("/") if not rel: raise ZertoError("Empty guest_path") parts = rel.split("/") - name, parent = parts[-1], "/".join(parts[:-1]) + name, parent_rel = parts[-1], "/".join(parts[:-1]) - partitions = await browsable_partitions(client, session_id) - if not partitions: - raise ZertoError( - "FLR mounted but no browsable partition. Unsupported partition type " - "(LVM/unknown) cannot be restored by FLR." - ) - tried = [] + partitions = await wait_partitions_stable(client, session_id) + tried: list[str] = [] for vol in partitions: - probe = f"{vol}/{parent}" if parent else vol + vol_dec = _decode_flr_path(vol).rstrip("/") + low_rel, low_vol = rel.lower(), vol_dec.lower() + if low_rel == low_vol or low_rel.startswith(low_vol + "/"): + # Windows: guest path already starts with the drive-letter partition + probe = parent_rel or vol_dec + else: + probe = f"{vol_dec}/{parent_rel}" if parent_rel else vol_dec tried.append(probe) try: rows = path_items(await client.browse_flr(session_id, path=probe)) except ZertoError: continue - for row in rows: - decoded = _decode_flr_path(row.get("Path")) - if decoded.rsplit("/", 1)[-1] == name: - return decoded + # exact match first; Windows is case-insensitive, Linux is not, so only + # fall back to a case-insensitive match when nothing matched exactly. + for want_exact in (True, False): + for row in rows: + raw = str(row.get("Path") or "") + got = _decode_flr_path(raw).rsplit("/", 1)[-1] + if got == name if want_exact else got.lower() == name.lower(): + return raw raise ZertoError( f"{guest_path!r} not found in the FLR mount. Looked under {tried}. " - "The file may not have replicated into that checkpoint yet." + f"Browsable partitions were {partitions}. Either the path is wrong, or " + "the file had not replicated into that checkpoint yet." ) diff --git a/src/zerto_rewind_mcp/server.py b/src/zerto_rewind_mcp/server.py index 85cc6fc..addae28 100644 --- a/src/zerto_rewind_mcp/server.py +++ b/src/zerto_rewind_mcp/server.py @@ -260,6 +260,55 @@ async def zerto_add_mutating_tool( return _dump({"ok": True, "entry": entry.as_dict()}) +async def _site_name(client: ZertoClient, identifier: str | None) -> str: + if not identifier: + return "unknown site" + try: + local = await client.get_localsite() + if str(local.get("SiteIdentifier")) == identifier: + return str(local.get("SiteName") or identifier) + for peer in await client.get_peersites(): + if str(peer.get("SiteIdentifier")) == identifier: + return str(peer.get("PeerSiteName") or identifier) + except ZertoError: + pass + return identifier + + +async def _flr_site_gate(client: ZertoClient, vpg_identifier: str) -> dict[str, Any]: + """Refuse FLR on a VPG whose recovery site is not this ZVM. + + FLR only exists at the VPG's RECOVERY site: the mount is created there. A + VPG replicating to a cloud ZCA has to be recovered from that ZCA's API, not + this one. Until this server can hold credentials for every ZVM/ZCA in an + estate and route the call, restrict FLR to local replication (protected + site == recovery site) and say plainly where the operation actually lives, + rather than letting it fail as a confusing path or mount error. + """ + try: + vpg = await client.get_vpg(vpg_identifier) + except ZertoError as exc: + return {"ok": False, "message": f"Could not read VPG {vpg_identifier}: {exc}"} + protected = (vpg.get("ProtectedSite") or {}).get("identifier") + recovery = (vpg.get("RecoverySite") or {}).get("identifier") + if protected and recovery and str(protected) == str(recovery): + return {"ok": True} + where = await _site_name(client, str(recovery) if recovery else None) + return { + "ok": False, + "not_local_replication": True, + "vpg_name": vpg.get("VpgName"), + "recovery_site": where, + "message": ( + f"FLR for VPG {vpg.get('VpgName')!r} lives at its recovery site " + f"({where}), not at this ZVM. This server only supports file " + "recovery for locally replicated VPGs (protected site == recovery " + "site). Point an MCP instance at that ZVM/ZCA, or use a bounded " + "whole-VM operation instead." + ), + } + + async def _live_session_ids(client: ZertoClient) -> set[str]: try: rows = session_rows(await client.list_flrs()) @@ -327,6 +376,10 @@ async def zerto_recover_file( Requires confirmed=true (human yes). Cannot run during clone/test/live/EJC. 10.9 FLR Operator role fails; use an Administrator account. + + Locally replicated VPGs only. FLR is performed at the VPG's recovery site, + so a VPG replicating to a cloud ZCA must be recovered from that ZCA's API. + guest_path is the path on the guest: /home/x/f.conf or C:\\Users\\x\\f.txt. """ if not confirmed: return _dump( @@ -339,6 +392,9 @@ async def zerto_recover_file( } ) client = get_client() + gate = await _flr_site_gate(client, vpg_identifier) + if not gate.get("ok"): + return _dump(gate) dest = Path(dest_dir or _settings.get("recovery_dir") or "./recovered") dest.mkdir(parents=True, exist_ok=True) session_id: str | None = None diff --git a/tests/test_recover.py b/tests/test_recover.py index b9a6153..484bfc7 100644 --- a/tests/test_recover.py +++ b/tests/test_recover.py @@ -46,39 +46,96 @@ def test_decode_flr_path(): ) -def test_resolve_flr_path_picks_browsable_partition(): +class _BrowseFake: + """Fake FLR mount. root is the partition list; tree maps probe path -> rows.""" + + def __init__(self, root, tree): + self.root, self.tree, self.seen = root, tree, [] + + async def browse_flr(self, session_id, path="", recursive=False): + self.seen.append(path) + if path == "": + return {"PathItems": self.root} + return {"PathItems": self.tree.get(path, [])} + + +def _resolve(client, guest_path): import asyncio - from zerto_rewind_mcp.recover import resolve_flr_path + from zerto_rewind_mcp import recover - root = { - "PathItems": [ + recover.PARTITION_POLL_INTERVAL_S = 0 # do not sleep in tests + return asyncio.run(recover.resolve_flr_path(client, "sess", guest_path)) + + +def test_resolve_flr_path_linux_prepends_partition(): + client = _BrowseFake( + [ {"Path": "Volume1-Unknown", "IsBrowsable": False}, {"Path": "Volume2-Ext4", "IsBrowsable": True}, - ] - } - listing = { - "PathItems": [ - {"Path": "Volume2-Ext4%2fhome%2fjustin%2f.bashrc", "Type": "File"}, - {"Path": "Volume2-Ext4%2fhome%2fjustin%2fapp-config.yaml", "Type": "File"}, - ] - } - - class FakeClient: - def __init__(self): - self.seen = [] - - async def browse_flr(self, session_id, path="", recursive=False): - self.seen.append(path) - return root if path == "" else listing - - client = FakeClient() - got = asyncio.run(resolve_flr_path(client, "sess", "/home/justin/app-config.yaml")) - assert got == "Volume2-Ext4/home/justin/app-config.yaml" - # must not try the unrestorable partition + ], + { + "Volume2-Ext4/home/justin": [ + {"Path": "Volume2-Ext4%2fhome%2fjustin%2f.bashrc", "Type": "File"}, + {"Path": "Volume2-Ext4%2fhome%2fjustin%2fapp-config.yaml", "Type": "File"}, + ] + }, + ) + # returns the raw path browse gave us; download accepts it verbatim + assert _resolve(client, "/home/justin/app-config.yaml") == ( + "Volume2-Ext4%2fhome%2fjustin%2fapp-config.yaml" + ) assert "Volume1-Unknown/home/justin" not in client.seen +def test_resolve_flr_path_windows_does_not_double_the_drive_letter(): + # On Windows the drive letter IS the partition: C%3a decodes to "C:". + client = _BrowseFake( + [ + {"Path": "C%3a", "IsBrowsable": True}, + {"Path": "Volume2-FAT32", "IsBrowsable": False}, + ], + {"C:/Users/justin": [{"Path": "C%3a%2fUsers%2fjustin%2fapp.conf", "Type": "File"}]}, + ) + assert _resolve(client, r"C:\Users\justin\app.conf") == "C%3a%2fUsers%2fjustin%2fapp.conf" + # the bug this guards: probing C:/C:/Users/justin + assert not any(p.count("C:") > 1 for p in client.seen) + + +def test_resolve_flr_path_windows_handles_spaces_encoded_as_plus(): + client = _BrowseFake( + [{"Path": "C%3a", "IsBrowsable": True}], + { + "C:/Program Files/app": [ + {"Path": "C%3a%2fProgram+Files%2fapp%2fmy+config.ini", "Type": "File"} + ] + }, + ) + got = _resolve(client, r"C:\Program Files\app\my config.ini") + assert got == "C%3a%2fProgram+Files%2fapp%2fmy+config.ini" + + +def test_wait_partitions_stable_ignores_the_early_mislabelled_list(): + import asyncio + + from zerto_rewind_mcp import recover + + recover.PARTITION_POLL_INTERVAL_S = 0 + # first poll is the pre-enumeration list, then it settles on the real one + polls = [ + [{"Path": "Volume4-Unknown", "IsBrowsable": False}], + [{"Path": "C%3a", "IsBrowsable": True}], + [{"Path": "C%3a", "IsBrowsable": True}], + ] + + class Settling: + async def browse_flr(self, session_id, path="", recursive=False): + return {"PathItems": polls.pop(0) if polls else [{"Path": "C%3a", "IsBrowsable": True}]} + + got = asyncio.run(recover.wait_partitions_stable(Settling(), "sess")) + assert got == ["C%3a"] + + def test_session_rows_and_id_shapes(): from zerto_rewind_mcp.recover import session_id_of, session_rows @@ -159,3 +216,43 @@ def test_teardown_leaves_pre_existing_sessions_alone(): out = asyncio.run(_teardown_flr(client, None, {"someone-else"})) assert out["ended"] == [] assert client.ended == [] + + +class _SiteFake: + def __init__(self, protected, recovery): + self._vpg = { + "VpgName": "demo-vpg", + "ProtectedSite": {"identifier": protected}, + "RecoverySite": {"identifier": recovery}, + } + + async def get_vpg(self, vpg_identifier): + return self._vpg + + async def get_localsite(self): + return {"SiteIdentifier": "site-local", "SiteName": "VMware Site"} + + async def get_peersites(self): + return [{"SiteIdentifier": "site-aws", "PeerSiteName": "aws-zca"}] + + +def test_flr_gate_allows_local_replication(): + import asyncio + + from zerto_rewind_mcp.server import _flr_site_gate + + out = asyncio.run(_flr_site_gate(_SiteFake("site-local", "site-local"), "v1")) + assert out["ok"] is True + + +def test_flr_gate_refuses_remote_recovery_site_and_names_it(): + import asyncio + + from zerto_rewind_mcp.server import _flr_site_gate + + out = asyncio.run(_flr_site_gate(_SiteFake("site-local", "site-aws"), "v1")) + assert out["ok"] is False + assert out["not_local_replication"] is True + # must tell the operator where the operation actually lives + assert out["recovery_site"] == "aws-zca" + assert "aws-zca" in out["message"]