From c99788e9d2a3d97e69f59c991a06a63c741f43c2 Mon Sep 17 00:00:00 2001 From: Matias Bordese Date: Wed, 17 Jan 2024 13:30:11 -0300 Subject: [PATCH] Update schedule on-call cache on scheduled refresh tasks (#3699) Related to https://github.com/grafana/oncall/issues/3673 Keep cache up to date on every schedule refresh task run (which should keep cache populated every time), helping on any call using cached information (particularly the direct paging slack dialog building). --- CHANGELOG.md | 4 ++ engine/apps/schedules/constants.py | 3 ++ engine/apps/schedules/ical_utils.py | 31 +++++++----- .../schedules/tasks/refresh_ical_files.py | 5 +- .../tests/tasks/test_refresh_ical_files.py | 49 ++++++++++++++++++- 5 files changed, 79 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4025b42e..6f175681 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Fixed Webhooks UI not allowing simple webhooks to be created ([#3691](https://github.com/grafana/oncall/pull/3691)) - Fix posting Slack message when route is deleted by @vadimkerr ([#3702](https://github.com/grafana/oncall/pull/3702)) +### Changed + +- Update schedules on-call users cache on every scheduled schedule refresh task ([#3699](https://github.com/grafana/oncall/pull/3699)). + ## v1.3.88 (2024-01-16) ### Fixed diff --git a/engine/apps/schedules/constants.py b/engine/apps/schedules/constants.py index 818ddeb1..880cf50d 100644 --- a/engine/apps/schedules/constants.py +++ b/engine/apps/schedules/constants.py @@ -26,3 +26,6 @@ CALENDAR_TYPE_FINAL = "final" EXPORT_WINDOW_DAYS_AFTER = 180 EXPORT_WINDOW_DAYS_BEFORE = 15 + +SCHEDULE_ONCALL_CACHE_KEY_PREFIX = "schedule_oncall_users_" +SCHEDULE_ONCALL_CACHE_TTL = 15 * 60 # 15 minutes in seconds diff --git a/engine/apps/schedules/ical_utils.py b/engine/apps/schedules/ical_utils.py index 9dede8fc..eb9840d5 100644 --- a/engine/apps/schedules/ical_utils.py +++ b/engine/apps/schedules/ical_utils.py @@ -33,6 +33,8 @@ from apps.schedules.constants import ( RE_EVENT_UID_V1, RE_EVENT_UID_V2, RE_PRIORITY, + SCHEDULE_ONCALL_CACHE_KEY_PREFIX, + SCHEDULE_ONCALL_CACHE_TTL, ) from apps.schedules.ical_events import ical_events from common.cache import ensure_cache_key_allocates_to_the_same_hash_slot @@ -387,6 +389,22 @@ def get_oncall_users_for_multiple_schedules( return oncall_users +def _generate_cache_key_for_schedule_oncall_users(schedule: "OnCallSchedule") -> str: + return ensure_cache_key_allocates_to_the_same_hash_slot( + f"{SCHEDULE_ONCALL_CACHE_KEY_PREFIX}{schedule.public_primary_key}", SCHEDULE_ONCALL_CACHE_KEY_PREFIX + ) + + +def update_cached_oncall_users_for_schedule(schedule: "OnCallSchedule"): + oncall_users = get_oncall_users_for_multiple_schedules([schedule]) + users = oncall_users.get(schedule, []) + cache.set( + _generate_cache_key_for_schedule_oncall_users(schedule), + [user.public_primary_key for user in users], + timeout=SCHEDULE_ONCALL_CACHE_TTL, + ) + + def get_cached_oncall_users_for_multiple_schedules(schedules: typing.List["OnCallSchedule"]) -> SchedulesOnCallUsers: """ More "performant" version of `apps.schedules.ical_utils.get_oncall_users_for_multiple_schedules` @@ -404,22 +422,13 @@ def get_cached_oncall_users_for_multiple_schedules(schedules: typing.List["OnCal from apps.schedules.models import OnCallSchedule from apps.user_management.models import User - CACHE_KEY_PREFIX = "schedule_oncall_users_" - - def _generate_cache_key_for_schedule_oncall_users(schedule: "OnCallSchedule") -> str: - return ensure_cache_key_allocates_to_the_same_hash_slot( - f"{CACHE_KEY_PREFIX}{schedule.public_primary_key}", CACHE_KEY_PREFIX - ) - def _get_schedule_public_primary_key_from_schedule_oncall_users_cache_key(cache_key: str) -> str: """ remove any brackets that might be included in the cache key (when redis cluster is active). See `_generate_cache_key_for_schedule_oncall_users` just above """ cache_key = cache_key.replace("{", "").replace("}", "") - return cache_key.replace(CACHE_KEY_PREFIX, "") - - CACHE_TTL = 15 * 60 # 15 minutes in seconds + return cache_key.replace(SCHEDULE_ONCALL_CACHE_KEY_PREFIX, "") cache_keys = [_generate_cache_key_for_schedule_oncall_users(schedule) for schedule in schedules] @@ -464,7 +473,7 @@ def get_cached_oncall_users_for_multiple_schedules(schedules: typing.List["OnCal _generate_cache_key_for_schedule_oncall_users(schedule) ] = oncall_user_public_primary_keys - cache.set_many(new_results_to_update_in_cache, timeout=CACHE_TTL) + cache.set_many(new_results_to_update_in_cache, timeout=SCHEDULE_ONCALL_CACHE_TTL) # make two queries to the database, one to fetch the schedule objects we need and the other to fetch # the user objects we need diff --git a/engine/apps/schedules/tasks/refresh_ical_files.py b/engine/apps/schedules/tasks/refresh_ical_files.py index d0ae3a4a..a022d464 100644 --- a/engine/apps/schedules/tasks/refresh_ical_files.py +++ b/engine/apps/schedules/tasks/refresh_ical_files.py @@ -1,7 +1,7 @@ from celery.utils.log import get_task_logger from apps.alerts.tasks import notify_ical_schedule_shift # type: ignore[no-redef] -from apps.schedules.ical_utils import is_icals_equal +from apps.schedules.ical_utils import is_icals_equal, update_cached_oncall_users_for_schedule from apps.schedules.tasks import notify_about_empty_shifts_in_schedule_task, notify_about_gaps_in_schedule_task from apps.slack.tasks import start_update_slack_user_group_for_schedules from common.custom_celery_tasks import shared_dedicated_queue_retry_task @@ -81,6 +81,9 @@ def refresh_ical_file(schedule_pk): task_logger.info(f"run_task_overrides {schedule_pk} {run_task_primary} icals not equal") run_task = run_task_primary or run_task_overrides + # update cached schedule on-call users + update_cached_oncall_users_for_schedule(schedule) + if run_task: notify_about_empty_shifts_in_schedule_task.apply_async((schedule_pk,)) notify_about_gaps_in_schedule_task.apply_async((schedule_pk,)) diff --git a/engine/apps/schedules/tests/tasks/test_refresh_ical_files.py b/engine/apps/schedules/tests/tasks/test_refresh_ical_files.py index c6c8ced7..fdbbec36 100644 --- a/engine/apps/schedules/tests/tasks/test_refresh_ical_files.py +++ b/engine/apps/schedules/tests/tasks/test_refresh_ical_files.py @@ -2,8 +2,10 @@ import datetime from unittest.mock import patch import pytest +from django.core.cache import cache +from django.utils import timezone -from apps.schedules.models import OnCallScheduleICal, OnCallScheduleWeb +from apps.schedules.models import CustomOnCallShift, OnCallScheduleICal, OnCallScheduleWeb from apps.schedules.tasks.refresh_ical_files import refresh_ical_file, start_refresh_ical_files @@ -79,3 +81,48 @@ def test_refresh_ical_files_filter_orgs( assert len(called_args) == 1 assert schedule.id in called_args[0].args[0] assert schedule_from_deleted_org.id not in called_args[0].args[0] + + +@pytest.mark.django_db +def test_refresh_ical_updates_oncall_cache( + make_organization, + make_user_for_organization, + make_schedule, + make_on_call_shift, +): + organization = make_organization() + users = [make_user_for_organization(organization) for _ in range(2)] + + today = timezone.now().replace(hour=0, minute=0, second=0, microsecond=0) + schedule = make_schedule(organization, schedule_class=OnCallScheduleWeb) + shift_start_time = today - timezone.timedelta(hours=1) + on_call_shift = make_on_call_shift( + organization=organization, + shift_type=CustomOnCallShift.TYPE_ROLLING_USERS_EVENT, + start=shift_start_time, + rotation_start=shift_start_time, + duration=timezone.timedelta(seconds=(24 * 60 * 60)), + priority_level=1, + frequency=CustomOnCallShift.FREQUENCY_DAILY, + schedule=schedule, + ) + on_call_shift.add_rolling_users([users]) + + def _generate_cache_key(schedule): + return f"schedule_oncall_users_{schedule.public_primary_key}" + + # start with empty cache + cache.clear() + + # patch ical comparison to string compare + with patch("apps.schedules.tasks.refresh_ical_files.is_icals_equal", side_effect=lambda a, b: a == b): + # patch schedule refresh to avoid changing schedule status (keep as defined above) + with patch("apps.schedules.models.OnCallSchedule.refresh_ical_file", return_value=None): + # do not trigger tasks for real + with patch("apps.schedules.tasks.refresh_ical_files.notify_ical_schedule_shift"): + with patch("apps.schedules.tasks.refresh_ical_files.notify_about_empty_shifts_in_schedule_task"): + with patch("apps.schedules.tasks.refresh_ical_files.notify_about_gaps_in_schedule_task"): + refresh_ical_file(schedule.pk) + + cached_data = cache.get(_generate_cache_key(schedule)) + assert cached_data == [u.public_primary_key for u in users]