[Optimization] Improve DSCP Transparency test by reducing the number of flows - #5837
[Optimization] Improve DSCP Transparency test by reducing the number of flows#5837RishabhAgarwal-2001 wants to merge 1 commit into
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 optimizes the DSCP Transparency test by significantly reducing the number of generated flows. By grouping DSCP values and streamlining the validation process, the test's resource consumption on the ATE is lowered, leading to a substantial improvement in execution time without compromising test efficacy. 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 #5837 / b9c90ddVirtual 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 DSCP transparency test to map QoS queues to specific DSCP value ranges, reducing the number of generated flows. It also optimizes getQueueCounters by batching gNMI operations to avoid the N+1 query anti-pattern, and introduces a waitForAndGetTaggedMetrics helper to replace custom polling loops. The reviewer feedback highlights an issue in waitForAndGetTaggedMetrics where the check for missing metrics is hardcoded to all 64 DSCP values across both ports, which will cause false-positive errors during timeouts in subtests that only use a subset of these metrics. The reviewer suggests dynamically passing the expected metric IDs to the helper function to ensure accurate error reporting.
| func waitForAndGetTaggedMetrics(t *testing.T, ate *ondatra.ATEDevice, expectedMetricCount int) map[string][]*otgtelemetry.Flow_TaggedMetric { | ||
| t.Helper() | ||
| deadline := time.Now().Add(5 * time.Minute) | ||
| taggedMetricsQuery := gnmi.OTG().FlowAny().TaggedMetricAny().State() | ||
|
|
||
| var metricsByTag map[string][]*otgtelemetry.Flow_TaggedMetric | ||
| var lastCount int | ||
| var lastLogTime time.Time | ||
|
|
||
| for time.Now().Before(deadline) { | ||
| metricsByTag = make(map[string][]*otgtelemetry.Flow_TaggedMetric) | ||
| for _, val := range gnmi.LookupAll(t, ate.OTG(), taggedMetricsQuery) { | ||
| if et, ok := val.Val(); ok { | ||
| var dscp string | ||
| var port string | ||
| for _, tag := range et.Tags { | ||
| tagName := tag.GetTagName() | ||
| if strings.Contains(tagName, "dst-dscp-") { | ||
| dscp = tag.GetTagValue().GetValueAsHex() | ||
| parts := strings.Split(tagName, "-") | ||
| port = parts[len(parts)-1] | ||
| } | ||
| } | ||
| if dscp != "" && port != "" { | ||
| dscpInt, err := strconv.ParseInt(strings.TrimPrefix(dscp, "0x"), 16, 64) | ||
| if err == nil { | ||
| metricID := fmt.Sprintf("dscp-%d-%s", dscpInt, port) | ||
| metricsByTag[metricID] = append(metricsByTag[metricID], et) | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if len(metricsByTag) >= expectedMetricCount { | ||
| return metricsByTag | ||
| } | ||
|
|
||
| if len(metricsByTag) != lastCount || time.Since(lastLogTime) > 10*time.Second { | ||
| t.Logf("Waiting for tagged metrics... collected %d/%d distinct metric IDs", len(metricsByTag), expectedMetricCount) | ||
| lastCount = len(metricsByTag) | ||
| lastLogTime = time.Now() | ||
| } | ||
|
|
||
| time.Sleep(2 * time.Second) | ||
| } | ||
|
|
||
| var missing []string | ||
| for _, port := range []string{"atePort2", "atePort3"} { | ||
| for dscp := 0; dscp < 64; dscp++ { | ||
| metricID := fmt.Sprintf("dscp-%d-%s", dscp, port) | ||
| if _, ok := metricsByTag[metricID]; !ok { | ||
| missing = append(missing, metricID) | ||
| } | ||
| } | ||
| } | ||
| t.Fatalf("Timed out waiting for tagged metrics to populate on the ATE. Got %d, expected %d. Missing: %v", len(metricsByTag), expectedMetricCount, missing) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
The waitForAndGetTaggedMetrics helper function currently hardcodes the check for missing metrics to include all 64 DSCP values across both atePort2 and atePort3. However, different subtests expect different subsets of these metrics:
- Sub Test Add the ISIS feature profile #1 (No-Congestion) only sends traffic from
atePort2(64 metrics). - Sub Test Add the initial ISIS base and adjacency feature profile #3 (NC1 Congestion) only sends traffic for DSCPs 48-63 from both ports (32 metrics).
If any of these tests time out, the error message will be cluttered with false-positive "missing" metrics (e.g., all 64 atePort3 metrics for Sub Test #1, or DSCPs 0-47 for Sub Test #3), making debugging extremely difficult.
We can fix this by passing the expected metric IDs directly to waitForAndGetTaggedMetrics so it can dynamically and accurately report only the metrics that were actually expected but missing.
func waitForAndGetTaggedMetrics(t *testing.T, ate *ondatra.ATEDevice, expectedMetricIDs []string) map[string][]*otgtelemetry.Flow_TaggedMetric {
t.Helper()
deadline := time.Now().Add(5 * time.Minute)
taggedMetricsQuery := gnmi.OTG().FlowAny().TaggedMetricAny().State()
expectedSet := make(map[string]bool)
for _, id := range expectedMetricIDs {
expectedSet[id] = true
}
var metricsByTag map[string][]*otgtelemetry.Flow_TaggedMetric
var lastCount int
var lastLogTime time.Time
for time.Now().Before(deadline) {
metricsByTag = make(map[string][]*otgtelemetry.Flow_TaggedMetric)
for _, val := range gnmi.LookupAll(t, ate.OTG(), taggedMetricsQuery) {
if et, ok := val.Val(); ok {
var dscp string
var port string
for _, tag := range et.Tags {
tagName := tag.GetTagName()
if strings.Contains(tagName, "dst-dscp-") {
dscp = tag.GetTagValue().GetValueAsHex()
parts := strings.Split(tagName, "-")
port = parts[len(parts)-1]
}
}
if dscp != "" && port != "" {
dscpInt, err := strconv.ParseInt(strings.TrimPrefix(dscp, "0x"), 16, 64)
if err == nil {
metricID := fmt.Sprintf("dscp-%d-%s", dscpInt, port)
if expectedSet[metricID] {
metricsByTag[metricID] = append(metricsByTag[metricID], et)
}
}
}
}
}
if len(metricsByTag) >= len(expectedMetricIDs) {
return metricsByTag
}
if len(metricsByTag) != lastCount || time.Since(lastLogTime) > 10*time.Second {
t.Logf("Waiting for tagged metrics... collected %d/%d distinct metric IDs", len(metricsByTag), len(expectedMetricIDs))
lastCount = len(metricsByTag)
lastLogTime = time.Now()
}
time.Sleep(2 * time.Second)
}
var missing []string
for _, id := range expectedMetricIDs {
if _, ok := metricsByTag[id]; !ok {
missing = append(missing, id)
}
}
t.Fatalf("Timed out waiting for tagged metrics to populate on the ATE. Got %d, expected %d. Missing: %v", len(metricsByTag), len(expectedMetricIDs), missing)
return nil
}References
- Refactor repeated telemetry wait or validation logic into reusable helper functions to improve maintainability and facilitate future optimizations (e.g., batching gNMI lookups) or cross-vendor support.
| // Wait for ALL tagged metrics to populate first | ||
| deadline := time.Now().Add(45 * time.Second) | ||
| metricsPopulated := false | ||
| for time.Now().Before(deadline) { | ||
| allFound := true | ||
| for dscpValue := 0; dscpValue < 64; dscpValue++ { | ||
| etPath := gnmi.OTG().Flow(fmt.Sprintf("dscp-%d-%s", dscpValue, atePort2.Name)).TaggedMetricAny() | ||
| vals := gnmi.LookupAll(t, ate.OTG(), etPath.State()) | ||
| if len(vals) == 0 || !vals[0].IsPresent() { | ||
| allFound = false | ||
| break | ||
| } | ||
| } | ||
| if allFound { | ||
| metricsPopulated = true | ||
| break | ||
| } | ||
| time.Sleep(2 * time.Second) | ||
| } | ||
| if !metricsPopulated { | ||
| t.Fatalf("Timed out waiting for tagged metrics to populate on the ATE") | ||
| } | ||
| metricsByTag := waitForAndGetTaggedMetrics(t, ate, 64) |
There was a problem hiding this comment.
Update the call to waitForAndGetTaggedMetrics to pass the expected metric IDs for this subtest (only atePort2 and DSCPs 0-63).
// Wait for ALL tagged metrics to populate first
var expectedIDs []string
for dscp := 0; dscp < 64; dscp++ {
expectedIDs = append(expectedIDs, fmt.Sprintf("dscp-%d-%s", dscp, atePort2.Name))
}
metricsByTag := waitForAndGetTaggedMetrics(t, ate, expectedIDs)References
- Refactor repeated telemetry wait or validation logic into reusable helper functions to improve maintainability and facilitate future optimizations (e.g., batching gNMI lookups) or cross-vendor support.
| } | ||
| time.Sleep(2 * time.Second) | ||
| } | ||
| metricsByTag := waitForAndGetTaggedMetrics(t, ate, 128) |
There was a problem hiding this comment.
Update the call to waitForAndGetTaggedMetrics to pass the expected metric IDs for this subtest (both atePort2 and atePort3 for DSCPs 0-63).
var expectedIDs []string
for _, sourceAtePort := range []*attrs.Attributes{atePort2, atePort3} {
for dscp := 0; dscp < 64; dscp++ {
expectedIDs = append(expectedIDs, fmt.Sprintf("dscp-%d-%s", dscp, sourceAtePort.Name))
}
}
metricsByTag := waitForAndGetTaggedMetrics(t, ate, expectedIDs)References
- Refactor repeated telemetry wait or validation logic into reusable helper functions to improve maintainability and facilitate future optimizations (e.g., batching gNMI lookups) or cross-vendor support.
| // Wait for ALL 48-63 tagged metrics to populate first | ||
| deadline := time.Now().Add(45 * time.Second) | ||
| metricsPopulated := false | ||
| for time.Now().Before(deadline) { | ||
| allFound := true | ||
| for _, sourceAtePort := range []*attrs.Attributes{atePort2, atePort3} { | ||
| for dscpValue := 48; dscpValue < 64; dscpValue++ { | ||
| etPath := gnmi.OTG().Flow(fmt.Sprintf("dscp-%d-%s", dscpValue, sourceAtePort.Name)).TaggedMetricAny() | ||
| vals := gnmi.LookupAll(t, ate.OTG(), etPath.State()) | ||
| if len(vals) == 0 || !vals[0].IsPresent() { | ||
| allFound = false | ||
| break | ||
| } | ||
| } | ||
| if !allFound { | ||
| break | ||
| } | ||
| } | ||
| if allFound { | ||
| metricsPopulated = true | ||
| break | ||
| } | ||
| time.Sleep(2 * time.Second) | ||
| } | ||
| metricsByTag := waitForAndGetTaggedMetrics(t, ate, 32) |
There was a problem hiding this comment.
Update the call to waitForAndGetTaggedMetrics to pass the expected metric IDs for this subtest (both atePort2 and atePort3 for DSCPs 48-63).
// Wait for ALL 48-63 tagged metrics to populate first
var expectedIDs []string
for _, sourceAtePort := range []*attrs.Attributes{atePort2, atePort3} {
for dscp := 48; dscp < 64; dscp++ {
expectedIDs = append(expectedIDs, fmt.Sprintf("dscp-%d-%s", dscp, sourceAtePort.Name))
}
}
metricsByTag := waitForAndGetTaggedMetrics(t, ate, expectedIDs)References
- Refactor repeated telemetry wait or validation logic into reusable helper functions to improve maintainability and facilitate future optimizations (e.g., batching gNMI lookups) or cross-vendor support.
2 participants
Changes in this PR are to reduce the number of flows used by the DSCP Transparency test. In its current state, the test used 64 flows which consumed a lot of resource on OTG causing it to take a very long time. In our runs, we noticed test running for around 2 hours and ultimately timing out!
With these changes, we use 7 flows on OTG, without impacting the efficacy or leniency of the test. This resulted in a gain of around 80% in performance with total runtime now down to around 25 minutes.
README has also been updated to reflect the new reality of the implementation.