[TEMP][dhcpmon] Validate Fungible F2 repair stack - #94
Conversation
|
/azp run |
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
Temporary validation stack for dhcpmon “repair” commits targeting Fungible F2 and related topology/health-check correctness. The changes add runtime membership refresh and transactional counter/topology reconciliation while improving relay-disparity robustness and packet-event handling during refresh.
Changes:
- Add CONFIG_DB membership subscribers (VLAN_MEMBER / PORTCHANNEL_MEMBER) and main-thread topology reconciliation with packet-handler quiescing.
- Make relay-disparity detection more resilient (watermarks / grace window / episode reporting latch).
- Improve socket/event infrastructure (event suspend/resume, keepalive event, bounded packet processing per callback) and update aggregate-counter parent resolution.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/util.h | Removes legacy aggregate-counter helper (replaced by devman helper). |
| src/sock_mgr.h | Adds packet-handler suspend/resume and cache-counter pruning API. |
| src/sock_mgr.cpp | Implements keepalive event, suspend/resume, and cache-counter pruning. |
| src/packet_handler.cpp | Uses devman aggregate-counter naming and bounds packets processed per callback. |
| src/health_check.h | Adds per-check “reported” latch state. |
| src/health_check.cpp | Latches unhealthy episode reporting and resets latch on healthy. |
| src/event_mgr.h | Adds suspend/resume APIs for tagged event groups. |
| src/event_mgr.cpp | Implements suspend/resume; fixes fd logging after event_free. |
| src/dhcp_mon.h | Adds dhcp_mon_reconcile_topology() public API. |
| src/dhcp_mon.cpp | Adds CONFIG_DB subscriber events, transactional mapping/counter reconciliation, and refresh flow in health timer. |
| src/dhcp_devman.h | Adds transactional mapping refresh + parent/aggregate helper APIs. |
| src/dhcp_devman.cpp | Implements transactional mapping rebuild and parent/aggregate resolution for nested topologies. |
| src/dhcp_device.h | Adds API to reset relay health watermarks to current counters. |
| src/dhcp_device.cpp | Implements watermark-based disparity detection with grace window and state tracking. |
1ba370a to
e7ef03c
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (3)
src/sock_mgr.cpp:468
sockis unused in this structured binding loop, which will warn under-Wall. Rewrite the loop to avoid binding an unused variable.
int sock_mgr_resume_packet_handler()
{
for (const auto &[sock, info] : sock_map) {
if (info.event_mgr_ptr->resume_all_events(packet_handler_tag) < 0) {
sock_mgr_suspend_packet_handler();
return -1;
}
}
return 0;
}
src/dhcp_device.cpp:104
dhcp_device_reset_health_state(ifname)clearsrelay_flow_statesfor all interfaces/sockets, even though the API name/docs imply an interface-scoped reset. This can unintentionally drop pending relay-disparity state for other interfaces. Prefer erasing only the entry for the requested interface.
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,
(const int *)monitored_msgs, monitored_msg_sz);
}
src/dhcp_mon.cpp:484
- If
sock_mgr_resume_packet_handler()fails,sock_mgr_resume_packet_handler()has already suspended packet events (it callssock_mgr_suspend_packet_handler()on failure). Returning here leaves the daemon running with packet processing permanently disabled. Consider terminating to allow a supervisor restart, or implement an explicit recovery path.
if (topology_refresh_pending) {
sock_mgr_suspend_packet_handler();
int result = dhcp_mon_reconcile_topology();
if (sock_mgr_resume_packet_handler() < 0) {
syslog(LOG_ALERT, "Failed to resume packet handlers after topology refresh");
return;
}
Resolve nested Ethernet-to-PortChannel-to-VLAN membership and aggregate counters at each immediate parent without changing existing Dual-ToR attribution. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 39f979be-d826-4d5c-949a-f20abb58bb83 Signed-off-by: Xichen96 <lukelin0907@gmail.com>
Track DHCPv4 disparity per message type with absolute watermarks, adjacent-window TX credit, main-loop clear reset, and one report per persistent outage; disable the invalid DHCPv6 same-type signal. 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 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>
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>
e7ef03c to
93a2631
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 39f979be-d826-4d5c-949a-f20abb58bb83 Signed-off-by: Xichen96 <lukelin0907@gmail.com>
Reset only the requested interface, include direct container dependencies, and label the alert as DHCPv4-only. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 39f979be-d826-4d5c-949a-f20abb58bb83 Signed-off-by: Xichen96 <lukelin0907@gmail.com>
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>
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>
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Treat a self-mapped VLAN or PortChannel member as a root interface instead of creating a self aggregate. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 39f979be-d826-4d5c-949a-f20abb58bb83 Signed-off-by: Xichen96 <lukelin0907@gmail.com>
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>
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>
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
| uint64_t last_rx; | ||
| uint64_t last_tx; | ||
| uint32_t pending_windows; | ||
| uint8_t tx_credit; | ||
| bool initialized; |
Include family, direction, parent and child-aggregate deltas, expected ratio, and message type in persistent hierarchy mismatch logs for precise loss localization. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 39f979be-d826-4d5c-949a-f20abb58bb83 Signed-off-by: Xichen96 <lukelin0907@gmail.com>
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Reject non-resumable event tags before deletion, roll back partial event_del failures, and surface suspend errors through the socket manager for coherent fail-stop recovery. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 39f979be-d826-4d5c-949a-f20abb58bb83 Signed-off-by: Xichen96 <lukelin0907@gmail.com>
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
| std::vector<struct event *> deleted_events; | ||
| for (const auto &event : tagged_events->second) { | ||
| if (event_del(event) < 0) { | ||
| bool restore_failed = false; | ||
| for (struct event *deleted_event : deleted_events) { | ||
| if (event_add(deleted_event, NULL) < 0) { | ||
| restore_failed = true; | ||
| } | ||
| } | ||
| syslog(LOG_ALERT, "event_mgr: Failed to suspend event (fd=%d) with tag %s for %s", | ||
| event_get_fd(event), tag.c_str(), this->name.c_str()); | ||
| return restore_failed ? -2 : -1; | ||
| } | ||
| deleted_events.push_back(event); | ||
| } | ||
| return 0; |
Evaluate both an interface and its immediate-child aggregate before declaring a quiet window, so child-only packet observations expose the exact forwarding edge that did not advance. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 39f979be-d826-4d5c-949a-f20abb58bb83 Signed-off-by: Xichen96 <lukelin0907@gmail.com>
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
Temporary validation stack is complete. Regular master regression and F2 hardware evidence were captured; this PR must not merge. Focused fixes remain in the component PRs. |
Description of PR
Temporary CI/hardware validation stack for the Fungible F2 dhcpmon repairs.
Do not review or merge this PR.
Included signed commits:
PortChannel/VLAN hierarchy.
detection.
quiescing.
topology/counter reconciliation.
listener.
The conflict resolution preserves PR #89's single-ToR
Ethernet -> PortChannel -> VLANmapping while making PR #92's map rebuildtransactional.
Work item tracking
Type of change
Approach
What is the motivation for this PR?
Produce one Azure-CI-built dhcpmon artifact containing the exact final repair
stack for regular BJW and Fungible F2 hardware validation.
How did you do it?
Cherry-pick the five focused signed commits onto current master and resolve
only their expected integration overlap.
How did you verify/test it?
Pending Azure PR CI and hardware validation.
Any platform specific information?
No.
Back port request
None.
Tested branch
Pending.
Test result
Pending.
Documentation
Not applicable.