[Thinkit] Add L3 Admit for watch port testDs, don't ignore fatal fail…

…ures during switch reboot, Remove obsolete workaround & gNMI port interface tests. (#929)

* [Thinkit] Do not parameterize tests over gNMI config, Remove test access to sai::GetP4Info, Add some tolerance for ECN marked packet count, Log Ixia warning & Add Ixia and gNMI utilities needed for ECN testing.

* [Thinkit] Add helper libraries for manipulating and reading switch QoS config, Update max MTU & Remove workaround to create L3 ingress interfaces.

* [Thinkit] Add DSCP/scheduling test case.

* [Thinkit] Add test of round-robin scheduling weights.

* [Thinkit] Add test for strict queues, ensuring they are strictly prioritized.

* [Thinkit] Add L3 Admit for watch port testDs, don't ignore fatal failures during switch reboot, Remove obsolete workaround, gNMI port interface tests & gNMI port interface tests.

---------

Co-authored-by: kishanps <kishanps@google.com>
Co-authored-by: smolkaj <smolkaj@google.com>

tests/BUILD.bazel

@@ -74,6 +74,7 @@ cc_library(
         "@com_google_absl//absl/status",
         "@com_google_absl//absl/status:statusor",
         "@com_google_absl//absl/strings",
+        "@com_google_absl//absl/strings:str_format",
         "@com_google_googletest//:gtest",
     ],
 )

tests/forwarding/BUILD.bazel

@@ -306,6 +306,7 @@ cc_library(
         "//sai_p4/instantiations/google:sai_p4info_cc",
         "//sai_p4/instantiations/google:sai_pd_cc_proto",
         "//tests:thinkit_sanity_tests",
+        "//tests/lib:p4rt_fixed_table_programming_helper",
         "//thinkit:mirror_testbed",
         "//thinkit:mirror_testbed_fixture",
         "//thinkit:switch",

tests/forwarding/watch_port_test.cc

@@ -64,6 +64,7 @@
 #include "tests/forwarding/group_programming_util.h"
 #include "tests/forwarding/packet_test_util.h"
 #include "tests/forwarding/util.h"
+#include "tests/lib/p4rt_fixed_table_programming_helper.h"
 #include "tests/thinkit_sanity_tests.h"
 #include "thinkit/mirror_testbed.h"
 #include "thinkit/mirror_testbed_fixture.h"
@@ -273,6 +274,17 @@ absl::StatusOr<absl::flat_hash_map<int, int>> CountNumPacketsPerPort(
   return num_packets_per_port;
 }
 
+// Program L3 Admit table for the given mac-address.
+absl::Status ProgramL3Admit(pdpi::P4RuntimeSession& p4_session,
+                            const pdpi::IrP4Info& ir_p4info,
+                            const L3AdmitOptions& options) {
+  p4::v1::WriteRequest write_request;
+  ASSIGN_OR_RETURN(
+      *write_request.add_updates(),
+      L3AdmitTableUpdate(ir_p4info, p4::v1::Update::INSERT, options));
+  return pdpi::SetMetadataAndSendPiWriteRequest(&p4_session, write_request);
+}
+
 // Sends N packets from the control switch to sut at a rate of 500 packets/sec.
 absl::Status SendNPacketsToSut(int num_packets,
                                const TestConfiguration& test_config,
@@ -581,6 +593,15 @@ TEST_P(WatchPortTestFixture, VerifyBasicWcmpPacketDistribution) {
                                            p4::v1::Update::INSERT))
       << "Failed to program WCMP group: ";
 
+  // Allow the destination mac address to L3.
+  ASSERT_OK(ProgramL3Admit(
+      *sut_p4_session_, ir_p4info,
+      L3AdmitOptions{
+          .priority = 2070,
+          .dst_mac = std ::make_pair(pins::GetIthDstMac(0).ToString(),
+                                     "FF:FF:FF:FF:FF:FF"),
+      }));
+
   // Program default routing for all packets on SUT.
   ASSERT_OK(ProgramDefaultRoutes(*sut_p4_session_, ir_p4info, kVrfId,
                                  p4::v1::Update::INSERT));
@@ -660,6 +681,16 @@ TEST_P(WatchPortTestFixture, VerifyBasicWatchPortAction) {
   ASSERT_OK(pins::ProgramGroupWithMembers(environment, *sut_p4_session_,
                                            ir_p4info, kGroupId, members,
                                            p4::v1::Update::INSERT));
+
+  // Allow the destination mac address to L3.
+  ASSERT_OK(ProgramL3Admit(
+      *sut_p4_session_, ir_p4info,
+      L3AdmitOptions{
+          .priority = 2070,
+          .dst_mac = std ::make_pair(pins::GetIthDstMac(0).ToString(),
+                                     "FF:FF:FF:FF:FF:FF"),
+      }));
+
   // Program default routing for all packets on SUT.
   ASSERT_OK(ProgramDefaultRoutes(*sut_p4_session_, ir_p4info, kVrfId,
                                  p4::v1::Update::INSERT));
@@ -774,6 +805,16 @@ TEST_P(WatchPortTestFixture, VerifyWatchPortActionInCriticalState) {
   ASSERT_OK(pins::ProgramGroupWithMembers(environment, *sut_p4_session_,
                                            ir_p4info, kGroupId, members,
                                            p4::v1::Update::INSERT));
+
+  // Allow the destination mac address to L3.
+  ASSERT_OK(ProgramL3Admit(
+      *sut_p4_session_, ir_p4info,
+      L3AdmitOptions{
+          .priority = 2070,
+          .dst_mac = std ::make_pair(pins::GetIthDstMac(0).ToString(),
+                                     "FF:FF:FF:FF:FF:FF"),
+      }));
+
   // Program default routing for all packets on SUT.
   ASSERT_OK(ProgramDefaultRoutes(*sut_p4_session_, ir_p4info, kVrfId,
                                  p4::v1::Update::INSERT));
@@ -881,6 +922,16 @@ TEST_P(WatchPortTestFixture, VerifyWatchPortActionForSingleMember) {
   ASSERT_OK(pins::ProgramGroupWithMembers(environment, *sut_p4_session_,
                                            ir_p4info, kGroupId, members,
                                            p4::v1::Update::INSERT));
+
+  // Allow the destination mac address to L3.
+  ASSERT_OK(ProgramL3Admit(
+      *sut_p4_session_, ir_p4info,
+      L3AdmitOptions{
+          .priority = 2070,
+          .dst_mac = std ::make_pair(pins::GetIthDstMac(0).ToString(),
+                                     "FF:FF:FF:FF:FF:FF"),
+      }));
+
   // Program default routing for all packets on SUT.
   ASSERT_OK(ProgramDefaultRoutes(*sut_p4_session_, ir_p4info, kVrfId,
                                  p4::v1::Update::INSERT));
@@ -994,6 +1045,16 @@ TEST_P(WatchPortTestFixture, VerifyWatchPortActionForMemberModify) {
   ASSERT_OK(pins::ProgramGroupWithMembers(environment, *sut_p4_session_,
                                            ir_p4info, kGroupId, members,
                                            p4::v1::Update::INSERT));
+
+  // Allow the destination mac address to L3.
+  ASSERT_OK(ProgramL3Admit(
+      *sut_p4_session_, ir_p4info,
+      L3AdmitOptions{
+          .priority = 2070,
+          .dst_mac = std ::make_pair(pins::GetIthDstMac(0).ToString(),
+                                     "FF:FF:FF:FF:FF:FF"),
+      }));
+
   // Program default routing for all packets on SUT.
   ASSERT_OK(ProgramDefaultRoutes(*sut_p4_session_, ir_p4info, kVrfId,
                                  p4::v1::Update::INSERT));
@@ -1123,6 +1184,16 @@ TEST_P(WatchPortTestFixture, VerifyWatchPortActionForDownPortMemberInsert) {
   ASSERT_OK(pins::ProgramGroupWithMembers(environment, *sut_p4_session_,
                                            ir_p4info, kGroupId, members,
                                            p4::v1::Update::INSERT));
+
+  // Allow the destination mac address to L3.
+  ASSERT_OK(ProgramL3Admit(
+      *sut_p4_session_, ir_p4info,
+      L3AdmitOptions{
+          .priority = 2070,
+          .dst_mac = std ::make_pair(pins::GetIthDstMac(0).ToString(),
+                                     "FF:FF:FF:FF:FF:FF"),
+      }));
+
   // Program default routing for all packets on SUT.
   ASSERT_OK(ProgramDefaultRoutes(*sut_p4_session_, ir_p4info, kVrfId,
                                  p4::v1::Update::INSERT));

tests/lib/BUILD.bazel

@@ -85,6 +85,7 @@ cc_library(
     deps = [
         "//gutil:collections",
         "//gutil:proto",
+        "//gutil:status",
         "//lib/gnmi:gnmi_helper",
         "//lib/gnmi:openconfig_cc_proto",
         "//lib/validator:validator_lib",
@@ -106,6 +107,7 @@ cc_library(
         "@com_google_absl//absl/time",
         "@com_google_absl//absl/types:optional",
         "@com_google_absl//absl/types:span",
+        "@com_google_googletest//:gtest",
     ],
 )
 

tests/lib/switch_test_setup_helpers.cc

@@ -18,6 +18,7 @@
 #include "absl/types/optional.h"
 #include "absl/types/span.h"
 #include "glog/logging.h"
+#include "gtest/gtest.h"
 #include "gutil/collections.h"
 #include "gutil/proto.h"
 #include "gutil/status.h"
@@ -54,6 +55,21 @@ absl::Status PushGnmiAndWaitForConvergence(thinkit::Switch& thinkit_switch,
                                       gnmi_timeout);
 }
 
+// Wrapper around `TestGnoiSystemColdReboot` that ensures we don't ignore fatal
+// failures.
+absl::Status Reboot(thinkit::Switch& thinkit_switch) {
+  if (::testing::Test::HasFatalFailure()) {
+    return gutil::UnknownErrorBuilder()
+           << "skipping switch reboot due to pre-existing, fatal test failure";
+  }
+  TestGnoiSystemColdReboot(thinkit_switch);
+  if (::testing::Test::HasFatalFailure()) {
+    return gutil::UnknownErrorBuilder()
+           << "switch reboot failed with fatal error";
+  }
+  return absl::OkStatus();
+}
+
 absl::StatusOr<std::unique_ptr<pdpi::P4RuntimeSession>>
 CreateP4RuntimeSessionAndOptionallyPushP4Info(
     thinkit::Switch& thinkit_switch,
@@ -75,7 +91,7 @@ CreateP4RuntimeSessionAndOptionallyPushP4Info(
              "but I am asked to push a P4Info with the following diff:\n"
           << p4info_diff;
       RETURN_IF_ERROR(session->Finish());
-      TestGnoiSystemColdReboot(thinkit_switch);
+      RETURN_IF_ERROR(Reboot(thinkit_switch));
       // Reconnect after reboot.
       ASSIGN_OR_RETURN(
           session, pdpi::P4RuntimeSession::Create(thinkit_switch, metadata));

tests/qos/qos_test_util.cc

@@ -712,7 +712,6 @@ absl::Status SetBufferConfigParameters(
   //         << config_state_diff;
   // }
   return absl::OkStatus();
-
 }
 
 }  // namespace pins_test

tests/qos/qos_test_util.h

@@ -206,8 +206,9 @@ absl::StatusOr<std::vector<std::string>>
 GetStrictlyPrioritizedQueuesInDescendingOrderOfPriority(
     absl::string_view scheduler_policy_name, gnmi::gNMI::StubInterface &gnmi);
 // Get queues for an egress port.
-absl::StatusOr<std::vector<std::string>> GetQueuesByEgressPort(
-    absl::string_view egress_port, gnmi::gNMI::StubInterface &gnmi);
+absl::StatusOr<std::vector<std::string>>
+GetQueuesByEgressPort(absl::string_view egress_port,
+                      gnmi::gNMI::StubInterface &gnmi);
 
 // Reads the name of the buffer allocation profile applied
 // to the given egress port from the appropriate gNMI state path.

tests/thinkit_gnmi_interface_util.cc

@@ -17,6 +17,7 @@
 #include "absl/strings/match.h"
 #include "absl/strings/numbers.h"
 #include "absl/strings/str_cat.h"
+#include "absl/strings/str_format.h"
 #include "absl/strings/str_join.h"
 #include "absl/strings/str_replace.h"
 #include "absl/strings/str_split.h"
@@ -354,10 +355,10 @@ absl::StatusOr<SlotPortLane> GetSlotPortLaneForPort(
   }
   auto slot_port_lane_str = port.substr(kEthernetLen);
   std::vector<std::string> values = absl::StrSplit(slot_port_lane_str, '/');
-  if (values.size() != 3) {
-    return absl::InvalidArgumentError(
-        absl::StrCat("Requested port (", port,
-                     ") does not have a valid format (EthernetX/Y/Z)"));
+  if (values.size() < kSlotPortLaneMinValues) {
+    return absl::InvalidArgumentError(absl::StrCat(
+        "Requested port (", port,
+        ") does not have a valid format (EthernetX/Y/Z or EthernetX/Y)"));
   }
   SlotPortLane slot_port_lane;
   if (!absl::SimpleAtoi(values[kSlotIndex], &slot_port_lane.slot)) {
@@ -370,10 +371,15 @@ absl::StatusOr<SlotPortLane> GetSlotPortLaneForPort(
            << "Failed to convert string (" << values[kPortIndex]
            << ") to integer";
   }
-  if (!absl::SimpleAtoi(values[kLaneIndex], &slot_port_lane.lane)) {
-    return gutil::InternalErrorBuilder().LogError()
-           << "Failed to convert string (" << values[kLaneIndex]
-           << ") to integer";
+  // Unchannelizable ports will not contain a lane number.
+  // Use default lane number of 1.
+  slot_port_lane.lane = 1;
+  if (values.size() == kSlotPortLaneMaxValues) {
+    if (!absl::SimpleAtoi(values[kLaneIndex], &slot_port_lane.lane)) {
+      return gutil::InternalErrorBuilder().LogError()
+             << "Failed to convert string (" << values[kLaneIndex]
+             << ") to integer";
+    }
   }
   return slot_port_lane;
 }
@@ -507,7 +513,7 @@ absl::StatusOr<bool> IsCopperPort(gnmi::gNMI::StubInterface* sut_gnmi_stub,
                                   absl::string_view port) {
   // Get transceiver name for the port.
   auto state_path =
-      absl::StrCat("interfaces/interface[name=", port, "]/state/transceiver");
+      absl::StrFormat("interfaces/interface[name=%s]/state/transceiver", port);
   auto resp_parse_str = "openconfig-platform-transceiver:transceiver";
   ASSIGN_OR_RETURN(
       auto xcvrd_name,
@@ -517,27 +523,21 @@ absl::StatusOr<bool> IsCopperPort(gnmi::gNMI::StubInterface* sut_gnmi_stub,
         << port);
   StripSymbolFromString(xcvrd_name, '\"');
 
-  // TODO: Replace with PMD type when supported.
-  // Get cable length for the port transceiver.
-  state_path =
-      absl::StrCat("components/component[name=", xcvrd_name,
-                   "]/transceiver/state/openconfig-platform-ext:cable-length");
-  resp_parse_str = "openconfig-platform-ext:cable-length";
+  state_path = absl::StrFormat(
+      "components/component[name=%s]/transceiver/state/ethernet-pmd",
+      xcvrd_name);
+  resp_parse_str = "openconfig-platform-transceiver:ethernet-pmd";
   ASSIGN_OR_RETURN(
-      auto cable_length_str,
+      auto ethernet_pmd,
       GetGnmiStatePathInfo(sut_gnmi_stub, state_path, resp_parse_str),
-      _ << "Failed to get GNMI state path value for cable-length for "
+      _ << "Failed to get GNMI state path value for ethernet-pmd for "
            "port "
         << port);
-  StripSymbolFromString(cable_length_str, '\"');
+  StripSymbolFromString(ethernet_pmd, '\"');
 
-  // Only cable lengths of copper ports are a positive value.
-  float cable_length;
-  if (!absl::SimpleAtof(cable_length_str, &cable_length)) {
-    return gutil::InternalErrorBuilder().LogError()
-           << "Failed to convert string (" << cable_length_str << ") to float";
-  }
-  return (cable_length > 0);
+  // PMD state path value for copper ports ends in CR2/CR4/CR8.
+  auto pos = ethernet_pmd.find_last_of('_');
+  return (ethernet_pmd.substr(pos + 1, 2) == "CR");
 }
 
 absl::StatusOr<std::string> GenerateInterfaceBreakoutConfig(

tests/thinkit_gnmi_interface_util.h

@@ -38,6 +38,8 @@ inline constexpr char kEthernet[] = "Ethernet";
 const int kSlotIndex = 0;
 const int kPortIndex = 1;
 const int kLaneIndex = 2;
+const int kSlotPortLaneMinValues = 2;
+const int kSlotPortLaneMaxValues = 3;
 
 // PortBreakoutInfo contains physical channels and operational status for an
 // interface.