Skip to content

add l2mcd docker for igmp snooping#24327

Draft
jasper-hou-micas wants to merge 8 commits intosonic-net:202511from
jasper-hou-micas:igmp-snp
Draft

add l2mcd docker for igmp snooping#24327
jasper-hou-micas wants to merge 8 commits intosonic-net:202511from
jasper-hou-micas:igmp-snp

Conversation

@jasper-hou-micas
Copy link
Contributor

Why I did it

Work item tracking
  • Microsoft ADO (number only):

How I did it

How to verify it

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

  • 202205
  • 202211
  • 202305
  • 202311
  • 202405
  • 202411
  • 202505

Tested branch (Please provide the tested image version)

Description for the changelog

Link to config_db schema for YANG module changes

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

@mssonicbld
Copy link
Collaborator

/azp run Azure.sonic-buildimage

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link
Collaborator

/azp run Azure.sonic-buildimage

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link
Collaborator

/azp run Azure.sonic-buildimage

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link
Collaborator

/azp run Azure.sonic-buildimage

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link
Collaborator

/azp run Azure.sonic-buildimage

@azure-pipelines
Copy link

Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command.

@mssonicbld
Copy link
Collaborator

/azp run Azure.sonic-buildimage

@azure-pipelines
Copy link

Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command.

@mssonicbld
Copy link
Collaborator

/azp run Azure.sonic-buildimage

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Signed-off-by: Jasper.Hou <[email protected]>
Signed-off-by: Jasper.Hou <[email protected]>
Signed-off-by: Jasper.Hou <[email protected]>
Signed-off-by: Jasper.Hou <[email protected]>
Signed-off-by: Jasper.Hou <[email protected]>
@mssonicbld
Copy link
Collaborator

/azp run Azure.sonic-buildimage

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Signed-off-by: Jasper.Hou <[email protected]>
@mssonicbld
Copy link
Collaborator

/azp run Azure.sonic-buildimage

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link

@aidan-gallagher aidan-gallagher left a comment

Choose a reason for hiding this comment

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

This PR adds docker-l2mcd, systemd service, YANG models, CoPP traps, and build integration. The container is non-functional as shipped (all supervisorctl commands commented out). The PR also bundles several unrelated changes that affect all SONiC builds.

Critical High Medium Low Total
1 7 9 8 25

Additionally, the YANG model files in this PR (sonic-igmp-snooping.yang, sonic-mld-snooping.yang) have issues noted in the companion mgmt-common PR #185 review. Those findings are included inline below since the YANG files live in this PR.

Unrelated changes bundled: This PR includes changes to telemetry variables (docker-sonic-telemetry/telemetry_vars.j2), enables TRANSLIB_WRITE globally, adds default MGMT_PORT configuration (init_cfg.json.j2:24-29), and adds an x509 YANG leaf (sonic-device_metadata.yang). These affect all SONiC builds and are unrelated to IGMP snooping.

YANG model notes: Both YANG files have redundant leaves (out-intf key duplicated by port non-key, mrouter-intf key duplicated by mrouter_port), inconsistent naming (mrouter_port underscore vs hyphens elsewhere), and the IGMP model has L2MC_SUPPRESS container but MLD has no equivalent for IPv6 suppression. Timer leaves (query-interval, last-member-query-interval, query-max-response-time) are missing units statements -- the config guide clarifies they use seconds/milliseconds but this should be in the YANG source.

INCLUDE_ICCPD = n

# INCLUDE_L2MCD - build docker-l2mcd for IGMP Snooping
INCLUDE_L2MCD = y

Choose a reason for hiding this comment

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

High: INCLUDE_L2MCD defaults to y. Other optional features (e.g., INCLUDE_ICCPD, INCLUDE_SYSTEM_TELEMETRY) default to n. A new feature that is currently non-functional (commented-out start commands, hardcoded amd64, libpython3.8) should not be enabled by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The INCLUDE-XXX changes are enabled by default to avoid potential feature loss due to missing build-time options. We’re open to your feedback on this approach.

# ENABLE_TRANSLIB_WRITE - Enable translib write/config operations via the gNMI interface.
# Uncomment to enable:
# ENABLE_TRANSLIB_WRITE = y
ENABLE_TRANSLIB_WRITE = y

Choose a reason for hiding this comment

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

High: ENABLE_TRANSLIB_WRITE = y enables gNMI write access for all SONiC builds. This was intentionally left disabled by the community. Enabling it here is unrelated to L2MCD and has security implications (especially combined with the gnmi PR #527 which hardcodes admin roles).

@aidan-gallagher
Copy link

Documentation Review Notes

The vendor-provided documentation (configuration guide, YANG/API reference, test results) has the following notable gaps. These are not code issues but affect the ability to review and validate the feature.

Test Results

No IGMP snooping test results (Critical): All 14 documented test cases (IT-SONIC-MLD-SNP-BAS-001 through -014) are MLD snooping tests. Despite the PR suite being titled "IGMP Snooping" and IGMP being the primary feature, there are zero documented IGMP snooping test results.

No negative/boundary tests (High): All 14 tests are positive path only ("configure X, verify X works"). Missing: max multicast group limits, simultaneous IGMP+MLD on same VLAN (which the config guide says is unsupported), l2mcd crash recovery, malformed packets, and scale testing.

Configuration Guide

Silent multicast drop on enable (High): The guide states that when L2MC is enabled but optimised_multicast_flood is not, all unknown multicast is dropped at ingress. This means enabling IGMP snooping silently breaks all multicast traffic until the operator also enables flood optimization. Most IGMP snooping implementations flood unknown multicast by default. This should be prominently warned about.

IGMP/MLD mutual exclusion buried (Medium): The constraint that IGMP and MLD snooping cannot be enabled simultaneously on the same VLAN is only mentioned in the MLD section, not in the IGMP section.

YANG/API Reference

Redundant leaves unexplained (High): The YANG tree diagrams show L2MC_STATIC_MEMBER_LIST has both out-intf (key) and port (non-key) with identical types, and L2MC_MROUTER_LIST has both mrouter-intf (key) and mrouter_port. The document describes both individually but never explains why both exist or what a gNMI client should set.

@jasper-hou-micas
Copy link
Contributor Author

Documentation Review Notes

The vendor-provided documentation (configuration guide, YANG/API reference, test results) has the following notable gaps. These are not code issues but affect the ability to review and validate the feature.

@aidan-gallagher
The IGMP test cases were provided in the first email, and the MLD test cases are included in the latest attachment, with the duplicate items removed.
Both MLD and IGMP can now be enabled simultaneously, and the latest SCG document has been updated accordingly.

@mssonicbld
Copy link
Collaborator

/azp run Azure.sonic-buildimage

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

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