-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Update sflow HLD according related code. #746
Conversation
- What I did Update the sflow HLD according the code in sonic-swss and sonic-utilities. - Why I did it The HLD is not match the code behavior. - How I verified it Below is related code: https://github.com/Azure/sonic-utilities/blob/c7c01e4d30ccb88dd88645cd9390ed340e5ad119/config/main.py#L4243 https://github.com/Azure/sonic-swss/blob/321291aa7aca89a3ccef2bed36aff074eac1054f/cfgmgr/sflowmgr.h#L21 https://github.com/Azure/sonic-utilities/blob/c7c01e4d30ccb88dd88645cd9390ed340e5ad119/config/main.py#L4120 - Details if related Signed-off-by: Fred Yu [email protected]
@dgsudharsan , @padmanarayana to review |
doc/sflow/sflow_hld.md
Outdated
* 1-in-4,000 for a 40G link | ||
* 1-in-5,000 for a 50G link | ||
* 1-in-10,000 for a 100G link | ||
* 1-in-40,000 for a 400G link |
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.
Other changes look good. w.r.t the sample rate, discussed with @prsunny and @dgsudharsan. The HLD is good (i.e. the default sampling rate (of 1 in N packets sampled) should be (ifSpeed (in bits per sec) / 1e6). This is based on recommendation from InMon (as described in https://blog.sflow.com/2013/06/large-flow-detection.html : check the Link Speed/Sampling Rate table). Can you please raise PR for https://github.com/Azure/sonic-swss/blob/321291aa7aca89a3ccef2bed36aff074eac1054f/cfgmgr/sflowmgr.h#L21 to change the SFLOW_SAMPLE_RATE_VALUE_* to be (ifSpeed / 1e6) ?
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.
Sure, I will raise another PR for this.
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.
Hi @padmanarayana , I have raised a PR: sonic-swss#1623.
I also restored the default sample rate to "ifSpeed / 1e6" in this PR.
What I did
Update the sflow HLD according the code in sonic-swss and sonic-utilities.
Why I did it
The HLD is not match the code behavior.
How I verified it
Below is related code:
https://github.com/Azure/sonic-utilities/blob/c7c01e4d30ccb88dd88645cd9390ed340e5ad119/config/main.py#L4243
https://github.com/Azure/sonic-swss/blob/321291aa7aca89a3ccef2bed36aff074eac1054f/cfgmgr/sflowmgr.h#L21
https://github.com/Azure/sonic-utilities/blob/c7c01e4d30ccb88dd88645cd9390ed340e5ad119/config/main.py#L4120
Related PR:
[sflow] Fix sample rate defaults for sflow in CLI reference sonic-utilities#1367
Details if related
Signed-off-by: Fred Yu [email protected]