On Mon, Sep 1, 2025 at 10:24 AM Dumitru Ceara <[email protected]> wrote:
> 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
>
>
Thank you Dumitru,
I have addressed the nit, merged it into main and backported down to 24.03.
Thanks,
Ales
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
