Trigger alert group signal after transaction commit (#3001)

See https://docs.djangoproject.com/en/4.2/topics/db/transactions/#performing-actions-after-commit Related to https://github.com/grafana/oncall-private/issues/2015
2023-09-11 11:28:37 -03:00 · 2023-09-11 11:28:37 -03:00 · 14c32a74bf
commit 14c32a74bf
parent 9c3979a712
5 changed files with 72 additions and 56 deletions
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@ -13,6 +13,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

 ### Fixed

+- Avoid task retries because of missing AlertGroupLogRecord on send_alert_group_signal ([#3001](https://github.com/grafana/oncall/pull/3001))
 - Update escalation policies public API to handle new webhooks ([#2999](https://github.com/grafana/oncall/pull/2999))

 ## v1.3.36 (2023-09-07)
--- a/engine/apps/alerts/models/alert.py
+++ b/engine/apps/alerts/models/alert.py
@ -1,11 +1,12 @@
 import hashlib
 import logging
 import typing
+from functools import partial
 from uuid import uuid4

 from django.conf import settings
 from django.core.validators import MinLengthValidator
-from django.db import models
+from django.db import models, transaction
 from django.db.models import JSONField

 from apps.alerts import tasks
@ -163,7 +164,7 @@ class Alert(models.Model):
                        f"log record {log_record_for_root_incident.pk} with type "
                        f"'{log_record_for_root_incident.get_type_display()}'"
                    )
-                    tasks.send_alert_group_signal.apply_async((log_record_for_root_incident.pk,))
+                    transaction.on_commit(partial(tasks.send_alert_group_signal.delay, log_record_for_root_incident.pk))
                except AlertGroup.DoesNotExist:
                    pass

--- a/engine/apps/alerts/models/alert_group.py
+++ b/engine/apps/alerts/models/alert_group.py
@ -3,6 +3,7 @@ import logging
 import typing
 import urllib
 from collections import namedtuple
+from functools import partial
 from urllib.parse import urljoin

 from celery import uuid as celery_uuid
@ -1231,7 +1232,7 @@ class AlertGroup(AlertGroupSlackRenderingMixin, EscalationSnapshotMixin, models.
            alert_group.start_ack_reminder_if_needed()

            log_record = alert_group.log_records.create(type=AlertGroupLogRecord.TYPE_ACK, author=user)
-            send_alert_group_signal.apply_async((log_record.pk,))
+            transaction.on_commit(partial(send_alert_group_signal.delay, log_record.pk))

    @staticmethod
    def bulk_acknowledge(user: User, alert_groups: "QuerySet[AlertGroup]") -> None:
@ -1303,7 +1304,7 @@ class AlertGroup(AlertGroupSlackRenderingMixin, EscalationSnapshotMixin, models.
                state=AlertGroupState.RESOLVED,
            )
            log_record = alert_group.log_records.create(type=AlertGroupLogRecord.TYPE_RESOLVED, author=user)
-            send_alert_group_signal.apply_async((log_record.pk,))
+            transaction.on_commit(partial(send_alert_group_signal.delay, log_record.pk))

    @staticmethod
    def bulk_resolve(user: User, alert_groups: "QuerySet[AlertGroup]") -> None:
@ -1376,7 +1377,7 @@ class AlertGroup(AlertGroupSlackRenderingMixin, EscalationSnapshotMixin, models.
            if alert_group.is_root_alert_group:
                alert_group.start_escalation_if_needed()

-            send_alert_group_signal.apply_async((log_record.pk,))
+            transaction.on_commit(partial(send_alert_group_signal.delay, log_record.pk))

    @staticmethod
    def _bulk_restart_unresolve(user: User, alert_groups_to_restart_unresolve: "QuerySet[AlertGroup]") -> None:
@ -1419,7 +1420,7 @@ class AlertGroup(AlertGroupSlackRenderingMixin, EscalationSnapshotMixin, models.
            if alert_group.is_root_alert_group:
                alert_group.start_escalation_if_needed()

-            send_alert_group_signal.apply_async((log_record.pk,))
+            transaction.on_commit(partial(send_alert_group_signal.delay, log_record.pk))

    @staticmethod
    def _bulk_restart_unsilence(user: User, alert_groups_to_restart_unsilence: "QuerySet[AlertGroup]") -> None:
@ -1458,7 +1459,7 @@ class AlertGroup(AlertGroupSlackRenderingMixin, EscalationSnapshotMixin, models.
            )
            alert_group.start_escalation_if_needed()

-            send_alert_group_signal.apply_async((log_record.pk,))
+            transaction.on_commit(partial(send_alert_group_signal.delay, log_record.pk))

    @staticmethod
    def bulk_restart(user: User, alert_groups: "QuerySet[AlertGroup]") -> None:
@ -1594,7 +1595,7 @@ class AlertGroup(AlertGroupSlackRenderingMixin, EscalationSnapshotMixin, models.
                reason="Bulk action silence",
            )

-            send_alert_group_signal.apply_async((log_record.pk,))
+            transaction.on_commit(partial(send_alert_group_signal.delay, log_record.pk))
            if silence_for_period and alert_group.is_root_alert_group:
                alert_group.start_unsilence_task(countdown=silence_delay)

--- a/engine/apps/alerts/models/invitation.py
+++ b/engine/apps/alerts/models/invitation.py
@ -1,5 +1,6 @@
 import datetime
 import logging
+from functools import partial

 from django.db import models, transaction

@ -92,8 +93,8 @@ class Invitation(models.Model):
            f"log record {log_record.pk} with type '{log_record.get_type_display()}'"
        )

-        tasks.send_alert_group_signal.apply_async((log_record.pk,))
-        tasks.invite_user_to_join_incident.apply_async((invitation.pk,))
+        transaction.on_commit(partial(tasks.send_alert_group_signal.delay, log_record.pk))
+        transaction.on_commit(partial(tasks.invite_user_to_join_incident.delay, invitation.pk))

    @staticmethod
    def stop_invitation(invitation_pk, user):
@ -119,4 +120,4 @@ class Invitation(models.Model):
            f"call send_alert_group_signal for alert_group {invitation.alert_group.pk}, "
            f"log record {log_record.pk} with type '{log_record.get_type_display()}'"
        )
-        tasks.send_alert_group_signal.apply_async((log_record.pk,))
+        transaction.on_commit(partial(tasks.send_alert_group_signal.delay, log_record.pk))
--- a/engine/apps/api/tests/test_alert_group.py
+++ b/engine/apps/api/tests/test_alert_group.py
@ -1374,7 +1374,7 @@ def test_invalid_bulk_action(
    assert response.status_code == status.HTTP_400_BAD_REQUEST


-@patch("apps.alerts.tasks.send_alert_group_signal.send_alert_group_signal.apply_async", return_value=None)
+@patch("apps.alerts.tasks.send_alert_group_signal.send_alert_group_signal.delay", return_value=None)
@patch("apps.alerts.tasks.send_update_log_report_signal.send_update_log_report_signal.apply_async", return_value=None)
@patch("apps.alerts.models.AlertGroup.start_escalation_if_needed", return_value=None)
@pytest.mark.django_db
@ -1384,6 +1384,7 @@ def test_bulk_action_restart(
    mocked_start_escalate_alert,
    make_user_auth_headers,
    alert_group_internal_api_setup,
+    django_capture_on_commit_callbacks,
 ):
    client = APIClient()
    user, token, alert_groups = alert_group_internal_api_setup
@ -1406,18 +1407,20 @@ def test_bulk_action_restart(
        author=user,
    ).exists()

-    # restart alert groups
-    response = client.post(
-        url,
-        data={
-            "alert_group_pks": [alert_group.public_primary_key for alert_group in alert_groups],
-            "action": AlertGroup.RESTART,
-        },
-        format="json",
-        **make_user_auth_headers(user, token),
-    )
+    with django_capture_on_commit_callbacks(execute=True) as callbacks:
+        # restart alert groups
+        response = client.post(
+            url,
+            data={
+                "alert_group_pks": [alert_group.public_primary_key for alert_group in alert_groups],
+                "action": AlertGroup.RESTART,
+            },
+            format="json",
+            **make_user_auth_headers(user, token),
+        )

    assert response.status_code == status.HTTP_200_OK
+    assert len(callbacks) == 3

    assert resolved_alert_group.log_records.filter(
        type=AlertGroupLogRecord.TYPE_UN_RESOLVED,
@ -1439,7 +1442,7 @@ def test_bulk_action_restart(
    assert mocked_start_escalate_alert.called


-@patch("apps.alerts.tasks.send_alert_group_signal.send_alert_group_signal.apply_async", return_value=None)
+@patch("apps.alerts.tasks.send_alert_group_signal.send_alert_group_signal.delay", return_value=None)
@patch("apps.alerts.tasks.send_update_log_report_signal.send_update_log_report_signal.apply_async", return_value=None)
@pytest.mark.django_db
 def test_bulk_action_acknowledge(
@ -1447,6 +1450,7 @@ def test_bulk_action_acknowledge(
    mocked_log_report_signal_task,
    make_user_auth_headers,
    alert_group_internal_api_setup,
+    django_capture_on_commit_callbacks,
 ):
    client = APIClient()
    user, token, alert_groups = alert_group_internal_api_setup
@ -1459,18 +1463,20 @@ def test_bulk_action_acknowledge(
        author=user,
    ).exists()

-    # acknowledge alert groups
-    response = client.post(
-        url,
-        data={
-            "alert_group_pks": [alert_group.public_primary_key for alert_group in alert_groups],
-            "action": AlertGroup.ACKNOWLEDGE,
-        },
-        format="json",
-        **make_user_auth_headers(user, token),
-    )
+    with django_capture_on_commit_callbacks(execute=True) as callbacks:
+        # acknowledge alert groups
+        response = client.post(
+            url,
+            data={
+                "alert_group_pks": [alert_group.public_primary_key for alert_group in alert_groups],
+                "action": AlertGroup.ACKNOWLEDGE,
+            },
+            format="json",
+            **make_user_auth_headers(user, token),
+        )

    assert response.status_code == status.HTTP_200_OK
+    assert len(callbacks) == 3

    assert new_alert_group.log_records.filter(
        type=AlertGroupLogRecord.TYPE_ACK,
@ -1496,7 +1502,7 @@ def test_bulk_action_acknowledge(
    assert mocked_log_report_signal_task.called


-@patch("apps.alerts.tasks.send_alert_group_signal.send_alert_group_signal.apply_async", return_value=None)
+@patch("apps.alerts.tasks.send_alert_group_signal.send_alert_group_signal.delay", return_value=None)
@patch("apps.alerts.tasks.send_update_log_report_signal.send_update_log_report_signal.apply_async", return_value=None)
@pytest.mark.django_db
 def test_bulk_action_resolve(
@ -1504,6 +1510,7 @@ def test_bulk_action_resolve(
    mocked_log_report_signal_task,
    make_user_auth_headers,
    alert_group_internal_api_setup,
+    django_capture_on_commit_callbacks,
 ):
    client = APIClient()
    user, token, alert_groups = alert_group_internal_api_setup
@ -1516,18 +1523,20 @@ def test_bulk_action_resolve(
        author=user,
    ).exists()

-    # resolve alert groups
-    response = client.post(
-        url,
-        data={
-            "alert_group_pks": [alert_group.public_primary_key for alert_group in alert_groups],
-            "action": AlertGroup.RESOLVE,
-        },
-        format="json",
-        **make_user_auth_headers(user, token),
-    )
+    with django_capture_on_commit_callbacks(execute=True) as callbacks:
+        # resolve alert groups
+        response = client.post(
+            url,
+            data={
+                "alert_group_pks": [alert_group.public_primary_key for alert_group in alert_groups],
+                "action": AlertGroup.RESOLVE,
+            },
+            format="json",
+            **make_user_auth_headers(user, token),
+        )

    assert response.status_code == status.HTTP_200_OK
+    assert len(callbacks) == 3

    assert new_alert_group.log_records.filter(
        type=AlertGroupLogRecord.TYPE_RESOLVED,
@ -1548,7 +1557,7 @@ def test_bulk_action_resolve(
    assert mocked_log_report_signal_task.called


-@patch("apps.alerts.tasks.send_alert_group_signal.send_alert_group_signal.apply_async", return_value=None)
+@patch("apps.alerts.tasks.send_alert_group_signal.send_alert_group_signal.delay", return_value=None)
@patch("apps.alerts.tasks.send_update_log_report_signal.send_update_log_report_signal.apply_async", return_value=None)
@patch("apps.alerts.models.AlertGroup.start_unsilence_task", return_value=None)
@pytest.mark.django_db
@ -1558,6 +1567,7 @@ def test_bulk_action_silence(
    mocked_start_unsilence_task,
    make_user_auth_headers,
    alert_group_internal_api_setup,
+    django_capture_on_commit_callbacks,
 ):
    client = APIClient()
    user, token, alert_groups = alert_group_internal_api_setup
@ -1570,19 +1580,21 @@ def test_bulk_action_silence(
        author=user,
    ).exists()

-    # silence alert groups
-    response = client.post(
-        url,
-        data={
-            "alert_group_pks": [alert_group.public_primary_key for alert_group in alert_groups],
-            "action": AlertGroup.SILENCE,
-            "delay": 180,
-        },
-        format="json",
-        **make_user_auth_headers(user, token),
-    )
+    with django_capture_on_commit_callbacks(execute=True) as callbacks:
+        # silence alert groups
+        response = client.post(
+            url,
+            data={
+                "alert_group_pks": [alert_group.public_primary_key for alert_group in alert_groups],
+                "action": AlertGroup.SILENCE,
+                "delay": 180,
+            },
+            format="json",
+            **make_user_auth_headers(user, token),
+        )

    assert response.status_code == status.HTTP_200_OK
+    assert len(callbacks) == 4

    assert new_alert_group.log_records.filter(
        type=AlertGroupLogRecord.TYPE_SILENCE,