Browse Source

[master] Merge branch 'trac3794' (packet statistics in DHCPv4)

Tomek Mrugalski 10 years ago
parent
commit
a61f40c449

+ 1 - 1
doc/guide/Makefile.am

@@ -7,7 +7,7 @@ dist_html_DATA = $(HTMLDOCS) kea-guide.css
 
 DOCBOOK = kea-guide.xml intro.xml quickstart.xml install.xml admin.xml config.xml
 DOCBOOK += keactrl.xml dhcp4-srv.xml dhcp6-srv.xml logging.xml ddns.xml hooks.xml
-DOCBOOK += libdhcp.xml lfc.xml
+DOCBOOK += libdhcp.xml lfc.xml stats.xml
 
 EXTRA_DIST = $(DOCBOOK)
 DISTCLEANFILES = $(HTMLDOCS) $(DOCS) kea-messages.xml

+ 210 - 0
doc/guide/dhcp4-srv.xml

@@ -2582,6 +2582,216 @@ temporarily override a list of interface names and listen on all interfaces.
 
     </section>
 
+    <section id="dhcp4-stats">
+      <title>Statistics in DHCPv4 server</title>
+      <note>
+        <para>This section describes DHCPv4-specific statistics. For a general
+        overview and usage of statistics, see <xref linkend="stats" />.</para>
+      </note>
+
+      <para>
+        The DHCPv4 server supports the following statistics:
+      </para>
+        <table frame="all" id="dhcp4-statistics">
+          <title>DHCPv4 Statistics</title>
+          <tgroup cols='3'>
+          <colspec colname='statistic' align='center'/>
+          <colspec colname='type' align='center'/>
+          <colspec colname='description' align='left'/>
+          <thead>
+            <row>
+              <entry>Statistic</entry>
+              <entry>Data Type</entry>
+              <entry>Description</entry>
+            </row>
+          </thead>
+          <tbody>
+
+            <row>
+            <entry>pkt4-received</entry>
+            <entry>integer</entry>
+            <entry>
+            Number of packets received. This includes all packets: valid, bogus, corrupted,
+            rejected etc.  This statistic is expected to grow rapidly.
+            </entry>
+            </row>
+            <row>
+
+            <entry>pkt4-discover-received</entry>
+            <entry>integer</entry>
+            <entry>
+            Number of DHCPDISCOVER packets received. This statistic is expected to grow.
+            Its increase means that clients that just booted started their configuration process
+            and their initial packets reached your server.
+            </entry>
+            </row>
+
+            <row>
+            <entry>pkt4-offer-received</entry>
+            <entry>integer</entry>
+            <entry>
+            Number of DHCPOFFER packets received. This statistic
+            is expected to remain zero at all times, as DHCPOFFER packets are sent
+            by the server and the server is never expected to receive them. Non-zero
+            value indicates an error. One likely cause would be a misbehaving relay
+            agent that incorrectly forwards DHCPOFFER messages towards the server,
+            rather back to the clients.
+            </entry>
+            </row>
+
+            <row>
+            <entry>pkt4-request-received</entry>
+            <entry>integer</entry>
+            <entry>
+            Number of DHCPREQUEST packets received. This statistic
+            is expected to grow. Its increase means that clients that just booted
+            received server's response (DHCPOFFER), accepted it and now requesting
+            an address (DHCPREQUEST).
+            </entry>
+            </row>
+
+            <row>
+            <entry>pkt4-ack-received</entry>
+            <entry>integer</entry>
+            <entry>
+            Number of DHCPACK packets received. This statistic
+            is expected to remain zero at all times, as DHCPACK packets are sent
+            by the server and the server is never expected to receive them. Non-zero
+            value indicates an error. One likely cause would be a misbehaving relay
+            agent that incorrectly forwards DHCPACK messages towards the server,
+            rather back to the clients.
+            </entry>
+            </row>
+
+            <row>
+            <entry>pkt4-nak-received</entry>
+            <entry>integer</entry>
+            <entry>
+            Number of DHCPNAK packets received. This statistic
+            is expected to remain zero at all times, as DHCPNAK packets are sent
+            by the server and the server is never expected to receive them. Non-zero
+            value indicates an error. One likely cause would be a misbehaving relay
+            agent that incorrectly forwards DHCPNAK messages towards the server,
+            rather back to the clients.
+            </entry>
+            </row>
+
+            <row>
+            <entry>pkt4-release-received</entry>
+            <entry>integer</entry>
+            <entry>
+            Number of DHCPRELEASE packets received. This statistic
+            is expected to grow. Its increase means that clients that had an address
+            are shutting down or stop using their addresses.
+            </entry>
+            </row>
+
+            <row>
+            <entry>pkt4-decline-received</entry>
+            <entry>integer</entry>
+            <entry>
+            Number of DHCPDECLINE packets received. This statistic
+            is expected to remain close to zero. Its increase means that a client
+            that leased an address, but discovered that the address is currently
+            used by an unknown device in your network.
+            </entry>
+            </row>
+
+            <row>
+            <entry>pkt4-inform-received</entry>
+            <entry>integer</entry>
+            <entry>
+            Number of DHCPINFORM packets received. This statistic
+            is expected to grow. Its increase means that there are clients that
+            either do not need an address or already have an address and are
+            interested only in getting additional configuration parameters.
+            </entry>
+            </row>
+
+            <row>
+            <entry>pkt4-unknown-received</entry>
+            <entry>integer</entry>
+            <entry>
+            Number of packets received of an unknown type. Non-zero
+            value of this statistic indicates that the server received a packet
+            that it wasn't able to recognize: either with unsupported type
+            or possibly malformed (without message type option).
+            </entry>
+            </row>
+
+            <row>
+            <entry>pkt4-sent</entry>
+            <entry>integer</entry>
+            <entry>
+            Number of DHCPv4 packets sent. This statistic is expected to grow
+            every time the server transmits a packet. In general, it should
+            roughly match pkt4-received, as most incoming packets cause
+            server to respond. There are exceptions (e.g. DHCPRELEASE), so
+            do not worry, if it is lesser than pkt4-received.
+            </entry>
+            </row>
+
+            <row>
+            <entry>pkt4-offer-sent</entry>
+            <entry>integer</entry>
+            <entry>
+            Number of DHCPOFFER packets sent. This statistic is expected to
+            grow in most cases after a DHCPDISCOVER is processed. There are
+            certain uncommon, but valid cases where incoming DHCPDISCOVER is
+            dropped, but in general this statistic is expected to be close to
+            pkt4-discover-received.
+            </entry>
+            </row>
+
+            <row>
+            <entry>pkt4-ack-sent</entry>
+            <entry>integer</entry>
+            <entry>
+            Number of DHCPACK packets sent. This statistic is expected to
+            grow in most cases after a DHCPREQUEST is processed. There are
+            certain cases where DHCPNAK is sent instead. In general, the sum of
+            pkt4-ack-sent and pkt4-nak-sent should be close to
+            pkt4-request-received.
+            </entry>
+            </row>
+
+            <row>
+            <entry>pkt4-nak-sent</entry>
+            <entry>integer</entry>
+            <entry>
+            Number of DHCPNAK packets sent. This statistic is expected to
+            grow when the server choses to not honor the address requested by a
+            client. In general, the sum of pkt4-ack-sent and pkt4-nak-sent
+            should be close to pkt4-request-received.
+            </entry>
+            </row>
+
+            <row>
+            <entry>pkt4-parse-failed</entry>
+            <entry>integer</entry>
+            <entry>
+            Number of incoming packets that could not be parsed.  Non-zero value of
+            this statistic indicates that the server received malformed or truncated packet.
+            This may indicate problems in your network, faulty clients or server code bug.
+            </entry>
+            </row>
+
+            <row>
+            <entry>pkt4-receive-drop</entry>
+            <entry>integer</entry>
+            <entry>
+            Number of incoming packets that were dropped.
+            Exact reason for dropping packets is logged, but the most common
+            reasons may be: an unacceptable packet type, direct responses are
+            forbidden, or the server-id sent by the client does not match
+            the server's server-id.
+            </entry>
+            </row>
+
+        </tbody>
+        </tgroup>
+        </table>
+    </section>
 
     <section id="dhcp4-std">
       <title>Supported DHCP Standards</title>

+ 2 - 0
doc/guide/kea-guide.xml

@@ -73,6 +73,8 @@
 
   <xi:include xmlns:xi="http://www.w3.org/2001/XInclude" href="hooks.xml" />
 
+  <xi:include xmlns:xi="http://www.w3.org/2001/XInclude" href="stats.xml" />
+
   <xi:include xmlns:xi="http://www.w3.org/2001/XInclude" href="libdhcp.xml" />
 
   <xi:include xmlns:xi="http://www.w3.org/2001/XInclude" href="logging.xml" />

+ 33 - 0
doc/guide/stats.xml

@@ -0,0 +1,33 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!DOCTYPE book PUBLIC "-//OASIS//DTD DocBook XML V4.2//EN"
+"http://www.oasis-open.org/docbook/xml/4.2/docbookx.dtd" [
+<!ENTITY mdash  "&#x2014;" >
+]>
+
+  <!-- Note: Please use the following terminology:
+       - daemon - one process (e.g. kea-dhcp4)
+       - component - one piece of code within a daemon (e.g. libdhcp or hooks)
+       - server - currently equal to daemon, but the difference will be more
+                  prominent once we add client or relay support
+       - logger - one instance of isc::log::Logger
+       - structure - an element in config file (e.g. "Dhcp4")
+
+       Do not use:
+       - module => daemon
+    -->
+
+<chapter id="stats">
+  <title>Statistics</title>
+
+  <section>
+    <title>Statistics Overview</title>
+    
+    <para>
+      TODO: Describe statistics collection here as part of ticket #3800.
+      For DHCPv4 specific statistics, see <xref linkend="dhcp4-stats"/>.
+      For DHCPv6 specific statistics, see TODO.
+      For DDNS specific statistics, see TODO.
+    </para>
+  </section>
+</chapter>
+

+ 1 - 0
src/bin/dhcp4/Makefile.am

@@ -82,6 +82,7 @@ kea_dhcp4_LDADD += $(top_builddir)/src/lib/log/libkea-log.la
 kea_dhcp4_LDADD += $(top_builddir)/src/lib/config/libkea-cfgclient.la
 kea_dhcp4_LDADD += $(top_builddir)/src/lib/cc/libkea-cc.la
 kea_dhcp4_LDADD += $(top_builddir)/src/lib/hooks/libkea-hooks.la
+kea_dhcp4_LDADD += $(top_builddir)/src/lib/stats/libkea-stats.la
 
 kea_dhcp4dir = $(pkgdatadir)
 kea_dhcp4_DATA = dhcp4.spec

+ 104 - 1
src/bin/dhcp4/dhcp4_srv.cc

@@ -41,6 +41,7 @@
 #include <hooks/hooks_log.h>
 #include <hooks/hooks_manager.h>
 #include <util/strutil.h>
+#include <stats/stats_mgr.h>
 
 #include <asio.hpp>
 #include <boost/bind.hpp>
@@ -412,6 +413,13 @@ Dhcpv4Srv::run() {
             continue;
         }
 
+        // Log reception of the packet. We need to increase it early, as any
+        // failures in unpacking will cause the packet to be dropped. We
+        // will increase type specific packets further down the road.
+        // See processStatsReceived().
+        isc::stats::StatsMgr::instance().addValue("pkt4-received",
+                                                  static_cast<uint64_t>(1));
+
         // In order to parse the DHCP options, the server needs to use some
         // configuration information such as: existing option spaces, option
         // definitions etc. This is the kind of information which is not
@@ -470,10 +478,19 @@ Dhcpv4Srv::run() {
                     .arg(query->getLocalAddr().toText())
                     .arg(query->getIface())
                     .arg(e.what());
+
+                // Increase the statistics of parse failues and dropped packets.
+                isc::stats::StatsMgr::instance().addValue("pkt4-parse-failed",
+                                                          static_cast<uint64_t>(1));
+                isc::stats::StatsMgr::instance().addValue("pkt4-receive-drop",
+                                                          static_cast<uint64_t>(1));
                 continue;
             }
         }
 
+        // Update statistics accordingly for received packet.
+        processStatsReceived(query);
+
         // Assign this packet to one or more classes if needed. We need to do
         // this before calling accept(), because getSubnet4() may need client
         // class information.
@@ -482,6 +499,9 @@ Dhcpv4Srv::run() {
         // Check whether the message should be further processed or discarded.
         // There is no need to log anything here. This function logs by itself.
         if (!accept(query)) {
+            // Increase the statistic of dropped packets.
+            isc::stats::StatsMgr::instance().addValue("pkt4-receive-drop",
+                                                      static_cast<uint64_t>(1));
             continue;
         }
 
@@ -568,6 +588,10 @@ Dhcpv4Srv::run() {
                       DHCP4_PACKET_DROP_0007)
                 .arg(query->getLabel())
                 .arg(e.what());
+
+            // Increase the statistic of dropped packets.
+            isc::stats::StatsMgr::instance().addValue("pkt4-receive-drop",
+                                                      static_cast<uint64_t>(1));
         }
 
         if (!rsp) {
@@ -665,6 +689,10 @@ Dhcpv4Srv::run() {
                 .arg(static_cast<int>(rsp->getType()))
                 .arg(rsp->toText());
             sendPacket(rsp);
+
+            // Update statistics accordingly for sent packet.
+            processStatsSent(rsp);
+
         } catch (const std::exception& e) {
             LOG_ERROR(packet_logger, DHCP4_PACKET_SEND_FAIL)
                 .arg(rsp->getLabel())
@@ -1045,7 +1073,7 @@ Dhcpv4Srv::processHostnameOption(Dhcpv4Exchange& ex) {
         // clients do not handle the hostnames with the trailing dot.
         opt_hostname_resp->setValue(d2_mgr.qualifyName(hostname, false));
     }
-    
+
     LOG_DEBUG(ddns_logger, DBG_DHCP4_DETAIL_DATA, DHCP4_RESPONSE_HOSTNAME_DATA)
         .arg(ex.getQuery()->getLabel())
         .arg(opt_hostname_resp->getValue());
@@ -2235,5 +2263,80 @@ Daemon::getVersion(bool extended) {
     return (tmp.str());
 }
 
+void Dhcpv4Srv::processStatsReceived(const Pkt4Ptr& query) {
+    // Note that we're not bumping pkt4-received statistic as it was
+    // increased early in the packet reception code.
+
+    string stat_name = "pkt4-unknown-received";
+    try {
+        switch (query->getType()) {
+        case DHCPDISCOVER:
+            stat_name = "pkt4-discover-received";
+            break;
+        case DHCPOFFER:
+            // Should not happen, but let's keep a counter for it
+            stat_name = "pkt4-offer-received";
+            break;
+        case DHCPREQUEST:
+            stat_name = "pkt4-request-received";
+            break;
+        case DHCPACK:
+            // Should not happen, but let's keep a counter for it
+            stat_name = "pkt4-ack-received";
+            break;
+        case DHCPNAK:
+            // Should not happen, but let's keep a counter for it
+            stat_name = "pkt4-nak-received";
+            break;
+        case DHCPRELEASE:
+            stat_name = "pkt4-release-received";
+        break;
+        case DHCPDECLINE:
+            stat_name = "pkt4-decline-received";
+            break;
+        case DHCPINFORM:
+            stat_name = "pkt4-inform-received";
+            break;
+        default:
+            ; // do nothing
+        }
+    }
+    catch (...) {
+        // If the incoming packet doesn't have option 53 (message type)
+        // or a hook set pkt4_receive_skip, then Pkt4::getType() may
+        // throw an exception. That's ok, we'll then use the default
+        // name of pkt4-unknown-received.
+    }
+
+    isc::stats::StatsMgr::instance().addValue(stat_name,
+                                              static_cast<uint64_t>(1));
+}
+
+void Dhcpv4Srv::processStatsSent(const Pkt4Ptr& response) {
+    // Increase generic counter for sent packets.
+    isc::stats::StatsMgr::instance().addValue("pkt4-sent",
+                                              static_cast<uint64_t>(1));
+
+    // Increase packet type specific counter for packets sent.
+    string stat_name;
+    switch (response->getType()) {
+    case DHCPOFFER:
+        stat_name = "pkt4-offer-sent";
+        break;
+    case DHCPACK:
+        stat_name = "pkt4-ack-sent";
+        break;
+    case DHCPNAK:
+        stat_name = "pkt4-nak-sent";
+        break;
+    default:
+        // That should never happen
+        return;
+    }
+
+    isc::stats::StatsMgr::instance().addValue(stat_name,
+                                              static_cast<uint64_t>(1));
+}
+
 }   // namespace dhcp
 }   // namespace isc

+ 7 - 0
src/bin/dhcp4/dhcp4_srv.h

@@ -738,6 +738,13 @@ private:
     /// @return Option that contains netmask information
     static OptionPtr getNetmaskOption(const Subnet4Ptr& subnet);
 
+    /// @brief Updates statistics for received packets
+    /// @param query packet received
+    static void processStatsReceived(const Pkt4Ptr& query);
+
+    /// @brief Updates statistics for transmitted packets
+    /// @param query packet transmitted
+    static void processStatsSent(const Pkt4Ptr& response);
 
     uint16_t port_;  ///< UDP port number on which server listens.
     bool use_bcast_; ///< Should broadcast be enabled on sockets (if true).

+ 1 - 0
src/bin/dhcp4/tests/Makefile.am

@@ -116,6 +116,7 @@ dhcp4_unittests_LDADD += $(top_builddir)/src/lib/exceptions/libkea-exceptions.la
 dhcp4_unittests_LDADD += $(top_builddir)/src/lib/log/libkea-log.la
 dhcp4_unittests_LDADD += $(top_builddir)/src/lib/util/libkea-util.la
 dhcp4_unittests_LDADD += $(top_builddir)/src/lib/hooks/libkea-hooks.la
+dhcp4_unittests_LDADD += $(top_builddir)/src/lib/stats/libkea-stats.la
 dhcp4_unittests_LDADD += $(top_builddir)/src/lib/dhcpsrv/testutils/libdhcpsrvtest.la
 dhcp4_unittests_LDADD += $(top_builddir)/src/lib/util/io/libkea-util-io.la
 endif

+ 13 - 5
src/bin/dhcp4/tests/dhcp4_client.cc

@@ -142,6 +142,8 @@ Dhcp4Client::applyConfiguration() {
         return;
     }
 
+    // Let's keep the old lease in case this is a response to Inform.
+    Lease4 old_lease = config_.lease_;
     config_.reset();
 
     // Routers
@@ -175,11 +177,17 @@ Dhcp4Client::applyConfiguration() {
         config_.serverid_ = opt_serverid->readAddress();
     }
 
-    /// @todo Set the valid lifetime, t1, t2 etc.
-    config_.lease_ = Lease4(IOAddress(context_.response_->getYiaddr()),
-                            context_.response_->getHWAddr(),
-                            0, 0, 0, 0, 0, time(NULL), 0, false, false,
-                            "");
+    // If the message sent was Inform, we don't want to throw
+    // away the old lease info, just the bits about options.
+    if (context_.query_->getType() == DHCPINFORM) {
+        config_.lease_ = old_lease;
+    } else {
+        /// @todo Set the valid lifetime, t1, t2 etc.
+        config_.lease_ = Lease4(IOAddress(context_.response_->getYiaddr()),
+                                context_.response_->getHWAddr(),
+                                0, 0, 0, 0, 0, time(NULL), 0, false, false,
+                                "");
+    }
 }
 
 void

+ 79 - 16
src/bin/dhcp4/tests/dhcp4_srv_unittest.cc

@@ -43,6 +43,7 @@
 #include <hooks/server_hooks.h>
 #include <hooks/hooks_manager.h>
 #include <config/ccsession.h>
+#include <stats/stats_mgr.h>
 
 #include <boost/scoped_ptr.hpp>
 
@@ -61,6 +62,26 @@ using namespace isc::test;
 
 namespace {
 
+const char* CONFIGS[] = {
+    // Configuration 0:
+    // - 1 subnet: 10.254.226.0/25
+    // - used for recorded traffic (see PktCaptures::captureRelayedDiscover)
+    "{ \"interfaces-config\": {"
+        "    \"interfaces\": [ \"*\" ]"
+        "},"
+        "\"rebind-timer\": 2000, "
+        "\"renew-timer\": 1000, "
+        "\"subnet4\": [ { "
+        "    \"pools\": [ { \"pool\": \"10.254.226.0/25\" } ],"
+        "    \"subnet\": \"10.254.226.0/24\", "
+        "    \"rebind-timer\": 2000, "
+        "    \"renew-timer\": 1000, "
+        "    \"valid-lifetime\": 4000,"
+        "    \"interface\": \"eth0\" "
+        " } ],"
+        "\"valid-lifetime\": 4000 }"
+};
+
 // This test verifies that the destination address of the response
 // message is set to giaddr, when giaddr is set to non-zero address
 // in the received message.
@@ -922,22 +943,7 @@ TEST_F(Dhcpv4SrvTest, relayAgentInfoEcho) {
     // subnet 10.254.226.0/24 is in use, because this packet
     // contains the giaddr which belongs to this subnet and
     // this giaddr is used to select the subnet
-    std::string config = "{ \"interfaces-config\": {"
-        "    \"interfaces\": [ \"*\" ]"
-        "},"
-        "\"rebind-timer\": 2000, "
-        "\"renew-timer\": 1000, "
-        "\"subnet4\": [ { "
-        "    \"pools\": [ { \"pool\": \"10.254.226.0/25\" } ],"
-        "    \"subnet\": \"10.254.226.0/24\", "
-        "    \"rebind-timer\": 2000, "
-        "    \"renew-timer\": 1000, "
-        "    \"valid-lifetime\": 4000,"
-        "    \"interface\": \"eth0\" "
-        " } ],"
-        "\"valid-lifetime\": 4000 }";
-
-    configure(config);
+    configure(CONFIGS[0]);
 
     // Let's create a relayed DISCOVER. This particular relayed DISCOVER has
     // added option 82 (relay agent info) with 3 suboptions. The server
@@ -3345,4 +3351,61 @@ TEST_F(Dhcpv4SrvTest, acceptMessageType) {
     }
 }
 
+// Test checks whether statistic is bumped up appropriately when Decline
+// message is received.
+TEST_F(Dhcpv4SrvTest, statisticsDecline) {
+    NakedDhcpv4Srv srv(0);
+
+    pretendReceivingPkt(srv, CONFIGS[0], DHCPDECLINE, "pkt4-decline-received");
+}
+
+// Test checks whether statistic is bumped up appropriately when Offer
+// message is received (this should never happen in a sane metwork).
+TEST_F(Dhcpv4SrvTest, statisticsOfferRcvd) {
+    NakedDhcpv4Srv srv(0);
+
+    pretendReceivingPkt(srv, CONFIGS[0], DHCPOFFER, "pkt4-offer-received");
+}
+
+// Test checks whether statistic is bumped up appropriately when Ack
+// message is received (this should never happen in a sane metwork).
+TEST_F(Dhcpv4SrvTest, statisticsAckRcvd) {
+    NakedDhcpv4Srv srv(0);
+
+    pretendReceivingPkt(srv, CONFIGS[0], DHCPACK, "pkt4-ack-received");
+}
+
+// Test checks whether statistic is bumped up appropriately when Nak
+// message is received (this should never happen in a sane metwork).
+TEST_F(Dhcpv4SrvTest, statisticsNakRcvd) {
+    NakedDhcpv4Srv srv(0);
+
+    pretendReceivingPkt(srv, CONFIGS[0], DHCPNAK, "pkt4-nak-received");
+}
+
+// Test checks whether statistic is bumped up appropriately when Release
+// message is received.
+TEST_F(Dhcpv4SrvTest, statisticsReleaseRcvd) {
+    NakedDhcpv4Srv srv(0);
+
+    pretendReceivingPkt(srv, CONFIGS[0], DHCPRELEASE, "pkt4-release-received");
+}
+
+// Test checks whether statistic is bumped up appropriately when unknown
+// message is received.
+TEST_F(Dhcpv4SrvTest, statisticsUnknownRcvd) {
+    NakedDhcpv4Srv srv(0);
+
+    pretendReceivingPkt(srv, CONFIGS[0], 200, "pkt4-unknown-received");
+
+    // There should also be pkt4-receive-drop stat bumped up
+    using namespace isc::stats;
+    StatsMgr& mgr = StatsMgr::instance();
+    ObservationPtr drop_stat = mgr.getObservation("pkt4-receive-drop");
+
+    // This statistic must be present and must be set to 1.
+    ASSERT_TRUE(drop_stat);
+    EXPECT_EQ(1, drop_stat->getInteger().first);
+}
+
 }; // end of anonymous namespace

+ 52 - 0
src/bin/dhcp4/tests/dhcp4_test_utils.cc

@@ -25,10 +25,12 @@
 #include <dhcp/option_custom.h>
 #include <dhcp/iface_mgr.h>
 #include <dhcp/tests/iface_mgr_test_config.h>
+#include <dhcp/tests/pkt_captures.h>
 #include <dhcpsrv/cfgmgr.h>
 #include <dhcpsrv/lease.h>
 #include <dhcpsrv/lease_mgr.h>
 #include <dhcpsrv/lease_mgr_factory.h>
+#include <stats/stats_mgr.h>
 
 using namespace std;
 using namespace isc::asiolink;
@@ -53,6 +55,9 @@ Dhcpv4SrvTest::Dhcpv4SrvTest()
     CfgMgr::instance().clear();
     CfgMgr::instance().getStagingCfg()->getCfgSubnets4()->add(subnet_);
     CfgMgr::instance().commit();
+
+    // Let's wipe all existing statistics.
+    isc::stats::StatsMgr::instance().removeAll();
 }
 
 Dhcpv4SrvTest::~Dhcpv4SrvTest() {
@@ -60,6 +65,9 @@ Dhcpv4SrvTest::~Dhcpv4SrvTest() {
     // Make sure that we revert to default value
     CfgMgr::instance().clear();
     CfgMgr::instance().echoClientId(true);
+
+    // Let's wipe all existing statistics.
+    isc::stats::StatsMgr::instance().removeAll();
 }
 
 void Dhcpv4SrvTest::addPrlOption(Pkt4Ptr& pkt) {
@@ -594,6 +602,50 @@ Dhcpv4SrvTest::createExchange(const Pkt4Ptr& query) {
     return (Dhcpv4Exchange(srv_.alloc_engine_, query, srv_.selectSubnet(query)));
 }
 
+void
+Dhcpv4SrvTest::pretendReceivingPkt(NakedDhcpv4Srv& srv, const std::string& config,
+                                   uint8_t pkt_type, const std::string& stat_name) {
+
+    IfaceMgrTestConfig test_config(true);
+    IfaceMgr::instance().openSockets4();
+
+    // Apply the configuration we just received.
+    configure(config);
+
+    // Let's just use one of the actual captured packets that we have.
+    Pkt4Ptr pkt = isc::test::PktCaptures::captureRelayedDiscover();
+
+    // We just need to tweak it a it, to pretend that it's type is as desired.
+    // Note that when receiving a packet, its on-wire form is stored in data_
+    // field. Most methods (including setType()) operates on option objects
+    // (objects stored in options_ after unpack() is called). Finally, outgoing
+    // packets are stored in out_buffer_. So we need to go through the full
+    // unpack/tweak/pack cycle and do repack, i.e. move the output buffer back
+    // to incoming buffer.
+    pkt->unpack();
+    pkt->setType(pkt_type); // Set message type.
+    pkt->pack();
+    pkt->data_.resize(pkt->getBuffer().getLength());
+    // Copy out_buffer_ to data_ to pretend that it's what was just received.
+    memcpy(&pkt->data_[0], pkt->getBuffer().getData(), pkt->getBuffer().getLength());
+
+    // Simulate that we have received that traffic
+    srv.fakeReceive(pkt);
+    srv.run();
+
+    using namespace isc::stats;
+    StatsMgr& mgr = StatsMgr::instance();
+    ObservationPtr pkt4_rcvd = mgr.getObservation("pkt4-received");
+    ObservationPtr tested_stat = mgr.getObservation(stat_name);
+
+    // All expected statstics must be present.
+    ASSERT_TRUE(pkt4_rcvd);
+    ASSERT_TRUE(tested_stat);
+
+    // They also must have expected values.
+    EXPECT_EQ(1, pkt4_rcvd->getInteger().first);
+    EXPECT_EQ(1, tested_stat->getInteger().first);
+}
 
 }; // end of isc::dhcp::test namespace
 }; // end of isc::dhcp namespace

+ 14 - 0
src/bin/dhcp4/tests/dhcp4_test_utils.h

@@ -391,6 +391,20 @@ public:
     void configure(const std::string& config, NakedDhcpv4Srv& srv,
                    const bool commit = true);
 
+    /// @brief Pretents a packet of specified type was received.
+    ///
+    /// Instantiates fake network interfaces, configures passed Dhcpv4Srv,
+    /// then creates a message of specified type and sends it to the
+    /// server and then checks whether expected statstics were set
+    /// appropriately.
+    ///
+    /// @param srv the DHCPv4 server to be used
+    /// @param config JSON configuration to be used
+    /// @param pkt_type type of the packet to be faked
+    /// @param stat_name name of the expected statistic
+    void pretendReceivingPkt(NakedDhcpv4Srv& srv, const std::string& config,
+                             uint8_t pkt_type, const std::string& stat_name);
+
     /// @brief Create @c Dhcpv4Exchange from client's query.
     Dhcpv4Exchange createExchange(const Pkt4Ptr& query);
 

+ 115 - 0
src/bin/dhcp4/tests/dora_unittest.cc

@@ -24,6 +24,7 @@
 #include <dhcp4/tests/dhcp4_test_utils.h>
 #include <dhcp4/tests/dhcp4_client.h>
 #include <boost/shared_ptr.hpp>
+#include <stats/stats_mgr.h>
 
 using namespace isc;
 using namespace isc::asiolink;
@@ -198,6 +199,17 @@ public:
         : Dhcpv4SrvTest(),
           iface_mgr_test_config_(true) {
         IfaceMgr::instance().openSockets4();
+
+        // Let's wipe all existing statistics.
+        isc::stats::StatsMgr::instance().removeAll();
+    }
+
+    /// @brief Desctructor.
+    ///
+    /// Cleans up statistics after the test.
+    ~DORATest() {
+        // Let's wipe all existing statistics.
+        isc::stats::StatsMgr::instance().removeAll();
     }
 
     /// @brief Test that server returns the same lease for the client which is
@@ -1017,4 +1029,107 @@ TEST_F(DORATest, reservationsWithConflicts) {
     ASSERT_EQ(in_pool_addr, clientB.config_.lease_.addr_);
 }
 
+/// This test verifies that after a client completes its DORA exchange,
+/// appropriate statistics are updated.
+TEST_F(DORATest, statisticsDORA) {
+    Dhcp4Client client(Dhcp4Client::SELECTING);
+    // Configure DHCP server.
+    configure(DORA_CONFIGS[0], *client.getServer());
+
+    // Perform 4-way exchange with the server but to not request any
+    // specific address in the DHCPDISCOVER message.
+    ASSERT_NO_THROW(client.doDORA());
+
+    // Make sure that the server responded.
+    ASSERT_TRUE(client.getContext().response_);
+    Pkt4Ptr resp = client.getContext().response_;
+    // Make sure that the server has responded with DHCPACK.
+    ASSERT_EQ(DHCPACK, static_cast<int>(resp->getType()));
+
+    // Ok, let's check the statistics.
+    using namespace isc::stats;
+    StatsMgr& mgr = StatsMgr::instance();
+    ObservationPtr pkt4_received = mgr.getObservation("pkt4-received");
+    ObservationPtr pkt4_discover_received = mgr.getObservation("pkt4-discover-received");
+    ObservationPtr pkt4_offer_sent = mgr.getObservation("pkt4-offer-sent");
+    ObservationPtr pkt4_request_received = mgr.getObservation("pkt4-request-received");
+    ObservationPtr pkt4_ack_sent = mgr.getObservation("pkt4-ack-sent");
+    ObservationPtr pkt4_sent = mgr.getObservation("pkt4-sent");
+
+    // All expected statstics must be present.
+    ASSERT_TRUE(pkt4_received);
+    ASSERT_TRUE(pkt4_discover_received);
+    ASSERT_TRUE(pkt4_offer_sent);
+    ASSERT_TRUE(pkt4_request_received);
+    ASSERT_TRUE(pkt4_ack_sent);
+    ASSERT_TRUE(pkt4_sent);
+
+    // They also must have expected values.
+    EXPECT_EQ(2, pkt4_received->getInteger().first);
+    EXPECT_EQ(1, pkt4_discover_received->getInteger().first);
+    EXPECT_EQ(1, pkt4_offer_sent->getInteger().first);
+    EXPECT_EQ(1, pkt4_request_received->getInteger().first);
+    EXPECT_EQ(1, pkt4_ack_sent->getInteger().first);
+    EXPECT_EQ(2, pkt4_sent->getInteger().first);
+
+    // Let the client send request 3 times, which should make the server
+    // to send 3 acks.
+    client.setState(Dhcp4Client::RENEWING);
+    ASSERT_NO_THROW(client.doRequest());
+    ASSERT_NO_THROW(client.doRequest());
+    ASSERT_NO_THROW(client.doRequest());
+
+    // Let's see if the stats are properly updated.
+    EXPECT_EQ(5, pkt4_received->getInteger().first);
+    EXPECT_EQ(1, pkt4_discover_received->getInteger().first);
+    EXPECT_EQ(1, pkt4_offer_sent->getInteger().first);
+    EXPECT_EQ(4, pkt4_request_received->getInteger().first);
+    EXPECT_EQ(4, pkt4_ack_sent->getInteger().first);
+    EXPECT_EQ(5, pkt4_sent->getInteger().first);
+}
+
+// This test verifies that after a client completes an exchange that result
+// in NAK, appropriate statistics are updated.
+TEST_F(DORATest, statisticsNAK) {
+    Dhcp4Client client(Dhcp4Client::SELECTING);
+    // Configure DHCP server.
+    configure(DORA_CONFIGS[0], *client.getServer());
+    // Obtain a lease from the server using the 4-way exchange.
+
+    // Get a lease.
+    client.doDORA();
+
+    // Wipe all stats.
+    isc::stats::StatsMgr::instance().removeAll();
+
+    client.setState(Dhcp4Client::INIT_REBOOT);
+    client.config_.lease_.addr_ = IOAddress("10.0.0.30");
+    ASSERT_NO_THROW(client.doRequest());
+    // Make sure that the server responded.
+    ASSERT_TRUE(client.getContext().response_);
+    Pkt4Ptr resp = client.getContext().response_;
+    EXPECT_EQ(DHCPNAK, static_cast<int>(resp->getType()));
+
+    using namespace isc::stats;
+    StatsMgr& mgr = StatsMgr::instance();
+    ObservationPtr pkt4_received = mgr.getObservation("pkt4-received");
+    ObservationPtr pkt4_request_received = mgr.getObservation("pkt4-request-received");
+    ObservationPtr pkt4_ack_sent = mgr.getObservation("pkt4-ack-sent");
+    ObservationPtr pkt4_nak_sent = mgr.getObservation("pkt4-nak-sent");
+    ObservationPtr pkt4_sent = mgr.getObservation("pkt4-sent");
+
+    // All expected statstics must be present.
+    ASSERT_TRUE(pkt4_received);
+    ASSERT_TRUE(pkt4_request_received);
+    ASSERT_FALSE(pkt4_ack_sent); // No acks were sent, no such statistic expected.
+    ASSERT_TRUE(pkt4_nak_sent);
+    ASSERT_TRUE(pkt4_sent);
+
+    // They also must have expected values.
+    EXPECT_EQ(1, pkt4_received->getInteger().first);
+    EXPECT_EQ(1, pkt4_request_received->getInteger().first);
+    EXPECT_EQ(1, pkt4_nak_sent->getInteger().first);
+    EXPECT_EQ(1, pkt4_sent->getInteger().first);
+}
+
 } // end of anonymous namespace

+ 65 - 0
src/bin/dhcp4/tests/inform_unittest.cc

@@ -19,6 +19,7 @@
 #include <dhcp/tests/iface_mgr_test_config.h>
 #include <dhcp4/tests/dhcp4_test_utils.h>
 #include <dhcp4/tests/dhcp4_client.h>
+#include <stats/stats_mgr.h>
 
 using namespace isc;
 using namespace isc::asiolink;
@@ -136,6 +137,17 @@ public:
         : Dhcpv4SrvTest(),
           iface_mgr_test_config_(true) {
         IfaceMgr::instance().openSockets4();
+
+        // Let's wipe all existing statistics.
+        isc::stats::StatsMgr::instance().removeAll();
+    }
+
+    /// @brief Desctructor.
+    ///
+    /// Cleans up statistics after the test.
+    ~InformTest() {
+        // Let's wipe all existing statistics.
+        isc::stats::StatsMgr::instance().removeAll();
     }
 
     /// @brief Interface Manager's fake configuration control.
@@ -378,5 +390,58 @@ TEST_F(InformTest, relayedClientNoCiaddr) {
     EXPECT_EQ("192.0.2.203", client.config_.dns_servers_[1].toText());
 }
 
+/// This test verifies that after a client completes its INFORM exchange,
+/// appropriate statistics are updated.
+TEST_F(InformTest, statisticsInform) {
+    Dhcp4Client client;
+    // Configure DHCP server.
+    configure(INFORM_CONFIGS[0], *client.getServer());
+    // Request some configuration when DHCPINFORM is sent.
+    client.requestOptions(DHO_LOG_SERVERS, DHO_COOKIE_SERVERS);
+    // Preconfigure the client with the IP address.
+    client.createLease(IOAddress("10.0.0.56"), 600);
+
+    // Send DHCPINFORM message to the server.
+    ASSERT_NO_THROW(client.doInform());
+
+    // Make sure that the server responded.
+    ASSERT_TRUE(client.getContext().response_);
+    Pkt4Ptr resp = client.getContext().response_;
+    // Make sure that the server has responded with DHCPACK.
+    ASSERT_EQ(DHCPACK, static_cast<int>(resp->getType()));
+
+    // Ok, let's check the statistics.
+    using namespace isc::stats;
+    StatsMgr& mgr = StatsMgr::instance();
+    ObservationPtr pkt4_received = mgr.getObservation("pkt4-received");
+    ObservationPtr pkt4_inform_received = mgr.getObservation("pkt4-inform-received");
+    ObservationPtr pkt4_ack_sent = mgr.getObservation("pkt4-ack-sent");
+    ObservationPtr pkt4_sent = mgr.getObservation("pkt4-sent");
+
+    // All expected statistics must be present.
+    ASSERT_TRUE(pkt4_received);
+    ASSERT_TRUE(pkt4_inform_received);
+    ASSERT_TRUE(pkt4_ack_sent);
+    ASSERT_TRUE(pkt4_sent);
+
+    // And they must have expected values.
+    EXPECT_EQ(1, pkt4_received->getInteger().first);
+    EXPECT_EQ(1, pkt4_inform_received->getInteger().first);
+    EXPECT_EQ(1, pkt4_ack_sent->getInteger().first);
+    EXPECT_EQ(1, pkt4_sent->getInteger().first);
+
+    // Let the client send iform 4 times, which should make the server
+    // to send 4 acks.
+    ASSERT_NO_THROW(client.doInform());
+    ASSERT_NO_THROW(client.doInform());
+    ASSERT_NO_THROW(client.doInform());
+    ASSERT_NO_THROW(client.doInform());
+
+    // Let's see if the stats are properly updated.
+    EXPECT_EQ(5, pkt4_received->getInteger().first);
+    EXPECT_EQ(5, pkt4_inform_received->getInteger().first);
+    EXPECT_EQ(5, pkt4_ack_sent->getInteger().first);
+    EXPECT_EQ(5, pkt4_sent->getInteger().first);
+}
 
 } // end of anonymous namespace

+ 24 - 14
src/lib/dhcp/pkt4.cc

@@ -241,17 +241,9 @@ Pkt4::unpack() {
     // }
     (void)offset;
 
-    // @todo check will need to be called separately, so hooks can be called
-    // after the packet is parsed, but before its content is verified
-    check();
-}
-
-void Pkt4::check() {
-    uint8_t msg_type = getType();
-    if (msg_type > DHCPLEASEACTIVE) {
-        isc_throw(BadValue, "Invalid DHCP message type received: "
-                  << static_cast<int>(msg_type));
-    }
+    // No need to call check() here. There are thorough tests for this
+    // later (see Dhcp4Srv::accept()). We want to drop the packet later,
+    // so we'll be able to log more detailed drop reason.
 }
 
 uint8_t Pkt4::getType() const {
@@ -274,12 +266,30 @@ uint8_t Pkt4::getType() const {
 void Pkt4::setType(uint8_t dhcp_type) {
     OptionPtr opt = getOption(DHO_DHCP_MESSAGE_TYPE);
     if (opt) {
-        // There is message type option already, update it
-        opt->setUint8(dhcp_type);
+
+        // There is message type option already, update it. It seems that
+        // we do have two types of objects representing message-type option.
+        // It would be more preferable to use only one type, but there's no
+        // easy way to enforce it.
+        //
+        // One is an instance of the Option class. It stores type in
+        // Option::data_, so Option::setUint8() and Option::getUint8() can be
+        // used. The other one is an instance of OptionInt<uint8_t> and
+        // it stores message type as integer, hence
+        // OptionInt<uint8_t>::getValue() and OptionInt<uint8_t>::setValue()
+        // should be used.
+        boost::shared_ptr<OptionInt<uint8_t> > type_opt =
+            boost::dynamic_pointer_cast<OptionInt<uint8_t> >(opt);
+        if (type_opt) {
+            type_opt->setValue(dhcp_type);
+        } else {
+            opt->setUint8(dhcp_type);
+        }
     } else {
         // There is no message type option yet, add it
         std::vector<uint8_t> tmp(1, dhcp_type);
-        opt = OptionPtr(new Option(Option::V4, DHO_DHCP_MESSAGE_TYPE, tmp));
+        opt = OptionPtr(new OptionInt<uint8_t>(Option::V4, DHO_DHCP_MESSAGE_TYPE,
+                                               tmp.begin(), tmp.end()));
         addOption(opt);
     }
 }

+ 0 - 13
src/lib/dhcp/pkt4.h

@@ -96,19 +96,6 @@ public:
     /// Method with throw exception if packet parsing fails.
     virtual void unpack();
 
-    /// @brief performs sanity check on a packet.
-    ///
-    /// This is usually performed after unpack(). It checks if packet is sane:
-    /// required options are present, fields have sane content etc.
-    /// For example verifies that DHCP_MESSAGE_TYPE is present and have
-    /// reasonable value. This method is expected to grow significantly.
-    /// It makes sense to separate unpack() and check() for testing purposes.
-    ///
-    /// @todo It is called from unpack() directly. It should be separated.
-    ///
-    /// Method will throw exception if anomaly is found.
-    void check();
-
     /// @brief Returns text representation of the primary packet identifiers
     ///
     /// This method is intended to be used to provide a consistent way to

+ 1 - 1
src/lib/dhcp/tests/pkt4_unittest.cc

@@ -978,7 +978,7 @@ TEST_F(Pkt4Test, toText) {
     EXPECT_EQ("local_address=192.0.2.34:67, remote_adress=192.10.33.4:68, "
               "msg_type=DHCPDISCOVER (1), transid=0x9ef,\n"
               "options:\n"
-              "  type=053, len=001: 01\n"
+              "  type=053, len=001: 1 (uint8)\n"
               "  type=087, len=011: \"lorem ipsum\" (string)\n"
               "  type=123, len=004: 192.0.2.3\n"
               "  type=156, len=004: 123456 (uint32)",