Skip to content

Conversation

@azaslavsky
Copy link
Contributor

Exports are already done on a sequential, per-model basis, going in reverse dependency order. This change modifies that scheme to work across an RPC boundary: code running on REGION instances (the only kind of instance that may perform import/export operations) requests the exports using either the local implementation (if the model is REGION as well), or by sending an RPC call to the same method on the remote CONTROL silo instance. Code executed in MONOLITH mode (aka, self-hosted and single tenant instances) does all exporting locally.

The new import_export_service is thus a bit different from some of the others in hybrid_cloud. Whereas existing services like Access provide a means to get the same information using different logic depending on which Silo they are located in (ex: using Organization or OrganizationMapping as the source of truth), this service executes identical logic in all silo kinds, and requires users to pick the implementation themselves. That is, users must examine the model they are about to request be exported, and make their own decision as to which implementation, remote or local, to use. This is a bit unergonomic compared to other hybrid_cloud services, but it is only used by one rather unique endpoint (the import/export functions) for a very specific purpose, making this API a bit easier to swallow.

Issue: getsentry/team-ospo#196

@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Oct 7, 2023
azaslavsky added a commit that referenced this pull request Oct 7, 2023
This finishes out the work started in #57740 and enables imports over
RPC as well. Like that PR, imports are already done on a sequential, per
model basis, so this change just consists of moving every such call
across an RPC boundary.

Closes getsentry/team-ospo#196
@azaslavsky azaslavsky force-pushed the azaslavsky/backup/refactor/instance_id branch from 30e058c to abf878c Compare October 7, 2023 00:12
@azaslavsky azaslavsky force-pushed the azaslavsky/backup/export_rpc branch from 0ef3704 to f87f84f Compare October 7, 2023 00:12
azaslavsky added a commit that referenced this pull request Oct 9, 2023
This finishes out the work started in #57740 and enables imports over
RPC as well. Like that PR, imports are already done on a sequential, per
model basis, so this change just consists of moving every such call
across an RPC boundary.

Closes getsentry/team-ospo#196
@azaslavsky azaslavsky force-pushed the azaslavsky/backup/refactor/instance_id branch from abf878c to e0c8734 Compare October 11, 2023 00:39
@azaslavsky azaslavsky force-pushed the azaslavsky/backup/export_rpc branch from f87f84f to c587ad3 Compare October 11, 2023 00:39
@azaslavsky azaslavsky force-pushed the azaslavsky/backup/refactor/instance_id branch 2 times, most recently from ce029d5 to 00aa0dc Compare October 11, 2023 20:50
@azaslavsky azaslavsky force-pushed the azaslavsky/backup/export_rpc branch from c587ad3 to e362bd8 Compare October 11, 2023 20:50
@azaslavsky azaslavsky requested a review from corps October 11, 2023 20:52
@azaslavsky azaslavsky force-pushed the azaslavsky/backup/refactor/instance_id branch from 5833527 to f91f271 Compare October 12, 2023 20:35
@azaslavsky azaslavsky force-pushed the azaslavsky/backup/export_rpc branch from e362bd8 to 2b66385 Compare October 12, 2023 20:35
@azaslavsky azaslavsky force-pushed the azaslavsky/backup/refactor/instance_id branch from f91f271 to 9e83388 Compare October 13, 2023 02:03
@azaslavsky azaslavsky force-pushed the azaslavsky/backup/export_rpc branch from 2b66385 to 5516fd8 Compare October 13, 2023 02:04
@codecov
Copy link

codecov bot commented Oct 13, 2023

Codecov Report

Merging #57740 (1bf2a99) into master (d4f5811) will increase coverage by 0.01%.
Report is 4 commits behind head on master.
The diff coverage is 85.71%.

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

@@            Coverage Diff             @@
##           master   #57740      +/-   ##
==========================================
+ Coverage   79.02%   79.03%   +0.01%     
==========================================
  Files        5130     5134       +4     
  Lines      223033   223269     +236     
  Branches    37566    37599      +33     
==========================================
+ Hits       176255   176466     +211     
- Misses      41140    41166      +26     
+ Partials     5638     5637       -1     
Files Coverage Δ
src/sentry/backup/dependencies.py 91.35% <100.00%> (+1.31%) ⬆️
...ry/services/hybrid_cloud/import_export/__init__.py 100.00% <100.00%> (ø)
src/sentry/testutils/silo.py 84.91% <100.00%> (ø)
src/sentry/backup/helpers.py 95.34% <75.00%> (-4.66%) ⬇️
...try/services/hybrid_cloud/import_export/service.py 90.47% <90.47%> (ø)
...entry/services/hybrid_cloud/import_export/model.py 95.52% <95.52%> (ø)
...sentry/services/hybrid_cloud/import_export/impl.py 91.66% <91.66%> (ø)
src/sentry/backup/exports.py 81.57% <81.81%> (-3.37%) ⬇️
src/sentry/testutils/helpers/backups.py 89.36% <63.63%> (-8.01%) ⬇️

... and 6 files with indirect coverage changes

Base automatically changed from azaslavsky/backup/refactor/instance_id to master October 13, 2023 15:46
@azaslavsky azaslavsky force-pushed the azaslavsky/backup/export_rpc branch from 5516fd8 to 1bf2a99 Compare October 13, 2023 16:08
@azaslavsky azaslavsky marked this pull request as ready for review October 13, 2023 16:41
@azaslavsky azaslavsky requested a review from a team as a code owner October 13, 2023 16:41
@azaslavsky azaslavsky requested a review from a team October 13, 2023 16:41
Exports are already done on a sequential, per-model basis, going in
reverse dependency order. This change modifies that scheme to work
across an RPC boundary: code running on REGION instances (the only kind
of instance that may perform import/export operations) requests the
exports using either the local implementation (if the model is REGION as
well), or by sending an RPC call to the same method on the remote
CONTROL silo instance. Code executed in MONOLITH mode (aka, self-hosted
and single tenant instances) does all exporting locally.

The new `import_export_service` is thus a bit different from some of the
others in `hybrid_cloud`. Whereas existing services like `Access`
provide a means to get the same information using different logic
depending on which Silo they are located in (ex: using `Organization` or
`OrganizationMapping` as the source of truth), this service executes
identical logic in all silo kinds, and requires users to pick the
implementation themselves. That is, users must examine the model they
are about to request be exported, and make their own decision as to
which implementation, remote or local, to use. This is a bit unergonomic
compared to other `hybrid_cloud` services, but it is only used by one
rather unique endpoint (the import/export functions) for a very specific
purpose, making this API a bit easier to swallow.

Issue: getsentry/team-ospo#196
@azaslavsky azaslavsky force-pushed the azaslavsky/backup/export_rpc branch from ae98c03 to 2ffec79 Compare October 13, 2023 19:48
azaslavsky added a commit that referenced this pull request Oct 13, 2023
This finishes out the work started in #57740 and enables imports over
RPC as well. Like that PR, imports are already done on a sequential, per
model basis, so this change just consists of moving every such call
across an RPC boundary.

Closes getsentry/team-ospo#185
Closes getsentry/team-ospo#196
Copy link
Member

@hubertdeng123 hubertdeng123 left a comment

Choose a reason for hiding this comment

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

did a tentative scan and looks fine to me, but I'm lacking context. +1 to unblock

@azaslavsky azaslavsky merged commit df24d61 into master Oct 13, 2023
@azaslavsky azaslavsky deleted the azaslavsky/backup/export_rpc branch October 13, 2023 23:25
azaslavsky added a commit that referenced this pull request Oct 16, 2023
This finishes out the work started in #57740 and enables imports over
RPC as well. Like that PR, imports are already done on a sequential, per
model basis, so this change just consists of moving every such call
across an RPC boundary.

Closes getsentry/team-ospo#185
Closes getsentry/team-ospo#196
azaslavsky added a commit that referenced this pull request Oct 17, 2023
This finishes out the work started in #57740 and enables imports over
RPC as well. Like that PR, imports are already done on a sequential, per
model basis, so this change just consists of moving every such call
across an RPC boundary.

Closes getsentry/team-ospo#185
Closes getsentry/team-ospo#196
azaslavsky added a commit that referenced this pull request Oct 18, 2023
feat(backup): Enable imports over RPC

This finishes out the work started in #57740 and enables imports over
RPC as well. Like that PR, imports are already done on a sequential, per
model basis, so this change just consists of moving every such call
across an RPC boundary.

Closes getsentry/team-ospo#185
Closes getsentry/team-ospo#196
Closes getsentry/team-ospo#202
azaslavsky added a commit that referenced this pull request Oct 18, 2023
This finishes out the work started in #57740 and enables imports over
RPC as well. Like that PR, imports are already done on a sequential, per
model basis, so this change just consists of moving every such call
across an RPC boundary.

This is a second attempt at landing this PR, with fewer tests tested in
both modes.
azaslavsky added a commit that referenced this pull request Oct 19, 2023
This finishes out the work started in #57740 and enables imports over
RPC as well. Like that PR, imports are already done on a sequential, per
model basis, so this change just consists of moving every such call
across an RPC boundary.

This is a second attempt at landing this PR, with fewer tests tested in
both modes.
@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.

3 participants