[cli] Add sample_rate and truncate_size to mirror session CLI by Janetxxx · Pull Request #4459 · sonic-net/sonic-utilities

Janetxxx · 2026-04-15T05:03:20Z

What I did

config mirror_session erspan add: add --sample_rate and --truncate_size options
show mirror_session: add Sample Rate and Truncate Size columns to ERSPAN output
Values stored as strings in CONFIG_DB, consistent with existing fields

How I did it

How to verify it

admin@sonic:~$  sudo config mirror_session erspan add test1 \
>      10.0.1.192 10.0.1.193 8 64 \
>      --sample_rate 50000 --truncate_size 128
admin@sonic:~$ sonic-db-cli CONFIG_DB hgetall "MIRROR_SESSION|test1"
{'dscp': '8', 'dst_ip': '10.0.1.193', 'sample_rate': '50000', 'src_ip': '10.0.1.192', 'truncate_size': '128', 'ttl': '64', 'type': 'ERSPAN'}
admin@sonic:~$  show mirror_session
ERSPAN Sessions
Name    Status    SRC IP      DST IP      GRE      DSCP    TTL  Queue    Policer    Monitor Port    SRC Port    Direction      Sample Rate    Truncate Size
------  --------  ----------  ----------  -----  ------  -----  -------  ---------  --------------  ----------  -----------  -------------  ---------------
test1   error     10.0.1.192  10.0.1.193              8     64                                                                       50000              128

SPAN Sessions
Name    Status    DST Port    SRC Port    Direction    Queue    Policer
------  --------  ----------  ----------  -----------  -------  ---------

Previous command output (if the output of a command-line utility has changed)

New command output (if the output of a command-line utility has changed)

mssonicbld · 2026-04-15T05:03:29Z

/azp run

azure-pipelines · 2026-04-15T05:03:41Z

Azure Pipelines successfully started running 1 pipeline(s).

Copilot

Pull request overview

Adds support for configuring and displaying ERSPAN mirroring sampling/truncation parameters in SONiC’s operator-facing CLI, extending existing config mirror_session and show mirror_session behavior.

Changes:

Extend config mirror_session erspan add with --sample_rate and --truncate_size, persisting them into MIRROR_SESSION entries.
Extend ERSPAN session display (acl-loader show session / show mirror_session) with “Sample Rate” and “Truncate Size” columns.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
`config/main.py`	Adds new ERSPAN CLI options and stores `sample_rate` / `truncate_size` in the session config.
`acl_loader/main.py`	Extends ERSPAN session table output to include new columns for the added fields.

- config mirror_session erspan add: add --sample_rate and --truncate_size options - show mirror_session: add Sample Rate and Truncate Size columns to ERSPAN output - Values stored as strings in CONFIG_DB, consistent with existing fields Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Janet Cui <janet970527@gmail.com>

mssonicbld · 2026-04-15T05:29:40Z

/azp run

azure-pipelines · 2026-04-15T05:29:51Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2026-04-15T05:30:10Z

/azp run

azure-pipelines · 2026-04-15T05:30:22Z

Azure Pipelines successfully started running 1 pipeline(s).

Signed-off-by: Janet Cui <janet970527@gmail.com>

mssonicbld · 2026-04-15T05:37:30Z

/azp run

azure-pipelines · 2026-04-15T05:37:41Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2026-04-15T06:36:25Z

/azp run

azure-pipelines · 2026-04-15T06:36:36Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2026-04-15T06:57:05Z

/azp run

azure-pipelines · 2026-04-15T06:57:17Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2026-04-15T07:06:25Z

/azp run

azure-pipelines · 2026-04-15T07:06:36Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2026-04-15T09:28:11Z

/azp run

azure-pipelines · 2026-04-15T09:28:27Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2026-04-15T09:32:05Z

/azp run

azure-pipelines · 2026-04-15T09:32:17Z

Azure Pipelines successfully started running 1 pipeline(s).

banidoru

All prior concerns addressed in this iteration.

sample_rate/truncate_size now routed through gather_session_info ✅
Boundary tests for validator holes (1-255, 1-63) and edges added ✅
Negative tests now assert error message text ✅
Show test includes mock session with populated sample_rate/truncate_size ✅
File-level noqa justified (multi-line string blocks) ✅

No new issues found. LGTM.

banidoru

All prior concerns addressed in this iteration.

sample_rate/truncate_size now routed through gather_session_info ✅
Boundary tests for validator holes (1-255, 1-63) and edges added ✅
Negative tests now assert error message text ✅
Show test includes mock session with populated sample_rate/truncate_size ✅
File-level noqa justified (multi-line string blocks) ✅

No new issues found. LGTM.

banidoru

Iteration 4 re-review — all prior threads addressed. LGTM.

CLI validation now correctly enforces documented ranges (256–8388608 / 64–9216) with 0 as sentinel
sample_rate/truncate_size now routed through gather_session_info for consistency
Comprehensive boundary tests added for both validators
Mock DB entry with populated values verifies show output rendering
noqa rationale accepted — per-line annotations not feasible inside triple-quoted test strings

banidoru

All reviewers approved. LGTM.

- Tighten CLI validation for sample_rate (0 or 256-8388608) and truncate_size (0 or 64-9216) - Move sample_rate/truncate_size handling into gather_session_info for consistency - Add positive test for valid sample_rate and truncate_size values - Add show test coverage for populated sample_rate and truncate_size values Signed-off-by: Janet Cui <janet970527@gmail.com>

mssonicbld · 2026-04-21T00:50:10Z

/azp run

azure-pipelines · 2026-04-21T00:50:21Z

Azure Pipelines successfully started running 1 pipeline(s).

banidoru

All prior feedback addressed in this iteration. LGTM.

✅ sample_rate/truncate_size now routed through gather_session_info for consistency
✅ Validation callbacks enforce documented ranges (256–8388608 / 64–9216) with 0-as-disabled sentinel
✅ Comprehensive boundary tests cover hole ranges, exact boundaries, and above-max values
✅ Positive test verifies valid values pass through to add_erspan
✅ Mock DB entry with populated sample_rate/truncate_size added; show test covers rendered output

banidoru

LGTM, with a few minor suggestions to improve test coverage and code consistency.

banidoru

All prior feedback addressed in this iteration:

sample_rate/truncate_size routed through gather_session_info for consistency
Negative tests now assert error message content
Boundary/hole value tests added (1, 255, 63 etc.)
Mock DB includes populated session; show test verifies rendered values
CLI validators match documented ranges
File-level noqa justified by PR author (triple-quoted strings)

LGTM.

banidoru

All reviewers approved. LGTM.

mssonicbld · 2026-04-22T10:16:18Z

/azp run

azure-pipelines · 2026-04-22T10:16:24Z

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.

…sages Signed-off-by: Janet Cui <janet970527@gmail.com>

mssonicbld · 2026-04-22T10:23:38Z

/azp run

azure-pipelines · 2026-04-22T10:23:49Z

Azure Pipelines successfully started running 1 pipeline(s).

banidoru

All prior review concerns addressed: magic numbers extracted to constants, validation routed through gather_session_info, boundary/hole tests added, error message assertions added, populated mock session for show test.
Validators correctly enforce documented ranges (256–8388608, 64–9216) with 0 as disabled sentinel.
Test coverage is thorough: negative, boundary, hole-range, and positive-path cases all present.
Code is clean and consistent with existing patterns in the file.
LGTM.

banidoru

Iteration 6 re-review:

All 19 prior review threads are resolved — concerns addressed across iterations. ✅
Constants extracted to module level (SAMPLE_RATE_MIN/MAX, TRUNCATE_SIZE_MIN/MAX) per r12f's feedback. ✅
Validation, CLI, and docs are now consistent on ranges. ✅
Test coverage is comprehensive: boundary, hole, negative with error message assertions, and positive happy-path. ✅
Minor nit: validator error messages say must be 256..8388608 but don't mention 0 is also valid — could confuse users passing 0 intentionally. Low priority.

banidoru · 2026-04-22T10:30:05Z

+SAMPLE_RATE_MAX = 8388608
+TRUNCATE_SIZE_MIN = 64
+TRUNCATE_SIZE_MAX = 9216
+


Minor nit: The error message must be 256..8388608 doesn't mention that 0 is also a valid value (disabled). A user seeing this error might not realize 0 is accepted. Consider: must be 0 (disabled) or 256..8388608. Same for validate_truncate_size. Low priority — not blocking.

Still a valid nit. The error message says must be 256..8388608 but doesn't mention 0 is also accepted. Low priority, but would improve UX.

Still open. Error messages still say must be 256..8388608 / must be 64..9216 without mentioning 0 is valid. Low priority nit but would improve UX.

Still valid nit — error message omits that 0 is accepted. Non-blocking, but would be a nice UX improvement in a follow-up.

Addressed by new design direction. Help text now says "Omit for full mirror" instead of "0 = disabled", so the error message must be 256..8388608 is consistent — users are not expected to explicitly pass 0. The magic numbers are now constants too. Looks good.

banidoru

All reviewers approved. LGTM.

banidoru

LGTM — all prior concerns addressed. One minor nit remains (error message could mention 0 is valid), not blocking.

Constants extracted, validation callbacks enforce documented ranges ✅
gather_session_info handles sample_rate/truncate_size consistently ✅
Boundary tests cover hole values and edges ✅
Show test includes sampled session with populated values ✅
Negative tests assert error message content ✅

banidoru

The error message for sample_rate and truncate_size validation should ideally mention that 0 is a valid value for disabling the feature. This is a minor UX improvement.
The use of magic numbers for range validation in config/main.py was pointed out in a previous review. Using named constants would improve readability and maintainability.

banidoru · 2026-04-22T10:49:05Z

+SAMPLE_RATE_MAX = 8388608
+TRUNCATE_SIZE_MIN = 64
+TRUNCATE_SIZE_MAX = 9216
+


The error message must be 256..8388608 does not mention that 0 is also a valid value (for disabling). A user seeing this error might not realize 0 is accepted. Consider updating the message to something like: must be 0 (disabled) or 256..8388608.

Still open — same concern as the other thread. Error message doesn't mention 0 is accepted.

Same nit — error message doesn't mention 0 is valid. Non-blocking.

Same as above — the reframed help text ("Omit for full mirror" / "Omit for no truncation") makes the error message consistent. No longer need to mention 0. Resolved.

banidoru

Iteration 7 re-review:

Most prior feedback addressed: constants for magic numbers, validation callbacks with correct ranges, gather_session_info integration, boundary/hole tests, error message assertions, populated mock data for show test. Good progress.
Two open nits remain: error messages (must be 256..8388608 / must be 64..9216) don't mention 0 is also valid. Low priority but would improve UX.
Code is clean and well-structured. No new issues found in this iteration.
Overall: solid shape. The 0-mention nit is the only remaining item.

banidoru

All reviewers approved. LGTM.

banidoru

Iteration 8 review:

Constants extracted — magic numbers replaced with SAMPLE_RATE_MIN/MAX, TRUNCATE_SIZE_MIN/MAX. Good.
gather_session_info integration — sample_rate/truncate_size now routed through the shared helper. Consistency concern resolved.
Test coverage solid — boundary tests, hole-value tests, positive path, error message assertions, and show test with populated values all present.
One open nit remains: error messages still say must be 256..8388608 without mentioning 0 is valid. Low priority but would improve UX.
Overall: PR is in good shape. No new issues found in this iteration.

banidoru

Constants extracted for magic numbers — clean improvement
Help text and doc updated for consistency
Tests properly updated to match new error message format
Minor nit still open: error messages omit that 0 is accepted (low priority, non-blocking)
LGTM overall

banidoru

Constants extracted for magic numbers — clean and maintainable
Help text reframed to "Omit for full mirror" / "Omit for no truncation" — better UX, error messages now consistent
Doc, CLI validation, and test assertions all in sync
All prior review concerns addressed across iterations
LGTM — no new issues found

banidoru

All reviewers approved. LGTM.

Copilot AI review requested due to automatic review settings April 15, 2026 05:03

Copilot started reviewing on behalf of Janetxxx April 15, 2026 05:03 View session

Copilot AI reviewed Apr 15, 2026

View reviewed changes

Comment thread acl_loader/main.py Outdated

Comment thread config/main.py Outdated

Comment thread config/main.py Outdated

Comment thread config/main.py Outdated

Janetxxx force-pushed the dev/jc/port_mirror_erspan branch from 2c4db9f to a2c3e10 Compare April 15, 2026 05:29

fix cli int range

8a7a9b5

Signed-off-by: Janet Cui <janet970527@gmail.com>

Janetxxx force-pushed the dev/jc/port_mirror_erspan branch from e9f141a to 8a7a9b5 Compare April 15, 2026 05:37

Janetxxx force-pushed the dev/jc/port_mirror_erspan branch from 339ddc5 to 8113bf3 Compare April 15, 2026 06:56

Janetxxx force-pushed the dev/jc/port_mirror_erspan branch from 8113bf3 to a53c931 Compare April 15, 2026 07:06

Janetxxx force-pushed the dev/jc/port_mirror_erspan branch from a53c931 to c056dbb Compare April 15, 2026 09:28

Janetxxx force-pushed the dev/jc/port_mirror_erspan branch from c056dbb to c7cbc82 Compare April 15, 2026 09:31

Janetxxx force-pushed the dev/jc/port_mirror_erspan branch from c7cbc82 to 7013c65 Compare April 15, 2026 10:45

banidoru approved these changes Apr 20, 2026

View reviewed changes

Janetxxx force-pushed the dev/jc/port_mirror_erspan branch from d5c7f06 to 9c9fe39 Compare April 21, 2026 00:50

banidoru approved these changes Apr 21, 2026

View reviewed changes

banidoru reviewed Apr 21, 2026

View reviewed changes

Comment thread config/main.py

Comment thread tests/config_mirror_session_test.py

Comment thread tests/config_mirror_session_test.py

banidoru approved these changes Apr 21, 2026

View reviewed changes

r12f reviewed Apr 22, 2026

View reviewed changes

Comment thread config/main.py Outdated

Sync CLI with YANG changes: remove 0 as valid value, update error mes…

9b0d02c

…sages Signed-off-by: Janet Cui <janet970527@gmail.com>

Janetxxx force-pushed the dev/jc/port_mirror_erspan branch from 877f215 to 9b0d02c Compare April 22, 2026 10:23

banidoru approved these changes Apr 22, 2026

View reviewed changes

banidoru reviewed Apr 22, 2026

View reviewed changes

banidoru approved these changes Apr 22, 2026

View reviewed changes

banidoru reviewed Apr 22, 2026

View reviewed changes

banidoru approved these changes Apr 22, 2026

View reviewed changes

banidoru reviewed Apr 22, 2026

View reviewed changes

banidoru approved these changes Apr 22, 2026

View reviewed changes

Conversation

Janetxxx commented Apr 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What I did

How I did it

How to verify it

Previous command output (if the output of a command-line utility has changed)

New command output (if the output of a command-line utility has changed)

Uh oh!

mssonicbld commented Apr 15, 2026

Uh oh!

azure-pipelines Bot commented Apr 15, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mssonicbld commented Apr 15, 2026

Uh oh!

azure-pipelines Bot commented Apr 15, 2026

Uh oh!

mssonicbld commented Apr 15, 2026

Uh oh!

azure-pipelines Bot commented Apr 15, 2026

Uh oh!

mssonicbld commented Apr 15, 2026

Uh oh!

azure-pipelines Bot commented Apr 15, 2026

Uh oh!

mssonicbld commented Apr 15, 2026

Uh oh!

azure-pipelines Bot commented Apr 15, 2026

Uh oh!

mssonicbld commented Apr 15, 2026

Uh oh!

azure-pipelines Bot commented Apr 15, 2026

Uh oh!

mssonicbld commented Apr 15, 2026

Uh oh!

azure-pipelines Bot commented Apr 15, 2026

Uh oh!

mssonicbld commented Apr 15, 2026

Uh oh!

azure-pipelines Bot commented Apr 15, 2026

Uh oh!

mssonicbld commented Apr 15, 2026

Uh oh!

azure-pipelines Bot commented Apr 15, 2026

Uh oh!

banidoru left a comment

Choose a reason for hiding this comment

Uh oh!

banidoru left a comment

Choose a reason for hiding this comment

Uh oh!

banidoru left a comment

Choose a reason for hiding this comment

Uh oh!

banidoru left a comment

Choose a reason for hiding this comment

Uh oh!

mssonicbld commented Apr 21, 2026

Uh oh!

azure-pipelines Bot commented Apr 21, 2026

Uh oh!

banidoru left a comment

Choose a reason for hiding this comment

Uh oh!

banidoru left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

banidoru left a comment

Janetxxx commented Apr 15, 2026 •

edited

Loading