Hi Numan
Thanks for the review and the comments
On Thu, Dec 7, 2023 at 12:48 AM Numan Siddique <num...@ovn.org> wrote:
> On Mon, Nov 20, 2023 at 7:09 AM Ales Musil <amu...@redhat.com> wrote:
> >
> > On Thu, Nov 2, 2023 at 3:58 PM Xavier Simonart <xsimo...@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 <xsimo...@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-dce...@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
> > > d...@openvswitch.org
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > >
> > >
> > Other than that it looks good.
> >
> > Acked-by: Ales Musil <amu...@redhat.com>
> >
> > Thanks,
> > Ales
> >
> > --
> >
> > Ales Musil
> >
> > Senior Software Engineer - OVN Core
> >
> > Red Hat EMEA <https://www.redhat.com>
> >
> > amu...@redhat.com
> > <https://red.ht/sig>
> > _______________________________________________
> > dev mailing list
> > d...@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
