Fix for SFLOW-1 to work with non-default sampling rate#3978
Fix for SFLOW-1 to work with non-default sampling rate#3978dplore merged 2 commits intoopenconfig:mainfrom
Conversation
Pull Request Functional Test Report for #3978 / ec04da6Virtual Devices
Hardware Devices
|
masood-shah
left a comment
There was a problem hiding this comment.
this implements the use of the deviation but doesnt explain why the default might be unsupported or where the value 262144 comes from. could you please link the specific technical reference (eg, Cisco doc ID, bug) that documents this platform's min supported ingress sFlow sampling rate and the technical justification for its deviation from the default within this test suite?
We need to decide if the deviated sample rate is acceptable for this test? The 1/1,000,000 rate was our operational standard. If we want to allow different rates or a range of rates, this test is the place to specify it. Can you help figure out our requirement @masood-shah ? |
Happy to help, but first, could someone explain how the values 1/1,000,000 and 262,144 were derived or determined? |
the min. and max. supported sample rate can differ based on platforms, for 8000 it can sample from 1:1 to 1:262144 |
| c.SampleSize = ygot.Uint16(256) | ||
| c.IngressSamplingRate = ygot.Uint32(1000000) | ||
| // override ingress sampling rate if default value of 1000000 is not supported | ||
| c.SetIngressSamplingRate(deviations.SflowIngressMinSamplingRate(d)) |
There was a problem hiding this comment.
This needs to be wrapped with a conditional (if the deviation exists, then use it).
for example:
if deviations.SflowIngressMinSamplingRate(d) {
switch d.Vendor() {
case ondatra.CISCO:
c.SetIngressSamplingRate(deviations.SflowIngressMinSamplingRate(d))
}
} else {
c.IngressSamplingRate = ygot.Uint32(1000000)
}
It is still up to @masood-shah to decide to approve
There was a problem hiding this comment.
Masood approved, so we just need to see the if statement implemented and then we can merge it
There was a problem hiding this comment.
@dplore Could you help review the PR (run the checks) , the requested changes have been made
4fcc6a6 to
7fb42c8
Compare
Pull Request Test Coverage Report for Build 16622485930Details
💛 - Coveralls |
This PR adds a deviation to allow a different ingress sampling rate than 1/1,000,000 for platforms.
No change is needed for a platform that supports default ingress sampling rate.