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

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@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(+)

Acked-By: Miguel Angel Ajo <majopela@redhat.com>


On Fri, Jan 5, 2018 at 11:44 AM Daniel Alvarez <dalvarez@redhat.com> 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@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@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>

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@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@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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@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@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@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>

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@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@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@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@openvswitch.org
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>

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@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@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@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@openvswitch.org
> > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > > _______________________________________________
> > > dev mailing list
> > > dev@openvswitch.org
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >

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 <blp@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@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@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@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@openvswitch.org
> > > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > > > _______________________________________________
> > > > dev mailing list
> > > > dev@openvswitch.org
> > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > > >
> > > _______________________________________________
> > > dev mailing list
> > > dev@openvswitch.org
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > >
>

Thanks Ben, Han, Miguel.


On Tue, Jan 9, 2018 at 11:59 AM, Miguel Angel Ajo Pelayo <
majopela@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@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@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@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@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@openvswitch.org
> > > > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > > > > _______________________________________________
> > > > > dev mailing list
> > > > > dev@openvswitch.org
> > > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > > > >
> > > > _______________________________________________
> > > > dev mailing list
> > > > dev@openvswitch.org
> > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > > >
> >
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>

On Tue, Jan 9, 2018 at 3:12 AM, Daniel Alvarez Sanchez <dalvarez@redhat.com>
wrote:
>
> Thanks Ben, Han, Miguel.
>
>
> On Tue, Jan 9, 2018 at 11:59 AM, Miguel Angel Ajo Pelayo <
> majopela@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.

Agree. Simplicity is preferred. Thanks for the detailed explaining.

On Tue, Jan 09, 2018 at 12:12:14PM +0100, Daniel Alvarez Sanchez wrote:
> Thanks Ben, Han, Miguel.
> 
> 
> On Tue, Jan 9, 2018 at 11:59 AM, Miguel Angel Ajo Pelayo <
> majopela@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.

Please add a link to this thread in the email archive to the commit
message.