added Enabled to AddReportFutureVersion API
- New parameter added to API
- Changed API to be more flexible in the future
Tested:
- Unit tests are passing
- Tested together with:
https://gerrit.openbmc.org/c/openbmc/bmcweb/+/49796/23
POST, PATCH, PUT for MetricReportDefinitions are working correctly
Signed-off-by: Krzysztof Grobelny <krzysztof.grobelny@intel.com>
Change-Id: Ic988446d283a581dc6866418b1b27a989c3bc9a0
diff --git a/src/report.cpp b/src/report.cpp
index 9ee60a4..c2a0371 100644
--- a/src/report.cpp
+++ b/src/report.cpp
@@ -204,12 +204,11 @@
void Report::setReadingBuffer(const ReportUpdates newReportUpdates)
{
- if (reportingType != ReportingType::onRequest &&
- (reportUpdates == ReportUpdates::overwrite ||
- newReportUpdates == ReportUpdates::overwrite))
+ const auto newBufferSize =
+ deduceBufferSize(newReportUpdates, reportingType);
+ if (readingsBuffer.size() != newBufferSize)
{
- readingsBuffer.clearAndResize(
- deduceBufferSize(newReportUpdates, reportingType));
+ readingsBuffer.clearAndResize(newBufferSize);
}
}
diff --git a/src/report_manager.cpp b/src/report_manager.cpp
index 1677bb3..5812311 100644
--- a/src/report_manager.cpp
+++ b/src/report_manager.cpp
@@ -9,7 +9,9 @@
#include <phosphor-logging/log.hpp>
#include <sdbusplus/exception.hpp>
+#include <sdbusplus/unpack_properties.hpp>
+#include <optional>
#include <stdexcept>
#include <system_error>
@@ -99,24 +101,46 @@
"AddReportFutureVersion",
[this](
boost::asio::yield_context& yield,
- const std::string& reportId, const std::string& reportName,
- const std::string& reportingType,
- const std::string& reportUpdates,
- const uint64_t appendLimit,
- const std::vector<std::string>& reportActions,
- const uint64_t interval, ReadingParameters metricParams) {
- constexpr auto enabledDefault = true;
- return addReport(yield, reportId, reportName,
- utils::toReportingType(reportingType),
- utils::transform(
- reportActions,
- [](const auto& reportAction) {
- return utils::toReportAction(
- reportAction);
- }),
- Milliseconds(interval), appendLimit,
- utils::toReportUpdates(reportUpdates),
- std::move(metricParams), enabledDefault)
+ const std::vector<
+ std::pair<std::string, AddReportFutureVersionVariant>>&
+ properties) {
+ std::optional<std::string> reportId;
+ std::optional<std::string> reportName;
+ std::optional<std::string> reportingType;
+ std::optional<std::vector<std::string>> reportActions;
+ std::optional<uint64_t> interval;
+ std::optional<uint64_t> appendLimit;
+ std::optional<std::string> reportUpdates;
+ std::optional<ReadingParameters> metricParams;
+ std::optional<bool> enabled;
+
+ sdbusplus::unpackProperties(
+ properties, "Id", reportId, "Name", reportName,
+ "ReportingType", reportingType, "ReportActions",
+ reportActions, "Interval", interval, "AppendLimit",
+ appendLimit, "ReportUpdates", reportUpdates,
+ "MetricParams", metricParams, "Enabled", enabled);
+
+ return addReport(
+ yield, reportId.value_or(""),
+ reportName.value_or(""),
+ utils::toReportingType(
+ reportingType.value_or(utils::enumToString(
+ ReportingType::onRequest))),
+ utils::transform(
+ reportActions.value_or(
+ std::vector<std::string>{}),
+ [](const auto& reportAction) {
+ return utils::toReportAction(
+ reportAction);
+ }),
+ Milliseconds(interval.value_or(0)),
+ appendLimit.value_or(0),
+ utils::toReportUpdates(
+ reportUpdates.value_or(utils::enumToString(
+ ReportUpdates::overwrite))),
+ metricParams.value_or(ReadingParameters{}),
+ enabled.value_or(true))
.getPath();
});
});
diff --git a/src/types/report_types.hpp b/src/types/report_types.hpp
index d927d0e..49dca73 100644
--- a/src/types/report_types.hpp
+++ b/src/types/report_types.hpp
@@ -13,6 +13,7 @@
#include <string>
#include <tuple>
#include <type_traits>
+#include <variant>
#include <vector>
using ReadingParametersPastVersion =
@@ -30,5 +31,9 @@
utils::tstring::Id, utils::tstring::CollectionTimeScope,
utils::tstring::CollectionDuration>;
+using AddReportFutureVersionVariant =
+ std::variant<std::monostate, bool, uint64_t, std::string,
+ std::vector<std::string>, ReadingParameters>;
+
ReadingParameters
toReadingParameters(const std::vector<LabeledMetricParameters>& labeled);
diff --git a/src/types/trigger_types.hpp b/src/types/trigger_types.hpp
index 4d0b3bd..9a053c8 100644
--- a/src/types/trigger_types.hpp
+++ b/src/types/trigger_types.hpp
@@ -4,6 +4,7 @@
#include "utils/conversion.hpp"
#include "utils/labeled_tuple.hpp"
#include "utils/tstring.hpp"
+#include "utils/variant_utils.hpp"
#include <string>
#include <tuple>
@@ -144,8 +145,7 @@
std::vector<discrete::ThresholdParam>>;
using TriggerThresholdParams =
- std::variant<std::vector<numeric::ThresholdParam>,
- std::vector<discrete::ThresholdParam>>;
+ utils::WithoutMonostate<TriggerThresholdParamsExt>;
using LabeledTriggerThresholdParams =
std::variant<std::vector<numeric::LabeledThresholdParam>,
diff --git a/src/utils/circular_vector.hpp b/src/utils/circular_vector.hpp
index f7ac8e5..be13873 100644
--- a/src/utils/circular_vector.hpp
+++ b/src/utils/circular_vector.hpp
@@ -53,6 +53,11 @@
return vec.end();
}
+ size_t size() const
+ {
+ return maxSize;
+ }
+
private:
std::vector<T>& vec;
size_t maxSize = 0;
diff --git a/src/utils/variant_utils.hpp b/src/utils/variant_utils.hpp
new file mode 100644
index 0000000..0843818
--- /dev/null
+++ b/src/utils/variant_utils.hpp
@@ -0,0 +1,31 @@
+#pragma once
+
+#include <concepts>
+#include <variant>
+
+namespace utils
+{
+namespace details
+{
+
+template <class T, class... Args>
+auto removeMonostate(std::variant<T, Args...>) -> std::variant<T, Args...>;
+
+template <class... Args>
+auto removeMonostate(std::variant<std::monostate, Args...>)
+ -> std::variant<Args...>;
+
+template <class Variant>
+struct WithoutMonostate
+{
+ private:
+ public:
+ using type = decltype(removeMonostate(Variant{}));
+};
+
+} // namespace details
+
+template <class Variant>
+using WithoutMonostate = typename details::WithoutMonostate<Variant>::type;
+
+} // namespace utils
diff --git a/tests/src/test_report.cpp b/tests/src/test_report.cpp
index 8be6397..579c714 100644
--- a/tests/src/test_report.cpp
+++ b/tests/src/test_report.cpp
@@ -22,6 +22,8 @@
#include <sdbusplus/exception.hpp>
+#include <ranges>
+
using namespace testing;
using namespace std::literals::string_literals;
using namespace std::chrono_literals;
@@ -969,11 +971,30 @@
public TestReport,
public WithParamInterface<ReportUpdatesReportParams>
{
+ public:
void SetUp() override
+ {}
+
+ void changeReport(ReportingType rt, Milliseconds interval)
{
- sut = makeReport(ReportParams(GetParam().reportParams)
- .reportingType(ReportingType::periodic)
- .interval(std::chrono::hours(1000)));
+ setProperty<std::string>(sut->getPath(), "ReportingType",
+ utils::enumToString(rt));
+ setProperty<uint64_t>(sut->getPath(), "Interval", interval.count());
+ }
+
+ auto readings()
+ {
+ auto [timestamp, readings] =
+ getProperty<Readings>(sut->getPath(), "Readings");
+ return readings;
+ }
+
+ void updateReportFourTimes()
+ {
+ for (int i = 0; i < 4; i++)
+ {
+ messanger.send(messages::UpdateReportInd{{sut->getId()}});
+ }
}
};
@@ -1067,22 +1088,57 @@
std::vector<ReadingData>{
{std::make_tuple("a"s, "b"s, 17.1, 114u),
std::make_tuple("aa"s, "bb"s, 42.0, 74u)}},
- true}));
+ true},
+ ReportUpdatesReportParams{
+ defaultParams()
+ .reportUpdates(ReportUpdates::appendStopsWhenFull)
+ .appendLimit(std::numeric_limits<uint64_t>::max()),
+ std::vector<ReadingData>{
+ {std::make_tuple("a"s, "b"s, 17.1, 114u),
+ std::make_tuple("aa"s, "bb"s, 42.0, 74u)}},
+ false}));
TEST_P(TestReportWithReportUpdatesAndLimit,
readingsAreUpdatedAfterIntervalExpires)
{
- for (int i = 0; i < 4; i++)
- {
- messanger.send(messages::UpdateReportInd{{sut->getId()}});
- }
+ sut = makeReport(ReportParams(GetParam().reportParams)
+ .reportingType(ReportingType::periodic)
+ .interval(std::chrono::hours(1000)));
- const auto [timestamp, readings] =
- getProperty<Readings>(sut->getPath(), "Readings");
- const auto enabled = getProperty<bool>(sut->getPath(), "Enabled");
+ updateReportFourTimes();
- EXPECT_THAT(readings, ElementsAreArray(GetParam().expectedReadings));
- EXPECT_EQ(enabled, GetParam().expectedEnabled);
+ EXPECT_THAT(readings(), ElementsAreArray(GetParam().expectedReadings));
+ EXPECT_THAT(getProperty<bool>(sut->getPath(), "Enabled"),
+ Eq(GetParam().expectedEnabled));
+}
+
+TEST_P(TestReportWithReportUpdatesAndLimit,
+ appendLimitIsRespectedAfterChangingToPeriodic)
+{
+ sut = makeReport(ReportParams(GetParam().reportParams)
+ .reportingType(ReportingType::onRequest)
+ .interval(std::chrono::hours(0)));
+
+ changeReport(ReportingType::periodic, std::chrono::hours(1000));
+ updateReportFourTimes();
+
+ EXPECT_THAT(readings(), ElementsAreArray(GetParam().expectedReadings));
+ EXPECT_THAT(getProperty<bool>(sut->getPath(), "Enabled"),
+ Eq(GetParam().expectedEnabled));
+}
+
+TEST_P(TestReportWithReportUpdatesAndLimit,
+ appendLimitIsIgnoredAfterChangingToOnRequest)
+{
+ sut = makeReport(ReportParams(GetParam().reportParams)
+ .reportingType(ReportingType::periodic)
+ .interval(std::chrono::hours(1000)));
+
+ changeReport(ReportingType::onRequest, Milliseconds{0});
+ updateReportFourTimes();
+
+ EXPECT_THAT(readings(), SizeIs(2u));
+ EXPECT_THAT(getProperty<bool>(sut->getPath(), "Enabled"), Eq(true));
}
class TestReportInitialization : public TestReport
diff --git a/tests/src/test_report_manager.cpp b/tests/src/test_report_manager.cpp
index 6ce0f1f..f593fd4 100644
--- a/tests/src/test_report_manager.cpp
+++ b/tests/src/test_report_manager.cpp
@@ -12,11 +12,15 @@
#include "utils/string_utils.hpp"
#include "utils/transform.hpp"
#include "utils/tstring.hpp"
+#include "utils/variant_utils.hpp"
using namespace testing;
using namespace std::string_literals;
using namespace std::chrono_literals;
+using AddReportFutureVersionVariantForSet =
+ utils::WithoutMonostate<AddReportFutureVersionVariant>;
+
class TestReportManager : public Test
{
public:
@@ -53,10 +57,10 @@
DbusEnvironment::synchronizeIoc();
}
- template <class... Args>
- requires(sizeof...(Args) > 1)
- std::pair<boost::system::error_code, std::string> addReport(
- Args&&... args)
+ std::pair<boost::system::error_code, std::string>
+ addReport(const std::vector<
+ std::pair<std::string, AddReportFutureVersionVariantForSet>>&
+ properties)
{
std::promise<std::pair<boost::system::error_code, std::string>>
addReportPromise;
@@ -67,22 +71,33 @@
},
DbusEnvironment::serviceName(), ReportManager::reportManagerPath,
ReportManager::reportManagerIfaceName, "AddReportFutureVersion",
- std::forward<Args>(args)...);
+ properties);
return DbusEnvironment::waitForFuture(addReportPromise.get_future());
}
auto addReport(const ReportParams& params)
{
- return addReport(params.reportId(), params.reportName(),
- utils::enumToString(params.reportingType()),
- utils::enumToString(params.reportUpdates()),
- params.appendLimit(),
- utils::transform(params.reportActions(),
- [](const auto v) {
- return utils::enumToString(v);
- }),
- params.interval().count(),
- toReadingParameters(params.metricParameters()));
+ std::vector<std::pair<std::string, AddReportFutureVersionVariantForSet>>
+ properties;
+
+ properties.emplace_back("Id", params.reportId());
+ properties.emplace_back("Name", params.reportName());
+ properties.emplace_back("ReportingType",
+ utils::enumToString(params.reportingType()));
+ properties.emplace_back("ReportUpdates",
+ utils::enumToString(params.reportUpdates()));
+ properties.emplace_back("AppendLimit", params.appendLimit());
+ properties.emplace_back("Enabled", params.enabled());
+ properties.emplace_back(
+ "ReportActions",
+ utils::transform(params.reportActions(), [](const auto v) {
+ return utils::enumToString(v);
+ }));
+ properties.emplace_back("Interval", params.interval().count());
+ properties.emplace_back("MetricParams",
+ toReadingParameters(params.metricParameters()));
+
+ return addReport(properties);
}
template <class T>
@@ -124,6 +139,36 @@
EXPECT_THAT(path, Eq(reportMock.getPath()));
}
+TEST_F(TestReportManager, addDisabledReport)
+{
+ reportParams.enabled(false);
+
+ EXPECT_CALL(reportFactoryMock, convertMetricParams(_, _));
+ reportFactoryMock.expectMake(reportParams, Ref(*sut), Ref(storageMock))
+ .WillOnce(Return(ByMove(std::move(reportMockPtr))));
+
+ auto [ec, path] = addReport(reportParams);
+ EXPECT_THAT(ec.value(), Eq(boost::system::errc::success));
+ EXPECT_THAT(path, Eq(reportMock.getPath()));
+}
+
+TEST_F(TestReportManager, addReportWithOnlyDefaultParams)
+{
+ EXPECT_CALL(reportFactoryMock, convertMetricParams(_, _));
+ EXPECT_CALL(reportFactoryMock,
+ make("Report"s, "Report"s, ReportingType::onRequest,
+ std::vector<ReportAction>{}, Milliseconds{}, 0,
+ ReportUpdates::overwrite, _, _,
+ std::vector<LabeledMetricParameters>{}, true, Readings{}))
+ .WillOnce(Return(ByMove(std::move(reportMockPtr))));
+
+ auto [ec, path] = addReport(
+ std::vector<
+ std::pair<std::string, AddReportFutureVersionVariantForSet>>{});
+ EXPECT_THAT(ec.value(), Eq(boost::system::errc::success));
+ EXPECT_THAT(path, Eq(reportMock.getPath()));
+}
+
TEST_F(TestReportManager, addOnChangeReport)
{
EXPECT_CALL(reportFactoryMock, convertMetricParams(_, _));
@@ -350,12 +395,8 @@
reportFactoryMock.expectMake(std::nullopt, Ref(*sut), Ref(storageMock))
.Times(0);
- auto [ec, path] = addReport(
- reportParams.reportName(), "InvalidReportingType",
- utils::transform(reportParams.reportActions(),
- [](const auto v) { return utils::enumToString(v); }),
- reportParams.interval().count(),
- toReadingParameters(reportParams.metricParameters()));
+ auto [ec, path] = addReport({{"Name", reportParams.reportName()},
+ {"ReportingType", "InvalidReportingType"}});
EXPECT_THAT(ec.value(), Eq(boost::system::errc::invalid_argument));
EXPECT_THAT(path, Eq(std::string()));