On 8/26/25 2:37 PM, Ilya Maximets 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,
> 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.
>
> 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.
>
>>
>> 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.
>
>> 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.
>
I was curious about this part initially too (as I authored 6856adc616a7)
but in the end yes, there is a bug with non-static mac bindings too.
Probably harder to hit. It was missing the "!op->nbrp" case.
>> Signed-off-by: Ales Musil <amu...@redhat.com>
>> ---
>> 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.
>
> 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.
>
Personally I was OK with removing mac bindings for stale datapaths here.
The addition part below will add what's missing. But sure, we could
also update the datapath binding reference in the update part.
> 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:
>
> Fixes: b22684d4e37c ("Add a northbound interface to program MAC_Binding
> table")
>
> Best regards, Ilya Maximets.
>
Regards,
Dumitru
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
