[#3513] address review

- fix documentation
- show null on system-time and clock-skew when uninitialized
- add UT CommunicationStateTest.getReportWithClockSkewTest

doc/sphinx/arm/hooks-ha.rst

@@ -2170,10 +2170,14 @@ server may start monitoring the DHCP traffic directed to the partner to see if
 the partner is responding to this traffic. More about the failover procedure can
 be found in :ref:`ha-load-balancing-config`.
 
-The ``system-time`` parameters hold the UTC time in ``%Y-%m-%d %H:%M:%S`` format
+The ``system-time`` parameter holds the UTC time when skew between local and
+partner node was last calculated. It is displayed in ``%Y-%m-%d %H:%M:%S`` format
 for each active node: local, and remote, respectively. The ``clock-skew``
 parameter is available in the ``remote`` map and holds the difference in seconds
-between the two times.
+between the two times. Local time is subtracted from the partner's time.
+A positive value means that the partner is ahead, while a negative value means
+that the partner is behind. Both ``system-time`` and ``clock-skew`` parameters
+can be null if the clock skew was not calculated yet.
 
 The ``connecting-clients``, ``unacked-clients``, ``unacked-clients-left``, and
 ``analyzed-packets`` parameters were introduced along with the

src/hooks/dhcp/high_availability/communication_state.cc

@@ -587,8 +587,13 @@ CommunicationState::getReport() const {
  }
  report->set("unacked-clients-left", Element::create(unacked_clients_left));
  report->set("analyzed-packets", Element::create(static_cast<long long>(getAnalyzedMessagesCount())));
- report->set("system-time", Element::create(ptimeToText(getPartnerTimeAtSkew(), 0)));
- report->set("clock-skew", Element::create(clock_skew_.total_seconds()));
+ if (partner_time_at_skew_.is_not_a_date_time()) {
+ report->set("system-time", Element::create());
+ report->set("clock-skew", Element::create());
+ } else {
+ report->set("system-time", Element::create(ptimeToText(partner_time_at_skew_, 0)));
+ report->set("clock-skew", Element::create(clock_skew_.total_seconds()));
+ }
 
  return (report);
 }
@@ -658,19 +663,11 @@ CommunicationState::setPartnerUnsentUpdateCountInternal(uint64_t unsent_update_c
 
 boost::posix_time::ptime
 CommunicationState::getMyTimeAtSkew() const {
- if (my_time_at_skew_.is_not_a_date_time()) {
- // Return current time.
- return boost::posix_time::microsec_clock::universal_time();
- }
  return my_time_at_skew_;
 }
 
 boost::posix_time::ptime
 CommunicationState::getPartnerTimeAtSkew() const {
- if (partner_time_at_skew_.is_not_a_date_time()) {
- // Return current time.
- return boost::posix_time::microsec_clock::universal_time();
- }
  return partner_time_at_skew_;
 }
 

src/hooks/dhcp/high_availability/communication_state.h

@@ -25,7 +25,6 @@
 #include <boost/shared_ptr.hpp>
 
 #include <functional>
-#include <map>
 #include <mutex>
 #include <set>
 #include <string>
@@ -701,17 +700,11 @@ class CommunicationState {
 public:
  /// @brief Retrieves the time of the local node when skew was last calculated.
  ///
- /// Used in reporting to the user, which is why being lenient with corner cases is important.
- /// That is why if the time was not initialized yet, it is approximated to the current time.
- ///
  /// @return my time at skew
  boost::posix_time::ptime getMyTimeAtSkew() const;
 
  /// @brief Retrieves the time of the partner node when skew was last calculated.
  ///
- /// Used in reporting to the user, which is why being lenient with corner cases is important.
- /// That is why if the time was not initialized yet, it is approximated to the current time.
- ///
  /// @return partner's time at skew
  boost::posix_time::ptime getPartnerTimeAtSkew() const;
 

src/hooks/dhcp/high_availability/ha_service.cc

@@ -1694,7 +1694,12 @@ HAService::processStatusGet() const {
  }
  local->set("scopes", list);
  local->set("server-name", Element::create(config_->getThisServerName()));
- local->set("system-time", Element::create(ptimeToText(communication_state_->getMyTimeAtSkew(), 0)));
+ auto const my_time(communication_state_->getMyTimeAtSkew());
+ if (my_time.is_not_a_date_time()) {
+ local->set("system-time", Element::create());
+ } else {
+ local->set("system-time", Element::create(ptimeToText(my_time, 0)));
+ }
  ha_servers->set("local", local);
 
  // Do not include remote server information if this is a backup server or

src/hooks/dhcp/high_availability/tests/communication_state_unittest.cc

@@ -38,6 +38,8 @@ using namespace isc::util;
 using namespace boost::posix_time;
 using namespace boost::gregorian;
 
+using namespace std;
+
 
 namespace {
 
@@ -129,6 +131,9 @@ class CommunicationStateTest : public HATest {
  /// @brief Tests unusual values used to create the report.
  void getReportDefaultValuesTest();
 
+ /// @brief Tests the report when clock is skewed.
+ void getReportWithClockSkewTest();
+
  /// @brief Tests that unsent updates count can be incremented and fetched.
  void getUnsentUpdateCountTest();
 
@@ -769,7 +774,7 @@ CommunicationStateTest::getReportTest() {
 
  // Check that system-time exists and that format is parsable by ptime.
  // Do not check exact value because it can be time-sensitive.
- checkThatTimeIsParsable(report);
+ checkThatTimeIsParsable(report, /* null_expected = */ true);
 
  // Compare with the expected output.
  std::string expected = "{"
@@ -782,7 +787,7 @@ CommunicationStateTest::getReportTest() {
  " \"unacked-clients\": 1,"
  " \"unacked-clients-left\": 10,"
  " \"analyzed-packets\": 2,"
- " \"clock-skew\": 0"
+ " \"clock-skew\": null"
  "}";
  expectEqWithDiff(Element::fromJSON(expected), report);
 }
@@ -796,7 +801,7 @@ CommunicationStateTest::getReportDefaultValuesTest() {
 
  // Check that system-time exists and that format is parsable by ptime.
  // Do not check exact value because it can be time-sensitive.
- checkThatTimeIsParsable(report);
+ checkThatTimeIsParsable(report, /* null_expected = */ true);
 
  // Compare with the expected output.
  std::string expected = "{"
@@ -809,11 +814,47 @@ CommunicationStateTest::getReportDefaultValuesTest() {
  " \"unacked-clients\": 0,"
  " \"unacked-clients-left\": 0,"
  " \"analyzed-packets\": 0,"
- " \"clock-skew\": 0"
+ " \"clock-skew\": null"
  "}";
  expectEqWithDiff(Element::fromJSON(expected), report);
 }
 
+// Tests that the communication state report is correct when clock is skewed.
+void
+CommunicationStateTest::getReportWithClockSkewTest() {
+ auto const now(microsec_clock::universal_time());
+ // RFC 1123 format
+ // Is freed automatically by std::locale. See [localization.locales.locale#6] and
+ // [localization.locales.locale.facet#2] in the C++ standard.
+ time_facet* facet(new time_facet("%a, %d %b %Y %H:%M:%S GMT"));
+ stringstream ss;
+ ss.imbue(std::locale(std::locale(), facet));
+ ss << now + seconds(2);
+ state_.setPartnerTime(ss.str());
+ ElementPtr report;
+ ASSERT_NO_THROW_LOG(report = state_.getReport());
+ ASSERT_TRUE(report);
+
+ // Check that system-time exists and that format is parsable by ptime.
+ // Do not check exact value because it can be time-sensitive.
+ checkThatTimeIsParsable(report, /* null_expected = */ false);
+
+ // Compare with the expected output.
+ std::string expected = R"({
+ "age": 0,
+ "in-touch": false,
+ "last-scopes": [ ],
+ "last-state": "",
+ "communication-interrupted": false,
+ "connecting-clients": 0,
+ "unacked-clients": 0,
+ "unacked-clients-left": 0,
+ "analyzed-packets": 0,
+ "clock-skew": 2
+ })";
+ expectEqWithDiff(Element::fromJSON(expected), report);
+}
+
 void
 CommunicationStateTest::getUnsentUpdateCountTest() {
  // Initially the count should be 0.
@@ -1221,6 +1262,15 @@ TEST_F(CommunicationStateTest, getReportDefaultValuesTestMultiThreading) {
  getReportDefaultValuesTest();
 }
 
+TEST_F(CommunicationStateTest, getReportWithClockSkewTest) {
+ getReportWithClockSkewTest();
+}
+
+TEST_F(CommunicationStateTest, getReportWithClockSkewTestMultiThreading) {
+ MultiThreadingMgr::instance().setMode(true);
+ getReportWithClockSkewTest();
+}
+
 TEST_F(CommunicationStateTest, getUnsentUpdateCountTest) {
  getUnsentUpdateCountTest();
 }

src/hooks/dhcp/high_availability/tests/ha_impl_unittest.cc

@@ -1809,8 +1809,8 @@ TEST_F(HAImplTest, statusGet) {
  EXPECT_TRUE(ha_servers);
  ElementPtr local(boost::const_pointer_cast<Element>(ha_servers->get("local")));
  ElementPtr remote(boost::const_pointer_cast<Element>(ha_servers->get("remote")));
- checkThatTimeIsParsable(local);
- checkThatTimeIsParsable(remote);
+ checkThatTimeIsParsable(local, /* null_expected = */ true);
+ checkThatTimeIsParsable(remote, /* null_expected = */ true);
 
  std::string expected =
  "{"
@@ -1837,7 +1837,7 @@ TEST_F(HAImplTest, statusGet) {
  " \"unacked-clients\": 0,"
  " \"unacked-clients-left\": 0,"
  " \"analyzed-packets\": 0,"
- " \"clock-skew\": 0"
+ " \"clock-skew\": null"
  " }"
  " }"
  " }"
@@ -1882,7 +1882,7 @@ TEST_F(HAImplTest, statusGetBackupServer) {
  got->get("arguments")->get("high-availability")->get(0)->get("ha-servers"));
  EXPECT_TRUE(ha_servers);
  ElementPtr local(boost::const_pointer_cast<Element>(ha_servers->get("local")));
- checkThatTimeIsParsable(local);
+ checkThatTimeIsParsable(local, /* null_expected = */ true);
 
  std::string expected =
  "{"
@@ -1940,7 +1940,7 @@ TEST_F(HAImplTest, statusGetPassiveBackup) {
  got->get("arguments")->get("high-availability")->get(0)->get("ha-servers"));
  EXPECT_TRUE(ha_servers);
  ElementPtr local(boost::const_pointer_cast<Element>(ha_servers->get("local")));
- checkThatTimeIsParsable(local);
+ checkThatTimeIsParsable(local, /* null_expected = */ true);
 
  std::string expected =
  "{"
@@ -2000,8 +2000,8 @@ TEST_F(HAImplTest, statusGetHubAndSpoke) {
  EXPECT_TRUE(ha_servers);
  ElementPtr local(boost::const_pointer_cast<Element>(ha_servers->get("local")));
  ElementPtr remote(boost::const_pointer_cast<Element>(ha_servers->get("remote")));
- checkThatTimeIsParsable(local);
- checkThatTimeIsParsable(remote);
+ checkThatTimeIsParsable(local, /* null_expected = */ true);
+ checkThatTimeIsParsable(remote, /* null_expected = */ true);
  }
 
  std::string expected =
@@ -2020,7 +2020,7 @@ TEST_F(HAImplTest, statusGetHubAndSpoke) {
  " \"remote\": {"
  " \"age\": 0,"
  " \"analyzed-packets\": 0,"
- " \"clock-skew\": 0,"
+ " \"clock-skew\": null,"
  " \"communication-interrupted\": false,"
  " \"connecting-clients\": 0,"
  " \"in-touch\": false,"
@@ -2045,7 +2045,7 @@ TEST_F(HAImplTest, statusGetHubAndSpoke) {
  " \"remote\": {"
  " \"age\": 0,"
  " \"analyzed-packets\": 0,"
- " \"clock-skew\": 0,"
+ " \"clock-skew\": null,"
  " \"communication-interrupted\": false,"
  " \"connecting-clients\": 0,"
  " \"in-touch\": false,"

src/hooks/dhcp/high_availability/tests/ha_service_unittest.cc

@@ -2639,8 +2639,8 @@ TEST_F(HAServiceTest, hotStandbyScopeSelectionThisPrimary) {
  // Do not check exact value because it can be time-sensitive.
  ElementPtr local(boost::const_pointer_cast<Element>(ha_servers->get("local")));
  ElementPtr remote(boost::const_pointer_cast<Element>(ha_servers->get("remote")));
- checkThatTimeIsParsable(local);
- checkThatTimeIsParsable(remote);
+ checkThatTimeIsParsable(local, /* null_expected = */ true);
+ checkThatTimeIsParsable(remote, /* null_expected = */ true);
 
  std::string expected = "{"
  " \"local\": {"
@@ -2661,7 +2661,7 @@ TEST_F(HAServiceTest, hotStandbyScopeSelectionThisPrimary) {
  " \"unacked-clients\": 0,"
  " \"unacked-clients-left\": 0,"
  " \"analyzed-packets\": 0,"
- " \"clock-skew\": 0"
+ " \"clock-skew\": null"
  " }"
  "}";
  expectEqWithDiff(Element::fromJSON(expected), ha_servers);
@@ -2698,8 +2698,8 @@ TEST_F(HAServiceTest, hotStandbyScopeSelectionThisStandby) {
  // Do not check exact value because it can be time-sensitive.
  ElementPtr local(boost::const_pointer_cast<Element>(ha_servers->get("local")));
  ElementPtr remote(boost::const_pointer_cast<Element>(ha_servers->get("remote")));
- checkThatTimeIsParsable(local);
- checkThatTimeIsParsable(remote);
+ checkThatTimeIsParsable(local, /* null_expected = */ true);
+ checkThatTimeIsParsable(remote, /* null_expected = */ true);
 
  std::string expected = "{"
  " \"local\": {"
@@ -2720,7 +2720,7 @@ TEST_F(HAServiceTest, hotStandbyScopeSelectionThisStandby) {
  " \"unacked-clients\": 0,"
  " \"unacked-clients-left\": 0,"
  " \"analyzed-packets\": 0,"
- " \"clock-skew\": 0"
+ " \"clock-skew\": null"
  " }"
  "}";
  expectEqWithDiff(Element::fromJSON(expected), ha_servers);
@@ -6500,7 +6500,7 @@ class HAServiceStateMachineTest : public HAServiceTest {
 // is available and it is doing load balancing.
 // 10. I stay in the partner-down state to force the partner to transition
 // to the waiting state and synchronize its database.
-TEST_F(HAServiceStateMachineTest, waitingParterDownLoadBalancingPartnerDown) {
+TEST_F(HAServiceStateMachineTest, waitingPartnerDownLoadBalancingPartnerDown) {
  HAConfigPtr config_storage = createValidConfiguration();
  // Disable syncing leases to avoid transitions via the syncing state.
  // In this state it is necessary to perform synchronous tasks.
@@ -6586,8 +6586,8 @@ TEST_F(HAServiceStateMachineTest, waitingParterDownLoadBalancingPartnerDown) {
  // Do not check exact value because it can be time-sensitive.
  ElementPtr mutable_local(boost::const_pointer_cast<Element>(ha_servers->get("local")));
  ElementPtr mutable_remote(boost::const_pointer_cast<Element>(ha_servers->get("remote")));
- checkThatTimeIsParsable(mutable_local);
- checkThatTimeIsParsable(mutable_remote);
+ checkThatTimeIsParsable(mutable_local, /* null_expected = */ false);
+ checkThatTimeIsParsable(mutable_remote, /* null_expected = */ false);
 
  std::string expected = "{"
  " \"local\": {"
@@ -6685,7 +6685,7 @@ TEST_F(HAServiceStateMachineTest, waitingParterDownLoadBalancingPartnerDown) {
 // 10. The partner unexpectedly shows up in the hot-standby mode. I stay in
 // the partner-down state to force the partner to transition to the waiting
 // state and synchronize its database.
-TEST_F(HAServiceStateMachineTest, waitingParterDownHotStandbyPartnerDown) {
+TEST_F(HAServiceStateMachineTest, waitingPartnerDownHotStandbyPartnerDown) {
  HAConfigPtr valid_config = createValidConfiguration(HAConfig::HOT_STANDBY);
 
  // Turn it into hot-standby configuration.

src/hooks/dhcp/high_availability/tests/ha_test.cc

@@ -149,7 +149,7 @@ HATest::signalServiceRunning(bool& running, std::mutex& mutex,
 
 
 void
-HATest::checkThatTimeIsParsable(ElementPtr node) {
+HATest::checkThatTimeIsParsable(ElementPtr const& node, bool const null_expected) {
  ConstElementPtr system_time(node->get("system-time"));
  EXPECT_TRUE(system_time);
 
@@ -160,16 +160,20 @@ HATest::checkThatTimeIsParsable(ElementPtr node) {
 
  stringstream ss;
  ss.imbue(std::locale(std::locale(), facet));
- EXPECT_EQ(system_time->getType(), Element::string);
- ss << system_time->stringValue();
- boost::posix_time::ptime t;
- ss >> t;
-
- // Reset stringstream.
- ss = stringstream();
-
- ss << t;
- EXPECT_NE(ss.str(), "not-a-date-time");
+ if (null_expected) {
+ EXPECT_EQ(system_time->getType(), Element::null);
+ } else {
+ EXPECT_EQ(system_time->getType(), Element::string);
+ ss << system_time->stringValue();
+ boost::posix_time::ptime t;
+ ss >> t;
+
+ // Reset stringstream.
+ ss = stringstream();
+
+ ss << t;
+ EXPECT_NE(ss.str(), "not-a-date-time");
+ }
 
  node->remove("system-time");
 }

src/hooks/dhcp/high_availability/tests/ha_test.h

@@ -130,14 +130,15 @@ class HATest : public ::testing::Test {
  std::condition_variable& condvar);
 
  /// @brief Check that a map element pointer representing the reported status
- /// of an HA node contains string element pointer indexed by the
+ /// of an HA node contains a string element pointer indexed by the
  /// "system-time" key that can be parsed in a ptime object.
  ///
  /// Also removes the "system-time" element for the purpose of holistically
  /// comparing the node without worrying about time-sensitive information.
  ///
- /// @brief param the node element pointer
- void checkThatTimeIsParsable(isc::data::ElementPtr node);
+ /// @param node the node element pointer
+ /// @param null_expected whether null is expected as the system-time value
+ void checkThatTimeIsParsable(isc::data::ElementPtr const& node, bool const null_expected);
 
 public: