You're probably right, it's probably not worth increasing the complexity
of ovn-northd when we can check this in the low level, and suppress
the final OpenFlow duplicates, the code for that is already there,
and working.
An alternate an not very intrusive approach would be lowering the
log level of the duplicate flow messages to DEBUG instead of INFO.
Let me recap a bit. We need this because it's the best way
we found on the db-consistency effort for networking-ovn, making
the security group rules a 1:1 with ACLS for ports, which sometimes
will mean that we have a duplicate ACL.
It happens when you have a port attached to several security groups
with equivalent rules:
sg-web:
ingress tcp port 80
ingress tcp port 22
sg-db:
ingress tcp port 1234
ingress tcp port 22
port-A:
security groups: sg-web + sg-db
Until now, we were deduplicating the "ingress tcp 22" acl resulting
of the two groups. But then tracking integrity/consistency of neutron_db
vs ovn-nb explodes in complexity. (dalvarez can probably explain better.)
On Mon, Jan 8, 2018 at 7:25 PM Ben Pfaff <[email protected]> wrote:
> 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 <
> [email protected]
> > > 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 <[email protected]> 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 <[email protected]>
> > > > > ---
> > > > >
> > > > > 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
> > > > > [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
> > >
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
