diff --git a/AppImage/scripts/health_persistence.py b/AppImage/scripts/health_persistence.py index a94d13e7..6230aec9 100644 --- a/AppImage/scripts/health_persistence.py +++ b/AppImage/scripts/health_persistence.py @@ -791,6 +791,7 @@ class HealthPersistence: resolution_type = COALESCE(resolution_type, 'auto'), resolution_reason = ? WHERE error_key = ? AND resolved_at IS NULL + AND NOT (acknowledged = 1 AND IFNULL(suppression_hours, 0) = -1) ''', (now, reason, error_key)) if cursor.rowcount > 0: @@ -904,10 +905,14 @@ class HealthPersistence: # Check if this error was acknowledged (dismissed) cursor.execute(''' - SELECT acknowledged FROM errors WHERE error_key = ? + SELECT acknowledged, suppression_hours FROM errors WHERE error_key = ? ''', (error_key,)) row = cursor.fetchone() + if row and row[0] == 1 and row[1] == -1: + # Silenced permanently: it stays silenced if the condition + # comes back, until the user re-enables it. + return if row and row[0] == 1: # Dismissed error that naturally resolved - delete entirely cursor.execute('DELETE FROM errors WHERE error_key = ?', (error_key,)) @@ -1297,6 +1302,13 @@ class HealthPersistence: now = datetime.now() now_iso = now.isoformat() + # A permanent dismissal that was closed only for its age is kept. + cursor.execute( + 'UPDATE errors SET resolved_at = NULL, resolution_type = NULL, resolution_reason = NULL ' + 'WHERE acknowledged = 1 AND suppression_hours = -1 AND resolution_reason IN (%s)' + % ','.join('?' * len(self._AGED_OUT_REASONS)), + tuple(sorted(self._AGED_OUT_REASONS))) + # Delete resolved errors older than 7 days cutoff_resolved = (now - timedelta(days=7)).isoformat() cursor.execute('DELETE FROM errors WHERE resolved_at < ?', (cutoff_resolved,)) @@ -1463,6 +1475,14 @@ class HealthPersistence: # NOTE: nothing here touches `disk_observations`. That table is the # permanent history of each disk and no cycle removes or hides rows. + # Why the stale cleanup closes a record when nothing else did: its age. + _AGED_OUT_REASONS = frozenset({ + 'Log error aged out (>48h)', + 'Temperature error stale (>24h no activity)', + 'Update/security notice aged out (>7d)', + 'Stale error (no activity >7d)', + }) + def _cleanup_stale_resources(self): """Resolve errors for resources that no longer exist. @@ -1490,7 +1510,8 @@ class HealthPersistence: # An error is considered unresolved if resolution_type is NULL or empty # (resolved_at alone is not sufficient - it may be in an inconsistent state) cursor.execute(''' - SELECT id, error_key, category, reason, first_seen, last_seen, severity FROM errors + SELECT id, error_key, category, reason, first_seen, last_seen, severity, + acknowledged, suppression_hours FROM errors WHERE resolution_type IS NULL OR resolution_type = '' ''') active_errors = cursor.fetchall() @@ -1694,7 +1715,9 @@ class HealthPersistence: return 0 for error_row in active_errors: - err_id, error_key, category, reason, first_seen, last_seen, severity = error_row + (err_id, error_key, category, reason, first_seen, last_seen, severity, + acknowledged, suppression_hours) = error_row + permanent = acknowledged == 1 and suppression_hours == -1 should_resolve = False resolution_reason = None age_hours = get_age_hours(first_seen) @@ -1899,6 +1922,12 @@ class HealthPersistence: should_resolve = True resolution_reason = 'Stale error (no activity >7d)' + # A problem silenced permanently is not recorded again on purpose, + # so its lack of activity says nothing: only its resource going + # away ends it. The user brings it back from Active Suppressions. + if should_resolve and permanent and resolution_reason in self._AGED_OUT_REASONS: + should_resolve = False + if should_resolve: cursor.execute(''' UPDATE errors SET resolved_at = ?, resolution_type = 'auto', resolution_reason = ? @@ -2348,6 +2377,10 @@ class HealthPersistence: updated_count = 0 for err_id, error_key, category, old_hours in dismissed: + if old_hours == -1: + # A permanent dismissal is the user's choice for that alert, + # and only ends from Active Suppressions. + continue setting_key = None for skey, meta in self._get_category_labels().items(): if meta['category'] == category: diff --git a/AppImage/scripts/notification_manager.py b/AppImage/scripts/notification_manager.py index 682d76e9..52a26c2c 100644 --- a/AppImage/scripts/notification_manager.py +++ b/AppImage/scripts/notification_manager.py @@ -1618,6 +1618,7 @@ class NotificationManager: # opt-in. Mirrors the same lists from _passes_cooldown. _DIGEST_EXEMPT_EVENTS = frozenset({ 'backup_complete', 'backup_fail', 'backup_start', + 'host_backup_complete', 'host_backup_fail', 'host_backup_start', 'replication_complete', 'replication_fail', 'vm_start', 'vm_stop', 'vm_shutdown', 'vm_restart', 'ct_start', 'ct_stop', 'ct_shutdown', 'ct_restart', @@ -2125,6 +2126,7 @@ class NotificationManager: # always be delivered. A 10s cooldown prevents exact duplicates # (webhook + tasks) but allows repeated backup jobs to report. _ALWAYS_DELIVER = {'backup_complete', 'backup_fail', 'backup_start', + 'host_backup_complete', 'host_backup_fail', 'host_backup_start', 'replication_complete', 'replication_fail'} if event.event_type in _ALWAYS_DELIVER and cooldown_str is None: cooldown = 10 diff --git a/AppImage/scripts/tests/test_host_backup_notifications_every_run.py b/AppImage/scripts/tests/test_host_backup_notifications_every_run.py new file mode 100644 index 00000000..b4036fde --- /dev/null +++ b/AppImage/scripts/tests/test_host_backup_notifications_every_run.py @@ -0,0 +1,44 @@ +"""Every run of a host backup is reported, the way a VM or CT backup is: a +nightly job is not the same event as the night before, and its result is not +kept for the daily digest.""" +import sys +from pathlib import Path +import unittest + +sys.path.insert(0, str(Path(__file__).resolve().parents[1])) +import notification_manager # noqa: E402 +from notification_events import NotificationEvent # noqa: E402 + +BACKUP_EVENTS = ('backup_start', 'backup_complete', 'backup_fail', + 'host_backup_start', 'host_backup_complete', 'host_backup_fail') + + +class HostBackupEveryRunTests(unittest.TestCase): + def setUp(self): + self.manager = notification_manager.NotificationManager.__new__(notification_manager.NotificationManager) + self.manager._config = {'email.digest_enabled': 'true'} + + def event(self, event_type, severity='INFO'): + return NotificationEvent(event_type, severity, {'hostname': 'pve'}, source='test', entity='node', entity_id='') + + def test_two_runs_a_day_apart_or_closer_are_both_sent(self): + for event_type in BACKUP_EVENTS: + with self.subTest(event_type=event_type): + severity = 'CRITICAL' if event_type.endswith('_fail') else 'INFO' + self.assertEqual(self.manager._cooldown_seconds(self.event(event_type, severity)), 10) + + def test_the_result_is_not_kept_for_the_daily_digest(self): + for event_type in BACKUP_EVENTS: + with self.subTest(event_type=event_type): + self.assertFalse(self.manager._should_buffer_for_digest('email', 'INFO', event_type)) + + def test_a_cooldown_set_by_the_user_is_still_respected(self): + self.manager._config['cooldown.host_backup_complete'] = '3600' + self.assertEqual(self.manager._cooldown_seconds(self.event('host_backup_complete')), 3600) + + def test_other_informational_events_still_go_to_the_digest(self): + self.assertTrue(self.manager._should_buffer_for_digest('email', 'INFO', 'update_summary')) + + +if __name__ == '__main__': + unittest.main() diff --git a/AppImage/scripts/tests/test_permanent_dismissal_is_kept.py b/AppImage/scripts/tests/test_permanent_dismissal_is_kept.py new file mode 100644 index 00000000..1da297bc --- /dev/null +++ b/AppImage/scripts/tests/test_permanent_dismissal_is_kept.py @@ -0,0 +1,108 @@ +"""An alert dismissed permanently stays dismissed until the user re-enables +it: not seeing it for a while, the condition recovering or a change of the +suppression settings does not bring it back.""" +import sqlite3 +import sys +import tempfile +import unittest +from datetime import datetime, timedelta +from pathlib import Path +from unittest.mock import patch + +SCRIPTS = Path(__file__).resolve().parents[1] +sys.path.insert(0, str(SCRIPTS)) +# The module opens the database of the host when it is imported. +_connect = sqlite3.connect +with patch.object(Path, 'mkdir'), patch('builtins.print'), \ + patch('sqlite3.connect', side_effect=lambda *args, **kwargs: _connect(':memory:')): + import health_persistence as hp_module + +KEY = 'pve_storage_full_local-lvm' + + +class PermanentDismissalTests(unittest.TestCase): + def setUp(self): + tmp = tempfile.TemporaryDirectory() + self.addCleanup(tmp.cleanup) + store = hp_module.HealthPersistence.__new__(hp_module.HealthPersistence) + for name, value in vars(hp_module.health_persistence).items(): + setattr(store, name, value) + store.data_dir = Path(tmp.name) + store.db_path = store.data_dir / 'health_monitor.db' + with patch('builtins.print'): + store._init_database() + self.store = store + + def seen(self): + return self.store.record_error(KEY, 'storage', 'WARNING', 'PVE storage local-lvm (lvmthin) at 96.0% used', + {'storage_name': 'local-lvm'}) + + def days_pass(self, days): + """Moves every date of the record back, as if that time had gone by.""" + conn = self.store._get_conn() + row = conn.execute('SELECT first_seen, last_seen, acknowledged_at, resolved_at FROM errors WHERE error_key = ?', + (KEY,)).fetchone() + if row: + moved = [(datetime.fromisoformat(v) - timedelta(days=days)).isoformat() if v else None for v in row] + conn.execute('UPDATE errors SET first_seen = ?, last_seen = ?, acknowledged_at = ?, resolved_at = ? ' + 'WHERE error_key = ?', (*moved, KEY)) + conn.commit() + conn.close() + + def cleanup(self): + with patch('builtins.print'), patch('subprocess.run', side_effect=OSError('no host tools in the test')): + self.store.cleanup_old_errors() + + def test_it_is_still_dismissed_weeks_later(self): + self.assertTrue(self.seen()['needs_notification']) + self.store.acknowledge_error(KEY, suppression_hours=-1) + for _ in range(6): + self.days_pass(8) + self.cleanup() + self.assertFalse(self.seen()['needs_notification']) + self.assertTrue(self.store.is_error_permanently_acknowledged(KEY)) + + def test_a_condition_that_recovers_and_returns_stays_dismissed(self): + self.seen() + self.store.acknowledge_error(KEY, suppression_hours=-1) + self.store.clear_error(KEY) + self.store.resolve_error(KEY, 'condition recovered') + self.days_pass(8) + self.cleanup() + self.assertFalse(self.seen()['needs_notification']) + self.assertTrue(self.store.is_error_permanently_acknowledged(KEY)) + + def test_a_record_already_closed_for_its_age_is_kept(self): + self.seen() + self.store.acknowledge_error(KEY, suppression_hours=-1) + conn = self.store._get_conn() + closed = (datetime.now() - timedelta(days=8)).isoformat() + conn.execute("UPDATE errors SET resolved_at = ?, resolution_type = 'auto', " + "resolution_reason = 'Stale error (no activity >7d)' WHERE error_key = ?", (closed, KEY)) + conn.commit() + conn.close() + self.cleanup() + self.assertFalse(self.seen()['needs_notification']) + + def test_saving_the_suppression_settings_leaves_it_permanent(self): + self.seen() + self.store.acknowledge_error(KEY, suppression_hours=-1) + with patch('builtins.print'): + self.store.sync_dismissed_suppression() + self.assertTrue(self.store.is_error_permanently_acknowledged(KEY)) + + def test_re_enabling_it_brings_the_alert_back(self): + self.seen() + self.store.acknowledge_error(KEY, suppression_hours=-1) + self.store.unacknowledge_error(KEY) + self.assertFalse(self.store.is_error_permanently_acknowledged(KEY)) + + def test_a_timed_dismissal_still_ends(self): + self.seen() + self.store.acknowledge_error(KEY, suppression_hours=24) + self.days_pass(2) + self.assertTrue(self.seen()['needs_notification']) + + +if __name__ == '__main__': + unittest.main()