[dhcpmon] Track PortChannels under downstream VLANs

src/dhcp_device.cpp

@@ -69,6 +69,8 @@ static const char *counter_desc[DHCP_COUNTERS_COUNT] = {
     [DHCP_COUNTERS_SNAPSHOT_V6] = "Snapshot_V6",
 };
 
+static std::string last_counter_mismatch;
+
 /**
  * @code check_counter_not_transmitted(ifname, rx_sock, tx_sock, monitored_msgs, monitored_msg_cnt);
  * @brief Check if there are received DHCP messages that are not transmitted out
@@ -217,70 +219,91 @@ static bool check_counters_delta_expected(const std::string &ifname, const std::
         uint64_t delta = counters.at(monitored_msgs[i]) - counters_snapshot.at(monitored_msgs[i]);
         uint64_t other_delta = other_counters.at(monitored_msgs[i]) - other_counters_snapshot.at(monitored_msgs[i]);
         if (delta * ratio != other_delta) {
+            const std::string *message_names = sock_info.is_v6 ? db_counter_name_v6 : db_counter_name;
+            last_counter_mismatch =
+                std::string(sock_info.is_v6 ? "IPv6 " : "IPv4 ") +
+                (sock_info.is_rx ? "RX" : "TX") +
+                " edge parent=" + ifname +
+                " parent_delta=" + std::to_string(delta) +
+                " child_aggregate=" + other_ifname +
+                " child_delta=" + std::to_string(other_delta) +
+                " expected_ratio=" + std::to_string(ratio) +
+                " message=" + message_names[monitored_msgs[i]];
             return false;
         }
     }
+
     return true;
 }
 
-static dhcp_mon_status_t dhcp_device_check_agg_equal_rx(const std::string &ifname)
+static dhcp_mon_status_t check_aggregate_health(const std::string &ifname, int sock, uint8_t ratio,
+                                                const int *monitored_msgs, size_t monitored_msg_cnt)
 {
     std::string agg_ifname = agg_dev_prefix + ifname;
-    return check_counters_delta_expected(ifname, agg_ifname, rx_sock, 1, (const int *)monitored_msgs, monitored_msg_sz) ?
+    if (!check_counter_increased(ifname, sock, monitored_msgs, monitored_msg_cnt) &&
+        !check_counter_increased(agg_ifname, sock, monitored_msgs, monitored_msg_cnt)) {
+        return DHCP_MON_STATUS_INDETERMINATE;
+    }
+    return check_counters_delta_expected(ifname, agg_ifname, sock, ratio,
+                                         monitored_msgs, monitored_msg_cnt) ?
            DHCP_MON_STATUS_HEALTHY : DHCP_MON_STATUS_UNHEALTHY;
 }
 
+const std::string &dhcp_device_get_last_counter_mismatch()
+{
+    return last_counter_mismatch;
+}
+
+static dhcp_mon_status_t dhcp_device_check_agg_equal_rx(const std::string &ifname)
+{
+    return check_aggregate_health(ifname, rx_sock, 1, (const int *)monitored_msgs, monitored_msg_sz);
+}
+
 static dhcp_mon_status_t dhcp_device_check_agg_equal_tx(const std::string &ifname)
 {
-    std::string agg_ifname = agg_dev_prefix + ifname;
-    return check_counters_delta_expected(ifname, agg_ifname, tx_sock, 1, (const int *)monitored_msgs, monitored_msg_sz) ?
-           DHCP_MON_STATUS_HEALTHY : DHCP_MON_STATUS_UNHEALTHY;
+    return check_aggregate_health(ifname, tx_sock, 1, (const int *)monitored_msgs, monitored_msg_sz);
 }
 
 static dhcp_mon_status_t dhcp_device_check_agg_equal_rx_v6(const std::string &ifname)
 {
-    std::string agg_ifname = agg_dev_prefix + ifname;
-    return check_counters_delta_expected(ifname, agg_ifname, rx_sock_v6, 1, (const int *)monitored_v6_msgs, monitored_v6_msg_sz) ?
-           DHCP_MON_STATUS_HEALTHY : DHCP_MON_STATUS_UNHEALTHY;
+    return check_aggregate_health(ifname, rx_sock_v6, 1, (const int *)monitored_v6_msgs, monitored_v6_msg_sz);
 }
 
 static dhcp_mon_status_t dhcp_device_check_agg_equal_tx_v6(const std::string &ifname)
 {
-    std::string agg_ifname = agg_dev_prefix + ifname;
-    return check_counters_delta_expected(ifname, agg_ifname, tx_sock_v6, 1, (const int *)monitored_v6_msgs, monitored_v6_msg_sz) ?
-           DHCP_MON_STATUS_HEALTHY : DHCP_MON_STATUS_UNHEALTHY;
+    return check_aggregate_health(ifname, tx_sock_v6, 1, (const int *)monitored_v6_msgs, monitored_v6_msg_sz);
 }
 
 static dhcp_mon_status_t dhcp_device_check_agg_multiple_rx(const std::string &ifname)
 {
-    std::string agg_ifname = agg_dev_prefix + ifname;
-    return check_counters_delta_expected(ifname, agg_ifname, rx_sock, readonly_access(rev_vlan_map, ifname).size() + readonly_access(rev_portchan_map, ifname).size(),
-                                        (const int *)monitored_msgs, monitored_msg_sz) ?
-           DHCP_MON_STATUS_HEALTHY : DHCP_MON_STATUS_UNHEALTHY;
+    return check_aggregate_health(ifname, rx_sock,
+                                  readonly_access(rev_vlan_map, ifname).size() +
+                                  readonly_access(rev_portchan_map, ifname).size(),
+                                  (const int *)monitored_msgs, monitored_msg_sz);
 }
 
 static dhcp_mon_status_t dhcp_device_check_agg_multiple_tx(const std::string &ifname)
 {
-    std::string agg_ifname = agg_dev_prefix + ifname;
-    return check_counters_delta_expected(ifname, agg_ifname, tx_sock, readonly_access(rev_vlan_map, ifname).size() + readonly_access(rev_portchan_map, ifname).size(),
-                                        (const int *)monitored_msgs, monitored_msg_sz) ?
-           DHCP_MON_STATUS_HEALTHY : DHCP_MON_STATUS_UNHEALTHY;
+    return check_aggregate_health(ifname, tx_sock,
+                                  readonly_access(rev_vlan_map, ifname).size() +
+                                  readonly_access(rev_portchan_map, ifname).size(),
+                                  (const int *)monitored_msgs, monitored_msg_sz);
 }
 
 static dhcp_mon_status_t dhcp_device_check_agg_multiple_rx_v6(const std::string &ifname)
 {
-    std::string agg_ifname = agg_dev_prefix + ifname;
-    return check_counters_delta_expected(ifname, agg_ifname, rx_sock_v6, readonly_access(rev_vlan_map, ifname).size() + readonly_access(rev_portchan_map, ifname).size(),
-                                        (const int *)monitored_v6_msgs, monitored_v6_msg_sz) ?
-           DHCP_MON_STATUS_HEALTHY : DHCP_MON_STATUS_UNHEALTHY;
+    return check_aggregate_health(ifname, rx_sock_v6,
+                                  readonly_access(rev_vlan_map, ifname).size() +
+                                  readonly_access(rev_portchan_map, ifname).size(),
+                                  (const int *)monitored_v6_msgs, monitored_v6_msg_sz);
 }
 
 static dhcp_mon_status_t dhcp_device_check_agg_multiple_tx_v6(const std::string &ifname)
 {
-    std::string agg_ifname = agg_dev_prefix + ifname;
-    return check_counters_delta_expected(ifname, agg_ifname, tx_sock_v6, readonly_access(rev_vlan_map, ifname).size() + readonly_access(rev_portchan_map, ifname).size(),
-                                        (const int *)monitored_v6_msgs, monitored_v6_msg_sz) ?
-           DHCP_MON_STATUS_HEALTHY : DHCP_MON_STATUS_UNHEALTHY;
+    return check_aggregate_health(ifname, tx_sock_v6,
+                                  readonly_access(rev_vlan_map, ifname).size() +
+                                  readonly_access(rev_portchan_map, ifname).size(),
+                                  (const int *)monitored_v6_msgs, monitored_v6_msg_sz);
 }
 
 /**
@@ -382,7 +405,10 @@ void dhcp_device_print_status_debug(const std::string &ifname, dhcp_counters_typ
 
 dhcp_mon_status_t dhcp_device_get_status(const std::string &ifname, dhcp_device_check_t check_type)
 {
-    if (sock_mgr_counters_unchanged(ifname, (const int *)monitored_msgs, monitored_msg_sz, (const int *)monitored_v6_msgs, monitored_v6_msg_sz)) {
+    bool hierarchy_check = check_type >= DHCP_DEVICE_CHECK_AGG_EQUAL_RX;
+    if (!hierarchy_check &&
+        sock_mgr_counters_unchanged(ifname, (const int *)monitored_msgs, monitored_msg_sz,
+                                    (const int *)monitored_v6_msgs, monitored_v6_msg_sz)) {
         return DHCP_MON_STATUS_INDETERMINATE;
     }
 

src/dhcp_device.h

@@ -242,6 +242,9 @@ void dhcp_device_free(dhcp_device_context_t *context);
  */
 dhcp_mon_status_t dhcp_device_get_status(const std::string &ifname, dhcp_device_check_t check_type);
 
+/** Return details for the most recent interface hierarchy mismatch */
+const std::string &dhcp_device_get_last_counter_mismatch();
+
 /**
  * @code dhcp_device_print_status(ifname, type);
  *

src/dhcp_devman.cpp

@@ -248,7 +248,10 @@ static void update_portchannel_mapping()
         auto second = key.find_last_of('|');
         auto portchannel = key.substr(first + 1, second - first - 1);
         auto ifname = key.substr(second + 1);
-        if (intfs.find(portchannel) == intfs.end()) {
+        bool portchannel_is_context = intfs.find(portchannel) != intfs.end();
+        bool portchannel_is_vlan_member = vlan_map.find(portchannel) != vlan_map.end();
+        // Dual-ToR downlink counters require MUX attribution that is unavailable on a nested PortChannel.
+        if (!portchannel_is_context && (!portchannel_is_vlan_member || dual_tor_mode)) {
             all_skipped_ifname += "<" + ifname + ", " + portchannel + ">, ";
             continue;
         }
@@ -296,7 +299,7 @@ int dhcp_devman_init()
     agg_dev_all = "Agg-" + downstream_ifname;
     agg_dev_prefix = agg_dev_all + "-";
 
-    // vlan and its members, portchannel and its members are initialized regardless of whether they are in cmdline
+    // PortChannel members depend on VLAN mappings to recognize a PortChannel under a monitored VLAN.
     update_vlan_mapping();
     update_portchannel_mapping();
 
@@ -321,15 +324,34 @@ const dhcp_device_context_t *dhcp_devman_get_device_context(const std::string &i
     if (iter != intfs.end()) {
         return iter->second;
     }
+    const auto port_channel = portchan_map.find(ifname);
+    if (port_channel != portchan_map.end() && ifname != port_channel->second) {
+        return dhcp_devman_get_device_context(port_channel->second);
+    }
     const auto vlan = vlan_map.find(ifname);
     if (vlan != vlan_map.end() && ifname != vlan->second) {
         return dhcp_devman_get_device_context(vlan->second);
     }
+    return NULL;
+}
+
+std::string dhcp_devman_get_parent_ifname(const std::string &ifname)
+{
     const auto port_channel = portchan_map.find(ifname);
     if (port_channel != portchan_map.end() && ifname != port_channel->second) {
-        return dhcp_devman_get_device_context(port_channel->second);
+        return port_channel->second;
     }
-    return NULL;
+    const auto vlan = vlan_map.find(ifname);
+    if (vlan != vlan_map.end() && ifname != vlan->second) {
+        return vlan->second;
+    }
+    return "";
+}
+
+std::string dhcp_devman_get_agg_counter_ifname(const std::string &ifname)
+{
+    const std::string parent_ifname = dhcp_devman_get_parent_ifname(ifname);
+    return parent_ifname.empty() ? agg_dev_all : agg_dev_prefix + parent_ifname;
 }
 
 void dhcp_devman_print_all_status(dhcp_counters_type_t type)

src/dhcp_devman.h

@@ -138,6 +138,28 @@ void dhcp_devman_free();
  */
 const dhcp_device_context_t* dhcp_devman_get_device_context(const std::string &ifname);
 
+/**
+ * @code dhcp_devman_get_parent_ifname(ifname);
+ *
+ * @brief find the immediate parent interface of a tracked interface.
+ *
+ * @param ifname           interface name
+ *
+ * @return parent interface name, or an empty string for a root context interface
+ */
+std::string dhcp_devman_get_parent_ifname(const std::string &ifname);
+
+/**
+ * @code dhcp_devman_get_agg_counter_ifname(ifname);
+ *
+ * @brief find the aggregate counter for the immediate parent of a tracked interface.
+ *
+ * @param ifname           interface name
+ *
+ * @return aggregate counter name
+ */
+std::string dhcp_devman_get_agg_counter_ifname(const std::string &ifname);
+
 /**
  * @code dhcp_devman_print_all_status(type);
  *

src/dhcp_mon.cpp

@@ -81,7 +81,7 @@ static void recalculate_agg_counter(all_counters_t &all_counters)
             if (mgmt_ifname == context->intf) {
                 continue;
             }
-            counter_t &agg_counter = all_counters.at(get_agg_counter_ifname(ifname, context->intf));
+            counter_t &agg_counter = all_counters.at(dhcp_devman_get_agg_counter_ifname(ifname));
             for (const auto &[msg_type, count] : counter) {
                 agg_counter[msg_type] += count;
             }

src/health_check.cpp

@@ -90,7 +90,7 @@ static dhcp_mon_status_t check_per_interface_rx_health()
 static void log_agg_per_interface_rx_error(int duration)
 {
     syslog(LOG_ALERT, "sum of rx per interface counter does not equal corresponding vlan/portchan counter."
-           " Duration: %d (sec)", duration);
+           " Duration: %d (sec). %s", duration, dhcp_device_get_last_counter_mismatch().c_str());
 }
 
 static dhcp_mon_status_t check_per_interface_tx_health()
@@ -112,7 +112,7 @@ static void log_agg_per_interface_tx_error(int duration)
 {
     syslog(LOG_ALERT, "each tx per interface counter does not equal corresponding vlan counter,"
            " or sum of tx per interface counter does not equal corresponding portchan counter."
-           " Duration: %d (sec)", duration);
+           " Duration: %d (sec). %s", duration, dhcp_device_get_last_counter_mismatch().c_str());
 }
 
 static dhcp_mon_status_t check_per_interface_rx_health_v6()

src/packet_handler.cpp

@@ -47,14 +47,12 @@ static void increase_cache_counter(const std::string &ifname, const dhcp_device_
 {
     _increase_cache_counter(ifname, sock, type);
 
-    // we seperate mgmt interface from others and do not increase agg counter
+    // we separate mgmt interface from others and do not increase agg counter
     if (mgmt_ifname != "" && mgmt_ifname.compare(context->intf) == 0) {
         return;
     }
 
-    // when ifname belongs to another context ifname, increase the aggregate counter for that context, 
-    // else when ifname is the context, we increase agg counter for all.
-    _increase_cache_counter(get_agg_counter_ifname(ifname, context->intf), sock, type);
+    _increase_cache_counter(dhcp_devman_get_agg_counter_ifname(ifname), sock, type);
 
     // optionally duplicate to context ifname, it will only be true when this is standby physical interface under a vlan on a dual tor
     if (dup_to_context) {

src/util.h

@@ -216,18 +216,6 @@ inline bool is_agg_counter(const std::string &ifname)
     return ifname.compare(0, agg_dev_prefix.size(), agg_dev_prefix) == 0 || ifname == agg_dev_all;
 }
 
-/**
- * @code                get_agg_counter_ifname(ifname, context);
- * @brief               Get aggregate counter name for given ifname and device context
- * @param ifname        Interface name
- * @param context       Pointer to device context
- * @return              Aggregate counter name
- */
-inline std::string get_agg_counter_ifname(const std::string &ifname, const std::string &context_ifname)
-{
-    return ifname != context_ifname ? agg_dev_prefix + context_ifname : agg_dev_all;
-}
-
 /**
  * @code                contains_value(v, value);
  * @brief               Check if a vector contains a specific value