Hi Adrian, I have some nitpicky comments below.

On 6/13/22 12:10, Adrian Moreno wrote:
Two new options are added to NB_Global table that enable drop
sampling by specifying the collector_set_id and the obs_domain_id of
the sample actions added to all drop flows.

For drops coming from an lflow, the sample has the following fields:
- obs_domain_id (32-bit): obs_domain_id << 8 | tunnel_key
   - 8 most significant bits: the obs_domain_id specified in the
     NB_Global options
   - 24 least significant bits: the tunnel_key
- obs_point_id: the cookie (first 32-bits of the lflow's UUID)

For drops that are inserted by ovn-controller without any associated
lflow, the sample will have the follwing fields:
- obs_domain_id (32-bit): obs_domain_id << 8
   - 8 most significant bits: the obs_domain_id specified in the
     NB_Global options
   - 24 least significant bits: 0
- obs_point_id: The table number

Signed-off-by: Adrian Moreno <[email protected]>
---
  controller/ovn-controller.c | 29 +++++++++---
  controller/physical.c       | 34 ++++++++++++--
  controller/physical.h       |  8 +++-
  northd/debug.c              | 93 +++++++++++++++++++++++++++++++++++--
  northd/debug.h              | 10 ++++
  northd/northd.c             | 73 +++++++++++++++--------------
  ovn-nb.xml                  | 24 ++++++++++
  tests/ovn.at                | 65 ++++++++++++++++----------
  8 files changed, 261 insertions(+), 75 deletions(-)

diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index 88e341593..0e0da362f 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -2942,7 +2942,7 @@ struct ed_type_pflow_output {
      /* Desired physical flows. */
      struct ovn_desired_flow_table flow_table;
      /* Drop debugging options. */
-    bool debug_drop;
+    struct physical_debug debug;
  };
static void init_physical_ctx(struct engine_node *node,
@@ -3013,8 +3013,15 @@ static void init_physical_ctx(struct engine_node *node,
      p_ctx->local_bindings = &rt_data->lbinding_data.bindings;
      p_ctx->patch_ofports = &non_vif_data->patch_ofports;
      p_ctx->chassis_tunnels = &non_vif_data->chassis_tunnels;
-    p_ctx->debug_drop = smap_get_bool(&sb_global->options,
+    p_ctx->debug.enabled = smap_get_bool(&sb_global->options,
                                        "debug_drop_mode", false);
+    p_ctx->debug.collector_set_id = smap_get_uint(&sb_global->options,
+                                                  "debug_drop_collector_set",
+                                                  0);
+
+    p_ctx->debug.obs_domain_id = smap_get_uint(&sb_global->options,
+                                               "debug_drop_domain_id",
+                                               0);
  }
static void *
@@ -3185,12 +3192,22 @@ pflow_output_sb_sb_global_handler(struct engine_node 
*node, void *data)
struct ed_type_pflow_output *pfo = data; - bool debug_drop = smap_get_bool(&sb_global->options,
+    bool debug_enabled = smap_get_bool(&sb_global->options,
                                         "debug_drop_mode", false);
-
-    if (pfo->debug_drop != debug_drop) {
+    uint32_t collector_set_id = smap_get_uint(&sb_global->options,
+                                              "debug_drop_collector_set",
+                                              0);
+    uint32_t obs_domain_id = smap_get_uint(&sb_global->options,
+                                           "debug_drop_domain_id",
+                                           0);
+
+    if (pfo->debug.enabled != debug_enabled ||
+        pfo->debug.collector_set_id != collector_set_id ||
+        pfo->debug.obs_domain_id != obs_domain_id) {
          engine_set_node_state(node, EN_UPDATED);
-        pfo->debug_drop = debug_drop;
+        pfo->debug.enabled = debug_enabled;
+        pfo->debug.collector_set_id = collector_set_id;
+        pfo->debug.obs_domain_id = obs_domain_id;
      }
      return true;
  }
diff --git a/controller/physical.c b/controller/physical.c
index 49f754af9..90b4260fd 100644
--- a/controller/physical.c
+++ b/controller/physical.c
@@ -823,25 +823,44 @@ put_zones_ofpacts(const struct zone_ids *zone_ids, struct 
ofpbuf *ofpacts_p)
      }
  }
+static void
+put_drop(const struct physical_debug *debug, uint8_t table_id,
+         struct ofpbuf *ofpacts)
+{
+    if (debug->collector_set_id) {
+        struct ofpact_sample *os = ofpact_put_SAMPLE(ofpacts);
+        os->probability = UINT16_MAX;
+        os->collector_set_id = debug->collector_set_id;
+        os->obs_domain_id = (debug->obs_domain_id << 24);
+        os->obs_point_id = table_id;
+    }
+}
+
  static void
  add_default_drop_flow(const struct physical_ctx *p_ctx,
                        uint8_t table_id,
                        struct ovn_desired_flow_table *flow_table)
  {
-    if (p_ctx->debug_drop) {
+    if (p_ctx->debug.enabled) {
          struct match match = MATCH_CATCHALL_INITIALIZER;
          struct ofpbuf ofpacts;
          ofpbuf_init(&ofpacts, 0);
+
+        put_drop(&p_ctx->debug, table_id, &ofpacts);
          ofctrl_add_flow(flow_table, table_id, 0, 0, &match,
                          &ofpacts, hc_uuid);
+
+        ofpbuf_uninit(&ofpacts);
      }
  }
+
  static void
  put_local_common_flows(uint32_t dp_key,
                         const struct sbrec_port_binding *pb,
                         const struct sbrec_port_binding *parent_pb,
                         const struct zone_ids *zone_ids,
+                       const struct physical_debug *debug,
                         struct ofpbuf *ofpacts_p,
                         struct ovn_desired_flow_table *flow_table)
  {
@@ -877,6 +896,7 @@ put_local_common_flows(uint32_t dp_key,
       * and the MLF_ALLOW_LOOPBACK flag is not set. */
      match_init_catchall(&match);
      ofpbuf_clear(ofpacts_p);
+    put_drop(debug, OFTABLE_CHECK_LOOPBACK, ofpacts_p);
      match_set_metadata(&match, htonll(dp_key));
      match_set_reg_masked(&match, MFF_LOG_FLAGS - MFF_REG0,
                           0, MLF_ALLOW_LOOPBACK);
@@ -1009,6 +1029,7 @@ consider_port_binding(struct ovsdb_idl_index 
*sbrec_port_binding_by_name,
                        const struct hmap *chassis_tunnels,
                        const struct sbrec_port_binding *binding,
                        const struct sbrec_chassis *chassis,
+                      const struct physical_debug *debug,
                        struct ovn_desired_flow_table *flow_table,
                        struct ofpbuf *ofpacts_p)
  {
@@ -1032,7 +1053,7 @@ consider_port_binding(struct ovsdb_idl_index 
*sbrec_port_binding_by_name,
struct zone_ids binding_zones = get_zone_ids(binding, ct_zones);
          put_local_common_flows(dp_key, binding, NULL, &binding_zones,
-                               ofpacts_p, flow_table);
+                               debug, ofpacts_p, flow_table);
ofpbuf_clear(ofpacts_p);
          match_outport_dp_and_port_keys(&match, dp_key, port_key);
@@ -1208,7 +1229,7 @@ consider_port_binding(struct ovsdb_idl_index 
*sbrec_port_binding_by_name,
          /* Pass the parent port binding if the port is a nested
           * container. */
          put_local_common_flows(dp_key, binding, parent_port, &zone_ids,
-                               ofpacts_p, flow_table);
+                               debug, ofpacts_p, flow_table);
/* Table 0, Priority 150 and 100.
           * ==============================
@@ -1337,6 +1358,7 @@ consider_port_binding(struct ovsdb_idl_index 
*sbrec_port_binding_by_name,
/* Drop LOCAL_ONLY traffic leaking through localnet ports. */
              ofpbuf_clear(ofpacts_p);
+            put_drop(debug, OFTABLE_CHECK_LOOPBACK, ofpacts_p);
              match_outport_dp_and_port_keys(&match, dp_key, port_key);
              match_set_reg_masked(&match, MFF_LOG_FLAGS - MFF_REG0,
                                   MLF_LOCAL_ONLY, MLF_LOCAL_ONLY);
@@ -1731,7 +1753,8 @@ physical_eval_port_binding(struct physical_ctx *p_ctx,
                            p_ctx->local_bindings,
                            p_ctx->patch_ofports,
                            p_ctx->chassis_tunnels,
-                          pb, p_ctx->chassis, flow_table, &ofpacts);
+                          pb, p_ctx->chassis, &p_ctx->debug,
+                          flow_table, &ofpacts);
      ofpbuf_uninit(&ofpacts);
  }
@@ -1825,7 +1848,8 @@ physical_run(struct physical_ctx *p_ctx,
                                p_ctx->local_bindings,
                                p_ctx->patch_ofports,
                                p_ctx->chassis_tunnels, binding,
-                              p_ctx->chassis, flow_table, &ofpacts);
+                              p_ctx->chassis, &p_ctx->debug,
+                              flow_table, &ofpacts);
      }
/* Handle output to multicast groups, in tables 37 and 38. */
diff --git a/controller/physical.h b/controller/physical.h
index b96641a6f..0830743d7 100644
--- a/controller/physical.h
+++ b/controller/physical.h
@@ -43,6 +43,12 @@ struct local_nonvif_data;
  #define OVN_GENEVE_TYPE 0x80     /* Critical option. */
  #define OVN_GENEVE_LEN 4
+struct physical_debug {
+    bool enabled;
+    uint32_t collector_set_id;
+    uint32_t obs_domain_id;
+};
+
  struct physical_ctx {
      struct ovsdb_idl_index *sbrec_port_binding_by_name;
      const struct sbrec_port_binding_table *port_binding_table;
@@ -58,7 +64,7 @@ struct physical_ctx {
      struct shash *local_bindings;
      struct simap *patch_ofports;
      struct hmap *chassis_tunnels;
-    bool debug_drop;
+    struct physical_debug debug;
  };
void physical_register_ovs_idl(struct ovsdb_idl *);
diff --git a/northd/debug.c b/northd/debug.c
index 3db0a3c4f..bf47919a9 100644
--- a/northd/debug.c
+++ b/northd/debug.c
@@ -4,20 +4,105 @@
#include "debug.h" +#include "openvswitch/dynamic-string.h"
+#include "openvswitch/vlog.h"
  #include "smap.h"
+VLOG_DEFINE_THIS_MODULE(debug)
+
  static struct debug_config config;
+bool
+debug_enabled(void)
+{
+    return config.enabled;
+}
+
+bool debug_sampling_enabled(void)
+{
+    return config.collector_set_id != 0;
+}
+
  void
  init_debug_config(const struct nbrec_nb_global *nb)
  {
const struct smap *options = &nb->options;
-    config.enabled = smap_get_bool(options, "debug_drop_mode", false);
+    bool enabled = smap_get_bool(options, "debug_drop_mode", false);
+    uint32_t collector_set_id = smap_get_uint(options,
+                                              "debug_drop_collector_set",
+                                              0);
+
+    uint32_t observation_domain_id = smap_get_uint(options,
+                                                   "debug_drop_domain_id",
+                                                   0);
+
+    if (enabled != config.enabled ||
+        collector_set_id != config.collector_set_id ||
+        observation_domain_id != config.observation_domain_id ||
+        !config.drop_action.string) {
+
+        if (observation_domain_id >= UINT8_MAX) {
+            VLOG_ERR("Observation domain id must be an 8-bit number");
+            return;
+        }
+
+        if (!enabled && collector_set_id) {
+            VLOG_WARN("Debug collection set configured, "
+                      "assuming debug_drop_mode");
+            enabled = true;
+        }
+
+        config.enabled = enabled;
+        config.collector_set_id = collector_set_id;
+        config.observation_domain_id = observation_domain_id;
+
+        ds_clear(&config.drop_action);
+
+        VLOG_INFO("Debug drop mode: %s", debug_enabled() ? "enabled" :
+                                                           "disabled");
+        if (debug_sampling_enabled()) {
+            ds_put_format(&config.drop_action,
+                          "sample(probability=65535,"
+                          "collector_set=%d,"
+                          "obs_domain=%d,"
+                          "obs_point=$cookie); ",
+                          config.collector_set_id,
+                          config.observation_domain_id);
+
+            ds_put_format(&config.drop_action, "/* drop */");

nit: Use ds_put_cstr() here since there are no %-style formatters in the string.

+            VLOG_INFO("Debug drop sampling: enabled");
+        } else {
+            ds_put_format(&config.drop_action, "drop;");

nit: Use ds_put_cstr() here as well.

+            VLOG_INFO("Debug drop sampling: disabled");
+        }
+    }
  }
-bool
-debug_enabled(void)
+void
+destroy_debug_config(void)
  {
-    return config.enabled;
+    if (config.drop_action.string) {
+        ds_destroy(&config.drop_action);
+        ds_init(&config.drop_action);
+    }
+}
+
+const char *
+debug_drop_action(void) {
+    if (OVS_UNLIKELY(debug_sampling_enabled())) {
+        return ds_cstr_ro(&config.drop_action);
+    } else {
+        return "drop;";
+    }
+}
+
+const char *
+debug_implicit_drop_action(void)
+{
+    if (OVS_UNLIKELY(debug_sampling_enabled())) {
+        return ds_cstr_ro(&config.drop_action);
+    } else {
+        return "/* drop */";
+    }
  }
diff --git a/northd/debug.h b/northd/debug.h
index 69d4da171..9a1c02986 100644
--- a/northd/debug.h
+++ b/northd/debug.h
@@ -19,13 +19,23 @@
  #include <stdbool.h>
#include "lib/ovn-nb-idl.h"
+#include "openvswitch/dynamic-string.h"
struct debug_config {
      bool enabled;
+    uint32_t collector_set_id;
+    uint32_t observation_domain_id;
+    struct ds drop_action;
  };
void init_debug_config(const struct nbrec_nb_global *nb);
+void destroy_debug_config(void);
bool debug_enabled(void);
+bool debug_sampling_enabled(void);
+
+const char *debug_drop_action(void);
+const char *debug_implicit_drop_action(void);
+const char *debug_reject_action(void);
#endif /* NORTHD_DEBUG_H */
diff --git a/northd/northd.c b/northd/northd.c
index 2fde9fb71..12c9975b9 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -3903,7 +3903,7 @@ build_lb_vip_actions(struct ovn_lb_vip *lb_vip,
          if (!n_active_backends) {
              if (!lb_vip->empty_backend_rej) {
                  ds_clear(action);
-                ds_put_cstr(action, "drop;");
+                ds_put_cstr(action, debug_drop_action());
                  skip_hash_fields = true;
              } else {
                  reject = true;
@@ -5052,7 +5052,7 @@ __ovn_lflow_add_default_drop(struct hmap *lflow_map,
                               const char *where)
  {
      if (OVS_UNLIKELY(debug_enabled())) {
-        ovn_lflow_add_at(lflow_map, od, stage, 0, "1", "drop;",
+        ovn_lflow_add_at(lflow_map, od, stage, 0, "1", debug_drop_action(),
                           NULL, NULL, NULL, where );
      }
  }
@@ -6315,7 +6315,7 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od,
              } else {
                  ds_put_format(match, " && (%s)", acl->match);
                  build_acl_log(actions, acl, meter_groups);
-                ds_put_cstr(actions, "/* drop */");
+                ds_put_cstr(actions, debug_implicit_drop_action());
                  ovn_lflow_add_with_hint(lflows, od, stage,
                                          acl->priority + OVN_ACL_PRI_OFFSET,
                                          ds_cstr(match), ds_cstr(actions),
@@ -6343,7 +6343,7 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od,
              } else {
                  ds_put_format(match, " && (%s)", acl->match);
                  build_acl_log(actions, acl, meter_groups);
-                ds_put_cstr(actions, "/* drop */");
+                ds_put_cstr(actions, debug_implicit_drop_action());
                  ovn_lflow_add_with_hint(lflows, od, stage,
                                          acl->priority + OVN_ACL_PRI_OFFSET,
                                          ds_cstr(match), ds_cstr(actions),
@@ -6360,7 +6360,7 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od,
                                         actions, &acl->header_, meter_groups);
              } else {
                  build_acl_log(actions, acl, meter_groups);
-                ds_put_cstr(actions, "/* drop */");
+                ds_put_cstr(actions, debug_implicit_drop_action());
                  ovn_lflow_add_with_hint(lflows, od, stage,
                                          acl->priority + OVN_ACL_PRI_OFFSET,
                                          acl->match, ds_cstr(actions),
@@ -6596,9 +6596,9 @@ build_acls(struct ovn_datapath *od, const struct 
chassis_features *features,
                        use_ct_inv_match ? "ct.inv || " : "",
                        ct_blocked_match);
          ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, UINT16_MAX - 3,
-                      ds_cstr(&match), "drop;");
+                      ds_cstr(&match), debug_drop_action());
          ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, UINT16_MAX - 3,
-                      ds_cstr(&match), "drop;");
+                      ds_cstr(&match),  debug_drop_action());
/* Ingress and Egress ACL Table (Priority 65535 - 3).
           *
@@ -7642,7 +7642,7 @@ build_drop_arp_nd_flows_for_unbound_router_ports(struct 
ovn_port *op,
                          rp->lsp_addrs[k].ipv4_addrs[l].addr_s);
                      ovn_lflow_add_with_lport_and_hint(
                          lflows, op->od, S_SWITCH_IN_EXTERNAL_PORT, 100,
-                        ds_cstr(&match), "drop;", port->key,
+                        ds_cstr(&match),  debug_drop_action(), port->key,
                          &op->nbsp->header_);
                  }
                  for (size_t l = 0; l < rp->lsp_addrs[k].n_ipv6_addrs; l++) {
@@ -7658,7 +7658,7 @@ build_drop_arp_nd_flows_for_unbound_router_ports(struct 
ovn_port *op,
                          rp->lsp_addrs[k].ipv6_addrs[l].addr_s);
                      ovn_lflow_add_with_lport_and_hint(
                          lflows, op->od, S_SWITCH_IN_EXTERNAL_PORT, 100,
-                        ds_cstr(&match), "drop;", port->key,
+                        ds_cstr(&match), debug_drop_action(), port->key,
                          &op->nbsp->header_);
                  }
@@ -7673,7 +7673,8 @@ build_drop_arp_nd_flows_for_unbound_router_ports(struct ovn_port *op,
                  ovn_lflow_add_with_lport_and_hint(lflows, op->od,
                                                    S_SWITCH_IN_EXTERNAL_PORT,
                                                    100, ds_cstr(&match),
-                                                  "drop;", port->key,
+                                                  debug_drop_action(),
+                                                  port->key,
                                                    &op->nbsp->header_);
              }
          }
@@ -7711,7 +7712,7 @@ build_lswitch_flows(const struct hmap *datapaths,
                            "outport = \""MC_UNKNOWN "\"; output;");
          } else {
              ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_UNKNOWN, 50,
-                          "outport == \"none\"", "drop;");
+                          "outport == \"none\"",  debug_drop_action());
          }
          ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_UNKNOWN, 0, "1",
                        "output;");
@@ -7754,18 +7755,18 @@ build_lswitch_lflows_admission_control(struct 
ovn_datapath *od,
          if (!is_vlan_transparent(od)) {
              /* Block logical VLANs. */
              ovn_lflow_add(lflows, od, S_SWITCH_IN_CHECK_PORT_SEC, 100,
-                          "vlan.present", "drop;");
+                          "vlan.present", debug_drop_action());
          }
/* Broadcast/multicast source address is invalid. */
          ovn_lflow_add(lflows, od, S_SWITCH_IN_CHECK_PORT_SEC, 100,
-                      "eth.src[40]", "drop;");
+                      "eth.src[40]", debug_drop_action());
ovn_lflow_add(lflows, od, S_SWITCH_IN_CHECK_PORT_SEC, 50, "1",
                        REGBIT_PORT_SEC_DROP" = check_in_port_sec(); next;");
ovn_lflow_add(lflows, od, S_SWITCH_IN_APPLY_PORT_SEC, 50,
-                      REGBIT_PORT_SEC_DROP" == 1", "drop;");
+                      REGBIT_PORT_SEC_DROP" == 1", debug_drop_action());
ovn_lflow_add(lflows, od, S_SWITCH_IN_APPLY_PORT_SEC, 0, "1", "next;");
          /* Port security flows have priority 50
@@ -8304,7 +8305,7 @@ build_lswitch_destination_lookup_bmcast(struct 
ovn_datapath *od,
                   */
                  if (!mcast_sw_info->flood_relay &&
                          !mcast_sw_info->flood_static) {
-                    ds_put_cstr(actions, "drop;");
+                    ds_put_cstr(actions, debug_drop_action());
                  }
ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_LKUP, 80,
@@ -8824,7 +8825,7 @@ build_routing_policy_flow(struct hmap *lflows, struct 
ovn_datapath *od,
                        out_port->json_key);
} else if (!strcmp(rule->action, "drop")) {
-        ds_put_cstr(&actions, "drop;");
+        ds_put_cstr(&actions, debug_drop_action());
      } else if (!strcmp(rule->action, "allow")) {
          uint32_t pkt_mark = ovn_smap_get_uint(&rule->options, "pkt_mark", 0);
          if (pkt_mark) {
@@ -9596,7 +9597,7 @@ add_route(struct hmap *lflows, struct ovn_datapath *od,
      struct ds common_actions = DS_EMPTY_INITIALIZER;
      struct ds actions = DS_EMPTY_INITIALIZER;
      if (is_discard_route) {
-        ds_put_format(&actions, "drop;");
+        ds_put_cstr(&actions, debug_drop_action());
      } else {
          ds_put_format(&common_actions, REG_ECMP_GROUP_ID" = 0; %s = ",
                        is_ipv4 ? REG_NEXT_HOP_IPV4 : REG_NEXT_HOP_IPV6);
@@ -10379,7 +10380,7 @@ build_lrouter_arp_flow(struct ovn_datapath *od, struct 
ovn_port *op,
          ds_put_format(&match, " && %s", ds_cstr(extra_match));
      }
      if (drop) {
-        ds_put_format(&actions, "drop;");
+        ds_put_cstr(&actions, debug_drop_action());
      } else {
          ds_put_format(&actions,
                        "eth.dst = eth.src; "
@@ -10435,7 +10436,7 @@ build_lrouter_nd_flow(struct ovn_datapath *od, struct 
ovn_port *op,
      }
if (drop) {
-        ds_put_format(&actions, "drop;");
+        ds_put_cstr(&actions, debug_drop_action());
          ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_IP_INPUT, priority,
                                  ds_cstr(&match), ds_cstr(&actions), hint);
      } else {
@@ -10582,7 +10583,7 @@ build_lrouter_drop_own_dest(struct ovn_port *op, enum 
ovn_stage stage,
char *match = xasprintf("ip4.dst == {%s}", ds_cstr(&match_ips));
              ovn_lflow_add_with_hint(lflows, op->od, stage, priority,
-                                    match, "drop;",
+                                    match, debug_drop_action(),
                                      &op->nbrp->header_);
              free(match);
          }
@@ -10608,7 +10609,7 @@ build_lrouter_drop_own_dest(struct ovn_port *op, enum 
ovn_stage stage,
char *match = xasprintf("ip6.dst == {%s}", ds_cstr(&match_ips));
              ovn_lflow_add_with_hint(lflows, op->od, stage, priority,
-                                    match, "drop;",
+                                    match, debug_drop_action(),
                                      &op->nbrp->header_);
              free(match);
          }
@@ -10776,7 +10777,7 @@ build_adm_ctrl_flows_for_lrouter(
          /* Logical VLANs not supported.
           * Broadcast/multicast source address is invalid. */
          ovn_lflow_add(lflows, od, S_ROUTER_IN_ADMISSION, 100,
-                      "vlan.present || eth.src[40]", "drop;");
+                      "vlan.present || eth.src[40]", debug_drop_action());
/* Default action for L2 security is to drop. */
          ovn_lflow_add_default_drop(lflows, od, S_ROUTER_IN_ADMISSION);
@@ -11352,7 +11353,7 @@ build_mcast_lookup_flows_for_lrouter(
           * i.e., router solicitation and router advertisement.
           */
          ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_ROUTING, 10550,
-                      "nd_rs || nd_ra", "drop;");
+                      "nd_rs || nd_ra", debug_drop_action());
          if (!od->mcast_info.rtr.relay) {
              return;
          }
@@ -11399,13 +11400,13 @@ build_mcast_lookup_flows_for_lrouter(
                  ds_put_format(match, "eth.src == %s && igmp",
                                op->lrp_networks.ea_s);
                  ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_ROUTING, 10550,
-                              ds_cstr(match), "drop;");
+                              ds_cstr(match), debug_drop_action());
ds_clear(match);
                  ds_put_format(match, "eth.src == %s && (mldv1 || mldv2)",
                                op->lrp_networks.ea_s);
                  ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_ROUTING, 10550,
-                              ds_cstr(match), "drop;");
+                              ds_cstr(match), debug_drop_action());
              }
ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_ROUTING, 10460,
@@ -11429,7 +11430,7 @@ build_mcast_lookup_flows_for_lrouter(
                            "};");
          } else {
              ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_ROUTING, 10450,
-                          "ip4.mcast || ip6.mcast", "drop;");
+                          "ip4.mcast || ip6.mcast", debug_drop_action());
          }
      }
  }
@@ -12234,7 +12235,7 @@ build_misc_local_traffic_drop_flows_for_lrouter(
                        "ip4.dst == 127.0.0.0/8 || "
                        "ip4.src == 0.0.0.0/8 || "
                        "ip4.dst == 0.0.0.0/8",
-                      "drop;");
+                      debug_drop_action());
/* Drop ARP packets (priority 85). ARP request packets for router's own
           * IPs are handled with priority-90 flows.
@@ -12242,7 +12243,7 @@ build_misc_local_traffic_drop_flows_for_lrouter(
           * IPs are handled with priority-90 flows.
           */
          ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_INPUT, 85,
-                      "arp || nd", "drop;");
+                      "arp || nd", debug_drop_action());
/* Allow IPv6 multicast traffic that's supposed to reach the
           * router pipeline (e.g., router solicitations).
@@ -12252,21 +12253,22 @@ build_misc_local_traffic_drop_flows_for_lrouter(
/* Drop other reserved multicast. */
          ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_INPUT, 83,
-                      "ip6.mcast_rsvd", "drop;");
+                      "ip6.mcast_rsvd", debug_drop_action());
/* Allow other multicast if relay enabled (priority 82). */
          ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_INPUT, 82,
                        "ip4.mcast || ip6.mcast",
-                      od->mcast_info.rtr.relay ? "next;" : "drop;");
+                      (od->mcast_info.rtr.relay ? "next;" :
+                                                  debug_drop_action()));
/* Drop Ethernet local broadcast. By definition this traffic should
           * not be forwarded.*/
          ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_INPUT, 50,
-                      "eth.bcast", "drop;");
+                      "eth.bcast", debug_drop_action());
/* TTL discard */
          ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_INPUT, 30,
-                      "ip4 && ip.ttl == {0, 1}", "drop;");
+                      "ip4 && ip.ttl == {0, 1}", debug_drop_action());
/* Pass other traffic not already handled to the next table for
           * routing. */
@@ -12528,7 +12530,7 @@ build_lrouter_ipv4_ip_input(struct ovn_port *op,
              op_put_v4_networks(match, op, true);
              ds_put_cstr(match, " && "REGBIT_EGRESS_LOOPBACK" == 0");
              ovn_lflow_add_with_hint(lflows, op->od, S_ROUTER_IN_IP_INPUT, 100,
-                                    ds_cstr(match), "drop;",
+                                    ds_cstr(match), debug_drop_action(),
                                      &op->nbrp->header_);
/* ICMP echo reply. These flows reply to ICMP echo requests
@@ -13562,8 +13564,8 @@ build_lrouter_nat_defrag_and_lb(struct ovn_datapath 
*od, struct hmap *lflows,
              struct ovn_port *op = ovn_port_find(ports, nat->logical_port);
              if (op && op->nbsp && !strcmp(op->nbsp->type, "virtual")) {
                  ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_GW_REDIRECT,
-                                        80, ds_cstr(match), "drop;",
-                                        &nat->header_);
+                                        80, ds_cstr(match),
+                                        debug_drop_action(), &nat->header_);
              }
              ds_put_format(match, " && is_chassis_resident(\"%s\")",
                            nat->logical_port);
@@ -15311,6 +15313,7 @@ northd_destroy(struct northd_data *data)
destroy_datapaths_and_ports(&data->datapaths, &data->ports,
                                  &data->lr_list);
+    destroy_debug_config();
  }
static void
diff --git a/ovn-nb.xml b/ovn-nb.xml
index 360fa96df..80cbc2e3e 100644
--- a/ovn-nb.xml
+++ b/ovn-nb.xml
@@ -271,6 +271,30 @@
          </p>
        </column>
+ <column name="options" key="debug_drop_domain_id">
+        <p>
+          If set to a 8-bit number and if
+          <code>debug_drop_collection_set</code> is also configured,
+          <code>ovn-northd</code> will add a <code>sample</code> action to
+          every logical flow that contains a 'drop' action.
+          The 8 most significant bits of the observation_domain_id field will
+          be those of the specified in the

s/of the//

+          <code> debug_drop_domain_id</code>.
+          The 24 least significant bits of the observation_domain_id field will
+          be those of the datapath's tunnel key.

s/those of//

+        </p>
+      </column>
+
+      <column name="options" key="debug_drop_collection_set">
+        <p>
+          If set to a 32-bit number <code>ovn-northd</code> will add a
+          <code>sample</code> action to every logical flow that contains a
+          'drop' action. The sample action will have the specified
+          collection_set_id. The value must match that of the local OVS
+          configuration as described in ovs-actions(7).
+        </p>
+      </column>
+
        <group title="Options for configuring interconnection route 
advertisement">
          <p>
            These options control how routes are advertised between OVN
diff --git a/tests/ovn.at b/tests/ovn.at
index 1466ee492..67e90b6a8 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -32064,6 +32064,41 @@ check_default_flows() {
      done
  }
+check_sample_drops() {
+    check ovn-nbctl -- remove NB_Global . options debug_drop_collector_set \
+                    -- remove NB_Global . options debug_drop_domain_id
+    check ovn-nbctl --wait=hv sync
+
+    ovs-ofctl dump-flows --no-stats br-int > oflows_nosample
+    AT_CAPTURE_FILE([oflows_nosample])
+    # Take match part of flows that contain "drop".
+    drop_matches="$(grep 'drop' oflows_nosample | grep -oP 'table=\d*, priority=.* 
')"
+
+    check ovn-nbctl -- set NB_Global . options:debug_drop_collector_set="123" \
+                    -- set NB_Global . options:debug_drop_domain_id="1"
+    check ovn-nbctl --wait=hv sync
+
+    ovs-ofctl dump-flows --no-stats br-int > oflows_sample
+    AT_CAPTURE_FILE([oflows_sample])
+
+    # Check that every drop has now contains a "sample" action.
+    for flow in "$drop_matches"; do
+        AT_CHECK([grep -q "$flow actions=.*sample.*" oflows_sample], [0], [ignore], 
[ignore], [echo "Flow $flow has a drop and did not get sampled"])
+    done
+}
+
+check_debug() {
+    as hv1
+    check_default_flows
+    as hv2
+    check_default_flows
+
+    as hv1
+    check_sample_drops
+    as hv2
+    check_sample_drops
+}
+
  check ovn-nbctl -- set NB_Global . options:debug_drop_mode="true"
# Logical network:
@@ -32131,10 +32166,7 @@ ovs-vsctl -- add-port br-int hv2-vif1 -- \
  wait_for_ports_up
  check ovn-nbctl --wait=hv sync
-as hv1
-check_default_flows
-as hv2
-check_default_flows
+check_debug
# Add stateless ACL
  check ovn-nbctl --wait=sb \
@@ -32142,10 +32174,7 @@ check ovn-nbctl --wait=sb \
  check ovn-nbctl --wait=sb \
                  -- acl-add ls2 from-lport 100 'ip4' allow-stateless
-as hv1
-check_default_flows
-as hv2
-check_default_flows
+check_debug
check ovn-nbctl --wait=sb acl-del ls1
  check ovn-nbctl --wait=sb acl-del ls2
@@ -32156,10 +32185,7 @@ check ovn-nbctl --wait=sb \
  check ovn-nbctl --wait=sb \
                  -- acl-add ls2 from-lport 100 "udp" allow-related
-as hv1
-check_default_flows
-as hv2
-check_default_flows
+check_debug
check ovn-nbctl --wait=sb acl-del ls1
  check ovn-nbctl --wait=sb acl-del ls2
@@ -32173,10 +32199,7 @@ check ovn-nbctl --wait=sb \
      -- lb-add lb2 "10.0.1.1" "10.0.1.2" \
      -- ls-lb-add ls2 lb2
-as hv1
-check_default_flows
-as hv2
-check_default_flows
+check_debug
# LB + stateless ACL
  check ovn-nbctl --wait=sb \
@@ -32184,10 +32207,7 @@ check ovn-nbctl --wait=sb \
  check ovn-nbctl --wait=sb \
                  -- acl-add ls2 from-lport 100 'ip4' allow-stateless
-as hv1
-check_default_flows
-as hv2
-check_default_flows
+check_debug
check ovn-nbctl --wait=sb acl-del ls1
  check ovn-nbctl --wait=sb acl-del ls2
@@ -32198,10 +32218,7 @@ check ovn-nbctl --wait=sb \
  check ovn-nbctl --wait=sb \
                  -- acl-add ls2 from-lport 100 "udp" allow-related
-as hv1
-check_default_flows
-as hv2
-check_default_flows
+check_debug
OVN_CLEANUP([hv1],[hv2])
  AT_CLEANUP



_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to