Skip to content

Conversation

@Xichen96
Copy link

@Xichen96 Xichen96 commented Nov 17, 2025

Currently dhcpmon only supports dhcpv4/ipv4

@mssonicbld
Copy link
Collaborator

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Xichen96 Xichen96 changed the title Add support for ipv6 Add support for dhcpv6 Nov 17, 2025
@Blueve Blueve requested a review from Copilot November 18, 2025 03:07
Copilot finished reviewing on behalf of Blueve November 18, 2025 03:10
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request adds comprehensive support for DHCPv6 to dhcpmon, which previously only supported DHCPv4. The changes include adding DHCPv6 packet handling, separate RX/TX sockets for IPv6, DHCPv6 message type definitions, counter management, health checks, and database operations for DHCPv6 counters.

Key changes:

  • Introduced DHCPv6 message types and packet handlers for both RX and TX directions
  • Added separate socket management for IPv4 and IPv6 DHCP traffic (4 sockets total: rx_sock, tx_sock, rx_sock_v6, tx_sock_v6)
  • Implemented DHCPv6-specific validation logic including relay message handling and option parsing

Reviewed Changes

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

Show a summary per file
File Description
src/util.h Added DHCPv6 counter table prefix, helper functions for checksums, address string generation, and debug logging
src/util.cpp Implemented checksum calculation functions for IP/IPv6 and UDP, JSON generation, and address formatting utilities
src/sock_mgr.h/cpp New socket manager to handle 4 separate sockets (rx/tx for v4/v6), event management, and counter operations
src/packet_handler.h/cpp New packet handler module with DHCPv6 message validation logic and packet processing for all 4 socket types
src/main.cpp Updated command-line parsing and initialization flow to use new socket manager
src/health_check.h/cpp Extracted health check logic into separate module with DHCPv6 health checks
src/event_mgr.h/cpp Enhanced event manager with tagging support for event grouping
src/dhcp_mon.h/cpp Refactored to use socket manager and support DHCPv6 counter updates and health monitoring
src/dhcp_devman.h/cpp Updated device manager to support IPv6 addresses, vlan/portchannel mapping, and DHCPv6 operations
src/dhcp_device.h/cpp Added DHCPv6 message types, health check types, and counter operations for IPv6
Comments suppressed due to low confidence (1)

src/dhcp_devman.cpp:1

  • The condition logic is incorrect. The intention appears to be ensuring exactly 1 IPv4 address AND exactly 1 IPv6 GUA AND exactly 1 IPv6 LLA. The current condition (num_ipv6_gua != 1 && num_ipv6_lla != 1) will pass if either GUA or LLA equals 1, which is not the intended behavior. It should be (num_ipv6_gua != 1 || num_ipv6_lla != 1) to require both to be exactly 1.
/**

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/util.cpp Outdated
res.reserve(300);
res.append("{");
for (int i = 0; i < message_type_count; i++) {
std::string value = std::to_string(counter == nullptr ? 0 : counter->at(i));
Copy link

Copilot AI Nov 18, 2025

Choose a reason for hiding this comment

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

Accessing counter->at(i) when counter is not nullptr can throw std::out_of_range exception if the key i doesn't exist in the map. Consider using counter->find(i) or providing a default value using the readonly_access helper function defined in the header.

Suggested change
std::string value = std::to_string(counter == nullptr ? 0 : counter->at(i));
uint64_t val = 0;
if (counter != nullptr) {
auto it = counter->find(i);
if (it != counter->end()) {
val = it->second;
}
}
std::string value = std::to_string(val);

Copilot uses AI. Check for mistakes.
@Xichen96 Xichen96 force-pushed the dev/xichenlin/add-ipv6 branch from bc69f6d to e80d843 Compare November 18, 2025 17:55
@mssonicbld
Copy link
Collaborator

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Xichen96 Xichen96 force-pushed the dev/xichenlin/add-ipv6 branch from e80d843 to 94f5b80 Compare November 18, 2025 18:00
@mssonicbld
Copy link
Collaborator

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link
Collaborator

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Xichen96 Xichen96 force-pushed the dev/xichenlin/add-ipv6 branch from 3348ba5 to bb16249 Compare November 25, 2025 15:16
@mssonicbld
Copy link
Collaborator

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

* The difference with DHCPv4 and v6 is that, first, DHCPv6 has relay and non-relay packets. Second, with DHCPv6, it is easy to tell that downstream
* is client or relay because they would have different msg type (non-relay and relay), and thus handled by different check profiles. However
* with DHCPv4, client and relay send packets of the same msg type, so we can't just mix them together. We only expect downstream client on T0 topo.
* With T1+ topo, we might have relay downstream as well.
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the T1+ scenario? The 2nd/3rd hop of DHCP relay?

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