From 51e390e70021458b426fc7e31e84a6f2b2c02004 Mon Sep 17 00:00:00 2001 From: Wei Dai Date: Wed, 14 Aug 2024 08:46:23 -0700 Subject: [PATCH] fix AgentDscpMarkingTest.VerifyDscpMarking on J3 Summary: As titled, fix AgentDscpMarkingTest.VerifyDscpMarking on J3 by: 1) use interface mac when creating L3 ecmp loop, otherwise, packets would be dropped during the second loop on J3. 2) change DSCP counter ACL action type to DENY. Otherwise, packets would be matched and increment counter for ttl times. 3) modified test verification part "Inject a pkt with dscp = ICP DSCP", since packets now will only hit counter ACL once. Note that, this verification is not that useful in my opinion, since the DSCP re-write ACL that the is focus of this test will never be hit anyway. Reviewed By: nivinl Differential Revision: D61146672 Privacy Context Container: L1125642 fbshipit-source-id: 966cc3cff110422e6239d42dea3b6dfca817fa28 --- .../agent_hw_tests/AgentDscpMarkingTests.cpp | 30 +++++++++++++------ fboss/agent/test/utils/DscpMarkingUtils.cpp | 8 +++-- fboss/agent/test/utils/DscpMarkingUtils.h | 5 +++- .../test/utils/TrafficPolicyTestUtils.cpp | 4 +-- .../agent/test/utils/TrafficPolicyTestUtils.h | 2 +- 5 files changed, 34 insertions(+), 15 deletions(-) diff --git a/fboss/agent/test/agent_hw_tests/AgentDscpMarkingTests.cpp b/fboss/agent/test/agent_hw_tests/AgentDscpMarkingTests.cpp index 51b2025469fee..31ee70da200e0 100644 --- a/fboss/agent/test/agent_hw_tests/AgentDscpMarkingTests.cpp +++ b/fboss/agent/test/agent_hw_tests/AgentDscpMarkingTests.cpp @@ -46,7 +46,9 @@ class AgentDscpMarkingTest : public AgentHwTest { auto l3Asics = ensemble.getL3Asics(); auto asic = utility::checkSameAndGetAsic(l3Asics); utility::addOlympicQosMaps(cfg, l3Asics); - utility::addDscpCounterAcl(asic, &cfg); + // drop packets are match to avoid packets matching multiple times in L3 + // loop case + utility::addDscpCounterAcl(asic, &cfg, cfg::AclActionType::DENY); utility::addDscpMarkingAcls(asic, &cfg, ensemble.isSai()); return cfg; } @@ -68,20 +70,26 @@ class AgentDscpMarkingTest : public AgentHwTest { * * Inject a pkt with dscp = ICP DSCP, l4SrcPort matching ACL2: * - The packet will match ACL1, thus counter incremented. - * - Packet egress via front panel port which is in loopback mode. - * - Thus, packet gets looped back. - * - Hits ACL1 again, and thus counter incremented twice. + * - Packet dropped after hitting ACL1 */ auto setup = [=, this]() { utility::EcmpSetupAnyNPorts6 ecmpHelper( - getProgrammedState(), RouterID(0), {cfg::PortType::INTERFACE_PORT}); + getProgrammedState(), + utility::getFirstInterfaceMac(getProgrammedState()), + RouterID(0), + false, + {cfg::PortType::INTERFACE_PORT}); resolveNeigborAndProgramRoutes(ecmpHelper, kEcmpWidth); }; auto verify = [=, this]() { utility::EcmpSetupAnyNPorts6 ecmpHelper( - getProgrammedState(), RouterID(0), {cfg::PortType::INTERFACE_PORT}); + getProgrammedState(), + utility::getFirstInterfaceMac(getProgrammedState()), + RouterID(0), + false, + {cfg::PortType::INTERFACE_PORT}); auto portId = ecmpHelper.ecmpPortDescriptorAt(0).phyPortID(); auto portStatsBefore = getLatestPortStats(portId); @@ -136,9 +144,9 @@ class AgentDscpMarkingTest : public AgentHwTest { // See detailed comment block at the beginning of this function EXPECT_EVENTUALLY_EQ( afterAclInOutPkts2 - afterAclInOutPkts, - (2 /* ACL hit twice */ * 2 /* l4SrcPort, l4DstPort */ * + (1 /* ACL hit once */ * 2 /* l4SrcPort, l4DstPort */ * utility::kUdpPorts().size()) + - (2 /* ACL hit twice */ * 2 /* l4SrcPort, l4DstPort */ * + (1 /* ACL hit once */ * 2 /* l4SrcPort, l4DstPort */ * utility::kTcpPorts().size())); }); } @@ -215,7 +223,11 @@ class AgentDscpMarkingTest : public AgentHwTest { // ingressed on the port, and be properly queued. if (frontPanel) { utility::EcmpSetupAnyNPorts6 ecmpHelper( - getProgrammedState(), RouterID(0), {cfg::PortType::INTERFACE_PORT}); + getProgrammedState(), + utility::getFirstInterfaceMac(getProgrammedState()), + RouterID(0), + false, + {cfg::PortType::INTERFACE_PORT}); auto outPort = ecmpHelper.ecmpPortDescriptorAt(kEcmpWidth).phyPortID(); getSw()->sendPacketOutOfPortAsync(std::move(txPacket), outPort); } else { diff --git a/fboss/agent/test/utils/DscpMarkingUtils.cpp b/fboss/agent/test/utils/DscpMarkingUtils.cpp index 36e835d03c3d4..2630e5b0b5b55 100644 --- a/fboss/agent/test/utils/DscpMarkingUtils.cpp +++ b/fboss/agent/test/utils/DscpMarkingUtils.cpp @@ -101,10 +101,14 @@ void addDscpMarkingAcls( hwAsic, config, IP_PROTO::IP_PROTO_TCP, kTcpPorts(), isSai); } -void addDscpCounterAcl(const HwAsic* hwAsic, cfg::SwitchConfig* config) { +void addDscpCounterAcl( + const HwAsic* hwAsic, + cfg::SwitchConfig* config, + cfg::AclActionType actionType) { // Create ACL to count the number of packets with DSCP == ICP - utility::addDscpAclToCfg( + auto acl = utility::addDscpAclToCfg( hwAsic, config, kDscpCounterAclName(), utility::kIcpDscp()); + acl->actionType() = actionType; std::vector counterTypes{cfg::CounterType::PACKETS}; utility::addTrafficCounter(config, kCounterName(), counterTypes); cfg::MatchAction matchAction = cfg::MatchAction(); diff --git a/fboss/agent/test/utils/DscpMarkingUtils.h b/fboss/agent/test/utils/DscpMarkingUtils.h index 1138cfd2e48fe..ef33b38bba69b 100644 --- a/fboss/agent/test/utils/DscpMarkingUtils.h +++ b/fboss/agent/test/utils/DscpMarkingUtils.h @@ -34,7 +34,10 @@ void addDscpMarkingAcls( const HwAsic* hwAsic, cfg::SwitchConfig* config, bool isSai); -void addDscpCounterAcl(const HwAsic* hwAsic, cfg::SwitchConfig* config); +void addDscpCounterAcl( + const HwAsic* hwAsic, + cfg::SwitchConfig* config, + cfg::AclActionType actionType); void addDscpMarkingAclTable(cfg::SwitchConfig* config, bool isSai); void addDscpAclEntryWithCounter( cfg::SwitchConfig* config, diff --git a/fboss/agent/test/utils/TrafficPolicyTestUtils.cpp b/fboss/agent/test/utils/TrafficPolicyTestUtils.cpp index ab3b9c70577d8..350f7ba735572 100644 --- a/fboss/agent/test/utils/TrafficPolicyTestUtils.cpp +++ b/fboss/agent/test/utils/TrafficPolicyTestUtils.cpp @@ -15,7 +15,7 @@ DECLARE_bool(enable_acl_table_group); namespace facebook::fboss::utility { -void addDscpAclToCfg( +cfg::AclEntry* addDscpAclToCfg( const HwAsic* hwAsic, cfg::SwitchConfig* config, const std::string& aclName, @@ -25,7 +25,7 @@ void addDscpAclToCfg( acl.dscp() = dscp; utility::addEtherTypeToAcl(hwAsic, &acl, cfg::EtherType::IPv6); - utility::addAclEntry(config, acl, utility::kDefaultAclTable()); + return utility::addAclEntry(config, acl, utility::kDefaultAclTable()); } void addL4SrcPortAclToCfg( diff --git a/fboss/agent/test/utils/TrafficPolicyTestUtils.h b/fboss/agent/test/utils/TrafficPolicyTestUtils.h index 1e5e2c68c12e3..b4de9cc766833 100644 --- a/fboss/agent/test/utils/TrafficPolicyTestUtils.h +++ b/fboss/agent/test/utils/TrafficPolicyTestUtils.h @@ -21,7 +21,7 @@ */ namespace facebook::fboss::utility { -void addDscpAclToCfg( +cfg::AclEntry* addDscpAclToCfg( const HwAsic* hwAsic, cfg::SwitchConfig* config, const std::string& aclName,