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

[orchagent]: Skip installing ACL counter when ACL mirror rule is inactive #3223

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

fountzou
Copy link

@fountzou fountzou commented Jul 5, 2024

  • Don't install counters when session rule is inactive.

Mirror session is not activated and thus, the ACL rule is not created in sairedis/SAI (there is no next hop for the mirror destination IP). However, orchagent creates and registers the ACL counter with the flexcounter (FC) and the counter is being polled every polling interval. Since the ACL counter is not attached to any ACL rule, the BCM SAI introduces print errors in syslog.

What I did
Removed the ACL counter when ACL mirror rule / session is inactive.

Why I did it
To solve #18844 issue.

How I verified it

  • Running everflow and acl testcases.
  • Checking syslog for the offending error logs.

Details if related

@fountzou fountzou requested a review from prsunny as a code owner July 5, 2024 17:00
@prsunny prsunny requested a review from bingwang-ms July 12, 2024 21:13
@@ -2055,6 +2055,11 @@ bool AclRuleMirror::activate()

if (!state)
{
// if the mirror session is inactive, do not create the counter yet. FlexCounter registration will be skipped as well.

Choose a reason for hiding this comment

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

In this situation, where the mirror session is inactive, was the ACL rule created and then removed?

Copy link
Author

Choose a reason for hiding this comment

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

Hi, thank you for reviewing. In this case the ACL rule, was not created at all by SAI.
ACL entry creation happens at the bottom of AclRuleMirror::activate() function via AclRule::createRule(), but code execution never reaches that point since function returns earlier when state == false, i.e., for inactive session.

Copy link

@DanielaMurin DanielaMurin Jul 17, 2024

Choose a reason for hiding this comment

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

If the ACL rule was never created, would it be possible not to create the counter at the beginning?
(Instead of it being created and then removed)

Copy link
Author

@fountzou fountzou Jul 22, 2024

Choose a reason for hiding this comment

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

Looking into it. Thanks for your suggestion.

Copy link
Author

Choose a reason for hiding this comment

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

PR has been updated based on your comment. Thank you!

Choose a reason for hiding this comment

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

Wonderful! :)

* Don't install counters when session rule is inactive.

Mirror session is not activated and thus, the ACL rule is not created in sairedis/SAI (there is no next hop for the mirror destination IP).
However, orchagent creates and registers the ACL counter with the flexcounter (FC) and the counter is being polled every polling interval.
Since the ACL counter is not attached to any ACL rule, the BCM SAI introduces print errors in syslog.

Signed-off-by: fountzou <[email protected]>
* Don't install counters when session rule is inactive.

Mirror session is not activated and thus, the ACL rule is not created in sairedis/SAI (there is no next hop for the mirror destination IP).
However, orchagent creates and registers the ACL counter with the flexcounter (FC) and the counter is being polled every polling interval.
Since the ACL counter is not attached to any ACL rule, the BCM SAI introduces print errors in syslog.

Signed-off-by: fountzou <[email protected]>
@deepak-singhal0408
Copy link
Contributor

@bingwang-ms can you please help review this?

orchagent/aclorch.cpp Outdated Show resolved Hide resolved
@mlok-nokia
Copy link

@prsunny @bingwang-ms @rlhui Please review this PR.

@bingwang-ms
Copy link
Contributor

LGTM now. Thanks for the fix.

@arlakshm
Copy link
Contributor

@prsunny, please help merge this PR.

@prsunny
Copy link
Collaborator

prsunny commented Sep 13, 2024

Restarting the VS tests

@prsunny
Copy link
Collaborator

prsunny commented Sep 13, 2024

/azp run Azure.sonic-swss

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@prsunny
Copy link
Collaborator

prsunny commented Sep 19, 2024

@fountzou , could you check the test failure?, also please fix the title of PR

@fountzou fountzou changed the title [orchagent]: Resolving issue #18844 [orchagent]: Skip installing ACL counter when ACL mirror rule is inactive Sep 19, 2024
@fountzou
Copy link
Author

fountzou commented Sep 19, 2024

Hi @prsunny. Title has been fixed -- thanks.

Looking into this but not sure yet if it is related to my code changes; Noticed in the pipelines that there are several PRs that fail in the same testcase.
https://dev.azure.com/mssonic/build/_build?definitionId=15&_a=summary
master: https://dev.azure.com/mssonic/build/_build?definitionId=15&_a=summary&repositoryFilter=154&branchFilter=11183%2C11183

Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

7 participants