Skip to content

Check PF counters through cli if not supported in OC #5002

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.

Sign up for GitHub

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

Open
ijernoiu wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Check PF counters through cli if not supported in OC #5002

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
901f36f
Check PF counters through cli if not supported in OC
ijernoiu Jan 21, 2026
0328a82
Merge branch 'main' into pf-1.2-cliTelemetryUpdate
ijernoiu Jan 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 61 additions & 16 deletions feature/policy_forwarding/encapsulation/otg_tests/encap_gre_ipv4/encap_gre_ipv4_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
package encap_gre_ipv4_test

import (
"context"
"fmt"
"os"
"strconv"
"strings"
"testing"
"time"

Expand Down Expand Up @@ -43,6 +45,10 @@ const (
port1 = "port1"
port2 = "port2"
port3 = "port3"
ruleIPv4Pass = "rule_ipv4_pass"
ruleIPv6Pass = "rule_ipv6_pass"
ruleIPv4Encap = "rule_ipv4_encap"
ruleIPv6Encap = "rule_ipv6_encap"
)

var (
Expand All @@ -55,17 +61,17 @@ var (
dutLoopbackAttrs attrs.Attributes

ruleSequenceMap = map[string]uint8{
"rule-src1-v4": 1,
"rule-src1-v6": 2,
"rule-src2-v4": 3,
"rule-src2-v6": 4,
ruleIPv4Pass: 1,
ruleIPv6Pass: 2,
ruleIPv4Encap: 3,
ruleIPv6Encap: 4,
}

ruleMatchedPackets = map[string]uint64{
"rule-src1-v4": 0,
"rule-src1-v6": 0,
"rule-src2-v4": 0,
"rule-src2-v6": 0,
ruleIPv4Pass: 0,
ruleIPv6Pass: 0,
ruleIPv4Encap: 0,
ruleIPv6Encap: 0,
}

dutPort1 = attrs.Attributes{Desc: "Dut port 1", IPv4: "192.0.2.1", IPv4Len: 30, IPv6: "2001:DB8:0::1", IPv6Len: 126}
Expand Down Expand Up @@ -143,7 +149,7 @@ func TestEncapGREIPv4(t *testing.T) {
},
checkEncapDscp: false,
checkEncapLoadBalanced: true,
policyRule: "rule-src1-v4",
policyRule: ruleIPv4Encap,
flowName: "FlowTC1",
},
{
Expand All @@ -164,7 +170,7 @@ func TestEncapGREIPv4(t *testing.T) {
},
checkEncapDscp: false,
checkEncapLoadBalanced: true,
policyRule: "rule-src1-v6",
policyRule: ruleIPv6Encap,
flowName: "FlowTC2",
},
{
Expand All @@ -178,7 +184,7 @@ func TestEncapGREIPv4(t *testing.T) {
},
checkEncapDscp: false,
checkEncapLoadBalanced: false,
policyRule: "rule-src2-v4",
policyRule: ruleIPv4Pass,
flowName: "FlowTC3",
},
{
Expand All @@ -192,7 +198,7 @@ func TestEncapGREIPv4(t *testing.T) {
},
checkEncapDscp: false,
checkEncapLoadBalanced: false,
policyRule: "rule-src2-v6",
policyRule: ruleIPv6Pass,
flowName: "FlowTC4",
},
{
Expand All @@ -218,7 +224,7 @@ func TestEncapGREIPv4(t *testing.T) {
},
checkEncapDscp: true,
checkEncapLoadBalanced: false,
policyRule: "rule-src1-v4",
policyRule: ruleIPv4Encap,
flowName: "FlowTC5",
},
{
Expand All @@ -244,7 +250,7 @@ func TestEncapGREIPv4(t *testing.T) {
},
checkEncapDscp: true,
checkEncapLoadBalanced: false,
policyRule: "rule-src1-v6",
policyRule: ruleIPv6Encap,
flowName: "FlowTC6",
},
}
Expand Down Expand Up @@ -511,14 +517,53 @@ func pushQosClassifierToDUT(t *testing.T, dut *ondatra.DUTDevice, qos *oc.Qos, i

}

func runCliCommand(t *testing.T, dut *ondatra.DUTDevice, cliCommand string) string {
cliClient := dut.RawAPIs().CLI(t)
output, err := cliClient.RunCommand(context.Background(), cliCommand)
if err != nil {
t.Errorf("Failed to execute CLI command '%s': %v", cliCommand, err)
Comment on lines +523 to +524
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

If the CLI command fails to execute, it's a critical issue that prevents the test from proceeding meaningfully. Using t.Fatalf here would immediately stop the test and mark it as failed, which is generally preferred for unrecoverable errors during test setup or execution.

Suggested change
if err != nil {
t.Errorf("Failed to execute CLI command '%s': %v", cliCommand, err)
if err != nil {
t.Fatalf("Failed to execute CLI command '%s': %v", cliCommand, err)
}

}
t.Logf("Received from cli: %s", output.Output())
return output.Output()
}

func checkPolicyStatistics(t *testing.T, dut *ondatra.DUTDevice, tc testCase) {
if deviations.PolicyForwardingGreEncapsulationOcUnsupported(dut) {
t.Fatalf("Dut %s %s %s does not support checking policy statistics through OC", dut.Vendor(), dut.Model(), dut.Version())
if deviations.PolicyForwardingCountersUnsupported(dut) {
checkPolicyStatisticsFromCLI(t, dut, tc)
} else {
checkPolicyStatisticsFromOC(t, dut, tc)
}
}

func checkPolicyStatisticsFromCLI(t *testing.T, dut *ondatra.DUTDevice, tc testCase) {
t.Logf("Checking policy statistics for flow %s", tc.flowName)
switch dut.Vendor() {
case ondatra.ARISTA:
//extract text from CLI output between rule name and packets
policyCountersCommand := fmt.Sprintf(`show traffic-policy %s interface counters | grep %s | sed -e 's/.*%s:\(.*\)packets.*/\1/'`, trafficPolicyName, tc.policyRule, tc.policyRule)
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The use of sed with a complex regular expression to extract data from CLI output can be brittle. CLI output formats can change, leading to test failures. Consider a more robust parsing method, such as splitting the output into lines and then fields, or using a more flexible regex if direct parsing is unavoidable. This improves maintainability and reduces the risk of future breakage.

For example, if the output format changes slightly, this regex might fail. A more resilient approach might involve:

  1. Splitting the cliOutput by newline to get individual lines.
  2. Iterating through lines to find the one containing tc.policyRule.
  3. Using strings.Fields or a simpler regex on that specific line to extract the number.

cliOutput := runCliCommand(t, dut, policyCountersCommand)
cliOutput = strings.TrimSpace(cliOutput)
if cliOutput == "" {
t.Errorf("No output for CLI command '%s'", policyCountersCommand)
return
}
totalMatched, err := strconv.ParseUint(cliOutput, 10, 64)
if err != nil {
t.Errorf("Invalid response for CLI command '%s': %v", cliOutput, err)
Comment on lines +551 to +552
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Similar to the runCliCommand error handling, if strconv.ParseUint fails, it indicates a fundamental issue with parsing the CLI output, making further test assertions unreliable. Using t.Fatalf here would ensure the test stops immediately, preventing misleading results.

if err != nil {
		t.Fatalf("Invalid response for CLI command '%s': %v", cliOutput, err)
	}

return
}
previouslyMatched := ruleMatchedPackets[tc.policyRule]
if totalMatched != previouslyMatched+noOfPackets {
t.Errorf("Expected %d packets matched by policy %s rule %s for flow %s, but got %d", noOfPackets, trafficPolicyName, tc.policyRule, tc.flowName, totalMatched-previouslyMatched)
} else {
t.Logf("%d packets matched by policy %s rule %s for flow %s", totalMatched-previouslyMatched, trafficPolicyName, tc.policyRule, tc.flowName)
}
ruleMatchedPackets[tc.policyRule] = totalMatched
default:
t.Errorf("Vendor %s is not supported for policy statistics check through CLI", dut.Vendor())
}
}

func checkPolicyStatisticsFromOC(t *testing.T, dut *ondatra.DUTDevice, tc testCase) {
totalMatched := gnmi.Get(t, dut, gnmi.OC().NetworkInstance(deviations.DefaultNetworkInstance(dut)).PolicyForwarding().Policy(trafficPolicyName).Rule(uint32(ruleSequenceMap[tc.policyRule])).MatchedPkts().State())
previouslyMatched := ruleMatchedPackets[tc.policyRule]
Expand Down
1 change: 1 addition & 0 deletions feature/policy_forwarding/encapsulation/otg_tests/encap_gre_ipv4/metadata.textproto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ platform_exceptions: {
policy_forwarding_to_next_hop_oc_unsupported: true
qos_remark_oc_unsupported: true
policy_forwarding_gre_encapsulation_oc_unsupported: true
policy_forwarding_counters_unsupported: true
}
}
tags: TAGS_AGGREGATION
6 changes: 6 additions & 0 deletions internal/deviations/deviations.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1859,3 +1859,9 @@ func NonStandardGRPCPort(dut *ondatra.DUTDevice) bool {
func TemperatureSensorCheck(dut *ondatra.DUTDevice) bool {
return lookupDUTDeviations(dut).GetTemperatureSensorCheck()
}

// Arista b/459507184
// Device does not support policy forwarding counters through OC
func PolicyForwardingCountersUnsupported(dut *ondatra.DUTDevice) bool {
return lookupDUTDeviations(dut).GetPolicyForwardingCountersUnsupported()
}
4 changes: 4 additions & 0 deletions proto/metadata.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1111,6 +1111,10 @@ message Metadata {
// Cisco: https://partnerissuetracker.corp.google.com/issues/475715208
bool temperature_sensor_check = 367;

// Arista b/459507184
// Device does not support policy forwarding counters through OC
bool policy_forwarding_counters_unsupported = 368;

// Reserved field numbers and identifiers.
reserved 84, 9, 28, 20, 38, 43, 90, 97, 55, 89, 19, 36, 35, 40, 113, 131, 141, 173, 234, 254, 231, 300;
}
Expand Down
25 changes: 18 additions & 7 deletions proto/metadata_go_proto/metadata.pb.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading