diff --git a/CHANGELOG.md b/CHANGELOG.md index 66adcf4..2bd1f6a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,33 @@ +## v0.118.1 — R-106 follow-up: the namespace was still being lost in the merge (2026-07-30) + +**v0.118.0's R-106 fix was incomplete and live validation caught it.** Deployed to demo-felhom, the +recipe read: + + "pbs": { ..., "namespace": "root", "namespace_state": "resolved", ... } + +`namespace_state: resolved` while the value was still the wrong `"root"` — which is a worse shape than +the original defect, because it asserts confidence in a wrong answer. The new state field is what made it +legible at a glance; without it this would have looked identical to the pre-fix output. + +**Cause.** `mergeConfig` (`internal/storage/observe.go:457`) overlays the CLUSTER storage config onto the +NODE entry via a hand-listed set of type-specific fields — `Type`, `Server`, `Export`, `Share`, +`Datastore`, `Fingerprint`, `VGName`, `ThinPool`, `Path`, `Content` — and `Namespace` was not on that list. +`NodeStorage` does not return the namespace at all (it is cluster-config only), so the merged entry's +`Namespace` was always empty and `StorageTarget.PBSNamespace` read `""`. `latestPBSCoord` then treated an +empty configured namespace as the genuine root namespace, which is correct logic fed a wrong input. + +**Why the tests did not catch it.** Every test added in v0.118.0 constructs `StorageTarget` values +directly — including the two that run `Collector.Collect()`, because they inject a `fakeObserver`. The +break was UPSTREAM of the collector, in the observer's merge, so "the production generation path" as I had +drawn it started one layer too late. Two new tests fix that: `TestObserve_CarriesPBSNamespaceThroughMerge` +drives the real `Observe` with the split PVE returns reproduced (namespace present in the cluster list, +absent from the node list — what PVE actually does), and `TestMergeConfig_CarriesPBSNamespace` tables the +merge itself including fill-if-empty vs never-clobber. Red-proof: removing the one added line fails both, +the first quoting the exact live symptom. Suite rc=0, 29 packages, 0 FAIL. + +`Username` is the one remaining unmerged type-specific field; nothing in the observer path consumes it, and +the comment on the merge now says to add it here the moment something does. + ## 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 — diff --git a/internal/storage/observe.go b/internal/storage/observe.go index 6ead825..f7fcbbf 100644 --- a/internal/storage/observe.go +++ b/internal/storage/observe.go @@ -488,5 +488,16 @@ func mergeConfig(node, cluster proxmox.Storage) proxmox.Storage { if node.Content == "" { node.Content = cluster.Content } + // R-106: the pbs namespace. `NodeStorage` does not return it AT ALL — it is cluster-config only — + // so without this line `Namespace` is always empty on the merged entry and every consumer sees a + // root-namespace box. Found by LIVE VALIDATION, not by the unit tests: the DR-recipe tests supply + // StorageTarget values directly, so they never crossed this merge. + // + // This function is a copy-only-what-is-needed allow-list, which is exactly how the gap arose. If you + // add a consumer of any other type-specific field (`Username` is the remaining unmerged one), add it + // here too and pin it in TestMergeConfig_CarriesPBSNamespace's table. + if node.Namespace == "" { + node.Namespace = cluster.Namespace + } return node } diff --git a/internal/storage/observe_test.go b/internal/storage/observe_test.go index 6addb8e..62cc32e 100644 --- a/internal/storage/observe_test.go +++ b/internal/storage/observe_test.go @@ -292,3 +292,66 @@ func TestObserve_MountReadFailureDegradesNotFatal(t *testing.T) { t.Errorf("lvmthin still derivable without mounts: %+v", got) } } + +// --------------------------------------------------------------------------------------------- +// R-106 — the pbs namespace must survive the cluster/node merge. +// --------------------------------------------------------------------------------------------- + +// TestObserve_CarriesPBSNamespaceThroughMerge pins the gap that shipped in agent v0.118.0 and was caught +// by LIVE VALIDATION rather than by tests: `mergeConfig` copies a hand-listed set of type-specific fields +// from the CLUSTER config onto the NODE entry, and `Namespace` was not on that list. `NodeStorage` does +// not return the namespace at all — it is cluster-config only — so `StorageTarget.PBSNamespace` was +// always empty and the DR recipe reported the root namespace on every per-customer box, exactly the +// R-106 symptom the fix was supposed to remove. +// +// The DR-recipe tests could not catch it: they construct StorageTarget values directly, so nothing +// crossed this merge. This test drives the REAL Observe path with the split PVE returns reproduced — +// namespace present in the cluster list, absent from the node list, which is what PVE actually does. +func TestObserve_CarriesPBSNamespaceThroughMerge(t *testing.T) { + api := &fakeStorageAPI{ + node: "demo-felhom", + // Cluster config: carries the type-specific fields, as /storage does. + cluster: []proxmox.Storage{{ + Storage: "felhom-pbs", Type: "pbs", Content: "backup", + Server: "10.77.0.1", Datastore: "felhom-offsite", Namespace: "demo-felhom", + }}, + // Node entry: live usage + active flag, and NO namespace — the shape that made the bug invisible. + nodeSt: []proxmox.Storage{{ + Storage: "felhom-pbs", Type: "pbs", Content: "backup", + Total: 100, Used: 10, Avail: 90, Active: 1, Enabled: 1, + }}, + } + o := NewObserver(api, &fakeHostReader{}, nil, quietLogger()) + + targets, err := o.Observe(context.Background()) + if err != nil { + t.Fatalf("Observe: %v", err) + } + if len(targets) != 1 { + t.Fatalf("want 1 target, got %d", len(targets)) + } + if got := targets[0].PBSNamespace; got != "demo-felhom" { + t.Errorf("PBSNamespace=%q, want %q — the namespace was lost in mergeConfig, so the DR recipe "+ + "reports the root namespace on a per-customer box (R-106)", got, "demo-felhom") + } +} + +// TestMergeConfig_CarriesPBSNamespace is the direct table over the merge itself: a node entry that omits +// a field takes the cluster's value, and a node entry that HAS one keeps its own (never clobbered). +func TestMergeConfig_CarriesPBSNamespace(t *testing.T) { + cluster := proxmox.Storage{Storage: "felhom-pbs", Type: "pbs", Namespace: "demo-hp", Datastore: "felhom-offsite"} + + // Node omits the namespace (the real PVE shape) → it must be filled from the cluster config. + if got := mergeConfig(proxmox.Storage{Storage: "felhom-pbs"}, cluster).Namespace; got != "demo-hp" { + t.Errorf("namespace absent on the node entry: got %q, want it merged from the cluster config", got) + } + // Node already has one → keep it (the merge is fill-if-empty, never overwrite). + nodeOwn := proxmox.Storage{Storage: "felhom-pbs", Namespace: "node-wins"} + if got := mergeConfig(nodeOwn, cluster).Namespace; got != "node-wins" { + t.Errorf("merge clobbered the node's own namespace: got %q", got) + } + // No cluster row at all → the node entry passes through untouched. + if got := mergeConfig(proxmox.Storage{Storage: "x", Namespace: "keep"}, proxmox.Storage{}).Namespace; got != "keep" { + t.Errorf("empty cluster row altered the node entry: got %q", got) + } +}