On Mon, Nov 20, 2023 at 7:09 AM Ales Musil <[email protected]> wrote:
>
> 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.
>
Hi Xavier,
Thanks for the patch.
If a logical switch has lots of logical ports, we would end up
calling "memcmp" for all of them which could be costly.
But using uuid_equals() should be fine.
I think we can simplify the code a bit since op->nbsp and new_nbsp
pointers would be different if a logical port was deleted
and re-added. I think we can just compare op->nbsp == new_nbsp here.
Does the below diff look good to you ?
---------------------------------------------------------------------------------------------
diff --git a/northd/northd.c b/northd/northd.c
index c043393860..3aa48ab074 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -5180,28 +5180,20 @@ lsp_can_be_inc_processed(const struct
nbrec_logical_switch_port *nbsp)
}
static bool
-ls_port_has_changed(const struct nbrec_logical_switch_port *old,
- const struct nbrec_logical_switch_port *new)
+ls_port_has_changed(const struct nbrec_logical_switch_port *new)
{
- if (old != new) {
- return true;
- }
/* XXX: Need a better OVSDB IDL interface for this check. */
return (nbrec_logical_switch_port_row_get_seqno(new,
OVSDB_IDL_CHANGE_MODIFY) > 0);
}
static struct ovn_port *
-ovn_port_find_in_datapath(struct ovn_datapath *od, const char *name,
- const struct uuid uuid)
+ovn_port_find_in_datapath(struct ovn_datapath *od,
+ const struct nbrec_logical_switch_port *nbsp)
{
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))) {
- continue;
- }
+ HMAP_FOR_EACH_WITH_HASH (op, dp_node, hash_string(nbsp->name, 0),
+ &od->ports) {
+ if (!strcmp(op->key, nbsp->name) && op->nbsp == nbsp) {
return op;
}
}
@@ -5386,8 +5378,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,
- new_nbsp->header_.uuid);
+ op = ovn_port_find_in_datapath(od, new_nbsp);
+
if (!op) {
if (!lsp_can_be_inc_processed(new_nbsp)) {
goto fail;
@@ -5403,7 +5395,7 @@ ls_handle_lsp_changes(struct ovsdb_idl_txn *ovnsb_idl_txn,
}
ovs_list_push_back(&ls_change->added_ports,
&op->list);
- } else if (ls_port_has_changed(op->nbsp, new_nbsp)) {
+ } else if (ls_port_has_changed(new_nbsp)) {
/* Existing port updated */
bool temp = false;
if (lsp_is_type_changed(op->sb, new_nbsp, &temp) ||
---------------------------------------------------------------------------------------------
>
> > + 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)])
> > +}
> > +
I think these functions - sleep_northd and wake_up_northd can be in
tests/ovn-common.at.
This patch
https://patchwork.ozlabs.org/project/ovn/patch/[email protected]/
does that.
You don't have to submit v2 for this. I can take care of it before merging,
Please let me know if the diff I shared above looks good to you ?
Thanks
Numan
> > +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
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
