R-106 follow-up: mergeConfig dropped the pbs namespace, so v0.118.0's fix was inert (v0.118.1)
Live validation caught what the tests could not. On demo-felhom the recipe read namespace "root" with namespace_state "resolved" — confident and wrong, a worse shape than the original defect. mergeConfig overlays the cluster storage config onto the node entry through a hand-listed set of fields and Namespace was not among them. NodeStorage does not return the namespace at all, so PBSNamespace always read "" and latestPBSCoord correctly treated that as the root namespace. Every v0.118.0 test built StorageTarget values directly — including the two through Collector.Collect(), which inject a fakeObserver — so nothing crossed the merge. Two new tests drive the real Observe path with PVE's actual split returns and table the merge itself. Red-proof: dropping the added line fails both. Suite rc=0, 29 packages, 0 FAIL.
This commit is contained in:
@@ -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)
|
## 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 —
|
**The recipe is read at the worst possible moment — by an operator rebuilding a machine that is gone —
|
||||||
|
|||||||
@@ -488,5 +488,16 @@ func mergeConfig(node, cluster proxmox.Storage) proxmox.Storage {
|
|||||||
if node.Content == "" {
|
if node.Content == "" {
|
||||||
node.Content = cluster.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
|
return node
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -292,3 +292,66 @@ func TestObserve_MountReadFailureDegradesNotFatal(t *testing.T) {
|
|||||||
t.Errorf("lvmthin still derivable without mounts: %+v", got)
|
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)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user