Skip to content

fix(backup): Fix OrgAuthToken async race#58064

Merged
azaslavsky merged 3 commits into
masterfrom
azaslavsky/backup/fix_org_auth_token
Oct 13, 2023
Merged

fix(backup): Fix OrgAuthToken async race#58064
azaslavsky merged 3 commits into
masterfrom
azaslavsky/backup/fix_org_auth_token

Conversation

@azaslavsky

Copy link
Copy Markdown
Contributor

OrgAuthToken and OrganizationMapping are both control silo models. When importing the former, we want to make sure there are no token collisions, and mint new tokens for the importing model if there are. When a minting a new token, we need the org slug, which we previously pulled from the corresponding entry in the OrganizationMapping table. However, this assumes that prior Organization import, which triggers a post_save() generation of the corresponding OrganizationMapping in the control silo, has already successfully completed when we get around to importing our OrgAuthToken (much later in the import process, see fixtures/backup/model_dependencies/sorted.json for more). This is likely, but not guaranteed, to be true.

A naive solution to this would be to make an RPC call from inside of OrgAuthToken's write_relocation_import method, but this causes an RPC inside of a database transaction in the control silo, which is disallowed. Instead, we modify the pk_map which tracks JSON pks -> newly imported pks, and add an additional optional field onto it: slug, which (for fields that possess it) stores the unique slug associated with that model instance. When we pass this over to the control silo, we'll already have all of the information we need to mint the new OrgAuthToken, thereby eliminating the need to do RPC-based lookups.

Closes #205

@github-actions github-actions Bot added the Scope: Backend Automatically applied to PRs that change backend components label Oct 13, 2023
@azaslavsky azaslavsky force-pushed the azaslavsky/backup/fix_monolith_env_detect branch from deb1456 to 9af7800 Compare October 13, 2023 01:55
@azaslavsky azaslavsky force-pushed the azaslavsky/backup/fix_org_auth_token branch from e354943 to f27245c Compare October 13, 2023 01:55
@codecov

codecov Bot commented Oct 13, 2023

Copy link
Copy Markdown

Codecov Report

Merging #58064 (f27245c) into azaslavsky/backup/fix_monolith_env_detect (2b219aa) will increase coverage by 0.00%.
Report is 3 commits behind head on azaslavsky/backup/fix_monolith_env_detect.
The diff coverage is 78.57%.

❗ Current head f27245c differs from pull request most recent head e27514b. Consider uploading reports for the commit e27514b to get more accurate results

@@                            Coverage Diff                             @@
##           azaslavsky/backup/fix_monolith_env_detect   #58064   +/-   ##
==========================================================================
  Coverage                                      78.94%   78.95%           
==========================================================================
  Files                                           5130     5130           
  Lines                                         223030   223055   +25     
  Branches                                       37562    37565    +3     
==========================================================================
+ Hits                                          176078   176114   +36     
+ Misses                                         41304    41292   -12     
- Partials                                        5648     5649    +1     
Files Coverage Δ
src/sentry/backup/imports.py 89.04% <100.00%> (-1.31%) ⬇️
src/sentry/testutils/hybrid_cloud.py 95.42% <100.00%> (ø)
src/sentry/testutils/pytest/sentry.py 79.80% <100.00%> (ø)
src/sentry/models/orgauthtoken.py 90.47% <90.90%> (-2.12%) ⬇️
src/sentry/backup/dependencies.py 88.54% <61.53%> (-1.53%) ⬇️

... and 11 files with indirect coverage changes

@azaslavsky azaslavsky force-pushed the azaslavsky/backup/fix_monolith_env_detect branch from c7a3d06 to 3af49e1 Compare October 13, 2023 14:50
@azaslavsky azaslavsky force-pushed the azaslavsky/backup/fix_org_auth_token branch from f27245c to e27514b Compare October 13, 2023 14:50
A `SENTRY_USE_MONOLITH_DBS=0` is now equivalent to the environment
variable being entirely absent, which seemed to be responsible for
previously divergent behavior.
@azaslavsky azaslavsky force-pushed the azaslavsky/backup/fix_monolith_env_detect branch from 3af49e1 to 9809892 Compare October 13, 2023 16:03
@azaslavsky azaslavsky force-pushed the azaslavsky/backup/fix_org_auth_token branch from e27514b to 1fde149 Compare October 13, 2023 16:03
OrgAuthToken and OrganizationMapping are both control silo models. When
importing the former, we want to make sure there are no token
collisions, and mint new tokens for the importing model if there are.
When a minting a new token, we need the org slug, which we previously
pulled from the corresponding entry in the OrganizationMapping table.
However, this assumes that prior Organization import, which triggers a
`post_save()` generation of the corresponding OrganizationMapping in the
control silo, has already successfully completed when we get around to
importing our OrgAuthToken (much later in the import process, see
fixtures/backup/model_dependencies/sorted.json for more). This is
likely, but not guaranteed, to be true.

A naive solution to this would be to make an RPC call from inside of
OrgAuthToken's `write_relocation_import` method, but this causes an RPC
inside of a database transaction in the control silo, which is
disallowed. Instead, we modify the `pk_map` which tracks JSON pks ->
newly imported pks, and add an additional optional field onto it:
`slug`, which (for fields that possess it) stores the unique slug
associated with that model instance. When we pass this over to the
control silo, we'll already have all of the information we need to mint
the new OrgAuthToken, thereby eliminating the need to do RPC-based
lookups.

Closes #205
@azaslavsky azaslavsky force-pushed the azaslavsky/backup/fix_org_auth_token branch from 1fde149 to 027cecf Compare October 13, 2023 16:04
@azaslavsky azaslavsky marked this pull request as ready for review October 13, 2023 16:40
@azaslavsky azaslavsky requested a review from a team October 13, 2023 16:40
Base automatically changed from azaslavsky/backup/fix_monolith_env_detect to master October 13, 2023 18:00
@azaslavsky azaslavsky merged commit 31ff2d5 into master Oct 13, 2023
@azaslavsky azaslavsky deleted the azaslavsky/backup/fix_org_auth_token branch October 13, 2023 18:40
@github-actions github-actions Bot locked and limited conversation to collaborators Oct 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Logging functionality should not send emails

2 participants