Fix NTP set race condition
There's currently a problem with phosphor-timesyncd, where enabling NTP
doesn't immediately reflect in the system status on return[1]. To say
it another way, NTP is not enabled/disabled atomically, which leads to
the following problem.
// Disable NTP
PATCH /redfish/v1/Managers/bmc/NetworkProtocol
{"NTP":{"ProtocolEnabled": false}}
// Set the time manually
PATCH /redfish/v1/Managers/bmc {"DateTime": "<timestring"}
Doing this in rapid succession leads to a 500 error, which is obviously
a bug. In the prior commit, this error was changed to a
PropertyValueConflict error, which is still incorrect, but at least
informative of what's going on. REST APIs are intended to have CRUD
compliance. The response should not be returned until the value has
been accepted, and not doing that can lead to problems.
This commit changes the backend to use systemd directly, rather than
routing through phosphor-settings, to avoid this race.
Quite possibly resolves #264 but haven't tested that.
Tested: The above procedure succeeds.
[1] https://github.com/systemd/systemd/pull/11424
Change-Id: I19241e7677d9b6415aff79ac65c474ae71984417
Signed-off-by: Ed Tanous <ed@tanous.net>
diff --git a/redfish-core/lib/network_protocol.hpp b/redfish-core/lib/network_protocol.hpp
index 747abbf..adafcbb 100644
--- a/redfish-core/lib/network_protocol.hpp
+++ b/redfish-core/lib/network_protocol.hpp
@@ -238,25 +238,30 @@
std::bind_front(afterNetworkPortRequest, asyncResp));
} // namespace redfish
-inline void handleNTPProtocolEnabled(
- const bool& ntpEnabled, const std::shared_ptr<bmcweb::AsyncResp>& asyncResp)
+inline void afterSetNTP(const std::shared_ptr<bmcweb::AsyncResp>& asyncResp,
+ const boost::system::error_code& ec)
{
- std::string timeSyncMethod;
- if (ntpEnabled)
+ if (ec)
{
- timeSyncMethod = "xyz.openbmc_project.Time.Synchronization.Method.NTP";
+ BMCWEB_LOG_DEBUG("Failed to set elapsed time. DBUS response error {}",
+ ec);
+ messages::internalError(asyncResp->res);
+ return;
}
- else
- {
- timeSyncMethod =
- "xyz.openbmc_project.Time.Synchronization.Method.Manual";
- }
+ asyncResp->res.result(boost::beast::http::status::no_content);
+}
- setDbusProperty(asyncResp, "xyz.openbmc_project.Settings",
- sdbusplus::message::object_path(
- "/xyz/openbmc_project/time/sync_method"),
- "xyz.openbmc_project.Time.Synchronization",
- "TimeSyncMethod", "NTP/ProtocolEnabled", timeSyncMethod);
+inline void handleNTPProtocolEnabled(
+ const std::shared_ptr<bmcweb::AsyncResp>& asyncResp, bool ntpEnabled)
+{
+ bool interactive = false;
+ auto callback = [asyncResp](const boost::system::error_code& ec) {
+ afterSetNTP(asyncResp, ec);
+ };
+ crow::connections::systemBus->async_method_call(
+ std::move(callback), "org.freedesktop.timedate1",
+ "/org/freedesktop/timedate1", "org.freedesktop.timedate1", "SetNTP",
+ ntpEnabled, interactive);
}
// Redfish states that ip addresses can be
@@ -420,27 +425,18 @@
inline void
getNTPProtocolEnabled(const std::shared_ptr<bmcweb::AsyncResp>& asyncResp)
{
- sdbusplus::asio::getProperty<std::string>(
- *crow::connections::systemBus, "xyz.openbmc_project.Settings",
- "/xyz/openbmc_project/time/sync_method",
- "xyz.openbmc_project.Time.Synchronization", "TimeSyncMethod",
- [asyncResp](const boost::system::error_code& ec,
- const std::string& timeSyncMethod) {
+ sdbusplus::asio::getProperty<bool>(
+ *crow::connections::systemBus, "org.freedesktop.timedate1",
+ "/org/freedesktop/timedate1", "org.freedesktop.timedate1", "NTP",
+ [asyncResp](const boost::system::error_code& ec, bool enabled) {
if (ec)
{
+ BMCWEB_LOG_WARNING(
+ "Failed to get NTP status, assuming not supported");
return;
}
- if (timeSyncMethod ==
- "xyz.openbmc_project.Time.Synchronization.Method.NTP")
- {
- asyncResp->res.jsonValue["NTP"]["ProtocolEnabled"] = true;
- }
- else if (timeSyncMethod == "xyz.openbmc_project.Time.Synchronization."
- "Method.Manual")
- {
- asyncResp->res.jsonValue["NTP"]["ProtocolEnabled"] = false;
- }
+ asyncResp->res.jsonValue["NTP"]["ProtocolEnabled"] = enabled;
});
}
@@ -502,7 +498,7 @@
if (ntpEnabled)
{
- handleNTPProtocolEnabled(*ntpEnabled, asyncResp);
+ handleNTPProtocolEnabled(asyncResp, *ntpEnabled);
}
if (ntpServerObjects)
{