external_storer: Add a log entry limit
This is to ensure that we don't get uncapped log entries. Default is set
to the first 20 logs being saved, and the next 980 being in a circualr
queue (1000 entries in total).
Tested:
Added unit test to ensure that the queue works as intended. Also
verified by injecting 1000 entries to ensure that the buffer indeed
saves the first 20, and caps it 1000.
```
//LogEntry1:
Created: ...Entries/66a97808-1e22-4c1e-b0f6-4b9eb7c714e7/index.json
...
//LogEntry20:
Created: ...Entries/9c07937a-9524-40f1-acef-19db73b46678/index.json
//LogEntry21:
Created: ...Entries/0bfb7ede-8020-4613-bfcf-5815ca176c79/index.json
...
//LogEntry1001:
Removed: ...Entries/0bfb7ede-8020-4613-bfcf-5815ca176c79
Created: ...Entries/31bdf3be-1e87-4e77-922d-9bbac09ac824/index.json
```
We can see that starting on the 1001st log, log entry 21 is deleted to
make room.
Signed-off-by: Brandon Kim <brandonkim@google.com>
Change-Id: Ic2badd7f01741d788cad829c9e203b7c4962fc8e
diff --git a/include/rde/external_storer_file.hpp b/include/rde/external_storer_file.hpp
index 6d813af..d059114 100644
--- a/include/rde/external_storer_file.hpp
+++ b/include/rde/external_storer_file.hpp
@@ -7,6 +7,7 @@
#include <boost/uuid/uuid_generators.hpp>
#include <filesystem>
+#include <queue>
#include <string>
namespace bios_bmc_smm_error_logger
@@ -44,6 +45,14 @@
*/
virtual bool createFile(const std::string& folderPath,
const nlohmann::json& jsonPdr) const = 0;
+
+ /**
+ * @brief Call remove_all on the filePath
+ *
+ * @param[in] filePath - path of the file/folder to remove
+ * @return true if successful.
+ */
+ virtual bool removeAll(const std::string& filePath) const = 0;
};
/**
@@ -55,6 +64,7 @@
bool createFolder(const std::string& folderPath) const override;
bool createFile(const std::string& folderPath,
const nlohmann::json& jsonPdr) const override;
+ bool removeAll(const std::string& filePath) const override;
};
/**
@@ -81,11 +91,16 @@
* Eg: "/run/bmcweb"
* @param[in] fileHandler - an ExternalStorerFileWriter object. This class
* will take the ownership of this object.
+ * @param[in] numSavedLogEntries - first N number of log entries to be saved
+ * in the queue (default shall be 20)
+ * @param[in] numLogEntries - number of non-saved log entries in the queue
+ * (default shall be 1000 - 20 = 980)
*/
ExternalStorerFileInterface(
const std::shared_ptr<sdbusplus::asio::connection>& conn,
std::string_view rootPath,
- std::unique_ptr<FileHandlerInterface> fileHandler);
+ std::unique_ptr<FileHandlerInterface> fileHandler,
+ uint32_t numSavedLogEntries = 20, uint32_t numLogEntries = 980);
bool publishJson(std::string_view jsonStr) override;
@@ -95,6 +110,12 @@
std::string logServiceId;
std::unique_ptr<CperFileNotifierHandler> cperNotifier;
boost::uuids::random_generator randomGen;
+ std::queue<std::string> logEntrySavedQueue;
+ std::queue<std::string> logEntryQueue;
+ // Default should be 20
+ const uint32_t maxNumSavedLogEntries;
+ // Default should be 1000 - maxNumSavedLogEntries(20) = 980
+ const uint32_t maxNumLogEntries;
/**
* @brief Get the type of the received PDR.
diff --git a/src/rde/external_storer_file.cpp b/src/rde/external_storer_file.cpp
index 1ffea44..3aa0abf 100644
--- a/src/rde/external_storer_file.cpp
+++ b/src/rde/external_storer_file.cpp
@@ -45,12 +45,27 @@
return true;
}
+bool ExternalStorerFileWriter::removeAll(const std::string& filePath) const
+{
+ // Attempt to delete the file
+ std::error_code ec;
+ std::filesystem::remove_all(filePath, ec);
+ if (ec)
+ {
+ return false;
+ }
+ stdplus::print(stderr, "Removed: {}\n", filePath);
+ return true;
+}
+
ExternalStorerFileInterface::ExternalStorerFileInterface(
const std::shared_ptr<sdbusplus::asio::connection>& conn,
std::string_view rootPath,
- std::unique_ptr<FileHandlerInterface> fileHandler) :
+ std::unique_ptr<FileHandlerInterface> fileHandler,
+ uint32_t numSavedLogEntries, uint32_t numLogEntries) :
rootPath(rootPath), fileHandler(std::move(fileHandler)), logServiceId(""),
- cperNotifier(std::make_unique<CperFileNotifierHandler>(conn))
+ cperNotifier(std::make_unique<CperFileNotifierHandler>(conn)),
+ maxNumSavedLogEntries(numSavedLogEntries), maxNumLogEntries(numLogEntries)
{}
bool ExternalStorerFileInterface::publishJson(std::string_view jsonStr)
@@ -118,6 +133,23 @@
return false;
}
+ // Check to see if we are hitting the limit of filePathQueue, delete oldest
+ // log entry first before processing another entry
+ if (logEntryQueue.size() == maxNumLogEntries)
+ {
+ std::string oldestFilePath = std::move(logEntryQueue.front());
+ logEntryQueue.pop();
+
+ if (!fileHandler->removeAll(oldestFilePath))
+ {
+ stdplus::print(
+ stderr,
+ "Failed to delete the oldest entry path, not processing the next log,: {}\n",
+ oldestFilePath);
+ return false;
+ }
+ }
+
std::string id = boost::uuids::to_string(randomGen());
std::string fullPath =
std::format("{}/redfish/v1/Systems/system/LogServices/{}/Entries/{}",
@@ -138,6 +170,18 @@
}
cperNotifier->createEntry(fullPath + "/index.json");
+
+ // Attempt to push to logEntrySavedQueue first, before pushing to
+ // logEntryQueue that can be popped
+ if (logEntrySavedQueue.size() < maxNumSavedLogEntries)
+ {
+ logEntrySavedQueue.push(std::move(fullPath));
+ }
+ else
+ {
+ logEntryQueue.push(std::move(fullPath));
+ }
+
return true;
}
diff --git a/test/external_storer_file_test.cpp b/test/external_storer_file_test.cpp
index 3cba35d..c0f2578 100644
--- a/test/external_storer_file_test.cpp
+++ b/test/external_storer_file_test.cpp
@@ -15,6 +15,7 @@
using ::testing::_;
using ::testing::DoAll;
+using ::testing::InSequence;
using ::testing::Return;
using ::testing::SaveArg;
@@ -26,6 +27,7 @@
MOCK_METHOD(bool, createFile,
(const std::string& path, const nlohmann::json& jsonPdr),
(const, override));
+ MOCK_METHOD(bool, removeAll, (const std::string& path), (const, override));
};
class ExternalStorerFileTest : public ::testing::Test
@@ -36,8 +38,9 @@
mockFileWriter(std::make_unique<MockFileWriter>())
{
mockFileWriterPtr = dynamic_cast<MockFileWriter*>(mockFileWriter.get());
+ // Set the queue of LogEntry to 1 saved entry and 2 non saved entry
exStorer = std::make_unique<ExternalStorerFileInterface>(
- conn, rootPath, std::move(mockFileWriter));
+ conn, rootPath, std::move(mockFileWriter), 1, 2);
}
protected:
@@ -127,6 +130,7 @@
TEST_F(ExternalStorerFileTest, LogEntryTest)
{
+ InSequence s;
// Before sending a LogEntry, first we need to push a LogService.
std::string jsonLogSerivce = R"(
{
@@ -146,19 +150,62 @@
.WillOnce(Return(true));
EXPECT_THAT(exStorer->publishJson(jsonLogSerivce), true);
- // Now send a LogEntry
+ // Now send a LogEntry#1, which will not be deleted
std::string jsonLogEntry = R"(
{
"@odata.id": "/some/odata/id",
"@odata.type": "#LogEntry.v1_13_0.LogEntry"
}
)";
-
nlohmann::json logEntryOut;
+ std::string logPath1;
EXPECT_CALL(*mockFileWriterPtr, createFile(_, _))
- .WillOnce(DoAll(SaveArg<1>(&logEntryOut), Return(true)));
-
+ .WillOnce(DoAll(SaveArg<0>(&logPath1), SaveArg<1>(&logEntryOut),
+ Return(true)));
EXPECT_THAT(exStorer->publishJson(jsonLogEntry), true);
+ EXPECT_FALSE(logPath1.empty());
+ EXPECT_NE(logEntryOut["Id"], nullptr);
+ EXPECT_EQ(logEntryOut["@odata.id"], nullptr);
+
+ // Now send a LogEntry#2, which will be the first to be deleted
+ std::string logPath2;
+ EXPECT_CALL(*mockFileWriterPtr, createFile(_, _))
+ .WillOnce(DoAll(SaveArg<0>(&logPath2), SaveArg<1>(&logEntryOut),
+ Return(true)));
+ EXPECT_THAT(exStorer->publishJson(jsonLogEntry), true);
+ EXPECT_FALSE(logPath2.empty());
+ EXPECT_NE(logEntryOut["Id"], nullptr);
+ EXPECT_EQ(logEntryOut["@odata.id"], nullptr);
+
+ // Now send a LogEntry#3
+ std::string logPath3;
+ EXPECT_CALL(*mockFileWriterPtr, createFile(_, _))
+ .WillOnce(DoAll(SaveArg<0>(&logPath3), SaveArg<1>(&logEntryOut),
+ Return(true)));
+ EXPECT_THAT(exStorer->publishJson(jsonLogEntry), true);
+ EXPECT_FALSE(logPath3.empty());
+ EXPECT_NE(logEntryOut["Id"], nullptr);
+ EXPECT_EQ(logEntryOut["@odata.id"], nullptr);
+
+ // Now send a LogEntry#4, we expect the LogEntry#2 to be deleted
+ std::string logPath4;
+ EXPECT_CALL(*mockFileWriterPtr, removeAll(logPath2)).WillOnce(Return(true));
+ EXPECT_CALL(*mockFileWriterPtr, createFile(_, _))
+ .WillOnce(DoAll(SaveArg<0>(&logPath4), SaveArg<1>(&logEntryOut),
+ Return(true)));
+ EXPECT_THAT(exStorer->publishJson(jsonLogEntry), true);
+ EXPECT_FALSE(logPath4.empty());
+ EXPECT_NE(logEntryOut["Id"], nullptr);
+ EXPECT_EQ(logEntryOut["@odata.id"], nullptr);
+
+ // Now send a LogEntry#5, we expect the LogEntry#3 to be deleted
+ std::string logPath5;
+ EXPECT_CALL(*mockFileWriterPtr, removeAll(logPath3)).WillOnce(Return(true));
+ EXPECT_CALL(*mockFileWriterPtr, createFile(_, _))
+ .WillOnce(DoAll(SaveArg<0>(&logPath5), SaveArg<1>(&logEntryOut),
+ Return(true)));
+ EXPECT_THAT(exStorer->publishJson(jsonLogEntry), true);
+ EXPECT_FALSE(logPath5.empty());
EXPECT_NE(logEntryOut["Id"], nullptr);
EXPECT_EQ(logEntryOut["@odata.id"], nullptr);
}