Fix WRR QoS counter convergence check - #6022
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the reliability of WRR traffic tests by implementing a more robust mechanism for verifying QoS telemetry counters. By moving away from static delays and adopting a state-aware convergence check, the test suite now ensures that counter samples are fresh and reflect the actual traffic state, significantly reducing flakiness in performance validation. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
Pull Request Functional Test Report for #6022 / 2a00ed6Virtual Devices
Hardware Devices
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the WRR traffic test to track unique queue names and wait for QoS counters to converge after traffic stops using gnmi.Watch instead of a static sleep. The review feedback points out two important issues: first, comparing the DUT's source timestamp with the test runner's local time can cause flakiness due to clock skew, so RecvTimestamp should be used instead; second, failing to retrieve initial QoS counters should trigger a t.Fatalf rather than a t.Errorf with a continue to prevent the test from proceeding with uninitialized values and potentially passing falsely.
| awaitCounter := func(counterName, queue string, query ygnmi.SingletonQuery[uint64], before, delta uint64) uint64 { | ||
| t.Helper() | ||
| want := before + delta | ||
| isConverged := func(val *ygnmi.Value[uint64]) bool { | ||
| got, present := val.Val() | ||
| return present && val.Timestamp.After(trafficStopTime) && got >= want | ||
| } | ||
| count, ok := gnmi.Watch(t, dut, query, counterConvergenceTimeout, isConverged).Await(t) | ||
| if count == nil { | ||
| t.Errorf("No %s sample for queue %q on interface %q within %v; want >= %d with source timestamp after %v", counterName, queue, dp3.Name(), counterConvergenceTimeout, want, trafficStopTime) | ||
| return 0 | ||
| } | ||
| got, present := count.Val() | ||
| if !ok || !present { | ||
| t.Errorf("%s for queue %q on interface %q did not converge within %v: got %d (present=%v), want >= %d; source timestamp %v, receive timestamp %v, want source timestamp after %v", counterName, queue, dp3.Name(), counterConvergenceTimeout, got, present, want, count.Timestamp, count.RecvTimestamp, trafficStopTime) | ||
| return got | ||
| } | ||
| t.Logf("%s for queue %q converged to %d (want >= %d), source timestamp %v, receive timestamp %v", counterName, queue, got, want, count.Timestamp, count.RecvTimestamp) | ||
| return got | ||
| } |
There was a problem hiding this comment.
Comparing the DUT's source timestamp (val.Timestamp) with the test runner's local time (trafficStopTime) can lead to flaky or failing tests due to clock skew between the DUT and the test runner. In shared or virtual lab environments, clock synchronization is often imperfect.
Using val.RecvTimestamp instead of val.Timestamp is much safer because RecvTimestamp is generated locally by the gNMI client on the test runner, which shares the same clock domain as trafficStopTime.
| awaitCounter := func(counterName, queue string, query ygnmi.SingletonQuery[uint64], before, delta uint64) uint64 { | |
| t.Helper() | |
| want := before + delta | |
| isConverged := func(val *ygnmi.Value[uint64]) bool { | |
| got, present := val.Val() | |
| return present && val.Timestamp.After(trafficStopTime) && got >= want | |
| } | |
| count, ok := gnmi.Watch(t, dut, query, counterConvergenceTimeout, isConverged).Await(t) | |
| if count == nil { | |
| t.Errorf("No %s sample for queue %q on interface %q within %v; want >= %d with source timestamp after %v", counterName, queue, dp3.Name(), counterConvergenceTimeout, want, trafficStopTime) | |
| return 0 | |
| } | |
| got, present := count.Val() | |
| if !ok || !present { | |
| t.Errorf("%s for queue %q on interface %q did not converge within %v: got %d (present=%v), want >= %d; source timestamp %v, receive timestamp %v, want source timestamp after %v", counterName, queue, dp3.Name(), counterConvergenceTimeout, got, present, want, count.Timestamp, count.RecvTimestamp, trafficStopTime) | |
| return got | |
| } | |
| t.Logf("%s for queue %q converged to %d (want >= %d), source timestamp %v, receive timestamp %v", counterName, queue, got, want, count.Timestamp, count.RecvTimestamp) | |
| return got | |
| } | |
| awaitCounter := func(counterName, queue string, query ygnmi.SingletonQuery[uint64], before, delta uint64) uint64 { | |
| t.Helper() | |
| want := before + delta | |
| isConverged := func(val *ygnmi.Value[uint64]) bool { | |
| got, present := val.Val() | |
| return present && val.RecvTimestamp.After(trafficStopTime) && got >= want | |
| } | |
| count, ok := gnmi.Watch(t, dut, query, counterConvergenceTimeout, isConverged).Await(t) | |
| if count == nil { | |
| t.Errorf("No %s sample for queue %q on interface %q within %v; want >= %d with receive timestamp after %v", counterName, queue, dp3.Name(), counterConvergenceTimeout, want, trafficStopTime) | |
| return 0 | |
| } | |
| got, present := count.Val() | |
| if !ok || !present { | |
| t.Errorf("%s for queue %q on interface %q did not converge within %v: got %d (present=%v), want >= %d; source timestamp %v, receive timestamp %v, want receive timestamp after %v", counterName, queue, dp3.Name(), counterConvergenceTimeout, got, present, want, count.Timestamp, count.RecvTimestamp, trafficStopTime) | |
| return got | |
| } | |
| t.Logf("%s for queue %q converged to %d (want >= %d), source timestamp %v, receive timestamp %v", counterName, queue, got, want, count.Timestamp, count.RecvTimestamp) | |
| return got | |
| } |
| for queue := range queueNames { | ||
| count, ok := gnmi.Watch(t, dut, gnmi.OC().Qos().Interface(dp3.Name()).Output().Queue(queue).TransmitPkts().State(), timeout, isPresent).Await(t) | ||
| if !ok { | ||
| t.Errorf("TransmitPkts count for queue %q on interface %q not available within %v", data.queue, dp3.Name(), timeout) | ||
| t.Errorf("TransmitPkts count for queue %q on interface %q not available within %v", queue, dp3.Name(), timeout) | ||
| continue | ||
| } | ||
| dutQosPktsBeforeTraffic[data.queue], _ = count.Val() | ||
| dutQosPktsBeforeTraffic[queue], _ = count.Val() | ||
|
|
||
| count, ok = gnmi.Watch(t, dut, gnmi.OC().Qos().Interface(dp3.Name()).Output().Queue(data.queue).DroppedPkts().State(), timeout, isPresent).Await(t) | ||
| count, ok = gnmi.Watch(t, dut, gnmi.OC().Qos().Interface(dp3.Name()).Output().Queue(queue).DroppedPkts().State(), timeout, isPresent).Await(t) | ||
| if !ok { | ||
| t.Errorf("DroppedPkts count for queue %q on interface %q not available within %v", data.queue, dp3.Name(), timeout) | ||
| t.Errorf("DroppedPkts count for queue %q on interface %q not available within %v", queue, dp3.Name(), timeout) | ||
| continue | ||
| } | ||
| dutQosDroppedPktsBeforeTraffic[data.queue], _ = count.Val() | ||
| dutQosDroppedPktsBeforeTraffic[queue], _ = count.Val() | ||
| } |
There was a problem hiding this comment.
Using t.Errorf and continue when initial QoS counters cannot be retrieved allows the test to proceed with uninitialized (zero) values. This can lead to silent test bypasses or false positives because the subsequent convergence check (got >= want) might be immediately satisfied if the actual counter on the DUT is already non-zero.
Since the test logic depends on accurate initial counters to calculate the expected increments, we should fail fast using t.Fatalf if the initial telemetry is unavailable.
for queue := range queueNames {
count, ok := gnmi.Watch(t, dut, gnmi.OC().Qos().Interface(dp3.Name()).Output().Queue(queue).TransmitPkts().State(), timeout, isPresent).Await(t)
if !ok {
t.Fatalf("TransmitPkts count for queue %q on interface %q not available within %v", queue, dp3.Name(), timeout)
}
dutQosPktsBeforeTraffic[queue], _ = count.Val()
count, ok = gnmi.Watch(t, dut, gnmi.OC().Qos().Interface(dp3.Name()).Output().Queue(queue).DroppedPkts().State(), timeout, isPresent).Await(t)
if !ok {
t.Fatalf("DroppedPkts count for queue %q on interface %q not available within %v", queue, dp3.Name(), timeout)
}
dutQosDroppedPktsBeforeTraffic[queue], _ = count.Val()
}References
- In tests, t.Fatalf is preferred over t.Errorf when a failure makes subsequent test steps meaningless, as this fails fast and reduces overall test execution time.
2 participants
Summary
Improve WRR traffic-test reliability by waiting for QoS telemetry counters to converge after traffic stops.
Changes