[ovs-dev,v1,3/3] northd: add drop sampling

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 <amorenoz@redhat.com>
---
 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(-)

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 <amorenoz@redhat.com>
> ---
>   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
>

On 6/16/22 22:32, Mark Michelson wrote:
> Hi Adrian, I have some nitpicky comments below.
> 

Thanks Mark, I'll addresss them in the next version of this series.

> 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 <amorenoz@redhat.com>
>> ---
>>   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
>>
>