#gpowerd Support loading empty saved actions
This change updates `findLargestFileId` to return `std::optional<uint64_t>`, allowing it to clearly indicate when no valid "savedactions_" files are found. `ReadSavedActions` now returns an empty `SavedActions` proto in this case, rather than an error.
Additional tests are added to cover scenarios with no files, invalid filenames, and corrupted file contents.
PiperOrigin-RevId: 834808486
Change-Id: I948e0ea485385df8d0e5056be0fbd2b450a6d89f
diff --git a/persistent_storage_impl.cc b/persistent_storage_impl.cc
index d39dfa3..bdff4a3 100644
--- a/persistent_storage_impl.cc
+++ b/persistent_storage_impl.cc
@@ -10,6 +10,7 @@
#include <ios>
#include <iterator>
#include <ostream>
+#include <optional>
#include <string>
#include <system_error> // NOLINT(build/c++11)
#include <vector>
@@ -103,12 +104,10 @@
if (!files.ok()) {
return files.status();
}
- auto id = findLargestFileId(files.value());
- if (!id.ok()) {
- return id.status();
- }
+ std::optional<uint64_t> id = findLargestFileId(files.value());
+ uint64_t current_id_val = id.has_value() ? id.value() : 0;
- std::string file_path = MakeFilePath(dir_path_, id.value());
+ std::string file_path = MakeFilePath(dir_path_, current_id_val);
// write the new record to the file, and close it
// this ensures that the data is always written before the file is collated
@@ -122,6 +121,11 @@
absl::StrCat("Failed to get file size", ec.message()));
}
if (file_size > max_file_size_) {
+ if (!id.has_value()) {
+ LOG(WARNING) << "A single record written to a new file " << file_path
+ << " exceeds max_file_size " << max_file_size_
+ << ". File size is " << file_size;
+ }
LOG(INFO) << "file size too large merging" << std::endl;
absl::StatusOr<SavedActions> saved_actions_write_ordered =
proto_reader::ReadProto<SavedActions>(file_path);
@@ -152,7 +156,7 @@
*(ptr_record->mutable_actions()) = i.second;
}
- std::string next_file = MakeFilePath(dir_path_, id.value() + 1);
+ std::string next_file = MakeFilePath(dir_path_, current_id_val + 1);
RETURN_IF_ERROR(WriteSavedActions(next_file, new_log));
return deleteAllFilesExceptLargest(dir_path_);
}
@@ -199,10 +203,12 @@
if (!files.ok()) {
return files.status();
}
- auto id = findLargestFileId(files.value());
- if (!id.ok()) {
- return id.status();
+
+ std::optional<uint64_t> id = findLargestFileId(files.value());
+ if (!id.has_value()) {
+ return SavedActions();
}
+
std::string file_path = MakeFilePath(dir_path_, id.value());
auto saved_actions = proto_reader::ReadProto<SavedActions>(file_path);
absl::StatusOr<SavedActions> compressed_saved_actions;
@@ -333,10 +339,10 @@
// "savedactions_1"
// "savedactions_2"
// where the largest id is the most recent file
-absl::StatusOr<uint64_t> PersistentStorageManagerImpl::findLargestFileId(
+std::optional<uint64_t>
+PersistentStorageManagerImpl::findLargestFileId(
absl::Span<const std::string> files) {
- std::error_code ec;
- uint64_t largest_id = 0;
+ std::optional<uint64_t> largest_id;
for (const auto& entry : files) {
std::vector<absl::string_view> file_name_parts = absl::StrSplit(entry, '_');
if (file_name_parts.size() != 2) {
@@ -353,7 +359,7 @@
LOG(WARNING) << "unknown file name prefix" << entry;
continue;
}
- if (file_id > largest_id) {
+ if (!largest_id.has_value() || file_id > *largest_id) {
largest_id = file_id;
}
}
@@ -382,9 +388,11 @@
if (!files.ok()) {
return files.status();
}
- auto largest = findLargestFileId(files.value());
- if (!largest.ok()) {
- return largest.status();
+ std::optional<uint64_t> largest = findLargestFileId(files.value());
+ if (!largest.has_value()) {
+ LOG(ERROR)
+ << "No largest file found, but expected to find files to delete.";
+ return absl::OkStatus();
}
std::string largest_file_name = MakeFileName(largest.value());
diff --git a/persistent_storage_impl.h b/persistent_storage_impl.h
index 2577dcb..d555709 100644
--- a/persistent_storage_impl.h
+++ b/persistent_storage_impl.h
@@ -2,6 +2,7 @@
#define PRODUCTION_BORG_MGMT_NODE_PROXY_SAFEPOWER_SAFEPOWER_AGENT_PERSISTENT_STORAGE_IMPL_H_
#include <cstdint>
+#include <optional>
#include <string>
#include <vector>
@@ -76,7 +77,7 @@
borg_mgmt::node_proxy::safepower::utils::FlightRecordRequest,
safepower_agent_persistence_proto::SavedAction, FlightRecordHash,
FlightRecordEq>& map);
- absl::StatusOr<uint64_t> findLargestFileId(
+ std::optional<uint64_t> findLargestFileId(
absl::Span<const std::string> files);
absl::StatusOr<std::vector<std::string> > listFiles(
absl::string_view file_path);
diff --git a/persistent_storage_impl_test.cc b/persistent_storage_impl_test.cc
index 1c62c06..1711a06 100644
--- a/persistent_storage_impl_test.cc
+++ b/persistent_storage_impl_test.cc
@@ -3,6 +3,7 @@
#include <cstdint>
#include <filesystem> // NOLINT(build/c++17)
#include <fstream>
+#include <ios>
#include <iterator>
#include <string>
#include <vector>
@@ -124,6 +125,16 @@
EXPECT_EQ(result_str, orig_str);
}
+TEST(PersistentStorageBMC, ReadBeforeWriteReturnsEmpty) {
+ std::string dir_name = CreateNewTestDir();
+ safepower_agent_config::PersistentStorageConfig config;
+ config.set_dir_path(dir_name);
+ persistent_storage::PersistentStorageManagerImpl persist(config);
+ auto result = persist.ReadSavedActions();
+ EXPECT_EQ(result.status(), absl::OkStatus());
+ EXPECT_EQ(result->saved_action_records_size(), 0);
+}
+
TEST(PersistentStorageBMC, WriteStateAndModifyAndRead) {
std::string dir_name = CreateNewTestDir();
auto saved_actions = GetSavedActionsWhole();
@@ -243,6 +254,76 @@
EXPECT_THAT(files, IsEmpty());
}
+void WriteActionsToFile(absl::string_view file_path,
+ const SavedActions& actions) {
+ std::ofstream ofs(std::string(file_path),
+ std::ios_base::binary | std::ios_base::out);
+ ASSERT_TRUE(ofs.is_open());
+ ASSERT_TRUE(actions.SerializeToOstream(&ofs));
+ ofs.close();
+ ASSERT_FALSE(ofs.fail());
+}
+
+TEST(PersistentStorageBMC, FindLargestFileIdIgnoresInvalidFileNames) {
+ std::string dir_name = CreateNewTestDir();
+ safepower_agent_config::PersistentStorageConfig config;
+ config.set_dir_path(dir_name);
+ persistent_storage::PersistentStorageManagerImpl persist(config);
+
+ auto actions1 = GetSavedActionsWhole("actions1");
+ auto actions2 = GetSavedActionsWhole("actions2");
+ auto actions_other = GetSavedActionsWhole("other");
+
+ WriteActionsToFile(absl::StrCat(dir_name, "/savedactions_1"), actions1);
+ WriteActionsToFile(absl::StrCat(dir_name, "/savedactions_foo"),
+ actions_other);
+ WriteActionsToFile(absl::StrCat(dir_name, "/notsavedactions_3"),
+ actions_other);
+ WriteActionsToFile(absl::StrCat(dir_name, "/savedactions"), actions_other);
+ WriteActionsToFile(absl::StrCat(dir_name, "/savedactions_"), actions_other);
+
+ auto result = persist.ReadSavedActions();
+ ASSERT_EQ(result.status(), absl::OkStatus());
+ ASSERT_EQ(result->saved_action_records_size(), 1);
+ EXPECT_EQ(result->saved_action_records(0)
+ .actions()
+ .original_request()
+ .flight_record()
+ .flight_name(),
+ "actions1");
+
+ WriteActionsToFile(absl::StrCat(dir_name, "/savedactions_2"), actions2);
+ result = persist.ReadSavedActions();
+ ASSERT_EQ(result.status(), absl::OkStatus());
+ ASSERT_EQ(result->saved_action_records_size(), 1);
+ EXPECT_EQ(result->saved_action_records(0)
+ .actions()
+ .original_request()
+ .flight_record()
+ .flight_name(),
+ "actions2");
+}
+
+TEST(PersistentStorageBMC, ReadSavedActionsIgnoresInvalidFileContent) {
+ std::string dir_name = CreateNewTestDir();
+ safepower_agent_config::PersistentStorageConfig config;
+ config.set_dir_path(dir_name);
+ persistent_storage::PersistentStorageManagerImpl persist(config);
+
+ auto actions1 = GetSavedActionsWhole("actions1");
+ WriteActionsToFile(absl::StrCat(dir_name, "/savedactions_1"), actions1);
+ // Overwrite the file with invalid content.
+ std::ofstream ofs(absl::StrCat(dir_name, "/savedactions_1"),
+ std::ios_base::binary | std::ios_base::out);
+ ASSERT_TRUE(ofs.is_open());
+ ofs << "invalid proto content";
+ ofs.close();
+ ASSERT_FALSE(ofs.fail());
+
+ auto result = persist.ReadSavedActions();
+ ASSERT_FALSE(result.ok());
+}
+
} // namespace
} // namespace persistent_storage
