-
Notifications
You must be signed in to change notification settings - Fork 531
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
[sflowmgrd] Infer sampling rate dynamically based on oper speed #2799
Conversation
Signed-off-by: Vivek Reddy Karri <vkarri@nvidia.com>
Signed-off-by: Vivek Reddy Karri <vkarri@nvidia.com>
Signed-off-by: Vivek Reddy Karri <vkarri@nvidia.com>
Signed-off-by: Vivek Reddy Karri <vkarri@nvidia.com>
Signed-off-by: Vivek Reddy Karri <vkarri@nvidia.com>
Signed-off-by: Vivek Reddy Karri <vkarri@nvidia.com>
Signed-off-by: Vivek Reddy Karri <vkarri@nvidia.com>
Signed-off-by: Vivek Reddy Karri <vkarri@nvidia.com>
Signed-off-by: Vivek Reddy Karri <vkarri@nvidia.com>
@venkatmahalingam @prsunny Can you please help to review the changes? |
@Gokulnath-Raja Please take a look at the changes. |
Kindly test following,
|
string alias = kfvKey(t); | ||
string op = kfvOp(t); | ||
auto values = kfvFieldsValues(t); | ||
string oper_speed = ""; |
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.
Do we need to default as NA_SPEED instead of empty check??
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.
orchagent may write NA_SPEED to oper_speed in STATE_DB, setting it as empty will let us know if oper_speed field is written to state-db
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.
ok thanks for the clarification
m_sflowPortConfMap[alias].speed.c_str(), | ||
oper_speed.c_str()); | ||
/* oper_speed is updated by orchagent if the vendor supports and oper status is up */ | ||
if (m_sflowPortConfMap[alias].oper_speed != oper_speed && !oper_speed.empty()) |
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.
Check first oper_speed != NA_SPEED && m_sflowPortConfMap[alias].oper_speed != oper_speed
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.
!oper_speed.empty() check is required to understand if something is written to STATE_DB. (otherwise we'll have duplicate writes to APPL_DB). If the oper_speed happen to be NA findSamplingRate() has logic to check if the speed is NA and if so, written the sampling rate based on configured speed
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.
ok thanks for the clarification
Hi, I've tested those scenarios and also ran sonic-mgmt tests to verify the functionality. |
@Gokulnath-Raja Can you please re-review? |
…c-net#2799) * Remove sflowSpeedRateInitMap and infer the default sampling rate based on speed on the interface. * Make the rate inference based on oper_speed.
@vivekrnv Can you please update the Quality Metric (Alpha/Beta/GA) for the feature either in this PR comments or in HLD itself based on https://github.com/sonic-net/SONiC/blob/master/doc/SONiC%20feature%20quality%20definition.md |
What I did
Why I did it
How I verified it
Test normal speed change without autoneg
Test rate change with autoneg
Ethernet205 <-> Ethernet209 Back to back connected