[dhcpmon] Validate downstream DHCPv4 reply fan-out by Xichen96 · Pull Request #107 · sonic-net/sonic-dhcpmon

Xichen96 · 2026-07-28T10:07:31Z

Why

ISC dhcrelay can send downstream replies using different destination addresses. The existing first-relay profile assumes broadcast replies, while the parent/member health check assumes every downstream VLAN TX packet is replicated to every configured VLAN member.

What changed

Validate DHCPOFFER, DHCPACK, and DHCPNAK through one downstream reply profile without a destination-IP assumption.
Require each IPv4 downstream VLAN TX packet to be observed on either one direct member for unicast or every direct member for broadcast.
Keep VLAN RX, PortChannel RX/TX, and all DHCPv6 parent/member comparisons exact 1:1.

Stack

Depends on #101. Review the final commit only:

6913601 [dhcpmon]: Validate downstream reply fan-out

Validation

git diff --check
Remote Azure Pipelines CI

mssonicbld · 2026-07-28T10:07:39Z

/azp run

azure-pipelines · 2026-07-28T10:07:40Z

Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run.

azure-pipelines · 2026-07-28T10:07:49Z

Azure Pipelines: Successfully started running 1 pipeline(s).

Copilot

Pull request overview

This PR updates dhcpmon’s downstream DHCPv4 validation and health-check aggregation logic to support environments where dhcrelay may transmit client-bound replies as either unicast or broadcast, while keeping other relay/aggregate comparisons aligned with the intended topology semantics.

Changes:

Accept downstream unicast or broadcast DHCPv4 DHCPOFFER/DHCPACK on TX, while keeping DHCPNAK broadcast-only.
Rework aggregate counter attribution to use immediate parent relationships (VLAN/PortChannel) and add explicit initialization of per-parent health states.
Update aggregate health comparison so IPv4 VLAN TX requires member aggregate >= parent (to allow unicast or broadcast replication), while keeping other aggregate comparisons strict.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
src/util.h	Simplifies aggregate-counter naming helper to be parent-based.
src/packet_handler.cpp	Updates aggregate counter increments to use devman-derived parent aggregate attribution.
src/health_check.h	Refactors health state definition and adds initialization API for discovered interface checks.
src/health_check.cpp	Builds a dynamic set of health checks for VLAN/PortChannel parents and logs per-interface disparity formats.
src/dhcp_mon.cpp	Initializes per-parent health states after counter initialization and updates aggregate recomputation attribution.
src/dhcp_devman.h	Adds APIs to resolve immediate parent and the correct aggregate counter target for an interface.
src/dhcp_devman.cpp	Implements parent traversal/context resolution and immediate-parent aggregate attribution.
src/dhcp_device.h	Consolidates aggregate check types into parent-vs-member-aggregate RX/TX (v4/v6).
src/dhcp_device.cpp	Implements new aggregate comparison rules, including VLAN TX “member aggregate >= parent” for IPv4.
src/dhcp_check_profile_relay.cpp	Allows unicast/broadcast for downstream `OFFER`/`ACK` by removing DST-IP constraint; keeps `NAK` broadcast-only.

mssonicbld · 2026-07-28T16:33:25Z

/azp run

azure-pipelines · 2026-07-28T16:33:34Z

Azure Pipelines: Successfully started running 1 pipeline(s).

Copilot

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

mssonicbld · 2026-07-28T17:01:23Z

/azp run

azure-pipelines · 2026-07-28T17:01:34Z

Azure Pipelines: Successfully started running 1 pipeline(s).

Copilot

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

src/dhcp_device.cpp:245

check_counters_delta_expected() computes deltas via unsigned subtraction, which can underflow when counters are cleared/reset and produce incorrect comparisons (often forcing false UNHEALTHY). Use a guarded delta (cur >= snap ? cur - snap : 0) for both interfaces.

    for (size_t i = 0; i < monitored_msg_cnt; i++) {
        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 ((!allow_member_fanout && delta != other_delta) || (allow_member_fanout && delta > other_delta)) {
            return false;

src/dhcp_device.cpp:126

get_counter_delta() subtracts current - snapshot without handling counter resets (e.g., CLI clear updates cache counters from DB). If the current value is lower than the snapshot, this underflows uint64_t and can look like a huge delta, leading to false health results.

This issue also appears on line 241 of the same file.

static uint64_t get_counter_delta(const std::string &ifname, int sock, int msg_type)
{
    const sock_info_t &sock_info = sock_mgr_get_sock_info(sock);
    return sock_info.all_counters.at(ifname).at(msg_type) -
           sock_info.all_counters_snapshot.at(ifname).at(msg_type);
}

mssonicbld · 2026-07-28T17:23:17Z

/azp run

azure-pipelines · 2026-07-28T17:23:27Z

Azure Pipelines: Successfully started running 1 pipeline(s).

Copilot

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

src/dhcp_device.cpp:126

get_counter_delta() subtracts snapshot from current using unsigned arithmetic. If counters are cleared/reset between snapshots (or wrap), this underflows and produces a huge delta, which will incorrectly flag health checks as unhealthy.

    const sock_info_t &sock_info = sock_mgr_get_sock_info(sock);
    return sock_info.all_counters.at(ifname).at(msg_type) -
           sock_info.all_counters_snapshot.at(ifname).at(msg_type);
}

 static dhcp_msg_check_profile_t tx_first_relay_reply = {
    {DHCP_CHECK_INTF_TYPE, (const void *)(new std::vector<dhcp_device_intf_t>{DHCP_DEVICE_INTF_TYPE_DOWNLINK, DHCP_DEVICE_INTF_TYPE_MGMT})},
    {DHCP_CHECK_SRC_IP, (const void *)(new std::vector<const in_addr *>{&vlan_ip})},
-    {DHCP_CHECK_DST_IP, (const void *)(new std::vector<const in_addr *>{&broadcast_ip})},
    {DHCP_CHECK_GIADDR, (const void *)(new std::vector<const in_addr *>{&giaddr_ip})},
 };


mssonicbld · 2026-07-28T17:35:19Z

/azp run

azure-pipelines · 2026-07-28T17:35:29Z

Azure Pipelines: Successfully started running 1 pipeline(s).

mssonicbld · 2026-07-28T17:39:36Z

/azp run

azure-pipelines · 2026-07-28T17:39:47Z

Azure Pipelines: Successfully started running 1 pipeline(s).

Copilot

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

Copilot

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

src/dhcp_device.cpp:245

check_counters_delta_expected() computes deltas with unsigned subtraction (current - snapshot). If counters are reduced during DB-to-cache sync (e.g., cleared in COUNTERS_DB), this underflows and can incorrectly fail/pass aggregate checks. Use get_counter_delta() (with clamping) for both sides when computing deltas.

    for (size_t i = 0; i < monitored_msg_cnt; i++) {
        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 ((!allow_member_fanout && delta != other_delta) || (allow_member_fanout && delta > other_delta)) {
            return false;

src/dhcp_device.cpp:126

get_counter_delta() subtracts snapshot from current using uint64_t. Cache counters can be decreased when syncing from COUNTERS_DB (e.g., after a CLI clear), which would underflow and look like a huge positive delta, causing false "counter increased" detections and spurious health alarms. Clamp the delta to 0 when current < snapshot.

This issue also appears on line 241 of the same file.

static uint64_t get_counter_delta(const std::string &ifname, int sock, int msg_type)
{
    const sock_info_t &sock_info = sock_mgr_get_sock_info(sock);
    return sock_info.all_counters.at(ifname).at(msg_type) -
           sock_info.all_counters_snapshot.at(ifname).at(msg_type);
}

mssonicbld · 2026-07-28T17:49:50Z

/azp run

azure-pipelines · 2026-07-28T17:50:02Z

Azure Pipelines: Successfully started running 1 pipeline(s).

Copilot

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

mssonicbld · 2026-07-31T17:58:04Z

/azp run

azure-pipelines · 2026-07-31T17:58:14Z

Azure Pipelines: Successfully started running 1 pipeline(s).

Bound each raw-socket callback to 64 packets and reset the recvfrom address length for every receive attempt. Rename three file-scope helpers that begin with underscores because C++ reserves those identifiers in the global namespace; static linkage already marks them as internal. Replace four nullptr comparisons with NULL to match the daemon's established style. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 39f979be-d826-4d5c-949a-f20abb58bb83 Signed-off-by: Xichen96 <lukelin0907@gmail.com>

Preserve VLAN and PortChannel membership so a physical member resolves through its immediate PortChannel parent to the monitored VLAN context. Keep direct VLAN-member precedence, bound hierarchy traversal, ignore self-references, and centralize parent, context, and aggregate-name lookup. Aggregate only to the immediate parent so pre-VLAN observations are not rolled into the root VLAN counter and misreported as relay loss. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 39f979be-d826-4d5c-949a-f20abb58bb83 Signed-off-by: Xichen96 <lukelin0907@gmail.com>

Populate one state per discovered VLAN or PortChannel check, derive aggregate ratios from topology, and format aggregate disparity errors with the interface and duration. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 39f979be-d826-4d5c-949a-f20abb58bb83 Signed-off-by: Xichen96 <lukelin0907@gmail.com>

Replace invalid same-message-type DHCPv6 disparity checks with relay-aware activity checks. Require RX Solicit, Request, or Relay-Forward activity to produce TX Relay-Forward activity, and RX Relay-Reply activity to produce TX Advertise, Reply, or Relay-Reply activity. Keep direct lookups for the single Relay-Forward and Relay-Reply counters and compare activity rather than magnitude so multiple configured servers remain valid. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 39f979be-d826-4d5c-949a-f20abb58bb83 Signed-off-by: Xichen96 <lukelin0907@gmail.com>

Allow first-hop DHCPOFFER, DHCPACK, and DHCPNAK profile validation without a destination-IP assumption. Require each IPv4 VLAN TX packet to appear on either one direct member for unicast or every direct member for broadcast; keep all other parent/member comparisons exact. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 39f979be-d826-4d5c-949a-f20abb58bb83 Signed-off-by: Xichen96 <lukelin0907@gmail.com>

mssonicbld · 2026-07-31T19:20:43Z

/azp run

azure-pipelines · 2026-07-31T19:20:52Z

Azure Pipelines: Successfully started running 1 pipeline(s).

Copilot

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/dhcp_device.cpp:418

The new aggregate fan-out health checks (AGG_RX/AGG_TX) still use monitored_msgs, which excludes DHCP_MESSAGE_TYPE_NAK. That conflicts with this PR’s stated goal of validating downstream replies including DHCPNAK, and means NAK fan-out issues won’t be detected by the aggregate checks.

        case DHCP_DEVICE_CHECK_AGG_RX:
            return check_aggregate_health(ifname, rx_sock, (const int *)monitored_msgs, monitored_msg_sz);
        case DHCP_DEVICE_CHECK_AGG_TX:
            return check_aggregate_health(ifname, tx_sock, (const int *)monitored_msgs, monitored_msg_sz);
        case DHCP_DEVICE_CHECK_AGG_RX_V6:

Copilot AI review requested due to automatic review settings July 28, 2026 10:07

Copilot started reviewing on behalf of Xichen96 July 28, 2026 10:08 View session

Copilot AI reviewed Jul 28, 2026

View reviewed changes

Xichen96 force-pushed the dev/xichenlin/accept-unicast-downstream-replies branch from 4854135 to 93dca81 Compare July 28, 2026 16:33

Copilot AI review requested due to automatic review settings July 28, 2026 16:33

Copilot started reviewing on behalf of Xichen96 July 28, 2026 16:33 View session

Xichen96 mentioned this pull request Jul 28, 2026

[dhcpmon] Validate configured DHCP server fan-out #108

Closed

1 task

Copilot AI reviewed Jul 28, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings July 28, 2026 17:01

Xichen96 force-pushed the dev/xichenlin/accept-unicast-downstream-replies branch from 93dca81 to 16880b9 Compare July 28, 2026 17:01

Copilot started reviewing on behalf of Xichen96 July 28, 2026 17:01 View session

Copilot AI reviewed Jul 28, 2026

View reviewed changes

Xichen96 force-pushed the dev/xichenlin/accept-unicast-downstream-replies branch from 16880b9 to aecd9c1 Compare July 28, 2026 17:23

Copilot AI review requested due to automatic review settings July 28, 2026 17:23

Copilot started reviewing on behalf of Xichen96 July 28, 2026 17:23 View session

Xichen96 changed the title ~~[dhcpmon] Accept unicast downstream DHCPv4 replies~~ [dhcpmon] Accept downstream DHCPv4 reply destinations Jul 28, 2026

Copilot AI reviewed Jul 28, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings July 28, 2026 17:35

Xichen96 force-pushed the dev/xichenlin/accept-unicast-downstream-replies branch from aecd9c1 to 0a242c1 Compare July 28, 2026 17:35

Copilot started reviewing on behalf of Xichen96 July 28, 2026 17:35 View session

Xichen96 force-pushed the dev/xichenlin/accept-unicast-downstream-replies branch from 0a242c1 to e161178 Compare July 28, 2026 17:39

Copilot AI reviewed Jul 28, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings July 28, 2026 17:42

Copilot started reviewing on behalf of Xichen96 July 28, 2026 17:43 View session

Copilot AI reviewed Jul 28, 2026

View reviewed changes

Xichen96 force-pushed the dev/xichenlin/accept-unicast-downstream-replies branch from e161178 to 72bf5fe Compare July 28, 2026 17:49

Copilot AI review requested due to automatic review settings July 28, 2026 17:49

Copilot started reviewing on behalf of Xichen96 July 28, 2026 17:50 View session

Xichen96 changed the title ~~[dhcpmon] Accept downstream DHCPv4 reply destinations~~ [dhcpmon] Validate downstream DHCPv4 reply fan-out Jul 28, 2026

Copilot AI reviewed Jul 28, 2026

View reviewed changes

Xichen96 mentioned this pull request Jul 31, 2026

[dhcpmon] Tolerate one packet across health windows #109

Draft

1 task

Xichen96 force-pushed the dev/xichenlin/accept-unicast-downstream-replies branch from 72bf5fe to 83ba1c4 Compare July 31, 2026 17:57

Xichen96 and others added 5 commits August 1, 2026 05:01

Copilot AI review requested due to automatic review settings July 31, 2026 19:20

Xichen96 force-pushed the dev/xichenlin/accept-unicast-downstream-replies branch from 83ba1c4 to 6913601 Compare July 31, 2026 19:20

Copilot started reviewing on behalf of Xichen96 July 31, 2026 19:21 View session

Copilot AI reviewed Jul 31, 2026

View reviewed changes

Uh oh!

Conversation

Xichen96 commented Jul 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why

What changed

Stack

Validation

Uh oh!

mssonicbld commented Jul 28, 2026

Uh oh!

azure-pipelines Bot commented Jul 28, 2026

Uh oh!

azure-pipelines Bot commented Jul 28, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

mssonicbld commented Jul 28, 2026

Uh oh!

azure-pipelines Bot commented Jul 28, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

mssonicbld commented Jul 28, 2026

Uh oh!

azure-pipelines Bot commented Jul 28, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

mssonicbld commented Jul 28, 2026

Uh oh!

azure-pipelines Bot commented Jul 28, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

mssonicbld commented Jul 28, 2026

Uh oh!

azure-pipelines Bot commented Jul 28, 2026

Uh oh!

mssonicbld commented Jul 28, 2026

Uh oh!

azure-pipelines Bot commented Jul 28, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

mssonicbld commented Jul 28, 2026

Uh oh!

azure-pipelines Bot commented Jul 28, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

mssonicbld commented Jul 31, 2026

Uh oh!

azure-pipelines Bot commented Jul 31, 2026

Uh oh!

mssonicbld commented Jul 31, 2026

Uh oh!

azure-pipelines Bot commented Jul 31, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Xichen96 commented Jul 28, 2026 •

edited

Loading