Revert "feat(hybridcloud) Use control file for user/sentryapp avatars (…

…#54118)"

This reverts commit 25d04ff.

Co-authored-by: markstory <24086+markstory@users.noreply.github.com>

pyproject.toml

@@ -746,6 +746,7 @@ module = [
     "sentry.tasks.deliver_from_outbox",
     "sentry.tasks.derive_code_mappings",
     "sentry.tasks.digests",
+    "sentry.tasks.files",
     "sentry.tasks.groupowner",
     "sentry.tasks.integrations",
     "sentry.tasks.integrations.create_comment",

src/sentry/models/avatars/base.py

@@ -43,11 +43,10 @@ def save(self, *args, **kwargs):
         return super().save(*args, **kwargs)
 
     def get_file(self):
-        # If we're getting a file, and the preferred write file
-        # type isn't present, move data over to new storage async.
-        file_id = getattr(self, self.file_write_fk(), None)
+        # Favor control_file_id if it exists and is set.
+        # Otherwise fallback to file_id. If still None, return.
         file_class = self.file_class()
-
+        file_id = getattr(self, self.file_fk())
         if file_id is None:
             file_id = self.file_id
             file_class = File
@@ -71,6 +70,9 @@ def get_file(self):
             self.update(**update)
             return None
 
+    def get_file_id(self):
+        return self.file_id
+
     def delete(self, *args, **kwargs):
         file = self.get_file()
         if file:
@@ -101,32 +103,21 @@ def get_cached_photo(self, size):
                 cache.set(cache_key, photo)
         return photo
 
-    def file_class(self):
-        return File
+    @classmethod
+    def file_class(cls):
+        from sentry.models import File
 
-    def file_fk(self) -> str:
-        """
-        Get the foreign key currently used by this record for blob storage.
-        Varies in ControlAvatarBase
-        """
-        return "file_id"
+        return File
 
-    def file_write_fk(self) -> str:
-        """
-        Get the foreign key that should be used for writes.
-        Varies in ControlAvatarBase
-        """
+    @classmethod
+    def file_fk(cls) -> str:
         return "file_id"
 
     @classmethod
     def save_avatar(cls, relation, type, avatar=None, filename=None, color=None):
         if avatar:
-            # Create an instance of the current class so we can
-            # access where new files should be stored.
-            dummy = cls()
-            file_class = dummy.file_class()
-            with atomic_transaction(using=router.db_for_write(file_class)):
-                photo = file_class.objects.create(name=filename, type=cls.FILE_TYPE)
+            with atomic_transaction(using=router.db_for_write(cls.file_class())):
+                photo = cls.file_class().objects.create(name=filename, type=cls.FILE_TYPE)
                 # XXX: Avatar may come in as a string instance in python2
                 # if it's not wrapped in BytesIO.
                 if isinstance(avatar, str):
@@ -150,12 +141,11 @@ def save_avatar(cls, relation, type, avatar=None, filename=None, color=None):
                 file.delete()
 
             if photo:
-                if instance.file_fk() != instance.file_write_fk():
-                    setattr(instance, instance.file_fk(), None)
-                setattr(instance, instance.file_write_fk(), photo.id)
+                setattr(instance, cls.file_fk(), photo.id)
                 instance.ident = uuid4().hex
 
             instance.avatar_type = [i for i, n in cls.AVATAR_TYPES if n == type][0]
+
             instance.save()
 
         if photo and not created:

src/sentry/models/avatars/control_base.py

@@ -1,8 +1,6 @@
-from typing import Type, Union
-
 from sentry.db.models import BoundedBigIntegerField
 from sentry.models.avatars.base import AvatarBase
-from sentry.models.files import ControlFile, File
+from sentry.models.files import ControlFileBlob, File
 
 
 class ControlAvatarBase(AvatarBase):
@@ -11,29 +9,19 @@ class ControlAvatarBase(AvatarBase):
     class Meta:
         abstract = True
 
-    def file_class(self) -> Union[Type[ControlFile], Type[File]]:
-        """
-        Select the file class this avatar has used.
-        File classes can vary by the avatar as we have migrated
-        storage for saas, but self-hosted and single-tenant instances
-        did not have relations and storage migrated.
-        """
-        if self.control_file_id:
+    @classmethod
+    def file_class(cls):
+        from sentry.models import ControlFile
+
+        if ControlFileBlob._storage_config():
             return ControlFile
-        if self.file_id:
-            return File
-        return ControlFile
+        return File
 
-    def file_fk(self) -> str:
-        if self.control_file_id:
+    @classmethod
+    def file_fk(cls) -> str:
+        if ControlFileBlob._storage_config():
             return "control_file_id"
-        if self.file_id:
-            return "file_id"
-        return "control_file_id"
-
-    def file_write_fk(self) -> str:
-        """Prefer controlfile as user/sentryapp avatars are stored in control silo"""
-        return "control_file_id"
+        return "file_id"
 
-    def get_file_id(self) -> int:
+    def get_file_id(self):
         return self.control_file_id or self.file_id

src/sentry/models/avatars/user_avatar.py

@@ -1,44 +1,25 @@
 from __future__ import annotations
 
-from enum import IntEnum
-
 from django.db import models
 
 from sentry.db.models import BaseManager, FlexibleForeignKey, control_silo_only_model
 
 from . import ControlAvatarBase
 
 
-class UserAvatarType(IntEnum):
-    LETTER_AVATAR = 0
-    UPLOAD = 1
-    GRAVATAR = 2
-
-    def api_name(self):
-        return self.name.lower()
-
-    @classmethod
-    def as_choices(cls) -> tuple[tuple[int, str], ...]:
-        return (
-            (cls.LETTER_AVATAR, cls.LETTER_AVATAR.api_name()),
-            (cls.UPLOAD, cls.UPLOAD.api_name()),
-            (cls.GRAVATAR, cls.GRAVATAR.api_name()),
-        )
-
-
 @control_silo_only_model
 class UserAvatar(ControlAvatarBase):
     """
     A UserAvatar associates a User with their avatar photo File
     and contains their preferences for avatar type.
     """
 
-    AVATAR_TYPES = UserAvatarType.as_choices()
+    AVATAR_TYPES = ((0, "letter_avatar"), (1, "upload"), (2, "gravatar"))
 
     FILE_TYPE = "avatar.file"
 
     user = FlexibleForeignKey("sentry.User", unique=True, related_name="avatar")
-    avatar_type = models.PositiveSmallIntegerField(default=0, choices=UserAvatarType.as_choices())
+    avatar_type = models.PositiveSmallIntegerField(default=0, choices=AVATAR_TYPES)
 
     objects = BaseManager(cache_fields=["user"])
 

src/sentry/models/files/control_fileblob.py

@@ -1,41 +1,10 @@
-from __future__ import annotations
-
-from typing import Any, Dict
-
-from sentry import options
 from sentry.db.models import control_silo_only_model
 from sentry.models.files.abstractfileblob import AbstractFileBlob
 from sentry.models.files.control_fileblobowner import ControlFileBlobOwner
 from sentry.options.manager import UnknownOption
 from sentry.tasks.files import delete_file_control
 
 
-def control_file_storage_config() -> Dict[str, Any] | None:
-    """
-    When sentry is deployed in a siloed mode file relations
-    used by control silo models are stored separately from
-    region silo resources.
-
-    While we consistently write to the ControlFile django
-    model for control silo resources, we can't ensure
-    that each deployment has separate control + region storage
-    backends. We coalesce those options here. None means use the
-    global default storage options.
-    """
-    try:
-        # If these options exist, use them. Otherwise fallback to default behavior
-        storage_backend = options.get("filestore.control.backend")
-        storage_options = options.get("filestore.control.options")
-        if storage_backend:
-            return {
-                "backend": storage_backend,
-                "options": storage_options,
-            }
-    except UnknownOption:
-        pass
-    return None
-
-
 @control_silo_only_model
 class ControlFileBlob(AbstractFileBlob):
     class Meta:
@@ -45,6 +14,23 @@ class Meta:
     FILE_BLOB_OWNER_MODEL = ControlFileBlobOwner
     DELETE_FILE_TASK = delete_file_control
 
+    # TODO(hybrid-cloud): This is a temporary measure to allow concurrent production deployments of filestore
+    # We override the behavior of only the control silo file storage implementation to use
+    # the new control instance while production runs in monolith mode
     @classmethod
-    def _storage_config(cls) -> Dict[str, Any] | None:
-        return control_file_storage_config()
+    def _storage_config(cls):
+        from sentry import options as options_store
+
+        config = None
+        try:
+            # If these options exist, use them. Otherwise fallback to default behavior
+            backend = options_store.get("filestore.control.backend")
+            options = options_store.get("filestore.control.options")
+            if backend:
+                config = {
+                    "backend": backend,
+                    "options": options,
+                }
+        except UnknownOption:
+            pass
+        return config

src/sentry/tasks/files.py

@@ -113,15 +113,17 @@ def copy_file_to_control_and_update_model(
     file_id: int,
     **kwargs,
 ):
-    from sentry.models.files import ControlFile, File
+    from sentry.models.files import ControlFile, ControlFileBlob, File
+
+    if ControlFileBlob._storage_config() is None:
+        return
 
     lock = f"copy-file-lock-{model_name}:{model_id}"
 
     with locks.get(lock, duration=60, name="copy-file-lock").acquire():
         # Short circuit duplicate copy calls
-        model_class = apps.get_model(app_name, model_name)
-        instance = model_class.objects.get(id=model_id)
-        if instance.control_file_id:
+        model = apps.get_app_config(app_name).get_model(model_name).objects.get(id=model_id)
+        if model.control_file_id:
             return
 
         file_model = File.objects.get(id=file_id)
@@ -137,6 +139,5 @@ def copy_file_to_control_and_update_model(
         )
         control_file.putfile(file_handle)
 
-        instance.control_file_id = control_file.id
-        instance.file_id = None
-        instance.save()
+        model.control_file_id = control_file.id
+        model.save()

tests/sentry/api/endpoints/test_sentry_app_avatar.py

@@ -2,7 +2,6 @@
 
 from sentry import options as options_store
 from sentry.models import SentryAppAvatar
-from sentry.models.files.control_file import ControlFile
 from sentry.testutils.cases import APITestCase
 from sentry.testutils.silo import control_silo_test
 
@@ -38,7 +37,7 @@ def create_avatar(self, is_color):
 
 
 @control_silo_test(stable=True)
-class SentryAppAvatarGetTest(SentryAppAvatarTestBase):
+class SentryAppAvatarTest(SentryAppAvatarTestBase):
     def test_get(self):
         response = self.get_success_response(self.unpublished_app.slug)
 
@@ -54,30 +53,6 @@ def test_get(self):
         assert simple_avatar["color"] is False
         assert response.data["uuid"] == str(self.unpublished_app.uuid)
 
-    def test_get_upload_control_file(self):
-        self.method = "put"
-        self.create_avatar(is_color=True)
-
-        self.method = "get"
-        self.create_avatar(is_color=True)
-        response = self.get_success_response(self.unpublished_app.slug)
-
-        assert response.status_code == 200, response.content
-        assert response.data["uuid"] == str(self.unpublished_app.uuid)
-        uploads = [
-            avatar for avatar in response.data["avatars"] if avatar["avatarType"] == "upload"
-        ]
-        upload = uploads[0]
-        assert upload["avatarType"] == "upload"
-        assert upload["avatarUuid"]
-
-        avatar = SentryAppAvatar.objects.filter(sentry_app_id=self.unpublished_app.id).first()
-        assert avatar
-        assert avatar.get_file_id()
-        assert avatar.control_file_id
-        file = avatar.get_file()
-        assert isinstance(file, ControlFile)
-
 
 @control_silo_test(stable=True)
 class SentryAppAvatarPutTest(SentryAppAvatarTestBase):
@@ -90,25 +65,11 @@ def test_upload(self):
         assert avatar.get_file_id()
         assert avatar.get_avatar_type_display() == "upload"
         color_avatar = self.get_avatar(resp)
-        assert color_avatar
-        assert color_avatar["avatarType"] == "upload"
-        assert color_avatar["avatarUuid"] is not None
-        assert color_avatar["color"] is True
-
-    def test_upload_control_file(self):
-        resp = self.create_avatar(is_color=True)
-
-        avatar = SentryAppAvatar.objects.get(sentry_app=self.unpublished_app, color=True)
-        assert avatar.get_file_id()
-        assert avatar.control_file_id
-        assert avatar.get_avatar_type_display() == "upload"
-        color_avatar = self.get_avatar(resp)
-        assert color_avatar
         assert color_avatar["avatarType"] == "upload"
         assert color_avatar["avatarUuid"] is not None
         assert color_avatar["color"] is True
 
-    def test_upload_control_with_storage_options(self):
+    def test_upload_control(self):
         with self.options(
             {
                 "filestore.control.backend": options_store.get("filestore.backend"),
@@ -121,7 +82,6 @@ def test_upload_control_with_storage_options(self):
             assert avatar.get_file_id()
             assert avatar.get_avatar_type_display() == "upload"
             color_avatar = self.get_avatar(resp)
-            assert color_avatar
             assert color_avatar["avatarType"] == "upload"
             assert color_avatar["avatarUuid"] is not None
             assert color_avatar["color"] is True
@@ -136,15 +96,13 @@ def test_upload_both(self):
         assert avatars[0].get_file_id()
         assert avatars[0].get_avatar_type_display() == "upload"
         color_avatar = self.get_avatar(resp)
-        assert color_avatar
         assert color_avatar["color"] is True
         assert color_avatar["avatarType"] == "upload"
         assert color_avatar["avatarUuid"] is not None
 
         assert avatars[1].get_file_id()
         assert avatars[1].get_avatar_type_display() == "upload"
         simple_avatar = self.get_avatar(resp, False)
-        assert simple_avatar
         assert simple_avatar["color"] is False
         assert simple_avatar["avatarType"] == "upload"
         assert simple_avatar["avatarUuid"] is not None
@@ -171,12 +129,10 @@ def test_revert_to_default(self):
         color_avatar = self.get_avatar(response)
         simple_avatar = self.get_avatar(response, False)
 
-        assert color_avatar
         assert color_avatar["avatarType"] == "default"
         assert color_avatar["avatarUuid"] is not None
         assert color_avatar["color"] is True
 
-        assert simple_avatar
         assert simple_avatar["avatarType"] == "default"
         assert simple_avatar["avatarUuid"] is not None
         assert simple_avatar["color"] is False