From 4861fec36a1c89d26e93bbb85405687a512b55e6 Mon Sep 17 00:00:00 2001 From: Julia Date: Tue, 14 Nov 2023 12:26:29 +0100 Subject: [PATCH 1/3] Add checking in acknowledge reminder if organization was deleted, add logging --- .../apps/alerts/tasks/acknowledge_reminder.py | 48 ++++++++++++------- 1 file changed, 32 insertions(+), 16 deletions(-) diff --git a/engine/apps/alerts/tasks/acknowledge_reminder.py b/engine/apps/alerts/tasks/acknowledge_reminder.py index 1f8b6f1da6..dd9848acf1 100644 --- a/engine/apps/alerts/tasks/acknowledge_reminder.py +++ b/engine/apps/alerts/tasks/acknowledge_reminder.py @@ -26,13 +26,11 @@ def acknowledge_reminder_task(alert_group_pk: int, unacknowledge_process_id: str if unacknowledge_process_id != alert_group.last_unique_unacknowledge_process_id: return + organization = alert_group.channel.organization + # Get timeout values - acknowledge_reminder_timeout = Organization.ACKNOWLEDGE_REMIND_DELAY[ - alert_group.channel.organization.acknowledge_remind_timeout - ] - unacknowledge_timeout = Organization.UNACKNOWLEDGE_TIMEOUT_DELAY[ - alert_group.channel.organization.unacknowledge_timeout - ] + acknowledge_reminder_timeout = Organization.ACKNOWLEDGE_REMIND_DELAY[organization.acknowledge_remind_timeout] + unacknowledge_timeout = Organization.UNACKNOWLEDGE_TIMEOUT_DELAY[organization.unacknowledge_timeout] # Don't proceed if the alert group is not in a state for acknowledgement reminder acknowledge_reminder_required = ( @@ -41,10 +39,18 @@ def acknowledge_reminder_task(alert_group_pk: int, unacknowledge_process_id: str and alert_group.acknowledged_by == AlertGroup.USER and acknowledge_reminder_timeout ) - if not acknowledge_reminder_required: - task_logger.info("AlertGroup is not in a state for acknowledgement reminder") + is_organization_deleted = organization.deleted_at is not None + log_info = ( + f"acknowledge_reminder_timeout option: {acknowledge_reminder_timeout}," + f"organization ppk: {organization.public_primary_key}," + f"organization is deleted: {is_organization_deleted}" + ) + if not acknowledge_reminder_required or is_organization_deleted: + task_logger.info(f"alert group {alert_group_pk} is not in a state for acknowledgement reminder. {log_info}") return + task_logger.info(f"alert group {alert_group_pk} is in a state for acknowledgement reminder. {log_info}") + # unacknowledge_timeout_task uses acknowledged_by_confirmed to check if acknowledgement reminder has been confirmed # by the user. Setting to None here to indicate that the user has not confirmed the acknowledgement reminder alert_group.acknowledged_by_confirmed = None @@ -80,13 +86,11 @@ def unacknowledge_timeout_task(alert_group_pk: int, unacknowledge_process_id: st if unacknowledge_process_id != alert_group.last_unique_unacknowledge_process_id: return + organization = alert_group.channel.organization + # Get timeout values - acknowledge_reminder_timeout = Organization.ACKNOWLEDGE_REMIND_DELAY[ - alert_group.channel.organization.acknowledge_remind_timeout - ] - unacknowledge_timeout = Organization.UNACKNOWLEDGE_TIMEOUT_DELAY[ - alert_group.channel.organization.unacknowledge_timeout - ] + acknowledge_reminder_timeout = Organization.ACKNOWLEDGE_REMIND_DELAY[organization.acknowledge_remind_timeout] + unacknowledge_timeout = Organization.UNACKNOWLEDGE_TIMEOUT_DELAY[organization.unacknowledge_timeout] # Don't proceed if the alert group is not in a state for auto-unacknowledge unacknowledge_required = ( @@ -96,16 +100,28 @@ def unacknowledge_timeout_task(alert_group_pk: int, unacknowledge_process_id: st and acknowledge_reminder_timeout and unacknowledge_timeout ) - if not unacknowledge_required: - task_logger.info("AlertGroup is not in a state for unacknowledge") + is_organization_deleted = organization.deleted_at is not None + log_info = ( + f"acknowledge_reminder_timeout option: {acknowledge_reminder_timeout}," + f"unacknowledge_timeout option: {unacknowledge_timeout}," + f"organization ppk: {organization.public_primary_key}," + f"organization is deleted: {is_organization_deleted}" + ) + if not unacknowledge_required or is_organization_deleted: + task_logger.info(f"alert group {alert_group_pk} is not in a state for unacknowledge by timeout. {log_info}") return if alert_group.acknowledged_by_confirmed: # acknowledgement reminder was confirmed by the user acknowledge_reminder_task.apply_async( (alert_group_pk, unacknowledge_process_id), countdown=acknowledge_reminder_timeout - unacknowledge_timeout ) + task_logger.info( + f"Acknowledgement reminder was confirmed by user. Rescheduling acknowledge_reminder_task..." + f"alert group: {alert_group_pk}, {log_info}" + ) return + task_logger.info(f"alert group {alert_group_pk} is in a state for unacknowledge by timeout. {log_info}") # If acknowledgement reminder wasn't confirmed by the user, unacknowledge the alert group and start escalation again log_record = alert_group.log_records.create( type=AlertGroupLogRecord.TYPE_AUTO_UN_ACK, author=alert_group.acknowledged_by_user From 629d7dca37b64f449c6e030b47ef7fcad2c115f4 Mon Sep 17 00:00:00 2001 From: Julia Date: Tue, 14 Nov 2023 12:55:35 +0100 Subject: [PATCH 2/3] Add tests --- .../alerts/tests/test_acknowledge_reminder.py | 40 +++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/engine/apps/alerts/tests/test_acknowledge_reminder.py b/engine/apps/alerts/tests/test_acknowledge_reminder.py index a47042fc59..f66fe9afd2 100644 --- a/engine/apps/alerts/tests/test_acknowledge_reminder.py +++ b/engine/apps/alerts/tests/test_acknowledge_reminder.py @@ -299,3 +299,43 @@ def test_unacknowledge_timeout_task_no_unacknowledge( ) assert not alert_group.log_records.exists() + + +@patch.object(acknowledge_reminder_task, "apply_async") +@patch.object(unacknowledge_timeout_task, "apply_async") +@pytest.mark.django_db +def test_ack_reminder_skip_deleted_org( + mock_acknowledge_reminder_task, + mock_unacknowledge_timeout_task, + ack_reminder_test_setup, +): + organization, alert_group, user = ack_reminder_test_setup() + organization.deleted_at = timezone.now() + organization.save() + + acknowledge_reminder_task(alert_group.pk, TASK_ID) + + mock_unacknowledge_timeout_task.assert_not_called() + mock_acknowledge_reminder_task.assert_not_called() + + assert not alert_group.log_records.exists() + + +@patch.object(acknowledge_reminder_task, "apply_async") +@patch.object(unacknowledge_timeout_task, "apply_async") +@pytest.mark.django_db +def test_unacknowledge_timeout_task_skip_deleted_org( + mock_acknowledge_reminder_task, + mock_unacknowledge_timeout_task, + ack_reminder_test_setup, +): + organization, alert_group, user = ack_reminder_test_setup() + organization.deleted_at = timezone.now() + organization.save() + + unacknowledge_timeout_task(alert_group.pk, TASK_ID) + + mock_unacknowledge_timeout_task.assert_not_called() + mock_acknowledge_reminder_task.assert_not_called() + + assert not alert_group.log_records.exists() From 862b7106462a72104d16bdf980e19ada3155129d Mon Sep 17 00:00:00 2001 From: Julia Date: Tue, 14 Nov 2023 12:58:13 +0100 Subject: [PATCH 3/3] Update CHANGELOG.md --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index a27a10028e..aea34e72fe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Forward headers for Amazon SNS when organizations are moved @mderynck ([#3326](https://github.com/grafana/oncall/pull/3326)) - Fix styling when light theme is turned on via system preferences by excluding dark theme css vars in this case ([#3336](https://github.com/grafana/oncall/pull/3336)) +- Fix issue when acknowledge reminder works for deleted organizations @Ferril ([#3345](https://github.com/grafana/oncall/pull/3345)) ## v1.3.57 (2023-11-10)