diff --git a/app/src/components/Nav.js b/app/src/components/Nav.js index f4c76281..231b22a6 100644 --- a/app/src/components/Nav.js +++ b/app/src/components/Nav.js @@ -195,22 +195,31 @@ export function NavBar() { if (driveTemperature && driveTemperature >= driveHighTemperature) { const color = driveTemperature >= driveCriticalTemperature ? highWarningColor : lowWarningColor; const icon = - const link = {icon}; + const link = {icon}; driveTemperatureIcon = ; } - // A drive's SMART self-assessment is failing — imminent drive failure. - // Always displayed (like power); too important to hide behind a transient - // warning in the priority chain. - const {failingDevices, failing: driveFailing} = useDriveHealth(); + // A drive's SMART health is degraded. FAIL is an imminent failure (back + // up now); WARN means unreadable/pending sectors are accumulating. Always + // displayed (like power); too important to hide behind a transient warning + // in the priority chain. Links to the Controller page, which shows the + // SMART detail (the Status page does not). + const {failingDevices, failing: driveFailing, warningDevices, warning: driveWarning} = useDriveHealth(); let driveHealthIcon; if (driveFailing) { const icon = ; - const link = {icon}; + const link = {icon}; const message = failingDevices.length === 1 ? `Drive ${failingDevices[0]} is failing its SMART health check! Back up your data.` : `Drives failing SMART health check: ${failingDevices.join(', ')}. Back up your data.`; driveHealthIcon = ; + } else if (driveWarning) { + const icon = ; + const link = {icon}; + const message = warningDevices.length === 1 + ? `Drive ${warningDevices[0]} has unreadable or pending sectors — check its SMART health.` + : `Drives with unreadable or pending sectors: ${warningDevices.join(', ')}. Check their SMART health.`; + driveHealthIcon = ; } // Power issues, this is always displayed if detected. diff --git a/app/src/components/admin/ControllerPage.js b/app/src/components/admin/ControllerPage.js index dc7d60b2..80e5fe57 100644 --- a/app/src/components/admin/ControllerPage.js +++ b/app/src/components/admin/ControllerPage.js @@ -559,7 +559,7 @@ const formatPowerOnHours = (hours) => { function SmartDetailsModal({drive, open, onClose}) { if (!drive) return null; - const healthColor = getHealthColor(drive.assessment); + const healthColor = getHealthColor(drive.health || drive.assessment); return ( @@ -617,6 +617,14 @@ function SmartDetailsModal({drive, open, onClose}) { : '-'} + + Uncorrectable Sectors + 0 ? 'orange' : 'inherit'}}> + {drive.offline_uncorrectable !== null && drive.offline_uncorrectable !== undefined + ? drive.offline_uncorrectable + : '-'} + + SMART Enabled {drive.smart_enabled ? 'Yes' : 'No'} @@ -1065,9 +1073,9 @@ function DiskSection() { - {drive.assessment || 'Unknown'} + {drive.health || drive.assessment || 'Unknown'} {drive.temperature !== null && drive.temperature !== undefined diff --git a/app/src/hooks/customHooks.js b/app/src/hooks/customHooks.js index 5bd1628e..8a6fde8e 100644 --- a/app/src/hooks/customHooks.js +++ b/app/src/hooks/customHooks.js @@ -1363,16 +1363,20 @@ export const useDriveTemperature = () => { } } -// Reports drives whose SMART self-assessment is failing, so the navbar can -// warn of an imminent drive failure. pySMART reports 'PASS', 'FAIL', or null -// (unknown); only 'FAIL' is a problem. +// Reports drives whose SMART health is degraded, so the navbar can warn of an +// imminent or developing drive failure. The backend's derived `health` is +// 'PASS', 'WARN', 'FAIL', or null (unknown): FAIL is an imminent failure +// (back up now), WARN means unreadable/pending sectors are accumulating. export const useDriveHealth = () => { const {status} = React.useContext(StatusContext); const drives = Array.isArray(status?.smart_stats?.drives) ? status.smart_stats.drives : []; - const failingDevices = drives - .filter((d) => d.assessment === 'FAIL') - .map((d) => d.device); - return {failingDevices, failing: failingDevices.length > 0}; + const verdict = (d) => (d.health || d.assessment); + const failingDevices = drives.filter((d) => verdict(d) === 'FAIL').map((d) => d.device); + const warningDevices = drives.filter((d) => verdict(d) === 'WARN').map((d) => d.device); + return { + failingDevices, failing: failingDevices.length > 0, + warningDevices, warning: warningDevices.length > 0, + }; } export const useLoad = () => { diff --git a/app/src/hooks/customHooks.test.js b/app/src/hooks/customHooks.test.js index f8cb1f60..c762081b 100644 --- a/app/src/hooks/customHooks.test.js +++ b/app/src/hooks/customHooks.test.js @@ -223,6 +223,43 @@ describe('useDriveHealth', () => { expect(result.current.failing).toBe(false); expect(result.current.failingDevices).toEqual([]); }); + + test('surfaces WARN drives separately from FAIL', () => { + const status = { + smart_stats: { + drives: [ + {device: 'sda', health: 'PASS', assessment: 'PASS'}, + {device: 'sdb', health: 'WARN', assessment: 'PASS'}, + {device: 'sdc', health: 'FAIL', assessment: 'FAIL'}, + ], + }, + }; + const {result} = renderHook(() => useDriveHealth(), + {wrapper: makeStatusWrapper(status)}); + expect(result.current.failingDevices).toEqual(['sdc']); + expect(result.current.warning).toBe(true); + expect(result.current.warningDevices).toEqual(['sdb']); + }); + + test('prefers derived health over raw assessment', () => { + // A drive self-assessing PASS but with pending sectors is WARN. + const status = { + smart_stats: {drives: [{device: 'sda', health: 'WARN', assessment: 'PASS'}]}, + }; + const {result} = renderHook(() => useDriveHealth(), + {wrapper: makeStatusWrapper(status)}); + expect(result.current.failing).toBe(false); + expect(result.current.warningDevices).toEqual(['sda']); + }); + + test('falls back to assessment when health is absent', () => { + const status = { + smart_stats: {drives: [{device: 'sda', assessment: 'FAIL'}]}, + }; + const {result} = renderHook(() => useDriveHealth(), + {wrapper: makeStatusWrapper(status)}); + expect(result.current.failingDevices).toEqual(['sda']); + }); }); describe('Paginator rendering', () => { diff --git a/controller/lib/smart.py b/controller/lib/smart.py index 9e989cba..cabf0eb9 100644 --- a/controller/lib/smart.py +++ b/controller/lib/smart.py @@ -2,6 +2,7 @@ SMART disk health monitoring for WROLPi Controller. """ +import re import subprocess from typing import Optional @@ -57,21 +58,47 @@ def get_all_smart_status() -> list[dict]: def _get_device_smart(device) -> dict: """Extract SMART data from a pySMART device.""" + pending = _get_attribute(device, "Current_Pending_Sector") + reallocated = _get_attribute(device, "Reallocated_Sector_Ct") + uncorrectable = _get_attribute(device, "Offline_Uncorrectable") return { "device": device.name, "path": f"/dev/{device.name}", "model": device.model, "serial": device.serial, "capacity": device.capacity, - "assessment": device.assessment, # PASS, FAIL, WARN + "assessment": device.assessment, # PASS, FAIL, WARN (pySMART's view) + "health": _derive_health(device.assessment, pending, uncorrectable), "temperature": _get_temperature(device), "power_on_hours": _get_attribute(device, "Power_On_Hours"), - "reallocated_sectors": _get_attribute(device, "Reallocated_Sector_Ct"), - "pending_sectors": _get_attribute(device, "Current_Pending_Sector"), + "reallocated_sectors": reallocated, + "pending_sectors": pending, + "offline_uncorrectable": uncorrectable, "smart_enabled": device.smart_enabled, } +def _derive_health(assessment, pending_sectors, offline_uncorrectable) -> Optional[str]: + """Derive the overall health verdict shown in the navbar and UIs. + + pySMART's ``assessment`` only reflects the drive's own pass/fail + self-assessment, which is driven by attributes flagged FAILING_NOW. A + drive can be quietly losing data to unreadable sectors while still + self-assessing PASS, so we downgrade to WARN when sectors are pending + reallocation or already uncorrectable. + + ``Reallocated_Sector_Ct`` is intentionally NOT a trigger: a small, + stable reallocated count is common and benign, and warning on it would + cry wolf. ``assessment`` of None (unsupported drive) passes through + unchanged. + """ + if (assessment or "").upper() == "FAIL": + return "FAIL" + if (pending_sectors or 0) > 0 or (offline_uncorrectable or 0) > 0: + return "WARN" + return assessment + + def _get_temperature(device) -> Optional[int]: """Get drive temperature. @@ -90,19 +117,34 @@ def _get_temperature(device) -> Optional[int]: def _get_attribute(device, name: str) -> Optional[int]: - """Get a specific SMART attribute value.""" + """Get a specific SMART attribute value as an integer.""" if not hasattr(device, 'attributes') or device.attributes is None: return None for attr in device.attributes: if attr and attr.name == name: - try: - return int(attr.raw) - except (ValueError, TypeError): - return None + return _parse_raw_int(attr.raw) return None +def _parse_raw_int(raw) -> Optional[int]: + """Parse the integer value out of a SMART attribute's raw field. + + smartctl/pySMART decorate some raw values in vendor-specific ways that + plain int() cannot handle: Seagate reports Power_On_Hours as + ``'50287h+00m+00.000s'`` and Temperature_Celsius as ``'53 (0 17 0 0 0)'``. + Extract the leading integer so these are not silently discarded. + """ + try: + return int(raw) + except (ValueError, TypeError): + pass + if raw is None: + return None + match = re.match(r'\s*(-?\d+)', str(raw)) + return int(match.group(1)) if match else None + + def _scan_devices() -> list[tuple[str, Optional[str]]]: """List SMART-capable devices via ``smartctl --scan``. diff --git a/controller/templates/index.html b/controller/templates/index.html index aa7a5807..72989518 100644 --- a/controller/templates/index.html +++ b/controller/templates/index.html @@ -1785,8 +1785,8 @@ let html = '
'; for (const drive of drives) { - const assessmentClass = getHealthClass(drive.assessment); - const assessmentText = drive.assessment || 'Unknown'; + const assessmentClass = getHealthClass(drive.health || drive.assessment); + const assessmentText = drive.health || drive.assessment || 'Unknown'; const tempText = drive.temperature !== null ? `${drive.temperature}°C` : '--'; html += ` @@ -1833,7 +1833,7 @@ title.textContent = `${drive.device} - Health Details`; - const assessmentClass = getHealthClass(drive.assessment); + const assessmentClass = getHealthClass(drive.health || drive.assessment); const formatHours = (hours) => { if (hours === null || hours === undefined) return '--'; const days = Math.floor(hours / 24); @@ -1877,6 +1877,10 @@ Pending Sectors ${drive.pending_sectors !== null ? drive.pending_sectors : '--'} + + Uncorrectable Sectors + ${drive.offline_uncorrectable !== null && drive.offline_uncorrectable !== undefined ? drive.offline_uncorrectable : '--'} + SMART Enabled ${drive.smart_enabled ? 'Yes' : 'No'} diff --git a/controller/test/test_smart.py b/controller/test/test_smart.py index 358c1e55..3d8478d1 100644 --- a/controller/test/test_smart.py +++ b/controller/test/test_smart.py @@ -4,6 +4,8 @@ Unit tests for controller.lib.smart module. from unittest import mock +import pytest + from controller.lib.smart import ( is_smart_available, get_all_smart_status, @@ -11,12 +13,22 @@ from controller.lib.smart import ( _get_device_smart, _get_temperature, _get_attribute, + _parse_raw_int, + _derive_health, _drive_is_asleep, HDD_HIGH_TEMPERATURE, HDD_CRITICAL_TEMPERATURE, ) +def _attr(name, raw): + a = mock.Mock() + a.name = name + a.raw = raw + a.when_failed = '' + return a + + class TestIsSmartAvailable: """Tests for is_smart_available function.""" @@ -90,8 +102,31 @@ class TestGetDeviceSmart: assert result["serial"] == "S1234" assert result["capacity"] == "500 GB" assert result["assessment"] == "PASS" + assert result["health"] == "PASS" assert result["smart_enabled"] is True + def test_warns_on_pending_sectors_despite_pass(self): + """A drive self-assessing PASS with pending sectors must surface a + WARN health while leaving the raw assessment untouched.""" + mock_device = mock.Mock() + mock_device.name = "sdc" + mock_device.model = "WDC WD140EDGZ" + mock_device.serial = "9LJ0BM0G" + mock_device.capacity = "14.0 TB" + mock_device.assessment = "PASS" + mock_device.smart_enabled = True + mock_device.attributes = [ + _attr("Current_Pending_Sector", "80"), + _attr("Offline_Uncorrectable", "51"), + _attr("Reallocated_Sector_Ct", "1"), + ] + + result = _get_device_smart(mock_device) + assert result["assessment"] == "PASS" + assert result["health"] == "WARN" + assert result["pending_sectors"] == 80 + assert result["offline_uncorrectable"] == 51 + class TestGetTemperature: """Tests for _get_temperature function.""" @@ -208,6 +243,48 @@ class TestGetAttribute: result = _get_attribute(mock_device, "Power_On_Hours") assert result is None + def test_parses_seagate_power_on_hours_format(self): + """Seagate reports Power_On_Hours as 'NNNNNh+00m+00.000s'; the leading + integer must be extracted rather than discarded to None.""" + mock_device = mock.Mock() + mock_device.attributes = [_attr("Power_On_Hours", "50287h+00m+00.000s")] + + result = _get_attribute(mock_device, "Power_On_Hours") + assert result == 50287 + + +class TestParseRawInt: + """Tests for _parse_raw_int — vendor-specific raw value parsing.""" + + @pytest.mark.parametrize("raw, expected", [ + ("80", 80), # plain string integer + (22225, 22225), # already an int + ("50287h+00m+00.000s", 50287), # Seagate Power_On_Hours + ("56 (Min/Max 17/66)", 56), # WD temperature decoration + ("53 (0 17 0 0 0)", 53), # Seagate temperature tuple + (None, None), # missing value + ("invalid", None), # no leading integer + ]) + def test_parse_raw_int(self, raw, expected): + assert _parse_raw_int(raw) == expected + + +class TestDeriveHealth: + """Tests for _derive_health — the verdict shown in the navbar/UIs.""" + + @pytest.mark.parametrize("assessment, pending, uncorrectable, expected", [ + ("PASS", 0, 0, "PASS"), # clean drive stays PASS + ("FAIL", 0, 0, "FAIL"), # self-assessed failure stays FAIL + ("PASS", 80, 0, "WARN"), # pending sectors downgrade PASS -> WARN + ("PASS", 0, 51, "WARN"), # uncorrectable sectors downgrade -> WARN + ("PASS", 0, 0, "PASS"), # reallocated is not a trigger (clean here) + ("FAIL", 80, 51, "FAIL"), # FAIL takes priority over sector WARN + (None, None, None, None), # unsupported drive passes through unknown + (None, 5, 0, "WARN"), # unknown assessment + pending -> WARN + ]) + def test_derive_health(self, assessment, pending, uncorrectable, expected): + assert _derive_health(assessment, pending, uncorrectable) == expected + class TestDriveIsAsleep: """Tests for _drive_is_asleep function (smartctl -n standby)."""