On 8/9/24 15:25, Rosemarie O'Riorden wrote:
> Static_MAC_Binding (SMB) records are only deleted from the
> southbound-db via ovn-northd when there is no such SMB found in the
> northbound-db. This leads to problems when a port becomes stale (ex.
> through lr deletion), because the Logical_Router and Logical_Router_Port
> records are removed from the nb-db, but not the Static_MAC_Binding record.
>
> So, when ovn-northd attempts to remove the corresponding
> Datapath_Binding and Port_Binding records from sb-db, it fails because
> of a referential integrity violation: the Static_MAC_Binding record
> contains a reference to Datapath_Binding.
>
> The corrected behavior is that SMB's are now deleted:
> 1. when there's no such SMB in the nb-db, and
> 2. when there is no port in the nb-db with the SMB's associated port name.
> Thus, all three records, Datapath_Binding, Port_Binding, and
> Static_MAC_Binding, are removed within the same transaction.
>
> Also added a unit test to verify correct behavior.
>
> Fixes: b22684d4e37c ("Add a northbound interface to program MAC_Binding
> table")
> Reported-at: https://issues.redhat.com/browse/FDP-723
> Signed-off-by: Rosemarie O'Riorden <[email protected]>
> ---
> northd/northd.c | 14 ++++++++++++--
> tests/ovn-northd.at | 13 +++++++++++++
> 2 files changed, 25 insertions(+), 2 deletions(-)
>
> diff --git a/northd/northd.c b/northd/northd.c
> index 0c73e70df..15c15a4c2 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -18336,7 +18336,8 @@ build_static_mac_binding_table(
> struct hmap *lr_ports)
> {
> /* Cleanup SB Static_MAC_Binding entries which do not have corresponding
> - * NB Static_MAC_Binding entries. */
> + * NB Static_MAC_Binding entries, and SB Static_MAC_Binding entries for
> + * which there is not a NB Port_Binding of the same name. */
> const struct nbrec_static_mac_binding *nb_smb;
> const struct sbrec_static_mac_binding *sb_smb;
> SBREC_STATIC_MAC_BINDING_TABLE_FOR_EACH_SAFE (sb_smb,
> @@ -18344,7 +18345,16 @@ build_static_mac_binding_table(
> nb_smb = static_mac_binding_by_port_ip(nbrec_static_mb_table,
> sb_smb->logical_port,
> sb_smb->ip);
> - if (!nb_smb) {
> + struct ovn_port *op = NULL;
> + struct ovn_datapath *od = NULL;
> + if (nb_smb) {
> + op = ovn_port_find(lr_ports, nb_smb->logical_port);
> + if (op) {
> + od = op->od;
> + }
> + }
> + bool delete_smb = !nb_smb || !op || !op->nbrp || !od || !od->sb;
> + if (delete_smb) {
> sbrec_static_mac_binding_delete(sb_smb);
> }
Thanks, Rosemarie! The code looks correct to me.
But it might be possible to make it simpler by reducing the amount of nested
'if' statements. For example, we could keep the original check as-is adding
the 'continue' after the row deletion and then get the port and perform port
checks afterwards without worrying about nb_smb being NULL, i.e.:
if (!nb_smb) {
sbrec_static_mac_binding_delete(sb_smb);
continue;
}
<Get the port, do other checks and delete the row, if failed>
We also may not need to define any extra variables, beside 'op', in this case.
What do you think?
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
