| Message ID | 20240809132559.939657-1-roriorden@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] northd: Clean up SB MAC bindings for deleted ports. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
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 <roriorden@redhat.com> > --- > 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.
On 8/9/24 10:22 AM, Ilya Maximets wrote: > 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? I see. That is simpler. Like this?: if (!nb_smb) { sbrec_static_mac_binding_delete(sb_smb); + continue; + } + struct ovn_port *op = op = ovn_port_find(lr_ports, nb_smb->logical_port); + if (!(op && op->nbrp && op->od && op->od->sb)) { + sbrec_static_mac_binding_delete(sb_smb); } } It builds and passes all checks. I can send a v2.
On 8/9/24 17:35, Rosemarie O'Riorden wrote: > On 8/9/24 10:22 AM, Ilya Maximets wrote: >> 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? > > I see. That is simpler. > Like this?: > > if (!nb_smb) { > sbrec_static_mac_binding_delete(sb_smb); > + continue; > + } > + struct ovn_port *op = op = ovn_port_find(lr_ports, > nb_smb->logical_port); > + if (!(op && op->nbrp && op->od && op->od->sb)) { > + sbrec_static_mac_binding_delete(sb_smb); > } > } > > It builds and passes all checks. I can send a v2. > Yeah. Looks good to me. I'd add an empty line before the port lookup and expand the !(a && b ...) into !a || !b ... . Seems easier to read this way, but up to you. Best regards, Ilya Maximets.
On 8/9/24 11:38 AM, Ilya Maximets wrote: > Yeah. Looks good to me. I'd add an empty line before the port lookup > and expand the !(a && b ...) into !a || !b ... . Seems easier to read > this way, but up to you. Ack.
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); } } diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index f2f42275a..8d0ac6c3c 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -8080,6 +8080,19 @@ wait_row_count Static_MAC_Binding 1 logical_port=lr0-p0 ip=192.168.10.100 mac="0 AT_CLEANUP ]) +OVN_FOR_EACH_NORTHD_NO_HV([ +AT_SETUP([LR NB Static_MAC_Binding garbage collection]) +ovn_start + +check ovn-nbctl lr-add lr -- lrp-add lr lr-lrp 00:00:00:00:00:01 1.1.1.1/24 +check ovn-nbctl static-mac-binding-add lr-lrp 1.1.1.2 00:00:00:00:00:02 +wait_row_count sb:Static_MAC_Binding 1 logical_port=lr-lrp ip=1.1.1.2 mac="00\:00\:00\:00\:00\:02" +check ovn-nbctl lr-del lr +wait_row_count sb:Static_MAC_Binding 0 + +AT_CLEANUP +]) + OVN_FOR_EACH_NORTHD_NO_HV([ AT_SETUP([LR neighbor lookup and learning flows]) ovn_start
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 <roriorden@redhat.com> --- northd/northd.c | 14 ++++++++++++-- tests/ovn-northd.at | 13 +++++++++++++ 2 files changed, 25 insertions(+), 2 deletions(-)