cleanup tiered storage temporary cache file if exceptions are thrown during download #24000

nvartolomei · 2024-11-04T17:02:05Z

See CORE-8113

Backports Required

Release Notes

Bug Fixes

Cleanup tiered storage temporary cache file if exceptions are thrown during download.

Noticed while manually reviewing code. No change in behavior expected.

nvartolomei · 2024-11-04T17:05:55Z

src/v/cloud_storage/cache_service.cc

+            co_await delete_file_and_empty_parents(
+              (dir_path / tmp_filename).native());
+        } catch (...) {
+            vlog(


We're still leaving a file here which is untracked but if this fails I believe the system has worse problems to deal with. We could commit the reservation but I don't think the code complexity is worth it.

nvartolomei · 2024-11-04T17:07:08Z

src/v/cloud_storage/cache_service.cc


-        throw disk_full_error;
+        std::rethrow_exception(eptr);
    }

    // commit write transaction


We can still throw below and leave the file untracked on disk. I think it is fine though. If getting file size and renaming fails we have worse problems to deal with.

auto put_size = co_await ss::file_size(src); co_await ss::rename_file(src, dest); // We will now update reservation.wrote_data(put_size, 1);

nvartolomei

Self review.

WillemKauf

Nice RCA, test, and fix!

src/v/cloud_storage/tests/cache_test.cc

src/v/cloud_storage/cache_service.cc

src/v/cloud_storage/tests/cache_test.cc

Prepare for tmp cleanup on failure.

vbotbuildovich · 2024-11-04T22:36:59Z

the below tests from https://buildkite.com/redpanda/redpanda/builds/57550#0192f8da-b7c5-4233-9e0a-631773ce65ea have failed and will be retried

catalog_schema_manager_rpunit

vbotbuildovich · 2024-11-05T14:35:18Z

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/57550#0192fc38-263c-4190-9b05-a0fabfe137d5
ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/57762#0193068c-d2c3-4759-8ecb-c9d83b393c01

nvartolomei · 2024-11-05T14:40:46Z

/cdt
rp_version=pr

nvartolomei · 2024-11-06T13:46:58Z

/cdt
rp_version=pr
tests/rptest/tests/partition_move_interruption_test.py

nvartolomei · 2024-11-06T13:48:20Z

/cdt
tests/rptest/tests/partition_move_interruption_test.py

src/v/cloud_storage/cache_service.cc

andrwng

LGTM, though +1 to Evgeny's comment about shutdown errors

src/v/cloud_storage/cache_service.cc

vbotbuildovich · 2024-11-07T14:03:40Z

/backport v24.2.x

vbotbuildovich · 2024-11-07T14:03:41Z

/backport v24.1.x

vbotbuildovich · 2024-11-07T14:04:50Z

Failed to create a backport PR to v24.1.x branch. I tried:

git remote add upstream https://github.com/redpanda-data/redpanda.git
git fetch --all
git checkout -b backport-pr-24000-v24.1.x-403 remotes/upstream/v24.1.x
git cherry-pick -x 6e1e998021 be77f709bc 5a93114f8a 50fef29c79 b34aab6da1 e90eafbbfb

Workflow run logs.

vbotbuildovich · 2024-11-07T14:04:53Z

Failed to create a backport PR to v24.2.x branch. I tried:

git remote add upstream https://github.com/redpanda-data/redpanda.git
git fetch --all
git checkout -b backport-pr-24000-v24.2.x-215 remotes/upstream/v24.2.x
git cherry-pick -x 6e1e998021 be77f709bc 5a93114f8a 50fef29c79 b34aab6da1 e90eafbbfb

Workflow run logs.

dotnwat · 2024-11-19T23:37:26Z

src/v/cloud_storage/cache_service.cc

+              && !ssx::is_shutdown_exception(delete_tmp_fut.get_exception())) {
+                vlog(
+                  cst_log.error,
+                  "Failed to delete tmp file {}: {}",
+                  tmp_filepath.native(),
+                  delete_tmp_fut.get_exception());


@nvartolomei I don't think it is safe to call get_exception twice. from what i can tell it moves the _state out of the future (see future::get_available_state_ref)

@dotnwat

Found by @dotnwat redpanda-data#24000 (comment)

@dotnwat

Found by @dotnwat redpanda-data#24000 (comment) (cherry picked from commit ec90163)

@dotnwat

Found by @dotnwat redpanda-data#24000 (comment) (cherry picked from commit ec90163)

@dotnwat

Found by @dotnwat redpanda-data#24000 (comment) (cherry picked from commit ec90163)

@dotnwat

Found by @dotnwat redpanda-data#24000 (comment)

nvartolomei added 2 commits November 4, 2024 15:04

cloud_storage: throw exception not a pointer

6e1e998

Noticed while manually reviewing code. No change in behavior expected.

test_utils: extract make_throwing_stream as a test_util

be77f70

github-actions bot added area/build area/redpanda labels Nov 4, 2024

nvartolomei requested review from Lazin, WillemKauf and andrwng November 4, 2024 17:03

nvartolomei commented Nov 4, 2024

View reviewed changes

WillemKauf previously approved these changes Nov 4, 2024

View reviewed changes

src/v/cloud_storage/tests/cache_test.cc Show resolved Hide resolved

src/v/cloud_storage/cache_service.cc Outdated Show resolved Hide resolved

src/v/cloud_storage/tests/cache_test.cc Show resolved Hide resolved

nvartolomei dismissed WillemKauf’s stale review via d63c50c November 4, 2024 17:59

nvartolomei force-pushed the nv/CORE-8113 branch 2 times, most recently from d63c50c to c8e245b Compare November 4, 2024 18:02

WillemKauf previously approved these changes Nov 4, 2024

View reviewed changes

nvartolomei added 3 commits November 4, 2024 20:24

cst/cache: Test current exception handling during put

5a93114

cst/cache: Extract tmp_filepath variable

50fef29

cst/cache: Refactor error handling during put

b34aab6

Prepare for tmp cleanup on failure.

nvartolomei dismissed WillemKauf’s stale review via eb4108b November 4, 2024 20:24

nvartolomei force-pushed the nv/CORE-8113 branch from c8e245b to eb4108b Compare November 4, 2024 20:24

nvartolomei force-pushed the nv/CORE-8113 branch from eb4108b to 013378e Compare November 6, 2024 13:48

Lazin reviewed Nov 6, 2024

View reviewed changes

src/v/cloud_storage/cache_service.cc Outdated Show resolved Hide resolved

andrwng reviewed Nov 6, 2024

View reviewed changes

src/v/cloud_storage/cache_service.cc Show resolved Hide resolved

src/v/cloud_storage/cache_service.cc Outdated Show resolved Hide resolved

nvartolomei enabled auto-merge November 7, 2024 07:28

Lazin reviewed Nov 7, 2024

View reviewed changes

src/v/cloud_storage/cache_service.cc Show resolved Hide resolved

cst/cache: Remove tmp file on put failure

e90eafb

nvartolomei force-pushed the nv/CORE-8113 branch from 013378e to e90eafb Compare November 7, 2024 10:37

nvartolomei requested a review from Lazin November 7, 2024 10:46

Lazin approved these changes Nov 7, 2024

View reviewed changes

nvartolomei merged commit 969693b into redpanda-data:dev Nov 7, 2024
16 checks passed

This was referenced Nov 7, 2024

[v24.1.x] cleanup tiered storage temporary cache file if exceptions are thrown during download #24056

Closed

[v24.2.x] cleanup tiered storage temporary cache file if exceptions are thrown during download #24057

Closed

This was referenced Nov 7, 2024

[v24.2.x] cleanup tiered storage temporary cache file if exceptions are thrown during download #24063

Merged

[v24.1.x] cleanup tiered storage temporary cache file if exceptions are thrown during download #24064

Merged

dotnwat reviewed Nov 19, 2024

View reviewed changes

nvartolomei added a commit to nvartolomei/redpanda that referenced this pull request Nov 20, 2024

cst/cache: fix use-after-move caused by calling get_exception twice

ec90163

Found by @dotnwat redpanda-data#24000 (comment)

nvartolomei mentioned this pull request Nov 20, 2024

cst/cache: fix use-after-move caused by calling get_exception twice #24189

Merged

7 tasks

nvartolomei deleted the nv/CORE-8113 branch November 20, 2024 01:17

vbotbuildovich pushed a commit to vbotbuildovich/redpanda that referenced this pull request Nov 20, 2024

cst/cache: fix use-after-move caused by calling get_exception twice

0a36379

Found by @dotnwat redpanda-data#24000 (comment) (cherry picked from commit ec90163)

vbotbuildovich pushed a commit to vbotbuildovich/redpanda that referenced this pull request Nov 20, 2024

cst/cache: fix use-after-move caused by calling get_exception twice

5c18cb9

Found by @dotnwat redpanda-data#24000 (comment) (cherry picked from commit ec90163)

vbotbuildovich pushed a commit to vbotbuildovich/redpanda that referenced this pull request Nov 20, 2024

cst/cache: fix use-after-move caused by calling get_exception twice

034b822

Found by @dotnwat redpanda-data#24000 (comment) (cherry picked from commit ec90163)

WillemKauf pushed a commit to WillemKauf/redpanda that referenced this pull request Dec 3, 2024

cst/cache: fix use-after-move caused by calling get_exception twice

75c67de

Found by @dotnwat redpanda-data#24000 (comment)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cleanup tiered storage temporary cache file if exceptions are thrown during download #24000

cleanup tiered storage temporary cache file if exceptions are thrown during download #24000

nvartolomei commented Nov 4, 2024 •

edited

Loading

nvartolomei Nov 4, 2024

nvartolomei Nov 4, 2024

nvartolomei left a comment

WillemKauf left a comment

vbotbuildovich commented Nov 4, 2024

vbotbuildovich commented Nov 5, 2024 •

edited

Loading

nvartolomei commented Nov 5, 2024

nvartolomei commented Nov 6, 2024

nvartolomei commented Nov 6, 2024

andrwng left a comment

vbotbuildovich commented Nov 7, 2024

vbotbuildovich commented Nov 7, 2024

vbotbuildovich commented Nov 7, 2024

vbotbuildovich commented Nov 7, 2024

dotnwat Nov 19, 2024

nvartolomei Nov 20, 2024

cleanup tiered storage temporary cache file if exceptions are thrown during download #24000

cleanup tiered storage temporary cache file if exceptions are thrown during download #24000

Conversation

nvartolomei commented Nov 4, 2024 • edited Loading

Backports Required

Release Notes

Bug Fixes

nvartolomei Nov 4, 2024

Choose a reason for hiding this comment

nvartolomei Nov 4, 2024

Choose a reason for hiding this comment

nvartolomei left a comment

Choose a reason for hiding this comment

WillemKauf left a comment

Choose a reason for hiding this comment

vbotbuildovich commented Nov 4, 2024

vbotbuildovich commented Nov 5, 2024 • edited Loading

nvartolomei commented Nov 5, 2024

nvartolomei commented Nov 6, 2024

nvartolomei commented Nov 6, 2024

andrwng left a comment

Choose a reason for hiding this comment

vbotbuildovich commented Nov 7, 2024

vbotbuildovich commented Nov 7, 2024

vbotbuildovich commented Nov 7, 2024

vbotbuildovich commented Nov 7, 2024

dotnwat Nov 19, 2024

Choose a reason for hiding this comment

nvartolomei Nov 20, 2024

Choose a reason for hiding this comment

nvartolomei commented Nov 4, 2024 •

edited

Loading

vbotbuildovich commented Nov 5, 2024 •

edited

Loading