On Thu, Nov 2, 2023 at 3:58 PM Xavier Simonart <[email protected]> wrote:
> When a logical switch port was deleted and added back quickly, it could > happen that the lsp was never reported up > > Signed-off-by: Xavier Simonart <[email protected]> > Hi Xavier, I have one small nit below which could be addressed during merge if others agree. --- > northd/northd.c | 17 +++++++++++------ > tests/ovn.at | 45 +++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 56 insertions(+), 6 deletions(-) > > diff --git a/northd/northd.c b/northd/northd.c > index f8b046d83..0bea3bf2c 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -5192,11 +5192,16 @@ ls_port_has_changed(const struct > nbrec_logical_switch_port *old, > } > > static struct ovn_port * > -ovn_port_find_in_datapath(struct ovn_datapath *od, const char *name) > +ovn_port_find_in_datapath(struct ovn_datapath *od, const char *name, > + const struct uuid uuid) > { > struct ovn_port *op; > HMAP_FOR_EACH_WITH_HASH (op, dp_node, hash_string(name, 0), > &od->ports) { > if (!strcmp(op->key, name)) { > + if (op->nbsp && memcmp(&op->nbsp->header_.uuid, &uuid, > + sizeof(uuid))) { > nit: There is uuid_equals() which we should use. > + continue; > + } > return op; > } > } > @@ -5381,8 +5386,8 @@ ls_handle_lsp_changes(struct ovsdb_idl_txn > *ovnsb_idl_txn, > /* Compare the individual ports in the old and new Logical Switches */ > for (size_t j = 0; j < changed_ls->n_ports; ++j) { > struct nbrec_logical_switch_port *new_nbsp = changed_ls->ports[j]; > - op = ovn_port_find_in_datapath(od, new_nbsp->name); > - > + op = ovn_port_find_in_datapath(od, new_nbsp->name, > + new_nbsp->header_.uuid); > if (!op) { > if (!lsp_can_be_inc_processed(new_nbsp)) { > goto fail; > @@ -5671,9 +5676,9 @@ northd_handle_sb_port_binding_changes( > * notification of that transaction, and we can ignore in this > * case. Fallback to recompute otherwise, to avoid dangling > * sb idl pointers and other unexpected behavior. */ > - if (op) { > - VLOG_WARN_RL(&rl, "A port-binding for %s is deleted but > the " > - "LSP still exists.", pb->logical_port); > + if (op && op->sb == pb) { > + VLOG_WARN_RL(&rl, "A port-binding for %s is deleted but " > + "the LSP still exists.", pb->logical_port); > return false; > } > } else { > diff --git a/tests/ovn.at b/tests/ovn.at > index 637d92bed..cfd39a918 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -36957,3 +36957,48 @@ AT_CHECK([grep -c "NXT_CT_FLUSH_ZONE" > hv1/ovs-vswitchd.log], [0], [dnl > OVN_CLEANUP([hv1]) > AT_CLEANUP > ]) > + > +OVN_FOR_EACH_NORTHD_NO_HV([ > +AT_SETUP([port up with slow northd]) > +ovn_start > + > +sleep_northd() { > + echo northd going to sleep > + AT_CHECK([kill -STOP $(cat northd/ovn-northd.pid)]) > +} > + > +wake_up_northd() { > + echo northd going to sleep > + AT_CHECK([kill -CONT $(cat northd/ovn-northd.pid)]) > +} > + > +net_add n1 > +sim_add hv1 > +as hv1 > +ovs-vsctl add-br br-phys > +ovn_attach n1 br-phys 192.168.0.11 > + > +check ovn-nbctl --wait=hv ls-add ls0 > +# Create a pilot port and wait it up to make sure we are ready for the > real > +# tests, so that the counters measured are accurate. > +check ovn-nbctl --wait=hv lsp-add ls0 lsp-pilot -- lsp-set-addresses > lsp-pilot "unknown" > +ovs-vsctl add-port br-int lsp-pilot -- set interface lsp-pilot > external_ids:iface-id=lsp-pilot > +wait_for_ports_up > +check ovn-nbctl --wait=hv sync > + > +check ovn-nbctl --wait=hv lsp-add ls0 lsp0-2 -- lsp-set-addresses lsp0-2 > "aa:aa:aa:00:00:02 192.168.0.12" > +ovs-vsctl add-port br-int lsp0-2 -- set interface lsp0-2 > external_ids:iface-id=lsp0-2 > +wait_for_ports_up > +check ovn-nbctl --wait=hv sync > + > +sleep_northd > +check ovn-nbctl lsp-del lsp0-2 > +check ovn-nbctl lsp-add ls0 lsp0-2 -- lsp-set-addresses lsp0-2 > "aa:aa:aa:00:00:02 192.168.0.12" > +wake_up_northd > + > +check ovn-nbctl --wait=sb sync > +wait_for_ports_up > + > +OVN_CLEANUP([hv1]) > +AT_CLEANUP > +]) > -- > 2.31.1 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Other than that it looks good. Acked-by: Ales Musil <[email protected]> Thanks, Ales -- Ales Musil Senior Software Engineer - OVN Core Red Hat EMEA <https://www.redhat.com> [email protected] <https://red.ht/sig> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
