Hi Ales, I have one minor finding below.
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.
> +
> /* 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
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
