[#3513] address review

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

Author: andrei-pavel

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: