[Thinkit] Re-mask packet-dropping bug, Added helper function which re…

…turns an `EK_PHYSICAL_PORT` name given an `EK_PORT` name, Add ingress and egress port parameters to QoS test, Enable modify in smoke test, gNMI port breakout tests, BERT test, Ensure that P4RT port IDs are properly reflected in the State path before considering switch configured, Ensure that P4RT port IDs in testbed are mirrored & Remove duplicate code from switch_test_setup_helpers test. (#953) * [Thinkit] Add provision to pass in ingress and egress ports to tests. * [Comb] Move utilities to common libraries, Pull logic for buffer configuration into helper function, Migrate some users to ReadStreamChannelResponsesAndFinish, Toggle port speed before test, Adding packet, Add tolerance for Queue stats check, Renamed Packet to PacketAtPort, L3 admit tests should choose only ports that are UP & Update L3 Admit Test to not rely on a custom GNMI Config. * [Thinkit] Re-mask packet-dropping bug, Added helper function which returns an `EK_PHYSICAL_PORT` name given an `EK_PORT` name, Add ingress and egress port parameters to QoS test, Enable modify in smoke test, gNMI port breakout tests, BERT test, Ensure that P4RT port IDs are properly reflected in the State path before considering switch configured, Ensure that P4RT port IDs in testbed are mirrored & Remove duplicate code from switch_test_setup_helpers test. --------- Co-authored-by: kishanps <[email protected]>
sonic-net · Jan 22, 2025 · 4ebf0d7 · 4ebf0d7
1 parent 83884fb
commit 4ebf0d7
Show file tree

Hide file tree

Showing 13 changed files with 314 additions and 288 deletions.
diff --git a/tests/BUILD.bazel b/tests/BUILD.bazel
@@ -76,6 +76,7 @@ cc_library(
         "@com_google_absl//absl/strings",
         "@com_google_absl//absl/strings:str_format",
         "@com_google_googletest//:gtest",
+        "@com_googlesource_code_re2//:re2",
     ],
 )
 

diff --git a/tests/forwarding/BUILD.bazel b/tests/forwarding/BUILD.bazel
@@ -209,6 +209,7 @@ cc_library(
         "//gutil:proto",
         "//gutil:status_matchers",
         "//lib/gnmi:gnmi_helper",
+        "//lib/gnmi:openconfig_cc_proto",
         "//p4_pdpi:ir",
         "//p4_pdpi:ir_cc_proto",
         "//p4_pdpi:p4_runtime_session",
@@ -218,6 +219,7 @@ cc_library(
 	"//tests/lib:p4rt_fixed_table_programming_helper",
         "//tests/lib:packet_in_helper",
         "//tests/lib:switch_test_setup_helpers",
+        "//thinkit:mirror_testbed",
         "//thinkit:mirror_testbed_fixture",
         "//thinkit:switch",
         "@com_github_google_glog//:glog",

diff --git a/tests/forwarding/l3_admit_test.cc b/tests/forwarding/l3_admit_test.cc
@@ -555,7 +555,18 @@ TEST_P(L3AdmitTestFixture, L3AdmitCanUseInPortToRestrictMacAddresses) {
   }
   LOG(INFO) << "Done collecting packets.";
 
-  EXPECT_EQ(good_packet_count, kNumberOfTestPacket);
+  if (GetParam()
+          .testbed_interface->GetMirrorTestbed()
+          .Environment()
+          .MaskKnownFailures()) {
+    // TODO: Reduce expected count by tolerance level.
+    const int kDropTolerance = 1;
+    int adjusted_good_packets = kNumberOfTestPacket - kDropTolerance;
+    EXPECT_GE(good_packet_count, adjusted_good_packets);
+    EXPECT_LE(good_packet_count, kNumberOfTestPacket);
+  } else {
+    EXPECT_EQ(good_packet_count, kNumberOfTestPacket);
+  }
   EXPECT_EQ(bad_packet_count, 0);
 }
 

diff --git a/tests/forwarding/l3_admit_test.h b/tests/forwarding/l3_admit_test.h
@@ -19,10 +19,13 @@
 #include <tuple>
 
 #include "gutil/status_matchers.h"
+#include "lib/gnmi/gnmi_helper.h"
+#include "lib/gnmi/openconfig.pb.h"
 #include "p4/config/v1/p4info.pb.h"
 #include "p4_pdpi/ir.h"
 #include "p4_pdpi/ir.pb.h"
 #include "tests/lib/switch_test_setup_helpers.h"
+#include "thinkit/mirror_testbed.h"
 #include "thinkit/mirror_testbed_fixture.h"
 #include "gmock/gmock.h"
 
@@ -39,15 +42,34 @@ class L3AdmitTestFixture : public testing::TestWithParam<L3AdmitTestParams> {
   void SetUp() override {
     GetParam().testbed_interface->SetUp();
 
+    thinkit::MirrorTestbed& testbed =
+        GetParam().testbed_interface->GetMirrorTestbed();
+
     // Initialize the connection and clear table entries for the SUT and Control
     // switch.
     ASSERT_OK_AND_ASSIGN(
         std::tie(sut_p4rt_session_, control_switch_p4rt_session_),
         pins_test::ConfigureSwitchPairAndReturnP4RuntimeSessionPair(
-            GetParam().testbed_interface->GetMirrorTestbed().Sut(),
-            GetParam().testbed_interface->GetMirrorTestbed().ControlSwitch(),
+            testbed.Sut(), testbed.ControlSwitch(),
             /*gnmi_config=*/std::nullopt, GetParam().p4info));
 
+    // The L3Admit tests assume identical P4RT port IDs are used between the SUT
+    // and control switch. So sending a packet from a given port ID on the
+    // control switch means it will arrive on the same port ID on the SUT. To
+    // achieve this, we mirror the SUTs OpenConfig interfaces <-> P4RT Port ID
+    // mapping to the control switch.
+    ASSERT_OK_AND_ASSIGN(
+        std::unique_ptr<gnmi::gNMI::StubInterface> sut_gnmi_stub,
+        testbed.Sut().CreateGnmiStub());
+    ASSERT_OK_AND_ASSIGN(const pins_test::openconfig::Interfaces sut_interfaces,
+                         pins_test::GetInterfacesAsProto(
+                             *sut_gnmi_stub, gnmi::GetRequest::CONFIG));
+    ASSERT_OK_AND_ASSIGN(
+        std::unique_ptr<gnmi::gNMI::StubInterface> control_gnmi_stub,
+        testbed.ControlSwitch().CreateGnmiStub());
+    ASSERT_OK(
+        pins_test::SetInterfaceP4rtIds(*control_gnmi_stub, sut_interfaces));
+
     ASSERT_OK_AND_ASSIGN(ir_p4info_, pdpi::CreateIrP4Info(GetParam().p4info));
   }
 

diff --git a/tests/forwarding/smoke_test.cc b/tests/forwarding/smoke_test.cc
@@ -43,7 +43,7 @@ TEST_P(SmokeTestFixture, SessionsAreNonNull) {
   ASSERT_NE(&GetControlP4RuntimeSession(), nullptr);
 }
 
-TEST_P(SmokeTestFixture, AclTableAddDeleteOkButModifyFails) {
+TEST_P(SmokeTestFixture, AclTableAddModifyDeleteOk) {
   const sai::WriteRequest pd_insert = gutil::ParseProtoOrDie<sai::WriteRequest>(
       R"pb(
         updates {
@@ -117,11 +117,8 @@ TEST_P(SmokeTestFixture, AclTableAddDeleteOkButModifyFails) {
     }
   } while (!pi_read_response.entities(0).table_entry().has_counter_data());
 
-  // To avoid any test failures during the submission process (test running with
-  // the pre-7.1 image), skip this check for now.
-  // ASSERT_OK(pdpi::SetMetadataAndSendPiWriteRequest(&GetSutP4RuntimeSession(),
-  //                                                   pi_modify));
-
+  ASSERT_OK(pdpi::SetMetadataAndSendPiWriteRequest(&GetSutP4RuntimeSession(),
+                                                   pi_modify));
   // Delete works.
   ASSERT_OK(pdpi::SetMetadataAndSendPiWriteRequest(&GetSutP4RuntimeSession(),
                                                    pi_delete));

diff --git a/tests/gnoi/bert_tests.cc b/tests/gnoi/bert_tests.cc
@@ -491,26 +491,22 @@ void VerifyOperStatusOnSetOfSutInterfaces(gnmi::gNMI::StubInterface& gnmi_stub,
   }
 }
 
-absl::Status ValidateControlSwitchPortsUp(
-    thinkit::ControlDevice& control_device,
-    const std::vector<std::string>& interfaces) {
-  ASSIGN_OR_RETURN(
-      const auto up_interfaces,
-      control_device.GetUpLinks(absl::Span<const std::string>(interfaces)));
-
-  std::vector<std::string> down_interfaces;
-  for (const std::string& interface : interfaces) {
-    if (!up_interfaces.contains(interface)) {
-      down_interfaces.push_back(interface);
-    }
+absl::Status ValidatePortsUp(
+    thinkit::Switch& sut, thinkit::ControlDevice& control_device,
+    const std::vector<std::string>& sut_interfaces,
+    const std::vector<std::string>& control_device_interfaces) {
+  absl::Status sut_ports_up_status =
+      pins_test::PortsUp(sut, absl::Span<const std::string>(sut_interfaces));
+  absl::Status control_switch_ports_up_status = control_device.ValidatePortsUp(
+      absl::Span<const std::string>(control_device_interfaces));
+
+  if (sut_ports_up_status.ok() && control_switch_ports_up_status.ok()) {
+    return absl::OkStatus();
   }
 
-  if (!down_interfaces.empty()) {
-    return absl::InternalError(
-        absl::StrCat("Some interfaces are not up on control switch: ",
-                     absl::StrJoin(down_interfaces, "\n")));
-  }
-  return absl::OkStatus();
+  EXPECT_OK(sut_ports_up_status);
+  EXPECT_OK(control_switch_ports_up_status);
+  return absl::InternalError("PortsUp validation failed.");
 }
 
 std::vector<std::string> SelectNInterfacesFromList(
@@ -864,11 +860,9 @@ TEST_P(BertTest, StartBertSucceeds) {
     GTEST_SKIP() << "No SUT interfaces to test";
   }
   thinkit::Switch& sut = generic_testbed_->Sut();
-  ASSERT_OK(
-      pins_test::PortsUp(sut, absl::Span<const std::string>(sut_interfaces_)));
-
   thinkit::ControlDevice& control_device = generic_testbed_->ControlDevice();
-  ASSERT_OK(ValidateControlSwitchPortsUp(control_device, peer_interfaces_));
+  ASSERT_OK(
+      ValidatePortsUp(sut, control_device, sut_interfaces_, peer_interfaces_));
 
   // Select 2 operational state "up" ports.
   sut_test_interfaces_ = absl::GetFlag(FLAGS_interfaces);
@@ -1031,9 +1025,9 @@ TEST_P(BertTest, StartBertSucceeds) {
                               HasSubstr("exists"))))
         << "Response: " << bert_response.ShortDebugString();
   }
+
   ASSERT_OK(
-      pins_test::PortsUp(sut, absl::Span<const std::string>(sut_interfaces_)));
-  ASSERT_OK(ValidateControlSwitchPortsUp(control_device, peer_interfaces_));
+      ValidatePortsUp(sut, control_device, sut_interfaces_, peer_interfaces_));
 }
 
 // Runs the BERT test on current maximum allowed number of interfaces. During
@@ -1047,11 +1041,9 @@ TEST_P(BertTest, RunBertOnMaximumAllowedPorts) {
     GTEST_SKIP() << "No SUT interfaces to test";
   }
   thinkit::Switch& sut = generic_testbed_->Sut();
-  ASSERT_OK(
-      pins_test::PortsUp(sut, absl::Span<const std::string>(sut_interfaces_)));
-
   thinkit::ControlDevice& control_device = generic_testbed_->ControlDevice();
-  ASSERT_OK(ValidateControlSwitchPortsUp(control_device, peer_interfaces_));
+  ASSERT_OK(
+      ValidatePortsUp(sut, control_device, sut_interfaces_, peer_interfaces_));
 
   // Get all the interfaces that are operational status "UP".
   sut_test_interfaces_ = sut_interfaces_;
@@ -1180,8 +1172,7 @@ TEST_P(BertTest, RunBertOnMaximumAllowedPorts) {
   absl::SleepFor(kPortsUpWaitTime);
 
   ASSERT_OK(
-      pins_test::PortsUp(sut, absl::Span<const std::string>(sut_interfaces_)));
-  ASSERT_OK(ValidateControlSwitchPortsUp(control_device, peer_interfaces_));
+      ValidatePortsUp(sut, control_device, sut_interfaces_, peer_interfaces_));
 }
 
 // Run BERT on a port. Issue StopBERT on the SUT port, this causes BERT to
@@ -1192,11 +1183,9 @@ TEST_P(BertTest, StopBertSucceeds) {
     GTEST_SKIP() << "No SUT interfaces to test";
   }
   thinkit::Switch& sut = generic_testbed_->Sut();
-  ASSERT_OK(
-      pins_test::PortsUp(sut, absl::Span<const std::string>(sut_interfaces_)));
-
   thinkit::ControlDevice& control_device = generic_testbed_->ControlDevice();
-  ASSERT_OK(ValidateControlSwitchPortsUp(control_device, peer_interfaces_));
+  ASSERT_OK(
+      ValidatePortsUp(sut, control_device, sut_interfaces_, peer_interfaces_));
 
   // Select one operational state "up" port.
   std::string interface = absl::GetFlag(FLAGS_interface);
@@ -1350,8 +1339,7 @@ TEST_P(BertTest, StopBertSucceeds) {
   }
 
   ASSERT_OK(
-      pins_test::PortsUp(sut, absl::Span<const std::string>(sut_interfaces_)));
-  ASSERT_OK(ValidateControlSwitchPortsUp(control_device, peer_interfaces_));
+      ValidatePortsUp(sut, control_device, sut_interfaces_, peer_interfaces_));
 }
 
 }  // namespace bert
diff --git a/tests/lib/BUILD.bazel b/tests/lib/BUILD.bazel
@@ -95,6 +95,7 @@ cc_library(
         "//tests:thinkit_sanity_tests",
         "//thinkit:mirror_testbed",
         "//thinkit:switch",
+        "@com_github_gnmi//proto/gnmi:gnmi_cc_grpc_proto",
         "@com_github_google_glog//:glog",
         "@com_github_p4lang_p4runtime//:p4info_cc_proto",
         "@com_github_p4lang_p4runtime//:p4runtime_cc_proto",

diff --git a/tests/lib/switch_test_setup_helpers.cc b/tests/lib/switch_test_setup_helpers.cc
@@ -29,6 +29,7 @@
 #include "p4/v1/p4runtime.pb.h"
 #include "p4_pdpi/ir_tools.h"
 #include "p4_pdpi/p4_runtime_session.h"
+#include "proto/gnmi/gnmi.grpc.pb.h"
 #include "tests/thinkit_sanity_tests.h"
 
 namespace pins_test {
@@ -61,14 +62,6 @@ absl::Status TryClearingTableEntries(
   return absl::OkStatus();
 }
 
-absl::Status PushGnmiAndWaitForConvergence(thinkit::Switch& thinkit_switch,
-                                           const std::string& gnmi_config,
-                                           absl::Duration gnmi_timeout) {
-  RETURN_IF_ERROR(PushGnmiConfig(thinkit_switch, gnmi_config));
-  return WaitForGnmiPortIdConvergence(thinkit_switch, gnmi_config,
-                                      gnmi_timeout);
-}
-
 // Wrapper around `TestGnoiSystemColdReboot` that ensures we don't ignore fatal
 // failures.
 absl::Status Reboot(thinkit::Switch& thinkit_switch) {
@@ -162,13 +155,21 @@ ConfigureSwitchAndReturnP4RuntimeSession(
   RETURN_IF_ERROR(TryClearingTableEntries(thinkit_switch, metadata));
 
   if (gnmi_config.has_value()) {
-    RETURN_IF_ERROR(
-        PushGnmiAndWaitForConvergence(thinkit_switch, *gnmi_config,
-                                      /*gnmi_timeout=*/kGnmiTimeoutDefault));
+    RETURN_IF_ERROR(PushGnmiConfig(thinkit_switch, *gnmi_config));
   }
 
-  return CreateP4RuntimeSessionAndOptionallyPushP4Info(thinkit_switch, p4info,
-                                                       metadata);
+  ASSIGN_OR_RETURN(auto p4rt_session,
+                   CreateP4RuntimeSessionAndOptionallyPushP4Info(
+                       thinkit_switch, p4info, metadata));
+
+  // Ensure that the P4RT port IDs configured on the switch are reflected in the
+  // state before returning.
+  ASSIGN_OR_RETURN(std::unique_ptr<gnmi::gNMI::StubInterface> gnmi_stub,
+                   thinkit_switch.CreateGnmiStub());
+  RETURN_IF_ERROR(
+      WaitForGnmiPortIdConvergence(*gnmi_stub,
+                                   /*gnmi_timeout=*/kGnmiTimeoutDefault));
+  return p4rt_session;
 }
 
 absl::StatusOr<std::pair<std::unique_ptr<pdpi::P4RuntimeSession>,