feat(hybridcloud) Use control file for user/sentryapp avatars #54118
Conversation
Going forward always write to ControlFile for User & sentry app avatars. This helps make application logic more consistent going forwards as new avatars are always stored in ControlFile. I've retained read paths for `File` backed avatars so that we preserve behavior in both single-tenant and self-hosted. We also retain the ability to write to a control silo specific storage bucket should an environment be configured in that way.
github-actions
bot
added
the
Scope: Backend
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #54118 +/- ##
=======================================
Coverage 79.65% 79.66%
=======================================
Files 4989 4989
Lines 211491 211509 +18
Branches 36048 36051 +3
=======================================
+ Hits 168471 168503 +32
+ Misses 37834 37825 -9
+ Partials 5186 5181 -5
|
| if ControlFileBlob._storage_config(): | ||
| def file_fk(self) -> str: | ||
| if self.control_file_id: | ||
| return "control_file_id" | ||
| return "file_id" | ||
| if self.file_id: | ||
| return "file_id" | ||
| return "control_file_id" |
There was a problem hiding this comment.
This behavior may be a bit problematic in split DB test contexts, since accessing the normal file_id in a fallback case will likely result in a cross-DB access in some transaction contexts (see test_doc_integration_avatar.py for example). Are we sure we don't want to just default to control_file_id at this point?
There was a problem hiding this comment.
Non-saas environments never moved useravatar or sentryapp avatars to control file (as they don't have the separate bucket config). We need to continue using file_id for those environments as that is where their files are.
I'm considering following up this change with a backfill migration that copies all the file/fileblob/fileblobindex records for user & sentryapp avatars into ControlFile. If we do that backfill then all environments will become consistent and we can simplify this code as we don't need to split reads anymore.
There was a problem hiding this comment.
Ahh I didn't realize we hadn't backfilled the non-saas deploys 😞 Yeah that makes sense. Should we maybe do an env check for split db mode and fail fast in that scenario just to ensure we don't keep around these cross-db accesses?
There was a problem hiding this comment.
Yeah a split db check could work too. I'd like to get rid of the cross-db reads, but how to get there is the part I'm still trying to figure out. Migrations only have access to historical models which don't have the methods we need to move file records between backends. We could spawn the copy file task, but we'll need to account for the task changing signature/going missing in the future.
|
PR reverted: 4e10fc3 |
) Mulligan on #54118. The first merge of this failed as the new logic is incompatible with a test in getsentry. This reverts commit 4e10fc3. Requires getsentry/getsentry#11392
Going forward always write to ControlFile for User & sentry app avatars. This helps make application logic more consistent going forwards as new avatars are always stored in ControlFile.
I've retained read paths for
Filebacked avatars so that we preserve behavior in both single-tenant and self-hosted. We also retain the ability to write to a control silo specific storage bucket should an environment be configured in that way.I'd like to phase out the storage rebuild task as we're not planning on doing it in any of the non-saas environments and self-hosted multi-region sentry isn't something we're planning on supporting either.