Skip to content

[dhcpmon] Identify parent and aggregate counter disparities - #100

Draft
Xichen96 wants to merge 3 commits into
sonic-net:masterfrom
Xichen96:dev/xichenlin/log-hierarchy-mismatch-edge
Draft

[dhcpmon] Identify parent and aggregate counter disparities#100
Xichen96 wants to merge 3 commits into
sonic-net:masterfrom
Xichen96:dev/xichenlin/log-hierarchy-mismatch-edge

Conversation

@Xichen96

@Xichen96 Xichen96 commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Description of PR

Report the exact parent interface and aggregate counter whose values diverged.

Dependency:

Review the final commit only:

831de60 [dhcpmon]: Track parent and aggregate counters per interface

Work item tracking
  • Microsoft ADO (number only): 38615656

Type of change

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

Approach

What is the motivation for this PR?

The existing aggregate counter alert says only that a VLAN/PortChannel
aggregate does not match. In the F2 path, operators must distinguish:

physical member -> PortChannel -> VLAN SVI

without interpreting a pre-VLAN loss as a relay drop.

How did you do it?

  • Evaluate activity on both the parent interface and its member-interface
    aggregate, so a child-only increment is not treated as an idle window.
  • Populate one health state per discovered VLAN or PortChannel check, so each
    parent/aggregate pair has an independent persistence counter.
  • Initialize those states explicitly after topology and counter discovery.
  • Use four aggregate checks (IPv4/IPv6 RX/TX); derive the expected ratio from
    the interface topology instead of encoding equal/multiple in the check type.
  • Store an error format in each health state.
  • Keep only the event callback in health state; the threshold loop logs the
    format directly with the interface name and duration. The internal
    Agg-* counter name is not exposed in the user-facing message.
  • Remove the category-wide aggregate wrappers and global last-mismatch state.

How did you verify/test it?

On F2 hardware with PortChannel1005 down due to min_links, packets reached
the PortChannel but not Vlan1000. The equivalent comparison identified:

Parent interface: Vlan1000, member aggregate: Agg-Vlan1000-Vlan1000

No relay-disparity event was emitted for that pre-VLAN loss.

Fresh exact-head Azure PR CI is pending. No local compilation is used.

Any platform specific information?

No.

Back port request

None. This targets master only.

Tested branch

master, plus supplemental F2 topology validation with master-target PR
artifacts on SONiC 202503.

Test result

The equivalent parent/aggregate comparison identified the expected counters on
F2 hardware, and the traditional master DHCPv4/DHCPv6 regression passed
unchanged. Exact-head acceptance awaits the fresh PR build.

Documentation

Not applicable.

Copilot AI review requested due to automatic review settings July 26, 2026 06:42
@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

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

@azure-pipelines

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

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR enhances dhcpmon’s hierarchy mismatch alerting by recording and reporting the specific parent → immediate-child aggregate edge where DHCP counter deltas diverge, improving operator diagnostics in nested interface hierarchies (e.g., Ethernet -> PortChannel -> VLAN SVI).

Changes:

  • Switch aggregate-counter attribution to “immediate parent” via new dhcp_devman_get_parent_ifname() / dhcp_devman_get_agg_counter_ifname().
  • Capture and expose detailed mismatch context (IP family, direction, parent/child deltas, expected ratio, message type) and append it to existing hierarchy syslog alerts.
  • Treat “child-only activity” as non-idle for hierarchy checks to avoid masking mismatches during the evaluation window.

Reviewed changes

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

Show a summary per file
File Description
src/util.h Removes the old inline aggregate-counter helper in favor of devman-owned logic.
src/packet_handler.cpp Updates cache counter aggregation to use devman immediate-parent aggregate naming.
src/health_check.cpp Appends last recorded hierarchy-mismatch detail to existing alert syslogs.
src/dhcp_mon.cpp Updates aggregate-counter recalculation to use devman immediate-parent aggregation.
src/dhcp_devman.h Adds APIs to retrieve immediate parent and corresponding aggregate-counter name.
src/dhcp_devman.cpp Implements parent/aggregate lookup and adjusts PortChannel-member mapping for nested hierarchies.
src/dhcp_device.h Exposes accessor for the most recent hierarchy mismatch detail string.
src/dhcp_device.cpp Records mismatch detail on first delta divergence; updates hierarchy health evaluation logic.
Comments suppressed due to low confidence (1)

src/dhcp_device.cpp:241

  • check_aggregate_health() takes ratio as uint8_t, but callers pass readonly_access(...).size() sums (size_t). If the parent has >255 children (e.g., large VLAN membership), the value truncates and can produce incorrect health results and misleading expected_ratio in mismatch logs. Use a wider type (e.g., size_t) for ratio here (and in check_counters_delta_expected).
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)
{

Comment thread src/dhcp_device.cpp Outdated
Copilot AI review requested due to automatic review settings July 26, 2026 07:28
@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

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

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Copilot AI review requested due to automatic review settings July 26, 2026 07:53
@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

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

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Comment thread src/dhcp_devman.cpp
Comment thread src/dhcp_device.cpp Outdated
Copilot AI review requested due to automatic review settings July 26, 2026 08:07
@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

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

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

@Xichen96 Xichen96 changed the title [dhcpmon] Identify hierarchy mismatch edge [dhcpmon] Identify where packets stop in the interface hierarchy Jul 27, 2026
Copilot AI review requested due to automatic review settings July 27, 2026 03:33
@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

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

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Copilot AI review requested due to automatic review settings July 27, 2026 03:58
@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@Xichen96
Xichen96 force-pushed the dev/xichenlin/log-hierarchy-mismatch-edge branch from 89400cf to 257b6b3 Compare July 28, 2026 05:25
Copilot AI review requested due to automatic review settings July 28, 2026 05:25
@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

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

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Comment thread src/dhcp_devman.cpp
Copilot AI review requested due to automatic review settings July 28, 2026 05:40
@Xichen96
Xichen96 force-pushed the dev/xichenlin/log-hierarchy-mismatch-edge branch from 257b6b3 to 8c0b84e Compare July 28, 2026 05:40
@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

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

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Copilot AI review requested due to automatic review settings July 28, 2026 05:58
@Xichen96
Xichen96 force-pushed the dev/xichenlin/log-hierarchy-mismatch-edge branch from 8c0b84e to 3ab0905 Compare July 28, 2026 05:58
@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

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

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

@Xichen96
Xichen96 force-pushed the dev/xichenlin/log-hierarchy-mismatch-edge branch from 3ab0905 to 6cf80d6 Compare July 31, 2026 17:57
@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

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

Xichen96 and others added 3 commits August 1, 2026 05:01
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>
@Xichen96
Xichen96 force-pushed the dev/xichenlin/log-hierarchy-mismatch-edge branch from 6cf80d6 to 831de60 Compare July 31, 2026 19:20
@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

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

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Suppressed comments (1)

src/health_check.cpp:110

  • Aggregate-disparity threshold logs only include the parent interface name (via state.ifname), but the PR goal is to report both the parent interface and the specific member-aggregate counter that diverged. Right now operators still can’t see which aggregate counter (e.g., Agg--) was compared.
                if (++state.count > dhcp_unhealthy_max_count) {
                    int duration = state.count * window_interval_sec;
                
                    if (state.alert) {
                        state.alert(duration);
                    }
                    syslog(LOG_ALERT, state.error_format, state.ifname.c_str(), duration);
                }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants