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

[DASH] ACL tags implementation #2915

Merged
merged 6 commits into from
Oct 24, 2023

Conversation

oleksandrivantsiv
Copy link
Collaborator

What I did
Implement the DASH ACL tags feature according to DASH ACL tags HLD

Why I did it
To add support for DASH ACL tags feature

How I verified it
Run extended VS tests that cover new implementation

Details if related

@oleksandrivantsiv
Copy link
Collaborator Author

/azpw run

@mssonicbld
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@oleksandrivantsiv
Copy link
Collaborator Author

/azpw run

@mssonicbld
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@oleksandrivantsiv
Copy link
Collaborator Author

@prsunny kindly reminder.
Can you please review or assign someone to review the PR?

Copy link
Contributor

@Pterosaur Pterosaur left a comment

Choose a reason for hiding this comment

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

All changes look good to me.

@prsunny , But there is a concern that we store FULL ACL cache(groups, rules, and tags) in the memory of orchagent. Do you have any suggestions about this?

@prsunny
Copy link
Collaborator

prsunny commented Oct 10, 2023

All changes look good to me.

@prsunny , But there is a concern that we store FULL ACL cache(groups, rules, and tags) in the memory of orchagent. Do you have any suggestions about this?

thats a good catch. We don't want to have cache for ACL rules, otherwise it will take high memory. @oleksandrivantsiv , could you please check on this and remove cache.

@oleksandrivantsiv
Copy link
Collaborator Author

All changes look good to me.
@prsunny , But there is a concern that we store FULL ACL cache(groups, rules, and tags) in the memory of orchagent. Do you have any suggestions about this?

thats a good catch. We don't want to have cache for ACL rules, otherwise it will take high memory. @oleksandrivantsiv , could you please check on this and remove cache.

@prsunny I have a plan to remove a cache in the separate PR by querying the information from APP DB directly. I suggest merging this PR to allow us to start working on the "E2E" flow testing.

@Pterosaur
Copy link
Contributor

All changes look good to me.
@prsunny , But there is a concern that we store FULL ACL cache(groups, rules, and tags) in the memory of orchagent. Do you have any suggestions about this?

thats a good catch. We don't want to have cache for ACL rules, otherwise it will take high memory. @oleksandrivantsiv , could you please check on this and remove cache.

@prsunny I have a plan to remove a cache in the separate PR by querying the information from APP DB directly. I suggest merging this PR to allow us to start working on the "E2E" flow testing.

@oleksandrivantsiv Agree. Could you please update a commit: 966c5bb that is for fixing an ACL bug?

@oleksandrivantsiv
Copy link
Collaborator Author

All changes look good to me.
@prsunny , But there is a concern that we store FULL ACL cache(groups, rules, and tags) in the memory of orchagent. Do you have any suggestions about this?

thats a good catch. We don't want to have cache for ACL rules, otherwise it will take high memory. @oleksandrivantsiv , could you please check on this and remove cache.

@prsunny I have a plan to remove a cache in the separate PR by querying the information from APP DB directly. I suggest merging this PR to allow us to start working on the "E2E" flow testing.

@oleksandrivantsiv Agree. Could you please update a commit: 966c5bb that is for fixing an ACL bug?

@Pterosaur I updated the branch to include the changes form 966c5bb commit.

@oleksandrivantsiv
Copy link
Collaborator Author

@prsunny, @Pterosaur if there are no other comments can you please approve the PR?

@oleksandrivantsiv
Copy link
Collaborator Author

/azpw run

@mssonicbld
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@liat-grozovik liat-grozovik merged commit dc8fd20 into sonic-net:master Oct 24, 2023
16 checks passed
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.

None yet

6 participants