On Thu, Oct 8, 2026 at 7:34 PM Mark Michelson <[email protected]> wrote:
> Hi Ales, I have one minor finding below. > Hi Mark, thank you for the review. > > On Thu, Oct 8, 2026 at 9:01 AM Ales Musil via dev > <[email protected]> wrote: > > > > The FDB entries would be cleaned up when the port or datapath > > was removed. However, that wasn't enough as there might have been > > stale entries when the port changed address from "unknown" or would > > be disabled. Make sure we do a proper cleanup for those changes too. > > > > Fixes: 679d3550303a ("northd: Cleanup stale FDB entries.") > > Reported-at: https://redhat.atlassian.net/browse/FDP-4432 > > Signed-off-by: Ales Musil <[email protected]> > > --- > > northd/en-northd.c | 29 ++++++++++++++-------------- > > northd/northd.c | 32 ++++++++++++++++++++----------- > > northd/northd.h | 2 ++ > > tests/ovn-northd.at | 46 +++++++++++++++++++++++++++++++++++++-------- > > 4 files changed, 75 insertions(+), 34 deletions(-) > > > > diff --git a/northd/en-northd.c b/northd/en-northd.c > > index 480dc61ca..c8c57ea5b 100644 > > --- a/northd/en-northd.c > > +++ b/northd/en-northd.c > > @@ -694,32 +694,31 @@ northd_sb_fdb_change_handler(struct engine_node > *node, void *data) > > const struct sbrec_fdb_table *sbrec_fdb_table = > > EN_OVSDB_GET(engine_get_input("SB_fdb", node)); > > > > + struct vector to_remove = > > + VECTOR_EMPTY_INITIALIZER(struct sbrec_fdb_table *); > > The vector actually contains pointers to struct sbrec_fdb, not > sbrec_fdb_table. The code works because pointers are all the same > size, but it's probably best to use the correct expected struct > pointer here. > Indeed, autocompletion failed me :) Fixed in v2. > > + > > /* check if changed rows are stale and delete them */ > > - const struct sbrec_fdb *fdb_e, *fdb_prev_del = NULL; > > + const struct sbrec_fdb *fdb_e; > > SBREC_FDB_TABLE_FOR_EACH_TRACKED (fdb_e, sbrec_fdb_table) { > > if (sbrec_fdb_is_deleted(fdb_e)) { > > continue; > > } > > > > - if (fdb_prev_del) { > > - sbrec_fdb_delete(fdb_prev_del); > > - } > > - > > - fdb_prev_del = fdb_e; > > - struct ovn_datapath *od > > - = ovn_datapath_find_by_key(&nd->ls_datapaths.datapaths, > > - fdb_e->dp_key); > > - if (od) { > > - if (ovn_tnlid_present(&od->port_tnlids, fdb_e->port_key)) { > > - fdb_prev_del = NULL; > > - } > > + struct ovn_datapath *od = > > + ovn_datapath_find_by_key(&nd->ls_datapaths.datapaths, > > + fdb_e->dp_key); > > + if (!od || ovn_datapath_is_stale(od) || > > + !ovn_tnlid_present(&od->fdb_ports_tnlids, fdb_e->port_key)) > { > > + vector_push(&to_remove, &fdb_e); > > } > > } > > > > - if (fdb_prev_del) { > > - sbrec_fdb_delete(fdb_prev_del); > > + VECTOR_FOR_EACH (&to_remove, fdb_e) { > > + sbrec_fdb_delete(fdb_e); > > } > > > > + vector_destroy(&to_remove); > > + > > return EN_HANDLED_UNCHANGED; > > } > > > > diff --git a/northd/northd.c b/northd/northd.c > > index e6a2333b5..7c4cb5a62 100644 > > --- a/northd/northd.c > > +++ b/northd/northd.c > > @@ -617,6 +617,7 @@ ovn_datapath_create(struct hmap *datapaths, const > struct uuid *key, > > od->sdp = sdp; > > od->nbs = nbs; > > od->nbr = nbr; > > + hmap_init(&od->fdb_ports_tnlids); > > hmap_init(&od->port_tnlids); > > od->port_key_hint = 0; > > hmap_insert(datapaths, &od->key_node, uuid_hash(&od->key)); > > @@ -653,6 +654,7 @@ ovn_datapath_destroy(struct ovn_datapath *od) > > /* Don't remove od->list. It is used within build_datapaths() > as a > > * private list and once we've exited that function it is not > safe to > > * use it. */ > > + ovn_destroy_tnlids(&od->fdb_ports_tnlids); > > ovn_destroy_tnlids(&od->port_tnlids); > > destroy_ipam_info(&od->ipam_info); > > vector_destroy(&od->router_ports); > > @@ -1163,6 +1165,7 @@ ovn_port_cleanup(struct ovn_port *port) > > if (port->tunnel_key) { > > ovs_assert(port->od); > > ovn_free_tnlid(&port->od->port_tnlids, port->tunnel_key); > > + ovn_free_tnlid(&port->od->fdb_ports_tnlids, port->tunnel_key); > > port->tunnel_key = 0; > > } > > for (int i = 0; i < port->n_lsp_addrs; i++) { > > @@ -3135,16 +3138,10 @@ cleanup_stale_fdb_entries(const struct > sbrec_fdb_table *sbrec_fdb_table, > > { > > const struct sbrec_fdb *fdb_e; > > SBREC_FDB_TABLE_FOR_EACH_SAFE (fdb_e, sbrec_fdb_table) { > > - bool delete = true; > > - struct ovn_datapath *od > > - = ovn_datapath_find_by_key(ls_datapaths, fdb_e->dp_key); > > - if (od) { > > - if (ovn_tnlid_present(&od->port_tnlids, fdb_e->port_key)) { > > - delete = false; > > - } > > - } > > - > > - if (delete) { > > + struct ovn_datapath *od = > > + ovn_datapath_find_by_key(ls_datapaths, fdb_e->dp_key); > > + if (!od || ovn_datapath_is_stale(od) || > > + !ovn_tnlid_present(&od->fdb_ports_tnlids, fdb_e->port_key)) > { > > sbrec_fdb_delete(fdb_e); > > } > > } > > @@ -4369,6 +4366,12 @@ ovn_port_add_tnlid(struct ovn_port *op, uint32_t > tunnel_key) > > if (tunnel_key > op->od->port_key_hint) { > > op->od->port_key_hint = tunnel_key; > > } > > + > > + /* Track the assigned tunnel_key for enabled LSP > > + * with unknown address. */ > > + if (op->nbsp && lsp_is_enabled(op->nbsp) && op->has_unknown) { > > + ovs_assert(ovn_add_tnlid(&op->od->fdb_ports_tnlids, > tunnel_key)); > > + } > > } > > return added; > > } > > @@ -4421,6 +4424,11 @@ ovn_port_allocate_key(struct ovn_port *op) > > if (!op->tunnel_key) { > > return false; > > } > > + > > + if (op->nbsp && lsp_is_enabled(op->nbsp) && op->has_unknown) { > > + ovs_assert(ovn_add_tnlid(&op->od->fdb_ports_tnlids, > > + op->tunnel_key)); > > + } > > } > > return true; > > } > > @@ -5091,7 +5099,9 @@ ls_handle_lsp_changes(struct ovsdb_idl_txn > *ovnsb_idl_txn, > > } > > add_op_to_northd_tracked_ports(&trk_lsps->updated, op); > > > > - if (old_tunnel_key != op->tunnel_key) { > > + if (old_tunnel_key != op->tunnel_key || > > + !lsp_is_enabled(op->nbsp) || > > + !op->has_unknown) { > > delete_fdb_entries(ni->sbrec_fdb_by_dp_and_port, > > od->tunnel_key, old_tunnel_key); > > } > > diff --git a/northd/northd.h b/northd/northd.h > > index a2b8a0c93..3d1fd2d31 100644 > > --- a/northd/northd.h > > +++ b/northd/northd.h > > @@ -421,6 +421,8 @@ struct ovn_datapath { > > struct vector router_ports; /* Vector of struct ovn_port *. */ > > struct vector switch_ports; /* Vector of struct ovn_port * of > > * type 'switch'. */ > > + struct hmap fdb_ports_tnlids; /* Tunnel keys for enabled LSP with > > + * unknown address. */ > > struct hmap port_tnlids; > > uint32_t port_key_hint; > > > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > > index 6866068fa..ec04d4d67 100644 > > --- a/tests/ovn-northd.at > > +++ b/tests/ovn-northd.at > > @@ -5344,28 +5344,38 @@ AT_SETUP([FDB cleanup]) > > ovn_start > > > > check ovn-nbctl ls-add sw0 > > -check ovn-nbctl lsp-add sw0 sw0-p1 > > -check ovn-nbctl lsp-add sw0 sw0-p2 > > -check ovn-nbctl lsp-add sw0 sw0-p3 > > +check ovn-nbctl lsp-add sw0 sw0-p1 -- lsp-set-addresses sw0-p1 unknown > > +check ovn-nbctl lsp-add sw0 sw0-p2 -- lsp-set-addresses sw0-p2 unknown > > +check ovn-nbctl lsp-add sw0 sw0-p3 -- lsp-set-addresses sw0-p3 unknown \ > > + -- set Logical_Switch_Port sw0-p3 enabled=false > > +check ovn-nbctl lsp-add sw0 sw0-p4 > > > > check ovn-nbctl ls-add sw1 > > -check ovn-nbctl lsp-add sw1 sw1-p1 > > -check ovn-nbctl lsp-add sw1 sw1-p2 > > -check ovn-nbctl --wait=sb lsp-add sw1 sw1-p3 > > +check ovn-nbctl lsp-add sw1 sw1-p1 -- lsp-set-addresses sw1-p1 unknown > > +check ovn-nbctl --wait=sb sync > > > > sw0_key=$(fetch_column datapath_binding tunnel_key > external_ids:name=sw0) > > sw1_key=$(fetch_column datapath_binding tunnel_key > external_ids:name=sw1) > > sw0p1_key=$(fetch_column port_binding tunnel_key logical_port=sw0-p1) > > sw0p2_key=$(fetch_column port_binding tunnel_key logical_port=sw0-p2) > > +sw0p3_key=$(fetch_column port_binding tunnel_key logical_port=sw0-p3) > > +sw0p4_key=$(fetch_column port_binding tunnel_key logical_port=sw0-p4) > > sw1p1_key=$(fetch_column port_binding tunnel_key logical_port=sw1-p1) > > > > check_uuid ovn-sbctl create FDB mac="00\:00\:00\:00\:00\:01" > dp_key=$sw0_key port_key=$sw0p1_key > > check_uuid ovn-sbctl create FDB mac="00\:00\:00\:00\:00\:02" > dp_key=$sw0_key port_key=$sw0p1_key > > check_uuid ovn-sbctl create FDB mac="00\:00\:00\:00\:00\:03" > dp_key=$sw0_key port_key=$sw0p2_key > > +check_uuid ovn-sbctl create FDB mac="00\:00\:00\:00\:00\:04" > dp_key=$sw0_key port_key=$sw0p3_key > > +check_uuid ovn-sbctl create FDB mac="00\:00\:00\:00\:00\:05" > dp_key=$sw0_key port_key=$sw0p4_key > > check_uuid ovn-sbctl create FDB mac="00\:00\:00\:00\:01\:01" > dp_key=$sw1_key port_key=$sw1p1_key > > check_uuid ovn-sbctl create FDB mac="00\:00\:00\:00\:01\:02" > dp_key=$sw1_key port_key=$sw1p1_key > > check_uuid ovn-sbctl create FDB mac="00\:00\:00\:00\:01\:03" > dp_key=$sw1_key port_key=$sw1p1_key > > > > +# Disabled port should clear FDB. > > +wait_row_count FDB 0 dp_key=$sw0_key port_key=$sw0p3_key > > +# Port without "unknown address" should clear FDB. > > +wait_row_count FDB 0 dp_key=$sw0_key port_key=$sw0p4_key > > + > > wait_row_count FDB 6 > > > > AT_CHECK([ovn-sbctl create fdb mac="00\:00\:00\:00\:01\:03" > dp_key=$sw1_key port_key=10], [1], [ignore], [ignore]) > > @@ -5383,12 +5393,32 @@ check ovn-nbctl lsp-del sw0-p1 > > wait_row_count FDB 1 > > > > check_column '00:00:00:00:00:03' FDB mac > > -ovn-sbctl list fdb > > +ovn-sbctl list FDB > > > > check_column $sw0_key FDB dp_key > > check_column $sw0p2_key FDB port_key > > > > -check ovn-nbctl --wait=sb lsp-add sw0 sw0-p1 > > +check ovn-nbctl --wait=sb lsp-add sw0 sw0-p1 -- lsp-set-addresses > sw0-p1 unknown > > +sw0p1_key=$(fetch_column port_binding tunnel_key logical_port=sw0-p1) > > +wait_row_count FDB 1 > > + > > +check_uuid ovn-sbctl create FDB mac="00\:00\:00\:00\:00\:01" > dp_key=$sw0_key port_key=$sw0p1_key > > +check_uuid ovn-sbctl create FDB mac="00\:00\:00\:00\:00\:02" > dp_key=$sw0_key port_key=$sw0p1_key > > +wait_row_count FDB 3 > > + > > +# Disabling clears FDB entries. > > +check ovn-nbctl --wait=sb set Logical_Switch_Port sw0-p1 enabled=false > > +check_uuid ovn-sbctl create FDB mac="00\:00\:00\:00\:00\:01" > dp_key=$sw0_key port_key=$sw0p1_key > > +check_uuid ovn-sbctl create FDB mac="00\:00\:00\:00\:00\:02" > dp_key=$sw0_key port_key=$sw0p1_key > > +wait_row_count FDB 1 > > + > > +check ovn-nbctl --wait=sb set Logical_Switch_Port sw0-p1 enabled=true > > +check_uuid ovn-sbctl create FDB mac="00\:00\:00\:00\:00\:01" > dp_key=$sw0_key port_key=$sw0p1_key > > +check_uuid ovn-sbctl create FDB mac="00\:00\:00\:00\:00\:02" > dp_key=$sw0_key port_key=$sw0p1_key > > +wait_row_count FDB 3 > > + > > +# Removing unknown clears FDB entries. > > +check ovn-nbctl --wait=sb lsp-set-addresses sw0-p1 "00:00:00:00:00:10 > 192.168.100.10" > > wait_row_count FDB 1 > > > > check ovn-nbctl lsp-del sw0-p2 > > -- > > 2.55.0 > > > > _______________________________________________ > > dev mailing list > > [email protected] > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > Regards, Ales _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
