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
