[Nexthop][fboss2-dev] Add delete qos policy subcommands

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

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

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

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",

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<CmdDeleteQosQueueConfigQueueId>,
                    argRegistrar<CmdDeleteQosQueueConfigQueueIdTraits>,
                }},
+           },
+           {
+               "policy",
+               "Delete a QoS policy or one of its map entries",
+               commandHandler<CmdDeleteQosPolicy>,
+               argRegistrar<CmdDeleteQosPolicyTraits>,
+               {{
+                   "map",
+                   "Remove a QoS map entry (dscp, tc-to-queue)",
+                   commandHandler<CmdDeleteQosPolicyMap>,
+                   argRegistrar<CmdDeleteQosPolicyMapTraits>,
+               }},
            }},
       },
 

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 <fmt/format.h>
+#include <algorithm>
+#include <stdexcept>
+
+namespace facebook::fboss::utils {
+
+std::vector<cfg::QosPolicy>::iterator findQosPolicy(
+    std::vector<cfg::QosPolicy>& qosPolicies,
+    const std::string& name) {
+  return std::find_if(
+      qosPolicies.begin(), qosPolicies.end(), [&name](const auto& policy) {
+        return *policy.name() == name;
+      });
+}
+
+std::vector<cfg::QosPolicy>::iterator findQosPolicyOrThrow(
+    std::vector<cfg::QosPolicy>& 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

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 <string>
+#include <vector>
+
+#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<cfg::QosPolicy>::iterator findQosPolicy(
+    std::vector<cfg::QosPolicy>& qosPolicies,
+    const std::string& name);
+
+// Like findQosPolicy, but throws std::runtime_error with a uniform
+// "No QoS policy named '<name>' exists" message when absent, so every delete
+// command reports the same error instead of repeating the find-then-throw.
+std::vector<cfg::QosPolicy>::iterator findQosPolicyOrThrow(
+    std::vector<cfg::QosPolicy>& qosPolicies,
+    const std::string& name);
+
+} // namespace facebook::fboss::utils

fboss/cli/fboss2/commands/config/qos/policy/CmdConfigQosPolicyMap.cpp

@@ -25,6 +25,7 @@
 #include <vector>
 
 #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) {

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 <fmt/format.h>
+#include <folly/String.h>
+#include <iostream>
+#include <stdexcept>
+#include <string>
+#include <vector>
+
+#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<std::string>& 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<std::string> findReferences(
+    const cfg::SwitchConfig& switchConfig,
+    const std::string& name) {
+  std::vector<std::string> 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<CmdDeleteQosPolicy, CmdDeleteQosPolicyTraits>::run();
+
+} // namespace facebook::fboss