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

[Everflow/ERSPAN] Set correct destination port and mac address when the nexthop is updated for ERSPAN mirror destination #2392

Merged
merged 3 commits into from
Sep 9, 2022

Conversation

saksarav-nokia
Copy link
Contributor

@saksarav-nokia saksarav-nokia commented Jul 22, 2022

Signed-off-by: Sakthivadivu Saravanaraj sakthivadivu.saravanaraj@nokia.com

What I did
Fixed the issues reported in sonic-net/sonic-buildimage#11457
Why I did it
getNeighborEntry: This function compares Neighbor alias with nexthop alias. Since the alias for nexthop was modified from systemport name to Inband port name in addNextHop, the alias comparison was failing and mirror session was deactivated. Modified the getNeighborEntry to compare the correct alias.
updateSessionDstMac: Modified the code to set the dest mac of the mirror to gMacAddress if it is voq switch
updateSessionDstPort: Set the dest port of the mirror to Rec port if the mirror is ERSPAN mirror
removeRanges: Delete the acl range only if it was created . If the acl entry with mirror action is created when the mirror is not activated, only counter is created. Acl range and acl rule are not created. So removeRanges returns false and hence "Failed to remove acl rule" errors printed many times in syslog.
AclRuleMirror::activate : If Acl rule with mirror action was created before the mirror was activated and deleted, the counter was deleted. So activating the mirror calls AclRule:::createRule without creating counter and orchagent calls sairedis with counter Object as NULL and sai_meta layer returns error, so orchagent crashes. Added code to create counter if not valid before creating the rule.
How I verified it
Verified ingress and egress mirror oc tests passes
Details if related

@saksarav-nokia saksarav-nokia requested a review from prsunny as a code owner July 22, 2022 21:19
@prsunny prsunny requested a review from judyjoseph July 22, 2022 21:42
@saksarav-nokia
Copy link
Contributor Author

@judyjoseph @prsunny , could you please review this PR since this is blocking everflow testing with github image

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Aug 3, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

@saksarav-nokia
Copy link
Contributor Author

@judyjoseph @abdosi @ysmanman , I looked at the PR #2024 and i already addressed the issue fixed by #2024 in this PR with additional fixes and we don't need #2024. With #2392, all everflow tests are passing except tests related to policer.

@judyjoseph
Copy link
Contributor

/easycla

@judyjoseph
Copy link
Contributor

/Azurepipelines run Azure.sonic-buildimage

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@judyjoseph
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

orchagent/mirrororch.cpp Outdated Show resolved Hide resolved
nexthop is updated for ERSPAN mirror destination

Signed-off-by: Sakthivadivu Saravanaraj <sakthivadivu.saravanaraj@nokia.com>
crash

Signed-off-by: Sakthivadivu Saravanaraj <sakthivadivu.saravanaraj@nokia.com>
@judyjoseph
Copy link
Contributor

@arlakshm, @ysmanman - could you take a look at the changes as well ? thanks !

Signed-off-by: Sakthivadivu Saravanaraj <sakthivadivu.saravanaraj@nokia.com>
Copy link
Contributor

@abdosi abdosi left a comment

Choose a reason for hiding this comment

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

LGTM.. Can we have UT that can simulate this change ?

@judyjoseph judyjoseph merged commit 13bda3c into sonic-net:master Sep 9, 2022
@rlhui rlhui added the chassis label Sep 9, 2022
@gechiang
Copy link
Contributor

@saksarav-nokia , can you raise a new PR in 202205 branch to cherry pick this same change and I will help approve it and merge it so that we don't need to wait for 202205 branch owner to perform the cherry-pick.

gechiang pushed a commit that referenced this pull request Sep 16, 2022
…he nexthop is updated for ERSPAN mirror destination (#2392) (#2455)

* [Everflow/ERSPAN] Set correct destination port and mac address when the nexthop is updated for ERSPAN mirror destination
* [Everflow] Fixed show mirror-session, Acl rule remove failure, orchagent crash

Signed-off-by: Sakthivadivu Saravanaraj <sakthivadivu.saravanaraj@nokia.com>

Signed-off-by: Sakthivadivu Saravanaraj <sakthivadivu.saravanaraj@nokia.com>
@gechiang
Copy link
Contributor

Removed label "Request for 202205 branch" because we have already merged in by the following PR:
#2455.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants