On Tue, Aug 26, 2025 at 2:37 PM Ilya Maximets <[email protected]> wrote:
> On 8/26/25 12:38 PM, Ales Musil wrote:
> > The SB MAC_Binding and Static_MAC_Binding use the same columns
> > as an indication of logical port and datapath. However, the check
> > if any of those rows is stale wasn't consistent. Unify it in a single
> > function to make sure that both tables have the same condition to
> > identify stale entries.
>
> Hi, Ales. Thanks for the fix!
>
Hi Ilya,
thank you for the review.
>
> The commit message is focusing on entirely wrong thing. Reading it
> it sounds like just a refactoring patch, which is not the case.
> Please, describe what the actual issue is and which condition is
> causing it.
>
It's actually focusing on the right thing. I can add more details
on the scenario when that happens, but it's still unification of
the logic.
>
> I'm also not sure if we need to combine the checks for both types
> of mac bindings as it will complicate the code and make it more
> error prone, see below.
>
I disagree, the issue is caused by a difference between how we remove
Static MAC Bindings and MAC Bindings. There shouldn't be any difference
whatsoever.
>
> >
> > Fixes: 417f1415b2f5 ("northd: Clean up SB MAC bindings for deleted
> ports.")
>
> I don't think this commit should be in the Fixes tag, it achieved
> what it meant to fix, i.e. it solved the case of router removal.
> This patch is aiming to solve the router re-creation, which is,
> though related, a different use case.
>
Again I don't agree, the fix solved just part of the problem.
>
> > Fixes: 6856adc616a7 ("ovn-northd: Clear SB records depending on stale
> datapaths.")
>
> Is there a bug in handling of non-static MAC bindings? If not, then
> this tag should also not be here.
>
As Dumitru pointed out there was a missing check for !op->nbrp
unlikely to happen, but still missing.
>
> > Signed-off-by: Ales Musil <[email protected]>
> > ---
> > northd/northd.c | 35 ++++++++++++++++++++++++++---------
> > tests/ovn-northd.at | 8 ++++++++
> > 2 files changed, 34 insertions(+), 9 deletions(-)
> >
> > diff --git a/northd/northd.c b/northd/northd.c
> > index 015f30a35..f9f132386 100644
> > --- a/northd/northd.c
> > +++ b/northd/northd.c
> > @@ -2867,6 +2867,26 @@ 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);
> > + if (!od || ovn_datapath_is_stale(od)) {
> > + return true;
> > + }
> > +
> > + const struct ovn_port *op = ovn_port_find(lr_ports, logical_port);
> > + if (!op || !op->nbrp) {
> > + return true;
> > + }
> > +
> > + return false;
> > +}
> > +
> > /* Remove mac_binding entries that refer to logical_ports which are
> > * deleted. */
> > static void
> > @@ -2876,11 +2896,8 @@ cleanup_mac_bindings(
> > {
> > const struct sbrec_mac_binding *b;
> > SBREC_MAC_BINDING_TABLE_FOR_EACH_SAFE (b, sbrec_mac_binding_table) {
> > - 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)) {
> > + if (mac_binding_is_stale(lr_datapaths, lr_ports, b->datapath,
> > + b->logical_port)) {
> > sbrec_mac_binding_delete(b);
> > }
> > }
> > @@ -18850,7 +18867,7 @@ 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,
> > - struct hmap *lr_ports)
> > + const struct hmap *lr_datapaths, 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
> > @@ -18866,8 +18883,8 @@ build_static_mac_binding_table(
> > continue;
> > }
> >
> > - struct ovn_port *op = ovn_port_find(lr_ports,
> nb_smb->logical_port);
> > - if (!op || !op->nbrp || !op->od || !op->od->sdp->sb_dp) {
> > + if (mac_binding_is_stale(lr_datapaths, lr_ports,
> sb_smb->datapath,
> > + sb_smb->logical_port)) {
>
> I don't think we should do that. The reason the code was written this
> way is so we can easily compare the conditions for creation below with
> the conditions for removal. Hiding the conditions and also changing
> them here creates a discrepancy in the logic between creation and the
> removal, which may cause similar issues in the future again.
>
Again as said above the logic for removal of static and dynamic
MBs shouldn't be different.
>
> The actual problem is not with the removal, but with the update below.
> The update logic doesn't consider that the datapath can change, but it
> can, because we search only by the names and the port and ip and the
> datapath is not part of the index.
>
But the datapath isn't changed, it is removed and a new one is created.
>
> So, I think, we need to keep the addition and removal in sync as it is
> now and fix the logic for the update to set the new datapath reference
> in case it changed. And the correct Fixes tag should be:
>
This would create more inconsistencies between static and dynamic MBs,
which we shouldn't do.
>
> Fixes: b22684d4e37c ("Add a northbound interface to program MAC_Binding
> table")
>
> Best regards, Ilya Maximets.
>
Thanks,
Ales
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
