Skip to content

[dhcpmon] Listen for membership updates - #93

Closed
Xichen96 wants to merge 28 commits into
sonic-net:masterfrom
Xichen96:dev/xichenlin/live-membership-listener
Closed

[dhcpmon] Listen for membership updates#93
Xichen96 wants to merge 28 commits into
sonic-net:masterfrom
Xichen96:dev/xichenlin/live-membership-listener

Conversation

@Xichen96

@Xichen96 Xichen96 commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Description of PR

Summary:

dhcpmon currently reads VLAN_MEMBER and PORTCHANNEL_MEMBER only at
startup. Adding, removing, or reassigning a member leaves packet attribution
and counter inventory stale until the process is restarted.

This change registers both SWSS subscriber file descriptors directly with the
existing main libevent loop. Notifications mark topology refresh pending. On
the next health tick, packet handlers are suspended, the transactional
membership/counter reconciliation runs on the main thread, handlers resume,
and a successfully applied mixed-state health window is skipped. A failed
reconciliation rolls back and continues health checks against the last valid
topology. The process PID remains unchanged.

This is a stacked draft:

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

Type of change

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

Approach

What is the motivation for this PR?

Fungible F2 validation includes PortChannel/member add, delete, and
reassignment scenarios. Requiring a service restart loses counters and creates
an avoidable monitoring gap.

How did you do it?

  • Create SubscriberStateTable instances for VLAN_MEMBER and
    PORTCHANNEL_MEMBER.
  • Register their selectable file descriptors as persistent events on the main
    libevent base.
  • Drain notification batches in the main-thread callbacks and coalesce them
    into one pending refresh.
  • Recover failed subscriber FDs by suspending/removing tagged config events,
    rebuilding subscribers, and requiring a full reconciliation.
  • Suspend packet callbacks, invoke the main-thread reconciliation API, and
    resume packet callbacks with no process restart.
  • Defer refresh while counter clear is active.
  • Skip a health window after topology state is successfully applied, so
    old/new counters are never compared together. A rolled-back failure keeps
    refresh pending while health checks continue on the prior topology.
  • Reconcile once synchronously after subscriptions are registered and before
    socket event loops start, closing the startup notification gap.

How did you verify/test it?

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

Planned validation keeps the same dhcpmon PID while:

  • adding/removing a physical PortChannel member;
  • adding/removing a PortChannel from the monitored VLAN;
  • deleting/recreating the same PortChannel with a new ifindex;
  • adding a previously unseen PortChannel and members;
  • running counter clear during membership churn.

Any platform specific information?

No.

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; no command or counter schema changes.

Xichen96 added 2 commits July 25, 2026 19:42
Allow packet handlers to be suspended and resumed without terminating socket event loops, and bound each callback batch so quiescing completes under sustained traffic.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 39f979be-d826-4d5c-949a-f20abb58bb83
Signed-off-by: Xichen96 <lukelin0907@gmail.com>
Rebuild membership maps transactionally and reconcile cache and COUNTERS_DB state while preserving unchanged interface counters and rolling back failures.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 39f979be-d826-4d5c-949a-f20abb58bb83
Signed-off-by: Xichen96 <lukelin0907@gmail.com>
Copilot AI review requested due to automatic review settings July 25, 2026 21:05
@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).

Xichen96 added 2 commits July 25, 2026 21:07
Register VLAN_MEMBER and PORTCHANNEL_MEMBER subscriber file descriptors on the main event loop and apply quiesced transactional refreshes without restarting dhcpmon.

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/live-membership-listener branch from 4e9274d to 049ef7c Compare July 25, 2026 21: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

This PR updates dhcpmon to react to runtime CONFIG_DB membership changes (VLAN_MEMBER / PORTCHANNEL_MEMBER) without requiring a process restart, keeping interface attribution and counter inventory consistent as topology changes.

Changes:

  • Registers CONFIG_DB membership subscribers directly on the main libevent loop and coalesces notifications into a pending topology refresh.
  • Adds packet-handler quiesce/resume support and bounds packet processing per callback to avoid indefinite drain under sustained traffic.
  • Introduces a main-thread topology reconciliation path that refreshes membership mappings and reconciles cache/DB counters transactionally.

Reviewed changes

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

Show a summary per file
File Description
src/sock_mgr.h Adds APIs for suspending/resuming packet handlers and pruning cache counters by valid interface set.
src/sock_mgr.cpp Implements packet-handler suspend/resume, adds a keepalive event per socket event base, and adds cache-counter pruning.
src/packet_handler.cpp Bounds packet processing per callback to a fixed batch size.
src/event_mgr.h Adds event suspend/resume APIs by tag.
src/event_mgr.cpp Implements tagged suspend/resume and fixes fd logging during delete/free.
src/dhcp_mon.h Adds dhcp_mon_reconcile_topology() API contract for main-thread reconciliation.
src/dhcp_mon.cpp Wires CONFIG_DB subscriber FDs into the main loop and runs reconcile on the health tick with packet quiescing.
src/dhcp_devman.h Declares a new transactional mapping refresh API.
src/dhcp_devman.cpp Refactors membership mapping rebuild into dhcp_devman_refresh_mappings() and clears reverse maps on shutdown.
Comments suppressed due to low confidence (1)

src/dhcp_mon.cpp:887

  • dhcp_mon_start() treats any non-zero return from dhcp_mon_reconcile_topology() as fatal. But that function returns 1 to indicate "counter clear active; defer reconciliation" (not an error). In that case, startup should continue with topology_refresh_pending left true so the next health tick can retry, rather than tearing down the main events.
    topology_refresh_pending = true;
    sock_mgr_suspend_packet_handler();
    if (dhcp_mon_reconcile_topology() != 0 || sock_mgr_resume_packet_handler() < 0) {
        goto unregister_main_events;
    }

Comment thread src/dhcp_mon.cpp Outdated
Copilot AI review requested due to automatic review settings July 25, 2026 21:09

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_mon.cpp

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 3 comments.

Comment thread src/dhcp_mon.cpp
Comment thread src/dhcp_devman.cpp
Comment thread src/event_mgr.cpp
Continue checking the last topology while counter clear or subscriber recovery delays a pending refresh.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 39f979be-d826-4d5c-949a-f20abb58bb83
Signed-off-by: Xichen96 <lukelin0907@gmail.com>
Copilot AI review requested due to automatic review settings July 25, 2026 23:15
@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.

Comments suppressed due to low confidence (2)

src/dhcp_mon.cpp:480

  • If resuming packet handlers fails, the callback returns while packet handlers remain suspended, leaving dhcpmon running but no longer processing packets. Consider failing fast (so a supervisor restarts the daemon) or implementing a recovery path that re-registers packet events before continuing.
        if (sock_mgr_resume_packet_handler() < 0) {
            syslog(LOG_ALERT, "Failed to resume packet handlers after topology refresh");
            return;
        }

src/event_mgr.cpp:103

  • resume_all_events() re-adds events with a NULL timeout, which will drop any previously configured timeout for timer-based events in this tag (e.g., EV_PERSIST timers added via add_event(..., &timeval, tag)). Either store/restore the original timeout per event, or document/guard that suspend/resume is only valid for fd-driven events (no timeouts).
int event_mgr::resume_all_events(const std::string &tag)
{
    for (const auto &event : this->event_map[tag]) {
        if (event_add(event, NULL) < 0) {
            this->suspend_all_events(tag);

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 2 comments.

Comments suppressed due to low confidence (1)

src/dhcp_mon.cpp:892

  • dhcp_mon_start() treats any non-zero return from dhcp_mon_reconcile_topology() as fatal. However dhcp_mon_reconcile_topology() explicitly returns 1 when counter clear is active (defer). On that path, startup will abort even though the daemon could continue running with the last-known topology and retry reconciliation on later health ticks.
    topology_refresh_pending = true;
    sock_mgr_suspend_packet_handler();
    if (dhcp_mon_reconcile_topology() != 0 || sock_mgr_resume_packet_handler() < 0) {
        goto unregister_main_events;
    }
    topology_refresh_pending = false;

Comment thread src/dhcp_mon.cpp
Comment thread src/dhcp_mon.cpp
Reset sockaddr length for each batch receive and reject timeout events from the fd-only resume path.

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 added 2 commits July 26, 2026 13:27
Limit the quiesced main-thread reconciliation helper to dhcp_mon.cpp so external callers cannot bypass its synchronization contract.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 39f979be-d826-4d5c-949a-f20abb58bb83
Signed-off-by: Xichen96 <lukelin0907@gmail.com>
Iterate through counter-map entries directly where only sock_info is needed, keeping the reconciliation build warning-free.

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

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 1 comment.

Comment thread src/dhcp_mon.cpp
Copilot AI review requested due to automatic review settings July 26, 2026 03:29

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 1 comment.

Comment thread src/dhcp_mon.cpp Outdated
Copy counter maps while holding counter-state and DB synchronization locks, then release packet callbacks before Redis I/O while retaining DB serialization through writeback.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 39f979be-d826-4d5c-949a-f20abb58bb83
Signed-off-by: Xichen96 <lukelin0907@gmail.com>
Copilot AI review requested due to automatic review settings July 26, 2026 03:56
@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.

Drain raw socket receive buffers while packet handlers remain suspended after a successful topology commit, preventing pre-change packets from being attributed with new mappings.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 39f979be-d826-4d5c-949a-f20abb58bb83
Signed-off-by: Xichen96 <lukelin0907@gmail.com>
Copilot AI review requested due to automatic review settings July 26, 2026 04:16
@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

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

Xichen96 added 2 commits July 26, 2026 14:17
Validate snapshot socket keys before looking up metadata so malformed or stale caller data cannot throw from sock_map.at().

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 39f979be-d826-4d5c-949a-f20abb58bb83
Signed-off-by: Xichen96 <lukelin0907@gmail.com>
Reconcile current and snapshot counter maps independently so operator[]-created snapshot entries cannot survive after an interface leaves the tracked topology.

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

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 04:20

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

Copy link
Copy Markdown
Contributor Author

Closing after architecture review. Full CONFIG_DB listening is deferred until dhcpmon is first CONFIG_DB-driven for a single VLAN and then multi-VLAN. This listener draft skips those prerequisites and will not be merged as-is.

@Xichen96 Xichen96 closed this Jul 26, 2026
@Xichen96
Xichen96 deleted the dev/xichenlin/live-membership-listener branch July 26, 2026 05:50
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