[ovs-dev] [PATCH] ovn-northd: Avoid duplicate logical flows in SB db
Miguel Angel Ajo Pelayo
majopela at redhat.com
Mon Jan 8 16:52:42 UTC 2018
Right!
We didn't hit that issue, but it'd make sense to fix in this patch I guess.
We could modify the hashing function to not include the action (not sure if
it does now..),
and also have a separate search function that ignores the action.
On Mon, Jan 8, 2018 at 5:39 PM Ben Pfaff <blp at ovn.org> wrote:
> I suspect that this doesn't go far enough, because it includes actions
> in the hash, so that it would fail to deduplicate two identical ACLs
> with different actions (e.g. "drop" versus "allow").
>
> On Fri, Jan 05, 2018 at 10:43:16AM +0000, Daniel Alvarez wrote:
> > When there are two ACLs in a Logical Switch with same direction,
> > priority, match and action fields, ovn-northd will generate the
> > exact same logical flow for them into SB database. This will make
> > ovn-controller log messages (INFO) saying that the duplicate flow
> > is going to be dropped.
> >
> > This patch avoids adding duplicate lflows into SB database so that
> > ovn-controller doesn't have to process them.
> >
> > Signed-off-by: Daniel Alvarez <dalvarez at redhat.com>
> > ---
> >
> > This patch is needed as part of the consistency work we're doing in the
> > OpenStack integration [0]. In our effort to ensure consistency across
> > objects in Neutron and OVN databases we find some special cases like
> > security group rules which match OVN ACLs but not in 1:1 relationship.
> > Until now, two identical security group rules beloning each to a
> > different security group would generate a single ACL in NB database.
> > With this behavior, there's no way to map the ACL in OVN to the
> > corresponding Neutron object.
> >
> > By implementing [0] we're trying to ensure this mapping so we make use
> > of the external_ids column of every table for this purpose. It may happen
> > that we'll have two identical ACLs but each referencing a different
> > Neutron object in their external_ids field. However, this will make
> > ovn-northd to generate two duplicate lflows into SB database which will
> > make ovn-controller drop them when installing the actual flows. With this
> > patch we'll avoid duplicate flows to be inserted in SB database in such
> > cases.
> >
> > [0]
> https://docs.openstack.org/networking-ovn/latest/contributor/design/database_consistency.html
> >
> > ovn/northd/ovn-northd.c | 11 +++++++++++
> > tests/ovn-northd.at | 24 ++++++++++++++++++++++++
> > 2 files changed, 35 insertions(+)
> >
> > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> > index 7e6b1d9..cc64861 100644
> > --- a/ovn/northd/ovn-northd.c
> > +++ b/ovn/northd/ovn-northd.c
> > @@ -428,6 +428,13 @@ struct macam_node {
> > struct eth_addr mac_addr; /* Allocated MAC address. */
> > };
> >
> > +static struct ovn_lflow *ovn_lflow_find(struct hmap *lflows,
> > + struct ovn_datapath *od,
> > + enum ovn_stage stage,
> > + uint16_t priority,
> > + const char *match,
> > + const char *actions);
> > +
> > static void
> > cleanup_macam(struct hmap *macam)
> > {
> > @@ -2298,6 +2305,10 @@ ovn_lflow_add_at(struct hmap *lflow_map, struct
> ovn_datapath *od,
> > const char *stage_hint, const char *where)
> > {
> > ovs_assert(ovn_stage_to_datapath_type(stage) ==
> ovn_datapath_get_type(od));
> > +
> > + if (ovn_lflow_find(lflow_map, od, stage, priority, match, actions))
> {
> > + return;
> > + }
> >
> > struct ovn_lflow *lflow = xmalloc(sizeof *lflow);
> > ovn_lflow_init(lflow, od, stage, priority,
> > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> > index 954e259..ba96c81 100644
> > --- a/tests/ovn-northd.at
> > +++ b/tests/ovn-northd.at
> > @@ -152,3 +152,27 @@ ovn-nbctl lsp-set-options S1-R1 router-port=R1-S1
> > AT_CHECK([test x`ovn-nbctl lsp-get-up S1-R1` = xup])
> >
> > AT_CLEANUP
> > +
> > +AT_SETUP([ovn -- check that duplicate acls don't generate duplicate
> lflows])
> > +AT_SKIP_IF([test $HAVE_PYTHON = no])
> > +ovn_start
> > +
> > +ovn-nbctl ls-add S1
> > +
> > +# Insert a duplicate ACL into NB database.
> > +ovn-nbctl -- --id=@acl create acl direction=to-lport priority=1000 \
> > + match='"tcp.dst == 22"' action=drop -- add logical_switch S1 acl
> @acl
> > +ovn-nbctl -- --id=@acl create acl direction=to-lport priority=1000 \
> > + match='"tcp.dst == 22"' action=drop -- add logical_switch S1 acl
> @acl
> > +
> > +# Check that there are two entries in ACL table in NB database.
> > +AT_CHECK([ovn-nbctl find ACL match='"tcp.dst == 22"' | \
> > +grep _uuid | wc -l], [0], [2
> > +])
> > +
> > +# Now make sure that only one logical flow is added to SB database.
> > +AT_CHECK([ovn-sbctl find Logical_Flow match='"tcp.dst == 22"' | \
> > +grep _uuid | wc -l], [0], [1
> > +])
> > +
> > +AT_CLEANUP
> > --
> > 1.8.3.1
> >
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
More information about the dev
mailing list