Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for port mirroring CLIs #936

Merged
merged 8 commits into from
Jul 1, 2020

Conversation

rupesh-k
Copy link
Contributor

@rupesh-k rupesh-k commented Jun 4, 2020

Signed-off-by: Rupesh Kumar rupesh-k.kumar@broadcom.com

- What I did

Port mirroring CLI implementation
HLD @ sonic-net/SONiC#58

- How I did it
Enhance existing mirror_session command to support port mirroring.

- How to verify it
Create ERSPAN/SPAN mirror session and verify show mirror_session.

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

config mirror_session add everflow0 10.1.1.1 12.1.1.1 10 10
root@sonic:/home/admin# show mirror_session
Name Status SRC IP DST IP GRE DSCP TTL Queue Policer Monitor Port SRC Port Direction


everflow0 active 10.1.1.1 12.1.1.1 10 10

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

config mirror_session add span sess1 Ethernet44 Ethernet40 rx

root@sonic:/home/admin# show mirror_session
ERSPAN Sessions
Name Status SRC IP DST IP GRE DSCP TTL Queue Policer Monitor Port SRC Port Direction


everflow0 active 10.1.1.1 12.1.1.1 0 10 10

SPAN Sessions
Name Status DST Port SRC Port Direction Queue Policer


sess1 active Ethernet44 Ethernet40 rx

@rupesh-k
Copy link
Contributor Author

Thanks @prsunny for adding reviewers to swss port-mirroring PR.
Can u please help here also.

@prsunny prsunny requested a review from daall June 24, 2020 15:04
@rupesh-k
Copy link
Contributor Author

Thanks @prsunny

Hi @daall ,

Can u please help with review, I hope we can make this feature for 202006 release :)

Thanks

While adding a new SPAN session, users need to configure the following fields that are used while forwarding the mirrored packets.
1) destination port,
2) optional - List of source ports- List of source ports which can have both Ethernet and LAG ports.
3) optional - Direction - Mirror session direction when configured along with Source port.

Choose a reason for hiding this comment

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

Could you specify the default direction as this is optional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When source port is specified, we can make direction default as both. Let me push that change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. taken care of this. Thanks.


While adding a new SPAN session, users need to configure the following fields that are used while forwarding the mirrored packets.
1) destination port,
2) optional - List of source ports- List of source ports which can have both Ethernet and LAG ports.

Choose a reason for hiding this comment

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

Can we add a note that atleast one port/LAG is required for local span?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we can have local span session with only destination port, which can be used in ACL mirroring.
Direction is optional in this case. Based on ingress/egress ACL, the corresponding flow gets mirrored.

config/main.py Show resolved Hide resolved
kperumalbfn
kperumalbfn previously approved these changes Jun 25, 2020
Copy link
Contributor

@daall daall left a comment

Choose a reason for hiding this comment

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

See comments.

acl_loader/main.py Outdated Show resolved Hide resolved
acl_loader/main.py Outdated Show resolved Hide resolved
config/main.py Outdated Show resolved Hide resolved
config/main.py Outdated Show resolved Hide resolved
config/main.py Outdated Show resolved Hide resolved
if validate_mirror_session_config(config_db, session_name, None, src_port, direction) is False:
return
config_db.set_entry("MIRROR_SESSION", session_name, session_info)
else:
Copy link
Contributor

Choose a reason for hiding this comment

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

@abdosi could you take a quick look at the multi-NPU logic here and make sure it looks OK to you?

Copy link
Contributor

Choose a reason for hiding this comment

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

Validation need to be updated so that src_port and dst_port are in same asic/namespace.
TO_DO item (not blocking for this PR)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. will take of this in next PR. How do I validate this ?

config/main.py Outdated Show resolved Hide resolved
config/main.py Outdated Show resolved Hide resolved
config/main.py Show resolved Hide resolved
doc/Command-Reference.md Outdated Show resolved Hide resolved
@daall daall requested a review from abdosi June 25, 2020 23:15
@rupesh-k
Copy link
Contributor Author

Thanks alot @daall .

click.echo("Error: Destination Interface {} is invalid".format(dst_port))
return False

if interface_is_in_vlan(vlan_member_table, dst_port):

Choose a reason for hiding this comment

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

If the port is a routed port, SPAN session is supported?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. Not supported. Please check in latest patchset.

click.echo("Error: Destination Interface {} has vlan config".format(dst_port))
return False

if interface_has_mirror_config(mirror_table, dst_port):

Choose a reason for hiding this comment

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

Not sure if I understand this correctly, can a single dst port cannot be a monitor (mirror-to) port
for multiple mirror sessions?
For example
case 1: mirror rx from Ethernet0 to Etherent4
case 2: mirror rx&tx from Ethernet2 to Etherent4
with the above check, case 2 will fail?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. This is not supported. We allow only one mirror session per port, The session can have any number of ports.

config/main.py Outdated
session_info['direction'] = direction.upper()

if policer is not None:
session_info['policer'] = policer

Choose a reason for hiding this comment

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

What is the need for policer and queue for SPAN sessions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is existing support in community. This is to police the mirrored traffic.


print(tabulate.tabulate(data, headers=header, tablefmt="simple", missingval=""))
if val.get("type") == "SPAN":

Choose a reason for hiding this comment

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

What is the need for policer and queue for SPAN sessions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as above. Extending it to SPAN also.

@rupesh-k
Copy link
Contributor Author

Thanks @daall for the review. I have taken care of the comments. Can you please review again.

@rupesh-k rupesh-k requested a review from daall June 29, 2020 17:08
acl_loader/main.py Outdated Show resolved Hide resolved
config/main.py Outdated Show resolved Hide resolved
config/main.py Outdated Show resolved Hide resolved
Copy link
Contributor

@daall daall left a comment

Choose a reason for hiding this comment

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

Looking a lot better! Found a couple more things, there are also a few from last time that got missed. You might have to click the "Load more..." button in the PR to see all of them. :)

config/main.py Outdated Show resolved Hide resolved
config/main.py Outdated Show resolved Hide resolved
config/main.py Outdated Show resolved Hide resolved
@rupesh-k
Copy link
Contributor Author

Looking a lot better! Found a couple more things, there are also a few from last time that got missed. You might have to click the "Load more..." button in the PR to see all of them. :)

Yes. sorry. :) i figured once u started first comment, will push in some time now. Thanks for quickly checking.

@rupesh-k
Copy link
Contributor Author

Thanks @daall . I have resolved all comments. Can u please review.

@rupesh-k rupesh-k requested a review from daall June 30, 2020 18:08
@amin-alavi
Copy link

@daall @kperumalbfn @dev-aviz , Just like the request in the related PR, and given the imminent cutting of 202006 branch, can this PR be merged and any further updates to be made in 202006 branch?

Thanks!

@rupesh-k
Copy link
Contributor Author

@daall found some other places where it can be fixed for simpler validation. Modified and pushed. :) thanks

@amin-alavi
Copy link

@abdosi , missed to add you to my request to review and approve.


- Usage:
```
config mirror_session span add <session_name> <dst_port> [source-port-list] [direction] [queue] [policer <policer_name>]
Copy link
Contributor

@abdosi abdosi Jul 1, 2020

Choose a reason for hiding this comment

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

this need to be enhance for Multi-asic platforms. dst port need to be on same asic where mirror session binded.
we need to enhance to take option -n and all dest port should be on same ASIC/namespace

We have enhanced similar for other features and PR# #878 with details

This can be TO_DO item and need not be needed with this PR

@rlhui

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. thanks for sharing PR. will take care in next PR. please suggest on how can we validate these.

for front_asic_namespaces in namespaces['front_ns']:
per_npu_configdb[front_asic_namespaces] = ConfigDBConnector(use_unix_socket_path=True, namespace=front_asic_namespaces)
per_npu_configdb[front_asic_namespaces].connect()
if validate_mirror_session_config(per_npu_configdb[front_asic_namespaces], session_name, dst_port, src_port, direction) is False:
Copy link
Contributor

Choose a reason for hiding this comment

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

Validation need to be updated so that src_port and dst_port are in same asic/namespace.
TO_DO item (not blocking for this PR)

@daall
Copy link
Contributor

daall commented Jul 1, 2020

retest this please

This command supports configuring both SPAN/ERSPAN sessions.
In SPAN user can configure mirroring of list of source ports/LAG to destination port in ingress/egress/both directions.
In ERSPAN user can configure mirroring of list of source ports/LAG to a destination IP.
Both SPAN/ERSPAN support ACL based mirroring and can be used in ACL configurations.
Copy link
Contributor

Choose a reason for hiding this comment

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

can we have Everflow ACL table with everflow mirror session and Data ACL Table with port based mirror session ?


- Usage:
```
config mirror_session span add <session_name> <dst_port> [source-port-list] [direction] [queue] [policer <policer_name>]
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it uses ACL internally ? Can we specify queue and policer without using ACL ? Also if does not use ACL internally can we police with direction as tx ?

Have we verified this combination and what is the behaviour at Sonic level if SAI return error for any of unsupported combination

@lguohan lguohan merged commit cfcb766 into sonic-net:master Jul 1, 2020
abdosi pushed a commit to abdosi/sonic-utilities that referenced this pull request Aug 4, 2020
* Add support for port mirroring CLIs

Signed-off-by: Rupesh Kumar <rupesh-k.kumar@broadcom.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants