[ovs-dev] [PATCH] ovn-northd: Avoid duplicate logical flows in SB db

Ben Pfaff blp at ovn.org
Mon Jan 8 18:25:16 UTC 2018


Let's step back and consider the options.  Duplicate flow matches can
happen, either because of bugs at any given level of the code, or
because of user-provided data that can't practically be validated before
it is passed down to the next level.

Consider just the ovn-northd level.  ovn-northd can do string
comparisons to determine whether two flow matches are identical, but
flow matches can be duplicates without being the same string (due to
white space differences, order of clauses, different ways to express the
same condition, implied versus explicit prerequisites, and so on).  You
quickly get into a question of the big-O to determine whether two
Boolean expressions are the same.  Furthermore, ovn-northd doesn't
currently parse flow matches (and it's probably better if it doesn't).

Worse, for correctness, it is not enough to know whether two flow
matches are identical.  Instead, you have to know whether they overlap.
Consider "ip4" versus "ip4 && ip4.src == 1.2.3.4".  These expressions
overlap because they both match ipv4 packets with source address
1.2.3.4; if they have the same priority, then the treatment of the
packet is ambiguous.  Most people would say that, in this case, the more
specific match should "win", but that's not always obvious (what if the
matches were "ip4 && ip4.src == 1.2.3.4" and "ip4 && ip4.dst ==
1.2.3.4"?), OpenFlow says the behavior is unspecified, and OVS doesn't
have predictable behavior in this case.  I believe that determining
whether a group of matches overlap requires superlinear time.

Some of this is a little easier at the ovn-controller level.
ovn-controller converts flow matches into OpenFlow matches, and
duplicates are more likely to be found out at that point.  I say "more
likely" because simple differences like white space, etc. will not
matter.  Some kinds of overlap will be found out too.

So it might be worthwhile to think more about the particular bug, and
determine whether whatever observed bad behavior can be better
suppressed at a lower level.

On Mon, Jan 08, 2018 at 09:43:08AM -0800, Han Zhou wrote:
> If both ACLs have same priority, match, ..., but different actions, it is a
> misconfiguration in NB. What could northd do here besides raising an error
> log?
> 
> Another point, would this type of check increase the difficulty of probably
> future incremental-processing in northd?
> 
> From my point of view, it might be better just keep northd simple, and let
> clients handle the correctness, and let ovn-controller to do the final
> check. In this case, could Neutron maintain multiple sg-rule ids in
> external-ids of the same ACL entry?
> 
> Thanks,
> Han
> 
> On Mon, Jan 8, 2018 at 8:52 AM, Miguel Angel Ajo Pelayo <majopela at redhat.com
> > wrote:
> 
> > 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
> > >
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >


More information about the dev mailing list