From c54abd873c086f5c0f45daab5b8858f99c04b787 Mon Sep 17 00:00:00 2001 From: Alex Zaslavsky Date: Thu, 5 Dec 2024 17:26:53 -0800 Subject: [PATCH] feat(relocation): Split autopause (#81762) After this rolls out, I'll update the options-automator to match. --- .github/CODEOWNERS | 2 +- src/sentry/api/endpoints/organization_fork.py | 5 ++- src/sentry/api/endpoints/relocations/index.py | 17 ++++++-- src/sentry/api/endpoints/relocations/retry.py | 5 ++- src/sentry/models/relocation.py | 8 ++++ src/sentry/options/defaults.py | 17 ++++++++ .../api/endpoints/relocations/test_index.py | 41 ++++++++++++++++++- .../api/endpoints/test_organization_fork.py | 40 +++++++++++++++++- 8 files changed, 124 insertions(+), 11 deletions(-) diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 933b585b428c33..9695c88749836f 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -97,7 +97,7 @@ Makefile @getsentry/owners-sentr /src/sentry/analytics/events/relocation_*.py @getsentry/open-source /src/sentry/api/endpoints/organization_fork.py @getsentry/open-source /src/sentry/api/endpoints/relocation/ @getsentry/open-source -/src/sentry/api/serialiers/models/relocation/ @getsentry/open-source +/src/sentry/api/serializers/models/relocation/ @getsentry/open-source /src/sentry/models/relocation/ @getsentry/open-source /src/sentry/relocation/ @getsentry/open-source /src/sentry/tasks/relocation.py @getsentry/open-source diff --git a/src/sentry/api/endpoints/organization_fork.py b/src/sentry/api/endpoints/organization_fork.py index 52ed47960682c5..5a1aff9d7e50a7 100644 --- a/src/sentry/api/endpoints/organization_fork.py +++ b/src/sentry/api/endpoints/organization_fork.py @@ -145,13 +145,14 @@ def post(self, request: Request, organization_id_or_slug) -> Response: # We do not create a `RelocationFile` yet. Instead, we trigger a series of RPC calls (via # `uploading_start`, scheduled below) to create an export of the organization we are seeking # duplicate from the foreign region. + provenance = Relocation.Provenance.SAAS_TO_SAAS with atomic_transaction(using=(router.db_for_write(Relocation))): new_relocation: Relocation = Relocation.objects.create( creator_id=request.user.id, owner_id=owner.id, step=Relocation.Step.UPLOADING.value, - scheduled_pause_at_step=get_autopause_value(), - provenance=Relocation.Provenance.SAAS_TO_SAAS, + scheduled_pause_at_step=get_autopause_value(provenance), + provenance=provenance, want_org_slugs=[org_mapping.slug], ) diff --git a/src/sentry/api/endpoints/relocations/index.py b/src/sentry/api/endpoints/relocations/index.py index b493fde70bc72c..c6cacef17f791d 100644 --- a/src/sentry/api/endpoints/relocations/index.py +++ b/src/sentry/api/endpoints/relocations/index.py @@ -140,11 +140,18 @@ def validate_relocation_uniqueness(owner: RpcUser | AnonymousUser | User) -> Res return None -def get_autopause_value() -> int | None: +def get_autopause_value(provenance: Relocation.Provenance) -> int | None: try: - return Relocation.Step[options.get("relocation.autopause")].value + return Relocation.Step[options.get(f"relocation.autopause.{str(provenance)}")].value except KeyError: - return None + # DEPRECATED: for now, we fall through to the old `relocation.autopause` if the more + # specific `relocation.autopause.*` does not exist OR is set to the default, an empty + # string. Once we remove the old setting, this block can go away, and we can use the mre + # specific autopause only. + try: + return Relocation.Step[options.get("relocation.autopause")].value + except KeyError: + return None @region_silo_endpoint @@ -263,12 +270,14 @@ def post(self, request: Request) -> Response: with atomic_transaction( using=(router.db_for_write(Relocation), router.db_for_write(RelocationFile)) ): + provenance = Relocation.Provenance.SELF_HOSTED relocation: Relocation = Relocation.objects.create( creator_id=request.user.id, owner_id=owner.id, want_org_slugs=org_slugs, step=Relocation.Step.UPLOADING.value, - scheduled_pause_at_step=get_autopause_value(), + scheduled_pause_at_step=get_autopause_value(provenance), + provenance=provenance, ) RelocationFile.objects.create( relocation=relocation, diff --git a/src/sentry/api/endpoints/relocations/retry.py b/src/sentry/api/endpoints/relocations/retry.py index 4f80e14b5d5505..1986bc3c015c38 100644 --- a/src/sentry/api/endpoints/relocations/retry.py +++ b/src/sentry/api/endpoints/relocations/retry.py @@ -111,7 +111,10 @@ def post(self, request: Request, relocation_uuid: str) -> Response: owner_id=relocation.owner_id, want_org_slugs=relocation.want_org_slugs, step=Relocation.Step.UPLOADING.value, - scheduled_pause_at_step=get_autopause_value(), + scheduled_pause_at_step=get_autopause_value( + Relocation.Provenance(relocation.provenance) + ), + provenance=relocation.provenance, ) relocation_retry_link_promo_code.send_robust( diff --git a/src/sentry/models/relocation.py b/src/sentry/models/relocation.py index 897e3530cce3d1..e563ae7c03d58b 100644 --- a/src/sentry/models/relocation.py +++ b/src/sentry/models/relocation.py @@ -90,6 +90,14 @@ class Provenance(IntEnum): def get_choices(cls) -> list[tuple[int, str]]: return [(key.value, key.name) for key in cls] + def __str__(self): + if self.name == "SELF_HOSTED": + return "self-hosted" + elif self.name == "SAAS_TO_SAAS": + return "saas-to-saas" + else: + raise ValueError("Cannot extract a filename from `RelocationFile.Kind.UNKNOWN`.") + # The user that requested this relocation - if the request was made by an admin on behalf of a # user, this will be different from `owner`. Otherwise, they are identical. creator_id = BoundedBigIntegerField() diff --git a/src/sentry/options/defaults.py b/src/sentry/options/defaults.py index 1626cc75f6bbd6..56868202949446 100644 --- a/src/sentry/options/defaults.py +++ b/src/sentry/options/defaults.py @@ -2366,12 +2366,29 @@ # Relocation: the step at which new relocations should be autopaused, requiring admin approval # before continuing. +# DEPRECATED: will be removed after the new `relocation.autopause.*` options are fully rolled out. register( "relocation.autopause", default="", flags=FLAG_AUTOMATOR_MODIFIABLE, ) +# Relocation: the step at which new `SELF_HOSTED` relocations should be autopaused, requiring an +# admin to unpause before continuing. +register( + "relocation.autopause.self-hosted", + default="", + flags=FLAG_AUTOMATOR_MODIFIABLE, +) + +# Relocation: the step at which new `SELF_HOSTED` relocations should be autopaused, requiring an +# admin to unpause before continuing. +register( + "relocation.autopause.saas-to-saas", + default="", + flags=FLAG_AUTOMATOR_MODIFIABLE, +) + # Relocation: globally limits the number of small (<=10MB) relocations allowed per silo per day. register( "relocation.daily-limit.small", diff --git a/tests/sentry/api/endpoints/relocations/test_index.py b/tests/sentry/api/endpoints/relocations/test_index.py index a6f99b282ff53f..b06592eedae8dd 100644 --- a/tests/sentry/api/endpoints/relocations/test_index.py +++ b/tests/sentry/api/endpoints/relocations/test_index.py @@ -437,7 +437,7 @@ def test_good_promo_code( { "relocation.enabled": True, "relocation.daily-limit.small": 1, - "relocation.autopause": "IMPORTING", + "relocation.autopause.self-hosted": "IMPORTING", } ) @patch("sentry.tasks.relocation.uploading_start.apply_async") @@ -492,7 +492,44 @@ def test_good_with_valid_autopause_option( { "relocation.enabled": True, "relocation.daily-limit.small": 1, - "relocation.autopause": "DOESNOTEXIST", + "relocation.autopause.saas-to-saas": "IMPORTING", + } + ) + @patch("sentry.tasks.relocation.uploading_start.apply_async") + def test_good_with_untriggered_autopause_option( + self, + uploading_start_mock: Mock, + relocation_link_promo_code_signal_mock: Mock, + analytics_record_mock: Mock, + ): + self.login_as(user=self.owner, superuser=False) + + with tempfile.TemporaryDirectory() as tmp_dir: + (_, tmp_pub_key_path) = self.tmp_keys(tmp_dir) + with open(FRESH_INSTALL_PATH, "rb") as f: + data = orjson.loads(f.read()) + with open(tmp_pub_key_path, "rb") as p: + response = self.get_success_response( + owner=self.owner.username, + file=SimpleUploadedFile( + "export.tar", + create_encrypted_export_tarball(data, LocalFileEncryptor(p)).getvalue(), + content_type="application/tar", + ), + orgs="testing", + format="multipart", + status_code=201, + ) + + assert response.data["status"] == Relocation.Status.IN_PROGRESS.name + assert response.data["step"] == Relocation.Step.UPLOADING.name + assert response.data["scheduledPauseAtStep"] is None + + @override_options( + { + "relocation.enabled": True, + "relocation.daily-limit.small": 1, + "relocation.autopause.self-hosted": "DOESNOTEXIST", } ) @patch("sentry.tasks.relocation.uploading_start.apply_async") diff --git a/tests/sentry/api/endpoints/test_organization_fork.py b/tests/sentry/api/endpoints/test_organization_fork.py index 2fe2a2b75eb9cf..88354f209cf35f 100644 --- a/tests/sentry/api/endpoints/test_organization_fork.py +++ b/tests/sentry/api/endpoints/test_organization_fork.py @@ -135,7 +135,7 @@ def test_good_simple_using_organization_id( { "relocation.enabled": True, "relocation.daily-limit.small": 1, - "relocation.autopause": "IMPORTING", + "relocation.autopause.saas-to-saas": "IMPORTING", } ) @assume_test_silo_mode(SiloMode.REGION, region_name=REQUESTING_TEST_REGION) @@ -169,6 +169,44 @@ def test_good_with_valid_autopause_option( replying_region_name=EXPORTING_TEST_REGION, ) + @override_options( + { + "relocation.enabled": True, + "relocation.daily-limit.small": 1, + "relocation.autopause.self-hosted": "IMPORTING", + } + ) + @assume_test_silo_mode(SiloMode.REGION, region_name=REQUESTING_TEST_REGION) + def test_good_with_untriggered_autopause_option( + self, + uploading_start_mock: Mock, + analytics_record_mock: Mock, + ): + self.login_as(user=self.superuser, superuser=True) + + response = self.get_success_response(self.existing_org.slug) + + assert response.data["status"] == Relocation.Status.IN_PROGRESS.name + assert response.data["step"] == Relocation.Step.UPLOADING.name + assert response.data["provenance"] == Relocation.Provenance.SAAS_TO_SAAS.name + assert response.data["scheduledPauseAtStep"] is None + + assert uploading_start_mock.call_count == 1 + uploading_start_mock.assert_called_with( + args=[UUID(response.data["uuid"]), EXPORTING_TEST_REGION, self.requested_org_slug] + ) + + assert analytics_record_mock.call_count == 1 + analytics_record_mock.assert_called_with( + "relocation.forked", + creator_id=int(response.data["creator"]["id"]), + owner_id=int(response.data["owner"]["id"]), + uuid=response.data["uuid"], + from_org_slug=self.requested_org_slug, + requesting_region_name=REQUESTING_TEST_REGION, + replying_region_name=EXPORTING_TEST_REGION, + ) + @override_options( {"relocation.enabled": False, "relocation.daily-limit.small": 1, "staff.ga-rollout": True} )