Skip to content

[dhcpmon] Start only available address families - #111

Open
Xichen96 wants to merge 1 commit into
sonic-net:masterfrom
Xichen96:dev/xichenlin/support-single-ip-family
Open

[dhcpmon] Start only available address families#111
Xichen96 wants to merge 1 commit into
sonic-net:masterfrom
Xichen96:dev/xichenlin/support-single-ip-family

Conversation

@Xichen96

@Xichen96 Xichen96 commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Why I did it

dhcpmon currently requires every non-management interface to have both a primary IPv4 address and an IPv6 GUA or link-local address. It also opens IPv4 and IPv6 socket pairs and initializes both protocol families unconditionally.

That makes the whole monitor exit when a VLAN or uplink is truly IPv4-only or IPv6-only, even though packet capture, counters, and health checks are already implemented independently for each family.

This change is a prerequisite for sonic-net/sonic-buildimage#27277 to generate monitors for DHCPv4-only and DHCPv6-only VLANs.

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

How I did it

  • Collect zero or one IPv4, IPv6 GUA, and IPv6 LLA from each non-management interface; reject only ambiguous multiple-address results.
  • Zero unavailable address fields so deferred family checks cannot consume uninitialized data.
  • Enable IPv4 when the downstream VLAN has an IPv4 address.
  • Enable IPv6 only when the downstream VLAN has both the GUA and LLA required by dhcp6relay.
  • In Dual-ToR, disable only the family whose required Loopback0 relay address is unavailable.
  • Open and register only the enabled IPv4 and/or IPv6 raw-socket pairs.
  • Reset both IPv4 and IPv6 COUNTERS_DB tables to zero at every startup so disabled families cannot expose stale counts.
  • Preserve the existing CLI clear-counter handshake for both families; a disabled family acknowledges cache reload as an immediate no-op because it has no in-memory cache.
  • Limit debug counters to enabled families and make unchanged detection family-specific so disabled-family health checks remain indeterminate.

Expected startup matrix:

Downstream VLAN addressing IPv4 monitoring IPv6 monitoring
IPv4 only Started Not started
IPv6 GUA + LLA only Not started Started
IPv4 + IPv6 Started Started
Neither Reject configuration

How to verify it

  • Public PR checks are pending on head 6b69a12ac2.
  • Prior-head Azure build 1180642 passed on amd64, arm64, and armhf; replacement checks are pending.
  • No local compilation or package build was run.
  • After this source PR is available in an image, the dependent buildimage and sonic-mgmt changes will validate IPv4-only, IPv6-only, and dual-stack process/counter behavior.

Which release branch to backport (provide reason below if selected)

  • 202305
  • 202311
  • 202405
  • 202411
  • 202505
  • 202511

Tested branch (Please provide the tested image version)

  • master - public PR checks pending on head 6b69a12ac2

Description for the changelog

Start dhcpmon monitoring only for address families available on the downstream VLAN.

Link to config_db schema for YANG module changes

N/A - no schema change.

A picture of a cute animal (not mandatory but encouraged)

N/A

Copilot AI review requested due to automatic review settings July 31, 2026 19:24
@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 updates dhcpmon to start monitoring only for the IP address families that are actually available on the downstream VLAN, instead of requiring both IPv4 and IPv6 on every monitored interface. This aligns socket creation, health checks, and DB/state publishing with the enabled families to support IPv4-only or IPv6-only VLAN deployments.

Changes:

  • Add dhcpv4_enabled / dhcpv6_enabled as device-manager-derived feature flags and use them to gate socket-pair initialization and registration.
  • Gate health-check execution, counter DB initialization/cleanup, and state publishing to only run for enabled families.
  • Allow interfaces to have IPv4-only, IPv6-only (GUA or LLA), or dual-stack addressing; zero out missing address fields to avoid stale/uninitialized data.

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 Document “disabled family” sockets as -1.
src/sock_mgr.cpp Initialize sockets to -1; open/close/register only enabled-family socket pairs; compile/attach BPF only for sockets in sock_map.
src/health_check.h Add is_v6 to health-check state to support family gating.
src/health_check.cpp Skip health checks for disabled families based on dhcpv4_enabled/dhcpv6_enabled.
src/dhcp_mon.cpp Gate COUNTERS_DB init/checks, stale cleanup, debug printing, and STATE_DB updates to enabled families; remove inactive-family DB/state entries.
src/dhcp_devman.h Export enabled-family flags for downstream VLAN.
src/dhcp_devman.cpp Derive enabled families from downstream VLAN addressing; enforce Loopback0 requirements only for enabled families.
src/dhcp_device.h Clarify that per-interface address fields can be “zero/unavailable”.
src/dhcp_device.cpp Zero address fields before discovery; relax address requirements to “at least one family, at most one primary per type”.

Comment thread src/dhcp_devman.cpp Outdated
Copilot AI review requested due to automatic review settings August 1, 2026 05:05
@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.

Suppressed comments (2)

src/dhcp_mon.cpp:183

  • cleanup_stale_db_counters() still iterates both families and only deletes entries for interfaces that are no longer tracked. When an address family is disabled (dhcpv4_enabled/dhcpv6_enabled false), the inactive-family COUNTERS_DB tables for tracked interfaces are not removed, which contradicts the PR goal of removing stale inactive-family counter entries.
 * @code              cleanup_stale_db_counters();
 * @brief             Clean up stale counter entries for interfaces that are no longer present
 * @param             none
 * @return            none
 */

src/dhcp_mon.cpp:113

  • PR description says only enabled address families should have COUNTERS_DB tables initialized, but initialize_db_counters() still unconditionally creates both IPv4 and IPv6 tables. This leaves inactive-family tables present (and can confuse consumers that interpret presence as enabled). Consider initializing only the enabled-family tables here.

This issue also appears on line 179 of the same file.

    table_name = construct_counter_db_table_key(ifname, false);
    init_value = generate_json_string(NULL, DHCP_MESSAGE_TYPE_COUNT, db_counter_name);
    mCountersDbPtr->hset(table_name, "RX", init_value);
    mCountersDbPtr->hset(table_name, "TX", init_value);

@Xichen96
Xichen96 requested review from Copilot August 1, 2026 05:35
@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

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

@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 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.

Suppressed comments (1)

src/dhcp_device.cpp:530

  • The new LOG_ALERT message is ambiguous: it fires both when an interface has no addresses and when it has too many (e.g., multiple primary IPv4s / multiple GUAs/LLAs). Including the observed counts in the alert makes diagnosis much easier, especially since this condition makes dhcpmon exit for that interface context.
            syslog(LOG_ALERT, "Interface %s has no usable IPv4 or IPv6 address", context->intf);

Copilot AI review requested due to automatic review settings August 1, 2026 05:40
@Xichen96
Xichen96 force-pushed the dev/xichenlin/support-single-ip-family branch from f300a1e to 7de55cb Compare August 1, 2026 05:41
@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

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

@Xichen96
Xichen96 force-pushed the dev/xichenlin/support-single-ip-family branch from 7de55cb to d3e8741 Compare August 1, 2026 05:44
@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

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

Copilot AI review requested due to automatic review settings August 1, 2026 15:36
@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/support-single-ip-family branch from 37083fc to a07e86c Compare August 1, 2026 15:43
Copilot AI review requested due to automatic review settings August 1, 2026 15:43
@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.

@azure-pipelines

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

1 similar comment
@azure-pipelines

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

@Xichen96

Xichen96 commented Aug 2, 2026

Copy link
Copy Markdown
Contributor Author

/azpw run

@mssonicbld

Copy link
Copy Markdown
Collaborator

⚠️ Notice: /azpw run only runs failed jobs now. If you want to trigger a whole pipline run, please rebase your branch or close and reopen the PR.
💡 Tip: You can also use /azpw retry to retry failed jobs directly.

Retrying failed(or canceled) jobs...

@Xichen96

Xichen96 commented Aug 2, 2026

Copy link
Copy Markdown
Contributor Author

/azpw retry

@mssonicbld

Copy link
Copy Markdown
Collaborator

Retrying failed(or canceled) jobs...

@azure-pipelines

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

@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

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

@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

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

@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.

Validate IPv4 and IPv6 prerequisites independently, log and skip unavailable families, and start only their sockets, cache counters, state coordination, debug output, and health checks while resetting both database counter families at startup.

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

Copilot-Session: 0dc64b79-c313-4ee3-81f0-dbcb8daf1681
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 9 out of 9 changed files in this pull request and generated no new comments.

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.

Suppressed comments (1)

src/dhcp_mon.cpp:229

  • update_disabled_cache_counter() writes to STATE_DB without taking db_sync_mutex. Other clear-counter synchronization writes (e.g., update_cache_counter_callback and the SIGUSR1 handler) perform STATE_DB writes under db_sync_mutex, so this new unlocked path can race concurrent DB operations from per-socket event threads during a clear-counter sync.
static void update_disabled_cache_counter()
{
    if (!dhcpv4_enabled) {
        std::string state_key = STATE_DB_COUNTER_UPDATE_PREFIX + downstream_ifname;
        mStateDbPtr->hset(state_key, "rx_cache_update", "done");

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