diff --git a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md index 0a90459e2a..f00aaa9eb2 100644 --- a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md +++ b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md @@ -20,6 +20,14 @@ management, and fleet control surfaces. Pulse v6 has one host-installed infrastructure agent binary, `pulse-agent`; host, Docker / Podman, Kubernetes, Proxmox-local, and other node-local telemetry are modules inside that binary, not separate customer-facing agent products. +The host telemetry module owns lossless physical-disk report assembly. Linux +smartctl permission/open failures are not standby; only the explicit +`-n standby,3` exit contract may mark a device sleeping. Identity-only rows +must retain sysfs model, serial, WWID, size, and transport when available, +smartctl retry modes merge rather than replace evidence, and partial NVMe JSON +must preserve field presence so omitted health counters are not fabricated as +zero. Direct SATA, SAS, NVMe, and controller-member inventory must all survive +one compressed unified-agent report without a collector-side suffix cap. ## Canonical Files @@ -1297,6 +1305,10 @@ the intentionally sparse public response. Direct Linux SATA/SAT-style block devices that return health but no temperature through smartctl auto-detection must retry explicit `-d sat` and `-d scsi` probes before settling on a no-temperature result. + Those retries must accumulate model, serial, WWN, health, temperature, and + SMART attributes across attempts. A later healthy result must not erase an + earlier explicit failure, and an omitted NVMe counter must stay absent while + a reported zero remains present through report serialization. Linux `/dev` and `/sys/block` identity parsing must use slash-based device semantics independent of the Go build host, so the Windows native CI lane proves the same SSD standby-guard decisions as Linux instead of interpreting diff --git a/docs/release-control/v6/internal/subsystems/api-contracts.md b/docs/release-control/v6/internal/subsystems/api-contracts.md index 39f186b78e..9b391802be 100644 --- a/docs/release-control/v6/internal/subsystems/api-contracts.md +++ b/docs/release-control/v6/internal/subsystems/api-contracts.md @@ -23,6 +23,13 @@ Own canonical runtime payload shapes between backend and frontend, including the trust boundary that keeps customer-safe support diagnostics and normal product API routes free of maintainer commercial analytics. +Physical-disk payloads preserve optional SMART counter presence, including +explicit zero values, and expose provider vendor metadata without converting +missing data into health. Unified-resource clients may request bounded server +pages until the response's declared `totalPages` is exhausted; no client-side +page ceiling may silently truncate a wide fleet. Disk history lookup resolves +an exact canonical resource ID first and accepts a metrics target, serial, or +WWN only when the match is unique and the hardware identifier is usable. ## Canonical Files diff --git a/docs/release-control/v6/internal/subsystems/monitoring.md b/docs/release-control/v6/internal/subsystems/monitoring.md index 5099813769..a7a42b8089 100644 --- a/docs/release-control/v6/internal/subsystems/monitoring.md +++ b/docs/release-control/v6/internal/subsystems/monitoring.md @@ -102,6 +102,16 @@ reporting source of truth. `MultiTenantMonitor.ListOrganizationIDs` may expose persisted organization IDs to API-owned background workers, but it must not initialize monitors, start pollers, or reinterpret tenant IDs as monitored resource health. +Proxmox physical-disk polling is also a continuity boundary. A failed or +permission-denied `disks/list` call must remain an error so the monitor can use +linked host-agent inventory or retain same-instance, same-node prior evidence; +it must never become a successful empty inventory that removes valid boot or +data disks. SMART enrichment matches serial, WWN, device path, and controller +member topology uniquely and fail-closed, rejects placeholder hardware +identifiers, preserves explicit failure over a later coarse healthy value, and +lets explicit SMART endurance replace contradictory Proxmox wearout. Missing +permission, ambiguous identity, standby, and absent SMART fields remain +neutral rather than borrowing telemetry from another disk. ## Canonical Files @@ -126,6 +136,7 @@ resource health. 19. `internal/dockeragent/swarm.go` 20. `internal/dockeragent/collect.go` 21. `pkg/proxmox/ceph.go` +21a. `pkg/proxmox/cluster_client.go` 22. `pkg/proxmox/zfs.go` 23. `internal/monitoring/guest_memory_sources.go` 24. `internal/monitoring/guest_memory_stability.go` @@ -1330,6 +1341,14 @@ ambiguous. Direct SATA, SAS, and NVMe device fallback IDs retain their legacy shape, while multiple controller members behind one block path add their controller target to the fallback identity. Per-member I/O must never inherit an aggregate controller counter. +The same rules apply to SATA and NVMe inventory: direct-disk source IDs keep +their historical shape, controller-member IDs add their member target, and +cross-source correlation is scoped to the canonical parent node. A successful +retry may enrich an earlier smartctl attempt but must not erase earlier model, +serial, failure, or counter evidence. SMART temperature selection accepts only +plausible readings, prefers ATA attribute 194 over 190 when higher-level +temperature fields are invalid, and preserves reported zero counters as known +values while leaving omitted counters unknown. Disk identity, temperature, I/O, controller association, and pool membership also carry typed collection state from `pkg/diskinventory`: `available`, diff --git a/docs/release-control/v6/internal/subsystems/registry.json b/docs/release-control/v6/internal/subsystems/registry.json index 5a78ed218b..0de34996db 100644 --- a/docs/release-control/v6/internal/subsystems/registry.json +++ b/docs/release-control/v6/internal/subsystems/registry.json @@ -5078,6 +5078,7 @@ "pkg/agents/host/report.go", "pkg/agents/kubernetes/report.go", "pkg/proxmox/ceph.go", + "pkg/proxmox/cluster_client.go", "pkg/proxmox/zfs.go" ], "verification": { @@ -5307,6 +5308,21 @@ "pkg/proxmox/zfs_test.go" ] }, + { + "id": "proxmox-cluster-client-runtime", + "label": "Proxmox cluster client continuity and fallback proof", + "match_prefixes": [], + "match_files": [ + "pkg/proxmox/cluster_client.go" + ], + "allow_same_subsystem_tests": false, + "test_prefixes": [], + "exact_files": [ + "internal/monitoring/monitor_pve_disk_fallback_test.go", + "pkg/proxmox/cluster_client_additional_api4_test.go", + "pkg/proxmox/cluster_client_api_test.go" + ] + }, { "id": "proxmox-ceph-runtime", "label": "proxmox ceph compatibility proof", diff --git a/docs/release-control/v6/internal/subsystems/storage-recovery.md b/docs/release-control/v6/internal/subsystems/storage-recovery.md index d0c322cba2..a26e39c098 100644 --- a/docs/release-control/v6/internal/subsystems/storage-recovery.md +++ b/docs/release-control/v6/internal/subsystems/storage-recovery.md @@ -24,6 +24,12 @@ Own the storage and recovery product surfaces, recovery-point persistence and querying, and the operator-facing storage health presentation layer while keeping adjacent commercial reporting APIs out of storage/recovery product state. +The physical-disks surface consumes every page in the server-declared unified +resource result, scopes same-named Proxmox nodes by instance, and never hides a +disk behind a client-side page ceiling. Search includes vendor, WWN, transport, +instance, controller, and member target. Explicit `0%` life remaining is a +known critical value for SSD/NVMe media; absent or negative wearout remains the +neutral unknown state. ## Canonical Files @@ -2295,6 +2301,12 @@ precedence. These rules belong to deterministic Collection Trust and read-state presentation. They do not establish backup coverage, restore readiness, or any other Recovery Assurance conclusion, and they introduce no storage mutation path outside canonical Actions. +Placeholder serials or WWNs never take identity precedence, and history target +resolution must fail closed when a hardware or metrics identifier is shared by +more than one disk. Node filtering may use a name-only match only when both the +disk and selected node lack instance identity; a known instance must match +exactly so equal node names in different Proxmox connections cannot leak rows +across views. `internal/hostagent/issue1595_sas_collection_test.go` and `internal/monitoring/issue1595_collection_trust_test.go` prove wide-SAS diff --git a/docs/release-control/v6/internal/subsystems/unified-resources.md b/docs/release-control/v6/internal/subsystems/unified-resources.md index 01b6adb4cc..e6ddf6cd12 100644 --- a/docs/release-control/v6/internal/subsystems/unified-resources.md +++ b/docs/release-control/v6/internal/subsystems/unified-resources.md @@ -32,6 +32,15 @@ Physical-disk resources own cross-source disk identity. When Proxmox inventory and host-agent SMART telemetry describe the same device, the merged resource must retain Proxmox node/instance source payloads while carrying SMART temperature, capacity, health, and identity enrichment. +Physical-disk matching is parent-scoped and topology-aware: persisted source +mappings survive registry rebuilds, controller members sharing a block path +remain distinct, placeholder serials/WWNs never become canonical or metrics +identity, and ambiguous matches fail closed. SMART counter metadata preserves +presence through pointer-valued JSON fields, so reported zero is distinct from +missing. The canonical disk payload also carries provider vendor metadata and +uses explicit SMART health, temperature, and wearout ahead of coarser +contradictory inventory values without manufacturing facts when telemetry is +absent. ## Canonical Files @@ -3460,6 +3469,12 @@ refreshes and Proxmox-scoped views can drop the disk even though inventory still exists. `HostSMARTMeta` must carry `sizeBytes` through adapter, clone, read-state, and API transport so disk capacity remains available after registry rebuilds. +That round trip must also retain provider vendor, controller/target topology, +and explicit zero-valued SMART counters. Registry matching may use usable +serial or WWN only inside a compatible physical-disk parent scope; when those +identifiers are absent or placeholders, host/device/topology fallback identity +is authoritative. An exact seeded source mapping may be reused after restart +only when its resource type and parent scope still agree. That same canonical physical-disk view must also expose source-independent host context. When a disk is API-backed rather than node-backed, typed views should fall back to canonical host identity such as `identity.hostnames` instead of diff --git a/frontend-modern/src/components/Storage/__tests__/diskResourceUtils.test.ts b/frontend-modern/src/components/Storage/__tests__/diskResourceUtils.test.ts index 86d42681a9..1d81f10b31 100644 --- a/frontend-modern/src/components/Storage/__tests__/diskResourceUtils.test.ts +++ b/frontend-modern/src/components/Storage/__tests__/diskResourceUtils.test.ts @@ -61,6 +61,30 @@ describe('diskResourceUtils', () => { instance: 'cluster-main', }), ).toBe(true); + expect( + matchesPhysicalDiskNode(buildDisk(), { + id: 'other-node-id', + name: 'tower', + instance: 'cluster-secondary', + }), + ).toBe(false); + expect( + matchesPhysicalDiskNode(buildDisk(), { + id: 'other-node-id', + name: 'tower', + }), + ).toBe(false); + expect( + matchesPhysicalDiskNode( + buildDisk({ + platformData: { proxmox: { nodeName: 'tower' } } as any, + }), + { + id: 'legacy-node-id', + name: 'tower', + }, + ), + ).toBe(true); }); it('resolves physical disk metric targets through linked agents when needed', () => { diff --git a/frontend-modern/src/components/Storage/diskResourceUtils.ts b/frontend-modern/src/components/Storage/diskResourceUtils.ts index 28419c5458..b5ccb4bcbc 100644 --- a/frontend-modern/src/components/Storage/diskResourceUtils.ts +++ b/frontend-modern/src/components/Storage/diskResourceUtils.ts @@ -48,11 +48,11 @@ export const matchesPhysicalDiskNode = ( const diskInstance = normalize(diskIdentity.instance); const targetInstance = normalize(target.instance); - if (!diskInstance || !targetInstance) { + if (!diskInstance && !targetInstance) { return true; } - return diskInstance === targetInstance; + return Boolean(diskInstance && targetInstance && diskInstance === targetInstance); }; export const resolvePhysicalDiskMetricResourceId = ( diff --git a/frontend-modern/src/features/storageBackups/__tests__/diskPresentation.test.ts b/frontend-modern/src/features/storageBackups/__tests__/diskPresentation.test.ts index 9c1913ecbc..95eb6c27af 100644 --- a/frontend-modern/src/features/storageBackups/__tests__/diskPresentation.test.ts +++ b/frontend-modern/src/features/storageBackups/__tests__/diskPresentation.test.ts @@ -174,7 +174,7 @@ describe('diskPresentation', () => { '', ); expect(getPhysicalDiskLifeLabel(makeDiskData({ wearout: 96 }))).toBe('96%'); - expect(getPhysicalDiskLifeLabel(makeDiskData({ wearout: 0 }))).toBe(''); + expect(getPhysicalDiskLifeLabel(makeDiskData({ wearout: 0 }))).toBe('0%'); expect(getPhysicalDiskLifeLabel(makeDiskData({ wearout: -1 }))).toBe(''); expect(getPhysicalDiskLifeTextClass(makeDiskData({ wearout: 96 }))).toContain('text-green'); expect(getPhysicalDiskLifeTextClass(makeDiskData({ wearout: 35 }))).toContain('text-amber'); @@ -381,6 +381,20 @@ describe('diskPresentation', () => { expect(matchesPhysicalDiskSearch(warningDisk, warningData, 'tank')).toBe(true); expect(matchesPhysicalDiskSearch(warningDisk, warningData, 'node:tower')).toBe(true); expect(matchesPhysicalDiskSearch(warningDisk, warningData, 'node:pve1')).toBe(false); + expect( + matchesPhysicalDiskSearch( + warningDisk, + { + ...warningData, + wwn: '5-c500-da60ca43', + controller: '/dev/sg2', + target: 'sssraid,0,1', + type: 'sas', + instance: 'cluster-a', + }, + '5-c500 sssraid cluster-a sas', + ), + ).toBe(true); expect(getPhysicalDiskSourceKey(warningDisk)).toBe('proxmox-pve'); expect(getPhysicalDiskRoleFilterValue(warningData)).toBe('cache-pool'); expect(normalizePhysicalDiskFacetFilter(' Cache Pool ')).toBe('cache-pool'); diff --git a/frontend-modern/src/features/storageBackups/diskPresentation.ts b/frontend-modern/src/features/storageBackups/diskPresentation.ts index 5002e6f94a..54e00408a7 100644 --- a/frontend-modern/src/features/storageBackups/diskPresentation.ts +++ b/frontend-modern/src/features/storageBackups/diskPresentation.ts @@ -43,6 +43,7 @@ export interface PhysicalDiskPresentationData { instance: string; devPath: string; model: string; + vendor?: string; serial: string; wwn: string; size: number; @@ -304,6 +305,7 @@ export function extractPhysicalDiskPresentationData( instance: diskNode.instance, devPath: pd.devPath || '', model: pd.model || resource.name || '', + vendor: pd.vendor || '', serial: pd.serial || '', wwn: pd.wwn || '', type: pd.diskType || '', @@ -404,9 +406,15 @@ export function matchesPhysicalDiskSearch( if (parsed.freeTerms.length === 0) return true; const haystack = [ disk.model, + disk.vendor, disk.devPath, disk.serial, + disk.wwn, disk.node, + disk.instance, + disk.type, + disk.controller, + disk.target, getPhysicalDiskRoleLabel(disk), getPhysicalDiskParentLabel(disk), getPhysicalDiskPlatformLabel( @@ -534,7 +542,7 @@ export function getPhysicalDiskHealthStatus( const criticalRisk = (disk.riskLevel || '').trim().toLowerCase() === 'critical'; const warningRisk = (disk.riskLevel || '').trim().toLowerCase() === 'warning'; const smartWarning = hasPhysicalDiskSmartWarning(disk); - const lowLife = disk.wearout > 0 && disk.wearout < 10; + const lowLife = disk.wearout >= 0 && disk.wearout < 10; if (normalizedHealth === 'FAILED' || criticalRisk) { return { @@ -625,13 +633,13 @@ export function getPhysicalDiskParentLabel(disk: PhysicalDiskPresentationData): } export function getPhysicalDiskLifeLabel(disk: PhysicalDiskPresentationData): string { - // Wearout is "% life remaining" (100 = new); 0 and -1 both mean unreported. - if (typeof disk.wearout !== 'number' || disk.wearout <= 0) return ''; + // Wearout is "% life remaining" (100 = new); only negative values are unreported. + if (typeof disk.wearout !== 'number' || disk.wearout < 0) return ''; return `${Math.min(disk.wearout, 100)}%`; } export function getPhysicalDiskLifeTextClass(disk: PhysicalDiskPresentationData): string { - if (typeof disk.wearout !== 'number' || disk.wearout <= 0) { + if (typeof disk.wearout !== 'number' || disk.wearout < 0) { return PHYSICAL_DISK_MUTED_PLACEHOLDER_CLASS; } if (disk.wearout < 20) return 'text-red-600 dark:text-red-400'; diff --git a/frontend-modern/src/hooks/__tests__/useUnifiedResources.test.ts b/frontend-modern/src/hooks/__tests__/useUnifiedResources.test.ts index 45e6085e61..0ed894efe9 100644 --- a/frontend-modern/src/hooks/__tests__/useUnifiedResources.test.ts +++ b/frontend-modern/src/hooks/__tests__/useUnifiedResources.test.ts @@ -330,6 +330,63 @@ describe('useUnifiedResources', () => { dispose(); }); + it('loads physical disks beyond the first resource page without dropping wide-node inventory', async () => { + const firstPage = Array.from({ length: 100 }, (_, index) => ({ + ...v2Resource, + id: `disk-${index + 1}`, + type: 'physical_disk', + name: `disk-${index + 1}`, + physicalDisk: { + devPath: `/dev/sd${index + 1}`, + serial: `SERIAL-${index + 1}`, + diskType: index === 0 ? 'nvme' : 'sas', + }, + })); + const secondPage = Array.from({ length: 25 }, (_, index) => ({ + ...v2Resource, + id: `disk-${index + 101}`, + type: 'physical_disk', + name: `disk-${index + 101}`, + physicalDisk: { + devPath: `/dev/sg${index + 101}`, + serial: `SERIAL-${index + 101}`, + diskType: 'sas', + }, + })); + apiFetchMock + .mockResolvedValueOnce({ + ok: true, + status: 200, + json: async () => ({ data: firstPage, meta: { totalPages: 2 } }), + }) + .mockResolvedValueOnce({ + ok: true, + status: 200, + json: async () => ({ data: secondPage, meta: { totalPages: 2 } }), + }); + + let dispose = () => {}; + let result: ReturnType | undefined; + createRoot((d) => { + dispose = d; + result = useUnifiedResources({ + query: 'type=physical_disk', + cacheKey: 'issue-1516-wide-node', + }); + }); + + await waitForResourceCount(() => result!.resources().length, 125); + expect(result!.resources()).toHaveLength(125); + expect(result!.resources().some((resource) => resource.id === 'disk-125')).toBe(true); + expect(apiFetchMock).toHaveBeenNthCalledWith( + 2, + '/api/resources?type=physical_disk&page=2&limit=100', + { cache: 'no-store' }, + ); + + dispose(); + }); + it('prefers websocket initial hydration over an immediate REST fetch for dashboard snapshots', async () => { setWsConnected(false); setWsInitialDataReceived(false); @@ -2045,6 +2102,7 @@ describe('useUnifiedResources', () => { expect(useUnifiedResourcesSource).not.toContain("const DEFAULT_ORG_SCOPE = 'default'"); expect(useUnifiedResourcesSource).not.toContain('const normalizeOrgScope ='); expect(useUnifiedResourcesSource).toContain('asTrimmedString'); + expect(useUnifiedResourcesSource).not.toContain('UNIFIED_RESOURCES_MAX_PAGES'); expect(useUnifiedResourcesSource).not.toContain( 'const asTrimmedString = (value: unknown): string | undefined => {', ); diff --git a/frontend-modern/src/hooks/useUnifiedResources.ts b/frontend-modern/src/hooks/useUnifiedResources.ts index 1bf178ad4d..ae044be61a 100644 --- a/frontend-modern/src/hooks/useUnifiedResources.ts +++ b/frontend-modern/src/hooks/useUnifiedResources.ts @@ -19,6 +19,7 @@ import type { ResourceAvailabilityMeta, ResourcePBSMeta, ResourcePolicyPostureSummary, + ResourcePhysicalDiskMeta, ResourceStatus, ResourceStorageMeta, ResourceStorageRisk, @@ -46,7 +47,6 @@ const DEFAULT_UNIFIED_RESOURCES_QUERY = const STORAGE_RECOVERY_UNIFIED_RESOURCES_QUERY = 'type=storage,pbs,pmg,vm,system-container,pod,agent,k8s-cluster,k8s-node,physical_disk,ceph'; const UNIFIED_RESOURCES_PAGE_LIMIT = 100; -const UNIFIED_RESOURCES_MAX_PAGES = 20; const UNIFIED_RESOURCES_CACHE_MAX_AGE_MS = 15_000; const UNIFIED_RESOURCES_WS_DEBOUNCE_MS = 800; const UNIFIED_RESOURCES_WS_MIN_REFETCH_INTERVAL_MS = 2_500; @@ -404,31 +404,7 @@ type APIResource = { availabilityChecks?: ResourceAvailabilityMeta[]; recentChanges?: ResourceChange[]; facetCounts?: ResourceFacetCounts; - physicalDisk?: { - devPath?: string; - model?: string; - serial?: string; - wwn?: string; - diskType?: string; - sizeBytes?: number; - health?: string; - wearout?: number; - temperature?: number; - rpm?: number; - used?: string; - smart?: { - powerOnHours?: number; - powerCycles?: number; - reallocatedSectors?: number; - pendingSectors?: number; - offlineUncorrectable?: number; - udmaCrcErrors?: number; - percentageUsed?: number; - availableSpare?: number; - mediaErrors?: number; - unsafeShutdowns?: number; - }; - }; + physicalDisk?: ResourcePhysicalDiskMeta; ceph?: { fsid?: string; healthStatus?: string; @@ -1098,7 +1074,7 @@ async function fetchUnifiedResources(query: string): Promise 0 && row.wearout < 20) || + ((row.wearout > 0 || + (row.wearout == 0 && + (strings.EqualFold(row.diskType, "ssd") || strings.EqualFold(row.diskType, "nvme")))) && + row.wearout < 20) || row.temperature > 55 || len(row.smartEvidence) > 0 } @@ -2810,11 +2813,11 @@ func unifiedPhysicalDiskSMARTIssueParts(attrs *unifiedresources.SMARTMeta) []str } var parts []string - appendSMARTInt64ValueIssue(&parts, "reallocated sectors", attrs.ReallocatedSectors) - appendSMARTInt64ValueIssue(&parts, "pending sectors", attrs.PendingSectors) - appendSMARTInt64ValueIssue(&parts, "offline uncorrectable", attrs.OfflineUncorrectable) - appendSMARTInt64ValueIssue(&parts, "UDMA CRC errors", attrs.UDMACRCErrors) - appendSMARTInt64ValueIssue(&parts, "media errors", attrs.MediaErrors) + appendSMARTInt64Issue(&parts, "reallocated sectors", attrs.ReallocatedSectors) + appendSMARTInt64Issue(&parts, "pending sectors", attrs.PendingSectors) + appendSMARTInt64Issue(&parts, "offline uncorrectable", attrs.OfflineUncorrectable) + appendSMARTInt64Issue(&parts, "UDMA CRC errors", attrs.UDMACRCErrors) + appendSMARTInt64Issue(&parts, "media errors", attrs.MediaErrors) return parts } diff --git a/internal/ai/patrol_ai_more_test.go b/internal/ai/patrol_ai_more_test.go index 2f5e73cac3..b8696e8d52 100644 --- a/internal/ai/patrol_ai_more_test.go +++ b/internal/ai/patrol_ai_more_test.go @@ -1096,7 +1096,7 @@ func TestSeedHealthAndAlerts_SMARTAttributeIssue(t *testing.T) { Wearout: 100, Temperature: 40, SMART: &unifiedresources.SMARTMeta{ - PendingSectors: 2, + PendingSectors: patrolSMARTInt64Pointer(2), }, }, }, @@ -1113,6 +1113,25 @@ func TestSeedHealthAndAlerts_SMARTAttributeIssue(t *testing.T) { } } +func patrolSMARTInt64Pointer(value int64) *int64 { return &value } + +func TestPatrolPhysicalDiskHealthIssueDistinguishesKnownZeroWearout(t *testing.T) { + if !patrolPhysicalDiskHealthIssue(patrolPhysicalDiskRow{ + diskType: "nvme", + health: "PASSED", + wearout: 0, + }) { + t.Fatal("explicit zero NVMe life should be a health issue") + } + if patrolPhysicalDiskHealthIssue(patrolPhysicalDiskRow{ + diskType: "sata", + health: "UNKNOWN", + wearout: 0, + }) { + t.Fatal("unreported SATA wearout should remain neutral") + } +} + func TestSeedHealthAndAlerts_WithIssues(t *testing.T) { ps := NewPatrolService(nil, nil) cfg := DefaultPatrolConfig() diff --git a/internal/api/metrics_history_fallback_test.go b/internal/api/metrics_history_fallback_test.go index 1dd102fe99..f9296eaefa 100644 --- a/internal/api/metrics_history_fallback_test.go +++ b/internal/api/metrics_history_fallback_test.go @@ -28,6 +28,86 @@ type metricsHistoryResponse struct { } `json:"points"` } +type staticPhysicalDiskResourceStore struct { + resources []unifiedresources.Resource +} + +func (store staticPhysicalDiskResourceStore) ShouldSkipAPIPolling(string) bool { + return false +} + +func (store staticPhysicalDiskResourceStore) GetPollingRecommendations() map[string]float64 { + return nil +} + +func (store staticPhysicalDiskResourceStore) GetAll() []unifiedresources.Resource { + return append([]unifiedresources.Resource(nil), store.resources...) +} + +func (staticPhysicalDiskResourceStore) PopulateFromSnapshot(models.StateSnapshot) {} + +func TestMetricsHistoryPhysicalDiskIdentityAliasesFailClosedWhenAmbiguous(t *testing.T) { + resources := []unifiedresources.Resource{ + { + ID: "physical-disk:node-a:sda", + Type: unifiedresources.ResourceTypePhysicalDisk, + PhysicalDisk: &unifiedresources.PhysicalDiskMeta{ + Serial: "DUPLICATE-SERIAL", + Temperature: 31, + }, + MetricsTarget: &unifiedresources.MetricsTarget{ + ResourceType: "disk", + ResourceID: "DUPLICATE-SERIAL", + }, + }, + { + ID: "physical-disk:node-b:sda", + Type: unifiedresources.ResourceTypePhysicalDisk, + PhysicalDisk: &unifiedresources.PhysicalDiskMeta{ + Serial: "DUPLICATE-SERIAL", + Temperature: 52, + }, + MetricsTarget: &unifiedresources.MetricsTarget{ + ResourceType: "disk", + ResourceID: "DUPLICATE-SERIAL", + }, + }, + } + monitor := &monitoring.Monitor{} + monitor.SetResourceStore(staticPhysicalDiskResourceStore{resources: resources}) + setUnexportedField(t, monitor, "metricsHistory", monitoring.NewMetricsHistory(10, time.Hour)) + router := &Router{monitor: monitor} + + req := httptest.NewRequest( + http.MethodGet, + "/api/metrics-store/history?resourceType=disk&resourceId=DUPLICATE-SERIAL&metric=smart_temp&range=5m", + nil, + ) + rec := httptest.NewRecorder() + router.handleMetricsHistory(rec, req) + if rec.Code != http.StatusServiceUnavailable { + t.Fatalf("ambiguous identity should not attach either live temperature: status=%d body=%s", rec.Code, rec.Body.String()) + } + + req = httptest.NewRequest( + http.MethodGet, + "/api/metrics-store/history?resourceType=disk&resourceId=physical-disk:node-b:sda&metric=smart_temp&range=5m", + nil, + ) + rec = httptest.NewRecorder() + router.handleMetricsHistory(rec, req) + if rec.Code != http.StatusOK { + t.Fatalf("exact canonical identity should resolve: status=%d body=%s", rec.Code, rec.Body.String()) + } + var response metricsHistoryResponse + if err := json.Unmarshal(rec.Body.Bytes(), &response); err != nil { + t.Fatalf("decode exact disk response: %v", err) + } + if len(response.Points) != 1 || response.Points[0].Value != 52 { + t.Fatalf("exact canonical identity resolved wrong disk: %+v", response.Points) + } +} + func TestMetricsHistoryFallbackUsesLivePoint(t *testing.T) { state := models.NewState() vm := models.VM{ diff --git a/internal/api/router.go b/internal/api/router.go index befdea9996..250722d628 100644 --- a/internal/api/router.go +++ b/internal/api/router.go @@ -60,6 +60,7 @@ import ( "github.com/rcourtman/pulse-go-rewrite/pkg/aicontracts" "github.com/rcourtman/pulse-go-rewrite/pkg/auth" internalauth "github.com/rcourtman/pulse-go-rewrite/pkg/auth" + "github.com/rcourtman/pulse-go-rewrite/pkg/diskinventory" "github.com/rcourtman/pulse-go-rewrite/pkg/extensions" metricstore "github.com/rcourtman/pulse-go-rewrite/pkg/metrics" "github.com/rcourtman/pulse-go-rewrite/pkg/reporting" @@ -9204,21 +9205,44 @@ func (r *Router) handleMetricsHistory(w http.ResponseWriter, req *http.Request) } for i := range physicalDisks { - disk := &physicalDisks[i] - serial := strings.TrimSpace(disk.PhysicalDisk.Serial) - wwn := strings.TrimSpace(disk.PhysicalDisk.WWN) - metricsTargetID := "" - if disk.MetricsTarget != nil { - metricsTargetID = strings.TrimSpace(disk.MetricsTarget.ResourceID) - } - if strings.EqualFold(disk.ID, target) || - (metricsTargetID != "" && strings.EqualFold(metricsTargetID, target)) || - (serial != "" && strings.EqualFold(serial, target)) || - (wwn != "" && strings.EqualFold(wwn, target)) { - return disk + if strings.EqualFold(physicalDisks[i].ID, target) { + return &physicalDisks[i] } } - return nil + uniqueMatch := func(predicate func(*unifiedresources.Resource) bool) *unifiedresources.Resource { + matchIndex := -1 + for index := range physicalDisks { + if !predicate(&physicalDisks[index]) { + continue + } + if matchIndex >= 0 { + return nil + } + matchIndex = index + } + if matchIndex < 0 { + return nil + } + return &physicalDisks[matchIndex] + } + if disk := uniqueMatch(func(candidate *unifiedresources.Resource) bool { + return candidate.MetricsTarget != nil && + strings.EqualFold(strings.TrimSpace(candidate.MetricsTarget.ResourceID), target) + }); disk != nil { + return disk + } + if !diskinventory.IsUsableHardwareID(target) { + return nil + } + return uniqueMatch(func(candidate *unifiedresources.Resource) bool { + if candidate.PhysicalDisk == nil { + return false + } + serial := strings.TrimSpace(candidate.PhysicalDisk.Serial) + wwn := strings.TrimSpace(candidate.PhysicalDisk.WWN) + return (diskinventory.IsUsableHardwareID(serial) && strings.EqualFold(serial, target)) || + (diskinventory.IsUsableHardwareID(wwn) && strings.EqualFold(wwn, target)) + }) } liveMetricPoints := func(resourceType, resourceID string) map[string]monitoring.MetricPoint { @@ -9345,35 +9369,35 @@ func (r *Router) handleMetricsHistory(w http.ResponseWriter, req *http.Request) } if pd.SMART != nil { s := pd.SMART - if s.PowerOnHours > 0 { - points["smart_power_on_hours"] = monitoring.MetricPoint{Timestamp: now, Value: float64(s.PowerOnHours)} + if s.PowerOnHours != nil { + points["smart_power_on_hours"] = monitoring.MetricPoint{Timestamp: now, Value: float64(*s.PowerOnHours)} } - if s.PowerCycles > 0 { - points["smart_power_cycles"] = monitoring.MetricPoint{Timestamp: now, Value: float64(s.PowerCycles)} + if s.PowerCycles != nil { + points["smart_power_cycles"] = monitoring.MetricPoint{Timestamp: now, Value: float64(*s.PowerCycles)} } - if s.ReallocatedSectors > 0 { - points["smart_reallocated_sectors"] = monitoring.MetricPoint{Timestamp: now, Value: float64(s.ReallocatedSectors)} + if s.ReallocatedSectors != nil { + points["smart_reallocated_sectors"] = monitoring.MetricPoint{Timestamp: now, Value: float64(*s.ReallocatedSectors)} } - if s.PendingSectors > 0 { - points["smart_pending_sectors"] = monitoring.MetricPoint{Timestamp: now, Value: float64(s.PendingSectors)} + if s.PendingSectors != nil { + points["smart_pending_sectors"] = monitoring.MetricPoint{Timestamp: now, Value: float64(*s.PendingSectors)} } - if s.OfflineUncorrectable > 0 { - points["smart_offline_uncorrectable"] = monitoring.MetricPoint{Timestamp: now, Value: float64(s.OfflineUncorrectable)} + if s.OfflineUncorrectable != nil { + points["smart_offline_uncorrectable"] = monitoring.MetricPoint{Timestamp: now, Value: float64(*s.OfflineUncorrectable)} } - if s.UDMACRCErrors > 0 { - points["smart_crc_errors"] = monitoring.MetricPoint{Timestamp: now, Value: float64(s.UDMACRCErrors)} + if s.UDMACRCErrors != nil { + points["smart_crc_errors"] = monitoring.MetricPoint{Timestamp: now, Value: float64(*s.UDMACRCErrors)} } - if s.PercentageUsed > 0 { - points["smart_percentage_used"] = monitoring.MetricPoint{Timestamp: now, Value: float64(s.PercentageUsed)} + if s.PercentageUsed != nil { + points["smart_percentage_used"] = monitoring.MetricPoint{Timestamp: now, Value: float64(*s.PercentageUsed)} } - if s.AvailableSpare > 0 { - points["smart_available_spare"] = monitoring.MetricPoint{Timestamp: now, Value: float64(s.AvailableSpare)} + if s.AvailableSpare != nil { + points["smart_available_spare"] = monitoring.MetricPoint{Timestamp: now, Value: float64(*s.AvailableSpare)} } - if s.MediaErrors > 0 { - points["smart_media_errors"] = monitoring.MetricPoint{Timestamp: now, Value: float64(s.MediaErrors)} + if s.MediaErrors != nil { + points["smart_media_errors"] = monitoring.MetricPoint{Timestamp: now, Value: float64(*s.MediaErrors)} } - if s.UnsafeShutdowns > 0 { - points["smart_unsafe_shutdowns"] = monitoring.MetricPoint{Timestamp: now, Value: float64(s.UnsafeShutdowns)} + if s.UnsafeShutdowns != nil { + points["smart_unsafe_shutdowns"] = monitoring.MetricPoint{Timestamp: now, Value: float64(*s.UnsafeShutdowns)} } } } diff --git a/internal/api/unified_agent_handlers_additional_test.go b/internal/api/unified_agent_handlers_additional_test.go index 22f72a67a0..9ca5c60565 100644 --- a/internal/api/unified_agent_handlers_additional_test.go +++ b/internal/api/unified_agent_handlers_additional_test.go @@ -6,15 +6,18 @@ import ( "crypto/rand" "encoding/base64" "encoding/json" + "fmt" "net/http" "net/http/httptest" "sync" "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/monitoring" "github.com/rcourtman/pulse-go-rewrite/internal/remoteconfig" + "github.com/rcourtman/pulse-go-rewrite/internal/utils" agentshost "github.com/rcourtman/pulse-go-rewrite/pkg/agents/host" ) @@ -150,6 +153,68 @@ func TestUnifiedAgentHandlers_HandleReportIncludesConfigOverride(t *testing.T) { } } +func TestUnifiedAgentHandlers_HandleGzipReportPreservesWideSMARTInventory(t *testing.T) { + handler, monitor := newUnifiedAgentHandlers(t, nil) + smart := make([]agentshost.DiskSMART, 128) + for index := range smart { + powerOnHours := int64(1000 + index) + percentageUsed := index % 100 + smart[index] = agentshost.DiskSMART{ + Device: fmt.Sprintf("sg%d", index), + Model: "ST18000NM019J", + Serial: fmt.Sprintf("ZR5D%04d", index), + WWN: fmt.Sprintf("5-c500-da60-%04d", index), + Type: "sas", + Controller: fmt.Sprintf("/dev/sg%d", index/24), + Target: fmt.Sprintf("sssraid,0,%d", index), + SizeBytes: 18000207937536, + Temperature: 30 + index%8, + Health: "PASSED", + Attributes: &agentshost.SMARTAttributes{ + PowerOnHours: &powerOnHours, + PercentageUsed: &percentageUsed, + }, + } + } + report := agentshost.Report{ + Agent: agentshost.AgentInfo{ID: "agent-wide", Version: "6.1.1"}, + Host: agentshost.HostInfo{ + ID: "machine-wide", + Hostname: "pve-wide.local", + Platform: "linux", + }, + Sensors: agentshost.Sensors{SMART: smart}, + Timestamp: time.Now().UTC(), + } + raw, err := json.Marshal(report) + if err != nil { + t.Fatalf("marshal wide report: %v", err) + } + compressed, err := utils.CompressJSON(raw) + if err != nil { + t.Fatalf("compress wide report: %v", err) + } + + req := httptest.NewRequest(http.MethodPost, "/api/agents/agent/report", bytes.NewReader(compressed)) + req.Header.Set("Content-Encoding", "gzip") + rec := httptest.NewRecorder() + handler.HandleReport(rec, req) + if rec.Code != http.StatusOK { + t.Fatalf("wide report status = %d, want 200: %s", rec.Code, rec.Body.String()) + } + + hosts := monitor.GetLiveHostsSnapshot() + if len(hosts) != 1 { + t.Fatalf("hosts = %d, want 1", len(hosts)) + } + if got := len(hosts[0].Sensors.SMART); got != len(smart) { + t.Fatalf("SMART inventory was truncated: got %d disks, want %d", got, len(smart)) + } + if last := hosts[0].Sensors.SMART[len(smart)-1]; last.Serial != "ZR5D0127" || last.Target != "sssraid,0,127" { + t.Fatalf("last SMART entry was corrupted or dropped: %+v", last) + } +} + func TestUnifiedAgentHandlers_HandleReportSuppressesConfigOverrideForUnboundExecToken(t *testing.T) { handler, monitor := newUnifiedAgentHandlers(t, nil) diff --git a/internal/hostagent/collector_observability_test.go b/internal/hostagent/collector_observability_test.go index 24aa536c43..ca43cbd6be 100644 --- a/internal/hostagent/collector_observability_test.go +++ b/internal/hostagent/collector_observability_test.go @@ -120,7 +120,7 @@ func TestCollectDeviceSMART_LogsStructuredContextWhenDeviceInStandby(t *testing. execLookPath = func(string) (string, error) { return "smartctl", nil } smartRunCommandOutput = func(ctx context.Context, name string, args ...string) ([]byte, error) { - return exec.CommandContext(ctx, "sh", "-c", "exit 2").Output() + return exec.CommandContext(ctx, "sh", "-c", "exit 3").Output() } logOutput := captureSmartctlLogs(t) diff --git a/internal/hostagent/issue1516_physical_disk_regression_test.go b/internal/hostagent/issue1516_physical_disk_regression_test.go new file mode 100644 index 0000000000..53340c07d8 --- /dev/null +++ b/internal/hostagent/issue1516_physical_disk_regression_test.go @@ -0,0 +1,220 @@ +//go:build !windows + +package hostagent + +import ( + "context" + "os/exec" + "testing" + + "github.com/rcourtman/pulse-go-rewrite/pkg/diskinventory" +) + +func TestIssue1516PermissionFailureRemainsUnknownAndKeepsSysfsIdentity(t *testing.T) { + if _, err := exec.LookPath("sh"); err != nil { + t.Skip("sh not available") + } + stubLinuxSysfs(t, + []string{"sda"}, + map[string]string{ + "/sys/block/sda/size": "937703088\n", + "/sys/block/sda/device/model": "Permission-limited disk\n", + "/sys/block/sda/device/wwid": "naa.5000c500abcdef01\n", + }, + ) + + origRun := smartRunCommandOutput + origLook := execLookPath + t.Cleanup(func() { + smartRunCommandOutput = origRun + execLookPath = origLook + }) + execLookPath = func(string) (string, error) { return "smartctl", nil } + smartRunCommandOutput = func(ctx context.Context, name string, args ...string) ([]byte, error) { + if len(args) == 1 && args[0] == "--scan-open" { + return nil, nil + } + return exec.CommandContext(ctx, "sh", "-c", "exit 2").Output() + } + + results, err := CollectSMARTLocal(context.Background(), nil) + if err != nil { + t.Fatalf("CollectSMARTLocal() error = %v", err) + } + if len(results) != 1 { + t.Fatalf("disk count = %d, want 1: %+v", len(results), results) + } + disk := results[0] + if disk.Device != "sda" || disk.Model != "Permission-limited disk" || + disk.WWN != "naa.5000c500abcdef01" || disk.SizeBytes != 480103981056 { + t.Fatalf("sysfs identity was not preserved: %+v", disk) + } + if disk.Standby || disk.Health != "UNKNOWN" || disk.Type != "" || disk.Temperature != 0 { + t.Fatalf("permission failure fabricated disk state: %+v", disk) + } + if disk.Collection == nil || + disk.Collection.Serial.State != diskinventory.FieldMissing || + disk.Collection.Temperature.State != diskinventory.FieldUnavailable { + t.Fatalf("collection status = %+v, want missing serial and unavailable temperature", disk.Collection) + } +} + +func TestIssue1516StandbyDiskKeepsStableIdentityWithoutSMARTClaims(t *testing.T) { + if _, err := exec.LookPath("sh"); err != nil { + t.Skip("sh not available") + } + stubLinuxSysfs(t, + []string{"sdb"}, + map[string]string{ + "/sys/block/sdb/size": "35156656128\n", + "/sys/block/sdb/device/model": "ST18000NM000J\n", + "/sys/block/sdb/device/serial": "ZR5STANDBY\n", + "/sys/block/sdb/device/wwid": "naa.5000c500standby1\n", + "/sys/block/sdb/device/protocol": "SAS\n", + "/sys/block/sdb/queue/rotational": "1\n", + }, + ) + + origRun := smartRunCommandOutput + origLook := execLookPath + t.Cleanup(func() { + smartRunCommandOutput = origRun + execLookPath = origLook + }) + execLookPath = func(string) (string, error) { return "smartctl", nil } + smartRunCommandOutput = func(ctx context.Context, name string, args ...string) ([]byte, error) { + if len(args) == 1 && args[0] == "--scan-open" { + return []byte("/dev/sdb -d scsi # sleeping SAS disk\n"), nil + } + return exec.CommandContext(ctx, "sh", "-c", "exit 3").Output() + } + + results, err := CollectSMARTLocal(context.Background(), nil) + if err != nil { + t.Fatalf("CollectSMARTLocal() error = %v", err) + } + if len(results) != 1 { + t.Fatalf("disk count = %d, want 1: %+v", len(results), results) + } + disk := results[0] + if !disk.Standby || disk.Device != "sdb" || disk.Model != "ST18000NM000J" || + disk.Serial != "ZR5STANDBY" || disk.WWN != "naa.5000c500standby1" || + disk.Type != "sas" || disk.SizeBytes != 18000207937536 { + t.Fatalf("standby disk identity was lost: %+v", disk) + } + if disk.Health != "" || disk.Temperature != 0 || disk.Attributes != nil { + t.Fatalf("standby disk fabricated SMART evidence: %+v", disk) + } + if disk.Collection == nil || + disk.Collection.Serial.State != diskinventory.FieldAvailable || + disk.Collection.Temperature.State != diskinventory.FieldUnavailable { + t.Fatalf("standby collection status = %+v", disk.Collection) + } +} + +func TestIssue1516SATRetryMergesTemperatureWithoutDiscardingIdentity(t *testing.T) { + stubLinuxSysfs(t, + []string{"sdc"}, + map[string]string{ + "/sys/block/sdc/size": "937703088\n", + "/sys/block/sdc/queue/rotational": "0\n", + }, + ) + + origRun := smartRunCommandOutput + origLook := execLookPath + t.Cleanup(func() { + smartRunCommandOutput = origRun + execLookPath = origLook + }) + execLookPath = func(string) (string, error) { return "smartctl", nil } + + var attempts int + smartRunCommandOutput = func(ctx context.Context, name string, args ...string) ([]byte, error) { + attempts++ + if len(args) >= 2 && args[0] == "-d" && args[1] == "sat" { + return []byte(`{ + "device":{"name":"/dev/sdc","type":"sat","protocol":"ATA"}, + "temperature":{"current":54} + }`), nil + } + return []byte(`{ + "device":{"name":"/dev/sdc","type":"scsi","protocol":"SCSI"}, + "model_name":"Crucial BX500 480GB", + "serial_number":"2110E5A8FEE7", + "smart_status":{"passed":true} + }`), nil + } + + result, err := collectSMARTTarget(context.Background(), smartctlTarget{Path: "/dev/sdc"}) + if err != nil { + t.Fatalf("collectSMARTTarget() error = %v", err) + } + if attempts != 2 { + t.Fatalf("attempt count = %d, want auto then SAT", attempts) + } + if result.Model != "Crucial BX500 480GB" || result.Serial != "2110E5A8FEE7" || + result.Health != "PASSED" || result.Type != "sata" || result.Temperature != 54 { + t.Fatalf("retry evidence was not merged: %+v", result) + } +} + +func TestIssue1516SMARTAttemptMergeKeepsFailureEvidence(t *testing.T) { + firstPassed := mergeSMARTAttemptEvidence( + &DiskSMART{Health: "PASSED"}, + &DiskSMART{Health: "FAILED"}, + ) + if firstPassed.Health != "FAILED" { + t.Fatalf("later SMART failure was discarded: %+v", firstPassed) + } + firstFailed := mergeSMARTAttemptEvidence( + &DiskSMART{Health: "FAILED"}, + &DiskSMART{Health: "PASSED"}, + ) + if firstFailed.Health != "FAILED" { + t.Fatalf("later PASSED result erased SMART failure: %+v", firstFailed) + } +} + +func TestIssue1516TemperaturePrecedenceAndPartialNVMeRemainNeutral(t *testing.T) { + result, err := parseSMARTOutput([]byte(`{ + "device":{"name":"/dev/sda","type":"sat","protocol":"ATA"}, + "model_name":"Crucial BX500 480GB", + "serial_number":"2110E5A8FEE7", + "smart_status":{"passed":true}, + "temperature":{"current":400}, + "nvme_smart_health_information_log":{"temperature":350}, + "ata_sct_status":{"current":{"value":250}}, + "ata_smart_attributes":{"table":[ + {"id":190,"raw":{"value":39,"string":"39"}}, + {"id":194,"raw":{"value":0,"string":"54 (Min/Max 19/57)"}} + ]} + }`), smartctlTarget{Path: "/dev/sda", DeviceType: "sat"}) + if err != nil { + t.Fatalf("parse SATA SMART output: %v", err) + } + if result.Temperature != 54 { + t.Fatalf("temperature = %d, want valid ATA 194 value 54", result.Temperature) + } + + nvme, err := parseSMARTOutput([]byte(`{ + "device":{"name":"/dev/nvme0","type":"nvme","protocol":"NVMe"}, + "model_name":"ShiJi 128GB", + "serial_number":"NVME-BOOT-1", + "smart_status":{"passed":true}, + "nvme_smart_health_information_log":{"temperature":37} + }`), smartctlTarget{Path: "/dev/nvme0", DeviceType: "nvme"}) + if err != nil { + t.Fatalf("parse partial NVMe SMART output: %v", err) + } + if nvme.Temperature != 37 || nvme.Attributes != nil { + t.Fatalf("partial NVMe log fabricated counters: %+v", nvme) + } +} + +func TestIssue1516FirstNVMeNamespaceUsesNumericOrder(t *testing.T) { + stubLinuxSysfs(t, []string{"nvme0n10", "nvme0n2", "nvme0n1p1"}, nil) + if got := firstNVMeNamespace("nvme0"); got != "nvme0n2" { + t.Fatalf("firstNVMeNamespace() = %q, want nvme0n2", got) + } +} diff --git a/internal/hostagent/smartctl.go b/internal/hostagent/smartctl.go index 7d600de2bd..a310809395 100644 --- a/internal/hostagent/smartctl.go +++ b/internal/hostagent/smartctl.go @@ -85,13 +85,13 @@ type SMARTAttributes struct { } type nvmeSmartHealthInformationLogJSON struct { - Temperature int `json:"temperature"` - AvailableSpare int `json:"available_spare"` - PercentageUsed int `json:"percentage_used"` - PowerOnHours int64 `json:"power_on_hours"` - UnsafeShutdowns int64 `json:"unsafe_shutdowns"` - MediaErrors int64 `json:"media_errors"` - PowerCycles int64 `json:"power_cycles"` + Temperature int `json:"temperature"` + AvailableSpare *int `json:"available_spare"` + PercentageUsed *int `json:"percentage_used"` + PowerOnHours *int64 `json:"power_on_hours"` + UnsafeShutdowns *int64 `json:"unsafe_shutdowns"` + MediaErrors *int64 `json:"media_errors"` + PowerCycles *int64 `json:"power_cycles"` } type lsblkJSON struct { @@ -321,9 +321,6 @@ func CollectSMARTLocal(ctx context.Context, diskExclude []string) ([]DiskSMART, continue } refineLinuxBlockDeviceIdentity(smart, target) - if smart.Type == "" { - smart.Type = "sata" - } // The refine step can rename the device (nvme0 -> nvme0n1), so re-apply // exclusions against the canonical name the user actually sees. if matchesDeviceExclude(smart.Device, "/dev/"+smart.Device, diskExclude) { @@ -880,6 +877,21 @@ func refineLinuxBlockDeviceIdentity(smart *DiskSMART, target smartctlTarget) { smart.Type = evidence } } + if smart.Model == "" { + smart.Model = readTrimmedFile(filepath.Join("/sys/block", block, "device", "model")) + } + if smart.Serial == "" { + smart.Serial = readTrimmedFile(filepath.Join("/sys/block", block, "device", "serial")) + if smart.Serial != "" { + if smart.Collection == nil { + smart.Collection = &diskinventory.CollectionStatus{} + } + smart.Collection.Serial = diskinventory.Available("sysfs") + } + } + if smart.WWN == "" { + smart.WWN = linuxBlockDeviceWWID(block) + } if size := blockDeviceSizeBytes(block); size > 0 { smart.SizeBytes = size } @@ -974,13 +986,23 @@ func linuxBlockDeviceTransport(block string, target smartctlTarget) string { case strings.HasPrefix(deviceType, "sat"): return "sata" default: - // Preserve the legacy direct sdX fallback when the kernel supplies no - // transport evidence. Successful smartctl probes still override this - // with their protocol field before this fallback is needed. - return "sata" + return "" } } +func linuxBlockDeviceWWID(block string) string { + for _, candidate := range []string{ + filepath.Join("/sys/block", block, "device", "wwid"), + filepath.Join("/sys/block", block, "wwid"), + filepath.Join("/sys/block", block, "device", "wwn"), + } { + if value := readTrimmedFile(candidate); value != "" { + return value + } + } + return "" +} + // linuxIdentityOnlyDisks builds identity-only entries for physical disks whose // SMART probes produced nothing usable. A real disk that refuses SMART must // still be listed — Proxmox's own disks/list shows it, and a monitoring view @@ -1045,6 +1067,7 @@ func linuxIdentityOnlyDisks(missed []smartctlTarget, collected, multiplexed map[ Device: block, Model: readTrimmedFile(filepath.Join("/sys/block", block, "device", "model")), Serial: serial, + WWN: linuxBlockDeviceWWID(block), Type: linuxBlockDeviceTransport(block, target), Controller: controller, Target: controllerTarget, @@ -1064,8 +1087,8 @@ func linuxIdentityOnlyDisks(missed []smartctlTarget, collected, multiplexed map[ } // isMultiplexedDeviceType reports whether a smartctl -d type addresses a member -// disk behind a controller (e.g. "megaraid,7"), as opposed to a directly -// attached device ("", "sat", "nvme", "scsi", "sat,auto"). +// disk behind a controller (for example "megaraid,7", "areca,1/1", or +// "sssraid,0,1"), as opposed to a directly attached device. func isMultiplexedDeviceType(deviceType string) bool { idx := strings.IndexByte(deviceType, ',') if idx < 0 || idx+1 >= len(deviceType) { @@ -1115,7 +1138,15 @@ func firstNVMeNamespace(controller string) string { if suffix := name[len(prefix):]; suffix == "" || !isAllDigits(suffix) { continue } - if best == "" || name < best { + number, err := strconv.Atoi(name[len(prefix):]) + if err != nil { + continue + } + bestNumber := 0 + if best != "" { + bestNumber, _ = strconv.Atoi(best[len(prefix):]) + } + if best == "" || number < bestNumber { best = name } } @@ -1231,7 +1262,9 @@ func collectSMARTTarget(ctx context.Context, target smartctlTarget) (*DiskSMART, var exitErr *exec.ExitError if errors.As(err, &exitErr) { exitCode := exitErr.ExitCode() - if (exitCode == smartctlStandbyExitStatus || exitCode&2 != 0) && len(output) == 0 { + if exitCode == smartctlStandbyExitStatus && + len(output) == 0 && + smartctlArgsUseStandbyExitStatus(args) { standbyResult := &DiskSMART{ Device: filepath.Base(target.Path), Standby: true, @@ -1247,16 +1280,6 @@ func collectSMARTTarget(ctx context.Context, target smartctlTarget) (*DiskSMART, } continue } - // -n standby,3 makes a real standby exit 3; exit bit 2 with - // no output is an open/identify failure, not standby. When - // another attempt remains (Linux untyped retry), try it - // before reporting a phantom standby disk. - if runtimeGOOS == "linux" && i < len(attempts)-1 && exitCode != smartctlStandbyExitStatus { - if firstStandby == nil { - firstStandby = standbyResult - } - continue - } log.Debug(). Str("component", smartctlComponent). Str("action", "device_in_standby"). @@ -1288,6 +1311,8 @@ func collectSMARTTarget(ctx context.Context, target smartctlTarget) (*DiskSMART, result = enrichFreeBSDSCTTemperature(cmdCtx, smartctlPath, args, target, result) if firstParsed == nil { firstParsed = result + } else { + firstParsed = mergeSMARTAttemptEvidence(firstParsed, result) } if !shouldRetrySMARTTarget(target.Path, result, i, len(attempts)) { log.Debug(). @@ -1299,7 +1324,7 @@ func collectSMARTTarget(ctx context.Context, target smartctlTarget) (*DiskSMART, Int("temperature", result.Temperature). Str("health", result.Health). Msg("collected SMART data") - return result, nil + return firstParsed, nil } } @@ -1329,6 +1354,109 @@ func collectSMARTTarget(ctx context.Context, target smartctlTarget) (*DiskSMART, return nil, errSMARTDataUnavailable } +func smartctlArgsUseStandbyExitStatus(args []string) bool { + want := "standby," + strconv.Itoa(smartctlStandbyExitStatus) + for index := 0; index+1 < len(args); index++ { + if args[index] == "-n" && args[index+1] == want { + return true + } + } + return false +} + +func mergeSMARTAttemptEvidence(base, incoming *DiskSMART) *DiskSMART { + if base == nil { + return incoming + } + if incoming == nil { + return base + } + if base.Model == "" { + base.Model = incoming.Model + } + if base.Serial == "" { + base.Serial = incoming.Serial + } + if base.WWN == "" { + base.WWN = incoming.WWN + } + if base.SizeBytes <= 0 { + base.SizeBytes = incoming.SizeBytes + } + if base.Type == "" || base.Type == "scsi" { + if incoming.Type != "" { + base.Type = incoming.Type + } + } + if base.Controller == "" { + base.Controller = incoming.Controller + } + if base.Target == "" { + base.Target = incoming.Target + } + if base.Temperature <= 0 && incoming.Temperature > 0 { + base.Temperature = incoming.Temperature + } + if shouldReplaceSMARTAttemptHealth(base.Health, incoming.Health) { + base.Health = incoming.Health + } + base.Attributes = mergeSMARTAttributes(base.Attributes, incoming.Attributes) + base.Collection = diskinventory.MergeStatus(base.Collection, incoming.Collection) + return base +} + +func shouldReplaceSMARTAttemptHealth(existing, incoming string) bool { + incoming = strings.ToUpper(strings.TrimSpace(incoming)) + if incoming == "" || incoming == "UNKNOWN" { + return false + } + existing = strings.ToUpper(strings.TrimSpace(existing)) + if incoming == "FAILED" { + return true + } + return existing == "" || existing == "UNKNOWN" +} + +func mergeSMARTAttributes(base, incoming *SMARTAttributes) *SMARTAttributes { + if base == nil { + return incoming + } + if incoming == nil { + return base + } + if base.PowerOnHours == nil { + base.PowerOnHours = incoming.PowerOnHours + } + if base.PowerCycles == nil { + base.PowerCycles = incoming.PowerCycles + } + if base.ReallocatedSectors == nil { + base.ReallocatedSectors = incoming.ReallocatedSectors + } + if base.PendingSectors == nil { + base.PendingSectors = incoming.PendingSectors + } + if base.OfflineUncorrectable == nil { + base.OfflineUncorrectable = incoming.OfflineUncorrectable + } + if base.UDMACRCErrors == nil { + base.UDMACRCErrors = incoming.UDMACRCErrors + } + if base.PercentageUsed == nil { + base.PercentageUsed = incoming.PercentageUsed + } + if base.AvailableSpare == nil { + base.AvailableSpare = incoming.AvailableSpare + } + if base.MediaErrors == nil { + base.MediaErrors = incoming.MediaErrors + } + if base.UnsafeShutdowns == nil { + base.UnsafeShutdowns = incoming.UnsafeShutdowns + } + return base +} + func smartctlProbeAttempts(target smartctlTarget) [][]string { device := target.Path if target.DeviceType != "" { @@ -1535,21 +1663,27 @@ func parseSMARTOutput(output []byte, target smartctlTarget) (*DiskSMART, error) result.SizeBytes = smartData.UserCapacity.Bytes } - if smartData.Temperature.Current > 0 { + if validSMARTTemperature(smartData.Temperature.Current) { result.Temperature = smartData.Temperature.Current - } else if smartData.NVMeSmartHealthInformationLog != nil && smartData.NVMeSmartHealthInformationLog.Temperature > 0 { + } else if smartData.NVMeSmartHealthInformationLog != nil && validSMARTTemperature(smartData.NVMeSmartHealthInformationLog.Temperature) { result.Temperature = smartData.NVMeSmartHealthInformationLog.Temperature - } else if smartData.ATASCTStatus.Current.Value > 0 { + } else if validSMARTTemperature(smartData.ATASCTStatus.Current.Value) { result.Temperature = smartData.ATASCTStatus.Current.Value } else { - for _, attr := range smartData.ATASmartAttributes.Table { - if attr.ID == 194 || attr.ID == 190 { + for _, attributeID := range []int{194, 190} { + for _, attr := range smartData.ATASmartAttributes.Table { + if attr.ID != attributeID { + continue + } temp := parseRawValue(attr.Raw.String, attr.Raw.Value) - if temp > 0 && temp < 150 { + if validSMARTTemperature(int(temp)) { result.Temperature = int(temp) break } } + if result.Temperature > 0 { + break + } } } @@ -1587,6 +1721,10 @@ func parseSMARTOutput(output []byte, target smartctlTarget) (*DiskSMART, error) return result, nil } +func validSMARTTemperature(value int) bool { + return value > 0 && value < 150 +} + func parseSMARTTextOutput(text string, target smartctlTarget) (*DiskSMART, error) { fallback := parseSMARTTextFallback(text) result := &DiskSMART{ @@ -1743,20 +1881,37 @@ func parseSMARTAttributes(data *smartctlJSON, diskType string) *SMARTAttributes if diskType == "nvme" { if data.NVMeSmartHealthInformationLog != nil { - hasData = true nvmeLog := data.NVMeSmartHealthInformationLog - poh := nvmeLog.PowerOnHours - attrs.PowerOnHours = &poh - pc := nvmeLog.PowerCycles - attrs.PowerCycles = &pc - pu := nvmeLog.PercentageUsed - attrs.PercentageUsed = &pu - as := nvmeLog.AvailableSpare - attrs.AvailableSpare = &as - me := nvmeLog.MediaErrors - attrs.MediaErrors = &me - us := nvmeLog.UnsafeShutdowns - attrs.UnsafeShutdowns = &us + if nvmeLog.PowerOnHours != nil { + hasData = true + value := *nvmeLog.PowerOnHours + attrs.PowerOnHours = &value + } + if nvmeLog.PowerCycles != nil { + hasData = true + value := *nvmeLog.PowerCycles + attrs.PowerCycles = &value + } + if nvmeLog.PercentageUsed != nil { + hasData = true + value := *nvmeLog.PercentageUsed + attrs.PercentageUsed = &value + } + if nvmeLog.AvailableSpare != nil { + hasData = true + value := *nvmeLog.AvailableSpare + attrs.AvailableSpare = &value + } + if nvmeLog.MediaErrors != nil { + hasData = true + value := *nvmeLog.MediaErrors + attrs.MediaErrors = &value + } + if nvmeLog.UnsafeShutdowns != nil { + hasData = true + value := *nvmeLog.UnsafeShutdowns + attrs.UnsafeShutdowns = &value + } } } else if diskType == "sas" || diskType == "scsi" { // SCSI drives report no ATA attribute table; their counters live in diff --git a/internal/hostagent/smartctl_coverage_test.go b/internal/hostagent/smartctl_coverage_test.go index 3303b6ce75..68e636c6e2 100644 --- a/internal/hostagent/smartctl_coverage_test.go +++ b/internal/hostagent/smartctl_coverage_test.go @@ -271,8 +271,8 @@ func TestCollectSMARTLocalUsesSmartctlScanOpenTargetsOnLinux(t *testing.T) { Passed bool `json:"passed"` }{Passed: true} payload.NVMeSmartHealthInformationLog = &nvmeSmartHealthInformationLogJSON{ - PercentageUsed: 6, - AvailableSpare: 94, + PercentageUsed: smartctlIntPtr(6), + AvailableSpare: smartctlIntPtr(94), } out, _ := json.Marshal(payload) return out, nil @@ -588,7 +588,7 @@ func TestCollectDeviceSMARTStandby(t *testing.T) { timeNow = func() time.Time { return fixed } execLookPath = func(string) (string, error) { return "smartctl", nil } smartRunCommandOutput = func(ctx context.Context, name string, args ...string) ([]byte, error) { - return exec.CommandContext(ctx, "sh", "-c", "exit 2").Output() + return exec.CommandContext(ctx, "sh", "-c", "exit 3").Output() } result, err := collectDeviceSMART(context.Background(), "/dev/sda") diff --git a/internal/hostagent/smartctl_test.go b/internal/hostagent/smartctl_test.go index 19e4d869aa..2c4380d478 100644 --- a/internal/hostagent/smartctl_test.go +++ b/internal/hostagent/smartctl_test.go @@ -7,6 +7,9 @@ import ( "testing" ) +func smartctlIntPtr(value int) *int { return &value } +func smartctlInt64Ptr(value int64) *int64 { return &value } + func TestDetectDiskType(t *testing.T) { tests := []struct { name string @@ -295,12 +298,12 @@ func TestParseSMARTAttributes_SATA(t *testing.T) { func TestParseSMARTAttributes_NVMe(t *testing.T) { data := &smartctlJSON{} data.NVMeSmartHealthInformationLog = &nvmeSmartHealthInformationLogJSON{ - PowerOnHours: 5000, - PowerCycles: 100, - PercentageUsed: 5, - AvailableSpare: 100, - MediaErrors: 0, - UnsafeShutdowns: 12, + PowerOnHours: smartctlInt64Ptr(5000), + PowerCycles: smartctlInt64Ptr(100), + PercentageUsed: smartctlIntPtr(5), + AvailableSpare: smartctlIntPtr(100), + MediaErrors: smartctlInt64Ptr(0), + UnsafeShutdowns: smartctlInt64Ptr(12), } attrs := parseSMARTAttributes(data, "nvme") @@ -382,7 +385,14 @@ func TestParseSMARTAttributes_Standby(t *testing.T) { func TestParseSMARTAttributes_NVMeZeroValuesWhenLogPresent(t *testing.T) { data := &smartctlJSON{ - NVMeSmartHealthInformationLog: &nvmeSmartHealthInformationLogJSON{}, + NVMeSmartHealthInformationLog: &nvmeSmartHealthInformationLogJSON{ + PowerOnHours: smartctlInt64Ptr(0), + PowerCycles: smartctlInt64Ptr(0), + PercentageUsed: smartctlIntPtr(0), + AvailableSpare: smartctlIntPtr(0), + MediaErrors: smartctlInt64Ptr(0), + UnsafeShutdowns: smartctlInt64Ptr(0), + }, } attrs := parseSMARTAttributes(data, "nvme") diff --git a/internal/models/models.go b/internal/models/models.go index 950e8b84fc..8eaafd956b 100644 --- a/internal/models/models.go +++ b/internal/models/models.go @@ -2412,6 +2412,7 @@ type PhysicalDisk struct { Instance string `json:"instance"` DevPath string `json:"devPath"` // /dev/nvme0n1, /dev/sda Model string `json:"model"` + Vendor string `json:"vendor,omitempty"` Serial string `json:"serial"` WWN string `json:"wwn"` // World Wide Name Type string `json:"type"` // nvme, sata, sas diff --git a/internal/monitoring/canonical_guardrails_test.go b/internal/monitoring/canonical_guardrails_test.go index 24bf6640be..a7bc580831 100644 --- a/internal/monitoring/canonical_guardrails_test.go +++ b/internal/monitoring/canonical_guardrails_test.go @@ -2094,7 +2094,8 @@ func TestProxmoxDiskAlertsRunOnMergedDiskState(t *testing.T) { snippets: []string{ "func mergeHostAgentSMARTIntoDisks(disks []models.PhysicalDisk, nodes []models.Node, hosts []models.Host) []models.PhysicalDisk {", "deriveWearoutFromSMARTAttributes(matched.Attributes)", - `strings.EqualFold(updated[i].Health, "unknown")`, + "shouldUseHostAgentPhysicalDiskHealth(updated[i].Health, matched.Health)", + `if incoming == "FAILED"`, }, }, { diff --git a/internal/monitoring/merge_temps_test.go b/internal/monitoring/merge_temps_test.go index d3c6d50cf0..18f884afa7 100644 --- a/internal/monitoring/merge_temps_test.go +++ b/internal/monitoring/merge_temps_test.go @@ -469,9 +469,39 @@ func TestMergeHostAgentSMARTIntoDisks_PreservesExistingProxmoxIdentity(t *testin if got.Model != "Proxmox Model" || got.Serial != "PVE-SERIAL" || got.WWN != "pve-wwn" || got.Type != "sas" { t.Fatalf("existing Proxmox identity was overwritten: %+v", got) } - if got.Size != 100 || got.Temperature != 41 || got.Health != "PASSED" || got.StorageGroup != "api-pool" { + if got.Size != 100 || got.Temperature != 41 || got.StorageGroup != "api-pool" { t.Fatalf("existing Proxmox disk fields were overwritten: %+v", got) } + if got.Health != "FAILED" { + t.Fatalf("host SMART failure should override coarse Proxmox health: %+v", got) + } +} + +func TestMergeNVMeTempsIntoDisks_DuplicateSerialFailsClosed(t *testing.T) { + disks := []models.PhysicalDisk{{ + ID: "disk-1", + Node: "node1", + DevPath: "/dev/nvme9n1", + Serial: "DUPLICATE-SERIAL", + Type: "nvme", + Wearout: -1, + Health: "UNKNOWN", + }} + nodes := []models.Node{{ + Name: "node1", + Temperature: &models.Temperature{ + Available: true, + SMART: []models.DiskTemp{ + {Device: "/dev/nvme0n1", Serial: "DUPLICATE-SERIAL", Temperature: 31}, + {Device: "/dev/nvme1n1", Serial: "DUPLICATE-SERIAL", Temperature: 52}, + }, + }, + }} + + got := mergeNVMeTempsIntoDisks(disks, nodes) + if got[0].Temperature != 0 { + t.Fatalf("ambiguous serial attached the wrong temperature: %+v", got[0]) + } } func mustParseTime(t *testing.T, raw string) time.Time { diff --git a/internal/monitoring/monitor.go b/internal/monitoring/monitor.go index 31ea0421a0..ab2d108cde 100644 --- a/internal/monitoring/monitor.go +++ b/internal/monitoring/monitor.go @@ -386,55 +386,49 @@ func mergeNVMeTempsIntoDisks(disks []models.PhysicalDisk, nodes []models.Node) [ continue } - // Try to match by WWN (most reliable) - if updated[i].WWN != "" { - for _, temp := range smartTemps { - if temp.WWN != "" && strings.EqualFold(temp.WWN, updated[i].WWN) { - if temp.Temperature > 0 && !temp.StandbySkipped { - setPhysicalDiskTemperature(&updated[i], temp.Temperature, "proxmox_node_smart") - log.Debug(). - Str("disk", updated[i].DevPath). - Str("wwn", updated[i].WWN). - Int("temp", temp.Temperature). - Msg("Matched SMART temperature by WWN") - } - continue - } + // Try to match by WWN (most reliable), but never guess when an + // identity appears more than once in the node snapshot. + if diskinventory.IsUsableHardwareID(updated[i].WWN) { + if temp := uniqueDiskTemperatureMatch(smartTemps, func(candidate models.DiskTemp) bool { + return diskinventory.IsUsableHardwareID(candidate.WWN) && + strings.EqualFold(candidate.WWN, updated[i].WWN) + }); temp != nil && temp.Temperature > 0 && !temp.StandbySkipped { + setPhysicalDiskTemperature(&updated[i], temp.Temperature, "proxmox_node_smart") + log.Debug(). + Str("disk", updated[i].DevPath). + Str("wwn", updated[i].WWN). + Int("temp", temp.Temperature). + Msg("Matched SMART temperature by WWN") } } // Fall back to serial number match (case-insensitive) - if updated[i].Serial != "" && updated[i].Temperature == 0 { - for _, temp := range smartTemps { - if temp.Serial != "" && strings.EqualFold(temp.Serial, updated[i].Serial) { - if temp.Temperature > 0 && !temp.StandbySkipped { - setPhysicalDiskTemperature(&updated[i], temp.Temperature, "proxmox_node_smart") - log.Debug(). - Str("disk", updated[i].DevPath). - Str("serial", updated[i].Serial). - Int("temp", temp.Temperature). - Msg("Matched SMART temperature by serial") - } - continue - } + if diskinventory.IsUsableHardwareID(updated[i].Serial) && updated[i].Temperature == 0 { + if temp := uniqueDiskTemperatureMatch(smartTemps, func(candidate models.DiskTemp) bool { + return diskinventory.IsUsableHardwareID(candidate.Serial) && + strings.EqualFold(candidate.Serial, updated[i].Serial) + }); temp != nil && temp.Temperature > 0 && !temp.StandbySkipped { + setPhysicalDiskTemperature(&updated[i], temp.Temperature, "proxmox_node_smart") + log.Debug(). + Str("disk", updated[i].DevPath). + Str("serial", updated[i].Serial). + Int("temp", temp.Temperature). + Msg("Matched SMART temperature by serial") } } // Last resort: match by device path (normalized) if updated[i].Temperature == 0 { normalizedDevPath := normalizeSMARTDeviceIdentifier(updated[i].DevPath) - for _, temp := range smartTemps { - normalizedTempDev := normalizeSMARTDeviceIdentifier(temp.Device) - if normalizedTempDev != "" && normalizedTempDev == normalizedDevPath { - if temp.Temperature > 0 && !temp.StandbySkipped { - setPhysicalDiskTemperature(&updated[i], temp.Temperature, "proxmox_node_smart") - log.Debug(). - Str("disk", updated[i].DevPath). - Int("temp", temp.Temperature). - Msg("Matched SMART temperature by device path") - } - break - } + if temp := uniqueDiskTemperatureMatch(smartTemps, func(candidate models.DiskTemp) bool { + normalizedTempDev := normalizeSMARTDeviceIdentifier(candidate.Device) + return normalizedTempDev != "" && normalizedTempDev == normalizedDevPath + }); temp != nil && temp.Temperature > 0 && !temp.StandbySkipped { + setPhysicalDiskTemperature(&updated[i], temp.Temperature, "proxmox_node_smart") + log.Debug(). + Str("disk", updated[i].DevPath). + Int("temp", temp.Temperature). + Msg("Matched SMART temperature by device path") } } } @@ -478,6 +472,23 @@ func mergeNVMeTempsIntoDisks(disks []models.PhysicalDisk, nodes []models.Node) [ return updated } +func uniqueDiskTemperatureMatch(temperatures []models.DiskTemp, predicate func(models.DiskTemp) bool) *models.DiskTemp { + matchIndex := -1 + for index := range temperatures { + if !predicate(temperatures[index]) { + continue + } + if matchIndex >= 0 { + return nil + } + matchIndex = index + } + if matchIndex < 0 { + return nil + } + return &temperatures[matchIndex] +} + func setPhysicalDiskTemperature(disk *models.PhysicalDisk, temperature int, source string) { if disk == nil || temperature <= 0 { return @@ -536,39 +547,50 @@ func mergeHostAgentSMARTIntoDisks(disks []models.PhysicalDisk, nodes []models.No // Find matching SMART entry by WWN, serial, or topology-scoped device path. var matched *models.HostDiskSMART + uniqueMatch := func(predicate func(models.HostDiskSMART) bool) *models.HostDiskSMART { + matchIndex := -1 + for index := range smartData { + if !predicate(smartData[index]) { + continue + } + if matchIndex >= 0 { + return nil + } + matchIndex = index + } + if matchIndex < 0 { + return nil + } + return &smartData[matchIndex] + } // Try to match by WWN (most reliable) - if updated[i].WWN != "" { - for j := range smartData { - if smartData[j].WWN != "" && strings.EqualFold(smartData[j].WWN, updated[i].WWN) { - matched = &smartData[j] - break - } - } + if diskinventory.IsUsableHardwareID(updated[i].WWN) { + matched = uniqueMatch(func(candidate models.HostDiskSMART) bool { + return diskinventory.IsUsableHardwareID(candidate.WWN) && + strings.EqualFold(candidate.WWN, updated[i].WWN) && + diskTopologyCompatible(updated[i].Controller, updated[i].Target, candidate.Controller, candidate.Target) + }) } // Fall back to serial number match - if matched == nil && updated[i].Serial != "" { - for j := range smartData { - if smartData[j].Serial != "" && strings.EqualFold(smartData[j].Serial, updated[i].Serial) { - matched = &smartData[j] - break - } - } + if matched == nil && diskinventory.IsUsableHardwareID(updated[i].Serial) { + matched = uniqueMatch(func(candidate models.HostDiskSMART) bool { + return diskinventory.IsUsableHardwareID(candidate.Serial) && + strings.EqualFold(candidate.Serial, updated[i].Serial) && + diskTopologyCompatible(updated[i].Controller, updated[i].Target, candidate.Controller, candidate.Target) + }) } // Last resort: match by device path if matched == nil { normalizedDevPath := normalizeSMARTDeviceIdentifier(updated[i].DevPath) - for j := range smartData { - normalizedDiskDev := normalizeSMARTDeviceIdentifier(smartData[j].Device) - if normalizedDiskDev != "" && + matched = uniqueMatch(func(candidate models.HostDiskSMART) bool { + normalizedDiskDev := normalizeSMARTDeviceIdentifier(candidate.Device) + return normalizedDiskDev != "" && normalizedDiskDev == normalizedDevPath && - diskTopologyCompatible(updated[i].Controller, updated[i].Target, smartData[j].Controller, smartData[j].Target) { - matched = &smartData[j] - break - } - } + diskTopologyCompatible(updated[i].Controller, updated[i].Target, candidate.Controller, candidate.Target) + }) } if matched == nil { @@ -613,10 +635,8 @@ func mergeHostAgentSMARTIntoDisks(disks []models.PhysicalDisk, nodes []models.No // Always merge SMART attributes from host agent if matched.Attributes != nil { updated[i].SmartAttributes = smartAttributesCopy(matched.Attributes) - if updated[i].Wearout < 0 { - if derivedWearout := deriveWearoutFromSMARTAttributes(matched.Attributes); derivedWearout >= 0 { - updated[i].Wearout = derivedWearout - } + if derivedWearout := deriveWearoutFromSMARTAttributes(matched.Attributes); derivedWearout >= 0 { + updated[i].Wearout = derivedWearout } } if matched.IO != nil { @@ -625,7 +645,7 @@ func mergeHostAgentSMARTIntoDisks(disks []models.PhysicalDisk, nodes []models.No } updated[i].Collection = diskinventory.MergeStatus(updated[i].Collection, matched.Collection) - if (strings.TrimSpace(updated[i].Health) == "" || strings.EqualFold(updated[i].Health, "unknown")) && strings.TrimSpace(matched.Health) != "" { + if shouldUseHostAgentPhysicalDiskHealth(updated[i].Health, matched.Health) { updated[i].Health = matched.Health } } @@ -633,6 +653,18 @@ func mergeHostAgentSMARTIntoDisks(disks []models.PhysicalDisk, nodes []models.No return updated } +func shouldUseHostAgentPhysicalDiskHealth(existing, incoming string) bool { + incoming = strings.ToUpper(strings.TrimSpace(incoming)) + if incoming == "" || incoming == "UNKNOWN" { + return false + } + existing = strings.ToUpper(strings.TrimSpace(existing)) + if incoming == "FAILED" { + return true + } + return existing == "" || existing == "UNKNOWN" +} + func diskTopologyCompatible(leftController, leftTarget, rightController, rightTarget string) bool { leftController = strings.TrimSpace(leftController) leftTarget = strings.TrimSpace(leftTarget) @@ -760,46 +792,17 @@ func smartAttributesFromUnifiedMeta(in *unifiedresources.SMARTMeta) *models.SMAR return nil } - out := &models.SMARTAttributes{} - if in.PowerOnHours != 0 { - value := in.PowerOnHours - out.PowerOnHours = &value - } - if in.PowerCycles != 0 { - value := in.PowerCycles - out.PowerCycles = &value - } - if in.ReallocatedSectors != 0 { - value := in.ReallocatedSectors - out.ReallocatedSectors = &value - } - if in.PendingSectors != 0 { - value := in.PendingSectors - out.PendingSectors = &value - } - if in.OfflineUncorrectable != 0 { - value := in.OfflineUncorrectable - out.OfflineUncorrectable = &value - } - if in.UDMACRCErrors != 0 { - value := in.UDMACRCErrors - out.UDMACRCErrors = &value - } - if in.PercentageUsed != 0 { - value := in.PercentageUsed - out.PercentageUsed = &value - } - if in.AvailableSpare != 0 { - value := in.AvailableSpare - out.AvailableSpare = &value - } - if in.MediaErrors != 0 { - value := in.MediaErrors - out.MediaErrors = &value - } - if in.UnsafeShutdowns != 0 { - value := in.UnsafeShutdowns - out.UnsafeShutdowns = &value + out := &models.SMARTAttributes{ + PowerOnHours: cloneInt64Pointer(in.PowerOnHours), + PowerCycles: cloneInt64Pointer(in.PowerCycles), + ReallocatedSectors: cloneInt64Pointer(in.ReallocatedSectors), + PendingSectors: cloneInt64Pointer(in.PendingSectors), + OfflineUncorrectable: cloneInt64Pointer(in.OfflineUncorrectable), + UDMACRCErrors: cloneInt64Pointer(in.UDMACRCErrors), + PercentageUsed: cloneIntPointer(in.PercentageUsed), + AvailableSpare: cloneIntPointer(in.AvailableSpare), + MediaErrors: cloneInt64Pointer(in.MediaErrors), + UnsafeShutdowns: cloneInt64Pointer(in.UnsafeShutdowns), } if out.PowerOnHours == nil && out.PowerCycles == nil && @@ -816,6 +819,22 @@ func smartAttributesFromUnifiedMeta(in *unifiedresources.SMARTMeta) *models.SMAR return out } +func cloneIntPointer(in *int) *int { + if in == nil { + return nil + } + value := *in + return &value +} + +func cloneInt64Pointer(in *int64) *int64 { + if in == nil { + return nil + } + value := *in + return &value +} + func physicalDisksForInstanceFromReadState(readState unifiedresources.ReadState, instance string) []models.PhysicalDisk { if readState == nil { return nil diff --git a/internal/monitoring/monitor_integration_test.go b/internal/monitoring/monitor_integration_test.go index 092452c89f..6a12bce904 100644 --- a/internal/monitoring/monitor_integration_test.go +++ b/internal/monitoring/monitor_integration_test.go @@ -183,6 +183,103 @@ func TestMergeHostAgentSMARTIntoDisks_DerivesWearoutFromSMARTAttributes(t *testi } } +func TestMergeHostAgentSMARTIntoDisks_SMARTFailureAndWearoutOverrideCoarsePVEValues(t *testing.T) { + disks := []models.PhysicalDisk{{ + ID: "d1", + Node: "pve1", + Serial: "SER1", + Type: "ssd", + Health: "PASSED", + Wearout: 100, + }} + nodes := []models.Node{{Name: "pve1", LinkedAgentID: "host-1"}} + used := 95 + hosts := []models.Host{{ + ID: "host-1", + Sensors: models.HostSensorSummary{ + SMART: []models.HostDiskSMART{{ + Device: "/dev/sda", + Serial: "SER1", + Health: "FAILED", + Attributes: &models.SMARTAttributes{ + PercentageUsed: &used, + }, + }}, + }, + }} + + result := mergeHostAgentSMARTIntoDisks(disks, nodes, hosts) + if result[0].Health != "FAILED" { + t.Fatalf("SMART failure was suppressed by PVE health: %+v", result[0]) + } + if result[0].Wearout != 5 { + t.Fatalf("SMART wearout did not override contradictory PVE value: %+v", result[0]) + } +} + +func TestMergeHostAgentSMARTIntoDisks_DoesNotReplaceFailureWithPassed(t *testing.T) { + disks := []models.PhysicalDisk{{ID: "d1", Node: "pve1", Serial: "SER1", Health: "FAILED"}} + nodes := []models.Node{{Name: "pve1", LinkedAgentID: "host-1"}} + hosts := []models.Host{{ + ID: "host-1", + Sensors: models.HostSensorSummary{ + SMART: []models.HostDiskSMART{{Serial: "SER1", Health: "PASSED"}}, + }, + }} + + if got := mergeHostAgentSMARTIntoDisks(disks, nodes, hosts)[0].Health; got != "FAILED" { + t.Fatalf("PASSED replaced an existing failure: %q", got) + } +} + +func TestMergeHostAgentSMARTIntoDisks_DuplicateIdentityRequiresUniqueTopology(t *testing.T) { + disks := []models.PhysicalDisk{{ + ID: "d1", + Node: "pve1", + DevPath: "/dev/sg2", + Serial: "DUPLICATE-SERIAL", + Controller: "sssraid0", + Target: "sssraid,0,2", + Health: "UNKNOWN", + }} + nodes := []models.Node{{Name: "pve1", LinkedAgentID: "host-1"}} + hosts := []models.Host{{ + ID: "host-1", + Sensors: models.HostSensorSummary{ + SMART: []models.HostDiskSMART{ + { + Device: "/dev/sg2", + Serial: "DUPLICATE-SERIAL", + Controller: "sssraid0", + Target: "sssraid,0,1", + Temperature: 31, + }, + { + Device: "/dev/sg2", + Serial: "DUPLICATE-SERIAL", + Controller: "sssraid0", + Target: "sssraid,0,2", + Temperature: 47, + Health: "PASSED", + }, + }, + }, + }} + + result := mergeHostAgentSMARTIntoDisks(disks, nodes, hosts) + if result[0].Temperature != 47 || result[0].Health != "PASSED" { + t.Fatalf("topology-specific SMART evidence was not selected: %+v", result[0]) + } + + disks[0].Controller = "" + disks[0].Target = "" + disks[0].DevPath = "/dev/unknown" + result = mergeHostAgentSMARTIntoDisks(disks, nodes, hosts) + if result[0].Temperature != 0 || result[0].Health != "UNKNOWN" { + t.Fatalf("ambiguous SMART identity was guessed: %+v", result[0]) + } +} + func TestMergeHostAgentSMARTIntoDisks_FillsMissingHealthFromHostSMART(t *testing.T) { disks := []models.PhysicalDisk{ {ID: "d1", Node: "pve1", Serial: "SER1", Health: "UNKNOWN"}, @@ -281,24 +378,24 @@ func TestSmartAttributesFromUnifiedMeta_Nil(t *testing.T) { } func TestSmartAttributesFromUnifiedMeta_AllZero(t *testing.T) { - // All-zero fields should return nil (no meaningful data) + // No reported fields should return nil. if smartAttributesFromUnifiedMeta(&unifiedresources.SMARTMeta{}) != nil { - t.Error("all-zero SMARTMeta should return nil") + t.Error("SMARTMeta with no reported fields should return nil") } } func TestSmartAttributesFromUnifiedMeta_PopulatesFields(t *testing.T) { meta := &unifiedresources.SMARTMeta{ - PowerOnHours: 12345, - PowerCycles: 500, - ReallocatedSectors: 0, - PendingSectors: 2, - OfflineUncorrectable: 1, - UDMACRCErrors: 3, - PercentageUsed: 15, - AvailableSpare: 85, - MediaErrors: 0, - UnsafeShutdowns: 10, + PowerOnHours: smartInt64Pointer(12345), + PowerCycles: smartInt64Pointer(500), + ReallocatedSectors: smartInt64Pointer(0), + PendingSectors: smartInt64Pointer(2), + OfflineUncorrectable: smartInt64Pointer(1), + UDMACRCErrors: smartInt64Pointer(3), + PercentageUsed: smartIntPointer(15), + AvailableSpare: smartIntPointer(85), + MediaErrors: smartInt64Pointer(0), + UnsafeShutdowns: smartInt64Pointer(10), } result := smartAttributesFromUnifiedMeta(meta) if result == nil { @@ -310,8 +407,8 @@ func TestSmartAttributesFromUnifiedMeta_PopulatesFields(t *testing.T) { if result.PowerCycles == nil || *result.PowerCycles != 500 { t.Error("PowerCycles should be 500") } - if result.ReallocatedSectors != nil { - t.Error("zero ReallocatedSectors should not create a pointer") + if result.ReallocatedSectors == nil || *result.ReallocatedSectors != 0 { + t.Error("reported zero ReallocatedSectors should survive projection") } if result.PendingSectors == nil || *result.PendingSectors != 2 { t.Error("PendingSectors should be 2") @@ -319,14 +416,17 @@ func TestSmartAttributesFromUnifiedMeta_PopulatesFields(t *testing.T) { if result.PercentageUsed == nil || *result.PercentageUsed != 15 { t.Error("PercentageUsed should be 15") } - if result.MediaErrors != nil { - t.Error("zero MediaErrors should not create a pointer") + if result.MediaErrors == nil || *result.MediaErrors != 0 { + t.Error("reported zero MediaErrors should survive projection") } if result.UnsafeShutdowns == nil || *result.UnsafeShutdowns != 10 { t.Error("UnsafeShutdowns should be 10") } } +func smartIntPointer(value int) *int { return &value } +func smartInt64Pointer(value int64) *int64 { return &value } + // --- getNodeDisplayName --- func TestGetNodeDisplayName_NilInstance(t *testing.T) { diff --git a/internal/monitoring/monitor_pve.go b/internal/monitoring/monitor_pve.go index f4850492ef..6ad8879063 100644 --- a/internal/monitoring/monitor_pve.go +++ b/internal/monitoring/monitor_pve.go @@ -14,6 +14,7 @@ import ( "github.com/rcourtman/pulse-go-rewrite/internal/logging" "github.com/rcourtman/pulse-go-rewrite/internal/models" "github.com/rcourtman/pulse-go-rewrite/internal/monitoring/errors" + "github.com/rcourtman/pulse-go-rewrite/internal/unifiedresources" "github.com/rcourtman/pulse-go-rewrite/pkg/diskinventory" "github.com/rcourtman/pulse-go-rewrite/pkg/fsfilters" "github.com/rcourtman/pulse-go-rewrite/pkg/proxmox" @@ -899,8 +900,9 @@ func (m *Monitor) maybePollPhysicalDisksAsync( readState := m.GetUnifiedReadStateOrSnapshot() nodesFromState := nodesForInstanceFromReadState(readState, inst) hosts := hostsFromReadState(readState) - existingDisksMap := make(map[string]models.PhysicalDisk) - for _, disk := range physicalDisksForInstanceFromReadState(readState, inst) { + existingDisks := physicalDisksForInstanceFromReadState(readState, inst) + existingDisksMap := make(map[string]models.PhysicalDisk, len(existingDisks)) + for _, disk := range existingDisks { if disk.Instance == inst { existingDisksMap[disk.ID] = disk } @@ -1022,13 +1024,20 @@ func (m *Monitor) maybePollPhysicalDisksAsync( // Record each disk; alert evaluation happens after host-agent SMART merges // so the canonical disk view includes post-merge health/wearout data. for _, disk := range disks { - diskID := fmt.Sprintf("%s-%s-%s", inst, node.Node, strings.ReplaceAll(disk.DevPath, "/", "-")) + diskID := unifiedresources.ProxmoxPhysicalDiskSourceID( + inst, + node.Node, + disk.DevPath, + "", + "", + ) physicalDisk := models.PhysicalDisk{ ID: diskID, Node: node.Node, Instance: inst, DevPath: disk.DevPath, Model: disk.Model, + Vendor: disk.Vendor, Serial: disk.Serial, WWN: disk.WWN, Type: disk.Type, @@ -1074,7 +1083,7 @@ func (m *Monitor) maybePollPhysicalDisksAsync( allDisks = mergeNVMeTempsIntoDisks(allDisks, nodesFromState) allDisks = mergeHostAgentSMARTIntoDisks(allDisks, nodesFromState, hosts) for index := range allDisks { - if previous, ok := existingDisksMap[allDisks[index].ID]; ok { + if previous, ok := previousPhysicalDiskEvidence(allDisks[index], existingDisks); ok { allDisks[index] = preserveUnavailablePhysicalDiskEvidence(allDisks[index], previous) } } @@ -1165,7 +1174,13 @@ func physicalDisksFromHostAgentSMART(inst, nodeName string, smartEntries []model health = "UNKNOWN" } disks = append(disks, models.PhysicalDisk{ - ID: fmt.Sprintf("%s-%s-%s", inst, nodeName, strings.ReplaceAll(devPath, "/", "-")), + ID: unifiedresources.ProxmoxPhysicalDiskSourceID( + inst, + nodeName, + devPath, + smart.Controller, + smart.Target, + ), Node: nodeName, Instance: inst, DevPath: devPath, @@ -1197,6 +1212,64 @@ func cloneDiskIO(in *models.DiskIO) *models.DiskIO { return &out } +func previousPhysicalDiskEvidence(current models.PhysicalDisk, previous []models.PhysicalDisk) (models.PhysicalDisk, bool) { + sameScope := func(candidate models.PhysicalDisk) bool { + return strings.EqualFold(strings.TrimSpace(candidate.Instance), strings.TrimSpace(current.Instance)) && + strings.EqualFold(strings.TrimSpace(candidate.Node), strings.TrimSpace(current.Node)) + } + uniqueMatch := func(matches func(models.PhysicalDisk) bool) (models.PhysicalDisk, bool) { + var matched models.PhysicalDisk + found := false + for _, candidate := range previous { + if !sameScope(candidate) || !matches(candidate) { + continue + } + if found { + return models.PhysicalDisk{}, false + } + matched = candidate + found = true + } + return matched, found + } + + if serial := strings.TrimSpace(current.Serial); diskinventory.IsUsableHardwareID(serial) { + if matched, ok := uniqueMatch(func(candidate models.PhysicalDisk) bool { + return candidate.Serial != "" && strings.EqualFold(strings.TrimSpace(candidate.Serial), serial) + }); ok { + return matched, true + } + } + if wwn := strings.TrimSpace(current.WWN); diskinventory.IsUsableHardwareID(wwn) { + if matched, ok := uniqueMatch(func(candidate models.PhysicalDisk) bool { + return candidate.WWN != "" && strings.EqualFold(strings.TrimSpace(candidate.WWN), wwn) + }); ok { + return matched, true + } + } + if id := strings.TrimSpace(current.ID); id != "" { + if matched, ok := uniqueMatch(func(candidate models.PhysicalDisk) bool { + return strings.TrimSpace(candidate.ID) == id + }); ok { + return matched, true + } + } + + device := normalizeSMARTDeviceIdentifier(current.DevPath) + if device == "" { + return models.PhysicalDisk{}, false + } + return uniqueMatch(func(candidate models.PhysicalDisk) bool { + return normalizeSMARTDeviceIdentifier(candidate.DevPath) == device && + diskTopologyCompatible( + current.Controller, + current.Target, + candidate.Controller, + candidate.Target, + ) + }) +} + func preserveUnavailablePhysicalDiskEvidence(current, previous models.PhysicalDisk) models.PhysicalDisk { if current.Collection == nil { return current @@ -1231,6 +1304,7 @@ func proxmoxDiskFromPhysicalDisk(disk models.PhysicalDisk) proxmox.Disk { return proxmox.Disk{ DevPath: disk.DevPath, Model: disk.Model, + Vendor: disk.Vendor, Serial: disk.Serial, Type: disk.Type, Health: disk.Health, diff --git a/internal/monitoring/monitor_pve_disk_fallback_test.go b/internal/monitoring/monitor_pve_disk_fallback_test.go index ddf47ea013..95ad533534 100644 --- a/internal/monitoring/monitor_pve_disk_fallback_test.go +++ b/internal/monitoring/monitor_pve_disk_fallback_test.go @@ -85,6 +85,16 @@ func (*disksErrorPVEClient) GetDisks(ctx context.Context, node string) ([]proxmo return nil, fmt.Errorf("596 connection timed out") } +type permissionDeniedDisksPVEClient struct { + fakeStorageClient + called chan struct{} +} + +func (client *permissionDeniedDisksPVEClient) GetDisks(context.Context, string) ([]proxmox.Disk, error) { + close(client.called) + return nil, fmt.Errorf("403 permission check failed") +} + // A node whose Proxmox disks/list query fails (wide nodes can exceed the API // window) must still populate its Physical Disks view from the linked host // agent's SMART inventory instead of silently emptying (#1516). @@ -146,3 +156,65 @@ func TestMaybePollPhysicalDisksAsync_AgentFallbackWhenDiskQueryFails(t *testing. time.Sleep(10 * time.Millisecond) } } + +func TestMaybePollPhysicalDisksAsync_PermissionFailurePreservesPreviousInventory(t *testing.T) { + m := &Monitor{ + state: models.NewState(), + lastPhysicalDiskPoll: make(map[string]time.Time), + alertManager: alerts.NewManager(), + } + existing := models.PhysicalDisk{ + ID: "pve1-node1--dev-sda", + Node: "node1", + Instance: "pve1", + DevPath: "/dev/sda", + Model: "CT240BX500SSD1", + Serial: "SATA-BOOT-1", + Type: "sata", + Size: 240057409536, + Health: "PASSED", + Wearout: -1, + Temperature: 54, + Used: "LVM", + LastChecked: time.Now().Add(-time.Minute), + } + m.state.UpdatePhysicalDisks("pve1", []models.PhysicalDisk{existing}) + m.state.UpdateNodesForInstance("pve1", []models.Node{{ + ID: "pve1-node1", + Name: "node1", + Instance: "pve1", + }}) + beforePoll := m.state.GetSnapshot().LastUpdate + client := &permissionDeniedDisksPVEClient{called: make(chan struct{})} + + m.maybePollPhysicalDisksAsync( + context.Background(), + "pve1", + &config.PVEInstance{}, + client, + []proxmox.Node{{Node: "node1", Status: "online"}}, + map[string]string{"node1": "online"}, + []models.Node{{Name: "node1", Instance: "pve1"}}, + ) + + deadline := time.Now().Add(3 * time.Second) + select { + case <-client.called: + case <-time.After(3 * time.Second): + t.Fatal("permission-denied disk poll was not attempted") + } + for { + snapshot := m.state.GetSnapshot() + disks := snapshot.PhysicalDisks + if snapshot.LastUpdate.After(beforePoll) && len(disks) == 1 && disks[0].Serial == existing.Serial { + if disks[0].Temperature != 54 || disks[0].Used != "LVM" { + t.Fatalf("permission failure erased prior disk evidence: %+v", disks[0]) + } + return + } + if time.Now().After(deadline) { + t.Fatalf("permission failure did not preserve prior inventory: %+v", disks) + } + time.Sleep(10 * time.Millisecond) + } +} diff --git a/internal/storagehealth/risk.go b/internal/storagehealth/risk.go index 937b0edc4c..35bdd1adf5 100644 --- a/internal/storagehealth/risk.go +++ b/internal/storagehealth/risk.go @@ -33,6 +33,7 @@ type Sample struct { Health string Temperature int Wearout int + WearoutKnown bool PowerOnHours int64 PowerCycles int64 ReallocatedSectors int64 @@ -41,6 +42,7 @@ type Sample struct { UDMACRCErrors int64 PercentageUsed int AvailableSpare int + AvailableSpareKnown bool MediaErrors int64 UnsafeShutdowns int64 } @@ -51,6 +53,8 @@ func AssessPhysicalDisk(disk models.PhysicalDisk) Assessment { Health: disk.Health, Temperature: disk.Temperature, Wearout: disk.Wearout, + WearoutKnown: disk.Wearout > 0 || + (disk.Wearout == 0 && isNonRotationalDiskType(disk.Type)), } if attrs := disk.SmartAttributes; attrs != nil { if attrs.PowerOnHours != nil { @@ -73,9 +77,12 @@ func AssessPhysicalDisk(disk models.PhysicalDisk) Assessment { } if attrs.PercentageUsed != nil { sample.PercentageUsed = *attrs.PercentageUsed + sample.Wearout = 100 - *attrs.PercentageUsed + sample.WearoutKnown = true } if attrs.AvailableSpare != nil { sample.AvailableSpare = *attrs.AvailableSpare + sample.AvailableSpareKnown = true } if attrs.MediaErrors != nil { sample.MediaErrors = *attrs.MediaErrors @@ -116,9 +123,11 @@ func AssessHostSMARTDisk(disk models.HostDiskSMART) Assessment { if attrs.PercentageUsed != nil { sample.PercentageUsed = *attrs.PercentageUsed sample.Wearout = 100 - *attrs.PercentageUsed + sample.WearoutKnown = true } if attrs.AvailableSpare != nil { sample.AvailableSpare = *attrs.AvailableSpare + sample.AvailableSpareKnown = true } if attrs.MediaErrors != nil { sample.MediaErrors = *attrs.MediaErrors @@ -159,12 +168,12 @@ func AssessSample(sample Sample) Assessment { if sample.MediaErrors > 0 { addReason("media_errors", RiskCritical, fmt.Sprintf("Media errors detected (%d)", sample.MediaErrors)) } - if sample.Wearout > 0 && sample.Wearout <= 5 { + if (sample.WearoutKnown || sample.Wearout > 0) && sample.Wearout >= 0 && sample.Wearout <= 5 { addReason("wearout_low", RiskCritical, fmt.Sprintf("SSD life remaining is %d%%", sample.Wearout)) - } else if sample.Wearout > 0 && sample.Wearout < 10 { + } else if sample.Wearout > 5 && sample.Wearout < 10 { addReason("wearout_low", RiskWarning, fmt.Sprintf("SSD life remaining is %d%%", sample.Wearout)) } - if sample.AvailableSpare > 0 && sample.AvailableSpare <= 10 { + if (sample.AvailableSpareKnown || sample.AvailableSpare > 0) && sample.AvailableSpare <= 10 { addReason("nvme_available_spare_low", RiskCritical, fmt.Sprintf("NVMe available spare is %d%%", sample.AvailableSpare)) } else if sample.AvailableSpare > 0 && sample.AvailableSpare < 20 { addReason("nvme_available_spare_low", RiskWarning, fmt.Sprintf("NVMe available spare is %d%%", sample.AvailableSpare)) @@ -198,6 +207,15 @@ func AssessSample(sample Sample) Assessment { return assessment } +func isNonRotationalDiskType(diskType string) bool { + switch strings.ToLower(strings.TrimSpace(diskType)) { + case "nvme", "ssd": + return true + default: + return false + } +} + func HasKnownFirmwareBug(model string) bool { normalizedModel := strings.ToUpper(strings.TrimSpace(model)) knownProblematicModels := []string{ diff --git a/internal/storagehealth/risk_test.go b/internal/storagehealth/risk_test.go index 0b996aa87e..af5b55bff8 100644 --- a/internal/storagehealth/risk_test.go +++ b/internal/storagehealth/risk_test.go @@ -2,6 +2,8 @@ package storagehealth import ( "testing" + + "github.com/rcourtman/pulse-go-rewrite/internal/models" ) // --- AssessSample: comprehensive branch coverage --- @@ -121,6 +123,39 @@ func TestAssessSample_WearoutCritical(t *testing.T) { } } +func TestAssessSample_ReportedZeroWearoutIsCritical(t *testing.T) { + assessment := AssessSample(Sample{ + Health: "PASSED", + Wearout: 0, + WearoutKnown: true, + }) + if assessment.Level != RiskCritical { + t.Fatalf("expected reported wearout=0 to be critical, got %s", assessment.Level) + } +} + +func TestAssessSample_UnreportedZeroWearoutIsNeutral(t *testing.T) { + assessment := AssessSample(Sample{Health: "PASSED"}) + if assessment.Level != RiskHealthy { + t.Fatalf("expected zero-value unreported wearout to remain neutral, got %s", assessment.Level) + } +} + +func TestAssessPhysicalDisk_PercentageUsedProvesZeroLifeOnSATASSD(t *testing.T) { + percentageUsed := 100 + assessment := AssessPhysicalDisk(models.PhysicalDisk{ + Type: "sata", + Health: "PASSED", + Wearout: 0, + SmartAttributes: &models.SMARTAttributes{ + PercentageUsed: &percentageUsed, + }, + }) + if assessment.Level != RiskCritical { + t.Fatalf("reported percentage-used=100 should prove zero remaining life, got %s", assessment.Level) + } +} + func TestAssessSample_WearoutWarning(t *testing.T) { assessment := AssessSample(Sample{ Health: "PASSED", diff --git a/internal/unifiedresources/adapters.go b/internal/unifiedresources/adapters.go index 2f540df3a7..983039723d 100644 --- a/internal/unifiedresources/adapters.go +++ b/internal/unifiedresources/adapters.go @@ -759,7 +759,7 @@ func resourceFromHostUnraidPhysicalDisk(host models.Host, disk models.HostUnraid identity := ResourceIdentity{ Hostnames: uniqueStrings([]string{host.Hostname}), } - if serial := strings.TrimSpace(disk.Serial); serial != "" { + if serial := strings.TrimSpace(disk.Serial); diskinventory.IsUsableHardwareID(serial) { identity.MachineID = serial } return resource, identity @@ -829,6 +829,7 @@ func resourceFromHostSMARTDisk(host models.Host, disk models.HostDiskSMART) (Res if unraidDisk != nil { assessment = storagehealth.SummarizeAssessments(assessment, assessUnraidPhysicalDisk(*unraidDisk)) } + wearout := physicalDiskWearoutFromSMARTAttributes(disk.Attributes) storageGroup := unraidDiskGroup(unraidDisk) if storageGroup == "" { @@ -851,7 +852,7 @@ func resourceFromHostSMARTDisk(host models.Host, disk models.HostDiskSMART) (Res Target: strings.TrimSpace(disk.Target), SizeBytes: sizeBytes, Health: health, - Wearout: -1, + Wearout: wearout, Temperature: temperature, Used: used, StorageRole: unraidDiskRole(unraidDisk), @@ -871,15 +872,29 @@ func resourceFromHostSMARTDisk(host models.Host, disk models.HostDiskSMART) (Res identity := ResourceIdentity{ Hostnames: uniqueStrings([]string{host.Hostname}), } - if serial != "" { + if diskinventory.IsUsableHardwareID(serial) { identity.MachineID = serial - } else if disk.WWN != "" { + } else if diskinventory.IsUsableHardwareID(disk.WWN) { identity.MachineID = strings.TrimSpace(disk.WWN) } return resource, identity } +func physicalDiskWearoutFromSMARTAttributes(attrs *models.SMARTAttributes) int { + if attrs == nil || attrs.PercentageUsed == nil { + return -1 + } + used := *attrs.PercentageUsed + if used < 0 { + used = 0 + } + if used > 100 { + used = 100 + } + return 100 - used +} + func hostUnraidStorageIdentity(host models.Host) string { if machineID := strings.TrimSpace(host.MachineID); machineID != "" { return machineID + "/storage/unraid-array" @@ -2014,6 +2029,7 @@ func resourceFromPhysicalDisk(disk models.PhysicalDisk) (Resource, ResourceIdent pdMeta := &PhysicalDiskMeta{ DevPath: disk.DevPath, Model: disk.Model, + Vendor: disk.Vendor, Serial: disk.Serial, WWN: disk.WWN, DiskType: disk.Type, @@ -2054,9 +2070,9 @@ func resourceFromPhysicalDisk(disk models.PhysicalDisk) (Resource, ResourceIdent identity := ResourceIdentity{ Hostnames: uniqueStrings([]string{disk.Node}), } - if disk.Serial != "" { + if diskinventory.IsUsableHardwareID(disk.Serial) { identity.MachineID = disk.Serial - } else if disk.WWN != "" { + } else if diskinventory.IsUsableHardwareID(disk.WWN) { identity.MachineID = disk.WWN } @@ -2083,38 +2099,18 @@ func convertSMARTAttributes(attrs *models.SMARTAttributes) *SMARTMeta { if attrs == nil { return nil } - m := &SMARTMeta{} - if attrs.PowerOnHours != nil { - m.PowerOnHours = *attrs.PowerOnHours + return &SMARTMeta{ + PowerOnHours: cloneInt64Ptr(attrs.PowerOnHours), + PowerCycles: cloneInt64Ptr(attrs.PowerCycles), + ReallocatedSectors: cloneInt64Ptr(attrs.ReallocatedSectors), + PendingSectors: cloneInt64Ptr(attrs.PendingSectors), + OfflineUncorrectable: cloneInt64Ptr(attrs.OfflineUncorrectable), + UDMACRCErrors: cloneInt64Ptr(attrs.UDMACRCErrors), + PercentageUsed: cloneIntPtr(attrs.PercentageUsed), + AvailableSpare: cloneIntPtr(attrs.AvailableSpare), + MediaErrors: cloneInt64Ptr(attrs.MediaErrors), + UnsafeShutdowns: cloneInt64Ptr(attrs.UnsafeShutdowns), } - if attrs.PowerCycles != nil { - m.PowerCycles = *attrs.PowerCycles - } - if attrs.ReallocatedSectors != nil { - m.ReallocatedSectors = *attrs.ReallocatedSectors - } - if attrs.PendingSectors != nil { - m.PendingSectors = *attrs.PendingSectors - } - if attrs.OfflineUncorrectable != nil { - m.OfflineUncorrectable = *attrs.OfflineUncorrectable - } - if attrs.UDMACRCErrors != nil { - m.UDMACRCErrors = *attrs.UDMACRCErrors - } - if attrs.PercentageUsed != nil { - m.PercentageUsed = *attrs.PercentageUsed - } - if attrs.AvailableSpare != nil { - m.AvailableSpare = *attrs.AvailableSpare - } - if attrs.MediaErrors != nil { - m.MediaErrors = *attrs.MediaErrors - } - if attrs.UnsafeShutdowns != nil { - m.UnsafeShutdowns = *attrs.UnsafeShutdowns - } - return m } func physicalDiskTags(disk models.PhysicalDisk) []string { diff --git a/internal/unifiedresources/canonical_ids_types_test.go b/internal/unifiedresources/canonical_ids_types_test.go index 5fedbd238c..9843328de0 100644 --- a/internal/unifiedresources/canonical_ids_types_test.go +++ b/internal/unifiedresources/canonical_ids_types_test.go @@ -48,6 +48,43 @@ func TestHostSMARTMetaCarriesSizeBytesJSONContract(t *testing.T) { } } +func TestSMARTMetaJSONPreservesReportedZeroAndOmitsAbsentFields(t *testing.T) { + zeroInt := 0 + zeroInt64 := int64(0) + payload := SMARTMeta{ + PercentageUsed: &zeroInt, + AvailableSpare: &zeroInt, + ReallocatedSectors: &zeroInt64, + } + + data, err := json.Marshal(payload) + if err != nil { + t.Fatalf("marshal SMARTMeta: %v", err) + } + for _, expected := range []string{ + `"percentageUsed":0`, + `"availableSpare":0`, + `"reallocatedSectors":0`, + } { + if !strings.Contains(string(data), expected) { + t.Fatalf("reported zero field %s was omitted: %s", expected, data) + } + } + if strings.Contains(string(data), `"powerOnHours"`) { + t.Fatalf("absent SMART field was serialized: %s", data) + } + + var decoded SMARTMeta + if err := json.Unmarshal(data, &decoded); err != nil { + t.Fatalf("unmarshal SMARTMeta: %v", err) + } + if decoded.PercentageUsed == nil || *decoded.PercentageUsed != 0 || + decoded.AvailableSpare == nil || *decoded.AvailableSpare != 0 || + decoded.ReallocatedSectors == nil || *decoded.ReallocatedSectors != 0 { + t.Fatalf("reported zero fields did not survive round trip: %+v", decoded) + } +} + func TestContractResourceType(t *testing.T) { tests := []struct { name string diff --git a/internal/unifiedresources/clone.go b/internal/unifiedresources/clone.go index 58be2da018..8ffd35968a 100644 --- a/internal/unifiedresources/clone.go +++ b/internal/unifiedresources/clone.go @@ -714,8 +714,18 @@ func cloneSMARTMeta(in *SMARTMeta) *SMARTMeta { if in == nil { return nil } - out := *in - return &out + return &SMARTMeta{ + PowerOnHours: cloneInt64Ptr(in.PowerOnHours), + PowerCycles: cloneInt64Ptr(in.PowerCycles), + ReallocatedSectors: cloneInt64Ptr(in.ReallocatedSectors), + PendingSectors: cloneInt64Ptr(in.PendingSectors), + OfflineUncorrectable: cloneInt64Ptr(in.OfflineUncorrectable), + UDMACRCErrors: cloneInt64Ptr(in.UDMACRCErrors), + PercentageUsed: cloneIntPtr(in.PercentageUsed), + AvailableSpare: cloneIntPtr(in.AvailableSpare), + MediaErrors: cloneInt64Ptr(in.MediaErrors), + UnsafeShutdowns: cloneInt64Ptr(in.UnsafeShutdowns), + } } func clonePhysicalDiskRisk(in *PhysicalDiskRisk) *PhysicalDiskRisk { diff --git a/internal/unifiedresources/issue1516_physical_disk_regression_test.go b/internal/unifiedresources/issue1516_physical_disk_regression_test.go new file mode 100644 index 0000000000..50534ae8eb --- /dev/null +++ b/internal/unifiedresources/issue1516_physical_disk_regression_test.go @@ -0,0 +1,248 @@ +package unifiedresources + +import ( + "testing" + "time" + + "github.com/rcourtman/pulse-go-rewrite/internal/models" +) + +func TestIssue1516SeededControllerMembersKeepStableSourceIDsAcrossRestart(t *testing.T) { + parentID := "host-canonical" + seeded := []Resource{ + { + ID: parentID, + Type: ResourceTypeAgent, + Name: "pve-wide", + Sources: []DataSource{SourceAgent}, + Agent: &AgentData{AgentID: "host-1", Hostname: "pve-wide"}, + }, + { + ID: "disk-member-1", + Type: ResourceTypePhysicalDisk, + Name: "member-1", + ParentID: &parentID, + Sources: []DataSource{SourceAgent}, + PhysicalDisk: &PhysicalDiskMeta{ + DevPath: "/dev/sg2", + Controller: "sssraid0", + Target: "sssraid,0,1", + Wearout: -1, + }, + }, + { + ID: "disk-member-2", + Type: ResourceTypePhysicalDisk, + Name: "member-2", + ParentID: &parentID, + Sources: []DataSource{SourceAgent}, + PhysicalDisk: &PhysicalDiskMeta{ + DevPath: "/dev/sg2", + Controller: "sssraid0", + Target: "sssraid,0,2", + Wearout: -1, + }, + }, + } + + restarted := NewRegistry(nil) + restarted.IngestResources(seeded) + seededIDByTarget := make(map[string]string) + for _, disk := range restarted.ListByType(ResourceTypePhysicalDisk) { + if disk.PhysicalDisk != nil { + seededIDByTarget[disk.PhysicalDisk.Target] = disk.ID + } + } + + for _, test := range []struct { + sourceID string + target string + }{ + {"host-1:sg2@sssraid0/sssraid,0,1", "sssraid,0,1"}, + {"host-1:sg2@sssraid0/sssraid,0,2", "sssraid,0,2"}, + } { + got := restarted.ingest( + SourceAgent, + test.sourceID, + Resource{ + Type: ResourceTypePhysicalDisk, + Name: test.target, + ParentID: &parentID, + PhysicalDisk: &PhysicalDiskMeta{ + DevPath: "/dev/sg2", + Controller: "sssraid0", + Target: test.target, + Wearout: -1, + }, + }, + ResourceIdentity{Hostnames: []string{"pve-wide"}}, + ) + if want := seededIDByTarget[test.target]; want == "" || got != want { + t.Fatalf("restart source %q resolved to %q, want seeded ID %q", test.sourceID, got, want) + } + } + if got := len(restarted.ListByType(ResourceTypePhysicalDisk)); got != 2 { + t.Fatalf("controller members collapsed or duplicated after restart: got %d disks", got) + } +} + +func TestIssue1516AgentSMARTAndPVEInventoryMergeWithoutContradictingEvidence(t *testing.T) { + registry := NewRegistry(nil) + now := time.Date(2026, 7, 23, 12, 0, 0, 0, time.UTC) + used := 95 + zero := int64(0) + identity := ResourceIdentity{ + MachineID: "NVME-BOOT-1", + Hostnames: []string{"pve-a"}, + } + + agentID := registry.ingest( + SourceAgent, + "host-1:nvme0n1", + Resource{ + Type: ResourceTypePhysicalDisk, + Name: "NVMe boot disk", + Status: StatusWarning, + LastSeen: now, + PhysicalDisk: &PhysicalDiskMeta{ + DevPath: "/dev/nvme0n1", + Model: "Samsung 990 PRO", + Serial: "NVME-BOOT-1", + DiskType: "nvme", + Health: "FAILED", + Wearout: 5, + Temperature: 44, + SMART: &SMARTMeta{ + PercentageUsed: &used, + ReallocatedSectors: &zero, + }, + }, + }, + identity, + ) + pveID := registry.ingest( + SourceProxmox, + "cluster-a-pve-a--dev-nvme0n1", + Resource{ + Type: ResourceTypePhysicalDisk, + Name: "Samsung 990 PRO", + Status: StatusOnline, + LastSeen: now, + Proxmox: &ProxmoxData{ + Instance: "cluster-a", + NodeName: "pve-a", + }, + PhysicalDisk: &PhysicalDiskMeta{ + DevPath: "/dev/nvme0n1", + Model: "Samsung 990 PRO", + Vendor: "Samsung", + Serial: "NVME-BOOT-1", + DiskType: "ssd", + SizeBytes: 2_000_398_934_016, + Health: "PASSED", + Wearout: 100, + Used: "BIOS boot", + }, + }, + identity, + ) + if pveID != agentID { + t.Fatalf("agent and PVE views did not merge: agent=%q pve=%q", agentID, pveID) + } + + disks := registry.ListByType(ResourceTypePhysicalDisk) + if len(disks) != 1 || disks[0].PhysicalDisk == nil { + t.Fatalf("expected one merged physical disk, got %+v", disks) + } + disk := disks[0] + if disk.Proxmox == nil || disk.Proxmox.Instance != "cluster-a" || disk.Proxmox.NodeName != "pve-a" { + t.Fatalf("PVE node/instance facet was lost: %+v", disk.Proxmox) + } + if disk.PhysicalDisk.SizeBytes != 2_000_398_934_016 || + disk.PhysicalDisk.Vendor != "Samsung" || + disk.PhysicalDisk.Used != "BIOS boot" { + t.Fatalf("PVE-only boot/size fields were lost: %+v", disk.PhysicalDisk) + } + if disk.PhysicalDisk.Health != "FAILED" || + disk.PhysicalDisk.Wearout != 5 || + disk.PhysicalDisk.Temperature != 44 { + t.Fatalf("coarse PVE values replaced SMART evidence: %+v", disk.PhysicalDisk) + } + if disk.PhysicalDisk.SMART == nil || + disk.PhysicalDisk.SMART.ReallocatedSectors == nil || + *disk.PhysicalDisk.SMART.ReallocatedSectors != 0 { + t.Fatalf("reported zero SMART evidence was lost: %+v", disk.PhysicalDisk.SMART) + } +} + +func TestIssue1516ProxmoxSourceIDsSeparateNodesAndControllerMembers(t *testing.T) { + nodeA := ProxmoxPhysicalDiskSourceID("cluster-a", "pve-a", "/dev/sda", "", "") + nodeB := ProxmoxPhysicalDiskSourceID("cluster-a", "pve-b", "/dev/sda", "", "") + member1 := ProxmoxPhysicalDiskSourceID("cluster-a", "pve-a", "/dev/sg2", "sssraid0", "sssraid,0,1") + member2 := ProxmoxPhysicalDiskSourceID("cluster-a", "pve-a", "/dev/sg2", "sssraid0", "sssraid,0,2") + for _, pair := range [][2]string{ + {nodeA, nodeB}, + {member1, member2}, + {nodeA, member1}, + } { + if pair[0] == pair[1] { + t.Fatalf("distinct physical disks shared source ID %q", pair[0]) + } + } +} + +func TestIssue1516RepeatedSerialAcrossNodesDoesNotCrossAttachMetrics(t *testing.T) { + registry := NewRegistry(nil) + for _, node := range []string{"pve-a", "pve-b"} { + registry.ingest( + SourceProxmox, + ProxmoxPhysicalDiskSourceID("cluster-a", node, "/dev/sda", "", ""), + Resource{ + Type: ResourceTypePhysicalDisk, + Name: node + "-disk", + PhysicalDisk: &PhysicalDiskMeta{ + DevPath: "/dev/sda", + Serial: "DEFAULT-SERIAL", + DiskType: "sata", + Temperature: map[string]int{"pve-a": 31, "pve-b": 47}[node], + Wearout: -1, + }, + }, + ResourceIdentity{ + Hostnames: []string{node}, + }, + ) + } + + disks := registry.ListByType(ResourceTypePhysicalDisk) + if len(disks) != 2 { + t.Fatalf("same placeholder serial on different nodes collapsed to %d resource(s): %+v", len(disks), disks) + } + temperatureByHost := make(map[string]int) + for _, disk := range disks { + if disk.PhysicalDisk == nil || len(disk.Identity.Hostnames) != 1 { + t.Fatalf("unexpected disk projection: %+v", disk) + } + temperatureByHost[disk.Identity.Hostnames[0]] = disk.PhysicalDisk.Temperature + } + if temperatureByHost["pve-a"] != 31 || temperatureByHost["pve-b"] != 47 { + t.Fatalf("disk metrics crossed node boundaries: %+v", temperatureByHost) + } + metricA := PhysicalDiskMetricID(models.PhysicalDisk{ + ID: ProxmoxPhysicalDiskSourceID("cluster-a", "pve-a", "/dev/sda", "", ""), + Instance: "cluster-a", + Node: "pve-a", + DevPath: "/dev/sda", + Serial: "DEFAULT-SERIAL", + }) + metricB := PhysicalDiskMetricID(models.PhysicalDisk{ + ID: ProxmoxPhysicalDiskSourceID("cluster-a", "pve-b", "/dev/sda", "", ""), + Instance: "cluster-a", + Node: "pve-b", + DevPath: "/dev/sda", + Serial: "DEFAULT-SERIAL", + }) + if metricA == "" || metricB == "" || metricA == metricB { + t.Fatalf("placeholder serial shared metric key: pve-a=%q pve-b=%q", metricA, metricB) + } +} diff --git a/internal/unifiedresources/physical_disk_ids.go b/internal/unifiedresources/physical_disk_ids.go index 1c85ba2a31..7e2642d5a7 100644 --- a/internal/unifiedresources/physical_disk_ids.go +++ b/internal/unifiedresources/physical_disk_ids.go @@ -12,10 +12,10 @@ import ( // physical-disk metrics across sources. Stable hardware identity wins; a // source-specific fallback is only used when the disk exposes no serial/WWN. func PreferredPhysicalDiskMetricID(serial, wwn, fallback string) string { - if serial = strings.TrimSpace(serial); serial != "" { + if serial = strings.TrimSpace(serial); diskinventory.IsUsableHardwareID(serial) { return serial } - if wwn = strings.TrimSpace(wwn); wwn != "" { + if wwn = strings.TrimSpace(wwn); diskinventory.IsUsableHardwareID(wwn) { return wwn } return strings.TrimSpace(fallback) @@ -44,20 +44,45 @@ func HostUnraidDiskSourceID(host models.Host, disk models.HostUnraidDisk) string return PreferredPhysicalDiskMetricID(disk.Serial, "", fallback) } +// ProxmoxPhysicalDiskSourceID returns the source-native ID used for physical +// disks produced by the Proxmox monitor. Direct SATA/NVMe/SAS devices retain +// the historical path-shaped ID. Controller members that share one kernel +// device include their member target so they cannot overwrite each other. +func ProxmoxPhysicalDiskSourceID(instance, node, device, controller, target string) string { + legacy := fmt.Sprintf( + "%s-%s-%s", + strings.TrimSpace(instance), + strings.TrimSpace(node), + strings.ReplaceAll(strings.TrimSpace(device), "/", "-"), + ) + if !diskinventory.IsControllerMemberTarget(target) { + return legacy + } + if topologyID := diskinventory.PreferredID("", "", legacy, device, controller, target); topologyID != "" { + return topologyID + } + return legacy +} + func PhysicalDiskMetricID(disk models.PhysicalDisk) string { - if strings.TrimSpace(disk.Serial) != "" || strings.TrimSpace(disk.WWN) != "" { + if diskinventory.IsUsableHardwareID(disk.Serial) || diskinventory.IsUsableHardwareID(disk.WWN) { return PreferredPhysicalDiskMetricID(disk.Serial, disk.WWN, "") } fallback := strings.TrimSpace(disk.ID) + canonicalFallback := ProxmoxPhysicalDiskSourceID( + disk.Instance, + disk.Node, + disk.DevPath, + disk.Controller, + disk.Target, + ) if fallback == "" && strings.TrimSpace(disk.DevPath) != "" { - fallback = fmt.Sprintf( - "%s-%s-%s", - strings.TrimSpace(disk.Instance), - strings.TrimSpace(disk.Node), - strings.ReplaceAll(strings.TrimSpace(disk.DevPath), "/", "-"), - ) + fallback = canonicalFallback } if diskinventory.IsControllerMemberTarget(disk.Target) { + if fallback == canonicalFallback { + return fallback + } return diskinventory.PreferredID( "", "", @@ -74,10 +99,10 @@ func PhysicalDiskMetaMetricID(disk *PhysicalDiskMeta, fallback string) string { if disk == nil { return strings.TrimSpace(fallback) } - if serial := strings.TrimSpace(disk.Serial); serial != "" { + if serial := strings.TrimSpace(disk.Serial); diskinventory.IsUsableHardwareID(serial) { return serial } - if wwn := strings.TrimSpace(disk.WWN); wwn != "" { + if wwn := strings.TrimSpace(disk.WWN); diskinventory.IsUsableHardwareID(wwn) { return wwn } if diskinventory.IsControllerMemberTarget(disk.Target) && strings.TrimSpace(fallback) != "" { diff --git a/internal/unifiedresources/physical_disk_risk.go b/internal/unifiedresources/physical_disk_risk.go index 81182c5408..1c94883d93 100644 --- a/internal/unifiedresources/physical_disk_risk.go +++ b/internal/unifiedresources/physical_disk_risk.go @@ -84,22 +84,56 @@ func physicalDiskAssessmentFromMeta(meta *PhysicalDiskMeta) storagehealth.Assess Health: meta.Health, Temperature: meta.Temperature, Wearout: meta.Wearout, + WearoutKnown: meta.Wearout > 0 || + (meta.Wearout == 0 && physicalDiskTypeIsNonRotational(meta.DiskType)), } if meta.SMART != nil { - sample.PowerOnHours = meta.SMART.PowerOnHours - sample.PowerCycles = meta.SMART.PowerCycles - sample.ReallocatedSectors = meta.SMART.ReallocatedSectors - sample.PendingSectors = meta.SMART.PendingSectors - sample.OfflineUncorrectable = meta.SMART.OfflineUncorrectable - sample.UDMACRCErrors = meta.SMART.UDMACRCErrors - sample.PercentageUsed = meta.SMART.PercentageUsed - sample.AvailableSpare = meta.SMART.AvailableSpare - sample.MediaErrors = meta.SMART.MediaErrors - sample.UnsafeShutdowns = meta.SMART.UnsafeShutdowns + if meta.SMART.PowerOnHours != nil { + sample.PowerOnHours = *meta.SMART.PowerOnHours + } + if meta.SMART.PowerCycles != nil { + sample.PowerCycles = *meta.SMART.PowerCycles + } + if meta.SMART.ReallocatedSectors != nil { + sample.ReallocatedSectors = *meta.SMART.ReallocatedSectors + } + if meta.SMART.PendingSectors != nil { + sample.PendingSectors = *meta.SMART.PendingSectors + } + if meta.SMART.OfflineUncorrectable != nil { + sample.OfflineUncorrectable = *meta.SMART.OfflineUncorrectable + } + if meta.SMART.UDMACRCErrors != nil { + sample.UDMACRCErrors = *meta.SMART.UDMACRCErrors + } + if meta.SMART.PercentageUsed != nil { + sample.PercentageUsed = *meta.SMART.PercentageUsed + sample.Wearout = 100 - *meta.SMART.PercentageUsed + sample.WearoutKnown = true + } + if meta.SMART.AvailableSpare != nil { + sample.AvailableSpare = *meta.SMART.AvailableSpare + sample.AvailableSpareKnown = true + } + if meta.SMART.MediaErrors != nil { + sample.MediaErrors = *meta.SMART.MediaErrors + } + if meta.SMART.UnsafeShutdowns != nil { + sample.UnsafeShutdowns = *meta.SMART.UnsafeShutdowns + } } return storagehealth.AssessSample(sample) } +func physicalDiskTypeIsNonRotational(diskType string) bool { + switch strings.ToLower(strings.TrimSpace(diskType)) { + case "nvme", "ssd": + return true + default: + return false + } +} + func physicalDiskStatus(model, health string, assessment storagehealth.Assessment) ResourceStatus { switch assessment.Level { case storagehealth.RiskCritical, storagehealth.RiskWarning: diff --git a/internal/unifiedresources/physical_disk_risk_test.go b/internal/unifiedresources/physical_disk_risk_test.go index 64bb78970a..1065276431 100644 --- a/internal/unifiedresources/physical_disk_risk_test.go +++ b/internal/unifiedresources/physical_disk_risk_test.go @@ -117,7 +117,7 @@ func TestPhysicalDiskAssessmentFromMeta_PendingSectors(t *testing.T) { meta := &PhysicalDiskMeta{ Health: "PASSED", SMART: &SMARTMeta{ - PendingSectors: 3, + PendingSectors: ptrInt64(3), }, } assessment := physicalDiskAssessmentFromMeta(meta) @@ -159,6 +159,22 @@ func TestPhysicalDiskAssessmentFromMeta_LowWearout(t *testing.T) { } } +func TestPhysicalDiskAssessmentFromMeta_PercentageUsedProvesZeroLifeOnSATASSD(t *testing.T) { + percentageUsed := 100 + meta := &PhysicalDiskMeta{ + DiskType: "sata", + Health: "PASSED", + Wearout: 0, + SMART: &SMARTMeta{ + PercentageUsed: &percentageUsed, + }, + } + assessment := physicalDiskAssessmentFromMeta(meta) + if assessment.Level != storagehealth.RiskCritical { + t.Fatalf("reported percentage-used=100 should prove zero remaining life, got %s", assessment.Level) + } +} + // --- physicalDiskStatus --- func TestPhysicalDiskStatus_CriticalAssessment(t *testing.T) { diff --git a/internal/unifiedresources/registry.go b/internal/unifiedresources/registry.go index eef80263c8..ea10491bf2 100644 --- a/internal/unifiedresources/registry.go +++ b/internal/unifiedresources/registry.go @@ -766,14 +766,17 @@ func (rr *ResourceRegistry) seedSourceIDForResourceLocked(resource *Resource, so if resource.PhysicalDisk == nil { return "" } - fallback := "" if parentSourceID := rr.seedParentSourceIDLocked(resource, SourceAgent); parentSourceID != "" { - device := normalizePhysicalDiskDeviceToken(resource.PhysicalDisk.DevPath) - if device != "" { - fallback = fmt.Sprintf("%s:%s", parentSourceID, device) - } + return diskinventory.PreferredID( + resource.PhysicalDisk.Serial, + resource.PhysicalDisk.WWN, + parentSourceID, + resource.PhysicalDisk.DevPath, + resource.PhysicalDisk.Controller, + resource.PhysicalDisk.Target, + ) } - return PreferredPhysicalDiskMetricID(resource.PhysicalDisk.Serial, resource.PhysicalDisk.WWN, fallback) + return PreferredPhysicalDiskMetricID(resource.PhysicalDisk.Serial, resource.PhysicalDisk.WWN, "") } case SourceDocker: if resource.Docker == nil { @@ -2417,6 +2420,20 @@ func (rr *ResourceRegistry) ingest(source DataSource, sourceID string, resource resource.parentBySource = make(map[DataSource]string) rr.setSourceParent(&resource, source, resource.ParentID) + // Rehydrated registries seed exact source mappings from the persisted + // unified snapshot. Honor that durable mapping before attempting weaker + // identity correlation, but fail closed if a colliding source key belongs + // to a different physical-disk parent. + if mappedID := rr.bySource[source][sourceID]; mappedID != "" { + if existing := rr.resources[mappedID]; existing != nil && + existing.Type == resource.Type && + (resource.Type != ResourceTypePhysicalDisk || + physicalDiskMatchScopeCompatible(existing, &resource)) { + rr.mergeInto(existing, resource, source) + return existing.ID + } + } + // Linked resources must be mutually linked to avoid one-sided/ambiguous auto-merges. if linked := rr.resolveLinkedResource(source, sourceID, resource); linked != "" { existing := rr.resources[linked] @@ -2457,7 +2474,7 @@ func (rr *ResourceRegistry) ingest(source DataSource, sourceID string, resource } if resource.Type == ResourceTypeAgent || resource.Type == ResourceTypePhysicalDisk { - if match, excluded := rr.findMatch(identity, resource.Type, candidateID); match != nil { + if match, excluded := rr.findMatch(resource, candidateID); match != nil { existing := rr.resources[match.ResourceB] if existing != nil { rr.mergeInto(existing, resource, source) @@ -2525,9 +2542,9 @@ func (rr *ResourceRegistry) mergeLinkedKubernetesNode( return true } -func (rr *ResourceRegistry) findMatch(identity ResourceIdentity, resourceType ResourceType, candidateID string) (*MatchResult, bool) { +func (rr *ResourceRegistry) findMatch(incoming Resource, candidateID string) (*MatchResult, bool) { excludedMatch := false - candidates := rr.matcher.FindCandidates(identity) + candidates := rr.matcher.FindCandidates(incoming.Identity) for _, candidate := range candidates { if candidate.ID == "" { continue @@ -2536,7 +2553,11 @@ func (rr *ResourceRegistry) findMatch(identity ResourceIdentity, resourceType Re continue } existing := rr.resources[candidate.ID] - if existing == nil || existing.Type != resourceType { + if existing == nil || existing.Type != incoming.Type { + continue + } + if incoming.Type == ResourceTypePhysicalDisk && + !physicalDiskMatchScopeCompatible(existing, &incoming) { continue } if rr.isExcluded(candidate.ID, candidateID) { @@ -2554,6 +2575,24 @@ func (rr *ResourceRegistry) findMatch(identity ResourceIdentity, resourceType Re return nil, excludedMatch } +func physicalDiskMatchScopeCompatible(existing, incoming *Resource) bool { + if existing == nil || incoming == nil { + return false + } + existingParent := "" + if existing.ParentID != nil { + existingParent = CanonicalResourceID(strings.TrimSpace(*existing.ParentID)) + } + incomingParent := "" + if incoming.ParentID != nil { + incomingParent = CanonicalResourceID(strings.TrimSpace(*incoming.ParentID)) + } + if existingParent != "" || incomingParent != "" { + return existingParent != "" && existingParent == incomingParent + } + return identitiesShareHostname(existing.Identity, incoming.Identity) +} + func (rr *ResourceRegistry) resolveLinkedResource(source DataSource, sourceID string, resource Resource) string { if resource.Type == ResourceTypePhysicalDisk && (source == SourceAgent || source == SourceProxmox) { @@ -3072,8 +3111,7 @@ func (rr *ResourceRegistry) mergeInto(existing *Resource, incoming Resource, sou existing.PhysicalDisk.Wearout = previous.Wearout } if previous.SMART != nil { - smart := *previous.SMART - existing.PhysicalDisk.SMART = &smart + existing.PhysicalDisk.SMART = cloneSMARTMeta(previous.SMART) } if previous.IO != nil { existing.PhysicalDisk.IO = clonePhysicalDiskIOMeta(previous.IO) @@ -3305,6 +3343,9 @@ func mergePhysicalDiskData(existing *PhysicalDiskMeta, incoming *PhysicalDiskMet if incoming.Model != "" { merged.Model = incoming.Model } + if incoming.Vendor != "" { + merged.Vendor = incoming.Vendor + } if incoming.Serial != "" { merged.Serial = incoming.Serial } @@ -3367,8 +3408,7 @@ func mergePhysicalDiskData(existing *PhysicalDiskMeta, incoming *PhysicalDiskMet } merged.Collection = diskinventory.MergeStatus(merged.Collection, incoming.Collection) if incoming.SMART != nil { - smart := *incoming.SMART - merged.SMART = &smart + merged.SMART = cloneSMARTMeta(incoming.SMART) } merged.Risk = mergePhysicalDiskRisk( physicalDiskRiskFromAssessment(physicalDiskAssessmentFromMeta(&merged)), diff --git a/internal/unifiedresources/types.go b/internal/unifiedresources/types.go index 6e0c1b2de6..ce92d662fd 100644 --- a/internal/unifiedresources/types.go +++ b/internal/unifiedresources/types.go @@ -448,6 +448,7 @@ type ResourceIncident struct { type PhysicalDiskMeta struct { DevPath string `json:"devPath"` Model string `json:"model,omitempty"` + Vendor string `json:"vendor,omitempty"` Serial string `json:"serial,omitempty"` WWN string `json:"wwn,omitempty"` DiskType string `json:"diskType"` // nvme, sata, sas @@ -550,16 +551,16 @@ type CephServiceMeta struct { // SMARTMeta contains SMART attribute data for a physical disk. type SMARTMeta struct { - PowerOnHours int64 `json:"powerOnHours,omitempty"` - PowerCycles int64 `json:"powerCycles,omitempty"` - ReallocatedSectors int64 `json:"reallocatedSectors,omitempty"` - PendingSectors int64 `json:"pendingSectors,omitempty"` - OfflineUncorrectable int64 `json:"offlineUncorrectable,omitempty"` - UDMACRCErrors int64 `json:"udmaCrcErrors,omitempty"` - PercentageUsed int `json:"percentageUsed,omitempty"` - AvailableSpare int `json:"availableSpare,omitempty"` - MediaErrors int64 `json:"mediaErrors,omitempty"` - UnsafeShutdowns int64 `json:"unsafeShutdowns,omitempty"` + PowerOnHours *int64 `json:"powerOnHours,omitempty"` + PowerCycles *int64 `json:"powerCycles,omitempty"` + ReallocatedSectors *int64 `json:"reallocatedSectors,omitempty"` + PendingSectors *int64 `json:"pendingSectors,omitempty"` + OfflineUncorrectable *int64 `json:"offlineUncorrectable,omitempty"` + UDMACRCErrors *int64 `json:"udmaCrcErrors,omitempty"` + PercentageUsed *int `json:"percentageUsed,omitempty"` + AvailableSpare *int `json:"availableSpare,omitempty"` + MediaErrors *int64 `json:"mediaErrors,omitempty"` + UnsafeShutdowns *int64 `json:"unsafeShutdowns,omitempty"` } // HostSensorMeta contains host sensor readings. diff --git a/internal/unifiedresources/views_test.go b/internal/unifiedresources/views_test.go index 9d58bac937..d594e10a7c 100644 --- a/internal/unifiedresources/views_test.go +++ b/internal/unifiedresources/views_test.go @@ -10,6 +10,7 @@ import ( ) func ptrInt64(v int64) *int64 { return &v } +func ptrInt(v int) *int { return &v } func assertStringSlice(t *testing.T, got, want []string) { t.Helper() @@ -910,7 +911,7 @@ func TestView_PhysicalDiskViewAccessors(t *testing.T) { Temperature: 41, RPM: 0, Used: "zfs", - SMART: &SMARTMeta{PowerOnHours: 10, MediaErrors: 2}, + SMART: &SMARTMeta{PowerOnHours: ptrInt64(10), MediaErrors: ptrInt64(2)}, }, } @@ -928,7 +929,9 @@ func TestView_PhysicalDiskViewAccessors(t *testing.T) { if v.Health() != "PASSED" || v.Wearout() != 3 || v.Temperature() != 41 || v.RPM() != 0 || v.Used() != "zfs" { t.Fatalf("expected disk state to match, got health=%q wearout=%d temp=%d rpm=%d used=%q", v.Health(), v.Wearout(), v.Temperature(), v.RPM(), v.Used()) } - if smart := v.SMART(); smart == nil || smart.PowerOnHours != 10 || smart.MediaErrors != 2 { + if smart := v.SMART(); smart == nil || + smart.PowerOnHours == nil || *smart.PowerOnHours != 10 || + smart.MediaErrors == nil || *smart.MediaErrors != 2 { t.Fatalf("expected SMART metadata to match, got %+v", smart) } if !v.LastSeen().Equal(now) || v.ParentID() != parentID || v.ParentName() != "" { diff --git a/pkg/diskinventory/identity.go b/pkg/diskinventory/identity.go index b2c8dc6180..62d47ac42b 100644 --- a/pkg/diskinventory/identity.go +++ b/pkg/diskinventory/identity.go @@ -19,10 +19,10 @@ func DeviceToken(device string) string { // fallbacks to the reporting host. Controller and target are included only // when present, preserving legacy direct-device IDs for SATA/NVMe disks. func PreferredID(serial, wwn, scope, device, controller, target string) string { - if serial = strings.TrimSpace(serial); serial != "" { + if serial = strings.TrimSpace(serial); IsUsableHardwareID(serial) { return serial } - if wwn = strings.TrimSpace(wwn); wwn != "" { + if wwn = strings.TrimSpace(wwn); IsUsableHardwareID(wwn) { return wwn } @@ -39,18 +39,42 @@ func PreferredID(serial, wwn, scope, device, controller, target string) string { return fmt.Sprintf("%s:%s@%s/%s", scope, device, controller, target) } +// IsUsableHardwareID rejects controller placeholders that are not unique disk +// identities. Treating these as real serials collapses different disks and +// sends their SMART history to the same metric key. +func IsUsableHardwareID(value string) bool { + value = strings.TrimSpace(value) + if value == "" { + return false + } + upper := strings.ToUpper(value) + switch upper { + case "UNKNOWN", "N/A", "NA", "NONE", "NULL", "DEFAULT", "DEFAULT-SERIAL", + "TO BE FILLED BY O.E.M.", "0123456789": + return false + } + compact := strings.NewReplacer("-", "", ":", "", ".", "", " ", "").Replace(upper) + if compact == "" { + return false + } + allZero := true + allF := true + for _, char := range compact { + allZero = allZero && char == '0' + allF = allF && char == 'F' + } + return !allZero && !allF +} + // IsControllerMemberTarget reports whether target addresses one member behind -// a shared controller block path (for example megaraid,7 or cciss,1). +// a shared controller block path. Controller grammars vary after the numeric +// member prefix (for example megaraid,7, areca,1/1, and sssraid,0,1). func IsControllerMemberTarget(target string) bool { target = strings.TrimSpace(target) index := strings.IndexByte(target, ',') if index < 0 || index+1 >= len(target) { return false } - for _, char := range target[index+1:] { - if char < '0' || char > '9' { - return false - } - } - return true + next := target[index+1] + return next >= '0' && next <= '9' } diff --git a/pkg/diskinventory/identity_test.go b/pkg/diskinventory/identity_test.go index 60c0103212..b130141c7e 100644 --- a/pkg/diskinventory/identity_test.go +++ b/pkg/diskinventory/identity_test.go @@ -14,6 +14,8 @@ func TestPreferredIDPreservesDirectDeviceFallbacks(t *testing.T) { {name: "nvme", device: "nvme0n1", want: "host:nvme0n1"}, {name: "direct sas hctl", device: "sdb", controller: "0000:03:00.0", target: "6:0:0:0", want: "host:sdb"}, {name: "controller member", device: "sdc [megaraid,7]", controller: "sdc", target: "megaraid,7", want: "host:sdc@sdc/megaraid,7"}, + {name: "areca member", device: "sdc [areca,1/1]", controller: "arcmsr0", target: "areca,1/1", want: "host:sdc@arcmsr0/areca,1/1"}, + {name: "sssraid member", device: "sg2 [sssraid,0,1]", controller: "sssraid0", target: "sssraid,0,1", want: "host:sg2@sssraid0/sssraid,0,1"}, } { t.Run(test.name, func(t *testing.T) { got := PreferredID("", "", "host", test.device, test.controller, test.target) @@ -32,3 +34,23 @@ func TestPreferredIDKeepsExistingHardwareIdentityPriority(t *testing.T) { t.Fatalf("WWN identity = %q, want WWN", got) } } + +func TestPreferredIDRejectsPlaceholderHardwareIdentity(t *testing.T) { + for _, placeholder := range []string{ + "UNKNOWN", + "N/A", + "DEFAULT-SERIAL", + "0000-0000-0000", + "FFFF:FFFF", + } { + if IsUsableHardwareID(placeholder) { + t.Fatalf("placeholder %q was treated as usable hardware identity", placeholder) + } + if got := PreferredID(placeholder, "", "host-a", "/dev/sda", "", ""); got != "host-a:sda" { + t.Fatalf("placeholder %q produced ID %q, want scoped fallback", placeholder, got) + } + } + if !IsUsableHardwareID("ZR5DLAYJ") || !IsUsableHardwareID("naa.5000c500abcdef01") { + t.Fatal("real disk serial/WWN was rejected") + } +} diff --git a/pkg/proxmox/cluster_client.go b/pkg/proxmox/cluster_client.go index ec32b4c389..d189f9f3f1 100644 --- a/pkg/proxmox/cluster_client.go +++ b/pkg/proxmox/cluster_client.go @@ -1470,16 +1470,6 @@ func (cc *ClusterClient) GetDisks(ctx context.Context, node string) ([]Disk, err return nil }) - // Don't return error for transient connectivity issues - if err != nil && strings.Contains(err.Error(), "no healthy nodes available") { - log.Debug(). - Str("cluster", cc.name). - Str("node", node). - Err(err). - Msg("No healthy nodes for GetDisks - returning empty list") - return []Disk{}, nil - } - return result, err } diff --git a/pkg/proxmox/cluster_client_additional_api4_test.go b/pkg/proxmox/cluster_client_additional_api4_test.go index a0e9f8e535..59a4969829 100644 --- a/pkg/proxmox/cluster_client_additional_api4_test.go +++ b/pkg/proxmox/cluster_client_additional_api4_test.go @@ -5,6 +5,7 @@ import ( "fmt" "net/http" "net/http/httptest" + "strings" "testing" "time" ) @@ -106,10 +107,7 @@ func TestClusterClient_GetDisks_NoHealthyNodes(t *testing.T) { } disks, err := cc.GetDisks(context.Background(), "node1") - if err != nil { - t.Fatalf("expected no error, got %v", err) - } - if len(disks) != 0 { - t.Fatalf("expected empty disks on no healthy nodes, got %+v", disks) + if err == nil || !strings.Contains(err.Error(), "no healthy nodes available") { + t.Fatalf("expected no-healthy-nodes error, got disks=%+v err=%v", disks, err) } }