Delete orphan files for topics. #8185

ZeDRoman · 2023-01-12T16:14:59Z

This pr made for backporting temporary fix for previous revision. PR with proper solution will be introduced later.

When node is restarted while it was executing partition delete operation, this operation may be not finished and we will have orphan files for that partition left on device.
On node restart we don't have that partition data in memory so we couldn't retry partition delete operation and won't clean up partition files on reconciliation.
This pr bring garbage collector mechanism that will force delete partition files after controller log delete partition operation is reconciled.

Backports Required

UX Changes

Mechanism for cleaning partition orphan files

Release Notes

Bug Fixes

Mechanism for cleaning partition orphan files

dotnwat · 2023-01-12T18:48:06Z

src/v/storage/log_manager.cc

+      model::revision_id(boost::lexical_cast<uint64_t>(match[2].str())));
+}
+
+ss::future<> log_manager::remove_orphan(


bharathv

test failures seem related.

bharathv · 2023-01-13T00:41:30Z

tests/rptest/tests/topic_delete_test.py

+        down_node = self.redpanda.nodes[-1]
+        try:
+            # Make topic directory immutable to prevent deleting
+            down_node.account.ssh(


nice trick 👍

bharathv · 2023-01-13T02:37:22Z

src/v/storage/log_manager.cc

+                        ntp_directory_data->first == ntp.tp.partition
+                        && ntp_directory_data->second <= rev) {
+                          vlog(
+                            stlog.debug,


nit: bump to info? This may be useful if we realize that something was deleted by mistake in the past, may be can be moved after L453 to log ntp_directory.

bharathv · 2023-01-13T02:53:18Z

src/v/storage/log_manager.cc

@@ -401,6 +402,69 @@ ss::future<> log_manager::remove(model::ntp ntp) {
    });
 }

+/// Parse partition directory name
+static std::optional<std::pair<model::partition_id, model::revision_id>>


nit: think this can go in storage/fs_utils.h

bharathv · 2023-01-13T03:10:42Z

src/v/storage/log_manager.cc

+            .then([this, topic_directory_path]() {
+                return dispatch_topic_dir_deletion(topic_directory_path);


q: This is needed to cleanup the topic dir if this ntp is the last one to be cleaned up? Think this is okay because the newer topic revision (if one exists) will always have partitions? Hope there are no weird races if topics are deleted/created back to back in tight loops.

mmaslankaprv · 2023-01-13T07:34:57Z

src/v/storage/log_manager.cc

+/// Parse partition directory name
+static std::optional<std::pair<model::partition_id, model::revision_id>>
+parse_partition_directory(const ss::sstring& name) {
+    const std::regex re(R"(^(\d+)_(\d+)$)");


nit: can we make this static thread_local so it doesn't have to be compiled every time ?

mmaslankaprv · 2023-01-13T07:42:22Z

src/v/storage/log_manager.cc

+}
+
+ss::future<> log_manager::remove_orphan(
+  ss::sstring topic_directory_path, model::ntp ntp, model::revision_id rev) {


i think we do not need to provide topic directory here as we have an ntp already, it should be enough to provide base data directory

mmaslankaprv · 2023-01-13T07:42:56Z

src/v/cluster/controller_backend.cc

+    // partition (i.e. removal mode is global), we need to delete from the
+    // table regardless of whether a replica of 'ntp' is present on the
+    // node.


nit: unrelated change

mmaslankaprv · 2023-01-17T08:14:59Z

src/v/storage/log_manager.cc

@@ -401,6 +402,71 @@ ss::future<> log_manager::remove(model::ntp ntp) {
    });
 }

+ss::future<> log_manager::remove_orphan(
+  ss::sstring data_directory_path, model::ntp ntp, model::revision_id rev) {
+    vlog(stlog.info, "Asked to remove orphan for: {} revision: {}", ntp, rev);


nit: can we be more specific here i.e. Asked to remove orphaned partition directory

I think no, because we remove all directories with rev less than provided.
We will log exact directories later

mmaslankaprv · 2023-01-17T08:15:27Z

src/v/storage/log_manager.cc

+  ss::sstring data_directory_path, model::ntp ntp, model::revision_id rev) {
+    vlog(stlog.info, "Asked to remove orphan for: {} revision: {}", ntp, rev);
+
+    auto topic_directory_path = (std::filesystem::path(data_directory_path)


mmaslankaprv · 2023-01-17T08:15:48Z

src/v/storage/log_manager.cc

+    if (_logs.contains(ntp)) {
+        co_return;
+    }


This check can be done before creating a directory path

mmaslankaprv · 2023-01-17T08:16:47Z

src/v/storage/log_manager.cc

+          .done()
+          .finally(
+            [topic_directory]() mutable { return topic_directory.close(); });
+    } catch (ss::broken_promise const&) {


we should not catch broken_promise exception as it indicates some other issue f.e. promise is deleted before setting the result,

It occurs when we delete directory topic directory in other shard before closing it here.
We don't have any cross-shard mutex to do it, so I came up with this solution

This smells like a bug in the list_directory() implementation in seastar. It seems like if there is an exception, the underlying stream is destroyed without calling close destroying the promise.

mmaslankaprv · 2023-01-17T08:22:31Z

tests/rptest/tests/topic_delete_test.py

+            for node in self.redpanda.nodes:
+                self.logger.error(f"Storage listing on {node.name}:")
+                for line in node.account.ssh_capture(
+                        f"find {self.redpanda.DATA_DIR}"):
+                    self.logger.error(line.strip())
+
+            raise


this can be extracted to a separate function

When redpanda is restarted while delete operation is not finish Partition files might be left on disk. We need to cleanup orphan partition files

bharathv

lgtm.

bharathv · 2023-01-18T21:39:17Z

src/v/storage/log_manager.cc

+        }
+    }
+    vlog(
+      stlog.info,


nit: think we can lower this to debug (logged for every orphan cleanup).

Trying to clean up orphan topic directory:

We don't know if the topic directory is "orphan" at this point? there may be other partitions we are just attempting to schedule a deletion if it is empty.

ZeDRoman · 2023-01-20T11:27:26Z

/backport v22.2.x

ZeDRoman · 2023-01-20T11:27:37Z

/backport v22.1.x

vbotbuildovich · 2023-01-20T11:28:15Z

The pull request's base branch is not the default one. Cancelling backport...

Workflow run logs.

vbotbuildovich · 2023-01-20T11:28:25Z

The pull request's base branch is not the default one. Cancelling backport...

Workflow run logs.

github-actions bot added the area/redpanda label Jan 12, 2023

ZeDRoman mentioned this pull request Jan 12, 2023

controller: clean up topic orphan files on bootstrap #7940

Closed

6 tasks

ZeDRoman force-pushed the topic-delete-orphan-files branch from 5d5b371 to 5f8c4a9 Compare January 12, 2023 16:36

ZeDRoman marked this pull request as ready for review January 12, 2023 16:38

ZeDRoman requested review from bharathv and mmaslankaprv January 12, 2023 16:39

ZeDRoman force-pushed the topic-delete-orphan-files branch from 5f8c4a9 to bd8a715 Compare January 12, 2023 16:41

dotnwat reviewed Jan 12, 2023

View reviewed changes

bharathv reviewed Jan 13, 2023

View reviewed changes

mmaslankaprv reviewed Jan 13, 2023

View reviewed changes

ZeDRoman force-pushed the topic-delete-orphan-files branch 5 times, most recently from ff43304 to f617393 Compare January 16, 2023 21:08

mmaslankaprv reviewed Jan 17, 2023

View reviewed changes

ZeDRoman force-pushed the topic-delete-orphan-files branch from f617393 to 8e79664 Compare January 18, 2023 17:56

ZeDRoman requested review from bharathv and mmaslankaprv January 18, 2023 18:59

ZeDRoman added 2 commits January 18, 2023 22:01

controller: clean up topic orphan files

84023b0

When redpanda is restarted while delete operation is not finish Partition files might be left on disk. We need to cleanup orphan partition files

ducktape: delete orphan partition files test

79f90d3

ZeDRoman force-pushed the topic-delete-orphan-files branch from 8e79664 to 79f90d3 Compare January 18, 2023 19:01

bharathv approved these changes Jan 18, 2023

View reviewed changes

ZeDRoman merged commit c986662 into redpanda-data:v22.3.x Jan 20, 2023

ZeDRoman mentioned this pull request Jan 26, 2023

[v22.2.x] Topic delete orphan files #8434

Merged

6 tasks

graphcareful mentioned this pull request Jun 29, 2023

storage: Orphaned local segments not removed in all situations #11787

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delete orphan files for topics. #8185

Delete orphan files for topics. #8185

ZeDRoman commented Jan 12, 2023 •

edited

Loading

dotnwat Jan 12, 2023

bharathv left a comment

bharathv Jan 13, 2023

bharathv Jan 13, 2023

bharathv Jan 13, 2023

bharathv Jan 13, 2023

mmaslankaprv Jan 13, 2023

mmaslankaprv Jan 13, 2023

mmaslankaprv Jan 13, 2023

mmaslankaprv Jan 17, 2023

ZeDRoman Jan 17, 2023

mmaslankaprv Jan 17, 2023

mmaslankaprv Jan 17, 2023

mmaslankaprv Jan 17, 2023

ZeDRoman Jan 17, 2023

bharathv Jan 17, 2023

mmaslankaprv Jan 17, 2023

bharathv left a comment

bharathv Jan 18, 2023

ZeDRoman commented Jan 20, 2023

ZeDRoman commented Jan 20, 2023

vbotbuildovich commented Jan 20, 2023

vbotbuildovich commented Jan 20, 2023

		.then([this, topic_directory_path]() {
		return dispatch_topic_dir_deletion(topic_directory_path);

Delete orphan files for topics. #8185

Delete orphan files for topics. #8185

Conversation

ZeDRoman commented Jan 12, 2023 • edited Loading

Backports Required

UX Changes

Release Notes

Bug Fixes

Choose a reason for hiding this comment

bharathv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bharathv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZeDRoman commented Jan 20, 2023

ZeDRoman commented Jan 20, 2023

vbotbuildovich commented Jan 20, 2023

vbotbuildovich commented Jan 20, 2023

ZeDRoman commented Jan 12, 2023 •

edited

Loading