[Thinkit] Punt only ECN marked packets, Replace TestGnmiCheckSpecific…

…InterfaceStateOperation with CheckInterfaceOperStateOverGnmi, Add tolerance to flaky tests, Port status check was called twice in `TestMirroredGnmiCheckAllInterfaceState` test, Enable verification of bert_operation_id in StartBert response, Configure switches sequentially with ConfigureSwitchPairAndReturnP4RuntimeSessionPair & gNMI port mtu tests. (#945) * [Thinkit] Fix the gnoi factory reset test error message matching error, Allow some tolerance for protocol packets, Change neighbor entries to ipv6, Skip ACL modify during bcm-sai 7.1 upgrade, Rate limit punt traffic to control switch & Default to qos_queue 0x7 instead of 0x1. * [Thinkit] Punt only ECN marked packets, Replace TestGnmiCheckSpecificInterfaceStateOperation with CheckInterfaceOperStateOverGnmi, Add tolerance to flaky tests, Port status check was called twice in `TestMirroredGnmiCheckAllInterfaceState` test, Enable verification of bert_operation_id in StartBert response, Configure switches sequentially with ConfigureSwitchPairAndReturnP4RuntimeSessionPair & gNMI port mtu tests. --------- Co-authored-by: kishanps <[email protected]>
sonic-net · Jan 10, 2025 · 2a5a114 · 2a5a114
1 parent dac1d13
commit 2a5a114
Show file tree

Hide file tree

Showing 10 changed files with 182 additions and 114 deletions.
diff --git a/tests/forwarding/l3_admit_test.cc b/tests/forwarding/l3_admit_test.cc
@@ -526,7 +526,15 @@ TEST_P(L3AdmitTestFixture, DISABLED_L3AdmitCanUseInPortToRestrictMacAddresses) {
   }
   LOG(INFO) << "Done collecting packets.";
 
-  EXPECT_EQ(good_packet_count, kNumberOfTestPacket);
+  if (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/watch_port_test.cc b/tests/forwarding/watch_port_test.cc
@@ -100,6 +100,9 @@ constexpr absl::string_view kGroupId = "group-1";
 // Vrf used in the test.
 constexpr absl::string_view kVrfId = "vrf-1";
 
+// Time to wait for membership updates after link event triggers.
+constexpr absl::Duration kDurationToWaitForMembershipUpdates = absl::Seconds(2);
+
 // Time to wait after which received packets are processed.
 constexpr absl::Duration kDurationToWaitForPackets = absl::Seconds(5);
 
@@ -737,6 +740,9 @@ TEST_P(WatchPortTestFixture, VerifyBasicWatchPortAction) {
     ASSERT_OK(VerifyInterfaceState(*sut_gnmi_stub_, port_name,
                                    kOperStatusPathMatch, operation));
 
+    // Wait for the membership changes to be processed.
+    absl::SleepFor(kDurationToWaitForMembershipUpdates);
+
     // Clear the counters before the test.
     test_data_.ClearReceivedPackets();
 
@@ -863,6 +869,9 @@ TEST_P(WatchPortTestFixture, VerifyWatchPortActionInCriticalState) {
   ASSERT_OK(VerifyInterfaceState(*sut_gnmi_stub_, port_name,
                                  kOperStatusPathMatch, InterfaceState::kDown));
 
+  // Wait for the membership changes to be processed.
+  absl::SleepFor(kDurationToWaitForMembershipUpdates);
+
   // Clear the counters before the test.
   test_data_.ClearReceivedPackets();
 
@@ -979,6 +988,9 @@ TEST_P(WatchPortTestFixture, VerifyWatchPortActionForSingleMember) {
     ASSERT_OK(VerifyInterfaceState(*sut_gnmi_stub_, port_name,
                                    kOperStatusPathMatch, operation));
 
+    // Wait for the membership changes to be processed.
+    absl::SleepFor(kDurationToWaitForMembershipUpdates);
+
     // Clear the counters before the test.
     test_data_.ClearReceivedPackets();
 
@@ -1108,6 +1120,9 @@ TEST_P(WatchPortTestFixture, VerifyWatchPortActionForMemberModify) {
   ASSERT_OK(VerifyInterfaceState(*sut_gnmi_stub_, selected_port_name,
                                  kOperStatusPathMatch, InterfaceState::kUp));
 
+  // Wait for the membership changes to be processed.
+  absl::SleepFor(kDurationToWaitForMembershipUpdates);
+
   // Send 5000 packets and check for packet distribution.
   ASSERT_OK(SendNPacketsToSut(kNumTestPackets, test_config, members,
                               controller_port_ids, ir_p4info,
@@ -1228,6 +1243,9 @@ TEST_P(WatchPortTestFixture, VerifyWatchPortActionForDownPortMemberInsert) {
     ASSERT_OK(VerifyInterfaceState(*sut_gnmi_stub_, selected_port_name,
                                    kOperStatusPathMatch, operation));
 
+    // Wait for the membership changes to be processed.
+    absl::SleepFor(kDurationToWaitForMembershipUpdates);
+
     // Clear the counters before the test.
     test_data_.ClearReceivedPackets();
 

diff --git a/tests/gnoi/bert_tests.cc b/tests/gnoi/bert_tests.cc
@@ -145,9 +145,7 @@ void SendStartBertRequestSuccessfullyOnControlSwitch(
   ASSERT_THAT(response.per_port_responses(),
               SizeIs(request.per_port_requests_size()));
 
-  // TODO: Sandcastle does not populate bert_request_id in
-  // response, remove check for now.
-  // EXPECT_EQ(response.bert_operation_id(), request.bert_operation_id());
+  EXPECT_EQ(response.bert_operation_id(), request.bert_operation_id());
 
   for (int idx = 0; idx < response.per_port_responses_size(); ++idx) {
     auto per_port_response = response.per_port_responses(idx);

diff --git a/tests/lib/switch_test_setup_helpers.cc b/tests/lib/switch_test_setup_helpers.cc
@@ -2,7 +2,6 @@
 
 #include <deque>
 #include <functional>
-#include <future>  // NOLINT: third_party library.
 #include <memory>
 #include <optional>
 #include <string>
@@ -178,29 +177,18 @@ ConfigureSwitchPairAndReturnP4RuntimeSessionPair(
     std::optional<std::string> gnmi_config,
     std::optional<p4::config::v1::P4Info> p4info,
     const pdpi::P4RuntimeSessionOptionalArgs& metadata) {
-  // We configure both switches in parallel, since it may require rebooting the
-  // switch which is costly.
-  using T = absl::StatusOr<std::unique_ptr<pdpi::P4RuntimeSession>>;
-  T session1, session2;
-  {
-    std::future<T> future1 = std::async(std::launch::async, [&] {
-      return ConfigureSwitchAndReturnP4RuntimeSession(
-          thinkit_switch1, gnmi_config, p4info, metadata);
-    });
-    std::future<T> future2 = std::async(std::launch::async, [&] {
-      return ConfigureSwitchAndReturnP4RuntimeSession(
-          thinkit_switch2, gnmi_config, p4info, metadata);
-    });
-    session1 = future1.get();
-    session2 = future2.get();
-  }
-  RETURN_IF_ERROR(session1.status()).SetPrepend()
-      << "failed to configure switch '" << thinkit_switch1.ChassisName()
-      << "': ";
-  RETURN_IF_ERROR(session2.status()).SetPrepend()
-      << "failed to configure switch '" << thinkit_switch2.ChassisName()
-      << "': ";
-  return std::make_pair(std::move(*session1), std::move(*session2));
+  // TODO: Ideally, we would configure these in parallel instead.
+  ASSIGN_OR_RETURN(std::unique_ptr<pdpi::P4RuntimeSession> session1,
+                   ConfigureSwitchAndReturnP4RuntimeSession(
+                       thinkit_switch1, gnmi_config, p4info, metadata),
+                   _.SetPrepend() << "failed to configure switch '"
+                                  << thinkit_switch1.ChassisName() << "': ");
+  ASSIGN_OR_RETURN(std::unique_ptr<pdpi::P4RuntimeSession> session2,
+                   ConfigureSwitchAndReturnP4RuntimeSession(
+                       thinkit_switch2, gnmi_config, p4info, metadata),
+                   _.SetPrepend() << "failed to configure switch '"
+                                  << thinkit_switch2.ChassisName() << "': ");
+  return std::make_pair(std::move(session1), std::move(session2));
 }
 
 absl::Status MirrorSutP4rtPortIdConfigToControlSwitch(

diff --git a/tests/mtu_tests/BUILD.bazel b/tests/mtu_tests/BUILD.bazel
@@ -47,6 +47,7 @@ cc_library(
         "//p4_pdpi/packetlib",
         "//p4_pdpi/packetlib:packetlib_cc_proto",
         "//sai_p4/instantiations/google:sai_pd_cc_proto",
+        "//tests/forwarding:util",
         "//thinkit:control_device",
         "//thinkit:generic_testbed",
         "//thinkit:generic_testbed_fixture",

diff --git a/tests/mtu_tests/mtu_tests.cc b/tests/mtu_tests/mtu_tests.cc
@@ -1,6 +1,7 @@
 #include "tests/mtu_tests/mtu_tests.h"
 
 #include <memory>
+#include <optional>
 #include <string>
 #include <type_traits>
 #include <vector>
@@ -15,8 +16,6 @@
 #include "absl/time/clock.h"
 #include "absl/time/time.h"
 #include "glog/logging.h"
-#include "gmock/gmock.h"
-#include "gtest/gtest.h"
 #include "gutil/collections.h"
 #include "gutil/proto.h"
 #include "gutil/status.h"
@@ -38,10 +37,13 @@
 #include "p4_pdpi/pd.h"
 #include "sai_p4/instantiations/google/instantiations.h"
 #include "sai_p4/instantiations/google/sai_pd.pb.h"
+#include "tests/forwarding/util.h"
 #include "thinkit/control_device.h"
 #include "thinkit/generic_testbed.h"
 #include "thinkit/proto/generic_testbed.pb.h"
 #include "thinkit/switch.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
 
 namespace pins_test {
 
@@ -133,14 +135,15 @@ void MtuRoutingTestFixture::SetUp() {
 
 absl::StatusOr<NumPkts> MtuRoutingTestFixture::SendTraffic(
     const int num_pkts, absl::string_view egress_port,
-    absl::string_view ingress_port, absl::string_view test_packet_str) {
+    absl::string_view ingress_port, absl::string_view test_packet_str,
+    std::optional<absl::Duration> packet_delay) {
   auto test_packet = gutil::ParseProtoOrDie<packetlib::Packet>(test_packet_str);
   ASSIGN_OR_RETURN(std::string test_packet_data,
                    packetlib::SerializePacket(test_packet));
 
   absl::Mutex mutex;
   std::vector<std::string> received_packets;
-  int i;
+  int i = 0;
   {
     ASSIGN_OR_RETURN(auto finalizer,
                      testbed_->ControlDevice().CollectPackets());
@@ -149,11 +152,12 @@ absl::StatusOr<NumPkts> MtuRoutingTestFixture::SendTraffic(
               << egress_port;
     LOG(INFO) << "Test packet data: " << test_packet_str;
 
-    for (i = 0; i < num_pkts; i++) {
-      RETURN_IF_ERROR(
-          testbed_->ControlDevice().SendPacket(egress_port, test_packet_data))
+    while (i < num_pkts || unlimited_pkts_) {
+      RETURN_IF_ERROR(testbed_->ControlDevice().SendPacket(
+          egress_port, test_packet_data, packet_delay))
           << "failed to inject the packet.";
       LOG(INFO) << "SendPacket completed";
+      i++;
     }
     RETURN_IF_ERROR(finalizer->HandlePacketsFor(
         absl::Seconds(30),
@@ -169,12 +173,41 @@ absl::StatusOr<NumPkts> MtuRoutingTestFixture::SendTraffic(
   return NumPkts{i, static_cast<int>(received_packets.size())};
 }
 
+void MtuRoutingTestFixture::StartUnlimitedTraffic(
+    const std::string &egress_port, const std::string &ingress_port,
+    const std::string &test_packet,
+    std::optional<absl::Duration> packet_delay) {
+  // Send unlimited packets packets to SUT.
+  // Set flag to allow sending unlimited packets to true.
+  unlimited_pkts_ = true;
+  LOG(INFO) << "Starting unlimited traffic.";
+  absl::Duration packet_delay_val;
+  if (packet_delay.has_value()) {
+    packet_delay_val = packet_delay.value();
+  }
+  traffic_thread_ = std::thread{[test_packet, this, &egress_port, &ingress_port,
+                                 packet_delay_val] {
+    ASSERT_OK_AND_ASSIGN(num_pkts_, SendTraffic(0, egress_port, ingress_port,
+                                                test_packet, packet_delay_val));
+  }};
+}
+
+absl::StatusOr<NumPkts> MtuRoutingTestFixture::StopUnlimitedTraffic() {
+  unlimited_pkts_ = false;
+  traffic_thread_.join();
+  LOG(INFO) << "Stopped unlimited traffic.";
+  LOG(INFO) << "Sent " << num_pkts_.sent << " packets, received "
+            << num_pkts_.received << " packets.";
+  return num_pkts_;
+}
+
 namespace {
 
 using ::testing::HasSubstr;
 
 constexpr absl::string_view kMtuRespParseStr = "openconfig-interfaces:mtu";
 constexpr int kDefaultMtu = 9194;
+constexpr int kMtu4500 = 4500;
 
 // Map of mtu to payload length for packets that are expected to be
 // successfully egressed out of port under test.
@@ -268,5 +301,62 @@ TEST_P(MtuRoutingTestFixture, MtuTest) {
   SetPortMtu(stub_.get(), destination_link_.sut_interface,
              std::to_string(orig_mtu));
 }
+
+TEST_P(MtuRoutingTestFixture, VerifyTrafficWithMtuChangeTest) {
+  testbed_->Environment().SetTestCaseID("ae14bc06-be75-4e9b-8ff8-7defac538982");
+  // Get original mtu on port under test on SUT.
+  std::string if_state_path = absl::StrCat(
+      "interfaces/interface[name=", destination_link_.sut_interface,
+      "]/state/mtu");
+  ASSERT_OK_AND_ASSIGN(
+      std::string state_path_response,
+      GetGnmiStatePathInfo(stub_.get(), if_state_path, kMtuRespParseStr));
+  int orig_mtu;
+  ASSERT_TRUE(absl::SimpleAtoi(state_path_response, &orig_mtu));
+
+  // Set mtu to 4500 on port under test.
+  SetPortMtu(stub_.get(), destination_link_.sut_interface,
+             std::to_string(kMtu4500));
+
+  // Set up a route between the source and destination interfaces.
+  ASSERT_OK_AND_ASSIGN(
+      std::unique_ptr<pdpi::P4RuntimeSession> p4_session,
+      pdpi::P4RuntimeSession::CreateWithP4InfoAndClearTables(
+          testbed_->Sut(), MtuRoutingTestFixture::GetParam().p4_info));
+  P4rtProgrammingContext p4rt_context(p4_session.get(),
+                                      pdpi::SetMetadataAndSendPiWriteRequest);
+  ASSERT_OK(SetupRoute(&p4rt_context));
+
+  // Send 4k size packet from peer switch to SUT to be routed out of port
+  // under test and set mtu to 9194 for port under test while traffic is
+  // being routed from it.
+  auto test_packet =
+      GenerateTestPacket(basic_traffic::PortIdToIP(sut_destination_port_id_),
+                         /*payload_len*/ 4000);
+  StartUnlimitedTraffic(
+      /*egress_port*/ source_link_.peer_interface,
+      /*ingress_port*/ destination_link_.peer_interface,
+      /*test_packet*/ test_packet,
+      /*packet_delay*/ absl::Duration(absl::Milliseconds(100)));
+
+  // Allow time for traffic to start in order to verify mtu change while
+  // traffic is running.
+  absl::SleepFor(absl::Seconds(5));
+  SetPortMtu(stub_.get(), destination_link_.sut_interface,
+             std::to_string(kDefaultMtu));
+
+  // Allow time to ensure there is no packet loss due to mtu change before
+  // stopping traffic.
+  absl::SleepFor(absl::Seconds(5));
+  ASSERT_OK_AND_ASSIGN(auto pkts, StopUnlimitedTraffic());
+
+  // Verify that all the packets are routed out of port under test indicating
+  // mtu change does not impact traffic.
+  EXPECT_EQ(pkts.sent, pkts.received);
+
+  // Restore original mtu values on port under test on SUT.
+  SetPortMtu(stub_.get(), destination_link_.sut_interface,
+             std::to_string(orig_mtu));
+}
 }  // namespace
 }  // namespace pins_test
diff --git a/tests/mtu_tests/mtu_tests.h b/tests/mtu_tests/mtu_tests.h
@@ -1,12 +1,16 @@
 #ifndef PINS_TESTS_MTU_TESTS_MTU_TESTS_H_
 #define PINS_TESTS_MTU_TESTS_MTU_TESTS_H_
 
+#include <optional>
+#include <thread> // NOLINT: Need threads (instead of fiber) for upstream code.
+
 #include "absl/base/config.h"
 #include "absl/strings/string_view.h"
 #include "lib/p4rt/p4rt_programming_context.h"
 #include "lib/utils/generic_testbed_utils.h"
 #include "p4_pdpi/packetlib/packetlib.pb.h"
 #include "thinkit/generic_testbed_fixture.h"
+
 namespace pins_test {
 
 struct NumPkts {
@@ -24,20 +28,36 @@ class MtuRoutingTestFixture : public thinkit::GenericTestbedFixture<> {
   std::string GenerateTestPacket(absl::string_view destination_ip,
                                  int payload_len);
 
-  // Sends num_pkts/unlimited packets from egress_port. Collects packets on
+  // Sends num_pkts packets from egress_port. Collects packets on
   // ingress_port and returns number of packets sent and received.
-  absl::StatusOr<NumPkts> SendTraffic(int num_pkts,
-                                      absl::string_view egress_port,
-                                      absl::string_view ingress_port,
-                                      absl::string_view test_packet_str);
+  absl::StatusOr<NumPkts>
+  SendTraffic(int num_pkts, absl::string_view egress_port,
+              absl::string_view ingress_port, absl::string_view test_packet_str,
+              std::optional<absl::Duration> packet_delay = std::nullopt);
 
-  // Set up route from source port to destination port on SUT.
+  // Sets up route from source port to destination port on SUT.
   absl::Status SetupRoute(P4rtProgrammingContext *p4rt_context);
 
+  // Sends unlimited packets from egress_port. Collects packets on ingress_port
+  // and returns number of packets sent and received.
+  void StartUnlimitedTraffic(
+      const std::string &egress_port, const std::string &ingress_port,
+      const std::string &test_packet,
+      std::optional<absl::Duration> packet_delay = std::nullopt);
+
+  // Stops packet sending thread.
+  absl::StatusOr<NumPkts> StopUnlimitedTraffic();
+
   InterfaceLink source_link_, destination_link_;
   int sut_source_port_id_, sut_destination_port_id_;
   std::unique_ptr<thinkit::GenericTestbed> testbed_;
   std::unique_ptr<gnmi::gNMI::StubInterface> stub_;
+
+private:
+  std::thread traffic_thread_;
+  NumPkts num_pkts_{0, 0};
+  // Flag used to allow sending packets as long as the value is true.
+  bool unlimited_pkts_ = false;
 };
 
 } // namespace pins_test