R-106 + R-109: the DR recipe records the resolved namespace and names the backup target (v0.118.0)

Both defects were live on both demo boxes: the recipe said namespace "root" while storage.cfg said demo-felhom/demo-hp, and it never named which of two content=backup dir storages holds the local archives. R-106: the namespace came from the listed snapshot, but PBS omits `ns` per item once the list is namespace-scoped, so it was always empty and normalised to "root". It now resolves from the pbs STORAGE (storage.cfg's `namespace`) — the same field vzdump makes PVE read, so the recipe cannot disagree with the backup. R-109: backup_target resolves from the primary tier of cfg.Backup.BackupTiers(), the function the scheduler consults, and carries the mountpoint that separates /mnt/hdd_1 from /var/lib/vz. The resolver reports the tier IN EFFECT (daemon-start config), not agent.json on disk — a target move rewrites the file and deliberately does not restart. Unresolvable is recorded as unresolvable: resolved|unknown plus a distinct reason, never a default, an empty string, or a placeholder. Needs hub v0.83.0 — AssembleDRRecipe allow-lists top-level keys, so backup_target would otherwise be stored intact and dropped before any operator saw it. 9 tests, 4 red-proofs (each mutation asserted to have landed). Suite rc=0, 29 ok.
2026-07-30 13:11:08 +02:00
parent 1913e12031
commit 1c8a67eece
13 changed files with 670 additions and 103 deletions
@@ -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.)