[ovs-dev] [PATCH ovn] ovn-controller: Fix port-group incremental processing.

Han Zhou hzhou at ovn.org
Thu Jun 10 23:52:27 UTC 2021


On Thu, Jun 10, 2021 at 12:12 PM Mark Michelson <mmichels at redhat.com> wrote:
>
> Looks good to me, thanks Han!
>
> Acked-by: Mark Michelson <mmichels at redhat.com>
>

Thanks Mark! I applied it to master, branch-21.06 and 21.03.

> On 6/2/21 3:07 AM, Han Zhou wrote:
> > When SB port-group (per DP) is deleted and added back and the
> > notification to ovn-controller include both operations in a reversed
> > order, the current change handler in ovn-controller would end up
> > deleting the SB port-group in the local cache, which in turn result in
> > missing flows, and even worse it can result in crash if runtime data
> > handler is triggered later because we asserts the SB PG should be
> > equivalent to the local cache.
> >
> > This patch fixes it by always handling deletion for tracked PG changes,
> > just like how we are handling other tracked changes in I-P (e.g.
> > logical_flow). A test case is crafted to cover such scenario.
> >
> > Fixes: 0cfeba6b55 ("ovn-controller: Fix port group conjunction flow
explosion problem.")
> > Signed-off-by: Han Zhou <hzhou at ovn.org>
> > ---
> >   controller/ovn-controller.c |  6 +++++-
> >   tests/ovn.at                | 28 +++++++++++++++++++++++++++-
> >   2 files changed, 32 insertions(+), 2 deletions(-)
> >
> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > index d48ddc7a2..07c6fcfd1 100644
> > --- a/controller/ovn-controller.c
> > +++ b/controller/ovn-controller.c
> > @@ -1556,7 +1556,11 @@ port_groups_update(const struct
sbrec_port_group_table *port_group_table,
> >               expr_const_sets_remove(port_groups_cs_local, pg->name);
> >               port_group_ssets_delete(port_group_ssets, pg->name);
> >               sset_add(deleted, pg->name);
> > -        } else {
> > +        }
> > +    }
> > +
> > +    SBREC_PORT_GROUP_TABLE_FOR_EACH_TRACKED (pg, port_group_table) {
> > +        if (!sbrec_port_group_is_deleted(pg)) {
> >               port_group_ssets_add_or_update(port_group_ssets, pg);
> >               expr_const_sets_add_strings(port_groups_cs_local,
pg->name,
> >                                           (const char *const *)
pg->ports,
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index a21dbadac..7be494644 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -26562,7 +26562,7 @@ AT_CLEANUP
> >   ])
> >
> >   # Tests the efficiency of conjunction flow generation when port
groups are used
> > -# by ACLs. Make sure there is no open flow explosion in table 45 for
an ACL
> > +# by ACLs. Make sure there is no open flow explosion in table 44 for
an ACL
> >   # with self-referencing match condition of a port group:
> >   #
> >   # "outport == @pg1 && ip4.src == $pg1_ip4"
> > @@ -26648,6 +26648,32 @@ ovs-vsctl add-port br-int lsp1-1 -- set
interface lsp1-1 external_ids:iface-id=l
> >   check ovn-nbctl --wait=hv sync
> >   AT_CHECK([test $(ovs-ofctl dump-flows br-int table=44 | grep
conjunction | wc -l) == 24])
> >
> > +# 6. Simulate a SB port-group "del and add" notification to
ovn-controller in the
> > +#    same IDL iteration. ovn-controller should still program the same
flows. In
> > +#    reality it can happen when PG memembership change triggers a SB
port-group
> > +#    deletion and creation with the same SB port-group name, while the
> > +#    notification of the changes can come to ovn-controller in one
shot and the
> > +#    order of the "del" and "add" in the notification is undefined.
This test
> > +#    runs the scenario ten times to make sure the unpleasant order is
tested.
> > +
> > +for i in $(seq 1 10); do
> > +    # Delete and recreate the SB PG with same name and content.
> > +    sb_pg_name=2_pg1 # dp key + NB pg name
> > +    sb_pg_uuid=$(fetch_column port_group _uuid name=$sb_pg_name)
> > +    ports_=$(fetch_column port_group ports name=2_pg1)
> > +    ports=${ports_/ /,}
> > +    AT_CHECK([ovn-sbctl destroy port_group $sb_pg_uuid -- create
port_group name=$sb_pg_name ports=$ports], [0], [ignore])
> > +
> > +    # Unbind and bind lsp0-0 to trigger runtime data change as well.
> > +    ovs-vsctl del-port br-int lsp0-0
> > +    check ovn-nbctl --wait=hv sync
> > +    ovs-vsctl add-port br-int lsp0-0 -- set interface lsp0-0
external_ids:iface-id=lsp0-0
> > +    check ovn-nbctl --wait=hv sync
> > +
> > +    # Finally check flow count is the same as before.
> > +    AT_CHECK([test $(ovs-ofctl dump-flows br-int table=44 | grep
conjunction | wc -l) == 24])
> > +done
> > +
> >   # Make sure all the above was performed with I-P (no recompute)
> >   AT_CHECK([test $(ovn-appctl -t ovn-controller coverage/read-counter
lflow_run) == $lflow_run])
> >
> >
>


More information about the dev mailing list