From b0c361d4da1375aa89307fd475af88e6958c7680 Mon Sep 17 00:00:00 2001 From: Vybhav Date: Mon, 10 Aug 2026 17:04:21 +0000 Subject: [PATCH 1/3] Add fboss2-dev delete qos policy subcommands Adds the delete counterpart to the existing `config qos policy` family: fboss2-dev delete qos policy fboss2-dev delete qos policy map dscp fboss2-dev delete qos policy map tc-to-queue Whole-policy delete refuses when the policy is still named by dataPlaneTrafficPolicy or cpuTrafficPolicy.trafficPolicy, via either defaultQosPolicy or portIdToQosPolicy. The error reports the referring field so the operator knows what to unset first. Cascading instead would change forwarding on ports the user did not name. dscpMaps groups codepoints under an internalTrafficClass, so removing a DSCP value erases it from the group that lists it. A group left with no ingress codepoints and no fromTrafficClassToDscp rewrite carries no information and is dropped; one that still has the egress rewrite is kept. tc-to-queue is a plain map erase. Only dscp and tc-to-queue are removable; the other map types the config command writes are rejected by name. The policy-by-name scan and the DSCP bounds are shared with the config side via QosPolicyUtils, so both agree on what "the policy called X" means and on the valid DSCP range. --- cmake/CliFboss2.cmake | 8 + cmake/CliFboss2TestConfig.cmake | 1 + cmake/CliFboss2TestIntegrationTest.cmake | 1 + fboss/cli/fboss2/BUCK | 6 + fboss/cli/fboss2/CmdListConfig.cpp | 14 + .../commands/config/qos/QosPolicyUtils.cpp | 39 ++ .../commands/config/qos/QosPolicyUtils.h | 43 +++ .../qos/policy/CmdConfigQosPolicyMap.cpp | 20 +- .../delete/qos/policy/CmdDeleteQosPolicy.cpp | 119 ++++++ .../delete/qos/policy/CmdDeleteQosPolicy.h | 53 +++ .../qos/policy/CmdDeleteQosPolicyMap.cpp | 177 +++++++++ .../delete/qos/policy/CmdDeleteQosPolicyMap.h | 90 +++++ fboss/cli/fboss2/test/config/BUCK | 1 + .../test/config/CmdDeleteQosPolicyTest.cpp | 342 ++++++++++++++++++ fboss/cli/fboss2/test/integration_test/BUCK | 1 + .../integration_test/DeleteQosPolicyTest.cpp | 190 ++++++++++ 16 files changed, 1094 insertions(+), 11 deletions(-) create mode 100644 fboss/cli/fboss2/commands/config/qos/QosPolicyUtils.cpp create mode 100644 fboss/cli/fboss2/commands/config/qos/QosPolicyUtils.h create mode 100644 fboss/cli/fboss2/commands/delete/qos/policy/CmdDeleteQosPolicy.cpp create mode 100644 fboss/cli/fboss2/commands/delete/qos/policy/CmdDeleteQosPolicy.h create mode 100644 fboss/cli/fboss2/commands/delete/qos/policy/CmdDeleteQosPolicyMap.cpp create mode 100644 fboss/cli/fboss2/commands/delete/qos/policy/CmdDeleteQosPolicyMap.h create mode 100644 fboss/cli/fboss2/test/config/CmdDeleteQosPolicyTest.cpp create mode 100644 fboss/cli/fboss2/test/integration_test/DeleteQosPolicyTest.cpp diff --git a/cmake/CliFboss2.cmake b/cmake/CliFboss2.cmake index 75a97f40d27b9..945ea0da93197 100644 --- a/cmake/CliFboss2.cmake +++ b/cmake/CliFboss2.cmake @@ -995,6 +995,8 @@ add_library(fboss2_config_lib fboss/cli/fboss2/commands/config/qos/PortQueueConfigUtils.h fboss/cli/fboss2/commands/config/qos/default_policy/CmdConfigQosDefaultPolicy.cpp fboss/cli/fboss2/commands/config/qos/default_policy/CmdConfigQosDefaultPolicy.h + fboss/cli/fboss2/commands/config/qos/QosPolicyUtils.cpp + fboss/cli/fboss2/commands/config/qos/QosPolicyUtils.h fboss/cli/fboss2/commands/delete/qos/CmdDeleteQos.cpp fboss/cli/fboss2/commands/delete/qos/CmdDeleteQos.h fboss/cli/fboss2/commands/delete/qos/default_policy/CmdDeleteQosDefaultPolicy.cpp @@ -1090,6 +1092,12 @@ add_library(fboss2_config_lib fboss/cli/fboss2/commands/delete/protocol/static/route/CmdDeleteProtocolStaticRoute.h fboss/cli/fboss2/commands/delete/arp/CmdDeleteArp.cpp fboss/cli/fboss2/commands/delete/arp/CmdDeleteArp.h + fboss/cli/fboss2/commands/delete/qos/CmdDeleteQos.cpp + fboss/cli/fboss2/commands/delete/qos/CmdDeleteQos.h + fboss/cli/fboss2/commands/delete/qos/policy/CmdDeleteQosPolicy.cpp + fboss/cli/fboss2/commands/delete/qos/policy/CmdDeleteQosPolicy.h + fboss/cli/fboss2/commands/delete/qos/policy/CmdDeleteQosPolicyMap.cpp + fboss/cli/fboss2/commands/delete/qos/policy/CmdDeleteQosPolicyMap.h fboss/cli/fboss2/commands/delete/tunnel/CmdDeleteTunnel.cpp fboss/cli/fboss2/commands/delete/tunnel/CmdDeleteTunnel.h fboss/cli/fboss2/commands/delete/tunnel/ip_in_ip/CmdDeleteTunnelIpInIp.cpp diff --git a/cmake/CliFboss2TestConfig.cmake b/cmake/CliFboss2TestConfig.cmake index d0e15d2497718..382041e2f646e 100644 --- a/cmake/CliFboss2TestConfig.cmake +++ b/cmake/CliFboss2TestConfig.cmake @@ -43,6 +43,7 @@ add_executable(fboss2_cmd_config_test fboss/cli/fboss2/test/config/CmdDeleteDhcpTest.cpp fboss/cli/fboss2/test/config/CmdDeleteInterfaceIpv6NdpTest.cpp fboss/cli/fboss2/test/config/CmdDeleteInterfaceTest.cpp + fboss/cli/fboss2/test/config/CmdDeleteQosPolicyTest.cpp fboss/cli/fboss2/test/config/CmdDeleteQosQueueConfigTest.cpp fboss/cli/fboss2/test/config/CmdDeleteTunnelIpInIpTest.cpp fboss/cli/fboss2/test/config/CmdDeleteVlanTest.cpp diff --git a/cmake/CliFboss2TestIntegrationTest.cmake b/cmake/CliFboss2TestIntegrationTest.cmake index 1873293ac8710..6cb4f26541d12 100644 --- a/cmake/CliFboss2TestIntegrationTest.cmake +++ b/cmake/CliFboss2TestIntegrationTest.cmake @@ -45,6 +45,7 @@ add_executable(fboss2_integration_test fboss/cli/fboss2/test/integration_test/DeleteArpTest.cpp fboss/cli/fboss2/test/integration_test/DeleteDhcpSourceOverrideTest.cpp fboss/cli/fboss2/test/integration_test/DeleteInterfaceIpv6NdpTest.cpp + fboss/cli/fboss2/test/integration_test/DeleteQosPolicyTest.cpp fboss/cli/fboss2/test/integration_test/DeleteVlanTest.cpp fboss/cli/fboss2/test/integration_test/TunnelIpInIpTest.cpp fboss/cli/fboss2/utils/CmdInitUtils.cpp diff --git a/fboss/cli/fboss2/BUCK b/fboss/cli/fboss2/BUCK index 9e37cb04308c1..fe92d87f28a17 100644 --- a/fboss/cli/fboss2/BUCK +++ b/fboss/cli/fboss2/BUCK @@ -1161,6 +1161,7 @@ cpp_library( "commands/config/qos/PortQueueConfigUtils.cpp", "commands/config/qos/buffer_pool/CmdConfigQosBufferPool.cpp", "commands/config/qos/default_policy/CmdConfigQosDefaultPolicy.cpp", + "commands/config/qos/QosPolicyUtils.cpp", "commands/config/qos/policy/CmdConfigQosPolicy.cpp", "commands/config/qos/policy/CmdConfigQosPolicyMap.cpp", "commands/config/qos/priority_group_policy/CmdConfigQosPriorityGroupPolicy.cpp", @@ -1207,6 +1208,8 @@ cpp_library( "commands/delete/protocol/static/route/CmdDeleteProtocolStaticRoute.cpp", "commands/delete/qos/CmdDeleteQos.cpp", "commands/delete/qos/default_policy/CmdDeleteQosDefaultPolicy.cpp", + "commands/delete/qos/policy/CmdDeleteQosPolicy.cpp", + "commands/delete/qos/policy/CmdDeleteQosPolicyMap.cpp", "commands/delete/qos/queue_config/CmdDeleteQosQueueConfig.cpp", "commands/delete/qos/queue_config/CmdDeleteQosQueueConfigQueueId.cpp", "commands/delete/tunnel/CmdDeleteTunnel.cpp", @@ -1303,6 +1306,7 @@ cpp_library( "commands/config/qos/PortQueueConfigUtils.h", "commands/config/qos/buffer_pool/CmdConfigQosBufferPool.h", "commands/config/qos/default_policy/CmdConfigQosDefaultPolicy.h", + "commands/config/qos/QosPolicyUtils.h", "commands/config/qos/policy/CmdConfigQosPolicy.h", "commands/config/qos/policy/CmdConfigQosPolicyMap.h", "commands/config/qos/priority_group_policy/CmdConfigQosPriorityGroupPolicy.h", @@ -1349,6 +1353,8 @@ cpp_library( "commands/delete/protocol/static/route/CmdDeleteProtocolStaticRoute.h", "commands/delete/qos/CmdDeleteQos.h", "commands/delete/qos/default_policy/CmdDeleteQosDefaultPolicy.h", + "commands/delete/qos/policy/CmdDeleteQosPolicy.h", + "commands/delete/qos/policy/CmdDeleteQosPolicyMap.h", "commands/delete/qos/queue_config/CmdDeleteQosQueueConfig.h", "commands/delete/qos/queue_config/CmdDeleteQosQueueConfigQueueId.h", "commands/delete/tunnel/CmdDeleteTunnel.h", diff --git a/fboss/cli/fboss2/CmdListConfig.cpp b/fboss/cli/fboss2/CmdListConfig.cpp index 164a2fdca30c9..af42b2a4e065e 100644 --- a/fboss/cli/fboss2/CmdListConfig.cpp +++ b/fboss/cli/fboss2/CmdListConfig.cpp @@ -144,6 +144,8 @@ #include "fboss/cli/fboss2/commands/delete/protocol/static/route/CmdDeleteProtocolStaticRoute.h" #include "fboss/cli/fboss2/commands/delete/qos/CmdDeleteQos.h" #include "fboss/cli/fboss2/commands/delete/qos/default_policy/CmdDeleteQosDefaultPolicy.h" +#include "fboss/cli/fboss2/commands/delete/qos/policy/CmdDeleteQosPolicy.h" +#include "fboss/cli/fboss2/commands/delete/qos/policy/CmdDeleteQosPolicyMap.h" #include "fboss/cli/fboss2/commands/delete/qos/queue_config/CmdDeleteQosQueueConfig.h" #include "fboss/cli/fboss2/commands/delete/qos/queue_config/CmdDeleteQosQueueConfigQueueId.h" #include "fboss/cli/fboss2/commands/delete/tunnel/CmdDeleteTunnel.h" @@ -1122,6 +1124,18 @@ const CommandTree& kConfigCommandTree() { commandHandler, argRegistrar, }}, + }, + { + "policy", + "Delete a QoS policy or one of its map entries", + commandHandler, + argRegistrar, + {{ + "map", + "Remove a QoS map entry (dscp, tc-to-queue)", + commandHandler, + argRegistrar, + }}, }}, }, diff --git a/fboss/cli/fboss2/commands/config/qos/QosPolicyUtils.cpp b/fboss/cli/fboss2/commands/config/qos/QosPolicyUtils.cpp new file mode 100644 index 0000000000000..5a5e3b9925ba3 --- /dev/null +++ b/fboss/cli/fboss2/commands/config/qos/QosPolicyUtils.cpp @@ -0,0 +1,39 @@ +/* + * Copyright (c) 2004-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + */ + +#include "fboss/cli/fboss2/commands/config/qos/QosPolicyUtils.h" + +#include +#include +#include + +namespace facebook::fboss::utils { + +std::vector::iterator findQosPolicy( + std::vector& qosPolicies, + const std::string& name) { + return std::find_if( + qosPolicies.begin(), qosPolicies.end(), [&name](const auto& policy) { + return *policy.name() == name; + }); +} + +std::vector::iterator findQosPolicyOrThrow( + std::vector& qosPolicies, + const std::string& name) { + auto it = findQosPolicy(qosPolicies, name); + if (it == qosPolicies.end()) { + throw std::runtime_error( + fmt::format("No QoS policy named '{}' exists", name)); + } + return it; +} + +} // namespace facebook::fboss::utils diff --git a/fboss/cli/fboss2/commands/config/qos/QosPolicyUtils.h b/fboss/cli/fboss2/commands/config/qos/QosPolicyUtils.h new file mode 100644 index 0000000000000..d503549df180f --- /dev/null +++ b/fboss/cli/fboss2/commands/config/qos/QosPolicyUtils.h @@ -0,0 +1,43 @@ +/* + * Copyright (c) 2004-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + */ + +#pragma once + +#include +#include + +#include "fboss/agent/gen-cpp2/switch_config_types.h" + +namespace facebook::fboss::utils { + +// DSCP is a 6-bit field (RFC 2474): valid codepoints are 0..63. Shared so the +// config and delete qos-policy-map commands validate against one definition +// rather than each carrying their own copy. +constexpr int16_t kMinDscp = 0; +constexpr int16_t kMaxDscp = 63; + +// Locates the QosPolicy named `name` in `qosPolicies`, or end() when absent. +// sw.qosPolicies is a list keyed only by the name field, so every command that +// touches a policy has to do this scan; sharing it keeps `config qos policy` +// and `delete qos policy` agreeing on what "the policy called X" means. +// +// Returns an iterator rather than a pointer so callers can erase. +std::vector::iterator findQosPolicy( + std::vector& qosPolicies, + const std::string& name); + +// Like findQosPolicy, but throws std::runtime_error with a uniform +// "No QoS policy named '' exists" message when absent, so every delete +// command reports the same error instead of repeating the find-then-throw. +std::vector::iterator findQosPolicyOrThrow( + std::vector& qosPolicies, + const std::string& name); + +} // namespace facebook::fboss::utils diff --git a/fboss/cli/fboss2/commands/config/qos/policy/CmdConfigQosPolicyMap.cpp b/fboss/cli/fboss2/commands/config/qos/policy/CmdConfigQosPolicyMap.cpp index e34e87b4ff256..e1efa8221d6fc 100644 --- a/fboss/cli/fboss2/commands/config/qos/policy/CmdConfigQosPolicyMap.cpp +++ b/fboss/cli/fboss2/commands/config/qos/policy/CmdConfigQosPolicyMap.cpp @@ -25,6 +25,7 @@ #include #include "fboss/agent/gen-cpp2/switch_config_types.h" +#include "fboss/cli/fboss2/commands/config/qos/QosPolicyUtils.h" #include "fboss/cli/fboss2/commands/config/qos/policy/CmdConfigQosPolicy.h" #include "fboss/cli/fboss2/session/ConfigSession.h" #include "fboss/cli/fboss2/utils/HostInfo.h" @@ -35,9 +36,8 @@ namespace { constexpr int16_t kMinTCValue = 0; constexpr int16_t kMaxTCValue = 7; -// DSCP: 6-bit field (RFC 2474) -constexpr int8_t kMinDscpValue = 0; -constexpr int8_t kMaxDscpValue = 63; +// DSCP bounds (0..63) are shared with the delete command via utils::kMinDscp / +// utils::kMaxDscp in QosPolicyUtils.h. // MPLS EXP/TC: 3-bit field (RFC 3032, RFC 5462) constexpr int8_t kMinExpValue = 0; constexpr int8_t kMaxExpValue = 7; @@ -74,12 +74,12 @@ std::string getMapTypeString(QosMapType mapType, QosMapDirection direction) { // Validates value range based on type token and returns the map type. QosMapType validateAndGetMapType(const std::string& typeToken, int16_t value) { if (typeToken == "dscp") { - if (value < kMinDscpValue || value > kMaxDscpValue) { + if (value < utils::kMinDscp || value > utils::kMaxDscp) { throw std::invalid_argument( fmt::format( "DSCP value must be between {} and {}, got: {}", - kMinDscpValue, - kMaxDscpValue, + utils::kMinDscp, + utils::kMaxDscp, value)); } return QosMapType::DSCP; @@ -317,11 +317,9 @@ CmdConfigQosPolicyMapTraits::RetType CmdConfigQosPolicyMap::queryClient( // Find or create the QosPolicy with the given name cfg::QosPolicy* targetPolicy = nullptr; - for (auto& policy : qosPolicies) { - if (*policy.name() == name) { - targetPolicy = &policy; - break; - } + if (auto it = utils::findQosPolicy(qosPolicies, name); + it != qosPolicies.end()) { + targetPolicy = &*it; } if (targetPolicy == nullptr) { diff --git a/fboss/cli/fboss2/commands/delete/qos/policy/CmdDeleteQosPolicy.cpp b/fboss/cli/fboss2/commands/delete/qos/policy/CmdDeleteQosPolicy.cpp new file mode 100644 index 0000000000000..ef24c14a6b59c --- /dev/null +++ b/fboss/cli/fboss2/commands/delete/qos/policy/CmdDeleteQosPolicy.cpp @@ -0,0 +1,119 @@ +/* + * Copyright (c) 2004-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + */ + +#include "fboss/cli/fboss2/commands/delete/qos/policy/CmdDeleteQosPolicy.h" + +#include "fboss/cli/fboss2/CmdHandler.cpp" + +#include +#include +#include +#include +#include +#include + +#include "fboss/agent/gen-cpp2/switch_config_types.h" +#include "fboss/cli/fboss2/commands/config/qos/QosPolicyUtils.h" +#include "fboss/cli/fboss2/session/ConfigSession.h" +#include "fboss/cli/fboss2/utils/HostInfo.h" + +namespace facebook::fboss { + +namespace { + +// Collect every place a TrafficPolicyConfig can name a QoS policy, so the +// delete refuses instead of leaving a config that points at a policy which no +// longer exists. +void collectReferences( + const cfg::TrafficPolicyConfig& policyConfig, + const std::string& name, + const std::string& location, + std::vector& refs) { + if (policyConfig.defaultQosPolicy().has_value() && + *policyConfig.defaultQosPolicy() == name) { + refs.push_back(fmt::format("{}.defaultQosPolicy", location)); + } + if (policyConfig.portIdToQosPolicy().has_value()) { + for (const auto& [portId, policyName] : *policyConfig.portIdToQosPolicy()) { + if (policyName == name) { + refs.push_back( + fmt::format("{}.portIdToQosPolicy[{}]", location, portId)); + } + } + } +} + +std::vector findReferences( + const cfg::SwitchConfig& switchConfig, + const std::string& name) { + std::vector refs; + if (switchConfig.dataPlaneTrafficPolicy().has_value()) { + collectReferences( + *switchConfig.dataPlaneTrafficPolicy(), + name, + "dataPlaneTrafficPolicy", + refs); + } + if (switchConfig.cpuTrafficPolicy().has_value() && + switchConfig.cpuTrafficPolicy()->trafficPolicy().has_value()) { + collectReferences( + *switchConfig.cpuTrafficPolicy()->trafficPolicy(), + name, + "cpuTrafficPolicy.trafficPolicy", + refs); + } + return refs; +} + +} // namespace + +CmdDeleteQosPolicyTraits::RetType CmdDeleteQosPolicy::queryClient( + const HostInfo& /* hostInfo */, + const ObjectArgType& policyName) { + auto& session = ConfigSession::getInstance(); + auto& agentConfig = session.getAgentConfig(); + auto& switchConfig = *agentConfig.sw(); + + const std::string name = policyName.getName(); + if (name.empty()) { + throw std::runtime_error("qos policy name is required"); + } + auto& qosPolicies = *switchConfig.qosPolicies(); + + auto it = utils::findQosPolicyOrThrow(qosPolicies, name); + + // Refuse rather than cascade: clearing the referring field would silently + // change forwarding behaviour on ports the user did not name. Point at the + // exact fields so the operator knows what to unset first — there is no CLI + // today for defaultQosPolicy / portIdToQosPolicy (unlike + // `delete interface … queuing-policy`). + auto refs = findReferences(switchConfig, name); + if (!refs.empty()) { + throw std::runtime_error( + fmt::format( + "Cannot delete QoS policy '{}': still referenced by {}. " + "Unset those fields first, then retry the delete.", + name, + folly::join(", ", refs))); + } + + qosPolicies.erase(it); + session.saveConfig(); + + return fmt::format("Successfully deleted QoS policy '{}'", name); +} + +void CmdDeleteQosPolicy::printOutput(const RetType& logMsg) { + std::cout << logMsg << std::endl; +} + +template void CmdHandler::run(); + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/delete/qos/policy/CmdDeleteQosPolicy.h b/fboss/cli/fboss2/commands/delete/qos/policy/CmdDeleteQosPolicy.h new file mode 100644 index 0000000000000..f7a6ea4813b5b --- /dev/null +++ b/fboss/cli/fboss2/commands/delete/qos/policy/CmdDeleteQosPolicy.h @@ -0,0 +1,53 @@ +/* + * Copyright (c) 2004-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + */ + +#pragma once + +#include +#include +#include "fboss/cli/fboss2/CmdHandler.h" +#include "fboss/cli/fboss2/commands/config/qos/policy/CmdConfigQosPolicy.h" +#include "fboss/cli/fboss2/commands/delete/qos/CmdDeleteQos.h" +#include "fboss/cli/fboss2/utils/HostInfo.h" + +namespace facebook::fboss { + +struct CmdDeleteQosPolicyTraits : public WriteCommandTraits { + using ParentCmd = CmdDeleteQos; + static void addCliArg(CLI::App& cmd, std::vector& args) { + // required() + expected(1) keeps CLI11 from reclassifying the policy name + // as a subcommand when it happens to match one elsewhere in the tree. + cmd.add_option( + "qos_policy_name", + args, + "Name of the QoS policy to remove from qosPolicies") + ->required() + ->expected(1); + } + // Shared with `config qos policy` so the accepted name syntax cannot drift + // between the two commands. + using ObjectArgType = QosPolicyName; + using RetType = std::string; +}; + +class CmdDeleteQosPolicy + : public CmdHandler { + public: + using ObjectArgType = CmdDeleteQosPolicyTraits::ObjectArgType; + using RetType = CmdDeleteQosPolicyTraits::RetType; + + RetType queryClient( + const HostInfo& hostInfo, + const ObjectArgType& policyName); + + void printOutput(const RetType& logMsg); +}; + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/delete/qos/policy/CmdDeleteQosPolicyMap.cpp b/fboss/cli/fboss2/commands/delete/qos/policy/CmdDeleteQosPolicyMap.cpp new file mode 100644 index 0000000000000..8472a3dd05514 --- /dev/null +++ b/fboss/cli/fboss2/commands/delete/qos/policy/CmdDeleteQosPolicyMap.cpp @@ -0,0 +1,177 @@ +/* + * Copyright (c) 2004-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + */ + +#include "fboss/cli/fboss2/commands/delete/qos/policy/CmdDeleteQosPolicyMap.h" + +#include "fboss/cli/fboss2/CmdHandler.cpp" + +#include +#include +#include +#include +#include +#include +#include + +#include "fboss/agent/gen-cpp2/switch_config_types.h" +#include "fboss/cli/fboss2/commands/config/qos/QosPolicyUtils.h" +#include "fboss/cli/fboss2/session/ConfigSession.h" +#include "fboss/cli/fboss2/utils/HostInfo.h" + +namespace facebook::fboss { + +namespace { +constexpr auto kSupportedMapTypes = "dscp, tc-to-queue"; +} // namespace + +DeleteQosMapEntry::DeleteQosMapEntry(std::vector v) { + if (v.size() != 2) { + throw std::invalid_argument( + fmt::format( + "Expected ' ', got {} argument(s). " + "Valid map types: {}", + v.size(), + kSupportedMapTypes)); + } + + const std::string& mapType = v[0]; + if (mapType == "dscp") { + mapType_ = DeleteQosMapType::DSCP; + } else if (mapType == "tc-to-queue") { + mapType_ = DeleteQosMapType::TC_TO_QUEUE; + } else { + throw std::invalid_argument( + fmt::format( + "Unknown map type '{}'. Valid map types: {}", + mapType, + kSupportedMapTypes)); + } + + auto parsed = folly::tryTo(v[1]); + if (!parsed.hasValue()) { + throw std::invalid_argument( + fmt::format("Invalid value '{}'. Must be an integer", v[1])); + } + key_ = parsed.value(); + + if (mapType_ == DeleteQosMapType::DSCP) { + if (key_ < utils::kMinDscp || key_ > utils::kMaxDscp) { + throw std::invalid_argument( + fmt::format( + "dscp value must be between {} and {}, got: {}", + utils::kMinDscp, + utils::kMaxDscp, + v[1])); + } + } else if (key_ < 0) { + throw std::invalid_argument( + fmt::format("traffic class must not be negative, got: {}", v[1])); + } + + data_ = std::move(v); +} + +namespace { + +// Removes `dscp` from whichever DscpQosMap lists it. An entry that ends up +// with no ingress codepoints and no egress rewrite carries no information, so +// it is dropped rather than left behind as an empty shell. +bool eraseDscp(std::vector& dscpMaps, int16_t dscp) { + auto byteVal = static_cast(dscp); + bool erased = false; + + for (auto& entry : dscpMaps) { + auto& fromList = *entry.fromDscpToTrafficClass(); + auto it = std::find(fromList.begin(), fromList.end(), byteVal); + if (it != fromList.end()) { + fromList.erase(it); + erased = true; + } + } + + if (erased) { + dscpMaps.erase( + std::remove_if( + dscpMaps.begin(), + dscpMaps.end(), + [](const cfg::DscpQosMap& entry) { + return entry.fromDscpToTrafficClass()->empty() && + !entry.fromTrafficClassToDscp().has_value(); + }), + dscpMaps.end()); + } + + return erased; +} + +} // namespace + +CmdDeleteQosPolicyMapTraits::RetType CmdDeleteQosPolicyMap::queryClient( + const HostInfo& /* hostInfo */, + const QosPolicyName& policyName, + const ObjectArgType& entry) { + auto& session = ConfigSession::getInstance(); + auto& agentConfig = session.getAgentConfig(); + auto& switchConfig = *agentConfig.sw(); + + const std::string name = policyName.getName(); + auto& qosPolicies = *switchConfig.qosPolicies(); + + auto policyIt = utils::findQosPolicyOrThrow(qosPolicies, name); + + if (!policyIt->qosMap().has_value()) { + throw std::runtime_error( + fmt::format("QoS policy '{}' has no qosMap configured", name)); + } + auto& qosMap = *policyIt->qosMap(); + + switch (entry.getMapType()) { + case DeleteQosMapType::DSCP: { + if (!eraseDscp(*qosMap.dscpMaps(), entry.getKey())) { + throw std::runtime_error( + fmt::format( + "QoS policy '{}' has no dscp mapping for value {}", + name, + entry.getKey())); + } + session.saveConfig(); + return fmt::format( + "Successfully deleted QoS policy '{}' dscp mapping for value {}", + name, + entry.getKey()); + } + case DeleteQosMapType::TC_TO_QUEUE: { + auto& tcToQueue = *qosMap.trafficClassToQueueId(); + if (tcToQueue.erase(entry.getKey()) == 0) { + throw std::runtime_error( + fmt::format( + "QoS policy '{}' has no tc-to-queue mapping for traffic class {}", + name, + entry.getKey())); + } + session.saveConfig(); + return fmt::format( + "Successfully deleted QoS policy '{}' tc-to-queue mapping for traffic class {}", + name, + entry.getKey()); + } + } + + throw std::runtime_error("Unhandled map type"); +} + +void CmdDeleteQosPolicyMap::printOutput(const RetType& logMsg) { + std::cout << logMsg << std::endl; +} + +template void +CmdHandler::run(); + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/delete/qos/policy/CmdDeleteQosPolicyMap.h b/fboss/cli/fboss2/commands/delete/qos/policy/CmdDeleteQosPolicyMap.h new file mode 100644 index 0000000000000..b82b003a3b54e --- /dev/null +++ b/fboss/cli/fboss2/commands/delete/qos/policy/CmdDeleteQosPolicyMap.h @@ -0,0 +1,90 @@ +/* + * Copyright (c) 2004-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + */ + +#pragma once + +#include +#include +#include +#include "fboss/cli/fboss2/CmdHandler.h" +#include "fboss/cli/fboss2/commands/delete/qos/policy/CmdDeleteQosPolicy.h" +#include "fboss/cli/fboss2/utils/CmdUtilsCommon.h" +#include "fboss/cli/fboss2/utils/HostInfo.h" + +namespace facebook::fboss { + +/** + * Which QosMap a delete targets. Only the two map types the config command + * writes as individually addressable entries are removable today; the + * remaining types CmdConfigQosPolicyMap supports are rejected by name. + */ +enum class DeleteQosMapType { + DSCP, // QosMap.dscpMaps + TC_TO_QUEUE, // QosMap.trafficClassToQueueId +}; + +/** + * Parses the entry to remove: + * + * dscp - drop a DSCP codepoint from the policy's dscpMaps + * tc-to-queue - drop a traffic class from trafficClassToQueueId + * + * The value alone identifies the entry: a DSCP codepoint appears in at most + * one DscpQosMap, and a traffic class is a map key. + */ +class DeleteQosMapEntry : public utils::BaseObjectArgType { + public: + // NOLINTNEXTLINE(google-explicit-constructor) + /* implicit */ DeleteQosMapEntry(std::vector v); + + DeleteQosMapType getMapType() const { + return mapType_; + } + + int16_t getKey() const { + return key_; + } + + private: + DeleteQosMapType mapType_{DeleteQosMapType::TC_TO_QUEUE}; + int16_t key_{0}; +}; + +struct CmdDeleteQosPolicyMapTraits : public WriteCommandTraits { + using ParentCmd = CmdDeleteQosPolicy; + static void addCliArg(CLI::App& cmd, std::vector& args) { + cmd.add_option( + "map_entry", + args, + " where map-type is one of:\n" + " dscp remove a DSCP to traffic-class mapping\n" + " tc-to-queue remove a traffic-class to queue mapping") + ->required() + ->expected(2); + } + using ObjectArgType = DeleteQosMapEntry; + using RetType = std::string; +}; + +class CmdDeleteQosPolicyMap + : public CmdHandler { + public: + using ObjectArgType = CmdDeleteQosPolicyMapTraits::ObjectArgType; + using RetType = CmdDeleteQosPolicyMapTraits::RetType; + + RetType queryClient( + const HostInfo& hostInfo, + const QosPolicyName& policyName, + const ObjectArgType& entry); + + void printOutput(const RetType& logMsg); +}; + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/test/config/BUCK b/fboss/cli/fboss2/test/config/BUCK index ed105d2d8998b..e7e972266cfeb 100644 --- a/fboss/cli/fboss2/test/config/BUCK +++ b/fboss/cli/fboss2/test/config/BUCK @@ -50,6 +50,7 @@ cpp_unittest( "CmdDeleteTunnelIpInIpTest.cpp", "CmdDeleteVlanTest.cpp", "ConfigSessionSystemdTest.cpp", + "CmdDeleteQosPolicyTest.cpp", "ProfileValidationTest.cpp", ], headers = [ diff --git a/fboss/cli/fboss2/test/config/CmdDeleteQosPolicyTest.cpp b/fboss/cli/fboss2/test/config/CmdDeleteQosPolicyTest.cpp new file mode 100644 index 0000000000000..97c0403784307 --- /dev/null +++ b/fboss/cli/fboss2/test/config/CmdDeleteQosPolicyTest.cpp @@ -0,0 +1,342 @@ +// (c) Meta Platforms, Inc. and affiliates. Confidential and proprietary. + +#include "fboss/agent/gen-cpp2/switch_config_types.h" +#include "fboss/cli/fboss2/test/config/CmdConfigTestBase.h" + +#include +#include +#include +#include + +#include "fboss/cli/fboss2/commands/delete/qos/policy/CmdDeleteQosPolicy.h" +#include "fboss/cli/fboss2/commands/delete/qos/policy/CmdDeleteQosPolicyMap.h" +#include "fboss/cli/fboss2/session/ConfigSession.h" + +namespace facebook::fboss { + +// Two policies: "unreferenced" is free to delete, "in-use" is named by +// dataPlaneTrafficPolicy.defaultQosPolicy so the delete must refuse it. +// dscpMaps groups codepoints under an internalTrafficClass, matching the +// shape CmdConfigQosPolicyMap writes. +static const std::string kSeedConfig = R"({ + "sw": { + "qosPolicies": [ + { + "name": "unreferenced", + "rules": [], + "qosMap": { + "dscpMaps": [ + {"internalTrafficClass": 0, "fromDscpToTrafficClass": [0, 1, 2]}, + {"internalTrafficClass": 1, "fromDscpToTrafficClass": [8], "fromTrafficClassToDscp": 8} + ], + "expMaps": [], + "trafficClassToQueueId": {"0": 0, "1": 1, "2": 2} + } + }, + { + "name": "in-use", + "rules": [], + "qosMap": { + "dscpMaps": [], + "expMaps": [], + "trafficClassToQueueId": {"0": 0} + } + }, + {"name": "port-ref", "rules": []}, + {"name": "cpu-ref", "rules": []}, + {"name": "no-map", "rules": []} + ], + "dataPlaneTrafficPolicy": { + "defaultQosPolicy": "in-use", + "portIdToQosPolicy": {"1": "port-ref"} + }, + "cpuTrafficPolicy": { + "trafficPolicy": { + "defaultQosPolicy": "cpu-ref" + } + } + } +})"; + +class CmdDeleteQosPolicyTestFixture : public CmdConfigTestBase { + public: + CmdDeleteQosPolicyTestFixture() + : CmdConfigTestBase( + "fboss_del_qos_policy_test_%%%%-%%%%-%%%%-%%%%", + kSeedConfig) {} + + protected: + static const cfg::QosPolicy* findPolicy(const std::string& name) { + const auto& policies = + *ConfigSession::getInstance().getAgentConfig().sw()->qosPolicies(); + for (const auto& policy : policies) { + if (*policy.name() == name) { + return &policy; + } + } + return nullptr; + } + + static const cfg::DscpQosMap* findDscpEntry( + const std::string& policyName, + int16_t trafficClass) { + const auto* policy = findPolicy(policyName); + if (policy == nullptr || !policy->qosMap().has_value()) { + return nullptr; + } + for (const auto& entry : *policy->qosMap()->dscpMaps()) { + if (*entry.internalTrafficClass() == trafficClass) { + return &entry; + } + } + return nullptr; + } +}; + +// ---------------------------------------------------------------- arg parsing + +// The name type is shared with `config qos policy`; arity is enforced by +// CLI11 (required/expected(1)), so only the empty-name guard is ours. +TEST_F(CmdDeleteQosPolicyTestFixture, policyNameArgValidation) { + EXPECT_EQ(QosPolicyName({"p1"}).getName(), "p1"); + EXPECT_EQ(QosPolicyName({}).getName(), ""); + + setupTestableConfigSession("delete qos policy", ""); + auto cmd = CmdDeleteQosPolicy(); + EXPECT_THROW( + cmd.queryClient(localhost(), QosPolicyName({})), std::runtime_error); +} + +TEST_F(CmdDeleteQosPolicyTestFixture, mapEntryArgValidation) { + EXPECT_EQ(DeleteQosMapEntry({"dscp", "40"}).getKey(), 40); + EXPECT_EQ( + DeleteQosMapEntry({"dscp", "40"}).getMapType(), DeleteQosMapType::DSCP); + EXPECT_EQ( + DeleteQosMapEntry({"tc-to-queue", "3"}).getMapType(), + DeleteQosMapType::TC_TO_QUEUE); + + // wrong arity + EXPECT_THROW(DeleteQosMapEntry({"dscp"}), std::invalid_argument); + EXPECT_THROW(DeleteQosMapEntry({"dscp", "1", "2"}), std::invalid_argument); + // unknown map type + EXPECT_THROW( + DeleteQosMapEntry({"pfc-pri-to-pg", "1"}), std::invalid_argument); + // non-integer value + EXPECT_THROW(DeleteQosMapEntry({"dscp", "abc"}), std::invalid_argument); + // dscp out of range + EXPECT_THROW(DeleteQosMapEntry({"dscp", "64"}), std::invalid_argument); + EXPECT_THROW(DeleteQosMapEntry({"dscp", "-1"}), std::invalid_argument); + // negative traffic class + EXPECT_THROW(DeleteQosMapEntry({"tc-to-queue", "-1"}), std::invalid_argument); + // boundary values accepted + EXPECT_NO_THROW(DeleteQosMapEntry({"dscp", "0"})); + EXPECT_NO_THROW(DeleteQosMapEntry({"dscp", "63"})); +} + +// -------------------------------------------------------- delete whole policy + +TEST_F(CmdDeleteQosPolicyTestFixture, deleteUnreferencedPolicy) { + setupTestableConfigSession("delete qos policy", "unreferenced"); + + auto cmd = CmdDeleteQosPolicy(); + auto result = cmd.queryClient(localhost(), QosPolicyName({"unreferenced"})); + + EXPECT_THAT(result, ::testing::HasSubstr("Successfully deleted")); + EXPECT_EQ(findPolicy("unreferenced"), nullptr); + // the other policy survives + EXPECT_NE(findPolicy("in-use"), nullptr); +} + +TEST_F(CmdDeleteQosPolicyTestFixture, deleteReferencedPolicyRefused) { + setupTestableConfigSession("delete qos policy", "in-use"); + + auto cmd = CmdDeleteQosPolicy(); + try { + cmd.queryClient(localhost(), QosPolicyName({"in-use"})); + FAIL() << "expected delete of referenced policy to throw"; + } catch (const std::runtime_error& e) { + EXPECT_THAT( + e.what(), + ::testing::AllOf( + ::testing::HasSubstr("dataPlaneTrafficPolicy.defaultQosPolicy"), + ::testing::HasSubstr("Unset those fields first"))); + } + + // refusal leaves the policy in place + EXPECT_NE(findPolicy("in-use"), nullptr); +} + +TEST_F(CmdDeleteQosPolicyTestFixture, deleteMissingPolicyFails) { + setupTestableConfigSession("delete qos policy", "nope"); + + auto cmd = CmdDeleteQosPolicy(); + EXPECT_THROW( + cmd.queryClient(localhost(), QosPolicyName({"nope"})), + std::runtime_error); +} + +// ------------------------------------------------------------ delete map dscp + +TEST_F(CmdDeleteQosPolicyTestFixture, deleteDscpFromSharedEntry) { + setupTestableConfigSession("delete qos policy map", "dscp 1"); + + auto cmd = CmdDeleteQosPolicyMap(); + auto result = cmd.queryClient( + localhost(), + QosPolicyName({"unreferenced"}), + DeleteQosMapEntry({"dscp", "1"})); + + EXPECT_THAT(result, ::testing::HasSubstr("Successfully deleted")); + + // tc 0 keeps its other codepoints; the entry itself survives + const auto* entry = findDscpEntry("unreferenced", 0); + ASSERT_NE(entry, nullptr); + EXPECT_THAT(*entry->fromDscpToTrafficClass(), ::testing::ElementsAre(0, 2)); +} + +TEST_F(CmdDeleteQosPolicyTestFixture, deleteLastDscpDropsEntry) { + setupTestableConfigSession("delete qos policy map", "dscp 0"); + + auto cmd = CmdDeleteQosPolicyMap(); + // remove every codepoint mapped to tc 0 + for (const auto& dscp : {"0", "1", "2"}) { + cmd.queryClient( + localhost(), + QosPolicyName({"unreferenced"}), + DeleteQosMapEntry({"dscp", dscp})); + } + + // entry carried no egress rewrite, so it is dropped rather than left empty + EXPECT_EQ(findDscpEntry("unreferenced", 0), nullptr); +} + +TEST_F(CmdDeleteQosPolicyTestFixture, deleteDscpKeepsEntryWithEgressRewrite) { + setupTestableConfigSession("delete qos policy map", "dscp 8"); + + auto cmd = CmdDeleteQosPolicyMap(); + cmd.queryClient( + localhost(), + QosPolicyName({"unreferenced"}), + DeleteQosMapEntry({"dscp", "8"})); + + // fromTrafficClassToDscp is still set, so the entry must stay + const auto* entry = findDscpEntry("unreferenced", 1); + ASSERT_NE(entry, nullptr); + EXPECT_TRUE(entry->fromDscpToTrafficClass()->empty()); + EXPECT_TRUE(entry->fromTrafficClassToDscp().has_value()); +} + +TEST_F(CmdDeleteQosPolicyTestFixture, deleteMissingDscpFails) { + setupTestableConfigSession("delete qos policy map", "dscp 63"); + + auto cmd = CmdDeleteQosPolicyMap(); + EXPECT_THROW( + cmd.queryClient( + localhost(), + QosPolicyName({"unreferenced"}), + DeleteQosMapEntry({"dscp", "63"})), + std::runtime_error); +} + +// ----------------------------------------------------- delete map tc-to-queue + +TEST_F(CmdDeleteQosPolicyTestFixture, deleteTcToQueue) { + setupTestableConfigSession("delete qos policy map", "tc-to-queue 1"); + + auto cmd = CmdDeleteQosPolicyMap(); + auto result = cmd.queryClient( + localhost(), + QosPolicyName({"unreferenced"}), + DeleteQosMapEntry({"tc-to-queue", "1"})); + + EXPECT_THAT(result, ::testing::HasSubstr("Successfully deleted")); + + const auto* policy = findPolicy("unreferenced"); + ASSERT_NE(policy, nullptr); + const auto& tcToQueue = *policy->qosMap()->trafficClassToQueueId(); + EXPECT_EQ(tcToQueue.count(1), 0); + // neighbouring keys untouched + EXPECT_EQ(tcToQueue.at(0), 0); + EXPECT_EQ(tcToQueue.at(2), 2); +} + +TEST_F(CmdDeleteQosPolicyTestFixture, deleteMissingTcToQueueFails) { + setupTestableConfigSession("delete qos policy map", "tc-to-queue 7"); + + auto cmd = CmdDeleteQosPolicyMap(); + EXPECT_THROW( + cmd.queryClient( + localhost(), + QosPolicyName({"unreferenced"}), + DeleteQosMapEntry({"tc-to-queue", "7"})), + std::runtime_error); +} + +TEST_F(CmdDeleteQosPolicyTestFixture, mapDeleteOnMissingPolicyFails) { + setupTestableConfigSession("delete qos policy map", "tc-to-queue 0"); + + auto cmd = CmdDeleteQosPolicyMap(); + EXPECT_THROW( + cmd.queryClient( + localhost(), + QosPolicyName({"nope"}), + DeleteQosMapEntry({"tc-to-queue", "0"})), + std::runtime_error); +} + +// A policy named only through portIdToQosPolicy (not defaultQosPolicy) is still +// refused, and the error points at the per-port field so the operator knows +// which port binding to unset. +TEST_F(CmdDeleteQosPolicyTestFixture, deletePortReferencedPolicyRefused) { + setupTestableConfigSession("delete qos policy", "port-ref"); + + auto cmd = CmdDeleteQosPolicy(); + try { + cmd.queryClient(localhost(), QosPolicyName({"port-ref"})); + FAIL() << "expected delete of port-referenced policy to throw"; + } catch (const std::runtime_error& e) { + EXPECT_THAT( + std::string(e.what()), + ::testing::HasSubstr("dataPlaneTrafficPolicy.portIdToQosPolicy")); + } + + EXPECT_NE(findPolicy("port-ref"), nullptr); +} + +// A policy named by cpuTrafficPolicy.trafficPolicy is refused, exercising the +// second TrafficPolicyConfig findReferences scans. +TEST_F(CmdDeleteQosPolicyTestFixture, deleteCpuReferencedPolicyRefused) { + setupTestableConfigSession("delete qos policy", "cpu-ref"); + + auto cmd = CmdDeleteQosPolicy(); + try { + cmd.queryClient(localhost(), QosPolicyName({"cpu-ref"})); + FAIL() << "expected delete of cpu-referenced policy to throw"; + } catch (const std::runtime_error& e) { + EXPECT_THAT( + std::string(e.what()), + ::testing::HasSubstr("cpuTrafficPolicy.trafficPolicy")); + } + + EXPECT_NE(findPolicy("cpu-ref"), nullptr); +} + +// Deleting a map entry on a policy that has no qosMap reports the missing map +// rather than dereferencing an unset optional. +TEST_F(CmdDeleteQosPolicyTestFixture, mapDeleteOnPolicyWithNoQosMapFails) { + setupTestableConfigSession("delete qos policy map", "dscp 1"); + + auto cmd = CmdDeleteQosPolicyMap(); + try { + cmd.queryClient( + localhost(), + QosPolicyName({"no-map"}), + DeleteQosMapEntry({"dscp", "1"})); + FAIL() << "expected map delete on policy without qosMap to throw"; + } catch (const std::runtime_error& e) { + EXPECT_THAT( + std::string(e.what()), ::testing::HasSubstr("no qosMap configured")); + } +} + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/test/integration_test/BUCK b/fboss/cli/fboss2/test/integration_test/BUCK index b6ec5b952c37b..5ec6f4fa10eb8 100644 --- a/fboss/cli/fboss2/test/integration_test/BUCK +++ b/fboss/cli/fboss2/test/integration_test/BUCK @@ -53,6 +53,7 @@ cpp_binary( "DeleteArpTest.cpp", "DeleteDhcpSourceOverrideTest.cpp", "DeleteInterfaceIpv6NdpTest.cpp", + "DeleteQosPolicyTest.cpp", "DeleteInterfaceTest.cpp", "DeleteVlanTest.cpp", "Fboss2IntegrationTest.cpp", diff --git a/fboss/cli/fboss2/test/integration_test/DeleteQosPolicyTest.cpp b/fboss/cli/fboss2/test/integration_test/DeleteQosPolicyTest.cpp new file mode 100644 index 0000000000000..7a91b292e1da7 --- /dev/null +++ b/fboss/cli/fboss2/test/integration_test/DeleteQosPolicyTest.cpp @@ -0,0 +1,190 @@ +// (c) Meta Platforms, Inc. and affiliates. Confidential and proprietary. + +/** + * End-to-end test for `delete qos policy`: + * + * - delete qos policy map dscp — drops a codepoint from + * sw.qosPolicies[*].qosMap.dscpMaps + * - delete qos policy map tc-to-queue — drops a key from + * sw.qosPolicies[*].qosMap.trafficClassToQueueId + * - delete qos policy — removes the policy + * + * The three run in one test because each is the inverse of a step that set the + * policy up: the scratch policy is created here, dismantled entry by entry, + * then removed, so nothing is left behind and the test does not depend on DUT + * state. A stock switch has no qosPolicies at all. + * + * The refusal path (deleting a policy still named by dataPlaneTrafficPolicy or + * cpuTrafficPolicy.trafficPolicy) is not covered here: no CLI command writes + * defaultQosPolicy or portIdToQosPolicy, so a reference cannot be created from + * the command line. That path is covered by CmdDeleteQosPolicyTest, which + * seeds the reference directly into the config. + * + * QoS map edits commit at HITLESS, so the agents do not restart; post-commit + * state is still read through waitForRunningConfig() to avoid racing the + * config reload. + */ + +#include +#include +#include +#include +#include + +#include "fboss/cli/fboss2/test/integration_test/Fboss2IntegrationTest.h" + +using namespace facebook::fboss; +using ::testing::HasSubstr; + +class DeleteQosPolicyTest : public Fboss2IntegrationTest { + protected: + // Name used for the scratch policy this test creates and removes. + static constexpr auto kPolicy = "fboss2-it-scratch-qos"; + static constexpr auto kDscp = "40"; + static constexpr auto kTrafficClass = "5"; + static constexpr auto kQueueId = "0"; + + static const folly::dynamic* findPolicy( + const folly::dynamic& config, + const std::string& name) { + if (!config.count("sw") || !config["sw"].count("qosPolicies")) { + return nullptr; + } + for (const auto& policy : config["sw"]["qosPolicies"]) { + if (policy.count("name") && policy["name"].asString() == name) { + return &policy; + } + } + return nullptr; + } + + static bool hasPolicy(const folly::dynamic& config, const std::string& name) { + return findPolicy(config, name) != nullptr; + } + + // True when the policy maps `dscp` to some traffic class. + static bool + hasDscp(const folly::dynamic& config, const std::string& name, int dscp) { + const auto* policy = findPolicy(config, name); + if (policy == nullptr || !policy->count("qosMap") || + !(*policy)["qosMap"].count("dscpMaps")) { + return false; + } + for (const auto& entry : (*policy)["qosMap"]["dscpMaps"]) { + if (!entry.count("fromDscpToTrafficClass")) { + continue; + } + for (const auto& value : entry["fromDscpToTrafficClass"]) { + if (value.asInt() == dscp) { + return true; + } + } + } + return false; + } + + // True when the policy maps traffic class `tc` to a queue. + static bool hasTcToQueue( + const folly::dynamic& config, + const std::string& name, + const std::string& tc) { + const auto* policy = findPolicy(config, name); + if (policy == nullptr || !policy->count("qosMap") || + !(*policy)["qosMap"].count("trafficClassToQueueId")) { + return false; + } + return (*policy)["qosMap"]["trafficClassToQueueId"].count(tc) > 0; + } +}; + +TEST_F(DeleteQosPolicyTest, DeleteMapEntriesThenPolicy) { + // 1. Create a scratch policy carrying one dscp mapping and one tc-to-queue + // mapping. `config qos policy map ...` creates the policy when it + // does not already exist. + XLOG(INFO) << "Creating qos policy " << kPolicy; + auto result = runCli( + {"config", + "qos", + "policy", + kPolicy, + "map", + "dscp", + kDscp, + "traffic-class", + kTrafficClass}); + ASSERT_EQ(result.exitCode, 0) + << "stdout=" << result.stdout << " stderr=" << result.stderr; + + result = runCli( + {"config", + "qos", + "policy", + kPolicy, + "map", + "tc-to-queue", + kTrafficClass, + kQueueId}); + ASSERT_EQ(result.exitCode, 0) + << "stdout=" << result.stdout << " stderr=" << result.stderr; + commitConfig(); + + auto config = waitForRunningConfig([&](const folly::dynamic& cfg) { + return hasPolicy(cfg, kPolicy) && hasDscp(cfg, kPolicy, std::stoi(kDscp)) && + hasTcToQueue(cfg, kPolicy, kTrafficClass); + }); + ASSERT_TRUE(hasPolicy(config, kPolicy)) + << "qos policy " << kPolicy << " missing after commit"; + ASSERT_TRUE(hasDscp(config, kPolicy, std::stoi(kDscp))); + ASSERT_TRUE(hasTcToQueue(config, kPolicy, kTrafficClass)); + + // 2. Delete the dscp mapping. The tc-to-queue mapping must survive. + XLOG(INFO) << "Deleting dscp " << kDscp << " from " << kPolicy; + result = runCli({"delete", "qos", "policy", kPolicy, "map", "dscp", kDscp}); + ASSERT_EQ(result.exitCode, 0) + << "stdout=" << result.stdout << " stderr=" << result.stderr; + commitConfig(); + config = waitForRunningConfig([&](const folly::dynamic& cfg) { + return !hasDscp(cfg, kPolicy, std::stoi(kDscp)); + }); + EXPECT_FALSE(hasDscp(config, kPolicy, std::stoi(kDscp))); + EXPECT_TRUE(hasTcToQueue(config, kPolicy, kTrafficClass)) + << "deleting the dscp mapping must not touch trafficClassToQueueId"; + + // 3. Deleting the same dscp again is an error, not a silent success. + result = runCli({"delete", "qos", "policy", kPolicy, "map", "dscp", kDscp}); + EXPECT_NE(result.exitCode, 0); + EXPECT_THAT(result.stderr, HasSubstr("no dscp mapping")); + + // 4. Delete the tc-to-queue mapping. + XLOG(INFO) << "Deleting tc-to-queue " << kTrafficClass << " from " << kPolicy; + result = runCli( + {"delete", + "qos", + "policy", + kPolicy, + "map", + "tc-to-queue", + kTrafficClass}); + ASSERT_EQ(result.exitCode, 0) + << "stdout=" << result.stdout << " stderr=" << result.stderr; + commitConfig(); + config = waitForRunningConfig([&](const folly::dynamic& cfg) { + return !hasTcToQueue(cfg, kPolicy, kTrafficClass); + }); + EXPECT_FALSE(hasTcToQueue(config, kPolicy, kTrafficClass)); + + // 5. Delete the policy itself. Nothing references it, so this is allowed. + XLOG(INFO) << "Deleting qos policy " << kPolicy; + result = runCli({"delete", "qos", "policy", kPolicy}); + ASSERT_EQ(result.exitCode, 0) + << "stdout=" << result.stdout << " stderr=" << result.stderr; + commitConfig(); + config = waitForRunningConfig( + [&](const folly::dynamic& cfg) { return !hasPolicy(cfg, kPolicy); }); + EXPECT_FALSE(hasPolicy(config, kPolicy)); + + // 6. Deleting it again reports that it does not exist. + result = runCli({"delete", "qos", "policy", kPolicy}); + EXPECT_NE(result.exitCode, 0); + EXPECT_THAT(result.stderr, HasSubstr("No QoS policy named")); +} From 2263fad1c5b66b588bc1a170ab7a7f2f2e6d675f Mon Sep 17 00:00:00 2001 From: Vybhav Date: Fri, 14 Aug 2026 05:59:43 +0000 Subject: [PATCH 2/3] Extend delete qos policy map to every QosMap type The config command writes all seven QosMap types but only dscp and tc-to-queue were deletable, leaving the other five as one-way doors: settable via CLI with no CLI way back. Adds mpls-exp, dot1p, pfc-pri-to-queue, tc-to-pg and pfc-pri-to-pg using the same tokens as the config command. List maps (dscp/exp/pcp) share one erase helper: the codepoint is removed from whichever entry's ingress list carries it, and entries left with no codepoints and no egress rewrite are dropped. Optional simple maps are reset once their last key is erased, returning the field to its unset default. --- .../qos/policy/CmdDeleteQosPolicyMap.cpp | 177 ++++++++++++++++-- .../delete/qos/policy/CmdDeleteQosPolicyMap.h | 32 +++- .../test/config/CmdDeleteQosPolicyTest.cpp | 144 +++++++++++++- 3 files changed, 323 insertions(+), 30 deletions(-) diff --git a/fboss/cli/fboss2/commands/delete/qos/policy/CmdDeleteQosPolicyMap.cpp b/fboss/cli/fboss2/commands/delete/qos/policy/CmdDeleteQosPolicyMap.cpp index 8472a3dd05514..1e0985ec664ea 100644 --- a/fboss/cli/fboss2/commands/delete/qos/policy/CmdDeleteQosPolicyMap.cpp +++ b/fboss/cli/fboss2/commands/delete/qos/policy/CmdDeleteQosPolicyMap.cpp @@ -28,7 +28,12 @@ namespace facebook::fboss { namespace { -constexpr auto kSupportedMapTypes = "dscp, tc-to-queue"; +constexpr auto kSupportedMapTypes = + "dscp, mpls-exp, dot1p, tc-to-queue, pfc-pri-to-queue, tc-to-pg, " + "pfc-pri-to-pg"; +// Traffic classes, PFC priorities, EXP and PCP codepoints are all 3-bit. +constexpr int16_t kMinMapKey = 0; +constexpr int16_t kMaxMapKey = 7; } // namespace DeleteQosMapEntry::DeleteQosMapEntry(std::vector v) { @@ -44,8 +49,18 @@ DeleteQosMapEntry::DeleteQosMapEntry(std::vector v) { const std::string& mapType = v[0]; if (mapType == "dscp") { mapType_ = DeleteQosMapType::DSCP; + } else if (mapType == "mpls-exp") { + mapType_ = DeleteQosMapType::MPLS_EXP; + } else if (mapType == "dot1p") { + mapType_ = DeleteQosMapType::DOT1P; } else if (mapType == "tc-to-queue") { mapType_ = DeleteQosMapType::TC_TO_QUEUE; + } else if (mapType == "pfc-pri-to-queue") { + mapType_ = DeleteQosMapType::PFC_PRI_TO_QUEUE; + } else if (mapType == "tc-to-pg") { + mapType_ = DeleteQosMapType::TC_TO_PG; + } else if (mapType == "pfc-pri-to-pg") { + mapType_ = DeleteQosMapType::PFC_PRI_TO_PG; } else { throw std::invalid_argument( fmt::format( @@ -70,9 +85,14 @@ DeleteQosMapEntry::DeleteQosMapEntry(std::vector v) { utils::kMaxDscp, v[1])); } - } else if (key_ < 0) { + } else if (key_ < kMinMapKey || key_ > kMaxMapKey) { throw std::invalid_argument( - fmt::format("traffic class must not be negative, got: {}", v[1])); + fmt::format( + "{} value must be between {} and {}, got: {}", + mapType, + kMinMapKey, + kMaxMapKey, + v[1])); } data_ = std::move(v); @@ -80,15 +100,22 @@ DeleteQosMapEntry::DeleteQosMapEntry(std::vector v) { namespace { -// Removes `dscp` from whichever DscpQosMap lists it. An entry that ends up -// with no ingress codepoints and no egress rewrite carries no information, so -// it is dropped rather than left behind as an empty shell. -bool eraseDscp(std::vector& dscpMaps, int16_t dscp) { - auto byteVal = static_cast(dscp); +// Removes `codepoint` from whichever map entry's ingress list carries it. An +// entry that ends up with no ingress codepoints and no egress rewrite carries +// no information, so it is dropped rather than left behind as an empty shell. +// Shared by the three structurally identical list maps (DscpQosMap, ExpQosMap, +// PcpQosMap); the projections select the ingress list and the egress rewrite. +template +bool eraseCodepoint( + std::vector& maps, + int16_t codepoint, + GetIngressList getIngressList, + GetEgressValue getEgressValue) { + auto byteVal = static_cast(codepoint); bool erased = false; - for (auto& entry : dscpMaps) { - auto& fromList = *entry.fromDscpToTrafficClass(); + for (auto& entry : maps) { + auto& fromList = getIngressList(entry); auto it = std::find(fromList.begin(), fromList.end(), byteVal); if (it != fromList.end()) { fromList.erase(it); @@ -97,20 +124,61 @@ bool eraseDscp(std::vector& dscpMaps, int16_t dscp) { } if (erased) { - dscpMaps.erase( + maps.erase( std::remove_if( - dscpMaps.begin(), - dscpMaps.end(), - [](const cfg::DscpQosMap& entry) { - return entry.fromDscpToTrafficClass()->empty() && - !entry.fromTrafficClassToDscp().has_value(); + maps.begin(), + maps.end(), + [&](const MapEntry& entry) { + return getIngressList(const_cast(entry)).empty() && + !getEgressValue(entry).has_value(); }), - dscpMaps.end()); + maps.end()); } return erased; } +bool eraseDscp(std::vector& dscpMaps, int16_t dscp) { + return eraseCodepoint( + dscpMaps, + dscp, + [](cfg::DscpQosMap& e) -> auto& { return *e.fromDscpToTrafficClass(); }, + [](const cfg::DscpQosMap& e) { return e.fromTrafficClassToDscp(); }); +} + +bool eraseExp(std::vector& expMaps, int16_t exp) { + return eraseCodepoint( + expMaps, + exp, + [](cfg::ExpQosMap& e) -> auto& { return *e.fromExpToTrafficClass(); }, + [](const cfg::ExpQosMap& e) { return e.fromTrafficClassToExp(); }); +} + +bool erasePcp(std::vector& pcpMaps, int16_t pcp) { + return eraseCodepoint( + pcpMaps, + pcp, + [](cfg::PcpQosMap& e) -> auto& { return *e.fromPcpToTrafficClass(); }, + [](const cfg::PcpQosMap& e) { return e.fromTrafficClassToPcp(); }); +} + +// Erases `key` from an optional map field. Returns false when the +// field is unset or the key is absent. A map left empty by the erase is +// reset, so the field returns to its unset default. +template +bool eraseOptionalMapKey(OptionalMapRef field, int16_t key) { + if (!field.has_value()) { + return false; + } + if (field->erase(key) == 0) { + return false; + } + if (field->empty()) { + field.reset(); + } + return true; +} + } // namespace CmdDeleteQosPolicyMapTraits::RetType CmdDeleteQosPolicyMap::queryClient( @@ -147,6 +215,39 @@ CmdDeleteQosPolicyMapTraits::RetType CmdDeleteQosPolicyMap::queryClient( name, entry.getKey()); } + case DeleteQosMapType::MPLS_EXP: { + if (!eraseExp(*qosMap.expMaps(), entry.getKey())) { + throw std::runtime_error( + fmt::format( + "QoS policy '{}' has no mpls-exp mapping for value {}", + name, + entry.getKey())); + } + session.saveConfig(); + return fmt::format( + "Successfully deleted QoS policy '{}' mpls-exp mapping for value {}", + name, + entry.getKey()); + } + case DeleteQosMapType::DOT1P: { + bool erased = qosMap.pcpMaps().has_value() && + erasePcp(*qosMap.pcpMaps(), entry.getKey()); + if (!erased) { + throw std::runtime_error( + fmt::format( + "QoS policy '{}' has no dot1p mapping for value {}", + name, + entry.getKey())); + } + if (qosMap.pcpMaps()->empty()) { + qosMap.pcpMaps().reset(); + } + session.saveConfig(); + return fmt::format( + "Successfully deleted QoS policy '{}' dot1p mapping for value {}", + name, + entry.getKey()); + } case DeleteQosMapType::TC_TO_QUEUE: { auto& tcToQueue = *qosMap.trafficClassToQueueId(); if (tcToQueue.erase(entry.getKey()) == 0) { @@ -162,6 +263,48 @@ CmdDeleteQosPolicyMapTraits::RetType CmdDeleteQosPolicyMap::queryClient( name, entry.getKey()); } + case DeleteQosMapType::PFC_PRI_TO_QUEUE: { + if (!eraseOptionalMapKey(qosMap.pfcPriorityToQueueId(), entry.getKey())) { + throw std::runtime_error( + fmt::format( + "QoS policy '{}' has no pfc-pri-to-queue mapping for priority {}", + name, + entry.getKey())); + } + session.saveConfig(); + return fmt::format( + "Successfully deleted QoS policy '{}' pfc-pri-to-queue mapping for priority {}", + name, + entry.getKey()); + } + case DeleteQosMapType::TC_TO_PG: { + if (!eraseOptionalMapKey(qosMap.trafficClassToPgId(), entry.getKey())) { + throw std::runtime_error( + fmt::format( + "QoS policy '{}' has no tc-to-pg mapping for traffic class {}", + name, + entry.getKey())); + } + session.saveConfig(); + return fmt::format( + "Successfully deleted QoS policy '{}' tc-to-pg mapping for traffic class {}", + name, + entry.getKey()); + } + case DeleteQosMapType::PFC_PRI_TO_PG: { + if (!eraseOptionalMapKey(qosMap.pfcPriorityToPgId(), entry.getKey())) { + throw std::runtime_error( + fmt::format( + "QoS policy '{}' has no pfc-pri-to-pg mapping for priority {}", + name, + entry.getKey())); + } + session.saveConfig(); + return fmt::format( + "Successfully deleted QoS policy '{}' pfc-pri-to-pg mapping for priority {}", + name, + entry.getKey()); + } } throw std::runtime_error("Unhandled map type"); diff --git a/fboss/cli/fboss2/commands/delete/qos/policy/CmdDeleteQosPolicyMap.h b/fboss/cli/fboss2/commands/delete/qos/policy/CmdDeleteQosPolicyMap.h index b82b003a3b54e..7e9285d137740 100644 --- a/fboss/cli/fboss2/commands/delete/qos/policy/CmdDeleteQosPolicyMap.h +++ b/fboss/cli/fboss2/commands/delete/qos/policy/CmdDeleteQosPolicyMap.h @@ -21,23 +21,32 @@ namespace facebook::fboss { /** - * Which QosMap a delete targets. Only the two map types the config command - * writes as individually addressable entries are removable today; the - * remaining types CmdConfigQosPolicyMap supports are rejected by name. + * Which QosMap a delete targets — every map type CmdConfigQosPolicyMap can + * write is removable, so config and delete cannot drift apart. */ enum class DeleteQosMapType { DSCP, // QosMap.dscpMaps + MPLS_EXP, // QosMap.expMaps + DOT1P, // QosMap.pcpMaps TC_TO_QUEUE, // QosMap.trafficClassToQueueId + PFC_PRI_TO_QUEUE, // QosMap.pfcPriorityToQueueId + TC_TO_PG, // QosMap.trafficClassToPgId + PFC_PRI_TO_PG, // QosMap.pfcPriorityToPgId }; /** * Parses the entry to remove: * - * dscp - drop a DSCP codepoint from the policy's dscpMaps - * tc-to-queue - drop a traffic class from trafficClassToQueueId + * dscp - drop a DSCP codepoint from the policy's dscpMaps + * mpls-exp - drop an EXP codepoint from expMaps + * dot1p - drop a PCP codepoint from pcpMaps + * tc-to-queue - drop a traffic class from trafficClassToQueueId + * pfc-pri-to-queue - drop a PFC priority from pfcPriorityToQueueId + * tc-to-pg - drop a traffic class from trafficClassToPgId + * pfc-pri-to-pg - drop a PFC priority from pfcPriorityToPgId * - * The value alone identifies the entry: a DSCP codepoint appears in at most - * one DscpQosMap, and a traffic class is a map key. + * The value alone identifies the entry: a codepoint appears in at most one + * ingress map entry, and the rest are map keys. */ class DeleteQosMapEntry : public utils::BaseObjectArgType { public: @@ -64,8 +73,13 @@ struct CmdDeleteQosPolicyMapTraits : public WriteCommandTraits { "map_entry", args, " where map-type is one of:\n" - " dscp remove a DSCP to traffic-class mapping\n" - " tc-to-queue remove a traffic-class to queue mapping") + " dscp remove a DSCP to traffic-class mapping\n" + " mpls-exp remove an EXP to traffic-class mapping\n" + " dot1p remove a PCP to traffic-class mapping\n" + " tc-to-queue remove a traffic-class to queue mapping\n" + " pfc-pri-to-queue remove a PFC priority to queue mapping\n" + " tc-to-pg remove a traffic-class to PG mapping\n" + " pfc-pri-to-pg remove a PFC priority to PG mapping") ->required() ->expected(2); } diff --git a/fboss/cli/fboss2/test/config/CmdDeleteQosPolicyTest.cpp b/fboss/cli/fboss2/test/config/CmdDeleteQosPolicyTest.cpp index 97c0403784307..ef3c357a8f568 100644 --- a/fboss/cli/fboss2/test/config/CmdDeleteQosPolicyTest.cpp +++ b/fboss/cli/fboss2/test/config/CmdDeleteQosPolicyTest.cpp @@ -29,8 +29,16 @@ static const std::string kSeedConfig = R"({ {"internalTrafficClass": 0, "fromDscpToTrafficClass": [0, 1, 2]}, {"internalTrafficClass": 1, "fromDscpToTrafficClass": [8], "fromTrafficClassToDscp": 8} ], - "expMaps": [], - "trafficClassToQueueId": {"0": 0, "1": 1, "2": 2} + "expMaps": [ + {"internalTrafficClass": 2, "fromExpToTrafficClass": [5]} + ], + "pcpMaps": [ + {"internalTrafficClass": 3, "fromPcpToTrafficClass": [6]} + ], + "trafficClassToQueueId": {"0": 0, "1": 1, "2": 2}, + "pfcPriorityToQueueId": {"3": 3}, + "trafficClassToPgId": {"4": 4, "5": 5}, + "pfcPriorityToPgId": {"6": 6} } }, { @@ -119,8 +127,7 @@ TEST_F(CmdDeleteQosPolicyTestFixture, mapEntryArgValidation) { EXPECT_THROW(DeleteQosMapEntry({"dscp"}), std::invalid_argument); EXPECT_THROW(DeleteQosMapEntry({"dscp", "1", "2"}), std::invalid_argument); // unknown map type - EXPECT_THROW( - DeleteQosMapEntry({"pfc-pri-to-pg", "1"}), std::invalid_argument); + EXPECT_THROW(DeleteQosMapEntry({"no-such-map", "1"}), std::invalid_argument); // non-integer value EXPECT_THROW(DeleteQosMapEntry({"dscp", "abc"}), std::invalid_argument); // dscp out of range @@ -272,6 +279,135 @@ TEST_F(CmdDeleteQosPolicyTestFixture, deleteMissingTcToQueueFails) { std::runtime_error); } +// ------------------------------------------------- delete map remaining kinds + +TEST_F(CmdDeleteQosPolicyTestFixture, deleteMplsExpDropsEntry) { + setupTestableConfigSession("delete qos policy map", "mpls-exp 5"); + + auto cmd = CmdDeleteQosPolicyMap(); + auto result = cmd.queryClient( + localhost(), + QosPolicyName({"unreferenced"}), + DeleteQosMapEntry({"mpls-exp", "5"})); + + EXPECT_THAT(result, ::testing::HasSubstr("Successfully deleted")); + // 5 was the entry's only codepoint and it has no egress rewrite, so the + // whole ExpQosMap entry is dropped. + const auto* policy = findPolicy("unreferenced"); + ASSERT_NE(policy, nullptr); + EXPECT_TRUE(policy->qosMap()->expMaps()->empty()); +} + +TEST_F(CmdDeleteQosPolicyTestFixture, deleteMissingMplsExpFails) { + setupTestableConfigSession("delete qos policy map", "mpls-exp 0"); + + auto cmd = CmdDeleteQosPolicyMap(); + EXPECT_THROW( + cmd.queryClient( + localhost(), + QosPolicyName({"unreferenced"}), + DeleteQosMapEntry({"mpls-exp", "0"})), + std::runtime_error); +} + +TEST_F(CmdDeleteQosPolicyTestFixture, deleteLastDot1pResetsPcpMaps) { + setupTestableConfigSession("delete qos policy map", "dot1p 6"); + + auto cmd = CmdDeleteQosPolicyMap(); + auto result = cmd.queryClient( + localhost(), + QosPolicyName({"unreferenced"}), + DeleteQosMapEntry({"dot1p", "6"})); + + EXPECT_THAT(result, ::testing::HasSubstr("Successfully deleted")); + // 6 was the only PCP codepoint; the emptied optional pcpMaps is reset. + const auto* policy = findPolicy("unreferenced"); + ASSERT_NE(policy, nullptr); + EXPECT_FALSE(policy->qosMap()->pcpMaps().has_value()); +} + +TEST_F(CmdDeleteQosPolicyTestFixture, deleteMissingDot1pFails) { + setupTestableConfigSession("delete qos policy map", "dot1p 0"); + + auto cmd = CmdDeleteQosPolicyMap(); + EXPECT_THROW( + cmd.queryClient( + localhost(), + QosPolicyName({"unreferenced"}), + DeleteQosMapEntry({"dot1p", "0"})), + std::runtime_error); +} + +TEST_F(CmdDeleteQosPolicyTestFixture, deleteLastPfcPriToQueueResetsMap) { + setupTestableConfigSession("delete qos policy map", "pfc-pri-to-queue 3"); + + auto cmd = CmdDeleteQosPolicyMap(); + auto result = cmd.queryClient( + localhost(), + QosPolicyName({"unreferenced"}), + DeleteQosMapEntry({"pfc-pri-to-queue", "3"})); + + EXPECT_THAT(result, ::testing::HasSubstr("Successfully deleted")); + // 3 was the only key; the emptied optional map is reset to unset. + const auto* policy = findPolicy("unreferenced"); + ASSERT_NE(policy, nullptr); + EXPECT_FALSE(policy->qosMap()->pfcPriorityToQueueId().has_value()); +} + +TEST_F(CmdDeleteQosPolicyTestFixture, deleteTcToPgKeepsRemainingKeys) { + setupTestableConfigSession("delete qos policy map", "tc-to-pg 4"); + + auto cmd = CmdDeleteQosPolicyMap(); + auto result = cmd.queryClient( + localhost(), + QosPolicyName({"unreferenced"}), + DeleteQosMapEntry({"tc-to-pg", "4"})); + + EXPECT_THAT(result, ::testing::HasSubstr("Successfully deleted")); + const auto* policy = findPolicy("unreferenced"); + ASSERT_NE(policy, nullptr); + const auto& tcToPg = policy->qosMap()->trafficClassToPgId(); + ASSERT_TRUE(tcToPg.has_value()); + EXPECT_EQ(tcToPg->count(4), 0); + EXPECT_EQ(tcToPg->at(5), 5); +} + +TEST_F(CmdDeleteQosPolicyTestFixture, deletePfcPriToPgOnUnsetKeyFails) { + setupTestableConfigSession("delete qos policy map", "pfc-pri-to-pg 0"); + + auto cmd = CmdDeleteQosPolicyMap(); + EXPECT_THROW( + cmd.queryClient( + localhost(), + QosPolicyName({"unreferenced"}), + DeleteQosMapEntry({"pfc-pri-to-pg", "0"})), + std::runtime_error); +} + +TEST_F(CmdDeleteQosPolicyTestFixture, deletePfcPriToPg) { + setupTestableConfigSession("delete qos policy map", "pfc-pri-to-pg 6"); + + auto cmd = CmdDeleteQosPolicyMap(); + auto result = cmd.queryClient( + localhost(), + QosPolicyName({"unreferenced"}), + DeleteQosMapEntry({"pfc-pri-to-pg", "6"})); + + EXPECT_THAT(result, ::testing::HasSubstr("Successfully deleted")); + const auto* policy = findPolicy("unreferenced"); + ASSERT_NE(policy, nullptr); + EXPECT_FALSE(policy->qosMap()->pfcPriorityToPgId().has_value()); +} + +TEST_F(CmdDeleteQosPolicyTestFixture, mapEntryValueRangeValidation) { + // Non-dscp kinds are 3-bit: 8 is out of range for all of them. + EXPECT_THROW(DeleteQosMapEntry({"dot1p", "8"}), std::invalid_argument); + EXPECT_THROW( + DeleteQosMapEntry({"pfc-pri-to-queue", "8"}), std::invalid_argument); + EXPECT_THROW(DeleteQosMapEntry({"tc-to-pg", "-1"}), std::invalid_argument); + EXPECT_THROW(DeleteQosMapEntry({"mpls-exp", "8"}), std::invalid_argument); +} + TEST_F(CmdDeleteQosPolicyTestFixture, mapDeleteOnMissingPolicyFails) { setupTestableConfigSession("delete qos policy map", "tc-to-queue 0"); From 42a6c937ddf6459dfbba0e6d0baae514cec097d1 Mon Sep 17 00:00:00 2001 From: Vybhav Date: Fri, 14 Aug 2026 09:44:10 +0000 Subject: [PATCH 3/3] Drop repeat-delete negative checks from the integration test Missing-entry and missing-policy errors are unit-tested; the DUT run sticks to the positive flow: create, dismantle entry by entry, remove. --- .../integration_test/DeleteQosPolicyTest.cpp | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/fboss/cli/fboss2/test/integration_test/DeleteQosPolicyTest.cpp b/fboss/cli/fboss2/test/integration_test/DeleteQosPolicyTest.cpp index 7a91b292e1da7..2a50cae45fe2f 100644 --- a/fboss/cli/fboss2/test/integration_test/DeleteQosPolicyTest.cpp +++ b/fboss/cli/fboss2/test/integration_test/DeleteQosPolicyTest.cpp @@ -27,14 +27,12 @@ #include #include -#include #include #include #include "fboss/cli/fboss2/test/integration_test/Fboss2IntegrationTest.h" using namespace facebook::fboss; -using ::testing::HasSubstr; class DeleteQosPolicyTest : public Fboss2IntegrationTest { protected: @@ -150,12 +148,7 @@ TEST_F(DeleteQosPolicyTest, DeleteMapEntriesThenPolicy) { EXPECT_TRUE(hasTcToQueue(config, kPolicy, kTrafficClass)) << "deleting the dscp mapping must not touch trafficClassToQueueId"; - // 3. Deleting the same dscp again is an error, not a silent success. - result = runCli({"delete", "qos", "policy", kPolicy, "map", "dscp", kDscp}); - EXPECT_NE(result.exitCode, 0); - EXPECT_THAT(result.stderr, HasSubstr("no dscp mapping")); - - // 4. Delete the tc-to-queue mapping. + // 3. Delete the tc-to-queue mapping. XLOG(INFO) << "Deleting tc-to-queue " << kTrafficClass << " from " << kPolicy; result = runCli( {"delete", @@ -173,7 +166,7 @@ TEST_F(DeleteQosPolicyTest, DeleteMapEntriesThenPolicy) { }); EXPECT_FALSE(hasTcToQueue(config, kPolicy, kTrafficClass)); - // 5. Delete the policy itself. Nothing references it, so this is allowed. + // 4. Delete the policy itself. Nothing references it, so this is allowed. XLOG(INFO) << "Deleting qos policy " << kPolicy; result = runCli({"delete", "qos", "policy", kPolicy}); ASSERT_EQ(result.exitCode, 0) @@ -182,9 +175,4 @@ TEST_F(DeleteQosPolicyTest, DeleteMapEntriesThenPolicy) { config = waitForRunningConfig( [&](const folly::dynamic& cfg) { return !hasPolicy(cfg, kPolicy); }); EXPECT_FALSE(hasPolicy(config, kPolicy)); - - // 6. Deleting it again reports that it does not exist. - result = runCli({"delete", "qos", "policy", kPolicy}); - EXPECT_NE(result.exitCode, 0); - EXPECT_THAT(result.stderr, HasSubstr("No QoS policy named")); }