-
Notifications
You must be signed in to change notification settings - Fork 522
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
[intfsorch] add RIF flex counter group #765
Conversation
Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>
retest this please |
orchagent/intfsorch.cpp
Outdated
} | ||
} | ||
|
||
// for (auto it = m_rifsToRemove.begin(); it != m_rifsToRemove.end();) |
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.
Commented out by mistake or irrelevant?
Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>
depend on schema sonic-net/sonic-swss-common#256 |
retest this please |
orchagent/intfsorch.cpp
Outdated
/* Initialize DB connectors */ | ||
m_counter_db = shared_ptr<DBConnector>(new DBConnector(COUNTERS_DB, DBConnector::DEFAULT_UNIXSOCKET, 0)); | ||
m_flex_db = shared_ptr<DBConnector>(new DBConnector(FLEX_COUNTER_DB, DBConnector::DEFAULT_UNIXSOCKET, 0)); | ||
// m_state_db = shared_ptr<DBConnector>(new DBConnector(STATE_DB, DBConnector::DEFAULT_UNIXSOCKET, 0)); |
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.
Please remove commented code
orchagent/intfsorch.cpp
Outdated
string value; | ||
for (auto it = m_rifsToAdd.begin(); it != m_rifsToAdd.end(); ++it) | ||
{ | ||
SWSS_LOG_NOTICE("Registering begin -> %s ", it->m_alias.c_str()); |
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 we have a separate for-loop for this log? With the log in below for-loop, this appears to be redundant.
orchagent/intfsorch.cpp
Outdated
for (auto it = m_rifsToAdd.begin(); it != m_rifsToAdd.end(); ) | ||
{ | ||
const auto id = sai_serialize_object_id(it->m_rif_id); | ||
SWSS_LOG_NOTICE("Registering %s, id %s", it->m_alias.c_str(), id.c_str()); |
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.
Suggest to change this to INFO level
orchagent/intfsorch.cpp
Outdated
} | ||
if (m_vidToRidTable->hget("", id, value)) | ||
{ | ||
SWSS_LOG_NOTICE("Registering %s it is ready", it->m_alias.c_str()); |
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.
Suggest INFO level
orchagent/intfsorch.cpp
Outdated
{ | ||
SWSS_LOG_ENTER(); | ||
|
||
SWSS_LOG_NOTICE("Registering %ld new intfs, deleting %ld ", m_rifsToAdd.size(), m_rifsToRemove.size()); |
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.
Suggest to change to INFO/DEBUG level
orchagent/intfsorch.h
Outdated
|
||
SelectableTimer* m_updateMapsTimer = nullptr; | ||
std::vector<Port> m_rifsToAdd; | ||
std::vector<Port> m_rifsToRemove; |
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.
I don't see this updated any where. Is it missed?
@@ -466,6 +507,9 @@ bool IntfsOrch::removeRouterIntfs(Port &port) | |||
return false; | |||
} | |||
|
|||
const auto id = sai_serialize_object_id(port.m_rif_id); | |||
removeRifFromFlexCounter(id, port.m_alias); |
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 is this called directly whereas addRifToFlexCounter
is based on timer?
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.
this is due to FC logic.
The Flex counter will try to determine the object's type based on the vid. The VIDTOIRID is not instantly available on create.
For the delete flow it's the opposite.
If there will be some issues with this logic, I believe the FC flow should be changed. Right now it's tricky to dynamically add/remove objects that need to be polled.
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.
How to ensure deleting flex counter is before deleting rif in syncd? If deleting rif happened before deleting flex counter, rid cannot be found in the VIDTOIRID.
Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>
Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>
retest this please |
…d ports (sonic-net#765) Provided a new ConfigMgmt class for - Config Validation - Adding ports - Deleting ports Signed-off-by: Praveen Chaudhary pchaudhary@linkedin.com
Signed-off-by: Mykola Faryma mykolaf@mellanox.com
What I did
Why I did it
How I verified it
Details if related