From dac1d1391e275ccae8947222c3b324832b1e2e7d Mon Sep 17 00:00:00 2001 From: divyagayathri-hcl <159437886+divyagayathri-hcl@users.noreply.github.com> Date: Fri, 10 Jan 2025 01:59:56 +0000 Subject: [PATCH] [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. (#944) Co-authored-by: kishanps --- tests/forwarding/l3_admit_test.cc | 108 +++++++++--------- tests/forwarding/smoke_test.cc | 10 +- tests/forwarding/test_data.cc | 2 +- tests/forwarding/watch_port_test.cc | 5 +- tests/gnoi/factory_reset_test.cc | 3 +- .../p4rt_fixed_table_programming_helper.cc | 32 +++--- ...4rt_fixed_table_programming_helper_test.cc | 19 ++- tests/qos/cpu_qos_test.cc | 9 +- tests/qos/frontpanel_qos_test.cc | 11 +- 9 files changed, 113 insertions(+), 86 deletions(-) diff --git a/tests/forwarding/l3_admit_test.cc b/tests/forwarding/l3_admit_test.cc index 56f3f070..9b50e4cd 100644 --- a/tests/forwarding/l3_admit_test.cc +++ b/tests/forwarding/l3_admit_test.cc @@ -258,7 +258,9 @@ absl::Status SendUdpPacket(pdpi::P4RuntimeSession& session, ASSIGN_OR_RETURN(std::string packet, UdpPacket(dst_mac, dst_ip, absl::Substitute("[Packet:$0] $1", i, payload))); - RETURN_IF_ERROR(InjectEgressPacket(port_id, packet, ir_p4info, &session)); + // Rate limit to 500pps to avoid punt packet drops on the control switch. + RETURN_IF_ERROR(InjectEgressPacket(port_id, packet, ir_p4info, &session, + /*packet_delay=*/absl::Milliseconds(2))); } return absl::OkStatus(); } @@ -282,7 +284,7 @@ TEST_P(L3AdmitTestFixture, .switch_ip = std::make_pair("10.0.0.1", 32), .peer_port = "1", .peer_mac = "00:00:00:00:00:02", - .peer_ip = "10.0.0.2", + .peer_ip = "fe80::2", .router_interface_id = "rif-1", .nexthop_id = "nexthop-1", }; @@ -371,7 +373,7 @@ TEST_P(L3AdmitTestFixture, L3AdmitCanUseMaskToAllowMultipleMacAddresses) { .switch_ip = std::make_pair("10.0.0.1", 32), .peer_port = "1", .peer_mac = "00:00:00:00:00:02", - .peer_ip = "10.0.0.2", + .peer_ip = "fe80::2", .router_interface_id = "rif-1", .nexthop_id = "nexthop-1", }; @@ -450,7 +452,7 @@ TEST_P(L3AdmitTestFixture, DISABLED_L3AdmitCanUseInPortToRestrictMacAddresses) { .switch_ip = std::make_pair("10.0.0.1", 32), .peer_port = "1", .peer_mac = "00:00:00:00:00:02", - .peer_ip = "10.0.0.2", + .peer_ip = "fe80::2", .router_interface_id = "rif-1", .nexthop_id = "nexthop-1", }; @@ -540,56 +542,56 @@ ASSERT_OK( // Add an L3 route to enable forwarding, but do not add an explicit L3Admit // rule. - L3Route l3_route{ - .vrf_id = "vrf-1", - .switch_mac = "00:00:00:00:00:01", - .switch_ip = std::make_pair("10.0.0.1", 32), - .peer_port = "1", - .peer_mac = "00:00:00:00:00:02", - .peer_ip = "10.0.0.2", - .router_interface_id = "rif-1", - .nexthop_id = "nexthop-1", - }; - ASSERT_OK(AddAndSetDefaultVrf(GetSutP4RuntimeSession(), - sai::GetIrP4Info(sai::Instantiation::kMiddleblock), - l3_route.vrf_id)); - ASSERT_OK(AddL3Route(GetSutP4RuntimeSession(), - sai::GetIrP4Info(sai::Instantiation::kMiddleblock), - l3_route)); - - // Send 1 set of packets to the switch using the switch's MAC address from the - // L3 route. - const int kNumberOfTestPacket = 100; - const std::string kGoodPayload = - "Testing L3 forwarding. This packet should arrive to packet in."; - ASSERT_OK(SendUdpPacket(GetControlP4RuntimeSession(), - sai::GetIrP4Info(sai::Instantiation::kMiddleblock), - /*port_id=*/"1", kNumberOfTestPacket, - /*dst_mac=*/"00:00:00:00:00:01", - /*dst_ip=*/"10.0.0.1", kGoodPayload)); - - absl::Time timeout = absl::Now() + absl::Minutes(1); - int good_packet_count = 0; - while (good_packet_count < kNumberOfTestPacket) { - if (packetio_control->HasPacketInMessage()) { - ASSERT_OK_AND_ASSIGN(p4::v1::StreamMessageResponse response, - packetio_control->GetNextPacketInMessage()); - - // Verify this is the packet we expect. - packetlib::Packet packet_in = - packetlib::ParsePacket(response.packet().payload()); - if (response.update_case() == p4::v1::StreamMessageResponse::kPacket && - absl::StrContains(packet_in.payload(), kGoodPayload)) { - ++good_packet_count; - } else { - LOG(WARNING) << "Unexpected response: " << response.DebugString(); - } +L3Route l3_route{ + .vrf_id = "vrf-1", + .switch_mac = "00:00:00:00:00:01", + .switch_ip = std::make_pair("10.0.0.1", 32), + .peer_port = "1", + .peer_mac = "00:00:00:00:00:02", + .peer_ip = "fe80::2", + .router_interface_id = "rif-1", + .nexthop_id = "nexthop-1", +}; +ASSERT_OK(AddAndSetDefaultVrf( + GetSutP4RuntimeSession(), + sai::GetIrP4Info(sai::Instantiation::kMiddleblock), l3_route.vrf_id)); +ASSERT_OK(AddL3Route(GetSutP4RuntimeSession(), + sai::GetIrP4Info(sai::Instantiation::kMiddleblock), + l3_route)); + +// Send 1 set of packets to the switch using the switch's MAC address from the +// L3 route. +const int kNumberOfTestPacket = 100; +const std::string kGoodPayload = + "Testing L3 forwarding. This packet should arrive to packet in."; +ASSERT_OK(SendUdpPacket(GetControlP4RuntimeSession(), + sai::GetIrP4Info(sai::Instantiation::kMiddleblock), + /*port_id=*/"1", kNumberOfTestPacket, + /*dst_mac=*/"00:00:00:00:00:01", + /*dst_ip=*/"10.0.0.1", kGoodPayload)); + +absl::Time timeout = absl::Now() + absl::Minutes(1); +int good_packet_count = 0; +while (good_packet_count < kNumberOfTestPacket) { + if (packetio_control->HasPacketInMessage()) { + ASSERT_OK_AND_ASSIGN(p4::v1::StreamMessageResponse response, + packetio_control->GetNextPacketInMessage()); + + // Verify this is the packet we expect. + packetlib::Packet packet_in = + packetlib::ParsePacket(response.packet().payload()); + if (response.update_case() == p4::v1::StreamMessageResponse::kPacket && + absl::StrContains(packet_in.payload(), kGoodPayload)) { + ++good_packet_count; + } else { + LOG(WARNING) << "Unexpected response: " << response.DebugString(); } + } - if (absl::Now() > timeout) { - LOG(ERROR) << "Reached timeout waiting on packets to arrive."; - break; - } + if (absl::Now() > timeout) { + LOG(ERROR) << "Reached timeout waiting on packets to arrive."; + break; + } } LOG(INFO) << "Done collecting packets."; @@ -646,7 +648,7 @@ TEST_P(L3AdmitTestFixture, L3PacketsCanBeClassifiedByDestinationMac) { .switch_ip = std::make_pair("10.0.0.1", 32), .peer_port = "1", .peer_mac = "00:00:00:00:00:02", - .peer_ip = "10.0.0.2", + .peer_ip = "fe80::2", .router_interface_id = "rif-1", .nexthop_id = "nexthop-1", }; diff --git a/tests/forwarding/smoke_test.cc b/tests/forwarding/smoke_test.cc index 6c4ff8ee..ce3575b5 100644 --- a/tests/forwarding/smoke_test.cc +++ b/tests/forwarding/smoke_test.cc @@ -117,12 +117,10 @@ TEST_P(SmokeTestFixture, AclTableAddDeleteOkButModifyFails) { } } while (!pi_read_response.entities(0).table_entry().has_counter_data()); - // Many ACL table attribues are NOT modifiable currently due to missing SAI - // implementation. Because there are no production use-cases, these are - // de-prioritized. - ASSERT_THAT(pdpi::SetMetadataAndSendPiWriteRequest(&GetSutP4RuntimeSession(), - pi_modify), - StatusIs(absl::StatusCode::kUnknown)); + // 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)); // Delete works. ASSERT_OK(pdpi::SetMetadataAndSendPiWriteRequest(&GetSutP4RuntimeSession(), diff --git a/tests/forwarding/test_data.cc b/tests/forwarding/test_data.cc index fc50004a..57bb4ee1 100644 --- a/tests/forwarding/test_data.cc +++ b/tests/forwarding/test_data.cc @@ -40,7 +40,7 @@ std::vector CreateUpTo255GenericTableEntries( ip_protocol { value: "0x11" mask: "0xff" } l4_dst_port { value: "0x0043" mask: "0xffff" } } - action { trap { qos_queue: "0x1" } } + action { trap { qos_queue: "0x7" } } priority: 2040 } )pb"); diff --git a/tests/forwarding/watch_port_test.cc b/tests/forwarding/watch_port_test.cc index e5e6176e..336e290d 100644 --- a/tests/forwarding/watch_port_test.cc +++ b/tests/forwarding/watch_port_test.cc @@ -310,8 +310,9 @@ absl::Status SendNPacketsToSut(int num_packets, pins::GenerateIthPacket(test_config, i)); ASSIGN_OR_RETURN(std::string raw_packet, SerializePacket(packet)); ASSIGN_OR_RETURN(std::string port_string, pdpi::IntToDecimalString(port)); - RETURN_IF_ERROR( - pins::InjectEgressPacket(port_string, raw_packet, ir_p4info, &p4_session)); + RETURN_IF_ERROR(InjectEgressPacket(port_string, raw_packet, ir_p4info, + &p4_session, + /*packet_delay=*/std::nullopt)); dvaas::Packet p; p.set_port(port_string); diff --git a/tests/gnoi/factory_reset_test.cc b/tests/gnoi/factory_reset_test.cc index 34da0a61..a0b5b0e9 100644 --- a/tests/gnoi/factory_reset_test.cc +++ b/tests/gnoi/factory_reset_test.cc @@ -54,7 +54,8 @@ void IssueGnoiFactoryResetAndValidateStatus( EXPECT_OK(status); } else { EXPECT_EQ(status.error_code(), expected_status.error_code()); - EXPECT_EQ(status.error_message(), expected_status.error_message()); + EXPECT_THAT(status.error_message(), + testing::HasSubstr(expected_status.error_message())); } } diff --git a/tests/lib/p4rt_fixed_table_programming_helper.cc b/tests/lib/p4rt_fixed_table_programming_helper.cc index dd841bd1..211f2d8a 100644 --- a/tests/lib/p4rt_fixed_table_programming_helper.cc +++ b/tests/lib/p4rt_fixed_table_programming_helper.cc @@ -142,19 +142,19 @@ absl::StatusOr NeighborTableUpdate( type: $0 entity { table_entry { - table_name: "router_interface_table" + table_name: "neighbor_table" matches { name: "router_interface_id" exact { str: "$1" } } + matches { + name: "neighbor_id" + exact { ipv6: "$2" } + } action { - name: "set_port_and_src_mac" - params { - name: "port" - value { str: "$2" } - } + name: "set_dst_mac" params { - name: "src_mac" + name: "dst_mac" value { mac: "$3" } } } @@ -177,20 +177,20 @@ absl::StatusOr NexthopTableUpdate( type: $0 entity { table_entry { - table_name: "neighbor_table" + table_name: "nexthop_table" matches { - name: "router_interface_id" + name: "nexthop_id" exact { str: "$1" } } - matches { - name: "neighbor_id" - exact { ipv6: "$2" } - } action { - name: "set_dst_mac" + name: "set_ip_nexthop" params { - name: "dst_mac" - value { mac: "$3" } + name: "router_interface_id" + value { str: "$2" } + } + params { + name: "neighbor_id" + value { ipv6: "$3" } } } } diff --git a/tests/lib/p4rt_fixed_table_programming_helper_test.cc b/tests/lib/p4rt_fixed_table_programming_helper_test.cc index f11c2cc0..cfd40b63 100644 --- a/tests/lib/p4rt_fixed_table_programming_helper_test.cc +++ b/tests/lib/p4rt_fixed_table_programming_helper_test.cc @@ -22,7 +22,7 @@ #include "sai_p4/instantiations/google/instantiations.h" #include "sai_p4/instantiations/google/sai_p4info.h" -namespace gpins { +namespace pins { namespace { using ::gutil::StatusIs; @@ -112,6 +112,7 @@ TEST_P(L3RouteProgrammingTest, NeighborId) { /*dst_mac=*/"00:01:02:03:04:05")); EXPECT_THAT(pi_update, HasExactMatch("rid-1")); + EXPECT_THAT(pi_update, HasExactMatch("\001")); EXPECT_THAT(pi_update, HasActionParam("\001\002\003\004\005")); } @@ -119,12 +120,23 @@ TEST_P(L3RouteProgrammingTest, NeighborIdFailsWithInvalidMacAddress) { EXPECT_THAT( pins::NeighborTableUpdate(sai::GetIrP4Info(GetParam()), p4::v1::Update::INSERT, /*router_interface_id=*/"rid-1", - /*neighbor_id=*/"peer-1", + /*neighbor_id=*/"fe80::201:02ff:fe03:0405", /*dst_mac=*/"invalid_format"), StatusIs(absl::StatusCode::kInvalidArgument, HasSubstr("Invalid MAC address"))); } +TEST_P(L3RouteProgrammingTest, NexthopId) { + ASSERT_OK_AND_ASSIGN( + p4::v1::Update pi_update, + NexthopTableUpdate(sai::GetIrP4Info(GetParam()), p4::v1::Update::INSERT, + /*nexthop_id=*/"nexthop-2", + /*router_interface_id=*/"rid-2", + /*neighbor_id=*/"::1")); + EXPECT_THAT(pi_update, HasExactMatch("nexthop-2")); + EXPECT_THAT(pi_update, HasActionParam("rid-2")); + EXPECT_THAT(pi_update, HasActionParam("\001")); +} TEST_P(L3RouteProgrammingTest, VrfTableAddFailsWithEmptyId) { EXPECT_THAT( @@ -132,6 +144,7 @@ TEST_P(L3RouteProgrammingTest, VrfTableAddFailsWithEmptyId) { /*vrf_id=*/""), StatusIs(absl::StatusCode::kInvalidArgument)); } + TEST_P(L3RouteProgrammingTest, Ipv4TableDoesNotRequireAnAction) { // The helper class will assume a default (e.g. drop). ASSERT_OK_AND_ASSIGN( @@ -220,4 +233,4 @@ INSTANTIATE_TEST_SUITE_P( }); } // namespace -} // namespace gpins +} // namespace pins diff --git a/tests/qos/cpu_qos_test.cc b/tests/qos/cpu_qos_test.cc index 1356df5b..ac202782 100644 --- a/tests/qos/cpu_qos_test.cc +++ b/tests/qos/cpu_qos_test.cc @@ -442,7 +442,8 @@ TEST_P(CpuQosTestWithoutIxia, PerEntryAclCounterIncrementsWhenEntryIsHit) { packetlib::SerializePacket(test_packet)); ASSERT_OK(pins::InjectEgressPacket( /*port=*/link_used_for_test_packets.control_device_port_p4rt_name, - /*packet=*/raw_packet, ir_p4info, control_p4rt_session.get())); + /*packet=*/raw_packet, ir_p4info, control_p4rt_session.get(), + /*packet_delay=*/std::nullopt)); // Check that the counters increment within kMaxQueueCounterUpdateTime. absl::Time time_packet_sent = absl::Now(); @@ -702,7 +703,8 @@ TEST_P(CpuQosTestWithoutIxia, packetlib::SerializePacket(packet)); ASSERT_OK(pins::InjectEgressPacket( /*port=*/link_used_for_test_packets.control_device_port_p4rt_name, - /*packet=*/raw_packet, ir_p4info, control_p4rt_session.get())); + /*packet=*/raw_packet, ir_p4info, control_p4rt_session.get(), + /*packet_delay=*/std::nullopt)); LOG(INFO) << "Sleeping for " << kMaxQueueCounterUpdateTime << " before checking for queue counter increment."; @@ -791,7 +793,8 @@ TEST_P(CpuQosTestWithoutIxia, TrafficToLoopbackIpGetsMappedToCorrectQueues) { for (int iter = 0; iter < kPacketCount; iter++) { ASSERT_OK(pins::InjectEgressPacket( /*port=*/link_used_for_test_packets.control_device_port_p4rt_name, - /*packet=*/raw_packet, ir_p4info, control_p4rt_session.get())); + /*packet=*/raw_packet, ir_p4info, control_p4rt_session.get(), + /*packet_delay=*/std::nullopt)); } // Read counter of the target queue again. absl::Time time_packet_sent = absl::Now(); diff --git a/tests/qos/frontpanel_qos_test.cc b/tests/qos/frontpanel_qos_test.cc index fb3774bb..bf21b3f6 100644 --- a/tests/qos/frontpanel_qos_test.cc +++ b/tests/qos/frontpanel_qos_test.cc @@ -80,7 +80,9 @@ using ::json_yang::FormatJsonBestEffort; using ::testing::Contains; using ::testing::Eq; using ::testing::Field; +using ::testing::Ge; using ::testing::IsEmpty; +using ::testing::Le; using ::testing::Not; using ::testing::Pair; using ::testing::ResultOf; @@ -472,7 +474,14 @@ TEST_P(FrontpanelQosTest, "\nAfter :", ToString(final_counters))); EXPECT_THAT(delta_counters, ResultOf(TotalPacketsForQueue, - Eq(kIxiaTrafficStats.num_tx_frames()))); + Ge(kIxiaTrafficStats.num_tx_frames()))); + // Protocol packets such as LLDP can be transmitted via queue during test. + // Add some tolerance to account for these packets. + constexpr int kMaxToleratedExtraPackets = 5; + EXPECT_THAT( + delta_counters, + ResultOf(TotalPacketsForQueue, Le(kIxiaTrafficStats.num_tx_frames() + + kMaxToleratedExtraPackets))); EXPECT_THAT(delta_counters, Field(&QueueCounters::num_packets_transmitted, Eq(kIxiaTrafficStats.num_rx_frames()))); }