[ovs-dev] [PATCH] ovn-northd: Avoid duplicate logical flows in SB db
Daniel Alvarez Sanchez
dalvarez at redhat.com
Tue Jan 9 11:12:14 UTC 2018
Thanks Ben, Han, Miguel.
On Tue, Jan 9, 2018 at 11:59 AM, Miguel Angel Ajo Pelayo <
majopela at redhat.com> wrote:
> 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.)
>
> Yes, your explanation is quite precise and I elaborated a bit
more in the 'annotation' part in the patch.
As Miguel says, we need some way to map OVN resources to Neutron
ones so we're basically using the external_ids field to map an ACL to the
corresponding OpenStack security group rule. As Han suggests, we could
add multiple sg_rule_id's to the external_ids field but that would be
race-condition
prone and will add complexity to the networking-ovn side. Especially when we
eventually want to move over to a model where we have ACLs per SG per
rule and not per SG per rule per port.
I'm fine with your suggestion of addressing it at a lower level in
ovn-controller
and maybe just change the log level from INFO to DEBUG.
I'll post a patch in a while for this.
Thanks again for your valuable comments :)
>
>
>
> On Mon, Jan 8, 2018 at 7:25 PM Ben Pfaff <blp at ovn.org> 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 <
> > 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
> > > >
> >
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
More information about the dev
mailing list