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
