Hi Ales,
Thanks for the fix!
On 8/29/25 4:06 PM, Ales Musil wrote:
> Unfortunately the previous fix wasn't enough, when we call
> "sbrec_static_mac_binding_table_get_for_uuid()" it will still return
> row even if was marked as deleted in the current transaction. This
That's a bit unfortunate in my opinion. It makes it harder to use
get_for_uuid() in the context of incremental processing. Ilya, should
we consider changing the semantics of *_table_get_for_uuid()?
> would cause the Static MAC binding to be removed in one txn and added
> in other one, causing traffic disruptions. To properly handle that
> update the datapath reference instead. This should do the update
> in a single transaction instead.
>
> Fixes: e8a0ade51cc1 ("northd: Remove stale Static MAC Bindings during SB
> datapath re-creation.")
> Suggested-by: Ilya Maximets <[email protected]>
> Signed-off-by: Ales Musil <[email protected]>
> ---
> northd/northd.c | 61 ++++++++++++++++++++-----------------------------
> 1 file changed, 25 insertions(+), 36 deletions(-)
>
> diff --git a/northd/northd.c b/northd/northd.c
> index e0a329a17..3456e2918 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -2867,19 +2867,6 @@ common:
> }
> }
>
> -static bool
> -mac_binding_is_stale(const struct hmap *lr_datapaths,
> - const struct hmap *lr_ports,
> - const struct sbrec_datapath_binding *dp,
> - const char *logical_port)
> -{
> - const struct ovn_datapath *od =
> - ovn_datapath_from_sbrec(NULL, lr_datapaths, dp);
> -
> - return !od || ovn_datapath_is_stale(od) ||
> - !ovn_port_find(lr_ports, logical_port);
> -}
> -
> /* Remove mac_binding entries that refer to logical_ports which are
> * deleted. */
> static void
> @@ -2889,8 +2876,11 @@ cleanup_mac_bindings(
> {
> const struct sbrec_mac_binding *b;
> SBREC_MAC_BINDING_TABLE_FOR_EACH_SAFE (b, sbrec_mac_binding_table) {
> - if (mac_binding_is_stale(lr_datapaths, lr_ports, b->datapath,
> - b->logical_port)) {
> + const struct ovn_datapath *od =
> + ovn_datapath_from_sbrec(NULL, lr_datapaths, b->datapath);
> +
> + if (!od || ovn_datapath_is_stale(od) ||
> + !ovn_port_find(lr_ports, b->logical_port)) {
> sbrec_mac_binding_delete(b);
> }
> }
> @@ -18860,34 +18850,19 @@ build_static_mac_binding_table(
> struct ovsdb_idl_txn *ovnsb_txn,
> const struct nbrec_static_mac_binding_table *nbrec_static_mb_table,
> const struct sbrec_static_mac_binding_table *sbrec_static_mb_table,
> - const struct hmap *lr_datapaths, const struct hmap *lr_ports)
> + const struct hmap *lr_ports)
> {
> - /* Cleanup SB Static_MAC_Binding entries which do not have corresponding
> - * NB Static_MAC_Binding entries, and SB Static_MAC_Binding entries for
> - * which there is not a NB Logical_Router_Port of the same name or
> - * entries which SB Datapath_Binding reference is considered
> - * stale/removed. */
> - const struct nbrec_static_mac_binding *nb_smb;
> + struct hmapx stale = HMAPX_INITIALIZER(&stale);
> const struct sbrec_static_mac_binding *sb_smb;
> SBREC_STATIC_MAC_BINDING_TABLE_FOR_EACH_SAFE (sb_smb,
Nit: this can be SBREC_STATIC_MAC_BINDING_TABLE_FOR_EACH() now.
> sbrec_static_mb_table) {
> - nb_smb = nbrec_static_mac_binding_table_get_for_uuid(
> - nbrec_static_mb_table, &sb_smb->header_.uuid);
> - if (!nb_smb) {
> - sbrec_static_mac_binding_delete(sb_smb);
> - continue;
> - }
> -
> - if (mac_binding_is_stale(lr_datapaths, lr_ports, sb_smb->datapath,
> - sb_smb->logical_port)) {
> - sbrec_static_mac_binding_delete(sb_smb);
> - }
> + hmapx_add(&stale, CONST_CAST(void *, sb_smb));
> }
>
> /* Create/Update SB Static_MAC_Binding entries with corresponding values
> * from NB Static_MAC_Binding entries. */
> - NBREC_STATIC_MAC_BINDING_TABLE_FOR_EACH (
> - nb_smb, nbrec_static_mb_table) {
> + const struct nbrec_static_mac_binding *nb_smb;
> + NBREC_STATIC_MAC_BINDING_TABLE_FOR_EACH (nb_smb, nbrec_static_mb_table) {
> struct ovn_port *op = ovn_port_find(lr_ports, nb_smb->logical_port);
> if (op && op->nbrp) {
> struct ovn_datapath *od = op->od;
> @@ -18909,6 +18884,11 @@ build_static_mac_binding_table(
> sbrec_static_mac_binding_set_datapath(mb,
> od->sdp->sb_dp);
> } else {
> /* Update existing entry if there is a change*/
> + if (mb->datapath != od->sdp->sb_dp) {
> + sbrec_static_mac_binding_set_datapath(
> + mb, od->sdp->sb_dp);
> + }
> +
> if (strcmp(mb->mac, nb_smb->mac)) {
> sbrec_static_mac_binding_set_mac(mb, nb_smb->mac);
> }
> @@ -18927,10 +18907,19 @@ build_static_mac_binding_table(
> sbrec_static_mac_binding_set_override_dynamic_mac(mb,
> nb_smb->override_dynamic_mac);
> }
> +
> + hmapx_find_and_delete(&stale, mb);
> }
> }
> }
> }
> +
> + /* Cleanup SB Static_MAC_Binding entries which are no longer needed. */
> + struct hmapx_node *node;
> + HMAPX_FOR_EACH (node, &stale) {
> + sbrec_static_mac_binding_delete(node->data);
> + }
> + hmapx_destroy(&stale);
> }
>
> static void
> @@ -19209,7 +19198,7 @@ ovnnb_db_run(struct northd_input *input_data,
> build_static_mac_binding_table(ovnsb_txn,
> input_data->nbrec_static_mac_binding_table,
> input_data->sbrec_static_mac_binding_table,
> - &data->lr_datapaths.datapaths, &data->lr_ports);
> + &data->lr_ports);
> stopwatch_stop(BUILD_LFLOWS_CTX_STOPWATCH_NAME, time_msec());
> stopwatch_start(CLEAR_LFLOWS_CTX_STOPWATCH_NAME, time_msec());
>
With the small nit addressed:
Acked-by: Dumitru Ceara <[email protected]>
Regards,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
