| Message ID | 20231102145741.437199-1-xsimonar@redhat.com |
|---|---|
| State | Accepted |
| Delegated to: | Numan Siddique |
| Headers | show |
| Series | [ovs-dev] northd: fix missing port up when deleting and adding back an lsp | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | fail | github build: failed |
On Thu, Nov 2, 2023 at 3:58 PM Xavier Simonart <xsimonar@redhat.com> 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 <xsimonar@redhat.com> > 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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Other than that it looks good. Acked-by: Ales Musil <amusil@redhat.com> Thanks, Ales
On Mon, Nov 20, 2023 at 7:09 AM Ales Musil <amusil@redhat.com> wrote: > > On Thu, Nov 2, 2023 at 3:58 PM Xavier Simonart <xsimonar@redhat.com> 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 <xsimonar@redhat.com> > > > > 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/20231206140155.3439552-1-dceara@redhat.com/ 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 > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > > Other than that it looks good. > > Acked-by: Ales Musil <amusil@redhat.com> > > Thanks, > Ales > > -- > > Ales Musil > > Senior Software Engineer - OVN Core > > Red Hat EMEA <https://www.redhat.com> > > amusil@redhat.com > <https://red.ht/sig> > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Hi Numan Thanks for the review and the comments On Thu, Dec 7, 2023 at 12:48 AM Numan Siddique <numans@ovn.org> wrote: > On Mon, Nov 20, 2023 at 7:09 AM Ales Musil <amusil@redhat.com> wrote: > > > > On Thu, Nov 2, 2023 at 3:58 PM Xavier Simonart <xsimonar@redhat.com> > 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 <xsimonar@redhat.com> > > > > > > > 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) { > Do we still need to compare the name/key ? Is "if (op->nbsp == nbsp) {" not enough ? > 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/20231206140155.3439552-1-dceara@redhat.com/ > 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 ? > One nit above. The diff you proposed looks much better than my initial proposal and ok to me. Thanks Xavier > > 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 > > > dev@openvswitch.org > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > > > > > Other than that it looks good. > > > > Acked-by: Ales Musil <amusil@redhat.com> > > > > Thanks, > > Ales > > > > -- > > > > Ales Musil > > > > Senior Software Engineer - OVN Core > > > > Red Hat EMEA <https://www.redhat.com> > > > > amusil@redhat.com > > <https://red.ht/sig> > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
On Fri, Dec 8, 2023 at 5:21 AM Xavier Simonart <xsimonar@redhat.com> wrote: > > Hi Numan > > Thanks for the review and the comments > > On Thu, Dec 7, 2023 at 12:48 AM Numan Siddique <numans@ovn.org> wrote: > > > On Mon, Nov 20, 2023 at 7:09 AM Ales Musil <amusil@redhat.com> wrote: > > > > > > On Thu, Nov 2, 2023 at 3:58 PM Xavier Simonart <xsimonar@redhat.com> > > 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 <xsimonar@redhat.com> > > > > > > > > > > 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) { > > > Do we still need to compare the name/key ? Is "if (op->nbsp == nbsp) {" > not enough ? Yes we still need to. A user can update the logical port name. i.e ovn-nbctl set logical_switch_port p0 name=foo I applied this patch to the main branch. I guess this needs a backport to branch-23.09 and I'll backport it once all the tests pass. Numan > > > 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/20231206140155.3439552-1-dceara@redhat.com/ > > 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 ? > > > One nit above. > The diff you proposed looks much better than my initial proposal and ok to > me. > > Thanks > Xavier > > > > > 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 > > > > dev@openvswitch.org > > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > > > > > > > > Other than that it looks good. > > > > > > Acked-by: Ales Musil <amusil@redhat.com> > > > > > > Thanks, > > > Ales > > > > > > -- > > > > > > Ales Musil > > > > > > Senior Software Engineer - OVN Core > > > > > > Red Hat EMEA <https://www.redhat.com> > > > > > > amusil@redhat.com > > > <https://red.ht/sig> > > > _______________________________________________ > > > dev mailing list > > > dev@openvswitch.org > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
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))) { + 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 +])
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 <xsimonar@redhat.com> --- northd/northd.c | 17 +++++++++++------ tests/ovn.at | 45 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 6 deletions(-)