ref(hc): Create ControlOption to support cross silo options (#48833)

Converting the OptionStore to use different model backends for region or
control silos. SAAS continues using the existing table (which is
regional, the target of the SAAS transfer).

Goal here is such that Options are not in the hc service call path. This
folds nicely into other work on changing the way options are loaded; the
interface is retained, the only functional difference is which model /
table is used underneath to support HC abstractions.

migrations_lockfile.txt

@@ -6,5 +6,5 @@ To resolve this, rebase against latest master and regenerate your migration. Thi
 will then be regenerated, and you should be able to merge without conflicts.
 
 nodestore: 0002_nodestore_no_dictfield
-sentry: 0439_backfill_notificationsetting
+sentry: 0440_add_control_option
 social_auth: 0001_initial

src/sentry/admin.py

@@ -1,5 +1,4 @@
 from html import escape
-from pprint import saferepr
 
 from django import forms
 from django.conf import settings
@@ -21,7 +20,6 @@ from sentry.models import (
     AuditLogEntry,
     AuthIdentity,
     AuthProvider,
-    Option,
     Organization,
     OrganizationMember,
     Project,
@@ -33,24 +31,6 @@ csrf_protect_m = method_decorator(csrf_protect)
 sensitive_post_parameters_m = method_decorator(sensitive_post_parameters())
 
 
-class OptionAdmin(admin.ModelAdmin):
-    list_display = ("key", "last_updated")
-    fields = ("key", "value_repr", "last_updated")
-    readonly_fields = ("key", "value_repr", "last_updated")
-    search_fields = ("key",)
-
-    def value_repr(self, instance):
-        return '<pre style="display:inline-block;white-space:pre-wrap;">{}</pre>'.format(
-            escape(saferepr(instance.value))
-        )
-
-    value_repr.short_description = "Value"
-    value_repr.allow_tags = True
-
-
-admin.site.register(Option, OptionAdmin)
-
-
 class ProjectAdmin(admin.ModelAdmin):
     list_display = ("name", "slug", "organization", "status", "date_added")
     list_filter = ("status", "public")

src/sentry/db/models/fields/jsonfield.py

@@ -67,10 +67,15 @@ class JSONField(models.TextField):
     - always using a text field
     - being able to serialize dates/decimals
     - not emitting deprecation warnings
+
+    By default, this field will also invoke the Creator descriptor when setting the attribute.
+    This can make it difficult to use json fields that receive raw strings, so optionally setting no_creator_hook=True
+    surpresses this behavior.
     """
 
     default_error_messages = {"invalid": _("'%s' is not a valid JSON string.")}
     description = "JSON object"
+    no_creator_hook = False
 
     def __init__(self, *args, **kwargs):
         if not kwargs.get("null", False):
@@ -87,7 +92,8 @@ class JSONField(models.TextField):
         with previous Django behavior.
         """
         super().contribute_to_class(cls, name)
-        setattr(cls, name, Creator(self))
+        if not self.no_creator_hook:
+            setattr(cls, name, Creator(self))
 
     def validate(self, value, model_instance):
         if not self.null and value is None:
@@ -114,7 +120,7 @@ class JSONField(models.TextField):
         return "text"
 
     def to_python(self, value):
-        if isinstance(value, str):
+        if isinstance(value, str) or self.no_creator_hook:
             if value == "":
                 if self.null:
                     return None

src/sentry/migrations/0440_add_control_option.py

@@ -0,0 +1,46 @@
+# Generated by Django 2.2.28 on 2023-05-12 17:33
+
+import django.utils.timezone
+from django.db import migrations, models
+
+import sentry.db.models.fields.bounded
+import sentry.db.models.fields.picklefield
+from sentry.new_migrations.migrations import CheckedMigration
+
+
+class Migration(CheckedMigration):
+    # This flag is used to mark that a migration shouldn't be automatically run in production. For
+    # the most part, this should only be used for operations where it's safe to run the migration
+    # after your code has deployed. So this should not be used for most operations that alter the
+    # schema of a table.
+    # Here are some things that make sense to mark as dangerous:
+    # - Large data migrations. Typically we want these to be run manually by ops so that they can
+    #   be monitored and not block the deploy for a long period of time while they run.
+    # - Adding indexes to large tables. Since this can take a long time, we'd generally prefer to
+    #   have ops run this and not block the deploy. Note that while adding an index is a schema
+    #   change, it's completely safe to run the operation after the code has deployed.
+    is_dangerous = False
+
+    dependencies = [
+        ("sentry", "0439_backfill_notificationsetting"),
+    ]
+
+    operations = [
+        migrations.CreateModel(
+            name="ControlOption",
+            fields=[
+                (
+                    "id",
+                    sentry.db.models.fields.bounded.BoundedBigAutoField(
+                        primary_key=True, serialize=False
+                    ),
+                ),
+                ("key", models.CharField(max_length=128, unique=True)),
+                ("last_updated", models.DateTimeField(default=django.utils.timezone.now)),
+                ("value", sentry.db.models.fields.picklefield.PickledObjectField(editable=False)),
+            ],
+            options={
+                "db_table": "sentry_controloption",
+            },
+        ),
+    ]

src/sentry/models/options/__init__.py

@@ -1,10 +1,11 @@
-from .option import Option
+from .option import ControlOption, Option
 from .organization_option import OrganizationOption
 from .project_option import ProjectOption
 from .user_option import UserOption
 
 __all__ = (
     "Option",
+    "ControlOption",
     "OrganizationOption",
     "ProjectOption",
     "UserOption",

src/sentry/models/options/option.py

@@ -1,12 +1,11 @@
 from django.db import models
 from django.utils import timezone
 
-from sentry.db.models import Model, control_silo_only_model, sane_repr
+from sentry.db.models import Model, control_silo_only_model, region_silo_only_model, sane_repr
 from sentry.db.models.fields.picklefield import PickledObjectField
 
 
-@control_silo_only_model
-class Option(Model):  # type: ignore
+class BaseOption(Model):  # type: ignore
     """
     Global options which apply in most situations as defaults,
     and generally can be overwritten by per-project options.
@@ -18,11 +17,33 @@ class Option(Model):  # type: ignore
     __include_in_export__ = True
 
     key = models.CharField(max_length=128, unique=True)
-    value = PickledObjectField()
     last_updated = models.DateTimeField(default=timezone.now)
 
+    class Meta:
+        abstract = True
+
+    value = PickledObjectField()
+
+    __repr__ = sane_repr("key", "value")
+
+
+@region_silo_only_model
+class Option(BaseOption):
+    __include_in_export__ = True
+
     class Meta:
         app_label = "sentry"
         db_table = "sentry_option"
 
     __repr__ = sane_repr("key", "value")
+
+
+@control_silo_only_model
+class ControlOption(BaseOption):
+    __include_in_export__ = True
+
+    class Meta:
+        app_label = "sentry"
+        db_table = "sentry_controloption"
+
+    __repr__ = sane_repr("key", "value")

src/sentry/options/__init__.py

@@ -1,9 +1,6 @@
 from celery.signals import task_postrun
 from django.core.signals import request_finished
 
-from sentry.services.hybrid_cloud import silo_mode_delegation, stubbed
-from sentry.silo import SiloMode
-
 from .manager import (  # NOQA
     DEFAULT_FLAGS,
     FLAG_ADMIN_MODIFIABLE,
@@ -21,7 +18,7 @@ from .manager import (  # NOQA
     OptionsManager,
     UnknownOption,
 )
-from .store import AbstractOptionsStore, OptionsStore
+from .store import OptionsStore
 
 __all__ = (
     "get",
@@ -35,24 +32,7 @@ __all__ = (
 )
 
 # See notes in ``runner.initializer`` regarding lazy cache configuration.
-_local_store_impl = OptionsStore(cache=None)
-
-
-def impl_locally() -> AbstractOptionsStore:
-    return _local_store_impl
-
-
-# An abstraction for hybrid cloud.  Currently, under the hood, all silo modes still use the original options store.
-# However, to allow tests to validate abstraction for future silo separation, we need to use a delegator that can,
-# eventually, use a new implementation.
-default_store: AbstractOptionsStore = silo_mode_delegation(
-    {
-        SiloMode.MONOLITH: impl_locally,
-        SiloMode.REGION: stubbed(impl_locally, SiloMode.CONTROL),
-        SiloMode.CONTROL: impl_locally,
-    }
-)
-
+default_store = OptionsStore(cache=None)
 task_postrun.connect(default_store.maybe_clean_local_cache)
 request_finished.connect(default_store.maybe_clean_local_cache)
 

src/sentry/options/store.py

@@ -1,6 +1,5 @@
 from __future__ import annotations
 
-import abc
 import dataclasses
 import logging
 from random import random
@@ -9,9 +8,6 @@ from typing import Any, Optional
 
 from django.db.utils import OperationalError, ProgrammingError
 from django.utils import timezone
-from django.utils.functional import cached_property
-
-from sentry.services.hybrid_cloud import InterfaceWithLifecycle
 
 CACHE_FETCH_ERR = "Unable to fetch option cache for %s"
 CACHE_UPDATE_ERR = "Unable to update option cache for %s"
@@ -44,33 +40,7 @@ def _make_cache_value(key, value):
     return (value, now + key.ttl, now + key.ttl + key.grace)
 
 
-class AbstractOptionsStore(InterfaceWithLifecycle, abc.ABC):
-    @abc.abstractmethod
-    def get(self, key: Key, silent=False) -> Any | None:
-        pass
-
-    @abc.abstractmethod
-    def set(self, key: Key, value: Any) -> None:
-        pass
-
-    @abc.abstractmethod
-    def set_cache(self, key: Key, vlaue: Any) -> None:
-        pass
-
-    @abc.abstractmethod
-    def delete(self, key: Key) -> None:
-        pass
-
-    @abc.abstractmethod
-    def set_cache_impl(self, cache: Any) -> None:
-        pass
-
-    @abc.abstractmethod
-    def maybe_clean_local_cache(self):
-        pass
-
-
-class OptionsStore(AbstractOptionsStore):
+class OptionsStore:
     """
     Abstraction for the Option storage logic that should be driven
     by the OptionsManager.
@@ -87,10 +57,17 @@ class OptionsStore(AbstractOptionsStore):
         self.ttl = ttl
         self.flush_local_cache()
 
-    @cached_property
+    @property
     def model(self):
-        from sentry.models.options import Option
+        return self.model_cls()
+
+    @classmethod
+    def model_cls(cls):
+        from sentry.models.options import ControlOption, Option
+        from sentry.silo import SiloMode
 
+        if SiloMode.get_current_mode() == SiloMode.CONTROL:
+            return ControlOption
         return Option
 
     def get(self, key, silent=False):

src/sentry/tasks/options.py

@@ -3,8 +3,7 @@ from datetime import timedelta
 
 from django.utils import timezone
 
-from sentry.models import Option
-from sentry.options import default_manager
+from sentry.options import default_manager, default_store
 from sentry.options.manager import UnknownOption
 from sentry.tasks.base import instrumented_task
 
@@ -23,7 +22,7 @@ def sync_options(cutoff=ONE_HOUR):
     """
     cutoff_dt = timezone.now() - timedelta(seconds=cutoff)
     # TODO(dcramer): this doesnt handle deleted options (which shouldn't be allowed)
-    for option in Option.objects.filter(last_updated__gte=cutoff_dt).iterator():
+    for option in default_store.model.objects.filter(last_updated__gte=cutoff_dt).iterator():
         try:
             opt = default_manager.lookup_key(option.key)
             default_manager.store.set_cache(opt, option.value)

tests/sentry/options/test_manager.py

@@ -5,7 +5,6 @@ import pytest
 from django.conf import settings
 from django.core.cache.backends.locmem import LocMemCache
 
-from sentry.models import Option
 from sentry.options.manager import (
     DEFAULT_FLAGS,
     FLAG_IMMUTABLE,
@@ -18,9 +17,11 @@ from sentry.options.manager import (
 )
 from sentry.options.store import OptionsStore
 from sentry.testutils import TestCase
+from sentry.testutils.silo import all_silo_test
 from sentry.utils.types import Int, String
 
 
+@all_silo_test(stable=True)
 class OptionsManagerTest(TestCase):
     @cached_property
     def store(self):
@@ -149,7 +150,7 @@ class OptionsManagerTest(TestCase):
 
         # Make sure that we don't touch either of the stores
         with patch.object(self.store.cache, "get", side_effect=RuntimeError()):
-            with patch.object(Option.objects, "get_queryset", side_effect=RuntimeError()):
+            with patch.object(self.store.model.objects, "get_queryset", side_effect=RuntimeError()):
                 assert self.manager.get("nostore") == ""
                 self.store.flush_local_cache()
 
@@ -206,7 +207,7 @@ class OptionsManagerTest(TestCase):
             assert self.manager.get("prioritize_disk") == "foo"
 
     def test_db_unavailable(self):
-        with patch.object(Option.objects, "get_queryset", side_effect=RuntimeError()):
+        with patch.object(self.store.model.objects, "get_queryset", side_effect=RuntimeError()):
             # we can't update options if the db is unavailable
             with pytest.raises(RuntimeError):
                 self.manager.set("foo", "bar")
@@ -214,7 +215,7 @@ class OptionsManagerTest(TestCase):
         self.manager.set("foo", "bar")
         self.store.flush_local_cache()
 
-        with patch.object(Option.objects, "get_queryset", side_effect=RuntimeError()):
+        with patch.object(self.store.model.objects, "get_queryset", side_effect=RuntimeError()):
             assert self.manager.get("foo") == "bar"
             self.store.flush_local_cache()
 
@@ -227,11 +228,12 @@ class OptionsManagerTest(TestCase):
                     self.store.flush_local_cache()
 
     def test_db_and_cache_unavailable(self):
+        self.store.cache.clear()
         self.manager.set("foo", "bar")
         self.store.flush_local_cache()
 
         with self.settings(SENTRY_OPTIONS={"foo": "baz"}):
-            with patch.object(Option.objects, "get_queryset", side_effect=RuntimeError()):
+            with patch.object(self.store.model.objects, "get_queryset", side_effect=RuntimeError()):
                 with patch.object(self.store.cache, "get", side_effect=RuntimeError()):
                     assert self.manager.get("foo") == "baz"
                     self.store.flush_local_cache()