-
Notifications
You must be signed in to change notification settings - Fork 11
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 LocalSecGrpClassifierCounter #581
Conversation
mchalla
commented
Aug 1, 2024
- added as an extension of SecGrpClassifierCounter
- added make checks as a copy of SecGrpStatsManager_test.cpp, replacing all the classifiers with Local, and convert the called functions to use template arguments.
- The make checks cover the prometheus tests
- this would also result in pushing the LocalSecGrpClassifierCounter to the leaf, should we decide to push the local secgrps to the leaf, we can strip the Local before sending it.
I missed changing testSecGrpDelete to use local classifier. I am seeing an issue where deleting the classifier is not deleting the counter for local case and will update with a fix. |
- added as an extension of SecGrpClassifierCounter - added make checks as a copy of SecGrpStatsManager_test.cpp, replacing all the classifiers with Local, and convert the called functions to use template arguments. - PolicyStatsManager also listens to LocalL24Classifiers - The make checks cover the prometheus tests - this would also result in pushing the LocalSecGrpClassifierCounter to the leaf, should we decide to push the local secgrps to the leaf, we can strip the Local before sending it. The following is the mapping L24Classifier <Contract> L24ClassifierCounter L24Classifier <SecGroup> SecGrpClassifierCounter LocalL24Classifier <LocalSecGroup> LocalSecGrpClassifierCounter Signed-off-by: Madhu Challa <challa@gmail.com>
37a90a3
to
4b0b5d1
Compare
The issue was the policy stats manager not listening to locall24classifier events. After fixing it the test passes. |
@@ -46,7 +47,8 @@ using boost::system::error_code; | |||
SecGrpStatsManager::SecGrpStatsManager(Agent* agent_, IdGenerator& idGen_, | |||
SwitchManager& switchManager_, | |||
long timer_interval_) | |||
: PolicyStatsManager(agent_,idGen_,switchManager_,timer_interval_) { | |||
: PolicyStatsManager(agent_,idGen_,switchManager_,timer_interval_), | |||
agent(agent_) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need to store Agent* in the SecGrpStatsManager subclass as well. It's protected in the superclass. Can't you just access it from there as needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh i did not realize that. i will change it via a separate commit since i already merged this change.