[dhcpmon] Validate DHCPv6 relay transformations

[Xichen96]

Description of PR

Replace the invalid DHCPv6 same-message-type disparity check with relay-aware
forward and return-path validation.

Work item tracking

• Microsoft ADO (number only): 38615655

Type of change

• Bug fix

Approach

DHCPv6 transforms client messages at the relay boundary: for example, Solicit
becomes Relay-Forward and Relay-Reply becomes Advertise/Reply. Comparing the
same message type on RX and TX cannot reliably indicate relay loss.

The positive DHCPv6 relay-health check now validates:

• RX Solicit, Request, or Relay-Forward produces at least one TX Relay-Forward.
• RX Relay-Reply produces at least one TX Advertise, Reply, or Relay-Reply.

The check compares activity rather than magnitude so multiple configured DHCPv6
servers remain valid. Configured-server fan-out validation is intentionally
separate. DHCPv6 counters and interface-hierarchy checks remain unchanged.

Stack

Depends on #100. Review the final commit only:

95df31b [dhcpmon]: Validate DHCPv6 relay transformations

Verification

No local compilation was used. Azure PR CI will validate the updated branch.

Back port request

None. This targets master only.

[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

This PR disables the DHCPv6 “same-message-type relay-disparity” positive health signal in dhcpmon by making the DHCPv6 positive-health check return DHCP_MON_STATUS_INDETERMINATE, reflecting that DHCPv6 message types legitimately change across the relay boundary.

Changes:

• Updated the DHCPv6 positive-health check to always return INDETERMINATE instead of comparing RX/TX counters.
• Added an in-code rationale noting DHCPv6 client vs relay message-type differences across the relay boundary.

[mssonicbld]

/azp run

[azure-pipelines]

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

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (1)

src/dhcp_device.cpp:122

• The Doxygen @code example shows dhcp_device_check_positive_health_v6() with no arguments, but the function still takes a const std::string& parameter and is called with ifname elsewhere in this file. This mismatch can confuse readers and break generated docs.

/**
 * @code dhcp_device_check_positive_health_v6();
 * @brief Same-message-type RX/TX comparison is not valid across the DHCPv6 relay boundary.
 * @return DHCP_MON_STATUS_INDETERMINATE

[mssonicbld]

/azp run

[azure-pipelines]

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

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (1)

src/dhcp_device.cpp:124

• The Doxygen block documents dhcp_device_check_positive_health_v6() as parameterless, but the function still takes a const std::string& and is called with ifname later in this file. This makes the docs misleading and the signature (unnamed parameter) harder to understand; it can also trigger Doxygen @code/signature mismatches. Consider naming the parameter, documenting it as unused, and explicitly marking it unused in the body to avoid compiler warnings.

 * @code dhcp_device_check_positive_health_v6();
 * @brief Same-message-type RX/TX comparison is not valid across the DHCPv6 relay boundary.
 * @return DHCP_MON_STATUS_INDETERMINATE
 */
static dhcp_mon_status_t dhcp_device_check_positive_health_v6(const std::string &)

[mssonicbld]

/azp run

[azure-pipelines]

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

[Copilot]

Pull request overview

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

[mssonicbld]

/azp run

[azure-pipelines]

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

[mssonicbld]

/azp run

[azure-pipelines]

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

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 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 9 out of 9 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

src/dhcp_devman.h:148

• The updated Doxygen for dhcp_devman_get_device_context() no longer documents that the function can return NULL when an interface is unmapped, but the implementation still returns NULL in that case (e.g., when no parent mapping exists or when MAX_CONTEXT_DEPTH is exceeded). This can mislead callers and generates inaccurate API docs.

 * @brief find the tracked input interface that owns an interface.
 *
 * @param ifname           interface name
 *
 * @return The interface's tracked context; a tracked input interface returns its own context

[mssonicbld]

/azp run

[azure-pipelines]

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

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (2)

src/dhcp_device.cpp:125

• get_counter_delta() subtracts unsigned uint64_t counters directly. If counters ever reset/are cleared between snapshots (e.g. via counter-clear / DB sync), this underflows and produces a huge “delta”, causing false positives in check_counter_increased() and DHCPv6 relay/aggregate health decisions. Use a saturating delta (0 when current < snapshot) to preserve the previous “only count increases” semantics.

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);

src/health_check.cpp:31

• relay_disparity_error says “none transmitted”, but the underlying positive checks mark UNHEALTHY when expected relay output is missing (same-type for v4; relay-forward / advertise-reply transformations for v6). It can be triggered even when some TX traffic exists, which makes this error message misleading for operators.

static const char relay_disparity_error[] =
    "dhcpmon detected DHCPv4/v6 packets received but none transmitted for intf: %s. Duration: %d (sec)";

[mssonicbld]

/azp run

[azure-pipelines]

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

[mssonicbld]

/azp run

[azure-pipelines]

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

[Copilot]

Pull request overview

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

Suppressed comments (2)

src/dhcp_device.cpp:237

• check_counters_delta_expected() (used by the aggregate disparity checks) still derives deltas via unsigned subtraction inside its loop. If counters ever decrease (e.g., after a COUNTERS_DB sync/clear), those subtractions underflow and can produce false disparities. Use a safe delta (clamp negative to 0), ideally by reusing get_counter_delta().

static bool check_counters_delta_expected(const std::string &ifname, const std::string &other_ifname, int sock,
                                         size_t ratio, const int *monitored_msgs, size_t monitored_msg_cnt)
{
    const sock_info_t &sock_info = sock_mgr_get_sock_info(sock);
    const counter_t &counters = sock_info.all_counters.at(ifname);

src/dhcp_device.cpp:126

• get_counter_delta() subtracts the snapshot from the current counter without guarding against counter resets/DB resync. Cache counters can be rewritten from COUNTERS_DB (see update_cache_counter_callback) without updating snapshots, so current may be less than snapshot and this subtraction will underflow, producing a huge delta and false health-check results. Clamp negative deltas to 0 (or update snapshots when syncing).

This issue also appears on line 233 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);
}