[dhcpmon] Make relay disparity detection resilient

[Xichen96]

Description of PR

Summary:

The current relay-disparity check marks a health window unhealthy whenever RX
increases and TX does not increase in the same snapshot interval. RX and TX are
captured on independent socket threads, so a valid transmit observed in the
adjacent window can be reported as a relay failure. Once the unhealthy threshold
is crossed, the same event and log are also emitted every subsequent window.

This change tracks per-message relay activity with absolute counter watermarks,
retains one-window TX credit for RX/TX observation skew, preserves a pending
unmatched RX across idle windows, and clears it when TX activity appears.
Health state is explicitly reset after counter initialization and counter-clear
synchronization. Each DHCPv4 message type owns its persistence/report state, so
one type cannot borrow duration from another.

The existing DHCPv6 same-type disparity check is removed because DHCPv6 changes
message types across the relay boundary (for example, Solicit to Relay-Forward);
it cannot produce a reliable same-type RX/TX signal. DHCPv6 counters and
hierarchy checks remain unchanged.

Dependencies:

• [dhcpmon] Track PortChannels under downstream VLANs #89 makes the root RX counter
  represent the VLAN/SVI relay boundary for the Fungible F2 nested topology.
• [dhcpmon] Apply small packet and C++ hardening fixes #96 bounds packet callback
  work.
• [dhcpmon] Protect counters from concurrent packet and health updates #97 serializes packet-counter
  writes with health sampling, counter clear, and DB synchronization.
• [dhcpmon] Write COUNTERS_DB without blocking packet handling #98 writes COUNTERS_DB from a
  stable snapshot without blocking packet capture during Redis I/O.
  This PR must merge after those counter-state prerequisites.
• This draft will be rebased after
  [dhcp_device] Skip address initialization for management interface #87 and
  [dhcpmon] Clean up source include dependencies #88 merge.

Work item tracking

• Microsoft ADO (number only): 38615655

Type of change

• Bug fix
• New feature
• Refactor / cleanup
• Documentation update
• Test improvement

Approach

What is the motivation for this PR?

The existing per-window comparison is sensitive to capture-thread and snapshot
timing. A reliable relay-health signal must tolerate adjacent-window ordering,
preserve a genuinely unmatched RX, and survive idle periods and counter clears
without replaying stale totals.

How did you do it?

• Track absolute RX/TX watermarks independently for each monitored DHCPv4
  message type.
• Retain a one-window TX credit to tolerate TX-before-RX observation ordering.
• Keep an unmatched RX pending across idle windows until TX activity arrives.
• Track pending-window age and report latches independently per message type.
• Seed health watermarks at daemon initialization.
• Schedule an atomic report/watermark reset on the main health loop after all
  counter-cache updates complete during a clear operation.
• Publish/log only once per unhealthy episode and reset the report latch after
  recovery.
• Stop publishing the invalid DHCPv6 same-type disparity signal.
• Preserve the existing
  sonic-events-dhcp-relay:dhcp-relay-disparity event name and payload.

How did you verify/test it?

Pending this PR's Azure CI and master hardware validation.

Planned validation:

• Existing telemetry disparity event test still produces the same event.
• The event still appears on the 11th consecutive unmatched DHCPv4 window.
• RX in one window and TX in the adjacent window does not alert.
• TX observed one window before RX does not alert.
• RX with no later TX alerts after the configured persistence threshold.
• Idle windows do not clear a genuinely pending RX.
• Recovery allows a later unhealthy episode to publish again.
• Counter clear during traffic does not replay historical totals or alert.

Any platform specific information?

No. The issue comes from independent packet-capture threads and health-window
sampling.

Back port request

None. This repair targets master only.

Tested branch

Pending.

Test result

Pending this PR's Azure CI and master hardware validation.

Documentation

Not applicable; the existing event and counter interfaces are preserved.

[mssonicbld]

/azp run

[azure-pipelines]

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

[azure-pipelines]

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

[Copilot]

Pull request overview

Improves DHCP relay disparity health detection to be resilient to RX/TX observation skew between independent capture threads, avoid replaying stale totals after counter clears, and ensure alerts/logs are emitted once per unhealthy episode until recovery.

Changes:

• Add per-check “reported” latching so an unhealthy episode publishes/logs once and can re-trigger after recovery.
• Replace per-window snapshot comparison with per-message absolute watermark tracking (pending RX + one-window TX credit + grace window) for relay disparity.
• Reset relay-health watermarks at daemon init and after counter-cache resync completes during counter clear.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/health_check.h	Adds per-check reported flag to suppress repeat reporting within an unhealthy episode.
src/health_check.cpp	Uses reported latch to emit alert/log once per unhealthy episode and reset after recovery.
src/dhcp_mon.cpp	Resets relay-health watermarks after counter initialization and after counter-clear synchronization completes.
src/dhcp_device.h	Declares dhcp_device_reset_health_state() API for seeding/resetting relay-health watermarks.
src/dhcp_device.cpp	Implements watermark-based relay disparity logic with mutex-protected per-message flow state and grace-window handling.

[mssonicbld]

/azp run

[azure-pipelines]

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

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (1)

src/dhcp_device.cpp:102

• dhcp_device_reset_health_state() currently clears relay_flow_states for all interfaces/sockets, even though the API and comment describe resetting watermarks for a single interface. This can unintentionally discard per-interface health state if the helper is ever used for more than agg_dev_all (and makes the function behavior surprising). Prefer erasing/reinitializing only the entry for the requested interface (and relevant socket key).

void dhcp_device_reset_health_state(const std::string &ifname)
{
    std::lock_guard<std::mutex> lock(relay_flow_state_mutex);
    relay_flow_states.clear();
    initialize_relay_flow_states(ifname, rx_sock, tx_sock,

[mssonicbld]

/azp run

[azure-pipelines]

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

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (1)

src/dhcp_device.cpp:102

• dhcp_device_reset_health_state() clears relay_flow_states for all interfaces/sockets, but the API/docstring says it resets watermarks for the specified interface. If this helper is ever used for more than one interface (or if additional sockets start using relay_flow_states), calling it for one interface would wipe state for others and can cause incorrect/uninitialized health-window calculations.

void dhcp_device_reset_health_state(const std::string &ifname)
{
    std::lock_guard<std::mutex> lock(relay_flow_state_mutex);
    relay_flow_states.clear();
    initialize_relay_flow_states(ifname, rx_sock, tx_sock,

[mssonicbld]

/azp run

[azure-pipelines]

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

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (1)

src/dhcp_device.cpp:210

• The DHCPv6 positive relay check’s docstring still describes a concrete health check, but the implementation always returns DHCP_MON_STATUS_INDETERMINATE (by design, since client/relay message types differ). Please update the function documentation (and ideally the corresponding enum documentation) to match the actual behavior to avoid operator/developer confusion.

/**
 * @code dhcp_device_check_positive_health_v6(ifname);
 * @brief Check that DHCPv6 relayed messages are being transmitted out of this interface/dev
 *        using its counters. The interface is positively healthy if there are DHCPv6 message
 *        travelling through it.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

[azure-pipelines]

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

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (1)

src/dhcp_device.cpp:168

• In the rx_delta>0 path, if TX activity is also observed in the same window, tx_credit is set to 0. This loses the one-window TX credit that should protect against TX-before-RX observation ordering in the next window whenever any TX is observed. Consider retaining credit when current_tx_activity is true.

                state.tx_credit = 0;

[mssonicbld]

/azp run

[azure-pipelines]

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

[Copilot]

Pull request overview

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

[mssonicbld]

/azp run

[azure-pipelines]

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

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (1)

src/dhcp_device.cpp:113

• The helper documentation block above get_untransmitted_windows() still references the old check_counter_not_transmitted() API and claims it returns a "DHCP relay health status", but the function now returns a per-message-type map of pending (unmatched) windows. This makes the comment misleading for future maintenance.

 * @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
 *        of this interface/device using its counters.
 * @param ifname            interface name
 * @param rx_sock           rx socket

[mssonicbld]

/azp run

[azure-pipelines]

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

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (1)

src/health_check.cpp:239

• In DHCP_MON_STATUS_INDETERMINATE, the code currently increments state_data[i].count. That can trigger false alerts/logs during idle periods (or when counters are unchanged) because the unhealthy counter increases without any UNHEALTHY evidence. It also prevents re-reporting after the count naturally decays back to 0 because reported never resets in this path. Consider decrementing the count toward 0 and clearing reported when the count reaches 0.

            case DHCP_MON_STATUS_INDETERMINATE:
                if (state_data[i].count) {
                    state_data[i].count++;
                }
                break;

[mssonicbld]

/azp run

[azure-pipelines]

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

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

[Xichen96]

Closing after informal scope review. Replaced by smaller PRs: #101 disables invalid DHCPv6 disparity, #102 latches health alerts per episode, #103 tracks unmatched DHCPv4 windows, #104 reports per-message disparity state, and #105 resets disparity state after counter clear.