[feedback] Report store does garbage collection
The store uses a simple FIFO-policy garbage collection when there isn't
enough space for a new report.
Bug: 47137
TESTED=`fx test crash-report-tests`
Change-Id: I7b3cf5ae258be8275c61f09fd0501b52161c25dc
Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/404986
Commit-Queue: Alex Pankhurst <pankhurst@google.com>
Reviewed-by: Francois Rousseau <frousseau@google.com>
Testability-Review: Francois Rousseau <frousseau@google.com>
Testability-Review: Alex Pankhurst <pankhurst@google.com>
diff --git a/src/developer/forensics/crash_reports/BUILD.gn b/src/developer/forensics/crash_reports/BUILD.gn
index ec21e6b..927961d 100644
--- a/src/developer/forensics/crash_reports/BUILD.gn
+++ b/src/developer/forensics/crash_reports/BUILD.gn
@@ -350,7 +350,10 @@
"store.h",
]
- public_deps = [ ":report" ]
+ public_deps = [
+ ":report",
+ "//src/developer/forensics/utils:storage_size",
+ ]
deps = [
"//sdk/lib/syslog/cpp",
diff --git a/src/developer/forensics/crash_reports/store.cc b/src/developer/forensics/crash_reports/store.cc
index 804a45a..bafd4c9 100644
--- a/src/developer/forensics/crash_reports/store.cc
+++ b/src/developer/forensics/crash_reports/store.cc
@@ -48,8 +48,7 @@
return contents;
}
-bool WriteAnnotationsAsJson(const std::string& path,
- const std::map<std::string, std::string>& annotations) {
+std::string FormatAnnotationsAsJson(const std::map<std::string, std::string>& annotations) {
rapidjson::Document json(rapidjson::kObjectType);
auto& allocator = json.GetAllocator();
@@ -64,8 +63,7 @@
json.Accept(writer);
- const std::string annotations_json = buffer.GetString();
- return files::WriteFile(path, annotations_json);
+ return buffer.GetString();
}
bool ReadAnnotations(const std::string& path, std::map<std::string, std::string>* annotations) {
@@ -105,7 +103,8 @@
} // namespace
-Store::Store(const std::string& root_dir) : root_dir_(root_dir) {}
+Store::Store(const std::string& root_dir, StorageSize max_size)
+ : root_dir_(root_dir), max_size_(max_size), current_size_(0u) {}
std::optional<Store::Uid> Store::Add(const Report report) {
for (const auto& key : kReservedAttachmentNames) {
@@ -125,20 +124,36 @@
return std::nullopt;
}
- auto MakeFilepath = [dir](const std::string& filename) { return files::JoinPath(dir, filename); };
+ const std::string annotations_json = FormatAnnotationsAsJson(report.Annotations());
- if (!WriteAnnotationsAsJson(MakeFilepath(kAnnotationsFilename), report.Annotations())) {
- FX_LOGS(ERROR) << "Failed to write annotations";
+ // Determine the size of the report.
+ StorageSize report_size = StorageSize::Bytes(annotations_json.size());
+ for (const auto& [_, v] : report.Attachments()) {
+ report_size += StorageSize::Bytes(v.size());
+ }
+ if (report.Minidump().has_value()) {
+ report_size += StorageSize::Bytes(report.Minidump().value().size());
+ }
+
+ // Ensure there's enough space in the store for the report.
+ if (!MakeFreeSpace(report_size)) {
+ FX_LOGS(ERROR) << "Failed to make space for report";
return std::nullopt;
}
+ auto MakeFilepath = [dir](const std::string& filename) { return files::JoinPath(dir, filename); };
+
+ // Write the report's content to the the filesystem.
+ if (!files::WriteFile(MakeFilepath(kAnnotationsFilename), annotations_json)) {
+ FX_LOGS(ERROR) << "Failed to write annotations";
+ return std::nullopt;
+ }
for (const auto& [filename, attachment] : report.Attachments()) {
if (!WriteAttachment(MakeFilepath(filename), attachment)) {
FX_LOGS(ERROR) << "Failed to write attachment " << filename;
return std::nullopt;
}
}
-
if (report.Minidump().has_value()) {
if (!WriteAttachment(MakeFilepath(kMinidumpFilename), report.Minidump().value())) {
FX_LOGS(ERROR) << "Failed to write minidump";
@@ -148,8 +163,12 @@
id_to_metadata_[id] = ReportMetadata{
.dir = dir,
+ .size = report_size,
.program_shortname = report.ProgramShortname(),
};
+ uids_.push_back(id);
+ current_size_ += report_size;
+
cleanup_on_error.cancel();
return id;
}
@@ -221,7 +240,9 @@
}
}
+ current_size_ -= id_to_metadata_[id].size;
id_to_metadata_.erase(id);
+ uids_.erase(std::remove(uids_.begin(), uids_.end(), id), uids_.end());
}
void Store::RemoveAll() {
@@ -229,7 +250,22 @@
FX_LOGS(ERROR) << "Failed to delete all reports";
}
files::CreateDirectory(root_dir_);
+
+ current_size_ = StorageSize::Bytes(0u);
id_to_metadata_.clear();
+ uids_.clear();
+}
+
+bool Store::MakeFreeSpace(const StorageSize required_space) {
+ if (required_space > max_size_) {
+ return false;
+ }
+
+ while ((current_size_ + required_space) > max_size_ && !uids_.empty()) {
+ Remove(uids_.front());
+ }
+
+ return true;
}
} // namespace crash_reports
diff --git a/src/developer/forensics/crash_reports/store.h b/src/developer/forensics/crash_reports/store.h
index 3e4d794..26c7a31 100644
--- a/src/developer/forensics/crash_reports/store.h
+++ b/src/developer/forensics/crash_reports/store.h
@@ -5,11 +5,13 @@
#ifndef SRC_DEVELOPER_FORENSICS_CRASH_REPORTS_STORE_H_
#define SRC_DEVELOPER_FORENSICS_CRASH_REPORTS_STORE_H_
+#include <deque>
#include <map>
#include <optional>
#include <string>
#include "src/developer/forensics/crash_reports/report.h"
+#include "src/developer/forensics/utils/storage_size.h"
namespace forensics {
namespace crash_reports {
@@ -23,7 +25,9 @@
// |root_dir| is the location in the filesystem where reports will be stored. For example,
// if |root_dir| is /tmp/store and a report for "foo" is filed, that report
// will be stored in /tmp/store/foo/<report Uid>.
- explicit Store(const std::string& root_dir);
+ // |max_size| is the maximum size the store can take, garbage collecting the reports of lowest
+ // Uids.
+ Store(const std::string& root_dir, StorageSize max_size);
// Adds a report to the store. If the operation fails, std::nullopt is returned, else a unique
// identifier referring to the report is returned.
@@ -40,17 +44,32 @@
bool Contains(const Uid& id) const;
private:
+ // Remove reports until |required_space| is free in the store.
+ //
+ // Return false if |required_space| cannot be freed.
+ bool MakeFreeSpace(StorageSize required_space);
+
struct ReportMetadata {
// The directory containing the report's files, e.g., /tmp/crashes/foo/<report Uid>
std::string dir;
+ // The total size taken by the report's files.
+ StorageSize size;
+
// "foo" in the above example, it shouldn't contain forward slashes.
std::string program_shortname;
};
std::string root_dir_;
+
+ StorageSize max_size_;
+ StorageSize current_size_;
+
std::map<Uid, ReportMetadata> id_to_metadata_;
+ // We store the Uids in FIFO order for easy garbage collection.
+ std::deque<Uid> uids_;
+
// TODO(47137): Uids may collide across component instances, however this will no longer happen
// once we rebuild the store's metadata when the component starts up.
Uid next_id_{0};
diff --git a/src/developer/forensics/crash_reports/tests/BUILD.gn b/src/developer/forensics/crash_reports/tests/BUILD.gn
index b8b7cb6..f9fe7f2 100644
--- a/src/developer/forensics/crash_reports/tests/BUILD.gn
+++ b/src/developer/forensics/crash_reports/tests/BUILD.gn
@@ -249,7 +249,9 @@
"//src/developer/forensics/crash_reports:store",
"//src/developer/forensics/testing:gtest_with_syslog_main",
"//src/developer/forensics/utils:sized_data",
+ "//src/developer/forensics/utils:storage_size",
"//src/lib/files",
+ "//third_party/googletest:gmock",
"//third_party/googletest:gtest",
"//third_party/rapidjson",
]
diff --git a/src/developer/forensics/crash_reports/tests/store_unittest.cc b/src/developer/forensics/crash_reports/tests/store_unittest.cc
index e65789d..269eced 100644
--- a/src/developer/forensics/crash_reports/tests/store_unittest.cc
+++ b/src/developer/forensics/crash_reports/tests/store_unittest.cc
@@ -10,6 +10,7 @@
#include <string>
#include <vector>
+#include <gmock/gmock.h>
#include <gtest/gtest.h>
#include "src/developer/forensics/utils/sized_data.h"
@@ -23,15 +24,21 @@
namespace crash_reports {
namespace {
+using testing::UnorderedElementsAreArray;
+
SizedData MakeSizedData(const std::string& content) {
return SizedData(content.begin(), content.end());
}
class StoreTest : public testing::Test {
public:
- StoreTest() : store_(tmp_dir_.path()) {}
+ StoreTest() : store_(std::make_unique<Store>(tmp_dir_.path(), StorageSize::Megabytes(1))) {}
protected:
+ void MakeNewStore(const StorageSize max_size) {
+ store_ = std::make_unique<Store>(tmp_dir_.path(), max_size);
+ }
+
std::optional<Store::Uid> Add(const std::string& program_shortname,
const std::map<std::string, std::string>& annotations,
const std::map<std::string, std::string>& attachments,
@@ -49,13 +56,13 @@
auto report = Report(program_shortname, annotations, std::move(attachments_data),
std::move(minidump_data));
- return store_.Add(std::move(report));
+ return store_->Add(std::move(report));
}
bool Get(const Store::Uid& id, std::string* program_shortname,
std::map<std::string, std::string>* annotations,
std::map<std::string, std::string>* attachments, std::optional<std::string>* minidump) {
- const auto report = store_.Get(id);
+ const auto report = store_->Get(id);
if (!report.has_value()) {
return false;
}
@@ -143,7 +150,7 @@
files::ScopedTempDir tmp_dir_;
protected:
- Store store_;
+ std::unique_ptr<Store> store_;
};
TEST_F(StoreTest, Succeed_Add) {
@@ -171,7 +178,7 @@
std::map<std::string, std::string> attachments;
std::optional<std::string> minidump;
- ASSERT_TRUE(store_.Contains(id.value()));
+ ASSERT_TRUE(store_->Contains(id.value()));
ASSERT_TRUE(Read(expected_program_shortname, id.value(), &annotations, &attachments, &minidump));
EXPECT_EQ(expected_annotations, annotations);
@@ -229,13 +236,35 @@
const auto id =
Add("program_shortname", /*annotations=*/{}, /*attachments=*/{}, /*minidump=*/std::nullopt);
EXPECT_TRUE(id.has_value());
- ASSERT_TRUE(store_.Contains(id.value()));
+ ASSERT_TRUE(store_->Contains(id.value()));
- store_.Remove(id.value());
- EXPECT_FALSE(store_.Contains(id.value()));
+ store_->Remove(id.value());
+ EXPECT_FALSE(store_->Contains(id.value()));
EXPECT_TRUE(GetProgramShortnames().empty());
}
+TEST_F(StoreTest, Succeed_GarbageCollection) {
+ const std::string minidump = "minidump";
+
+ // We set up the store so it can only hold one report at most, evicting the oldest ones first.
+ MakeNewStore(StorageSize::Bytes(minidump.size() + 4u /*the empty annotations.json*/));
+
+ const auto id1 = Add("program_name1", /*annotations=*/{}, /*attachments=*/{}, minidump);
+ const auto id2 = Add("program_name2", /*annotations=*/{}, /*attachments=*/{}, minidump);
+
+ EXPECT_FALSE(store_->Contains(id1.value()));
+ EXPECT_TRUE(store_->Contains(id2.value()));
+
+ EXPECT_THAT(GetProgramShortnames(), UnorderedElementsAreArray({"program_name2"}));
+
+ const auto id3 = Add("program_name3", /*annotations=*/{}, /*attachments=*/{}, minidump);
+
+ EXPECT_FALSE(store_->Contains(id2.value()));
+ EXPECT_TRUE(store_->Contains(id3.value()));
+
+ EXPECT_THAT(GetProgramShortnames(), UnorderedElementsAreArray({"program_name3"}));
+}
+
} // namespace
} // namespace crash_reports
} // namespace forensics
diff --git a/src/developer/forensics/utils/storage_size.h b/src/developer/forensics/utils/storage_size.h
index c84a72f..b96ad84 100644
--- a/src/developer/forensics/utils/storage_size.h
+++ b/src/developer/forensics/utils/storage_size.h
@@ -16,6 +16,8 @@
// prevent integer under/over flow and should be used with caution.
class StorageSize final {
public:
+ constexpr StorageSize() = default;
+
explicit constexpr StorageSize(uint64_t bytes) : bytes_(bytes) {}
static constexpr StorageSize Bytes(uint64_t bytes) { return StorageSize(bytes); }
