From ce57a1cc8b8043f25d3abe4f106cd1fe028d6587 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?P=C4=93teris=20Caune?= Date: Fri, 4 Nov 2016 16:30:19 +0200 Subject: [PATCH] Calculate `alert_after` in Python code instead of a database trigger. This will allow complex calculations down the road. --- hc/api/management/commands/droptriggers.py | 36 ++++++++++ hc/api/management/commands/ensuretriggers.py | 70 -------------------- hc/api/management/commands/sendalerts.py | 27 +++++--- hc/api/models.py | 3 + hc/api/tests/test_ensuretriggers.py | 27 -------- hc/api/tests/test_ping.py | 3 +- hc/api/tests/test_sendalerts.py | 64 ++++++++++++++++++ hc/api/views.py | 2 +- hc/front/tests/test_update_timeout.py | 11 ++- hc/front/views.py | 3 + 10 files changed, 136 insertions(+), 110 deletions(-) create mode 100644 hc/api/management/commands/droptriggers.py delete mode 100644 hc/api/management/commands/ensuretriggers.py delete mode 100644 hc/api/tests/test_ensuretriggers.py diff --git a/hc/api/management/commands/droptriggers.py b/hc/api/management/commands/droptriggers.py new file mode 100644 index 00000000..6c9a672f --- /dev/null +++ b/hc/api/management/commands/droptriggers.py @@ -0,0 +1,36 @@ +from django.core.management.base import BaseCommand +from django.db import connection + + +def _pg(cursor): + cursor.execute(""" + DROP TRIGGER IF EXISTS update_alert_after ON api_check; + """) + + +def _mysql(cursor): + cursor.execute(""" + DROP TRIGGER IF EXISTS update_alert_after; + """) + + +def _sqlite(cursor): + cursor.execute(""" + DROP TRIGGER IF EXISTS update_alert_after; + """) + + +class Command(BaseCommand): + help = 'Drops the `update_alert_after` trigger' + + def handle(self, *args, **options): + with connection.cursor() as cursor: + if connection.vendor == "postgresql": + _pg(cursor) + return "Dropped PostgreSQL trigger" + if connection.vendor == "mysql": + _mysql(cursor) + return "Dropped MySQL trigger" + if connection.vendor == "sqlite": + _sqlite(cursor) + return "Dropped SQLite trigger" diff --git a/hc/api/management/commands/ensuretriggers.py b/hc/api/management/commands/ensuretriggers.py deleted file mode 100644 index fc699aba..00000000 --- a/hc/api/management/commands/ensuretriggers.py +++ /dev/null @@ -1,70 +0,0 @@ -from django.core.management.base import BaseCommand -from django.db import connection - - -def _pg(cursor): - cursor.execute(""" - CREATE OR REPLACE FUNCTION update_alert_after() - RETURNS trigger AS $update_alert_after$ - BEGIN - IF NEW.last_ping IS NOT NULL THEN - NEW.alert_after := NEW.last_ping + NEW.timeout + NEW.grace; - END IF; - RETURN NEW; - END; - $update_alert_after$ LANGUAGE plpgsql; - - DROP TRIGGER IF EXISTS update_alert_after ON api_check; - - CREATE TRIGGER update_alert_after - BEFORE INSERT OR UPDATE OF last_ping, timeout, grace ON api_check - FOR EACH ROW EXECUTE PROCEDURE update_alert_after(); - """) - - -def _mysql(cursor): - cursor.execute(""" - DROP TRIGGER IF EXISTS update_alert_after; - """) - - cursor.execute(""" - CREATE TRIGGER update_alert_after - BEFORE UPDATE ON api_check - FOR EACH ROW SET - NEW.alert_after = - NEW.last_ping + INTERVAL (NEW.timeout + NEW.grace) MICROSECOND; - """) - - -def _sqlite(cursor): - cursor.execute(""" - DROP TRIGGER IF EXISTS update_alert_after; - """) - - cursor.execute(""" - CREATE TRIGGER update_alert_after - AFTER UPDATE OF last_ping, timeout, grace ON api_check - FOR EACH ROW BEGIN - UPDATE api_check - SET alert_after = - datetime(strftime('%s', last_ping) + - timeout/1000000 + grace/1000000, 'unixepoch') - WHERE id = OLD.id; - END; - """) - - -class Command(BaseCommand): - help = 'Ensures triggers exist in database' - - def handle(self, *args, **options): - with connection.cursor() as cursor: - if connection.vendor == "postgresql": - _pg(cursor) - return "Created PostgreSQL trigger" - if connection.vendor == "mysql": - _mysql(cursor) - return "Created MySQL trigger" - if connection.vendor == "sqlite": - _sqlite(cursor) - return "Created SQLite trigger" diff --git a/hc/api/management/commands/sendalerts.py b/hc/api/management/commands/sendalerts.py index 420d5c9f..a72ea43e 100644 --- a/hc/api/management/commands/sendalerts.py +++ b/hc/api/management/commands/sendalerts.py @@ -17,6 +17,11 @@ def notify(check_id, stdout): stdout.write("ERROR: %s %s %s\n" % (ch.kind, ch.value, error)) +def notify_on_thread(check_id, stdout): + t = Thread(target=notify, args=(check_id, stdout)) + t.start() + + class Command(BaseCommand): help = 'Sends UP/DOWN email alerts' owned = Check.objects.filter(user__isnull=False) @@ -27,25 +32,31 @@ class Command(BaseCommand): now = timezone.now() # Look for checks that are going down - flipped = "down" q = self.owned.filter(alert_after__lt=now, status="up") check = q.first() + # If none found, look for checks that are going up if not check: - # If none found, look for checks that are going up - flipped = "up" q = self.owned.filter(alert_after__gt=now, status="down") check = q.first() - if check: + if check is None: + return False + + q = Check.objects.filter(id=check.id, status=check.status) + current_status = check.get_status() + if check.status == current_status: + # Stored status is already up-to-date. Update alert_after + # as needed but don't send notifications + q.update(alert_after=check.get_alert_after()) + return True + else: # Atomically update status to the opposite - q = Check.objects.filter(id=check.id, status=check.status) - num_updated = q.update(status=flipped) + num_updated = q.update(status=current_status) if num_updated == 1: # Send notifications only if status update succeeded # (no other sendalerts process got there first) - t = Thread(target=notify, args=(check.id, self.stdout)) - t.start() + notify_on_thread(check.id, self.stdout) return True return False diff --git a/hc/api/models.py b/hc/api/models.py index 7a4c729b..5d6e5bcd 100644 --- a/hc/api/models.py +++ b/hc/api/models.py @@ -96,6 +96,9 @@ class Check(models.Model): return "down" + def get_alert_after(self): + return self.last_ping + self.timeout + self.grace + def in_grace_period(self): if self.status in ("new", "paused"): return False diff --git a/hc/api/tests/test_ensuretriggers.py b/hc/api/tests/test_ensuretriggers.py deleted file mode 100644 index 129f2661..00000000 --- a/hc/api/tests/test_ensuretriggers.py +++ /dev/null @@ -1,27 +0,0 @@ -from datetime import timedelta - -from django.test import TestCase -from django.utils import timezone - -from hc.api.management.commands.ensuretriggers import Command -from hc.api.models import Check - - -class EnsureTriggersTestCase(TestCase): - - def test_ensure_triggers(self): - Command().handle() - - check = Check.objects.create() - assert check.alert_after is None - - check.last_ping = timezone.now() - check.save() - check.refresh_from_db() - assert check.alert_after is not None - alert_after = check.alert_after - - check.last_ping += timedelta(days=1) - check.save() - check.refresh_from_db() - assert check.alert_after > alert_after diff --git a/hc/api/tests/test_ping.py b/hc/api/tests/test_ping.py index 5dcfc497..43b0be67 100644 --- a/hc/api/tests/test_ping.py +++ b/hc/api/tests/test_ping.py @@ -14,7 +14,8 @@ class PingTestCase(TestCase): assert r.status_code == 200 self.check.refresh_from_db() - assert self.check.status == "up" + self.assertEqual(self.check.status, "up") + self.assertEqual(self.check.alert_after, self.check.get_alert_after()) ping = Ping.objects.latest("id") assert ping.scheme == "http" diff --git a/hc/api/tests/test_sendalerts.py b/hc/api/tests/test_sendalerts.py index 5ee29d2a..5136a487 100644 --- a/hc/api/tests/test_sendalerts.py +++ b/hc/api/tests/test_sendalerts.py @@ -1,4 +1,5 @@ from datetime import timedelta +from mock import patch from django.utils import timezone from hc.api.management.commands.sendalerts import Command @@ -12,7 +13,70 @@ class SendAlertsTestCase(BaseTestCase): check = Check(user=self.alice, status="up") # 1 day 30 minutes after ping the check is in grace period: check.last_ping = timezone.now() - timedelta(days=1, minutes=30) + check.alert_after = check.get_alert_after() check.save() # Expect no exceptions-- Command().handle_one() + + @patch("hc.api.management.commands.sendalerts.notify_on_thread") + def test_it_notifies_when_check_goes_down(self, mock_notify): + check = Check(user=self.alice, status="up") + check.last_ping = timezone.now() - timedelta(days=2) + check.alert_after = check.get_alert_after() + check.save() + + result = Command().handle_one() + + # If it finds work, it should return True + self.assertTrue(result) + + # It should change stored status to "down" + check.refresh_from_db() + self.assertEqual(check.status, "down") + + # It should call `notify` + self.assertTrue(mock_notify.called) + + @patch("hc.api.management.commands.sendalerts.notify_on_thread") + def test_it_notifies_when_check_goes_up(self, mock_notify): + check = Check(user=self.alice, status="down") + check.last_ping = timezone.now() + check.alert_after = check.get_alert_after() + check.save() + + result = Command().handle_one() + + # If it finds work, it should return True + self.assertTrue(result) + + # It should change stored status to "up" + check.refresh_from_db() + self.assertEqual(check.status, "up") + + # It should call `notify` + self.assertTrue(mock_notify.called) + + # alert_after now should be set + self.assertTrue(check.alert_after) + + @patch("hc.api.management.commands.sendalerts.notify_on_thread") + def test_it_updates_alert_after(self, mock_notify): + check = Check(user=self.alice, status="up") + check.last_ping = timezone.now() - timedelta(hours=1) + check.alert_after = check.last_ping + check.save() + + result = Command().handle_one() + + # If it finds work, it should return True + self.assertTrue(result) + + # It should change stored status to "down" + check.refresh_from_db() + + # alert_after should have been increased + self.assertTrue(check.alert_after > check.last_ping) + + # notify should *not* have been called + self.assertFalse(mock_notify.called) diff --git a/hc/api/views.py b/hc/api/views.py index 5b7ca270..7f57d5ac 100644 --- a/hc/api/views.py +++ b/hc/api/views.py @@ -1,6 +1,5 @@ from datetime import timedelta as td -from django.core.exceptions import FieldError from django.db.models import F from django.http import HttpResponse, HttpResponseBadRequest, JsonResponse from django.utils import timezone @@ -24,6 +23,7 @@ def ping(request, code): check.n_pings = F("n_pings") + 1 check.last_ping = timezone.now() + check.alert_after = check.get_alert_after() if check.status in ("new", "paused"): check.status = "up" diff --git a/hc/front/tests/test_update_timeout.py b/hc/front/tests/test_update_timeout.py index 06ccad38..96e02af7 100644 --- a/hc/front/tests/test_update_timeout.py +++ b/hc/front/tests/test_update_timeout.py @@ -1,3 +1,4 @@ +from django.utils import timezone from hc.api.models import Check from hc.test import BaseTestCase @@ -7,6 +8,7 @@ class UpdateTimeoutTestCase(BaseTestCase): def setUp(self): super(UpdateTimeoutTestCase, self).setUp() self.check = Check(user=self.alice) + self.check.last_ping = timezone.now() self.check.save() def test_it_works(self): @@ -17,9 +19,12 @@ class UpdateTimeoutTestCase(BaseTestCase): r = self.client.post(url, data=payload) self.assertRedirects(r, "/checks/") - check = Check.objects.get(code=self.check.code) - assert check.timeout.total_seconds() == 3600 - assert check.grace.total_seconds() == 60 + self.check.refresh_from_db() + self.assertEqual(self.check.timeout.total_seconds(), 3600) + self.assertEqual(self.check.grace.total_seconds(), 60) + + # alert_after should be updated too + self.assertEqual(self.check.alert_after, self.check.get_alert_after()) def test_team_access_works(self): url = "/checks/%s/timeout/" % self.check.code diff --git a/hc/front/views.py b/hc/front/views.py index e3e14ba8..3b4f6236 100644 --- a/hc/front/views.py +++ b/hc/front/views.py @@ -165,6 +165,9 @@ def update_timeout(request, code): if form.is_valid(): check.timeout = td(seconds=form.cleaned_data["timeout"]) check.grace = td(seconds=form.cleaned_data["grace"]) + if check.last_ping: + check.alert_after = check.get_alert_after() + check.save() return redirect("hc-checks")