gsadmin/pulse
1
0
Fork 0
mirror of https://github.com/rcourtman/Pulse.git synced 2026-10-24 04:33:17 +00:00
Code Issues Packages Projects Releases Wiki Activity

Merge pull request #2082 from rcourtman/maintainer/01b9d5b9163d46c9be355e56c542fc50

Browse Source 
fix: preserve disk identity and tolerate TrueNAS alert argument shapes
This commit is contained in:
pulse-triage[bot]
2026-09-14 09:30:30 +00:00
committed by GitHub
parent a081769c05 7e21b005fa
commit ce1e99ba4e
11 changed files with 298 additions and 5 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/release-control/v6/internal/subsystems/agent-lifecycle.md
+17
View File
@@ -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.
docs/release-control/v6/internal/subsystems/monitoring.md
+28
View File
@@ -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.
docs/release-control/v6/internal/subsystems/registry.json
+4
View File
@@ -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",
docs/release-control/v6/internal/subsystems/unified-resources.md
+17
View File
@@ -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.
internal/monitoring/monitor.go
+7 -1
View File
@@ -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(),
internal/monitoring/physical_disk_roundtrip_test.go
+122
View File
@@ -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")
}
}
internal/truenas/alert_args_test.go
+82
View File
@@ -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")
}
})
}
}
internal/truenas/client.go
+7 -4
View File
@@ -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"`
internal/unifiedresources/views.go
+10
View File
@@ -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 ""
scripts/release_control/canonical_completion_guard_test.py
+3
View File
@@ -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",
scripts/release_control/subsystem_lookup_test.py
+1
View File
@@ -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",