Skip to content

Commit

Permalink
fix: tools-2941 properly upload state files to s3. (#101)
Browse files Browse the repository at this point in the history
* fix: properly upload state files to S3
  • Loading branch information
dwelch-spike authored Jul 17, 2024
1 parent 95985cd commit a6f2891
Show file tree
Hide file tree
Showing 4 changed files with 48 additions and 9 deletions.
24 changes: 24 additions & 0 deletions src/backup.c
Original file line number Diff line number Diff line change
Expand Up @@ -634,6 +634,30 @@ start_backup(backup_config_t* conf)
goto cleanup4;
}

if (conf->directory != NULL) {
// check if state_file_loc is an s3 path and matches
// conf->directory, this is not allowed. Allowing them to match
// means the state file would overwrite the backup files.

uint8_t file_proxy_type = file_proxy_path_type(state_file_loc);
if (file_proxy_type == FILE_PROXY_TYPE_S3) {
// what if one path ends with a '/' and the other doesn't?
if (state_file_loc[strlen(state_file_loc) - 1] == '/') {
state_file_loc[strlen(state_file_loc) - 1] = '\0';
}

if (conf->directory[strlen(conf->directory) - 1] == '/') {
conf->directory[strlen(conf->directory) - 1] = '\0';
}

if (strcmp(state_file_loc, conf->directory) == 0) {
err("--state-file-dst cannot be the same S3 path as --directory");
cf_free(state_file_loc);
goto cleanup4;
}
}
}

cf_free(conf->state_file_dst);
conf->state_file_dst = state_file_loc;
}
Expand Down
5 changes: 4 additions & 1 deletion src/backup_state.c
Original file line number Diff line number Diff line change
Expand Up @@ -136,8 +136,11 @@ backup_state_save(backup_state_t* state)
}

if (file_proxy_truncate(state->file) != 0) {
err("Unable to truncate backup state file");
inf("Warning: Unable to truncate backup state file");
// don't treat this as a critical error
// this always happens when the backup state is
// being written to S3 because file_proxy_s3_truncate
// is a no-op that returns EOF
}

// commit the backup state to the file
Expand Down
7 changes: 1 addition & 6 deletions src/upload_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -372,14 +372,9 @@ bool
UploadManager::_UploadPart(int part_number,
std::shared_ptr<Aws::StringStream>& body)
{
// Don't try uploading this part if the backup has been stopped, just
// immediately mark the part as failed and return.
backup_status_t* backup_status = get_g_backup_status();
if (backup_status_has_stopped(backup_status)) {
async_finished_mutex.lock();
failed_part_list.emplace_back(part_number, body);
async_finished_mutex.unlock();
return true;
inf("Upload manager received stop signal, finishing part: %d", part_number);
}

g_api.RegisterAsyncUpload();
Expand Down
21 changes: 19 additions & 2 deletions test/integration/test_s3.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,8 @@


def do_s3_backup(max_interrupts, n_records=10000, backup_opts=None,
restore_opts=None, state_file_dir=False, state_file_explicit=False, backup_cout=0):
restore_opts=None, state_file_dir=False, state_file_explicit=False,
backup_cout=0, state_file_to_s3=False):
if backup_opts == None:
backup_opts = []
if restore_opts == None:
Expand Down Expand Up @@ -70,6 +71,16 @@ def do_s3_backup(max_interrupts, n_records=10000, backup_opts=None,
elif state_file_explicit:
state_path = lib.temporary_path("asb.state")
state_file = state_path

if state_file_to_s3:
if state_file_explicit:
state_path = "s3://" + S3_BUCKET + "/test_dir"
state_file = state_path + "/" + lib.NAMESPACE + '.asb.state'
state_path = state_file
else:
# state path cannot be the same as backup directory
state_path = "s3://" + S3_BUCKET + "/state_file.asb.state"
state_file = state_path

while True:
opts = comp_enc_mode + backup_opts
Expand All @@ -86,7 +97,7 @@ def do_s3_backup(max_interrupts, n_records=10000, backup_opts=None,
filler = lambda context: record_gen.put_records(n_records, context,
lib.SET, False, 0)

if state_file_dir or state_file_explicit:
if state_file_dir or state_file_explicit or state_file_to_s3:
opts += ['--state-file-dst', state_path]

opts += COMMON_S3_OPTS
Expand Down Expand Up @@ -158,6 +169,12 @@ def do_s3_backup(max_interrupts, n_records=10000, backup_opts=None,

min_srv.stop_minio_server(MINIO_NAME)

def test_s3_state_file_to_s3():
do_s3_backup(1, n_records=50000, backup_opts=['--records-per-second', '2000'], state_file_to_s3=True)

def test_s3_backup_multiple_files_state_file_to_s3():
do_s3_backup(10, n_records=10000, backup_opts=['--file-limit', '1'], state_file_to_s3=True, state_file_explicit=True)

def test_s3_backup_small():
do_s3_backup(0, n_records=100, backup_opts=['--s3-region', S3_REGION])

Expand Down

0 comments on commit a6f2891

Please sign in to comment.