Fix duplicate orders for user notification policies #2278

vadimkerr · 2023-06-16T15:50:44Z

What this PR does

Fixes an issue when multiple user notification policies have duplicated order values, leading to the following unexpected behaviours:

Not possible to rearrange notification policies that have duplicated orders.
The notification system only executes the first policy from each order group. For example, if there are policies with orders [0, 0, 0, 0], only the first policy will be executed, and all others will be skipped. So the user will see four policies in the UI, while only one of them will be actually executed.

This PR fixes the issue by adding a unique index on (user_id, important, order) for UserNotificationPolicy model. However, it's not possible to add that unique index using the ordering library that we use due to it's implementation details.
I added a new abstract Django model OrderedModel that's able to work with such unique indices + under concurrent load.

Important info on this new OrderedModel abstract model:

Orders are unique on the DB level
Orders are allowed to be non-consecutive, for example order sequence [100, 150, 400] is valid
When deleting an instance, orders of other instances don't change. This is a notable difference from the library we use. I think it's better to only delete the instance without changing any other orders, because it reduces the number of dependencies between instances (e.g. Terraform drift will be much smaller this way if a policy is deleted via the web UI).

Which issue(s) this PR fixes

Related to https://github.com/grafana/oncall-private/issues/1680

Checklist

Unit, integration, and e2e (if applicable) tests updated
Documentation added (or pr:no public docs PR label added if not required)
CHANGELOG.md updated (or pr:no changelog PR label added if not required)

vadimkerr · 2023-06-19T15:17:03Z

engine/apps/base/models/ordered_model.py

+    return _retry_with_params
+
+
+class OrderedModel(models.Model):


Main focus of the PR

vadimkerr · 2023-06-19T15:17:56Z

engine/apps/api/serializers/user_notification_policy.py

    def create(self, validated_data):
-        prev_step = validated_data.pop("prev_step", None)


prev_step param seems to be deprecated, I couldn't find any usages

vadimkerr · 2023-06-19T15:18:59Z

docker-compose-developer.yml

@@ -208,7 +208,7 @@ services:
    container_name: mysql
    labels: *oncall-labels
    image: mysql:8.0.32
-    command: --default-authentication-plugin=mysql_native_password --character-set-server=utf8mb4 --collation-server=utf8mb4_unicode_ci
+    command: --default-authentication-plugin=mysql_native_password --character-set-server=utf8mb4 --collation-server=utf8mb4_unicode_ci --max_connections=1024


Increasing max_connections for local concurrency tests that open more connections than default number (151)

what do you think about adding a ./dev/mysql.ini file?

We have four containers (two in both this file + docker-compose-mysql-rabbitmq.yml) which run the mysql image. We could de-dupe this config and just volume mount this new file into all of these containers.

All 4 mysql containers share only these two settings: --character-set-server=utf8mb4 and --collation-server=utf8mb4_unicode_ci. Not sure if deduping those is worth of the extra complexity added by volume mounting the ini file. I'll take a closer look at this, I think it should be outside of the scope of this PR.

I agree that it's outside the scope of this PR 👍 just a thought that came to mind 😄

vadimkerr · 2023-06-20T13:49:33Z

engine/tox.ini

@@ -9,6 +9,6 @@ banned-modules =
 [pytest]
 # https://pytest-django.readthedocs.io/en/latest/configuring_django.html#order-of-choosing-settings
 # https://pytest-django.readthedocs.io/en/latest/database.html
-addopts = --color=yes --showlocals
+addopts = --no-migrations --color=yes --showlocals


This allows creating models on demand in tests, e.g. when testing abstract models such as TestOrderedModel.
Without --no-migrations it would be required to generate & store migrations for such test models.

…dimkerr/ordered-model

matiasb

LGTM

# What this PR does Fix duplicate `order` values for models `EscalationPolicy` and `ChannelFilter` using the same approach as #2278. - Make internal API action `move_to_position` a part of [OrderedModelViewSet](https://github.com/grafana/oncall/pull/2568/files#diff-eb62521ccbcb072d1bd2156adeadae3b5895bce6d0d54432a23db3948b0ada54R11-R34), so all ordered model views use the same logic. - Make public API serializers for ordered models inherit from [OrderedModelSerializer](https://github.com/grafana/oncall/pull/2568/files#diff-d749f94af5a49adaf5074325cdfad10ddd5a52dbfd78b49582867ebb9c92fae5R6-R38), so ordered model views are consistent with each other in public API. - Remove `order` from plugin fronted, since it's not being used anywhere. The frontend uses step indices & `move_to_position` action instead. - Make escalation snapshot use step indices instead of orders, since orders are not guaranteed to be sequential (+fix a minor off-by-one bug) ## Which issue(s) this PR fixes grafana/oncall-private#1680 ## Checklist - [x] Unit, integration, and e2e (if applicable) tests updated - [x] Documentation added (or `pr:no public docs` PR label added if not required) - [x] `CHANGELOG.md` updated (or `pr:no changelog` PR label added if not required)

vadimkerr added 10 commits June 16, 2023 16:04

Add OrderedModel

219d549

Use OrderedModel in UserNotificationPolicy

362bab5

Update internal API view and serializers

32dfeb9

Update public API

21601aa

delete excess if

08bc4a0

increase dev mysql max_connections

230fa50

Add comments

20ff1db

typing + pk name

af50a5e

comment

0e989a5

comment

1c7aab9

vadimkerr commented Jun 19, 2023

View reviewed changes

engine/apps/base/models/ordered_model.py

return _retry_with_params

class OrderedModel(models.Model):

Copy link

Member Author

vadimkerr Jun 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Main focus of the PR

vadimkerr commented Jun 19, 2023

View reviewed changes

vadimkerr added the pr:no public docs Added to a PR that does not require public documentation updates label Jun 19, 2023

vadimkerr added 4 commits June 19, 2023 16:45

Merge branch 'dev' into vadimkerr/ordered-model

e0c8dcd

--no-migrations

b93cf70

use connection.ops.quote_name

637b979

remove raw SQL

e8cf3d3

vadimkerr commented Jun 20, 2023

View reviewed changes

vadimkerr added 9 commits June 20, 2023 14:56

Merge branch 'dev' into vadimkerr/ordered-model

6bc753a

changelog

b73ff95

ignore migration

00fb5f2

create_default_policies_for_user

c790132

create_default_policies_for_user

5bca007

test

93c5588

lock queryset

cb4d5ba

less retries

53a61c1

Merge branch 'dev' into vadimkerr/ordered-model

45c6997

vadimkerr marked this pull request as ready for review June 20, 2023 15:35

vadimkerr requested a review from a team June 20, 2023 15:35

vadimkerr added 4 commits June 20, 2023 16:56

_adjust_order

004c095

more tests

294a8e6

more tests

e9a73bb

Merge remote-tracking branch 'origin/vadimkerr/ordered-model' into va…

1fb035d

…dimkerr/ordered-model

matiasb approved these changes Jun 20, 2023

View reviewed changes

mderynck approved these changes Jun 20, 2023

View reviewed changes

joeyorlando approved these changes Jun 21, 2023

View reviewed changes

vadimkerr added this pull request to the merge queue Jun 21, 2023

Merged via the queue into dev with commit eada4a4 Jun 21, 2023

vadimkerr deleted the vadimkerr/ordered-model branch June 21, 2023 11:17

matiasb mentioned this pull request Jun 22, 2023

v1.2.46 #2313

Merged

vadimkerr mentioned this pull request Jul 18, 2023

Fix duplicate orders on routes and escalation policies #2568

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix duplicate orders for user notification policies #2278

Fix duplicate orders for user notification policies #2278

vadimkerr commented Jun 16, 2023 •

edited

Loading

vadimkerr Jun 19, 2023

vadimkerr Jun 19, 2023

vadimkerr Jun 19, 2023

joeyorlando Jun 21, 2023

vadimkerr Jun 21, 2023

joeyorlando Jun 21, 2023

vadimkerr Jun 20, 2023 •

edited

Loading

matiasb left a comment

		def create(self, validated_data):
		prev_step = validated_data.pop("prev_step", None)

Fix duplicate orders for user notification policies #2278

Fix duplicate orders for user notification policies #2278

Conversation

vadimkerr commented Jun 16, 2023 • edited Loading

What this PR does

Which issue(s) this PR fixes

Checklist

vadimkerr Jun 19, 2023

Choose a reason for hiding this comment

vadimkerr Jun 19, 2023

Choose a reason for hiding this comment

vadimkerr Jun 19, 2023

Choose a reason for hiding this comment

joeyorlando Jun 21, 2023

Choose a reason for hiding this comment

vadimkerr Jun 21, 2023

Choose a reason for hiding this comment

joeyorlando Jun 21, 2023

Choose a reason for hiding this comment

vadimkerr Jun 20, 2023 • edited Loading

Choose a reason for hiding this comment

matiasb left a comment

Choose a reason for hiding this comment

vadimkerr commented Jun 16, 2023 •

edited

Loading

vadimkerr Jun 20, 2023 •

edited

Loading