[backup] Improve attachment handling
ClosedPublic
Actions

Authored by michal on Aug 30 2023, 2:27 AM.

Details

Reviewers

bartek
• patryk
kamil
• jon

Commits

rCOMMdf0b40f1a30c: [backup] Improve attachment handling

Summary

ENG

Added better attachment handling and cleanup:

attachments actually need to be complete BlobInfo and not just holders so changes their type to Vec<BlobInfo> and their dynamodb type to list of maps
- also changes the column/constants name to better match this
- added a new helper conversion method for vector of any TryFromAttr type
added attachment cleanup on upload error, and on standard backup cleanup

Test Plan

Send upload endpoint with two attachments: "non-existant-hash" and "keys_hash" (the same hash as for the user_keys, so it will be uploaded during backup upload before handling attachments)
Checked that:

there two holders for "keys_hash" (one from user keys and one from an attachment)
there is a holder for "non-existant-hash" but there was a warning from backup

Uploaded similar backup, with different hashes, to trigger backup cleanup:

check that all hashes and holders from the previous backup are gone
check that there are new hashes and holders analogous to the previous backup

Restart backup service with dummy localstack endpoint so the upload fails:

observer no changes in the blob dynamodb

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

michal created this revision.Aug 30 2023, 2:27 AM

Herald added a reviewer: • jon. · View Herald TranscriptAug 30 2023, 2:27 AM

Herald added subscribers: tomek, ashoat. · View Herald Transcript

michal added a child revision: D9017: [backup] Cleanup utils.Aug 30 2023, 2:33 AM

Harbormaster completed remote builds in B22239: Diff 30536.Aug 30 2023, 2:44 AM

michal requested review of this revision.Aug 30 2023, 2:44 AM

michal added a child revision: D9019: [backup] Update db constants.Aug 30 2023, 2:46 AM

Mostly looks good to me, let someone else review

services/backup/src/database/backup_item.rs
133 ↗	(On Diff #30536)	I think macros make sense if you have items, but might be more idiomatic to just use `new()` in this case. Don't feel strongly, and it's just a matter of preference.
services/comm-services-lib/src/database.rs
285 ↗	(On Diff #30536)	I think we have 3 different implemenations of this, should probably move this to `comm-service-lib` if we able to. (I know we have some crate version issues)

bartek accepted this revision.Aug 30 2023, 12:08 PM

bartek added inline comments.

services/backup/src/database/backup_item.rs
133 ↗	(On Diff #30536)	agree with this one, especially since you use `Vec::new()` below
services/backup/src/http/handlers/backup.rs
67–68 ↗	(On Diff #30536)	You're inconsistent
67–78 ↗	(On Diff #30536)	If you want to be fancy, functional and immutable 😉
195 ↗	(On Diff #30536)	What does `:?` format do for `&str` arguments?
services/comm-services-lib/src/database.rs
285 ↗	(On Diff #30536)	This diff actually touches `comm-services-lib` 😅 I'm willing to finally take care of this once there's less traffic around DDB related code