From d27dad4a4bf03ffa467aef09ed29a508d51e1638 Mon Sep 17 00:00:00 2001 From: Steven Bal Date: Mon, 27 Jun 2022 10:37:05 +0200 Subject: [PATCH 1/6] :sparkles: [open-zaak/open-zaak#1203] Retry vars configurable via admin --- src/nrc/api/tasks.py | 77 ++++++++++++++++++++++++++++++++++++++------ 1 file changed, 67 insertions(+), 10 deletions(-) diff --git a/src/nrc/api/tasks.py b/src/nrc/api/tasks.py index b633a5ae..1189c9fb 100644 --- a/src/nrc/api/tasks.py +++ b/src/nrc/api/tasks.py @@ -1,12 +1,15 @@ import json import logging -from django.conf import settings from django.core.management import call_command from django.core.serializers.json import DjangoJSONEncoder from django.utils.translation import gettext_lazy as _ import requests +from celery.exceptions import Ignore, Retry +from celery.utils.time import get_exponential_backoff_interval +from notifications_api_common.models import NotificationsConfig +from vine.utils import wraps from nrc.celery import app from nrc.datamodel.models import Abonnement, NotificatieResponse @@ -14,19 +17,63 @@ logger = logging.getLogger(__name__) +def add_autoretry_behaviour(task, **options): + """ + Adapted from celery to use admin configurable autoretry settings + """ + autoretry_for = tuple( + options.get("autoretry_for", getattr(task, "autoretry_for", ())) + ) + retry_kwargs = options.get("retry_kwargs", getattr(task, "retry_kwargs", {})) + retry_jitter = options.get("retry_jitter", getattr(task, "retry_jitter", True)) + + if autoretry_for and not hasattr(task, "_orig_run"): + + @wraps(task.run) + def run(*args, **kwargs): + config = NotificationsConfig.get_solo() + max_retries = config.notification_delivery_max_retries + retry_backoff = config.notification_delivery_retry_backoff + retry_backoff_max = config.notification_delivery_retry_backoff_max + + task.max_retries = max_retries + + try: + return task._orig_run(*args, **kwargs) + except Ignore: + # If Ignore signal occurs task shouldn't be retried, + # even if it suits autoretry_for list + raise + except Retry: + raise + except autoretry_for as exc: + if retry_backoff: + retry_kwargs["countdown"] = get_exponential_backoff_interval( + factor=retry_backoff, + retries=task.request.retries, + maximum=retry_backoff_max, + full_jitter=retry_jitter, + ) + # Override max_retries + if hasattr(task, "override_max_retries"): + retry_kwargs["max_retries"] = getattr( + task, "override_max_retries", max_retries + ) + + ret = task.retry(exc=exc, **retry_kwargs) + # Stop propagation + if hasattr(task, "override_max_retries"): + delattr(task, "override_max_retries") + raise ret + + task._orig_run, task.run = task.run, run + + class NotificationException(Exception): pass -@app.task( - autoretry_for=( - NotificationException, - requests.RequestException, - ), - max_retries=settings.NOTIFICATION_DELIVERY_MAX_RETRIES, - retry_backoff=settings.NOTIFICATION_DELIVERY_RETRY_BACKOFF, - retry_backoff_max=settings.NOTIFICATION_DELIVERY_RETRY_BACKOFF_MAX, -) +@app.task def deliver_message(sub_id: int, msg: dict, **kwargs) -> None: """ send msg to subscriber @@ -81,3 +128,13 @@ def clean_old_notifications() -> None: cleans up old "Notificatie" and "NotificatieResponse" """ call_command("clean_old_notifications") + + +add_autoretry_behaviour( + deliver_message, + autoretry_for=( + NotificationException, + requests.RequestException, + ), + retry_jitter=False, +) From 6f0836994d87a63910d3507190d511d5406569b7 Mon Sep 17 00:00:00 2001 From: Steven Bal Date: Mon, 27 Jun 2022 11:32:13 +0200 Subject: [PATCH 2/6] :white_check_mark: [open-zaak/open-zaak#1203] Tests for configurable retry config --- src/nrc/api/tests/test_notificatie.py | 59 ++++++++++++++++++++++++++- 1 file changed, 58 insertions(+), 1 deletion(-) diff --git a/src/nrc/api/tests/test_notificatie.py b/src/nrc/api/tests/test_notificatie.py index 804122a7..b99473ff 100644 --- a/src/nrc/api/tests/test_notificatie.py +++ b/src/nrc/api/tests/test_notificatie.py @@ -1,14 +1,18 @@ from unittest.mock import patch -from django.test import override_settings +from django.test import TestCase, override_settings from django.utils.timezone import now +import requests_mock +from celery.exceptions import Retry +from notifications_api_common.models import NotificationsConfig from rest_framework import status from rest_framework.reverse import reverse from rest_framework.test import APITestCase from vng_api_common.conf.api import BASE_REST_FRAMEWORK from vng_api_common.tests import JWTAuthMixin +from nrc.api.tasks import deliver_message from nrc.datamodel.models import Notificatie from nrc.datamodel.tests.factories import ( AbonnementFactory, @@ -142,3 +146,56 @@ def test_notificatie_send_empty_kenmerk_value(self, mock_task): mock_task.assert_called_once_with( abon.id, msg, notificatie_id=Notificatie.objects.get().id, attempt=1 ) + + +@patch("nrc.api.tasks.get_exponential_backoff_interval") +@patch("nrc.api.tasks.NotificationsConfig.get_solo") +@patch("nrc.api.serializers.deliver_message.retry") +class NotificatieRetryTests(TestCase): + def test_notificatie_retry_use_global_config( + self, mock_retry, mock_config, mock_get_exponential_backoff + ): + """ + Verify that retry variables configured on `NotificationsConfig` override the + variables from the settings + """ + mock_config.return_value = NotificationsConfig( + notification_delivery_max_retries=4, + notification_delivery_retry_backoff=4, + notification_delivery_retry_backoff_max=28, + ) + kanaal = KanaalFactory.create( + naam="zaken", filters=["bron", "zaaktype", "vertrouwelijkheidaanduiding"] + ) + abon = AbonnementFactory.create(callback_url="https://example.com/callback") + filter_group = FilterGroupFactory.create(kanaal=kanaal, abonnement=abon) + FilterFactory.create( + filter_group=filter_group, key="bron", value="082096752011" + ) + msg = { + "kanaal": "zaken", + "hoofdObject": "https://ref.tst.vng.cloud/zrc/api/v1/zaken/d7a22", + "resource": "status", + "resourceUrl": "https://ref.tst.vng.cloud/zrc/api/v1/statussen/d7a22/721c9", + "actie": "create", + "aanmaakdatum": now(), + "kenmerken": { + "bron": "082096752011", + "zaaktype": "example.com/api/v1/zaaktypen/5aa5c", + "vertrouwelijkheidaanduiding": "openbaar", + }, + } + + mock_retry.side_effect = Retry() + with requests_mock.Mocker() as m: + m.post(abon.callback_url, status_code=404) + with self.assertRaises(Retry): + deliver_message(abon.id, msg) + + mock_get_exponential_backoff.assert_called_once_with( + factor=4, + retries=0, + maximum=28, + full_jitter=False, + ) + self.assertEqual(deliver_message.max_retries, 4) From c66ad80c0c1ff83d964fbed6cb26ff29ccd5b2ac Mon Sep 17 00:00:00 2001 From: Steven Bal Date: Tue, 16 Apr 2024 12:26:35 +0200 Subject: [PATCH 3/6] :sparkles: [open-zaak/open-zaak#1203] Setup config for notifs retry --- src/nrc/conf/includes/base.py | 21 ++++++++++------ src/nrc/config/notification_retry.py | 37 ++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 7 deletions(-) create mode 100644 src/nrc/config/notification_retry.py diff --git a/src/nrc/conf/includes/base.py b/src/nrc/conf/includes/base.py index 745788ef..1ced75cc 100644 --- a/src/nrc/conf/includes/base.py +++ b/src/nrc/conf/includes/base.py @@ -496,13 +496,6 @@ }, } -# Retry settings for delivering notifications to subscriptions -NOTIFICATION_DELIVERY_MAX_RETRIES = config("NOTIFICATION_DELIVERY_MAX_RETRIES", 5) -NOTIFICATION_DELIVERY_RETRY_BACKOFF = config("NOTIFICATION_DELIVERY_RETRY_BACKOFF", 3) -NOTIFICATION_DELIVERY_RETRY_BACKOFF_MAX = config( - "NOTIFICATION_DELIVERY_RETRY_BACKOFF_MAX", 48 -) - # # DJANGO-ADMIN-INDEX # @@ -565,6 +558,7 @@ "nrc.config.site.SiteConfigurationStep", "nrc.config.authorization.AuthorizationStep", "nrc.config.authorization.OpenZaakAuthStep", + "nrc.config.notification_retry.NotificationRetryConfigurationStep", ] # @@ -585,3 +579,16 @@ OPENZAAK_NOTIF_CONFIG_ENABLE = config("OPENZAAK_NOTIF_CONFIG_ENABLE", default=True) OPENZAAK_NOTIF_CLIENT_ID = config("OPENZAAK_NOTIF_CLIENT_ID", "") OPENZAAK_NOTIF_SECRET = config("OPENZAAK_NOTIF_SECRET", "") + +# setup configuration for Notification retry +# Retry settings for delivering notifications to subscriptions +NOTIFICATION_RETRY_CONFIG_ENABLE = config( + "NOTIFICATION_RETRY_CONFIG_ENABLE", default=True +) +NOTIFICATION_DELIVERY_MAX_RETRIES = config("NOTIFICATION_DELIVERY_MAX_RETRIES", None) +NOTIFICATION_DELIVERY_RETRY_BACKOFF = config( + "NOTIFICATION_DELIVERY_RETRY_BACKOFF", None +) +NOTIFICATION_DELIVERY_RETRY_BACKOFF_MAX = config( + "NOTIFICATION_DELIVERY_RETRY_BACKOFF_MAX", None +) diff --git a/src/nrc/config/notification_retry.py b/src/nrc/config/notification_retry.py new file mode 100644 index 00000000..15b3e3de --- /dev/null +++ b/src/nrc/config/notification_retry.py @@ -0,0 +1,37 @@ +from django.conf import settings + +from django_setup_configuration.configuration import BaseConfigurationStep +from notifications_api_common.models import NotificationsConfig + + +class NotificationRetryConfigurationStep(BaseConfigurationStep): + """ + Configure the notifications retry behaviour. + """ + + verbose_name = "Notification retry configuration" + required_settings = [] + optional_settings = [ + "NOTIFICATION_DELIVERY_MAX_RETRIES", + "NOTIFICATION_DELIVERY_RETRY_BACKOFF", + "NOTIFICATION_DELIVERY_RETRY_BACKOFF_MAX", + ] + enable_setting = "NOTIFICATION_RETRY_CONFIG_ENABLE" + + def is_configured(self) -> bool: + config = NotificationsConfig.get_solo() + for setting_name in self.optional_settings: + # It is considered configured if one or more fields have non-default values + model_field = getattr(NotificationsConfig, setting_name.lower()).field + if getattr(config, setting_name.lower()) != model_field.default: + return True + return False + + def configure(self): + config = NotificationsConfig.get_solo() + for setting_name in self.optional_settings: + if (setting_value := getattr(settings, setting_name)) is not None: + setattr(config, setting_name.lower(), setting_value) + config.save() + + def test_configuration(self): ... From 55e637001bacfeed821425687b72bbb9e932224b Mon Sep 17 00:00:00 2001 From: Steven Bal Date: Tue, 16 Apr 2024 12:35:25 +0200 Subject: [PATCH 4/6] :white_check_mark: [open-zaak/open-zaak#1203] Tests for notif retry setup config --- .../commands/test_setup_configuration.py | 7 ++++- .../test_notification_retry_configuration.py | 31 +++++++++++++++++++ 2 files changed, 37 insertions(+), 1 deletion(-) create mode 100644 src/nrc/tests/config/test_notification_retry_configuration.py diff --git a/src/nrc/tests/commands/test_setup_configuration.py b/src/nrc/tests/commands/test_setup_configuration.py index 14489f9a..31fbceac 100644 --- a/src/nrc/tests/commands/test_setup_configuration.py +++ b/src/nrc/tests/commands/test_setup_configuration.py @@ -16,6 +16,7 @@ from zgw_consumers.test import mock_service_oas_get from nrc.config.authorization import AuthorizationStep, OpenZaakAuthStep +from nrc.config.notification_retry import NotificationRetryConfigurationStep from nrc.config.site import SiteConfigurationStep @@ -29,6 +30,8 @@ OPENZAAK_NOTIF_SECRET="oz-secret", ) class SetupConfigurationTests(TestCase): + maxDiff = None + def setUp(self): super().setUp() @@ -66,13 +69,15 @@ def test_setup_configuration(self, m): command_output = stdout.getvalue().splitlines() expected_output = [ f"Configuration will be set up with following steps: [{SiteConfigurationStep()}, " - f"{AuthorizationStep()}, {OpenZaakAuthStep()}]", + f"{AuthorizationStep()}, {OpenZaakAuthStep()}, {NotificationRetryConfigurationStep()}]", f"Configuring {SiteConfigurationStep()}...", f"{SiteConfigurationStep()} is successfully configured", f"Configuring {AuthorizationStep()}...", f"{AuthorizationStep()} is successfully configured", f"Configuring {OpenZaakAuthStep()}...", f"{OpenZaakAuthStep()} is successfully configured", + f"Configuring {NotificationRetryConfigurationStep()}...", + f"{NotificationRetryConfigurationStep()} is successfully configured", "Instance configuration completed.", ] diff --git a/src/nrc/tests/config/test_notification_retry_configuration.py b/src/nrc/tests/config/test_notification_retry_configuration.py new file mode 100644 index 00000000..6185bc11 --- /dev/null +++ b/src/nrc/tests/config/test_notification_retry_configuration.py @@ -0,0 +1,31 @@ +from django.test import TestCase, override_settings + +from notifications_api_common.models import NotificationsConfig + +from nrc.config.notification_retry import NotificationRetryConfigurationStep + + +@override_settings( + NOTIFICATION_DELIVERY_MAX_RETRIES=4, + NOTIFICATION_DELIVERY_RETRY_BACKOFF=5, + NOTIFICATION_DELIVERY_RETRY_BACKOFF_MAX=6, +) +class NotificationRetryConfigurationTests(TestCase): + def test_configure(self): + configuration = NotificationRetryConfigurationStep() + configuration.configure() + + config = NotificationsConfig.get_solo() + + self.assertEqual(config.notification_delivery_max_retries, 4) + self.assertEqual(config.notification_delivery_retry_backoff, 5) + self.assertEqual(config.notification_delivery_retry_backoff_max, 6) + + def test_is_configured(self): + configuration = NotificationRetryConfigurationStep() + + self.assertFalse(configuration.is_configured()) + + configuration.configure() + + self.assertTrue(configuration.is_configured()) From b959509a64cee6abf3fb01dff5d2bed6628d93a0 Mon Sep 17 00:00:00 2001 From: Steven Bal Date: Tue, 16 Apr 2024 12:57:35 +0200 Subject: [PATCH 5/6] :memo: [open-zaak/open-zaak#1203] Docs for setup config for retry notifs --- docs/installation/configuration/env_config.md | 12 +---------- .../configuration/opennotifs_config_cli.rst | 21 +++++++++++++++++-- 2 files changed, 20 insertions(+), 13 deletions(-) diff --git a/docs/installation/configuration/env_config.md b/docs/installation/configuration/env_config.md index 4f08563f..db0dfa91 100644 --- a/docs/installation/configuration/env_config.md +++ b/docs/installation/configuration/env_config.md @@ -100,16 +100,6 @@ on Docker, since `localhost` is contained within the container: * `CELERY_RESULT_BACKEND`: the backend where the results of tasks will be stored (default: `redis://localhost:6379/1`) -* `NOTIFICATION_DELIVERY_MAX_RETRIES`: the maximum number of retries Celery will do if sending a notification failed. - -* `NOTIFICATION_DELIVERY_RETRY_BACKOFF`: a boolean or a number. If this option is set to - `True`, autoretries will be delayed following the rules of exponential backoff. If - this option is set to a number, it is used as a delay factor. - -* `NOTIFICATION_DELIVERY_RETRY_BACKOFF_MAX`: an integer, specifying number of seconds. - If ``retry_backoff`` is enabled, this option will set a maximum delay in seconds - between task autoretries. By default, this option is set to 48 seconds. - ### Cross-Origin-Resource-Sharing The following parameters control the CORS policy. @@ -130,7 +120,7 @@ The following parameters control the CORS policy. ### Initial configuration Open Notificaties supports `setup_configuration` management command, which allows configuration via -environment variables. +environment variables. All these environment variables are described at CLI configuration. diff --git a/docs/installation/configuration/opennotifs_config_cli.rst b/docs/installation/configuration/opennotifs_config_cli.rst index f05059d9..d5224915 100644 --- a/docs/installation/configuration/opennotifs_config_cli.rst +++ b/docs/installation/configuration/opennotifs_config_cli.rst @@ -27,8 +27,8 @@ Preparation The command executes the list of pluggable configuration steps, and each step requires specific environment variables, that should be prepared. -Here is the description of all available configuration steps and the environment variables, -use by each step. +Here is the description of all available configuration steps and the environment variables, +use by each step. Sites configuration ------------------- @@ -65,6 +65,23 @@ Make sure that the correct permissions are configured in Open Zaak Autorisaties for example, ``open-zaak``. Required. * ``OPENZAAK_NOTIF_SECRET``: some random string. Required. +Notification retry configuration +-------------------------------- + +Open Notifications has a retry mechanism to guarantee notification delivery, this mechanism +is described in :ref:`delivery_guarantees`. The parameters for this behavior can be configured via the +following environment variables. + +* ``NOTIFICATION_DELIVERY_MAX_RETRIES``: the maximum number of retries Celery will do if sending a notification failed. +* ``NOTIFICATION_DELIVERY_RETRY_BACKOFF``: a boolean or a number. If this option is set to + ``True``, autoretries will be delayed following the rules of exponential backoff. If + this option is set to a number, it is used as a delay factor. +* ``NOTIFICATION_DELIVERY_RETRY_BACKOFF_MAX``: an integer, specifying number of seconds. + If ``retry_backoff`` is enabled, this option will set a maximum delay in seconds + between task autoretries. By default, this option is set to 48 seconds. + +These settings can also later be changed via the admin interface, under ``Configuration > Notification component configuration``. + Execution ========= From 43d42f141371dd7529e05322dff7e909887827c0 Mon Sep 17 00:00:00 2001 From: Steven Bal Date: Tue, 23 Apr 2024 12:32:27 +0200 Subject: [PATCH 6/6] :ok_hand: [open-zaak/open-zaak#1203] PR feedback --- src/nrc/api/tasks.py | 57 +------------------ src/nrc/api/tests/test_notificatie.py | 4 +- .../commands/test_setup_configuration.py | 2 +- 3 files changed, 4 insertions(+), 59 deletions(-) diff --git a/src/nrc/api/tasks.py b/src/nrc/api/tasks.py index 1189c9fb..681190dc 100644 --- a/src/nrc/api/tasks.py +++ b/src/nrc/api/tasks.py @@ -6,10 +6,7 @@ from django.utils.translation import gettext_lazy as _ import requests -from celery.exceptions import Ignore, Retry -from celery.utils.time import get_exponential_backoff_interval -from notifications_api_common.models import NotificationsConfig -from vine.utils import wraps +from notifications_api_common.autoretry import add_autoretry_behaviour from nrc.celery import app from nrc.datamodel.models import Abonnement, NotificatieResponse @@ -17,58 +14,6 @@ logger = logging.getLogger(__name__) -def add_autoretry_behaviour(task, **options): - """ - Adapted from celery to use admin configurable autoretry settings - """ - autoretry_for = tuple( - options.get("autoretry_for", getattr(task, "autoretry_for", ())) - ) - retry_kwargs = options.get("retry_kwargs", getattr(task, "retry_kwargs", {})) - retry_jitter = options.get("retry_jitter", getattr(task, "retry_jitter", True)) - - if autoretry_for and not hasattr(task, "_orig_run"): - - @wraps(task.run) - def run(*args, **kwargs): - config = NotificationsConfig.get_solo() - max_retries = config.notification_delivery_max_retries - retry_backoff = config.notification_delivery_retry_backoff - retry_backoff_max = config.notification_delivery_retry_backoff_max - - task.max_retries = max_retries - - try: - return task._orig_run(*args, **kwargs) - except Ignore: - # If Ignore signal occurs task shouldn't be retried, - # even if it suits autoretry_for list - raise - except Retry: - raise - except autoretry_for as exc: - if retry_backoff: - retry_kwargs["countdown"] = get_exponential_backoff_interval( - factor=retry_backoff, - retries=task.request.retries, - maximum=retry_backoff_max, - full_jitter=retry_jitter, - ) - # Override max_retries - if hasattr(task, "override_max_retries"): - retry_kwargs["max_retries"] = getattr( - task, "override_max_retries", max_retries - ) - - ret = task.retry(exc=exc, **retry_kwargs) - # Stop propagation - if hasattr(task, "override_max_retries"): - delattr(task, "override_max_retries") - raise ret - - task._orig_run, task.run = task.run, run - - class NotificationException(Exception): pass diff --git a/src/nrc/api/tests/test_notificatie.py b/src/nrc/api/tests/test_notificatie.py index b99473ff..d6a66e38 100644 --- a/src/nrc/api/tests/test_notificatie.py +++ b/src/nrc/api/tests/test_notificatie.py @@ -148,8 +148,8 @@ def test_notificatie_send_empty_kenmerk_value(self, mock_task): ) -@patch("nrc.api.tasks.get_exponential_backoff_interval") -@patch("nrc.api.tasks.NotificationsConfig.get_solo") +@patch("notifications_api_common.autoretry.get_exponential_backoff_interval") +@patch("notifications_api_common.autoretry.NotificationsConfig.get_solo") @patch("nrc.api.serializers.deliver_message.retry") class NotificatieRetryTests(TestCase): def test_notificatie_retry_use_global_config( diff --git a/src/nrc/tests/commands/test_setup_configuration.py b/src/nrc/tests/commands/test_setup_configuration.py index 31fbceac..23421145 100644 --- a/src/nrc/tests/commands/test_setup_configuration.py +++ b/src/nrc/tests/commands/test_setup_configuration.py @@ -63,7 +63,7 @@ def test_setup_configuration(self, m): }, ) - call_command("setup_configuration", stdout=stdout) + call_command("setup_configuration", stdout=stdout, no_color=True) with self.subTest("Command output"): command_output = stdout.getvalue().splitlines()