Skip to content

Commit

Permalink
Text representations for drop sampling.
Browse files Browse the repository at this point in the history 
Created a new column in the southbound database to hardcode a human readable
description for flows. This first use is describing why the flow is dropping packets.
The new column is called flow_desc and will create southbound database entries like this

_uuid               : 20f1897b-477e-47ae-a32c-c546d83ec097
actions             : "sample(probability=65535,collector_set=123,obs_domain=1,obs_point=$cookie); /* drop */"
controller_meter    : []
external_ids        : {source="northd.c:8721", stage-name=ls_in_l2_unknown}
flow_desc           : "No L2 destination"
logical_datapath    : []
logical_dp_group    : ee3c3db5-98a2-4f34-8a84-409deae140a7
match               : "outport == \"none\""
pipeline            : ingress
priority            : 50
table_id            : 27
tags                : {}
hash                : 0

future work includes entering more flow_desc for more flows and adding
flow_desc to the actions as a comment.

Signed-off-by: Jacob Tanenbaum <[email protected]>
Suggested-by: Dumitru Ceara <[email protected]>
Reported-at: https://issues.redhat.com/browse/FDP-307
Acked-by: Ales Musil <[email protected]>
Acked-by: Dumitru Ceara <[email protected]>
Signed-off-by: Numan Siddique <[email protected]>
  • Loading branch information
@JacobTanenbaum @numansiddique
JacobTanenbaum authored and numansiddique committed Aug 8, 2024
1 parent 03862b9 commit 436a011
Show file tree
Hide file tree
Showing 7 changed files with 103 additions and 49 deletions.
2 changes: 2 additions & 0 deletions NEWS
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ Post v24.03.0
- Add "external_ids:ovn-encap-ip-default" config for ovn-controller to
determine the default encap IP when there are multiple encap IPs
configured.
- Added a new column in the southbound database "flow_desc" to provide
human readable context to flows.

OVN v24.03.0 - 01 Mar 2024
--------------------------
Expand Down
22 changes: 14 additions & 8 deletions northd/lflow-mgr.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
#include "debug.h"
#include "lflow-mgr.h"
#include "lib/ovn-parallel-hmap.h"
#include "lib/ovn-util.h"

VLOG_DEFINE_THIS_MODULE(lflow_mgr);

Expand All @@ -36,7 +37,7 @@ static void ovn_lflow_init(struct ovn_lflow *, struct ovn_datapath *od,
uint16_t priority, char *match,
char *actions, char *io_port,
char *ctrl_meter, char *stage_hint,
const char *where);
const char *where, const char *flow_desc);
static struct ovn_lflow *ovn_lflow_find(const struct hmap *lflows,
enum ovn_stage stage,
uint16_t priority, const char *match,
Expand All @@ -52,7 +53,7 @@ static struct ovn_lflow *do_ovn_lflow_add(
const char *actions, const char *io_port,
const char *ctrl_meter,
const struct ovsdb_idl_row *stage_hint,
const char *where);
const char *where, const char *flow_desc);


static struct ovs_mutex *lflow_hash_lock(const struct hmap *lflow_table,
Expand Down Expand Up @@ -173,6 +174,7 @@ struct ovn_lflow {
* 'dpg_bitmap'. */
struct ovn_dp_group *dpg; /* Link to unique Sb datapath group. */
const char *where;
const char *flow_desc;

struct uuid sb_uuid; /* SB DB row uuid, specified by northd. */
struct ovs_list referenced_by; /* List of struct lflow_ref_node. */
Expand Down Expand Up @@ -659,7 +661,7 @@ lflow_table_add_lflow(struct lflow_table *lflow_table,
const char *match, const char *actions,
const char *io_port, const char *ctrl_meter,
const struct ovsdb_idl_row *stage_hint,
const char *where,
const char *where, const char *flow_desc,
struct lflow_ref *lflow_ref)
OVS_EXCLUDED(fake_hash_mutex)
{
Expand All @@ -679,7 +681,7 @@ lflow_table_add_lflow(struct lflow_table *lflow_table,
do_ovn_lflow_add(lflow_table,
od ? ods_size(od->datapaths) : dp_bitmap_len,
hash, stage, priority, match, actions,
io_port, ctrl_meter, stage_hint, where);
io_port, ctrl_meter, stage_hint, where, flow_desc);

if (lflow_ref) {
struct lflow_ref_node *lrn =
Expand Down Expand Up @@ -733,7 +735,7 @@ lflow_table_add_lflow_default_drop(struct lflow_table *lflow_table,
{
lflow_table_add_lflow(lflow_table, od, NULL, 0, stage, 0, "1",
debug_drop_action(), NULL, NULL, NULL,
where, lflow_ref);
where, NULL, lflow_ref);
}

struct ovn_dp_group *
Expand Down Expand Up @@ -856,7 +858,8 @@ static void
ovn_lflow_init(struct ovn_lflow *lflow, struct ovn_datapath *od,
size_t dp_bitmap_len, enum ovn_stage stage, uint16_t priority,
char *match, char *actions, char *io_port, char *ctrl_meter,
char *stage_hint, const char *where)
char *stage_hint, const char *where,
const char *flow_desc)
{
lflow->dpg_bitmap = bitmap_allocate(dp_bitmap_len);
lflow->od = od;
Expand All @@ -867,6 +870,7 @@ ovn_lflow_init(struct ovn_lflow *lflow, struct ovn_datapath *od,
lflow->io_port = io_port;
lflow->stage_hint = stage_hint;
lflow->ctrl_meter = ctrl_meter;
lflow->flow_desc = flow_desc;
lflow->dpg = NULL;
lflow->where = where;
lflow->sb_uuid = UUID_ZERO;
Expand Down Expand Up @@ -960,7 +964,7 @@ do_ovn_lflow_add(struct lflow_table *lflow_table, size_t dp_bitmap_len,
const char *match, const char *actions,
const char *io_port, const char *ctrl_meter,
const struct ovsdb_idl_row *stage_hint,
const char *where)
const char *where, const char *flow_desc)
OVS_REQUIRES(fake_hash_mutex)
{
struct ovn_lflow *old_lflow;
Expand All @@ -982,7 +986,8 @@ do_ovn_lflow_add(struct lflow_table *lflow_table, size_t dp_bitmap_len,
xstrdup(match), xstrdup(actions),
io_port ? xstrdup(io_port) : NULL,
nullable_xstrdup(ctrl_meter),
ovn_lflow_hint(stage_hint), where);
ovn_lflow_hint(stage_hint), where,
flow_desc);

if (parallelization_state != STATE_USE_PARALLELIZATION) {
hmap_insert(&lflow_table->entries, &lflow->hmap_node, hash);
Expand Down Expand Up @@ -1050,6 +1055,7 @@ sync_lflow_to_sb(struct ovn_lflow *lflow,
sbrec_logical_flow_set_priority(sbflow, lflow->priority);
sbrec_logical_flow_set_match(sbflow, lflow->match);
sbrec_logical_flow_set_actions(sbflow, lflow->actions);
sbrec_logical_flow_set_flow_desc(sbflow, lflow->flow_desc);
if (lflow->io_port) {
struct smap tags = SMAP_INITIALIZER(&tags);
smap_add(&tags, "in_out_port", lflow->io_port);
Expand Down
27 changes: 21 additions & 6 deletions northd/lflow-mgr.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,8 @@ void lflow_table_add_lflow(struct lflow_table *, const struct ovn_datapath *,
const char *actions, const char *io_port,
const char *ctrl_meter,
const struct ovsdb_idl_row *stage_hint,
const char *where, struct lflow_ref *);
const char *where, const char *flow_desc,
struct lflow_ref *);
void lflow_table_add_lflow_default_drop(struct lflow_table *,
const struct ovn_datapath *,
enum ovn_stage stage,
Expand All @@ -91,20 +92,20 @@ void lflow_table_add_lflow_default_drop(struct lflow_table *,
STAGE_HINT, LFLOW_REF) \
lflow_table_add_lflow(LFLOW_TABLE, OD, NULL, 0, STAGE, PRIORITY, MATCH, \
ACTIONS, IN_OUT_PORT, CTRL_METER, STAGE_HINT, \
OVS_SOURCE_LOCATOR, LFLOW_REF)
OVS_SOURCE_LOCATOR, NULL, LFLOW_REF)

#define ovn_lflow_add_with_hint(LFLOW_TABLE, OD, STAGE, PRIORITY, MATCH, \
ACTIONS, STAGE_HINT, LFLOW_REF) \
lflow_table_add_lflow(LFLOW_TABLE, OD, NULL, 0, STAGE, PRIORITY, MATCH, \
ACTIONS, NULL, NULL, STAGE_HINT, \
OVS_SOURCE_LOCATOR, LFLOW_REF)
OVS_SOURCE_LOCATOR, NULL, LFLOW_REF)

#define ovn_lflow_add_with_dp_group(LFLOW_TABLE, DP_BITMAP, DP_BITMAP_LEN, \
STAGE, PRIORITY, MATCH, ACTIONS, \
STAGE_HINT, LFLOW_REF) \
lflow_table_add_lflow(LFLOW_TABLE, NULL, DP_BITMAP, DP_BITMAP_LEN, STAGE, \
PRIORITY, MATCH, ACTIONS, NULL, NULL, STAGE_HINT, \
OVS_SOURCE_LOCATOR, LFLOW_REF)
OVS_SOURCE_LOCATOR, NULL, LFLOW_REF)

#define ovn_lflow_add_default_drop(LFLOW_TABLE, OD, STAGE, LFLOW_REF) \
lflow_table_add_lflow_default_drop(LFLOW_TABLE, OD, STAGE, \
Expand All @@ -126,13 +127,27 @@ void lflow_table_add_lflow_default_drop(struct lflow_table *,
STAGE_HINT, LFLOW_REF) \
lflow_table_add_lflow(LFLOW_TABLE, OD, NULL, 0, STAGE, PRIORITY, MATCH, \
ACTIONS, IN_OUT_PORT, NULL, STAGE_HINT, \
OVS_SOURCE_LOCATOR, LFLOW_REF)
OVS_SOURCE_LOCATOR, NULL, LFLOW_REF)

#define ovn_lflow_add(LFLOW_TABLE, OD, STAGE, PRIORITY, MATCH, ACTIONS, \
LFLOW_REF) \
lflow_table_add_lflow(LFLOW_TABLE, OD, NULL, 0, STAGE, PRIORITY, MATCH, \
ACTIONS, NULL, NULL, NULL, OVS_SOURCE_LOCATOR, \
LFLOW_REF)
NULL, LFLOW_REF)

#define ovn_lflow_add_drop_with_desc(LFLOW_TABLE, OD, STAGE, PRIORITY, MATCH, \
DESCRIPTION, LFLOW_REF) \
lflow_table_add_lflow(LFLOW_TABLE, OD, NULL, 0, STAGE, PRIORITY, MATCH, \
debug_drop_action(), NULL, NULL, NULL, \
OVS_SOURCE_LOCATOR, DESCRIPTION, LFLOW_REF)

#define ovn_lflow_add_drop_with_lport_hint_and_desc(LFLOW_TABLE, OD, STAGE, \
PRIORITY, MATCH, \
IN_OUT_PORT, STAGE_HINT, \
DESCRIPTION, LFLOW_REF) \
lflow_table_add_lflow(LFLOW_TABLE, OD, NULL, 0, STAGE, PRIORITY, MATCH, \
debug_drop_action(), IN_OUT_PORT, NULL, STAGE_HINT, \
OVS_SOURCE_LOCATOR, DESCRIPTION, LFLOW_REF)

#define ovn_lflow_metered(LFLOW_TABLE, OD, STAGE, PRIORITY, MATCH, ACTIONS, \
CTRL_METER, LFLOW_REF) \
Expand Down
73 changes: 41 additions & 32 deletions northd/northd.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5980,9 +5980,10 @@ build_lswitch_output_port_sec_od(struct ovn_datapath *od,
REGBIT_PORT_SEC_DROP" = check_out_port_sec(); next;",
lflow_ref);

ovn_lflow_add(lflows, od, S_SWITCH_OUT_APPLY_PORT_SEC, 50,
REGBIT_PORT_SEC_DROP" == 1", debug_drop_action(),
lflow_ref);
ovn_lflow_add_drop_with_desc(
lflows, od, S_SWITCH_OUT_APPLY_PORT_SEC, 50,
REGBIT_PORT_SEC_DROP" == 1",
"Packet does not follow port security rules", lflow_ref);
ovn_lflow_add(lflows, od, S_SWITCH_OUT_APPLY_PORT_SEC, 0,
"1", "output;", lflow_ref);
}
Expand Down Expand Up @@ -9306,10 +9307,11 @@ build_drop_arp_nd_flows_for_unbound_router_ports(struct ovn_port *op,
port->json_key,
op->lsp_addrs[i].ea_s, op->json_key,
rp->lsp_addrs[k].ipv4_addrs[l].addr_s);
ovn_lflow_add_with_lport_and_hint(
ovn_lflow_add_drop_with_lport_hint_and_desc(
lflows, op->od, S_SWITCH_IN_EXTERNAL_PORT, 100,
ds_cstr(&match), debug_drop_action(), port->key,
&op->nbsp->header_, lflow_ref);
ds_cstr(&match), port->key,
&op->nbsp->header_,
"Drop ARP for unknown router ports", lflow_ref);
}
for (size_t l = 0; l < rp->lsp_addrs[k].n_ipv6_addrs; l++) {
ds_clear(&match);
Expand All @@ -9322,10 +9324,11 @@ build_drop_arp_nd_flows_for_unbound_router_ports(struct ovn_port *op,
rp->lsp_addrs[k].ipv6_addrs[l].addr_s,
rp->lsp_addrs[k].ipv6_addrs[l].sn_addr_s,
rp->lsp_addrs[k].ipv6_addrs[l].addr_s);
ovn_lflow_add_with_lport_and_hint(
ovn_lflow_add_drop_with_lport_hint_and_desc(
lflows, op->od, S_SWITCH_IN_EXTERNAL_PORT, 100,
ds_cstr(&match), debug_drop_action(), port->key,
&op->nbsp->header_, lflow_ref);
ds_cstr(&match), port->key,
&op->nbsp->header_, "Drop ND for unbound router ports",
lflow_ref);
}

ds_clear(&match);
Expand All @@ -9336,13 +9339,14 @@ build_drop_arp_nd_flows_for_unbound_router_ports(struct ovn_port *op,
port->json_key,
op->lsp_addrs[i].ea_s, rp->lsp_addrs[k].ea_s,
op->json_key);
ovn_lflow_add_with_lport_and_hint(lflows, op->od,
S_SWITCH_IN_EXTERNAL_PORT,
100, ds_cstr(&match),
debug_drop_action(),
port->key,
&op->nbsp->header_,
lflow_ref);
ovn_lflow_add_drop_with_lport_hint_and_desc(
lflows, op->od,
S_SWITCH_IN_EXTERNAL_PORT,
100, ds_cstr(&match),
port->key,
&op->nbsp->header_,
"Packet does not come from a chassis resident",
lflow_ref);
}
}
}
Expand All @@ -9367,9 +9371,9 @@ build_lswitch_lflows_l2_unknown(struct ovn_datapath *od,
"outport = \""MC_UNKNOWN "\"; output;",
lflow_ref);
} else {
ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_UNKNOWN, 50,
"outport == \"none\"", debug_drop_action(),
lflow_ref);
ovn_lflow_add_drop_with_desc(
lflows, od, S_SWITCH_IN_L2_UNKNOWN, 50, "outport == \"none\"",
"No L2 destination", lflow_ref);
}
ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_UNKNOWN, 0, "1",
"output;", lflow_ref);
Expand Down Expand Up @@ -9403,31 +9407,36 @@ build_lswitch_lflows_admission_control(struct ovn_datapath *od,
ovs_assert(od->nbs);

/* Default action for recirculated ICMP error 'packet too big'. */
ovn_lflow_add(lflows, od, S_SWITCH_IN_CHECK_PORT_SEC, 105,
"((ip4 && icmp4.type == 3 && icmp4.code == 4) ||"
" (ip6 && icmp6.type == 2 && icmp6.code == 0)) &&"
" flags.tunnel_rx == 1", debug_drop_action(), lflow_ref);
ovn_lflow_add_drop_with_desc(
lflows, od, S_SWITCH_IN_CHECK_PORT_SEC, 105,
"((ip4 && icmp4.type == 3 && icmp4.code == 4) ||"
" (ip6 && icmp6.type == 2 && icmp6.code == 0)) &&"
" flags.tunnel_rx == 1", "ICMP: packet too big", lflow_ref);

/* Logical VLANs not supported. */
if (!is_vlan_transparent(od)) {
/* Block logical VLANs. */
ovn_lflow_add(lflows, od, S_SWITCH_IN_CHECK_PORT_SEC, 100,
"vlan.present", debug_drop_action(),
lflow_ref);
ovn_lflow_add_drop_with_desc(
lflows, od, S_SWITCH_IN_CHECK_PORT_SEC,
100, "vlan.present",
"VLANs blocked due to vlan-passthru option",
lflow_ref);
}

/* Broadcast/multicast source address is invalid. */
ovn_lflow_add(lflows, od, S_SWITCH_IN_CHECK_PORT_SEC, 100,
"eth.src[40]", debug_drop_action(),
lflow_ref);
ovn_lflow_add_drop_with_desc(
lflows, od, S_SWITCH_IN_CHECK_PORT_SEC, 100,
"eth.src[40]", "Incoming Broadcast/multicast source"
" address is invalid", lflow_ref);

ovn_lflow_add(lflows, od, S_SWITCH_IN_CHECK_PORT_SEC, 50, "1",
REGBIT_PORT_SEC_DROP" = check_in_port_sec(); next;",
lflow_ref);

ovn_lflow_add(lflows, od, S_SWITCH_IN_APPLY_PORT_SEC, 50,
REGBIT_PORT_SEC_DROP" == 1", debug_drop_action(),
lflow_ref);
ovn_lflow_add_drop_with_desc(
lflows, od, S_SWITCH_IN_APPLY_PORT_SEC, 50,
REGBIT_PORT_SEC_DROP" == 1",
"Broadcast/multicast port security invalid", lflow_ref);

ovn_lflow_add(lflows, od, S_SWITCH_IN_APPLY_PORT_SEC, 0, "1", "next;",
lflow_ref);
Expand Down
8 changes: 5 additions & 3 deletions ovn-sb.ovsschema
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"name": "OVN_Southbound",
"version": "20.34.0",
"cksum": "2786607656 31376",
"version": "20.35.0",
"cksum": "2897301415 31493",
"tables": {
"SB_Global": {
"columns": {
Expand Down Expand Up @@ -116,7 +116,9 @@
"min": 0, "max": 1}},
"external_ids": {
"type": {"key": "string", "value": "string",
"min": 0, "max": "unlimited"}}},
"min": 0, "max": "unlimited"}},
"flow_desc": {"type": {"key": {"type": "string"},
"min": 0, "max": 1}}},
"isRoot": true},
"Logical_DP_Group": {
"columns": {
Expand Down
5 changes: 5 additions & 0 deletions ovn-sb.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2904,6 +2904,11 @@ tcp.flags = RST;
<code>ovn-controller</code>.
</column>

<column name="flow_desc">
Human-readable explanation of the flow, this is optional and used
to provide context for the given flow.
</column>

<column name="external_ids" key="stage-name">
Human-readable name for this flow's stage in the pipeline.
</column>
Expand Down
15 changes: 15 additions & 0 deletions tests/ovn-northd.at
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12416,6 +12416,21 @@ AT_CHECK([grep -e "DHCP_RELAY_" lflows | sed 's/table=../table=??/'], [0], [dnl
AT_CLEANUP
])

OVN_FOR_EACH_NORTHD_NO_HV([
AT_SETUP([check for flow_desc])
ovn_start

check ovn-nbctl -- set NB_Global . options:debug_drop_collector_set="123"
check ovn-nbctl ls-add ls1

check ovn-nbctl --wait=hv sync

flow_desc=$(fetch_column Logical_flow flow_desc match='"outport == \"none\""')
AT_CHECK([test "$flow_desc" != ""])

AT_CLEANUP
])

AT_SETUP([NB_Global and SB_Global incremental processing])

ovn_start
Expand Down

0 comments on commit 436a011

Please sign in to comment.