diff --git a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md index 6d28977f19..04d5d36d3b 100644 --- a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md +++ b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md @@ -8039,3 +8039,20 @@ an isolated copy of the same test binary. Joining that probe's handler does not join upgraded handlers from preceding tests; process isolation keeps their final log writes from racing with the hook. The child retains race instrumentation and an explicit timeout, and the original server still fails the ordering assertion. + +### Physical-disk provider identity round trip + +The monitor's disk readback preserves the Proxmox source ID separately from +its canonical resource ID. Skipped physical-disk polls may refresh SMART data +but must not feed canonical hashes into source inventory, changing identity at +each subsequent projection. Views without Proxmox source metadata retain the +legacy ID fallback. This does not relax cross-host/device correlation, migrate +old duplicates, change agent authority, or redefine serial-less disk matching. + +`TestPhysicalDiskSkippedPollPreservesSourceIdentity` in +`internal/monitoring/physical_disk_roundtrip_test.go` drives eight real skipped +polls and adapter rebuilds for both path-shaped and bare device names, checking +provider keys, canonical IDs, JSON projection, temperature/size/cadence, separate +nodes/controller members, and confirmed inventory removal. +`TestPhysicalDiskReadbackSourceIDFallback` covers missing source metadata. +This is synthetic runtime evidence, not USB hardware or reporter acceptance. diff --git a/docs/release-control/v6/internal/subsystems/monitoring.md b/docs/release-control/v6/internal/subsystems/monitoring.md index 9262174506..628240b6dd 100644 --- a/docs/release-control/v6/internal/subsystems/monitoring.md +++ b/docs/release-control/v6/internal/subsystems/monitoring.md @@ -1565,6 +1565,17 @@ HTTP polling through normal alert-manager publication, not destination delivery. ## Current State +TrueNAS REST alert arguments may be object, scalar, array, null or absent. +Non-object arguments must not abort snapshot collection or discard alerts. +Only typed object fields supply disk identity and SMART counters; neither scalar +arguments nor formatted messages establish those measurements. Alert identity, +severity, dismissal and timestamp remain intact. Malformed JSON and HTTP +failures still fail collection rather than falsely reporting recovery. +`TestRESTAlertArgsSnapshot` covers two snapshot cycles and mixed SMART alerts +through an in-memory HTTP transport with REST already selected; it does not +qualify negotiation or a physical TrueNAS appliance. + + ### Install snapshots aggregate alert quality across isolated tenant managers `ReloadableMonitor.AggregateInstallSnapshotCounts` loads each provisioned @@ -4027,3 +4038,20 @@ and acknowledgement, requiring no queued notification. Existing routing, quiet-hours and cooldown tests now use real active manager occurrences rather than fabricated inactive alerts. This is callback-boundary proof, not a claim that already admitted or provider-accepted deliveries can be recalled. + +### Physical-disk provider identity round trip + +The monitor's disk readback preserves the Proxmox source ID separately from +its canonical resource ID. Skipped physical-disk polls may refresh SMART data +but must not feed canonical hashes into source inventory, changing identity at +each subsequent projection. Views without Proxmox source metadata retain the +legacy ID fallback. This does not relax cross-host/device correlation, migrate +old duplicates, change agent authority, or redefine serial-less disk matching. + +`TestPhysicalDiskSkippedPollPreservesSourceIdentity` in +`internal/monitoring/physical_disk_roundtrip_test.go` drives eight real skipped +polls and adapter rebuilds for both path-shaped and bare device names, checking +provider keys, canonical IDs, JSON projection, temperature/size/cadence, separate +nodes/controller members, and confirmed inventory removal. +`TestPhysicalDiskReadbackSourceIDFallback` covers missing source metadata. +This is synthetic runtime evidence, not USB hardware or reporter acceptance. diff --git a/docs/release-control/v6/internal/subsystems/registry.json b/docs/release-control/v6/internal/subsystems/registry.json index d405bc9a2b..052e42b3aa 100644 --- a/docs/release-control/v6/internal/subsystems/registry.json +++ b/docs/release-control/v6/internal/subsystems/registry.json @@ -1623,6 +1623,7 @@ "internal/monitoring/docker_metric_presence_test.go", "internal/monitoring/monitor_host_agent_removal_lifecycle_test.go", "internal/monitoring/monitor_host_agents_test.go", + "internal/monitoring/physical_disk_roundtrip_test.go", "scripts/installtests/agent_state_dir_lifecycle_test.go", "scripts/installtests/install_ps1_test.go", "scripts/installtests/install_sh_test.go" @@ -6065,6 +6066,7 @@ "exact_files": [ "internal/api/resourceapi/resources_test.go", "internal/monitoring/monitor_polling_test.go", + "internal/truenas/alert_args_test.go", "internal/truenas/client_api_shapes_test.go", "internal/truenas/client_test.go", "internal/truenas/contract_test.go", @@ -6464,6 +6466,7 @@ "internal/monitoring/monitor_mock_alerts_test.go", "internal/monitoring/monitor_pve_cluster_refresh_test.go", "internal/monitoring/monitor_pve_guest_lxc_test.go", + "internal/monitoring/physical_disk_roundtrip_test.go", "internal/monitoring/proxmox_large_cluster_poll_budget_test.go", "internal/monitoring/pve_protection_observation_test.go", "internal/monitoring/ratetracker_test.go", @@ -8218,6 +8221,7 @@ "internal/monitoring/docker_filesystem_evidence_test.go", "internal/monitoring/issue1595_collection_trust_test.go", "internal/monitoring/issue1913_host_continuity_test.go", + "internal/monitoring/physical_disk_roundtrip_test.go", "internal/unifiedresources/adapter_coverage_test.go", "internal/unifiedresources/adapters_test.go", "internal/unifiedresources/ceph_pool_health_contract_test.go", diff --git a/docs/release-control/v6/internal/subsystems/unified-resources.md b/docs/release-control/v6/internal/subsystems/unified-resources.md index a152c7a2d2..4c0aefd1a6 100644 --- a/docs/release-control/v6/internal/subsystems/unified-resources.md +++ b/docs/release-control/v6/internal/subsystems/unified-resources.md @@ -5214,3 +5214,20 @@ inference and presentation path, including preservation of the reciprocal link; `TestProxmoxOneWayLinkRejectsHostLocalEndpointCorroboration` protect management network and explicit report-IP compatibility. These synthetic cases do not establish the cause of a particular installation's association. + +### Physical-disk provider identity round trip + +The monitor's disk readback preserves the Proxmox source ID separately from +its canonical resource ID. Skipped physical-disk polls may refresh SMART data +but must not feed canonical hashes into source inventory, changing identity at +each subsequent projection. Views without Proxmox source metadata retain the +legacy ID fallback. This does not relax cross-host/device correlation, migrate +old duplicates, change agent authority, or redefine serial-less disk matching. + +`TestPhysicalDiskSkippedPollPreservesSourceIdentity` in +`internal/monitoring/physical_disk_roundtrip_test.go` drives eight real skipped +polls and adapter rebuilds for both path-shaped and bare device names, checking +provider keys, canonical IDs, JSON projection, temperature/size/cadence, separate +nodes/controller members, and confirmed inventory removal. +`TestPhysicalDiskReadbackSourceIDFallback` covers missing source metadata. +This is synthetic runtime evidence, not USB hardware or reporter acceptance. diff --git a/internal/monitoring/monitor.go b/internal/monitoring/monitor.go index b37b640209..49d0cf35bb 100644 --- a/internal/monitoring/monitor.go +++ b/internal/monitoring/monitor.go @@ -735,8 +735,14 @@ func physicalDiskFromReadStateView(view *unifiedresources.PhysicalDiskView) mode return models.PhysicalDisk{Wearout: unifiedresources.WearoutUnreported} } + // Preserve the provider key across the canonical read-state round trip. + // Older/synthetic views without source metadata retain their existing fallback. + sourceID := view.SourceID() + if sourceID == "" { + sourceID = view.ID() + } disk := models.PhysicalDisk{ - ID: view.ID(), + ID: sourceID, Node: view.Node(), Instance: view.Instance(), DevPath: view.DevPath(), diff --git a/internal/monitoring/physical_disk_roundtrip_test.go b/internal/monitoring/physical_disk_roundtrip_test.go new file mode 100644 index 0000000000..4f00d814ad --- /dev/null +++ b/internal/monitoring/physical_disk_roundtrip_test.go @@ -0,0 +1,122 @@ +package monitoring + +import ( + "context" + "encoding/json" + "testing" + "time" + + "github.com/rcourtman/pulse-go-rewrite/internal/config" + "github.com/rcourtman/pulse-go-rewrite/internal/models" + "github.com/rcourtman/pulse-go-rewrite/internal/unifiedresources" +) + +// Exercise the real skipped-poll path, not just the ID helper: canonical views +// are converted back into source state and then re-ingested by the adapter. +func TestPhysicalDiskSkippedPollPreservesSourceIdentity(t *testing.T) { + for _, device := range []string{"/dev/sdx", "sdx"} { + t.Run(device, func(t *testing.T) { + state := models.NewState() + nodes := []models.Node{{ID: "pve-node1", Name: "node1", Instance: "pve"}, {ID: "pve-node2", Name: "node2", Instance: "pve"}} + state.UpdateNodesForInstance("pve", nodes) + var disks []models.PhysicalDisk + for _, fixture := range []struct{ node, target string }{{"node1", ""}, {"node2", ""}, {"node1", "megaraid,0"}, {"node1", "megaraid,1"}} { + disks = append(disks, models.PhysicalDisk{ + ID: unifiedresources.ProxmoxPhysicalDiskSourceID("pve", fixture.node, device, "", fixture.target), + Instance: "pve", Node: fixture.node, DevPath: device, Target: fixture.target, + Model: "USB fixture", Size: 1024, Temperature: 37, Wearout: -1, LastChecked: time.Now(), + ExpectedUpdateInterval: 5 * time.Minute, + }) + } + state.UpdatePhysicalDisks("pve", disks) + adapter := unifiedresources.NewMonitorAdapter(unifiedresources.NewRegistry(nil)) + adapter.PopulateFromSnapshot(state.GetSnapshot()) + m := &Monitor{state: state, resourceStore: adapter, lastPhysicalDiskPoll: map[string]time.Time{"pve": time.Now()}} + canonical := map[string]bool{} + for _, view := range adapter.PhysicalDisks() { + canonical[view.ID()] = true + } + if len(canonical) != len(disks) { + t.Fatalf("initial disks = %d, want %d", len(canonical), len(disks)) + } + for cycle := 0; cycle < 8; cycle++ { + m.maybePollPhysicalDisksAsync(context.Background(), "pve", &config.PVEInstance{}, nil, nil, nil, nil) + got := state.GetSnapshot().PhysicalDisks + if len(got) != len(disks) { + t.Fatalf("cycle %d: source count %d", cycle, len(got)) + } + wantIDs := map[string]bool{} + for _, disk := range disks { + wantIDs[disk.ID] = true + } + for _, disk := range got { + if !wantIDs[disk.ID] { + t.Fatalf("cycle %d: canonical ID leaked into provider state: %q", cycle, disk.ID) + } + if disk.DevPath != device || disk.Temperature != 37 || disk.Size != 1024 || disk.ExpectedUpdateInterval != 5*time.Minute { + t.Fatalf("cycle %d: metadata lost: %+v", cycle, disk) + } + } + adapter.PopulateFromSnapshot(state.GetSnapshot()) + views := adapter.PhysicalDisks() + if len(views) != len(disks) { + t.Fatalf("cycle %d: view count = %d", cycle, len(views)) + } + for _, view := range views { + if !canonical[view.ID()] { + t.Fatalf("cycle %d: canonical identity churn: %s", cycle, view.ID()) + } + } + // Retain the JSON-visible projection as well as the typed read-state checks. + payload, err := json.Marshal(adapter.GetAll()) + if err != nil { + t.Fatal(err) + } + var resources []unifiedresources.Resource + if err := json.Unmarshal(payload, &resources); err != nil { + t.Fatal(err) + } + count := 0 + for _, resource := range resources { + if resource.Type != unifiedresources.ResourceTypePhysicalDisk { + continue + } + count++ + if !canonical[resource.ID] || resource.Proxmox == nil || !wantIDs[resource.Proxmox.SourceID] || resource.PhysicalDisk == nil || resource.PhysicalDisk.Temperature != 37 { + t.Fatalf("cycle %d: JSON disk identity/metadata lost: %+v", cycle, resource) + } + } + if count != len(disks) { + t.Fatalf("cycle %d: JSON disk count %d", cycle, count) + } + } + // A confirmed full inventory removal must still remove source-owned disks. + state.UpdatePhysicalDisks("pve", nil) + adapter.PopulateFromSnapshot(state.GetSnapshot()) + if got := len(adapter.PhysicalDisks()); got != 0 { + t.Fatalf("removed inventory retained %d disks", got) + } + }) + } +} + +func TestPhysicalDiskReadbackSourceIDFallback(t *testing.T) { + for _, resource := range []unifiedresources.Resource{ + {ID: "canonical"}, + {ID: "canonical", Proxmox: &unifiedresources.ProxmoxData{}}, + {ID: "canonical", Proxmox: &unifiedresources.ProxmoxData{SourceID: " native "}}, + } { + view := unifiedresources.NewPhysicalDiskView(&resource) + want := "canonical" + if resource.Proxmox != nil && resource.Proxmox.SourceID != "" { + want = "native" + } + if got := physicalDiskFromReadStateView(&view).ID; got != want { + t.Fatalf("ID = %q, want %q", got, want) + } + } + var view unifiedresources.PhysicalDiskView + if view.SourceID() != "" { + t.Fatal("nil resource has a source ID") + } +} diff --git a/internal/truenas/alert_args_test.go b/internal/truenas/alert_args_test.go new file mode 100644 index 0000000000..197187cfea --- /dev/null +++ b/internal/truenas/alert_args_test.go @@ -0,0 +1,82 @@ +package truenas + +import ( + "context" + "fmt" + "io" + "net/http" + "strings" + "testing" + "time" +) + +// Exercise the real HTTP decoder and snapshot orchestration without a listener. +type alertArgsTransport map[string]apiResponse + +func (routes alertArgsTransport) RoundTrip(r *http.Request) (*http.Response, error) { + response, ok := routes[r.URL.Path] + status := response.status + if status == 0 { + status = http.StatusOK + } + if !ok { + status = http.StatusNotFound + response.body = "{}" + } + return &http.Response{StatusCode: status, Header: http.Header{"Content-Type": []string{"application/json"}}, Body: io.NopCloser(strings.NewReader(response.body)), Request: r}, nil +} + +func TestRESTAlertArgsSnapshot(t *testing.T) { + for _, args := range []string{`"53 errors on /dev/sda"`, `null`, `[]`, `53`, `true`, `{}`, `{"name":"/dev/sda","serial":"SER-A","ue":53}`} { + t.Run(args, func(t *testing.T) { + routes := alertArgsTransport(defaultAPIResponses()) + routes["/api/v2.0/system/info"] = apiResponse{body: `{"hostname":"truenas-main","version":"TrueNAS-13.0-U6.1"}`} + routes["/api/v2.0/alert/list"] = apiResponse{body: fmt.Sprintf(`[{"id":"smart-1","level":"WARNING","formatted":"53 errors on /dev/sda","source":"SMART","klass":"SMARTUncorrectedErrorsAlert","args":%s,"dismissed":true,"datetime":{"$date":1707400000000}},{"id":"spare","level":"CRITICAL","formatted":"Reserve low","klass":"SMARTSpareBlockCountAlert","args":{"sb":8},"datetime":{"$date":1707400000000}}]`, args)} + client, err := NewClient(ClientConfig{Host: "http://truenas.invalid", APIKey: "synthetic"}) + if err != nil { + t.Fatal(err) + } + client.httpClient.Transport = routes + // Negotiation is outside this REST decoder regression; never open a socket. + client.mode = TransportLegacyREST + if err := client.TestConnection(context.Background()); err != nil { + t.Fatal(err) + } + for cycle := 0; cycle < 2; cycle++ { + snapshot, err := client.FetchSnapshot(context.Background()) + if err != nil { + t.Fatalf("cycle %d: %v", cycle, err) + } + if snapshot.System.Hostname != "truenas-main" || snapshot.CollectedAt.IsZero() || len(snapshot.Pools) != 1 || len(snapshot.Datasets) != 1 || len(snapshot.Disks) != 2 || len(snapshot.Alerts) != 2 { + t.Fatalf("incomplete snapshot: %+v", snapshot) + } + a := snapshot.Alerts[0] + if a.ID != "smart-1" || a.Level != "WARNING" || a.Message != "53 errors on /dev/sda" || a.Source != "SMART" || a.Class != "SMARTUncorrectedErrorsAlert" || !a.Dismissed || !a.Datetime.Equal(time.UnixMilli(1707400000000).UTC()) { + t.Fatalf("lost alert: %+v", a) + } + object := strings.Contains(args, "serial") + if a.SMARTUncorrectedReported != object || a.SMARTAvailableSpareReported { + t.Fatalf("invented/lost evidence: %+v", a) + } + if object { + if a.DiskName != "/dev/sda" || a.DiskSerial != "SER-A" || a.SMARTUncorrectedErrors != 53 { + t.Fatalf("lost SMART metadata: %+v", a) + } + } else if a.DiskName != "" || a.DiskSerial != "" || a.SMARTUncorrectedErrors != 0 { + t.Fatalf("inferred metadata: %+v", a) + } + if b := snapshot.Alerts[1]; !b.SMARTAvailableSpareReported || b.SMARTAvailableSpare != 8 { + t.Fatalf("lost neighbouring alert: %+v", b) + } + } + routes["/api/v2.0/alert/list"] = apiResponse{body: `[{"args":`} + if _, err := client.FetchSnapshot(context.Background()); err == nil { + t.Fatal("malformed JSON must still fail") + } + routes["/api/v2.0/alert/list"] = apiResponse{status: http.StatusUnauthorized, body: "{}"} + if _, err := client.FetchSnapshot(context.Background()); err == nil { + t.Fatal("HTTP failure must still fail") + } + }) + } +} diff --git a/internal/truenas/client.go b/internal/truenas/client.go index d67d2e3e25..36c685ea94 100644 --- a/internal/truenas/client.go +++ b/internal/truenas/client.go @@ -1265,15 +1265,18 @@ func (c *Client) getAlertsREST(ctx context.Context) ([]Alert, error) { return nil, fmt.Errorf("parse alert %q datetime: %w", id, err) } - uncorrected, uncorrectedReported, availableSpare, availableReported := smartAlertEvidenceFromArgs(item.Class, item.Args) + // Legacy alert args may be any JSON value. Only objects provide typed + // disk identity or SMART evidence; formatted text remains the message. + args, _ := item.Args.(map[string]any) + uncorrected, uncorrectedReported, availableSpare, availableReported := smartAlertEvidenceFromArgs(item.Class, args) alerts = append(alerts, Alert{ ID: id, Level: strings.TrimSpace(item.Level), Message: strings.TrimSpace(item.Formatted), Source: strings.TrimSpace(item.Source), Class: strings.TrimSpace(item.Class), - DiskName: strings.TrimSpace(readStringAny(item.Args, "name", "disk", "device")), - DiskSerial: strings.TrimSpace(readStringAny(item.Args, "serial", "serial_number", "serialNumber")), + DiskName: strings.TrimSpace(readStringAny(args, "name", "disk", "device")), + DiskSerial: strings.TrimSpace(readStringAny(args, "serial", "serial_number", "serialNumber")), SMARTUncorrectedErrors: uncorrected, SMARTUncorrectedReported: uncorrectedReported, SMARTAvailableSpare: availableSpare, @@ -4533,7 +4536,7 @@ type alertResponse struct { Formatted string `json:"formatted"` Source string `json:"source"` Class string `json:"klass"` - Args map[string]any `json:"args"` + Args any `json:"args"` Dismissed bool `json:"dismissed"` Datetime struct { Date json.RawMessage `json:"$date"` diff --git a/internal/unifiedresources/views.go b/internal/unifiedresources/views.go index b87f86f285..ff0a0b79a0 100644 --- a/internal/unifiedresources/views.go +++ b/internal/unifiedresources/views.go @@ -2120,6 +2120,16 @@ func (v PhysicalDiskView) ID() string { return v.r.ID } +// SourceID returns the Proxmox-native inventory key, not the canonical +// resource ID. Polling adapters must preserve this key when writing views back +// into provider state; hashing a canonical ID again creates a new resource. +func (v PhysicalDiskView) SourceID() string { + if v.r == nil || v.r.Proxmox == nil { + return "" + } + return strings.TrimSpace(v.r.Proxmox.SourceID) +} + func (v PhysicalDiskView) Name() string { if v.r == nil { return "" diff --git a/scripts/release_control/canonical_completion_guard_test.py b/scripts/release_control/canonical_completion_guard_test.py index 6a7ee00a6c..4970cd5eee 100644 --- a/scripts/release_control/canonical_completion_guard_test.py +++ b/scripts/release_control/canonical_completion_guard_test.py @@ -229,6 +229,7 @@ class CanonicalCompletionGuardTest(unittest.TestCase): "internal/monitoring/docker_metric_presence_test.go", "internal/monitoring/monitor_host_agent_removal_lifecycle_test.go", "internal/monitoring/monitor_host_agents_test.go", + "internal/monitoring/physical_disk_roundtrip_test.go", "scripts/installtests/agent_state_dir_lifecycle_test.go", "scripts/installtests/install_ps1_test.go", "scripts/installtests/install_sh_test.go", @@ -322,6 +323,7 @@ class CanonicalCompletionGuardTest(unittest.TestCase): "internal/monitoring/monitor_mock_alerts_test.go", "internal/monitoring/monitor_pve_cluster_refresh_test.go", "internal/monitoring/monitor_pve_guest_lxc_test.go", + "internal/monitoring/physical_disk_roundtrip_test.go", "internal/monitoring/proxmox_large_cluster_poll_budget_test.go", "internal/monitoring/pve_protection_observation_test.go", "internal/monitoring/ratetracker_test.go", @@ -464,6 +466,7 @@ class CanonicalCompletionGuardTest(unittest.TestCase): "internal/monitoring/docker_metric_presence_test.go", "internal/monitoring/monitor_host_agent_removal_lifecycle_test.go", "internal/monitoring/monitor_host_agents_test.go", + "internal/monitoring/physical_disk_roundtrip_test.go", "scripts/installtests/agent_state_dir_lifecycle_test.go", "scripts/installtests/install_ps1_test.go", "scripts/installtests/install_sh_test.go", diff --git a/scripts/release_control/subsystem_lookup_test.py b/scripts/release_control/subsystem_lookup_test.py index 1a61b405fa..b0940daf5d 100644 --- a/scripts/release_control/subsystem_lookup_test.py +++ b/scripts/release_control/subsystem_lookup_test.py @@ -4337,6 +4337,7 @@ class SubsystemLookupTest(unittest.TestCase): "internal/monitoring/docker_metric_presence_test.go", "internal/monitoring/monitor_host_agent_removal_lifecycle_test.go", "internal/monitoring/monitor_host_agents_test.go", + "internal/monitoring/physical_disk_roundtrip_test.go", "scripts/installtests/agent_state_dir_lifecycle_test.go", "scripts/installtests/install_ps1_test.go", "scripts/installtests/install_sh_test.go",