On Fri, Dec 8, 2023 at 5:21 AM Xavier Simonart <[email protected]> wrote:
>
> Hi Numan
>
> Thanks for the review and the comments
>
> On Thu, Dec 7, 2023 at 12:48 AM Numan Siddique <[email protected]> wrote:
>
> > 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) {
> >
> 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/[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 ?
> >
> 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
> > > > [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
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
