diff --git a/CHANGELOG.md b/CHANGELOG.md index 6e0ad3e..66adcf4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,76 @@ +## v0.118.0 — R-106 + R-109: the DR recipe stops guessing (2026-07-30) + +**The recipe is read at the worst possible moment — by an operator rebuilding a machine that is gone — +and it was wrong about the PBS namespace and silent about the backup target.** Both were confirmed live +on both demo boxes before the fix, in the recipe the hub actually serves: + + "pbs": { "repo_id": "felhom-pbs", "namespace": "root", "latest_snapshot_id": "9201" } + (no backup_target field at all) + +while `/etc/pve/storage.cfg` on those same boxes reads `namespace demo-felhom` / `namespace demo-hp`. + +**R-106 — the namespace.** `DRPBSCoord.Namespace` was taken from the LISTED SNAPSHOT. PBS does not echo +`ns` per item once the request is already namespace-scoped via `?ns=` (`internal/pbs/client.go:118-120`), +so `Snapshot.Namespace` was always empty, `ToHub` normalised empty → `"root"` (`internal/pbs/report.go:22-25`), +and `latestPBSCoord` wrote that into the recipe. Every per-customer box therefore reported the root +namespace while its backups lived in a tenant one. It now resolves from the pbs STORAGE — `PBSNamespace`, +carried on `StorageTarget` from storage.cfg's `namespace` — which is the same field `vzdump --storage ` +makes PVE read, so the recipe cannot disagree with the backup that produced the snapshot. One state, one +owner. An unconfigured namespace still reads `"root"`: that is an ANSWER (the box is genuinely in the root +namespace), and `namespace_state: resolved` distinguishes it from not knowing. + +**R-109 — the backup target.** The recipe listed every storage's name/type/content and never said which +one holds the local whole-guest archives. Harmless while that was the well-known `local`; not harmless +since the 2026-07-28 vzdump-target move, after which every box carries TWO `content=backup` dir storages — +`felhom-backup` (live) and `local` (archives frozen at the move) — indistinguishable by name, type and +content. Picking the frozen one restores a guest that restores cleanly and is silently months stale. The +host-half now carries `backup_target`, resolved from the PRIMARY tier of `cfg.Backup.BackupTiers()` — the +same function the scheduler consults, not a re-derivation — plus the mountpoint, which is what actually +separates `/mnt/hdd_1` from `/var/lib/vz`. + +**Absence is recorded as absence.** Neither field emits a default, an empty string, or a placeholder when +it cannot be resolved. `backup_target` has three outcomes and the two unknowns are deliberately distinct: +`agent_backup_config_unavailable` (the collector has no config seam) and `not_a_known_storage` (the config +names a storage this host does not have — the id IS still recorded, because that sends an operator +somewhere useful while silence does not). `namespace_state` likewise refuses to default to root when there +is no storage row to read. + +**The resolver reports the tier IN EFFECT, not the file.** `SetBackupTargetResolver` closes over the +daemon-start config on purpose. A backup-target move rewrites `agent.json` and deliberately does NOT restart +the agent (the E-1 lesson: restarting mid-backup records a spurious failure for a run that succeeded), so +between the write and the restart the file names a target no backup is writing to yet. The live-reload shape +used for `escrow.pbs_storage_id` would have made the recipe point at the new storage while every archive +still landed on the old one. + +**A cross-repo half of this was load-bearing and is easy to miss:** the hub's `AssembleDRRecipe` allow-lists +top-level keys, so `backup_target` would have been *stored intact and silently dropped* before reaching any +operator. Shipped together with hub v0.83.0; neither half is useful alone. + +Not touched: the backup machinery. This corrects the RECORD, not the doing — and the record was never +acted on programmatically (the host_loss plan is surface-only, `internal/dr/plan.go:125`), so the blast +radius was exactly one operator reading a wrong value. `plan.PBS` inherits the fix for free. + +- `internal/hub/report.go` — `StorageTarget.PBSNamespace` (`json:"-"`, the ConfigPath precedent: the struct + is a cross-repo contract and nothing off-box needs this value). +- `internal/storage/observe.go` — populate it from `proxmox.Storage.Namespace`. +- `internal/hub/dr_recipe.go` — `DRBackupTarget`, `ConfiguredBackupTarget`, `DRState*`/`DRReason*`, + `namespace_state`/`namespace_reason`, `resolveBackupTarget`, namespace from the storage. +- `internal/hub/collect.go` — `SetBackupTargetResolver` seam; an unwired seam reports unknown, never a guess. +- `cmd/felhom-agent/main.go` — `primaryBackupTargetOf` wired at BOTH collector sites (the daemon and + `selftest=hub`, so "the report it would send" really matches what the daemon sends). +- `testdata/host-report.golden.json` — new keys; kept byte-identical with the hub's copy (`f4bc3554…`). + +Tests: 9 new. The consequence ones assert a box with two `content=backup` storages names the LIVE one and +does not name the frozen one — `assertBackupCandidateAmbiguity` fails the test if the fixture ever stops +posing that problem, so it cannot pass hollow. Fixtures are the storage set demo-felhom really had +(provenance recorded in the file: the box's own pre-fix recipe + its `storage.cfg`), not composed structs. +Two run the REAL path, `Collector.Collect()`, one of them specifically pinning that an unwired seam yields +unknown — a seam built and never wired is the failure mode this repo has hit four times. Red-proofs: 4, +each mutation asserted to have landed before running (revert R-106 → the live `"root"` symptom reproduces +in both the unit and production-path tests; make the unknown case guess → 5 assertions fire; drop the +field → the cross-repo contract guard fires too; hollow the fixture → the ambiguity guard fires). +Suite rc=0, 29 packages, 0 FAIL. + ## v0.117.0 — R-117: the liveness signal now tests liveness (2026-07-30) **`BoundUnderParent` reported a namespace that returned `EIO` on every read and write as healthy, and the diff --git a/REPORT.md b/REPORT.md index 0bd20ec..2913696 100644 --- a/REPORT.md +++ b/REPORT.md @@ -1,103 +1,76 @@ -# REPORT — v0.117.0: R-117, the liveness signal now tests liveness (2026-07-30) +# REPORT — R-106 + R-109: the DR recipe stops guessing (agent v0.118.0, 2026-07-30) -**Overwritten** per the standing rule. Full evidence: -`felhom.eu/documentation/audits/R117-v0117-2026-07-30.md`. Spike this builds on: -`audits/SPIKE-r117-bind-liveness-2026-07-30.md`. +**Overwritten** per the standing rule. Paired with **hub v0.83.0** — neither half is useful alone, see §5. -| | | -|---|---| -| agent | 0.116.0 → **0.117.0** @ `966d8f4` | -| published + vouched | **0.117.0**, sha `e6768a12eba1c9e0…` (anonymous GET verified, manifest read back) | -| deployed | **demo-hp 0.117.0** (from **0.113.0** — see R-121) · felhom-pve still **0.115.0**, out of scope | -| controller / golden / hub | 0.186.0 / 0.186.0 / 0.82.0 — **untouched** | -| tests | **849 → 863**, 29/29 packages, `build`/`vet`/`test` each `rc=0` read separately | +## 1. What was wrong, confirmed live BEFORE the fix -## The defect +Downloaded from the hub, `GET /customers//dr-recipe.json`, both demo boxes: -`BoundUnderParent` reported a namespace returning `EIO` on every read and write as healthy, and the gate -restarted the customer's apps onto it. Both existing terms — `GuestSeesMount` and `isHostMountpoint` — -parse a mountinfo line and then test only `fields[4]`, the mount POINT. **Field 3, `major:minor`, sat in -the same parsed slice and was discarded.** +```json +"pbs": { "repo_id": "felhom-pbs", "namespace": "root", "latest_snapshot_id": "9201" } +``` -## The fix — one predicate, both halves mandatory +and **no `backup_target` key at all**, while `pve_storage` offered two plausible candidates: -`bindLiveness` (`internal/localapi/intermediary.go`) becomes the third term of the conjunction at both -`/disks` construction sites. `/proc` only, **no block I/O**: +```json +{ "name": "felhom-backup", "type": "local-dir", "content": "backup" } +{ "name": "local", "type": "local", "content": "backup,import,vztmpl,iso" } +``` -- **the bind must name the same device as the raw mount** — sound, not heuristic: a stale bind pins the - dead superblock, which keeps the old device index allocated, which forces the returning device onto a - different number; -- **the filesystem must not have aborted** — `shutdown` or `emergency_ro`, both measured. +`/etc/pve/storage.cfg` on those same boxes says `namespace demo-felhom` and `namespace demo-hp`. -**The second is not optional.** A device that fails *without disappearing* gives the identical -all-signals-healthy state with the **devnos equal** and the drive never `Disconnected`, so the gate emits -nothing at all, indefinitely (R-117a). A devno-only fix passes every payload test; red-proof RP3 exists for -exactly that. +## 2. Root causes, at source -**Three states, never a bool:** `{Unknown, Live, StaleDevice, Aborted}`, `Unknown` the zero value, and -`Usable()` is the single place where unknown counts as **present** — absent stops a working customer's apps. +- **R-106** — `Snapshot.Namespace` decodes `ns` (`internal/pbs/client.go:97`), which PBS does not echo per + item once the list is namespace-scoped via `?ns=` (`:118-120`). Always empty → `ToHub` normalises empty + to `"root"` (`internal/pbs/report.go:22-25`) → `latestPBSCoord` writes it into the recipe. +- **R-109** — `BuildDRRecipeHostHalf` emitted `guests`/`drives`/`pve_storage`/`pbs` and nothing naming the + vzdump target. Benign until the 2026-07-28 target move gave every box a second `content=backup` dir + storage holding archives frozen at that date. -## The ruling (task §2.2) — and it is flagged for overrule +## 3. The fix -`StaleDevice` ⇒ **re-bind** (the raw mount is a healthy new superblock; repairs live, guest never restarts). -`Aborted` ⇒ **quiet no-op, and surface via `BoundUnderParent=false`** — a re-bind lands on the *same* dead -superblock and this runs every 20 s, so repairing would be an infinite silent retry that also masks the -state. **No operator decision taken quietly:** it does change customer-visible behaviour (apps stop and an -alarm fires where today nothing happens), but it routes an already-broken state into the **existing** gate, -event types and Hungarian copy — no new concept reaches the customer — and the alternative is applications -writing documents into a filesystem that rejects every write. +The namespace now resolves from the pbs **storage** (`StorageTarget.PBSNamespace`, from storage.cfg's +`namespace`) — the same field `vzdump --storage ` makes PVE read, so the recipe cannot disagree with +the backup that produced the snapshot. `backup_target` resolves from the **primary tier of +`cfg.Backup.BackupTiers()`** — the function the scheduler itself consults — and carries the mountpoint, +which is what actually separates `/mnt/hdd_1` from `/var/lib/vz`. -**No new recovery path.** `AttachDrive`'s normalize leg already did the repair and three call sites already -invoked it (20 s ticker, agent startup, and the controller's `Return` branch **before** `restartStacks`). -All three died on `if n == 1 && GuestSeesMount(...)` logging *"fully live, no-op"* about a dead namespace. +**Unresolvable is recorded as unresolvable**, never defaulted: `namespace_state` and `backup_target.state` +are `resolved | unknown`, with distinct reasons (`agent_backup_config_unavailable`, `not_a_known_storage`, +`no_pbs_storage_observed`). An unconfigured namespace still reads `"root"` — that is an answer, not a gap. -**Ordering trap, caught by a test:** reading the abort flag before comparing devices classifies the real -return state as aborted (its stale bind carries `shutdown` too) and refuses the repair **while still -reporting correctly** — so the abort flag is read off the RAW mount in the stale case. +**The resolver reports the tier IN EFFECT, not the file on disk.** A backup-target move rewrites +`agent.json` and deliberately does not restart the agent (the E-1 lesson), so a disk re-read would name a +storage no archive had reached yet. -## Live validation, demo-hp +## 4. Files -| State | raw | bind | verdict | usable | -|---|---|---|---|---| -| healthy | `8:16` | `8:16` | `live` | true | -| **R-117 return** | `8:32` `/dev/sdc` | `8:16 …,shutdown` | **`stale-device`** | **false** | -| **Q7 in-place abort** | `252:11 …,emergency_ro` | `252:11 …,emergency_ro` | **`filesystem-aborted`** | **false** | +`internal/hub/report.go`, `internal/storage/observe.go`, `internal/hub/dr_recipe.go`, +`internal/hub/collect.go`, `cmd/felhom-agent/main.go`, `internal/hub/testdata/host-report.golden.json`, +`REUSE.md`; tests in `internal/hub/{dr_recipe,contract,report}_test.go`, `internal/pbs/live_reporter_test.go`. -340–497 µs per call. **No block I/O proven by `strace`**: only `/proc/self/mountinfo`, **0** `statfs`. -**No regression through the real pipeline:** `GET /disks` with the controller's own credential shows the -live backup-target drive `bound_under_parent=True`, with 32 gate lines in 3 min as the positive observable -and zero spurious transitions. +## 5. The cross-repo half that is easy to miss -## Tests + red-proofs +The hub's `AssembleDRRecipe` **allow-lists** top-level keys. `backup_target` would have been stored intact +and dropped before any operator saw it. The same mechanism already cost `offsite_restic` its whole +existence (**R-122**, found this session). `REUSE.md` now records this as a two-repo change. -14 new tests; the payload ones drive the real `/disks` handler and the `AttachDrive` ones record the actual -privileged calls. Fixtures are the spike's captured mountinfo, injected as **data** via -`procSelfMountinfo`/`procGuestMountinfo`, so the real parser, predicate, `AttachDrive` and handler all run. +## 6. Tests -**6 red-proofs, each verified to have landed.** RP1/RP2 (term 3 removed per site), RP3 (**the P1-only -fix**), RP4 (`emergency_ro` dropped), RP5 (unknown reported dead), RP6 (aborted arm falls through to a -re-bind). +9 new. Consequence-level: a box with two `content=backup` storages names the **live** one and not the +frozen one; `assertBackupCandidateAmbiguity` fails if the fixture ever stops posing that problem. Fixtures +are the storage set demo-felhom really had, provenance recorded in-file. Two tests run the real +`Collector.Collect()` path, one pinning that an **unwired seam yields unknown**. -**A hollow test was caught, by RP1 failing to fail.** The aborted fixture first used a `/dev/mapper` -device, for which `RoleForStorage` derives `role="system"` — a system row never enters the block that -computes `BoundUnderParent`, so the field stayed false **by default**, the assertion passed, and no mutation -could ever fail it. Fixtures now assert the production row shape before asserting the field. +**4 red-proofs**, each mutation asserted to have landed before running: revert R-106 → the live `"root"` +symptom reproduces; make unknown guess a default → 5 assertions fire; drop the field → the cross-repo +contract guard fires; hollow the fixture → the ambiguity guard fires. -## Not yet done / not covered +Suite: **rc=0, 29 packages ok, 0 FAIL** (run separately from the commit). -- **The stale-bind repair on hardware.** `StablePathForRaw` hardcodes the production - `/mnt/felhom-drives`, so exercising `AttachDrive` for a scratch drive writes a submount into guest - 9201's namespace, which the task fences. It happened once inadvertently, was reverted within a minute - and 9201 verified unaffected, and was deliberately not repeated. Filed **R-117h**. -- **Sustained-load behaviour of the predicate** — flagged unmeasured by the spike and **still unmeasured**. -- **felhom-pve not upgraded** (0.115.0). The fleet is non-uniform. -- **R-117g** — an aborted filesystem is never cleared automatically, by design, so it alarms until a human - acts; there is no guided recovery. -- **R-121** — a box's installed agent can sit releases behind the vouched one and nothing notices; the - R-120 gate is golden-vs-fleet-controller and cannot see it. +## 7. Not touched -## Observations (not acted on) - -Guest 9201 has 2 stacked entries at `/mnt/felhom-drives/nvme-1tb` while the host has 1 (pre-existing; -`AttachDrive` normalizes the host count only). 14 stale `felhom-agent.bak-*` binaries on demo-hp (~200 MB), -nothing prunes them. 11 files in this repo fail `gofmt -l`, all pre-existing and none in `internal/localapi/`. +The backup machinery. This corrects the record, not the doing — and the record was never acted on +programmatically: the `host_loss` plan is surface-only (`internal/dr/plan.go:125`), so the blast radius was +exactly one operator reading a wrong value. `plan.PBS` inherits the fix for free. diff --git a/REUSE.md b/REUSE.md index 607e698..775820d 100644 --- a/REUSE.md +++ b/REUSE.md @@ -174,6 +174,14 @@ - **New privileged host op**: validate args (internal/storage/validate.go style) → exec via `Runner` → add a `Cmnd_Alias` to configs/felhom-agent.sudoers → add a probe vector to internal/capability/manifest.go (so degradation is visible) → ship sudoers with the binary. - **New reconcile action**: `ActionKind` + `classOfAction` (internal/reconcile/classify.go), plan emission in internal/reconcile/plan.go; destructive ⇒ gate handles it automatically. - **Hub-report field**: extend `hub.HostReport` (internal/hub/report.go) + `Collector` — hub side must mirror + allowlist it (cross-repo). +- **DR-recipe section (host-half)**: add the field to `DRRecipeHostHalf` (internal/hub/dr_recipe.go) **AND** to + the hub's `hostHalfShape` + `AssembledRecipe` (felhom.eu `hub/internal/store/dr_recipe.go`). Those two + hub structs are **ALLOW-LISTS**: a section only the agent knows about is stored intact and silently + dropped before any operator sees it — that is R-122, which cost `offsite_restic` its entire existence. + Then update BOTH copies of `testdata/host-report.golden.json` (byte-identical, cross-repo) and extend + `TestAssembleDRRecipe_CarriesEveryEmittedSection`. **A recipe field that cannot be resolved records an + explicit unknown with a reason — never a default, an empty string, or a placeholder** (`DRState*` / + `DRReason*`); a recipe read during a rebuild must not present a guess as a fact. - **Envelope-driven behavior**: implement `hub.EnvelopeObserver`, add to the `MultiObserver` in cmd/felhom-agent/main.go. - **Selftest mode**: `selftestFlag` + `runSelftest*` in cmd/felhom-agent/main.go. - **Config**: internal/config/config.go (`Load` + `applyEnv` `FELHOM_AGENT_*` overlay; keep secrets out of `Redacted()` output). diff --git a/cmd/felhom-agent/main.go b/cmd/felhom-agent/main.go index 7d0baea..ff13cf4 100644 --- a/cmd/felhom-agent/main.go +++ b/cmd/felhom-agent/main.go @@ -462,6 +462,7 @@ func runDaemon(cfg config.Config, logger *slog.Logger, logRing *applog.Ring) int pbsTargets := pbsTargetsFromPVE(cfg, px, logger) pbsReporter := pbs.NewLiveSnapshotReporter(pbsTargets, pbsStore, pbs.DefaultLiveSnapshotTimeout, logger) collector := hub.NewCollector(px, hub.SystemctlProber{}, observer, backupStore, backupStore, pbsReporter, cfg.Hub.HostID, version, logger) + collector.SetBackupTargetResolver(primaryBackupTargetOf(cfg)) // R-109: the recipe names the live target // Privileged-capability self-check (v0.44.0): probe the sudoers grants the non-root agent // depends on. The probe runs `sudo -n -l` LITERALLY (a policy LIST, never executing the // command), so it uses a DIRECT runner regardless of the agent's privileged mode. Probe once at @@ -1238,6 +1239,30 @@ func readTrimmed(path string) (string, error) { return s, nil } +// primaryBackupTargetOf returns the resolver the DR recipe uses to name WHICH storage holds this box's +// local whole-guest archives (R-109). +// +// It reads the PRIMARY tier out of cfg.Backup.BackupTiers() rather than calling BackupTarget() directly. +// Both return the same string today — BackupTiers() builds tier 0 from BackupTarget() — but the tier +// list is the function the scheduler itself consults, so if primary-tier derivation ever changes the +// recipe follows it instead of quietly disagreeing with the backup. One state, one owner. +// +// cfg is captured BY VALUE on purpose: that is the daemon-start snapshot, which is the config actually +// in effect. See SetBackupTargetResolver for why re-reading agent.json here would be wrong. +func primaryBackupTargetOf(cfg config.Config) func() hub.ConfiguredBackupTarget { + return func() hub.ConfiguredBackupTarget { + tiers, _ := cfg.Backup.BackupTiers() // warnings are logged where the tiers are armed + for _, t := range tiers { + if t.Primary { + return hub.ConfiguredBackupTarget{StorageID: t.TargetID, Known: true} + } + } + // Unreachable with today's BackupTiers (tier 0 is always primary), and if that ever stops being + // true the recipe says "I could not tell" rather than picking a tier at random. + return hub.ConfiguredBackupTarget{} + } +} + // buildRestoreTestScheduler constructs the restore-test cadence scheduler from config. It // disables the cadence (returns a scheduler that just waits) when the cadence is off or the // scratch band / restore storage is invalid — a misconfig must not crash the daemon, and the @@ -1560,6 +1585,10 @@ func runSelftestHub(ctx context.Context, cfg config.Config, logger *slog.Logger) // the selftest reflects exactly what a freshly-restarted daemon's first collect emits. pbsReporter := pbs.NewLiveSnapshotReporter(pbsTargetsFromPVE(cfg, px, logger), pbs.NewSnapshotStore(), pbs.DefaultLiveSnapshotTimeout, logger) collector := hub.NewCollector(px, hub.SystemctlProber{}, observer, nil, nil, pbsReporter, cfg.Hub.HostID, version, logger) + // R-109: wire the backup-target resolver here TOO. Without it selftest=hub would print a recipe whose + // backup_target reads unknown/agent_backup_config_unavailable while the daemon's is resolved — and + // this one-shot exists precisely so "the report it would send" can be trusted to match. + collector.SetBackupTargetResolver(primaryBackupTargetOf(cfg)) ctx, cancel := context.WithTimeout(ctx, 60*time.Second) defer cancel() diff --git a/internal/hub/collect.go b/internal/hub/collect.go index 516bd46..10bfcf5 100644 --- a/internal/hub/collect.go +++ b/internal/hub/collect.go @@ -89,6 +89,7 @@ type Collector struct { selfUpdate SelfUpdateReporter // D1: agent self-update pending status (nil → false) mgmtPlane MgmtPlaneReporter // G1: management-plane health (nil → stanza omitted) oob OOBReporter // H1: operator-access health (nil → stanza omitted) + backupTarget func() ConfiguredBackupTarget // R-109: primary backup tier id (nil → recipe records unknown) hostID string agentVersion string logger *slog.Logger @@ -123,6 +124,30 @@ func (c *Collector) SetTempReader(t TempReader) *Collector { return c } +// SetBackupTargetResolver wires the DR recipe to the agent's own backup config (R-109), so the recipe +// can name WHICH storage holds the local whole-guest archives. Returns the collector for chaining. +// +// The resolver MUST report the tier that is IN EFFECT, which is the daemon-start snapshot — NOT the +// current contents of agent.json. A backup-target move rewrites that file and deliberately does not +// restart the agent (the E-1 lesson: restarting mid-backup records a spurious failure for a run that +// succeeded), so between the write and the restart the file names a target no backup is writing to yet. +// Re-reading the file here — the live-reload shape used for escrow.pbs_storage_id — would make the +// recipe point at the new storage while every archive still landed on the old one. One state, one +// owner: the recipe follows what performs the backup. +func (c *Collector) SetBackupTargetResolver(f func() ConfiguredBackupTarget) *Collector { + c.backupTarget = f + return c +} + +// configuredBackupTarget consults the resolver. An unwired seam is reported as NOT KNOWN — never as a +// guess — so the recipe records an explicit unknown instead of a target the agent never verified. +func (c *Collector) configuredBackupTarget() ConfiguredBackupTarget { + if c.backupTarget == nil { + return ConfiguredBackupTarget{} + } + return c.backupTarget() +} + // SetCapabilityProber wires the privileged-capability self-check (v0.44.0): each collect runs it // and attaches the snapshot. nil → the report carries an empty []. Returns the collector for chaining. func (c *Collector) SetCapabilityProber(probe func(ctx context.Context) []capability.Status) *Collector { @@ -231,7 +256,7 @@ func (c *Collector) Collect(ctx context.Context) (*HostReport, error) { } // DR recipe host-half — derived from the just-collected guest/storage/PBS facts (no new reads). // Secret-free by construction (identifiers/intents/sizes/coordinates only). - report.DRRecipe = BuildDRRecipeHostHalf(report.Guests, report.StorageTargets, report.PBSSnapshots) + report.DRRecipe = BuildDRRecipeHostHalf(report.Guests, report.StorageTargets, report.PBSSnapshots, c.configuredBackupTarget()) // S3: offsite-tunnel status stanza (nil reporter = feature disabled → omitted; the pubkey in // it is the operator's revocation-recovery handle). if c.wg != nil { diff --git a/internal/hub/contract_test.go b/internal/hub/contract_test.go index 27c9405..1acddea 100644 --- a/internal/hub/contract_test.go +++ b/internal/hub/contract_test.go @@ -45,6 +45,14 @@ func TestHostReport_ContractMatchesGolden(t *testing.T) { State: StorageStateAttached, Reachable: true, MountPath: "/mnt/usb-backup", TotalBytes: 2000000000000, Smart: SmartSummary{Health: SmartUnknown}, }, + // A pbs target so the recipe's pbs coord has a storage.cfg row to resolve its namespace + // from (R-106). Without one the fixture produces namespace_state=unknown while the golden + // pins a resolved coord — key-set-equal but semantically a fiction. + { + Name: "felhom-pbs", Type: StorageTypePBS, DurableID: "repo+fp", Content: "backup", + State: StorageStateAttached, Reachable: true, PBSNamespace: "felhom-spike", + Smart: SmartSummary{Health: SmartUnknown}, + }, }, Backups: []Backup{ { @@ -74,7 +82,8 @@ func TestHostReport_ContractMatchesGolden(t *testing.T) { Cloudflared: Cloudflared{Status: "active"}, } // dr_recipe host-half: built from the same guest/storage/pbs facts (the production path). - report.DRRecipe = BuildDRRecipeHostHalf(report.Guests, report.StorageTargets, report.PBSSnapshots) + report.DRRecipe = BuildDRRecipeHostHalf(report.Guests, report.StorageTargets, report.PBSSnapshots, + ConfiguredBackupTarget{StorageID: "usb-backup", Known: true}) b, _ := json.Marshal(report) var got map[string]any json.Unmarshal(b, &got) @@ -106,6 +115,7 @@ func TestHostReport_ContractMatchesGolden(t *testing.T) { assertSameKeys(t, "dr_recipe.guests[0]", firstElem(field(grec, "guests")), firstElem(field(srec, "guests"))) assertSameKeys(t, "dr_recipe.drives[0]", firstElem(field(grec, "drives")), firstElem(field(srec, "drives"))) assertSameKeys(t, "dr_recipe.pve_storage[0]", firstElem(field(grec, "pve_storage")), firstElem(field(srec, "pve_storage"))) + assertSameKeys(t, "dr_recipe.backup_target", field(grec, "backup_target"), field(srec, "backup_target")) } // field extracts a nested object value from a decoded JSON map (nil if absent/not a map). diff --git a/internal/hub/dr_recipe.go b/internal/hub/dr_recipe.go index ede08e6..398365a 100644 --- a/internal/hub/dr_recipe.go +++ b/internal/hub/dr_recipe.go @@ -30,6 +30,36 @@ import "sort" // ignore-unknown (encoding/json default) for forward-compat, mirroring storage_manifest. const DRRecipeVersion = 1 +// Recipe field states (R-106/R-109). A recipe is read at the worst possible moment — by an operator +// rebuilding a machine that is gone — so a field the agent cannot resolve must SAY SO rather than emit +// a default, an empty string, or a plausible-looking placeholder. A guess read as fact costs more than +// an admitted gap: it sends the restore at the wrong archive and nothing contradicts it. This is the +// same cannot-tell-must-not-lie rule R-117 needed a third state for. +const ( + DRStateResolved = "resolved" + DRStateUnknown = "unknown" +) + +// Reasons a resolved-value field is unknown. Enum-shaped, never free text, so the wire stays pinnable +// and TestDRRecipeHostHalf_NoSecrets has a fixed vocabulary to walk. +const ( + // DRReasonNoBackupConfig: the collector was built without a backup-config seam, so the agent could + // not consult the very config its own scheduler reads. Nothing is guessed. + DRReasonNoBackupConfig = "agent_backup_config_unavailable" + // DRReasonNoSuchStorage: the configured target id matches no storage this host observes. The id is + // still recorded (it IS what the config says) and the state says it could not be corroborated. + DRReasonNoSuchStorage = "not_a_known_storage" + // DRReasonNoPBSStorage: snapshots exist but no pbs storage was observed, so there is no storage.cfg + // row to read the namespace from. + DRReasonNoPBSStorage = "no_pbs_storage_observed" +) + +// PBSRootNamespace is how the recipe spells PBS's root namespace. The PBS API spells it as the EMPTY +// string (and `pct restore --ns root` would name a namespace that does not exist) — "root" is a display +// convention this wire has always used, kept here so the field's meaning did not change under R-106. +// Only a box with no `namespace` line in its pbs storage.cfg stanza ever emits it. +const PBSRootNamespace = "root" + // DRRecipeHostHalf is the agent-emitted half (guest/drive/storage/PBS scaffolding). Derived entirely // from facts the report already collects — no new privileged reads. type DRRecipeHostHalf struct { @@ -38,6 +68,45 @@ type DRRecipeHostHalf struct { PBS *DRPBSCoord `json:"pbs,omitempty"` Drives []DRDrive `json:"drives"` PVEStorage []DRPVEStorage `json:"pve_storage"` + // BackupTarget names WHICH storage holds the local whole-guest archives (R-109). Always present — + // its own State field carries "I could not tell", so the section is never simply absent. + BackupTarget *DRBackupTarget `json:"backup_target"` +} + +// DRBackupTarget answers the one question pve_storage cannot: of every storage listed there, WHICH one +// does this box's primary backup tier actually write its whole-guest archives to? +// +// Before R-109 the recipe listed each storage's name/type/content and said nothing about the target. +// That was harmless while the target was the well-known `local`; the 2026-07-28 vzdump-target move +// ended that. Every box now carries TWO content=backup dir storages — `felhom-backup` (live) and +// `local` (archives frozen at the move, never refreshed since) — and they are indistinguishable by +// name, type and content alone. A restorer picking the frozen one gets a guest that restores cleanly +// and is silently months out of date, which is the worst shape a backup defect can take. +type DRBackupTarget struct { + // State is DRStateResolved | DRStateUnknown. A reader MUST consult it before trusting StorageID: + // the id is also recorded in one unknown case (see DRReasonNoSuchStorage). + State string `json:"state"` + // StorageID is the PVE storage id of the PRIMARY backup tier. Empty only when the config could not + // be consulted at all. + StorageID string `json:"storage_id,omitempty"` + // MountPath is where that storage's archives land on the host — the disambiguation a restorer + // actually needs, since it is what separates felhom-backup's /mnt/hdd_1 from local's /var/lib/vz. + // "" for a pbs target (no host mount) and when unresolved. + MountPath string `json:"mount_path,omitempty"` + // Reason is why State is unknown (one of the DRReason* constants); "" when resolved. + Reason string `json:"reason,omitempty"` +} + +// ConfiguredBackupTarget is what the agent's own backup config says the PRIMARY tier writes to. +// +// Known=false is a REAL state, not a nil-guard: it means the collector was constructed without the +// backup-config seam (the --selftest one-shots did exactly this before v0.118.0), and the recipe then +// records unknown instead of inventing a target. Deliberately a struct rather than a `(string, bool)` +// return — the (value, ok) shape is what made "errors degrade to unknown, never to no-backup" +// unimplementable in newestArchiveOn, and this field has the same three-way reading. +type ConfiguredBackupTarget struct { + StorageID string + Known bool } // DRGuest is the sizing needed to recreate the LXC at the right size (GuestSpec, already on the wire). @@ -51,8 +120,21 @@ type DRGuest struct { // DRPBSCoord is WHERE the whole-CT snapshot lives — COORDINATES ONLY. The encryption key is escrow-only; // the access token is identity-escrow-only. Neither is here. type DRPBSCoord struct { - RepoID string `json:"repo_id"` // the PVE pbs storage id (e.g. "felhom-pbs") — not a token - Namespace string `json:"namespace"` // PBS namespace the restore targets + RepoID string `json:"repo_id"` // the PVE pbs storage id (e.g. "felhom-pbs") — not a token + // Namespace is the PBS namespace the restore targets, resolved from the pbs storage's storage.cfg + // stanza — the same field `vzdump --storage ` makes PVE read, so the recipe cannot disagree + // with the backup that produced the snapshot. PBSRootNamespace when the box has no namespace + // configured; "" when NamespaceState is unknown. + // + // R-106: this used to come from the listed snapshot's own `ns`, which PBS does not echo per item once + // the request is already namespace-scoped via `?ns=` (internal/pbs/client.go). The field was + // therefore always empty, ToHub normalised empty → "root", and every per-customer box reported the + // root namespace while its backups were really in `demo-hp` / `demo-felhom`. + Namespace string `json:"namespace"` + // NamespaceState is DRStateResolved | DRStateUnknown — consult it before trusting Namespace. + NamespaceState string `json:"namespace_state"` + // NamespaceReason is why NamespaceState is unknown; "" when resolved. + NamespaceReason string `json:"namespace_reason,omitempty"` LatestSnapshotID string `json:"latest_snapshot_id"` // most-recent snapshot's backup_id (a coordinate) } @@ -83,7 +165,7 @@ const driveIntentEnrolled = "enrolled" // it is unit-tested directly (no live reads). drives[] = the user-data external drives (usb/local-dir // with a durable-id); pve_storage[] = every storage target (the storage.cfg scaffolding); pbs = the // latest PBS snapshot's coordinates; guests[] = each guest's sizing (skip guests with no spec). -func BuildDRRecipeHostHalf(guests []Guest, targets []StorageTarget, pbs []PBSSnapshot) *DRRecipeHostHalf { +func BuildDRRecipeHostHalf(guests []Guest, targets []StorageTarget, pbs []PBSSnapshot, backupTarget ConfiguredBackupTarget) *DRRecipeHostHalf { h := &DRRecipeHostHalf{ RecipeVersion: DRRecipeVersion, Guests: []DRGuest{}, @@ -103,11 +185,14 @@ func BuildDRRecipeHostHalf(guests []Guest, targets []StorageTarget, pbs []PBSSna }) } - var pbsRepoID string + var pbsRepoID, pbsNamespace string + var pbsStorageFound bool for _, t := range targets { h.PVEStorage = append(h.PVEStorage, DRPVEStorage{Name: t.Name, Type: t.Type, Content: t.Content}) - if t.Type == StorageTypePBS && pbsRepoID == "" { - pbsRepoID = t.Name // the pbs storage id is a coordinate, not the key + if t.Type == StorageTypePBS && !pbsStorageFound { + pbsStorageFound = true + pbsRepoID = t.Name // the pbs storage id is a coordinate, not the key + pbsNamespace = t.PBSNamespace // storage.cfg's namespace — "" here means the ROOT namespace } if isUserDataDrive(t) { h.Drives = append(h.Drives, DRDrive{ @@ -119,12 +204,41 @@ func BuildDRRecipeHostHalf(guests []Guest, targets []StorageTarget, pbs []PBSSna } } - if c := latestPBSCoord(pbs, pbsRepoID); c != nil { + h.BackupTarget = resolveBackupTarget(targets, backupTarget) + + if c := latestPBSCoord(pbs, pbsRepoID, pbsNamespace, pbsStorageFound); c != nil { h.PBS = c } return h } +// resolveBackupTarget records WHICH storage the primary backup tier writes to (R-109), or records +// explicitly that it could not tell. Three outcomes, and the two unknowns are deliberately distinct — +// "I could not read my own config" and "my config names a storage that is not here" send an operator +// to different places. +// +// MountPath prefers the live mount and falls back to the CONFIGURED path: during a rebuild the drive is +// frequently absent, and when it is, MountPath empties out while ConfigPath is the only thing left that +// still says which drive the row was about (the R-116 lesson). The storage's absence from the host is a +// separate signal (E-2's backup_target_absent); it does not make the recipe's answer unknown, because +// the question here is which storage.cfg row to restore FROM, and that is still known. +func resolveBackupTarget(targets []StorageTarget, cfg ConfiguredBackupTarget) *DRBackupTarget { + if !cfg.Known || cfg.StorageID == "" { + return &DRBackupTarget{State: DRStateUnknown, Reason: DRReasonNoBackupConfig} + } + for _, t := range targets { + if t.Name != cfg.StorageID { + continue + } + mount := t.MountPath + if mount == "" { + mount = t.ConfigPath + } + return &DRBackupTarget{State: DRStateResolved, StorageID: cfg.StorageID, MountPath: mount} + } + return &DRBackupTarget{State: DRStateUnknown, StorageID: cfg.StorageID, Reason: DRReasonNoSuchStorage} +} + // isUserDataDrive selects the external user-data drives the recipe enumerates (felhom-usb / felhom-flash // class): a usb or local-dir storage with a filesystem-UUID durable id and a host mount path. local / // lvmthin / pbs / nfs / cifs are scaffolding (they land in pve_storage) but are not user-data drives. @@ -137,16 +251,30 @@ func isUserDataDrive(t StorageTarget) bool { // latestPBSCoord picks the most-recent snapshot (lexical max of the RFC3339 backup_time) and returns // its coordinates. Returns nil when there is no snapshot to target. -func latestPBSCoord(snaps []PBSSnapshot, repoID string) *DRPBSCoord { +// +// The namespace comes from the pbs STORAGE (storage.cfg), never from the snapshot — see DRPBSCoord's +// Namespace comment for why the snapshot's own field cannot answer it (R-106). storageFound=false with +// snapshots present is a genuine unknown: something listed snapshots, but there is no storage row to +// read a namespace from, so the recipe says so rather than defaulting to root. +func latestPBSCoord(snaps []PBSSnapshot, repoID, namespace string, storageFound bool) *DRPBSCoord { if len(snaps) == 0 { return nil } sorted := append([]PBSSnapshot(nil), snaps...) sort.Slice(sorted, func(i, j int) bool { return sorted[i].BackupTime > sorted[j].BackupTime }) latest := sorted[0] - return &DRPBSCoord{ + c := &DRPBSCoord{ RepoID: repoID, - Namespace: latest.Namespace, LatestSnapshotID: latest.BackupID, + NamespaceState: DRStateUnknown, + NamespaceReason: DRReasonNoPBSStorage, } + if storageFound { + c.NamespaceState, c.NamespaceReason = DRStateResolved, "" + // An empty configured namespace is not a missing answer — it IS the root namespace. + if c.Namespace = namespace; c.Namespace == "" { + c.Namespace = PBSRootNamespace + } + } + return c } diff --git a/internal/hub/dr_recipe_test.go b/internal/hub/dr_recipe_test.go index 33176c8..574349c 100644 --- a/internal/hub/dr_recipe_test.go +++ b/internal/hub/dr_recipe_test.go @@ -1,8 +1,10 @@ package hub import ( + "context" "encoding/json" "regexp" + "strings" "testing" ) @@ -30,7 +32,7 @@ func TestBuildDRRecipeHostHalf(t *testing.T) { {Namespace: "root", BackupID: "9201", BackupTime: "2026-06-16T08:00:00Z"}, // latest } - h := BuildDRRecipeHostHalf(guests, targets, pbs) + h := BuildDRRecipeHostHalf(guests, targets, pbs, ConfiguredBackupTarget{StorageID: "felhom-flash", Known: true}) if h.RecipeVersion != 1 { t.Errorf("recipe_version=%d, want 1", h.RecipeVersion) @@ -68,7 +70,8 @@ func TestBuildDRRecipeHostHalf(t *testing.T) { // TestBuildDRRecipeHostHalf_NoPBS: no snapshots → pbs omitted (nil), no panic. func TestBuildDRRecipeHostHalf_NoPBS(t *testing.T) { - h := BuildDRRecipeHostHalf(nil, []StorageTarget{{Name: "local", Type: StorageTypeLocal}}, nil) + h := BuildDRRecipeHostHalf(nil, []StorageTarget{{Name: "local", Type: StorageTypeLocal}}, nil, + ConfiguredBackupTarget{StorageID: "local", Known: true}) if h.PBS != nil { t.Errorf("pbs should be nil with no snapshots, got %+v", h.PBS) } @@ -89,6 +92,7 @@ func TestDRRecipeHostHalf_V1DriveShape(t *testing.T) { MountPath: "/mnt/felhom-usb", TotalBytes: 931 << 30}, }, nil, + ConfiguredBackupTarget{StorageID: "felhom-usb", Known: true}, ) if len(h.Drives) != 1 { t.Fatalf("want 1 drive, got %d", len(h.Drives)) @@ -124,6 +128,7 @@ func TestDRRecipeHostHalf_NoSecrets(t *testing.T) { {Name: "felhom-usb", Type: StorageTypeUSB, DurableID: "uuid:da9e7089", Role: "bulk-data", MountPath: "/mnt/felhom-usb", TotalBytes: 1}, }, []PBSSnapshot{{Namespace: "root", BackupID: "9201", BackupTime: "2026-06-16T08:00:00Z"}}, + ConfiguredBackupTarget{StorageID: "felhom-usb", Known: true}, ) b, err := json.Marshal(h) if err != nil { @@ -132,6 +137,294 @@ func TestDRRecipeHostHalf_NoSecrets(t *testing.T) { assertNoSecretKeys(t, b) } +// --------------------------------------------------------------------------------------------- +// R-106 / R-109 — the recipe records the RESOLVED backup target and the REAL PBS namespace. +// --------------------------------------------------------------------------------------------- + +// capturedDemoFelhomTargets is the storage set demo-felhom really had on 2026-07-30, not an invented +// one. PROVENANCE — every field was captured, none composed: +// +// - names/types/contents: the pve_storage block of the box's own PRE-FIX recipe, downloaded from the +// hub at GET /customers/demo-felhom/dr-recipe.json (agent v0.115.0). +// - paths + is_mountpoint + the pbs namespace: `cat /etc/pve/storage.cfg` on felhom-pve, same day — +// `dir: local path /var/lib/vz`, `dir: felhom-backup path /mnt/hdd_1 is_mountpoint 1`, +// `pbs: felhom-pbs ... namespace demo-felhom`. +// +// THE AMBIGUITY THIS PINS IS REAL, and assertBackupCandidateAmbiguity below refuses to let the fixture +// quietly lose it: `local` and `felhom-backup` BOTH carry content=backup, and since the 2026-07-28 +// vzdump-target move `local` holds archives frozen at that date. Naming the wrong one restores a guest +// that is silently months stale. +func capturedDemoFelhomTargets() []StorageTarget { + return []StorageTarget{ + {Name: "local-lvm", Type: StorageTypeLVMThin, DurableID: "pve/data", Content: "images,rootdir"}, + { + Name: "felhom-backup", Type: StorageTypeLocalDir, Content: "backup", + DurableID: "uuid:47a3361a-91e0-4831-a69d-27f540ed3f48", + MountPath: "/mnt/hdd_1", ConfigPath: "/mnt/hdd_1", TotalBytes: 983351140352, + }, + { + Name: "felhom-pbs", Type: StorageTypePBS, Content: "backup", + DurableID: "repo+fp", PBSNamespace: "demo-felhom", + }, + // The decoy: same content, plausible name, historically THE vzdump target. ConfigPath only — + // `local` lives on the LVM root and is not its own mount, so the observer leaves MountPath empty. + {Name: "local", Type: StorageTypeLocal, Content: "backup,import,vztmpl,iso", ConfigPath: "/var/lib/vz"}, + } +} + +// capturedDemoFelhomSnapshots mirrors what the box's pre-fix recipe carried: latest_snapshot_id "9201". +// Namespace is deliberately EMPTY on every element — that is exactly what the PBS API returns once the +// list is namespace-scoped via `?ns=`, and it is the input that used to become the bogus "root". +func capturedDemoFelhomSnapshots() []PBSSnapshot { + return []PBSSnapshot{ + {Namespace: "", BackupID: "9201", BackupTime: "2026-07-29T22:00:00Z"}, + {Namespace: "", BackupID: "9201", BackupTime: "2026-07-30T22:00:00Z"}, // latest + } +} + +// assertBackupCandidateAmbiguity fails if the fixture stopped containing TWO plausible content=backup +// storages. Without this the consequence test below could pass on a fixture with only one candidate — +// which is precisely the hollow shape that let two defects ship green earlier in this arc. +func assertBackupCandidateAmbiguity(t *testing.T, h *DRRecipeHostHalf) { + t.Helper() + var candidates []string + for _, s := range h.PVEStorage { + if strings.Contains(s.Content, "backup") && (s.Type == StorageTypeLocalDir || s.Type == StorageTypeLocal) { + candidates = append(candidates, s.Name) + } + } + if len(candidates) < 2 { + t.Fatalf("fixture no longer poses the R-109 problem: want >=2 content=backup dir storages, got %v", candidates) + } +} + +// TestDRRecipe_BackupTargetNamesTheLiveStorage is THE consequence assertion for R-109: given a box that +// really carries two content=backup dir storages, the generated recipe names the LIVE one, gives its +// mountpoint, and does not name the frozen one. Not "the function returned a non-empty string". +func TestDRRecipe_BackupTargetNamesTheLiveStorage(t *testing.T) { + h := BuildDRRecipeHostHalf(nil, capturedDemoFelhomTargets(), capturedDemoFelhomSnapshots(), + ConfiguredBackupTarget{StorageID: "felhom-backup", Known: true}) + + assertBackupCandidateAmbiguity(t, h) + + bt := h.BackupTarget + if bt == nil { + t.Fatal("backup_target is absent — the recipe still cannot say where the local archives are (R-109)") + } + if bt.State != DRStateResolved { + t.Errorf("state=%q want %q (reason=%q)", bt.State, DRStateResolved, bt.Reason) + } + if bt.StorageID != "felhom-backup" { + t.Errorf("storage_id=%q — the recipe must name the LIVE target, not %q", bt.StorageID, "felhom-backup") + } + if bt.MountPath != "/mnt/hdd_1" { + t.Errorf("mount_path=%q want /mnt/hdd_1 — the mountpoint is what separates it from local's /var/lib/vz", bt.MountPath) + } + // Unambiguous: the frozen decoy must not be what the field names. + if bt.StorageID == "local" || bt.MountPath == "/var/lib/vz" { + t.Errorf("recipe names the FROZEN target (%q at %q) — a restore from it is silently stale", bt.StorageID, bt.MountPath) + } +} + +// TestDRRecipe_PBSNamespaceIsThePerCustomerOne is the consequence assertion for R-106: the recipe carries +// the namespace the box's backups actually live in, resolved from storage.cfg, and specifically NOT the +// "root" that every box used to report. +func TestDRRecipe_PBSNamespaceIsThePerCustomerOne(t *testing.T) { + h := BuildDRRecipeHostHalf(nil, capturedDemoFelhomTargets(), capturedDemoFelhomSnapshots(), + ConfiguredBackupTarget{StorageID: "felhom-backup", Known: true}) + + if h.PBS == nil { + t.Fatal("pbs coord absent with snapshots present") + } + if h.PBS.Namespace == PBSRootNamespace { + t.Errorf("namespace=%q — this is the R-106 symptom: the snapshot's empty ns normalised to root "+ + "while the box's backups are in demo-felhom", h.PBS.Namespace) + } + if h.PBS.Namespace != "demo-felhom" { + t.Errorf("namespace=%q want demo-felhom (storage.cfg's `namespace` on the pbs storage)", h.PBS.Namespace) + } + if h.PBS.NamespaceState != DRStateResolved { + t.Errorf("namespace_state=%q want %q (reason=%q)", h.PBS.NamespaceState, DRStateResolved, h.PBS.NamespaceReason) + } + if h.PBS.RepoID != "felhom-pbs" || h.PBS.LatestSnapshotID != "9201" { + t.Errorf("coord drifted: repo=%q snapshot=%q", h.PBS.RepoID, h.PBS.LatestSnapshotID) + } +} + +// TestDRRecipe_PBSNamespaceRootIsResolvedNotUnknown: a box with a pbs storage and NO namespace line is +// genuinely in the root namespace. That is an answer, not a gap — it must read resolved/"root", so the +// honest root case is never confused with "I could not tell". +func TestDRRecipe_PBSNamespaceRootIsResolvedNotUnknown(t *testing.T) { + h := BuildDRRecipeHostHalf(nil, + []StorageTarget{{Name: "felhom-pbs", Type: StorageTypePBS, Content: "backup", PBSNamespace: ""}}, + capturedDemoFelhomSnapshots(), + ConfiguredBackupTarget{StorageID: "felhom-pbs", Known: true}) + + if h.PBS.NamespaceState != DRStateResolved { + t.Errorf("namespace_state=%q — an unconfigured namespace IS the root namespace, not an unknown", h.PBS.NamespaceState) + } + if h.PBS.Namespace != PBSRootNamespace { + t.Errorf("namespace=%q want %q", h.PBS.Namespace, PBSRootNamespace) + } +} + +// TestDRRecipe_BackupTargetUnknownWhenConfigUnavailable is the WRONG case: the agent could not consult +// its own backup config. The recipe must say so explicitly and emit NO storage_id key at all — an +// absent value must not be representable as a plausible-looking answer. +func TestDRRecipe_BackupTargetUnknownWhenConfigUnavailable(t *testing.T) { + h := BuildDRRecipeHostHalf(nil, capturedDemoFelhomTargets(), nil, ConfiguredBackupTarget{}) + + bt := h.BackupTarget + if bt == nil { + t.Fatal("backup_target must be PRESENT and say unknown, not vanish") + } + if bt.State != DRStateUnknown || bt.Reason != DRReasonNoBackupConfig { + t.Errorf("state=%q reason=%q want %q/%q", bt.State, bt.Reason, DRStateUnknown, DRReasonNoBackupConfig) + } + // Absence recorded as absence: no id, and no id KEY on the wire. + if bt.StorageID != "" { + t.Errorf("storage_id=%q — an unresolvable target must not be filled in", bt.StorageID) + } + b, err := json.Marshal(bt) + if err != nil { + t.Fatal(err) + } + var keys map[string]json.RawMessage + if err := json.Unmarshal(b, &keys); err != nil { + t.Fatal(err) + } + for _, banned := range []string{"storage_id", "mount_path"} { + if _, ok := keys[banned]; ok { + t.Errorf("unknown backup_target must not carry a %q key; got %s", banned, b) + } + } + // And nothing in it may read as one of the real candidates. + for _, decoy := range []string{"felhom-backup", "local", "/var/lib/vz", "/mnt/hdd_1"} { + if strings.Contains(string(b), decoy) { + t.Errorf("unknown backup_target leaked a plausible value %q: %s", decoy, b) + } + } +} + +// TestDRRecipe_BackupTargetUnknownWhenStorageMissing: the config names a storage this host does not +// have. That is unknown for a DIFFERENT reason — and the configured id IS still recorded, because +// "config says felhom-backup, no such storage here" sends an operator somewhere useful while silence +// does not. +func TestDRRecipe_BackupTargetUnknownWhenStorageMissing(t *testing.T) { + targets := []StorageTarget{{Name: "local", Type: StorageTypeLocal, Content: "backup", ConfigPath: "/var/lib/vz"}} + h := BuildDRRecipeHostHalf(nil, targets, nil, ConfiguredBackupTarget{StorageID: "felhom-backup", Known: true}) + + bt := h.BackupTarget + if bt.State != DRStateUnknown || bt.Reason != DRReasonNoSuchStorage { + t.Errorf("state=%q reason=%q want %q/%q", bt.State, bt.Reason, DRStateUnknown, DRReasonNoSuchStorage) + } + if bt.StorageID != "felhom-backup" { + t.Errorf("storage_id=%q want the CONFIGURED id recorded even though it matched nothing", bt.StorageID) + } + // It must NOT silently fall back to the only content=backup storage present. + if bt.StorageID == "local" || bt.MountPath == "/var/lib/vz" { + t.Error("resolution fell back to the wrong storage instead of reporting unknown") + } +} + +// TestDRRecipe_PBSNamespaceUnknownWithoutPBSStorage: snapshots exist but no pbs storage was observed, so +// there is no storage.cfg row to read a namespace from. The recipe must NOT default to root — that +// default is the entire R-106 defect. +func TestDRRecipe_PBSNamespaceUnknownWithoutPBSStorage(t *testing.T) { + h := BuildDRRecipeHostHalf(nil, + []StorageTarget{{Name: "local", Type: StorageTypeLocal, Content: "backup"}}, + capturedDemoFelhomSnapshots(), + ConfiguredBackupTarget{StorageID: "local", Known: true}) + + if h.PBS == nil { + t.Fatal("pbs coord should still be emitted (the snapshot id is a real coordinate)") + } + if h.PBS.NamespaceState != DRStateUnknown || h.PBS.NamespaceReason != DRReasonNoPBSStorage { + t.Errorf("namespace_state=%q reason=%q want %q/%q", + h.PBS.NamespaceState, h.PBS.NamespaceReason, DRStateUnknown, DRReasonNoPBSStorage) + } + if h.PBS.Namespace != "" { + t.Errorf("namespace=%q — with no storage row to read, the field must be empty, never %q", + h.PBS.Namespace, PBSRootNamespace) + } +} + +// TestDRRecipe_BackupTargetUsesConfigPathWhenDeviceGone is the DR-shaped case: the recipe is read while +// the target drive is absent, so MountPath has emptied out. ConfigPath is then the only thing that still +// says where the archives live (the R-116 lesson) — and the target is still RESOLVED, because which +// storage.cfg row to restore from is known regardless of whether its device is currently present. +func TestDRRecipe_BackupTargetUsesConfigPathWhenDeviceGone(t *testing.T) { + targets := []StorageTarget{{ + Name: "felhom-backup", Type: StorageTypeLocalDir, Content: "backup", + MountPath: "", ConfigPath: "/mnt/hdd_1", // device gone: observer empties MountPath, keeps ConfigPath + }} + h := BuildDRRecipeHostHalf(nil, targets, nil, ConfiguredBackupTarget{StorageID: "felhom-backup", Known: true}) + + bt := h.BackupTarget + if bt.State != DRStateResolved { + t.Errorf("state=%q — an absent device does not make the TARGET unknown", bt.State) + } + if bt.MountPath != "/mnt/hdd_1" { + t.Errorf("mount_path=%q want the configured path /mnt/hdd_1", bt.MountPath) + } +} + +// fakePBSReporter is a PBSReporter returning fixed snapshots (the verify loop's seam). +type fakePBSReporter struct{ snaps []PBSSnapshot } + +func (f fakePBSReporter) PBSSnapshots(context.Context) []PBSSnapshot { return f.snaps } + +// TestCollectDRRecipe_ProductionPath runs the REAL generation path — Collector.Collect(), the method the +// daemon calls every cycle — rather than BuildDRRecipeHostHalf directly. It is here because both defects +// this file fixes were invisible to a direct-call test: the namespace one lived in what the observer put +// on StorageTarget, and the target one lived in whether anything wired the config seam at all. A seam +// that is correct and never wired is the failure mode this repo has hit four times. +func TestCollectDRRecipe_ProductionPath(t *testing.T) { + px := &fakePx{node: "n", ns: newTestNodeStatus()} + obs := fakeObserver{targets: capturedDemoFelhomTargets()} + pbsRep := fakePBSReporter{snaps: capturedDemoFelhomSnapshots()} + + c := NewCollector(px, fakeProber{status: "active"}, obs, nil, nil, pbsRep, "h", "0.118.0", quietLogger()) + c.SetBackupTargetResolver(func() ConfiguredBackupTarget { + return ConfiguredBackupTarget{StorageID: "felhom-backup", Known: true} + }) + + r, err := c.Collect(context.Background()) + if err != nil { + t.Fatalf("Collect: %v", err) + } + if r.DRRecipe == nil { + t.Fatal("collect produced no dr_recipe") + } + if bt := r.DRRecipe.BackupTarget; bt == nil || bt.State != DRStateResolved || bt.StorageID != "felhom-backup" { + t.Errorf("backup_target through Collect = %+v, want resolved/felhom-backup", bt) + } + if p := r.DRRecipe.PBS; p == nil || p.Namespace != "demo-felhom" || p.NamespaceState != DRStateResolved { + t.Errorf("pbs namespace through Collect = %+v, want demo-felhom/resolved", p) + } +} + +// TestCollectDRRecipe_UnwiredSeamReportsUnknown: a Collector built WITHOUT the resolver (every +// --selftest one-shot did exactly this before v0.118.0) must produce an explicit unknown. This is the +// test that would have caught shipping the seam without wiring it. +func TestCollectDRRecipe_UnwiredSeamReportsUnknown(t *testing.T) { + px := &fakePx{node: "n", ns: newTestNodeStatus()} + c := NewCollector(px, fakeProber{status: "active"}, fakeObserver{targets: capturedDemoFelhomTargets()}, + nil, nil, nil, "h", "0.118.0", quietLogger()) + + r, err := c.Collect(context.Background()) + if err != nil { + t.Fatalf("Collect: %v", err) + } + bt := r.DRRecipe.BackupTarget + if bt == nil || bt.State != DRStateUnknown || bt.Reason != DRReasonNoBackupConfig { + t.Fatalf("unwired resolver must yield unknown/%s, got %+v", DRReasonNoBackupConfig, bt) + } + if bt.StorageID != "" { + t.Errorf("unwired resolver invented a target %q", bt.StorageID) + } +} + // assertNoSecretKeys walks decoded JSON and fails on any object key matching secretNameRe. Shared by // the agent boundary assertions. (durable_id/repo_id/latest_snapshot_id are identifiers/coordinates — // none match the credential regex.) diff --git a/internal/hub/report.go b/internal/hub/report.go index 65ca10a..1938bb8 100644 --- a/internal/hub/report.go +++ b/internal/hub/report.go @@ -290,6 +290,20 @@ type StorageTarget struct { // field here would need a matching change in the other repo to stay non-drifting. Nothing off-box // needs this value — its only consumer is the agent's own /disks construction, in-process. ConfigPath string `json:"-"` + // PBSNamespace is the storage's CONFIGURED PBS namespace from storage.cfg (proxmox.Storage.Namespace) + // — "" for the root namespace, set for S4 per-customer tenancy. Present ONLY on pbs targets. + // + // It exists because storage.cfg is the ONE authority on which namespace this box's backups use: + // `vzdump --storage ` makes PVE read this exact field, and the agent's own verify client is + // built from it (`cmd/felhom-agent/main.go` → `pbs.Config{Namespace: s.Namespace}`). The DR recipe + // therefore resolves the namespace from HERE and not from a listed snapshot — a namespace-scoped + // PBS list does not echo `ns` per item, so the snapshot's own field is empty and normalising that + // empty to "root" is what made the recipe claim "root" on every per-customer box (R-106). + // + // `json:"-"` for the SAME reason as ConfigPath above: this struct is a cross-repo contract pinned by + // testdata/host-report.golden.json, and nothing off-box reads this value — its only consumer is the + // agent's own dr_recipe construction, in-process. + PBSNamespace string `json:"-"` // ClassHint is a fast|slow HINT derived from the backing disk's rotational flag — a // hint only; the authoritative class is hub-owned (locked decision). "" when not // derivable (network targets have no local rotational flag). diff --git a/internal/hub/report_test.go b/internal/hub/report_test.go index 5702e43..eb216d3 100644 --- a/internal/hub/report_test.go +++ b/internal/hub/report_test.go @@ -35,7 +35,7 @@ func TestHostReport_FieldNamesAndEmptyCollections(t *testing.T) { } // dr_recipe is always set on the real path (Collect); set it here too so the "no null" invariant // covers it (empty pbs is omitempty → omitted, never null). - r.DRRecipe = BuildDRRecipeHostHalf(r.Guests, r.StorageTargets, r.PBSSnapshots) + r.DRRecipe = BuildDRRecipeHostHalf(r.Guests, r.StorageTargets, r.PBSSnapshots, ConfiguredBackupTarget{}) b, err := json.Marshal(r) if err != nil { t.Fatal(err) diff --git a/internal/hub/testdata/host-report.golden.json b/internal/hub/testdata/host-report.golden.json index 5943e36..3ba3c29 100644 --- a/internal/hub/testdata/host-report.golden.json +++ b/internal/hub/testdata/host-report.golden.json @@ -141,7 +141,8 @@ ], "pbs": { "repo_id": "felhom-pbs", - "namespace": "root", + "namespace": "felhom-spike", + "namespace_state": "resolved", "latest_snapshot_id": "9001" }, "drives": [ @@ -154,7 +155,13 @@ ], "pve_storage": [ { "name": "local-lvm", "type": "lvmthin", "content": "rootdir,images" }, - { "name": "usb-backup", "type": "usb", "content": "backup" } - ] + { "name": "usb-backup", "type": "usb", "content": "backup" }, + { "name": "felhom-pbs", "type": "pbs", "content": "backup" } + ], + "backup_target": { + "state": "resolved", + "storage_id": "usb-backup", + "mount_path": "/mnt/usb-backup" + } } } diff --git a/internal/pbs/live_reporter_test.go b/internal/pbs/live_reporter_test.go index fcc52a0..9e12873 100644 --- a/internal/pbs/live_reporter_test.go +++ b/internal/pbs/live_reporter_test.go @@ -55,7 +55,8 @@ func TestLiveReporter_CoordPresentWithoutPriorVerify(t *testing.T) { // The whole point: a recipe built from the live read carries the pbs coord. h := hub.BuildDRRecipeHostHalf(nil, - []hub.StorageTarget{{Name: "felhom-pbs", Type: hub.StorageTypePBS, Content: "backup"}}, got) + []hub.StorageTarget{{Name: "felhom-pbs", Type: hub.StorageTypePBS, Content: "backup"}}, got, + hub.ConfiguredBackupTarget{StorageID: "felhom-pbs", Known: true}) if h.PBS == nil { t.Fatal("pbs coord absent despite a reachable PBS — the gap this fixes") } @@ -67,7 +68,8 @@ func TestLiveReporter_CoordPresentWithoutPriorVerify(t *testing.T) { bare := NewSnapshotStore() h2 := hub.BuildDRRecipeHostHalf(nil, []hub.StorageTarget{{Name: "felhom-pbs", Type: hub.StorageTypePBS, Content: "backup"}}, - bare.PBSSnapshots(context.Background())) + bare.PBSSnapshots(context.Background()), + hub.ConfiguredBackupTarget{StorageID: "felhom-pbs", Known: true}) if h2.PBS != nil { t.Fatal("companion sanity: the bare store should yield NO pbs coord (proves the live read is load-bearing)") } diff --git a/internal/storage/observe.go b/internal/storage/observe.go index b7b4bb1..6ead825 100644 --- a/internal/storage/observe.go +++ b/internal/storage/observe.go @@ -242,9 +242,14 @@ func (o *Observer) build(s proxmox.Storage, mounts []Mount) observed { // where it lives; it identifies nothing but itself, and it is not used for device or UUID // resolution anywhere. MountPath stays empty when the mount is gone, which is the truth. ConfigPath: s.Path, - ClassHint: classHint, - Role: "", // hub-owned; not derivable from a Proxmox def (slice 10) - Smart: hub.SmartSummary{Health: hub.SmartUnknown}, + // R-106: the CONFIGURED PBS namespace, carried verbatim from storage.cfg. Empty for every + // non-pbs storage (Proxmox only emits it on pbs), and empty for a pbs storage in the root + // namespace — the DR recipe distinguishes those two cases by the storage's TYPE, never by + // guessing from this string. + PBSNamespace: s.Namespace, + ClassHint: classHint, + Role: "", // hub-owned; not derivable from a Proxmox def (slice 10) + Smart: hub.SmartSummary{Health: hub.SmartUnknown}, } // Thin-pool DATA fill: surfaced prominently for lvmthin (metadata fill is Phase B/lvs).